diff mbox

[babeltrace,2/2] CTF writer: Add function to add an integer environment field value

Message ID 20160619025452.22156-2-simon.marchi@polymtl.ca
State Accepted, archived
Headers show

Commit Message

Simon Marchi June 19, 2016, 2:54 a.m. UTC
>From the Python API, it's only possible to set an environment field
value as a string (whatever you pass, it gets stringified).  This patch
adds a function to the CTF writer to allow setting an integer (int64_t)
environment field, an then exposes it in the Python interface.

The Python method Writer.add_environment_field now uses the type of the
passed value to determine which underlying C function to call (string or
integer).  Any other type is rejected.  This causes a behavior change,
since passing an integer value to add_environment_field used to produce
a string version of the value, whereas it will now produce an integer
version.  However, I think it will now behave more closely to the
expectation of a lambda user.

Example:

  w.add_environment_field("foo", 2)

Result before:

  env {
    foo = "2";
  };

Result after:

  env {
    foo = 2;
  };

Signed-off-by: Simon Marchi <simon.marchi at polymtl.ca>
---
 bindings/python/babeltrace/nativebt.i  |  2 ++
 bindings/python/babeltrace/writer.py   | 18 +++++++++++++++---
 formats/ctf/writer/writer.c            | 16 ++++++++++++++++
 include/babeltrace/ctf-writer/writer.h | 17 +++++++++++++++++
 4 files changed, 50 insertions(+), 3 deletions(-)

Comments

Philippe Proulx June 19, 2016, 5:38 a.m. UTC | #1
On Sat, Jun 18, 2016 at 10:54 PM, Simon Marchi <simon.marchi at polymtl.ca> wrote:
>
> From the Python API, it's only possible to set an environment field
> value as a string (whatever you pass, it gets stringified).  This patch
> adds a function to the CTF writer to allow setting an integer (int64_t)
> environment field, an then exposes it in the Python interface.
>
> The Python method Writer.add_environment_field now uses the type of the
> passed value to determine which underlying C function to call (string or
> integer).  Any other type is rejected.  This causes a behavior change,
> since passing an integer value to add_environment_field used to produce
> a string version of the value, whereas it will now produce an integer
> version.  However, I think it will now behave more closely to the
> expectation of a lambda user.

Thanks for this, it's a great little addition.

>
> 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

>
> Result before:
>
>   env {
>     foo = "2";
>   };
>
> Result after:
>
>   env {
>     foo = 2;
>   };
>
> Signed-off-by: Simon Marchi <simon.marchi at polymtl.ca>
> ---
>  bindings/python/babeltrace/nativebt.i  |  2 ++
>  bindings/python/babeltrace/writer.py   | 18 +++++++++++++++---
>  formats/ctf/writer/writer.c            | 16 ++++++++++++++++
>  include/babeltrace/ctf-writer/writer.h | 17 +++++++++++++++++
>  4 files changed, 50 insertions(+), 3 deletions(-)
>
> diff --git a/bindings/python/babeltrace/nativebt.i b/bindings/python/babeltrace/nativebt.i
> index ad20c13..345173f 100644
> --- a/bindings/python/babeltrace/nativebt.i
> +++ b/bindings/python/babeltrace/nativebt.i
> @@ -635,6 +635,7 @@ void bt_ctf_stream_put(struct bt_ctf_stream *stream);
>  %rename("_bt_ctf_writer_create") bt_ctf_writer_create(const char *path);
>  %rename("_bt_ctf_writer_create_stream") bt_ctf_writer_create_stream(struct bt_ctf_writer *writer, struct bt_ctf_stream_class *stream_class);
>  %rename("_bt_ctf_writer_add_environment_field") bt_ctf_writer_add_environment_field(struct bt_ctf_writer *writer, const char *name, const char *value);
> +%rename("_bt_ctf_writer_add_environment_field_int64") bt_ctf_writer_add_environment_field_int64(struct bt_ctf_writer *writer, const char *name, int64_t value);
>  %rename("_bt_ctf_writer_add_clock") bt_ctf_writer_add_clock(struct bt_ctf_writer *writer, struct bt_ctf_clock *clock);
>  %newobject bt_ctf_writer_get_metadata_string;
>  %rename("_bt_ctf_writer_get_metadata_string") bt_ctf_writer_get_metadata_string(struct bt_ctf_writer *writer);
> @@ -646,6 +647,7 @@ void bt_ctf_stream_put(struct bt_ctf_stream *stream);
>  struct bt_ctf_writer *bt_ctf_writer_create(const char *path);
>  struct bt_ctf_stream *bt_ctf_writer_create_stream(struct bt_ctf_writer *writer, struct bt_ctf_stream_class *stream_class);
>  int bt_ctf_writer_add_environment_field(struct bt_ctf_writer *writer, const char *name, const char *value);
> +int bt_ctf_writer_add_environment_field_int64(struct bt_ctf_writer *writer, const char *name, int64_t value);
>  int bt_ctf_writer_add_clock(struct bt_ctf_writer *writer, struct bt_ctf_clock *clock);
>  char *bt_ctf_writer_get_metadata_string(struct bt_ctf_writer *writer);
>  void bt_ctf_writer_flush_metadata(struct bt_ctf_writer *writer);
> diff --git a/bindings/python/babeltrace/writer.py b/bindings/python/babeltrace/writer.py
> index cd609af..8ed18c6 100644
> --- a/bindings/python/babeltrace/writer.py
> +++ b/bindings/python/babeltrace/writer.py
> @@ -2141,11 +2141,23 @@ class Writer:
>          Sets the CTF environment variable named *name* to value *value*
>          (converted to a string).
>
> -        :exc:`ValueError` is raised on error.
> +        :exc:`ValueError` or `TypeError` is raised on error.

