Re: [Qemu-devel] [PATCH 5/5] tests: qgraph API for the qtest driver framework

[Thomas Huth <thuth@redhat.com>]

On 2019-01-15 19:19, Paolo Bonzini wrote:
> From: Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
> 
> Add qgraph API that allows to add/remove nodes and edges from the graph,
> implementation of Depth First Search to discover the paths and basic unit
> test to check correctness of the API.
> Included also a main executable that takes care of starting the framework,
> create the nodes, set the available drivers/machines, discover the path and
> run tests.
> 
> graph.h provides the public API to manage the graph nodes/edges
> graph_extra.h provides a more private API used successively by the gtest integration part
> qos-test.c provides the main executable
> 
> Signed-off-by: Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
> [Paolo's changes compared to the Google Summer of Code submission:
>  * added subprocess to test options
>  * refactored object creation to support live migration tests
>  * removed driver .before callback (unused)
>  * removed test .after callbacks (replaced by GTest destruction queue)]
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
[...]
> +static void add_edge(const char *source, const char *dest,
> +                     QOSEdgeType type, QOSGraphEdgeOptions *opts)
> +{
> +    char *key;
> +    QOSGraphEdgeList *list = g_hash_table_lookup(edge_table, source);
> +
> +    if (!list) {
> +        list = g_new0(QOSGraphEdgeList, 1);
> +        key = g_strdup(source);
> +        g_hash_table_insert(edge_table, key, list);
> +    }
> +
> +    if (!opts) {
> +        opts = &(QOSGraphEdgeOptions) { };

Not sure, but this part looks suspicious to me ... isn't that pointer
going to be invalid as soon as the if-statement scope is over in the
next line?

> +    }
> +
> +    QOSGraphEdge *edge = g_new0(QOSGraphEdge, 1);
> +    edge->type = type;
> +    edge->dest = g_strdup(dest);
> +    edge->edge_name = g_strdup(opts->edge_name ? : dest);

I think I'd write the elvis operator rather as "?:", without the space
inbetween. (or does our checkpatch script dislike that?)

> +    edge->arg = g_memdup(opts->arg, opts->size_arg);
> +
> +    edge->before_cmd_line =
> +        opts->before_cmd_line ? g_strconcat(" ", opts->before_cmd_line, NULL) : NULL;
> +    edge->extra_device_opts =
> +        opts->extra_device_opts ? g_strconcat(",", opts->extra_device_opts, NULL) : NULL;
> +    edge->after_cmd_line =
> +        opts->after_cmd_line ? g_strconcat(" ", opts->after_cmd_line, NULL) : NULL;
> +
> +    QSLIST_INSERT_HEAD(list, edge, edge_list);
> +}
[...]
> +bool qos_graph_has_edge(const char *start, const char *dest)
> +{
> +    QOSGraphEdgeList *list = get_edgelist(start);
> +    QOSGraphEdge *e = search_list_edges(list, dest);
> +    if (e) {
> +        return TRUE;
> +    }
> +    return FALSE;
> +}

I'm surprised that TRUE and FALSE are also still available with capital
letters these days ... The spelling from stdbool.h is lowercase.

Anyway, maybe rather:

    return e != NULL;

?

[...]
> +void qos_add_test(const char *name, const char *interface,
> +                  QOSTestFunc test_func, QOSGraphTestOptions *opts)
> +{
> +    QOSGraphNode *node;
> +    char *test_name = g_strdup_printf("%s-tests/%s", interface, name);;
> +
> +    if (!opts) {
> +        opts = &(QOSGraphTestOptions) { };

Same as above ... is the pointer still valid after the next curly brace?

> +    }
> +    node = create_node(test_name, QNODE_TEST);
> +    node->u.test.function = test_func;
> +    node->u.test.arg = opts->arg;
> +    assert(!opts->edge.arg);
> +    assert(!opts->edge.size_arg);
> +
> +    node->u.test.before = opts->before;
> +    node->u.test.subprocess = opts->subprocess;
> +    node->available = TRUE;
> +    add_edge(interface, test_name, QEDGE_CONSUMED_BY, &opts->edge);
> +    g_free(test_name);
> +}
[...]
> + * There are three types of command line arguments:
> + * - in node      : created from the node name. For example, machines will
> + *                  have "-M <machine>" to its command line, while devices
> + *                  "-device <device>". It is automatically done by the
> + *                   framework.
> + * - after node   : added as additional argument to the node name.
> + *                  This argument is added optionally when creating edges,
> + *                  by setting the parameter @after_cmd_line and
> + *                  @extra_edge_opts in #QOSGraphEdgeOptions.
> + *                  The framework automatically adds
> + *                  a comma before @extra_edge_opts,
> + *                  because it is going to add attibutes

attributes

