Re: [Qemu-devel] [PATCH v4 21/28] qstring: Add qstring_wrap_str()

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Several spots in the code malloc a string, then wrap it in a
> QString, which has to duplicate the input.  Adding a new
> constructor with transfer semantics makes this pattern more
> efficient, comparable to the just-added transfer semantics to
> go from QString back to raw string.  Use the new
> qstring_wrap_str() where it makes sense.
>
> The new test still passes under valgrind.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v4: new patch
> ---
>  include/qapi/qmp/qstring.h |  1 +
>  block.c                    |  3 ++-
>  block/archipelago.c        |  6 ++----
>  blockdev.c                 |  3 +--
>  qobject/qstring.c          | 25 +++++++++++++++++++++++++
>  tests/check-qstring.c      | 24 ++++++++++++++++++++++++
>  6 files changed, 55 insertions(+), 7 deletions(-)
>
> diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
> index 2d55c87..97cf776 100644
> --- a/include/qapi/qmp/qstring.h
> +++ b/include/qapi/qmp/qstring.h
> @@ -25,6 +25,7 @@ typedef struct QString {
>  QString *qstring_new(void);
>  QString *qstring_from_str(const char *str);
>  QString *qstring_from_substr(const char *str, int start, int end);
> +QString *qstring_wrap_str(char *str);
>  size_t qstring_get_length(const QString *qstring);
>  const char *qstring_get_str(const QString *qstring);
>  char *qstring_consume_str(QString *qstring);
> diff --git a/block.c b/block.c
> index 551832f..32d06b5 100644
> --- a/block.c
> +++ b/block.c
> @@ -1448,7 +1448,8 @@ static int bdrv_append_temp_snapshot(BlockDriverState 
> *bs, int flags,
>      qdict_put(snapshot_options, "file.driver",
>                qstring_from_str("file"));
>      qdict_put(snapshot_options, "file.filename",
> -              qstring_from_str(tmp_filename));
> +              qstring_wrap_str(tmp_filename));
> +    tmp_filename = NULL;
>      qdict_put(snapshot_options, "driver",
>                qstring_from_str("qcow2"));
>

The g_free(tmp_filename) stays, because there are error paths bypassing
this spot.  Good.

> diff --git a/block/archipelago.c b/block/archipelago.c
> index 104f2f9..bfdc14c 100644
> --- a/block/archipelago.c
> +++ b/block/archipelago.c
> @@ -426,13 +426,11 @@ static void archipelago_parse_filename(const char 
> *filename, QDict *options,
>      parse_filename_opts(filename, errp, &volume, &segment_name, &mport, 
> &vport);
>
>      if (volume) {
> -        qdict_put(options, ARCHIPELAGO_OPT_VOLUME, qstring_from_str(volume));
> -        g_free(volume);
> +        qdict_put(options, ARCHIPELAGO_OPT_VOLUME, qstring_wrap_str(volume));
>      }
>      if (segment_name) {
>          qdict_put(options, ARCHIPELAGO_OPT_SEGMENT,
> -                  qstring_from_str(segment_name));
> -        g_free(segment_name);
> +                  qstring_wrap_str(segment_name));
>      }
>      if (mport != NoPort) {
>          qdict_put(options, ARCHIPELAGO_OPT_MPORT, qint_from_int(mport));
> diff --git a/blockdev.c b/blockdev.c
> index adc5058..f293cb3 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1089,8 +1089,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
> BlockInterfaceType block_default_type)
>              new_id = g_strdup_printf("%s%s%i", if_name[type],
>                                       mediastr, unit_id);
>          }
> -        qdict_put(bs_opts, "id", qstring_from_str(new_id));
> -        g_free(new_id);
> +        qdict_put(bs_opts, "id", qstring_wrap_str(new_id));
>      }
>
>      /* Add virtio block device */
> diff --git a/qobject/qstring.c b/qobject/qstring.c
> index 7a438e9..2af1914 100644
> --- a/qobject/qstring.c
> +++ b/qobject/qstring.c
> @@ -66,6 +66,31 @@ QString *qstring_from_str(const char *str)
>      return qstring_from_substr(str, 0, strlen(str) - 1);
>  }
>
> +/**
> + * qstring_wrap_str(): Create a new QString around a malloc'd C string
> + *
> + * Returns a strong reference, and caller must not use @str any more.
> + * @str may be NULL, in which case the QString will be "".

I'm not fond of conflating null pointers and "" (see also the trouble
with visit_type_str()).  For what it's worth, qstring_from_str(NULL)
crashes.  Can we reject null?

> + */
> +QString *qstring_wrap_str(char *str)
> +{
> +    QString *qstring;
> +
> +    qstring = g_malloc(sizeof(*qstring));
> +    qobject_init(QOBJECT(qstring), QTYPE_QSTRING);
> +
> +    if (str) {
> +        qstring->string = str;
> +        qstring->length = strlen(str);
> +    } else {
> +        qstring->string = g_strdup("");
> +        qstring->length = 0;
> +    }
> +    qstring->capacity = qstring->length;
> +
> +    return qstring;
> +}
> +
>  static void capacity_increase(QString *qstring, size_t len)
>  {
>      if (qstring->capacity < (qstring->length + len)) {
> diff --git a/tests/check-qstring.c b/tests/check-qstring.c
> index e6f58e0..0e2e9bd 100644
> --- a/tests/check-qstring.c
> +++ b/tests/check-qstring.c
> @@ -35,6 +35,29 @@ static void qstring_from_str_test(void)
>      QDECREF(qstring);
>  }
>
> +static void qstring_wrap_str_test(void)
> +{
> +    QString *qstring;
> +    char *str = g_strdup("QEMU");
> +
> +    qstring = qstring_wrap_str(str);
> +    g_assert(qstring != NULL);
> +    g_assert(qstring->base.refcnt == 1);
> +    g_assert(strcmp("QEMU", qstring->string) == 0);
> +    g_assert(qobject_type(QOBJECT(qstring)) == QTYPE_QSTRING);
> +
> +    QDECREF(qstring);
> +
> +    str = NULL;
> +    qstring = qstring_wrap_str(str);
> +    g_assert(qstring != NULL);
> +    g_assert(qstring->base.refcnt == 1);
> +    g_assert(strcmp("", qstring->string) == 0);
> +    g_assert(qobject_type(QOBJECT(qstring)) == QTYPE_QSTRING);
> +
> +    QDECREF(qstring);
> +}
> +
>  static void qstring_destroy_test(void)
>  {
>      QString *qstring = qstring_from_str("destroy test");
> @@ -120,6 +143,7 @@ int main(int argc, char **argv)
>      g_test_init(&argc, &argv, NULL);
>
>      g_test_add_func("/public/from_str", qstring_from_str_test);
> +    g_test_add_func("/public/wrap_str", qstring_wrap_str_test);
>      g_test_add_func("/public/destroy", qstring_destroy_test);
>      g_test_add_func("/public/get_str", qstring_get_str_test);
>      g_test_add_func("/public/append_chr", qstring_append_chr_test);