[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 5/5] tests: qgraph API for the qtest driver fram
From: |
Thomas Huth |
Subject: |
Re: [Qemu-devel] [PATCH 5/5] tests: qgraph API for the qtest driver framework |
Date: |
Fri, 18 Jan 2019 17:39:41 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 |
On 2019-01-15 19:19, Paolo Bonzini wrote:
> From: Emanuele Giuseppe Esposito <address@hidden>
>
> 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 <address@hidden>
> [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 <address@hidden>
> ---
[...]
> +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
- Re: [Qemu-devel] [PATCH 2/5] tests/libqos: rename qpci_init_pc and qpci_init_spapr functions, (continued)