Re: [Qemu-devel] [RFC PATCH v2 3/4] libqos: implement sdbus QMP driver

[Stefan Hajnoczi]

On Wed, Jan 03, 2018 at 06:49:24PM -0300, Philippe Mathieu-Daudé wrote:
> +static const char *qmp_sdbus_getpath(const char *blkname)
> +{
> +    QDict *response, *minfo;
> +    QList *list;
> +    const QListEntry *le;
> +    QString *qstr;
> +    const char *mname;
> +    QObject *qobj;
> +
> +    response = qmp("{ 'execute': 'query-block' }");

This function implicitly uses global_qtest.  If you ever want to do
multi-VM (e.g. migration) tests then this may become a problem because
there's no way to say which QEMU process to talk to.

This can be addressed by adding a QTestState *s argument to these
functions so that global_qtest isn't hardcoded.  Use qtest_qmp()
instead.

> +    g_assert_nonnull(response);
> +    list = qdict_get_qlist(response, "return");
> +    g_assert_nonnull(list);
> +
> +    QLIST_FOREACH_ENTRY(list, le) {
> +        QDict *response2;
> +
> +        minfo = qobject_to_qdict(qlist_entry_obj(le));
> +        g_assert(minfo);
> +        qobj = qdict_get(minfo, "qdev");
> +        if (!qobj) {
> +            continue;
> +        }
> +        qstr = qobject_to_qstring(qobj);
> +        g_assert(qstr);
> +        mname = qstring_get_str(qstr);
> +
> +        response2 = qmp("{ 'execute': 'qom-get',"
> +                        "  'arguments': { 'path': %s,"
> +                        "   'property': \"parent_bus\"}"
> +                        "}", mname);
> +        g_assert(response2);
> +        g_assert(qdict_haskey(response2, "return"));
> +        qobj = qdict_get(response2, "return");
> +        qstr = qobject_to_qstring(qobj);
> +        g_assert(qstr);
> +        mname = qstring_get_str(qstr);
> +
> +        return mname;
> +    }
> +    return NULL;
> +}

response is leaked.  I think something like this is needed:

  char *mname = g_strdup(qstring_get_str(qstr));
  QDECREF(response);
  return mname;

The caller must g_free() the return value too and the type must be char
* instead of const char *.

> +
> +static ssize_t qmp_mmc_do_cmd(SDBusAdapter *adapter, enum NCmd cmd, uint32_t 
> arg,
> +                              uint8_t **response)
> +{
> +    QMPSDBus *s = (QMPSDBus *)adapter;
> +    QDict *response1;
> +    QObject *qobj;
> +
> +    response1 = qmp("{ 'execute': 'x-debug-sdbus-command',"
> +                    "  'arguments': { 'qom-path': %s,"
> +                    "                 'command': %u, 'arg': %u}"
> +                    "}",
> +                    s->qom_path, cmd, arg);
> +    g_assert(qdict_haskey(response1, "return"));
> +    qobj = qdict_get(response1, "return");
> +    //QDECREF(response);
> +
> +    if (!qobj) {
> +        return -1;
> +    }
> +
> +    {
> +        QString *qstr;
> +        const gchar *mname;
> +        guchar *uc;
> +        gsize out_len;
> +        QDict *response2 = qobject_to_qdict(qobj);
> +
> +        if (!qdict_haskey(response2, "base64")) {
> +            return 0;
> +        }
> +        qobj = qdict_get(response2, "base64");
> +        qstr = qobject_to_qstring(qobj);
> +        if (!qstr) {
> +            puts("!qstr");
> +            return 0;
> +        }
> +        mname = qstring_get_str(qstr);
> +
> +        uc = g_base64_decode(mname, &out_len);
> +        if (response) {
> +            *response = uc;
> +        } else {
> +            g_free(uc);
> +        }
> +        return out_len;
> +
> +    }
> +
> +    return 0;
> +}

Please fix memory leaks here, too.  For example, response1 is leaked.

signature.asc
Description: PGP signature