> + *                  after the destination node pointed by
> + *                  the edge containing these options, and automatically
> + *                  adds a space before @after_cmd_line, because it
> + *                  adds an additional device, not an attribute.
> + * - before node  : added as additional argument to the node name.
> + *                  This argument is added optionally when creating edges,
> + *                  by setting the parameter @before_cmd_line in
> + *                  #QOSGraphEdgeOptions. This attribute
> + *                  is going to add attibutes before the destination node
> + *                  pointed by the edge containing these options. It is
> + *                  helpful to commands that are not node-representable,
> + *                  such as "-fdsev" or "-netdev".
> + *
> + * While adding command line in edges is always used, not all nodes names are
> + * used in every path walk: this is because the contained or produced ones
> + * are already added by QEMU, so only nodes that "consumes" will be used to
> + * build the command line. Also, nodes that will have { "abstract" : true }
> + * as QMP attribute will loose their command line, since they are not proper
> + * devices to be added in QEMU.
[...]
> + * - destructor: Opposite to the node constructor, destroys the object.
> + * This function is called after the test has been executed, and performs
> + * a complete cleanup of each node allocated field. In case no constuctor

constructor

> + * is provided, no destructor will be called.
> + *
> + */
> +struct QOSGraphObject {
> +    /* for produces edges, returns void * */
> +    QOSGetDriver get_driver;
> +    /* for contains edges, returns a QOSGraphObject * */
> +    QOSGetDevice get_device;
> +    /* start the hw, get ready for the test */
> +    QOSStartFunct start_hw;
> +    /* destroy this QOSGraphObject */
> +    QOSDestructorFunc destructor;
> +    /* free the memory associated to the QOSGraphObject and its contained
> +     * children */
> +    GDestroyNotify free;
> +};
[...]
> +static void apply_to_node(const char *name, bool is_machine, bool is_abstract)
> +{
> +    char *machine_name = NULL;
> +    if (is_machine) {
> +        const char *arch = qtest_get_arch();
> +        machine_name = g_strconcat(arch, "/", name, NULL);
> +        name = machine_name;
> +    }
> +    qos_graph_node_set_availability(name, TRUE);

true

> +    if (is_abstract) {
> +        qos_delete_cmd_line(name);
> +    }
> +    g_free(machine_name);
> +}
> +
> +/**
> + * apply_to_qlist(): using QMP queries QEMU for a list of
> + * machines and devices available, and sets the respective node
> + * as TRUE. If a node is found, also all its produced and contained
> + * child are marked available.
> + *
> + * See qos_graph_node_set_availability() for more info
> + */
> +static void apply_to_qlist(QList *list, bool is_machine)
> +{
> +    const QListEntry *p;
> +    const char *name;
> +    bool abstract;
> +    QDict *minfo;
> +    QObject *qobj;
> +    QString *qstr;
> +    QBool *qbool;
> +
> +    for (p = qlist_first(list); p; p = qlist_next(p)) {
> +        minfo = qobject_to(QDict, qlist_entry_obj(p));
> +        qobj = qdict_get(minfo, "name");
> +        qstr = qobject_to(QString, qobj);
> +        name = qstring_get_str(qstr);
> +
> +        qobj = qdict_get(minfo, "abstract");
> +        if (qobj) {
> +            qbool = qobject_to(QBool, qobj);
> +            abstract = qbool_get_bool(qbool);
> +        } else {
> +            abstract = false;
> +        }
> +
> +        apply_to_node(name, is_machine, abstract);
> +        qobj = qdict_get(minfo, "alias");
> +        if (qobj) {
> +            qstr = qobject_to(QString, qobj);
> +            name = qstring_get_str(qstr);
> +            apply_to_node(name, is_machine, abstract);
> +        }
> +    }
> +}
> +
> +/**
> + * qos_set_machines_devices_available(): sets availability of qgraph
> + * machines and devices.
> + *
> + * This function firstly starts QEMU with "-machine none" option,
> + * and then executes the QMP protocol asking for the list of devices
> + * and machines available.
> + *
> + * for each of these items, it looks up the corresponding qgraph node,
> + * setting it as available. The list currently returns all devices that
> + * are either machines or QEDGE_CONSUMED_BY other nodes.
> + * Therefore, in order to mark all other nodes, it recursively sets
> + * all its QEDGE_CONTAINS and QEDGE_PRODUCES child as available too.
> + */
> +static void qos_set_machines_devices_available(void)
> +{
> +    QDict *response;
> +    QDict *args = qdict_new();
> +    QList *list;
> +
> +    qtest_start("-machine none");
> +    response = qmp("{ 'execute': 'query-machines' }");
> +    list = qdict_get_qlist(response, "return");
> +
> +    apply_to_qlist(list, TRUE);

true

> +    qobject_unref(response);
> +
> +    qdict_put_bool(args, "abstract", TRUE);
true

> +    qdict_put_str(args, "implements", "device");
> +
> +    response = qmp("{'execute': 'qom-list-types',"
> +                   " 'arguments': %p }", args);
> +    g_assert(qdict_haskey(response, "return"));
> +    list = qdict_get_qlist(response, "return");
> +
> +    apply_to_qlist(list, FALSE);

false

> +    qtest_end();
> +    qobject_unref(response);
> +

Remove superfluous empty line?

> +}
> +
> +static QGuestAllocator *get_machine_allocator(QOSGraphObject *obj)
> +{
> +    if (obj->get_driver) {
> +        return obj->get_driver(obj, "memory");
> +    } else {
> +        return NULL;
> +    }
> +}

Maybe remove the "else" like this:

   if (obj->get_driver) {
       return obj->get_driver(obj, "memory");
   }
   return NULL;

?

 Thomas