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

[Philippe Proulx]

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