[lttng-dev] [PATCH babeltrace 2/2] CTF writer: Add function to add an integer environment field value

[Philippe Proulx]

On Sun, Jun 19, 2016 at 9:07 AM, Simon Marchi <simon.marchi at polymtl.ca> wrote:
> On 2016-06-19 01:38, Philippe Proulx wrote:
>>
>> Thanks for this, it's a great little addition.
>
>
> Thanks for the feedback!
>
>>> Example:
>>>
>>>   w.add_environment_field("foo", 2)
>>
>>
>> Not directly related, but an interface like `os.environ` would be nice
>> here
>> I think:
>>
>>     w.env["foo"] = 2
>
>
> Sure, not today though :).
>
>>> -        :exc:`ValueError` is raised on error.
>>> +        :exc:`ValueError` or `TypeError` is raised on error.
>>
>>
>> Use :exc:`TypeError`.
>
>
> Applied locally.
>
>>> +        if type(name) != str:
>>
>>
>> Use `is` operator.
>
>
> Applied locally:
>
>     if type(name) is not str:
>
>>> +        t = type(value)
>>
>>
>> `value_type` might be a better variable name.
>
>
> It doesn't provide much to use a variable in the first place.  I embedded
> type(value) in the two expressions.
>
>>> +        if t == str:
>>
>>
>> Use `is` operator.
>
>
> Done.
>
>>> +        elif t == int:
>>
>>
>> Use `is` operator.
>
>
> Done.
>
>>> +            ret =
>>> nbt._bt_ctf_writer_add_environment_field_int64(self._w,
>>> +                                                                 name,
>>> +                                                                 value)
>>
>>
>> Could you check that `value`'s value is in the range of the int64_t type,
>> so that we catch an illegal operation earlier? Otherwise it's just going
>> to
>> "work" an write the wrong value, I guess.
>
>
> It seems like SWIG does some validation.  Passing an out-of-range value
> throws:
>
>
>     Traceback (most recent call last):
>       File "babeltrace/examples/ctf_writer.py", line 52, in <module>
>         writer.add_environment_field("age_of_the_captain",
> 9223372036854775807+1)
>       File
> "/tmp/babeltrace/lib/python3.5/site-packages/babeltrace/writer.py", line
> 2158, in add_environment_field
>         value)
>     OverflowError: in method '_bt_ctf_writer_add_environment_field_int64',
> argument 3 of type 'int64_t'
>
>
> So I think we are covered, unless you'd like if the error message was more
> precise?


Given this rare (I think) condition, I think that's enough, at least to catch
the overflow in Python. Jérémie will confirm.

Phil