Use :exc:`TypeError`.

>          """
>
> -        ret = nbt._bt_ctf_writer_add_environment_field(self._w, str(name),
> -                                                       str(value))
> +        if type(name) != str:

Use `is` operator.

> +            raise TypeError("Field name must be a string.")
> +
> +        t = type(value)

`value_type` might be a better variable name.

> +
> +        if t == str:

Use `is` operator.

> +            ret = nbt._bt_ctf_writer_add_environment_field(self._w, name,
> +                                                           value)
> +        elif t == int:

Use `is` operator.

> +            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.

Thanks again,
Phil

> +        else:
> +            raise TypeError("Value type is not supported.")
>
>          if ret < 0:
>              raise ValueError("Could not add environment field to trace.")
> diff --git a/formats/ctf/writer/writer.c b/formats/ctf/writer/writer.c
> index 6c29493..012b3ef 100644
> --- a/formats/ctf/writer/writer.c
> +++ b/formats/ctf/writer/writer.c
> @@ -209,6 +209,22 @@ end:
>         return ret;
>  }
>
> +int bt_ctf_writer_add_environment_field_int64(struct bt_ctf_writer *writer,
> +               const char *name,
> +               int64_t value)
> +{
> +       int ret = -1;
> +
> +       if (!writer || !name) {
> +               goto end;
> +       }
> +
> +       ret = bt_ctf_trace_set_environment_field_integer(writer->trace, name,
> +               value);
> +end:
> +       return ret;
> +}
> +
>  int bt_ctf_writer_add_clock(struct bt_ctf_writer *writer,
>                 struct bt_ctf_clock *clock)
>  {
> diff --git a/include/babeltrace/ctf-writer/writer.h b/include/babeltrace/ctf-writer/writer.h
> index 940736f..140bc5f 100644
> --- a/include/babeltrace/ctf-writer/writer.h
> +++ b/include/babeltrace/ctf-writer/writer.h
> @@ -96,6 +96,23 @@ extern int bt_ctf_writer_add_environment_field(struct bt_ctf_writer *writer,
>                 const char *value);
>
>  /*
> + * bt_ctf_writer_add_environment_field_int64: add an environment field to the trace.
> + *
> + * Add an environment field to the trace. The name and value parameters are
> + * copied.
> + *
> + * @param writer Writer instance.
> + * @param name Name of the environment field (will be copied).
> + * @param value Value of the environment field.
> + *
> + * Returns 0 on success, a negative value on error.
> + */
> +extern int bt_ctf_writer_add_environment_field_int64(
> +               struct bt_ctf_writer *writer,
> +               const char *name,
> +               int64_t value);
> +
> +/*
>   * bt_ctf_writer_add_clock: add a clock to the trace.
>   *
>   * Add a clock to the trace. Clocks assigned to stream classes must be
> --
> 2.8.3
>
> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
Simon Marchi June 19, 2016, 1:07 p.m. UTC | #2
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?
Philippe Proulx June 19, 2016, 1:21 p.m. UTC | #3
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
diff mbox

Patch

diff --git a/bindings/python/babeltrace/nativebt.i b/bindings/python/babeltrace/nativebt.i
index ad20c13..345173f 100644
--- a/bindings/python/babeltrace/nativebt.i
+++ b/bindings/python/babeltrace/nativebt.i
@@ -635,6 +635,7 @@  void bt_ctf_stream_put(struct bt_ctf_stream *stream);
 %rename("_bt_ctf_writer_create") bt_ctf_writer_create(const char *path);
 %rename("_bt_ctf_writer_create_stream") bt_ctf_writer_create_stream(struct bt_ctf_writer *writer, struct bt_ctf_stream_class *stream_class);
 %rename("_bt_ctf_writer_add_environment_field") bt_ctf_writer_add_environment_field(struct bt_ctf_writer *writer, const char *name, const char *value);
+%rename("_bt_ctf_writer_add_environment_field_int64") bt_ctf_writer_add_environment_field_int64(struct bt_ctf_writer *writer, const char *name, int64_t value);
 %rename("_bt_ctf_writer_add_clock") bt_ctf_writer_add_clock(struct bt_ctf_writer *writer, struct bt_ctf_clock *clock);
 %newobject bt_ctf_writer_get_metadata_string;
 %rename("_bt_ctf_writer_get_metadata_string") bt_ctf_writer_get_metadata_string(struct bt_ctf_writer *writer);
@@ -646,6 +647,7 @@  void bt_ctf_stream_put(struct bt_ctf_stream *stream);
 struct bt_ctf_writer *bt_ctf_writer_create(const char *path);
 struct bt_ctf_stream *bt_ctf_writer_create_stream(struct bt_ctf_writer *writer, struct bt_ctf_stream_class *stream_class);
 int bt_ctf_writer_add_environment_field(struct bt_ctf_writer *writer, const char *name, const char *value);
+int bt_ctf_writer_add_environment_field_int64(struct bt_ctf_writer *writer, const char *name, int64_t value);
 int bt_ctf_writer_add_clock(struct bt_ctf_writer *writer, struct bt_ctf_clock *clock);
 char *bt_ctf_writer_get_metadata_string(struct bt_ctf_writer *writer);
 void bt_ctf_writer_flush_metadata(struct bt_ctf_writer *writer);
diff --git a/bindings/python/babeltrace/writer.py b/bindings/python/babeltrace/writer.py
index cd609af..8ed18c6 100644
--- a/bindings/python/babeltrace/writer.py
+++ b/bindings/python/babeltrace/writer.py
@@ -2141,11 +2141,23 @@  class Writer:
         Sets the CTF environment variable named *name* to value *value*
         (converted to a string).
 
-        :exc:`ValueError` is raised on error.
+        :exc:`ValueError` or `TypeError` is raised on error.
         """
 
