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

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4943 bytes --]

On Mon, Jul 09, 2018 at 11:11:30AM +0200, Emanuele Giuseppe Esposito wrote:
> +/* Graph Edge.*/
> +struct QOSGraphEdge {
> +    QOSEdgeType type;
> +    char *dest;
> +    char *arg; /* just for CONTAIS and CONSUMED_BY */

CONTAINS?

> +/**
> + * remove_node(): removes a node @val from the nodes hash table.
> + * Note that node->name is not free'd since it will represent the
> + * hash table key
> + */
> +static void remove_node(void *val)

This is a confusing function name and doc comment.  It does not remove
the node from a hash table, it just frees it.

Please call this destroy_node().  The g_hash_table_new_full() argument
is GDestroyNotify value_destroy_func, so that would be consistent with
glib naming too.

> +{
> +    QOSGraphNode *node = (QOSGraphNode *) val;

There is no need to cast void pointers in C.  Just "QOSGraphNode *node =
val;" will do.

> +/**
> + * build_driver_cmd_line(): builds the command line for the driver
> + * @node. The node name must be a valid qemu identifier, since it
> + * will be used to build the command line.
> + *
> + * It is also possible to pass an optional @args that will be
> + * concatenated to the command line.
> + *
> + * For drivers, prepend -device to the driver name.
> + */
> +static void build_driver_cmd_line(QOSGraphNode *node, const char *args)

Why is this called "driver" instead of "device"?

> +{
> +    if (args) {
> +        node->command_line = g_strconcat("-device ", node->name, ",",
> +                                          args, NULL);
> +    } else {
> +        node->command_line = g_strconcat("-device ", node->name, NULL);
> +    }
> +}
> +
> +/**
> + * build_test_cmd_line(): builds the command line for the test
> + * @node. The node name need not to be a valid qemu identifier, since it
> + * will not be used to build the command line.
> + *
> + * It is also possible to pass an optional @args that will be
> + * used as additional command line.
> + */
> +static void build_test_cmd_line(QOSGraphNode *node, const char *args)
> +{
> +    if (args) {
> +        node->command_line = g_strdup(args);
> +    } else {
> +        node->command_line = NULL;
> +    }

This is equivalent to:

  node->command_line = g_strdup(args);

https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-strdup

> diff --git a/tests/libqos/qgraph.h b/tests/libqos/qgraph.h
> new file mode 100644
> index 0000000000..54a1786c1e
> --- /dev/null
> +++ b/tests/libqos/qgraph.h
> @@ -0,0 +1,259 @@
> +/*
> + * libqos driver framework

The per-function doc comment are good.  Please also include higher-level
examples of how to use this.  At first glance all these functions are
overwhelming and it's not clear how to implement new tests, drivers,
interfaces, or machines.

See include/qom/object.h for inspiration.

> +/* edge types*/
> +enum QOSEdgeType {
> +    CONTAINS,
> +    PRODUCES,
> +    CONSUMED_BY
> +};
> +
> +/* node types*/
> +enum QOSNodeType {
> +    MACHINE,
> +    DRIVER,
> +    INTERFACE,
> +    TEST
> +};

The QOSEdgeType and QOSNodeType enum constants use commonly-used names
in the global namespace.  Please prefix them since this is a header
file that will be included from many source files.

> +
> +/**
> + * Each driver, test or machine will have this as first field.
> + * Depending on the edge, the node will call the corresponding
> + * function when walking the path.
> + *
> + * QOSGraphObject also provides a destructor, used to deallocate the
> + * after the test has been executed.
> + */
> +struct QOSGraphObject {
> +    /* for produces, returns void * */
> +    QOSGetDriver get_driver;

Unused?

> +    /* for contains, returns a QOSGraphObject * */
> +    QOSGetDevice get_device;

Unused?

> diff --git a/tests/libqos/qgraph_extra.h b/tests/libqos/qgraph_extra.h
> new file mode 100644
> index 0000000000..22850c0368
> --- /dev/null
> +++ b/tests/libqos/qgraph_extra.h
> @@ -0,0 +1,155 @@
> +/*
> + * libqos driver framework
> + *
> + * Copyright (c) 2018 Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License version 2 as published by the Free Software Foundation.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>
> + */
> +
> +#ifndef QGRAPH_EXTRA_H
> +#define QGRAPH_EXTRA_H
> +
> +#include "qgraph.h"
> +#define PRINT_DEBUG 0

I would expect the #ifdef to be in the C file that uses it.  PRINT_DEBUG
is too generic a name for a .h file, it may cause namespace collisions.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]