-        ret = nbt._bt_ctf_writer_add_environment_field(self._w, str(name),
-                                                       str(value))
+        if type(name) != str:
+            raise TypeError("Field name must be a string.")
+
+        t = type(value)
+
+        if t == str:
+            ret = nbt._bt_ctf_writer_add_environment_field(self._w, name,
+                                                           value)
+        elif t == int:
+            ret = nbt._bt_ctf_writer_add_environment_field_int64(self._w,
+                                                                 name,
+                                                                 value)
+        else:
+            raise TypeError("Value type is not supported.")
 
         if ret < 0:
             raise ValueError("Could not add environment field to trace.")
diff --git a/formats/ctf/writer/writer.c b/formats/ctf/writer/writer.c
index 6c29493..012b3ef 100644
--- a/formats/ctf/writer/writer.c
+++ b/formats/ctf/writer/writer.c
@@ -209,6 +209,22 @@  end:
 	return ret;
 }
 
+int bt_ctf_writer_add_environment_field_int64(struct bt_ctf_writer *writer,
+		const char *name,
+		int64_t value)
+{
+	int ret = -1;
+
+	if (!writer || !name) {
+		goto end;
+	}
+
+	ret = bt_ctf_trace_set_environment_field_integer(writer->trace, name,
+		value);
+end:
+	return ret;
+}
+
 int bt_ctf_writer_add_clock(struct bt_ctf_writer *writer,
 		struct bt_ctf_clock *clock)
 {
diff --git a/include/babeltrace/ctf-writer/writer.h b/include/babeltrace/ctf-writer/writer.h
index 940736f..140bc5f 100644
--- a/include/babeltrace/ctf-writer/writer.h
+++ b/include/babeltrace/ctf-writer/writer.h
@@ -96,6 +96,23 @@  extern int bt_ctf_writer_add_environment_field(struct bt_ctf_writer *writer,
 		const char *value);
 
 /*
+ * bt_ctf_writer_add_environment_field_int64: add an environment field to the trace.
+ *
+ * Add an environment field to the trace. The name and value parameters are
+ * copied.
+ *
+ * @param writer Writer instance.
+ * @param name Name of the environment field (will be copied).
+ * @param value Value of the environment field.
+ *
+ * Returns 0 on success, a negative value on error.
+ */
+extern int bt_ctf_writer_add_environment_field_int64(
+		struct bt_ctf_writer *writer,
+		const char *name,
+		int64_t value);
+
+/*
  * bt_ctf_writer_add_clock: add a clock to the trace.
  *
  * Add a clock to the trace. Clocks assigned to stream classes must be