diff mbox series

[v4,01/12] libqos/qgraph: add qemu_name to QOSGraphNode

Message ID c9a6671914385d0ec6dcd3aee1371d73e09ee33a.1602182956.git.qemu_oss@crudebyte.com (mailing list archive)
State New, archived
Headers show
Series 9pfs: add tests using local fs driver | expand

Commit Message

Christian Schoenebeck Oct. 8, 2020, 6:34 p.m. UTC
Add new member variable 'qemu_name' to struct QOSGraphNode.

This new member may be optionally set in case a different
name for the node (which must always be a unique name) vs.
its actually associated QEMU (QMP) device name is required.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/libqos/qgraph.c          | 1 +
 tests/qtest/libqos/qgraph_internal.h | 1 +
 2 files changed, 2 insertions(+)

Comments

Christian Schoenebeck Oct. 19, 2020, 10:35 a.m. UTC | #1
On Donnerstag, 8. Oktober 2020 20:34:56 CEST Christian Schoenebeck wrote:
> Add new member variable 'qemu_name' to struct QOSGraphNode.
> 
> This new member may be optionally set in case a different
> name for the node (which must always be a unique name) vs.
> its actually associated QEMU (QMP) device name is required.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  tests/qtest/libqos/qgraph.c          | 1 +
>  tests/qtest/libqos/qgraph_internal.h | 1 +
>  2 files changed, 2 insertions(+)

So what shall happen with these libqos patches 1..7? Is that a nack, or 
postpone for now?

Best regards,
Christian Schoenebeck
Thomas Huth Oct. 24, 2020, 6:08 a.m. UTC | #2
On 19/10/2020 12.35, Christian Schoenebeck wrote:
> On Donnerstag, 8. Oktober 2020 20:34:56 CEST Christian Schoenebeck wrote:
>> Add new member variable 'qemu_name' to struct QOSGraphNode.
>>
>> This new member may be optionally set in case a different
>> name for the node (which must always be a unique name) vs.
>> its actually associated QEMU (QMP) device name is required.
>>
>> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
>> ---
>>  tests/qtest/libqos/qgraph.c          | 1 +
>>  tests/qtest/libqos/qgraph_internal.h | 1 +
>>  2 files changed, 2 insertions(+)
> 
> So what shall happen with these libqos patches 1..7? Is that a nack, or 
> postpone for now?

I was hoping to see a review by Paolo or Laurent, who are much more familiar
with qos than I am ... but after having a look at the patches, I think I can
also give some feedback, too:

Patch 1 and 2 sound basically ok to me (should maybe be squashed together,
though), but the qos_node_create_driver_named() function currently seems to
be unused so far? So I'd postpone these two patches to the point in time
when you really need the qos_node_create_driver_named() function.

The other patches are basically fine with me, too, but please avoid the
hard-coded ESC codes that only work with certain terminals.

 Thomas
Christian Schoenebeck Oct. 24, 2020, 10:39 a.m. UTC | #3
On Samstag, 24. Oktober 2020 08:08:59 CEST Thomas Huth wrote:
> On 19/10/2020 12.35, Christian Schoenebeck wrote:
> > On Donnerstag, 8. Oktober 2020 20:34:56 CEST Christian Schoenebeck wrote:
> >> Add new member variable 'qemu_name' to struct QOSGraphNode.
> >> 
> >> This new member may be optionally set in case a different
> >> name for the node (which must always be a unique name) vs.
> >> its actually associated QEMU (QMP) device name is required.
> >> 
> >> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> >> ---
> >> 
> >>  tests/qtest/libqos/qgraph.c          | 1 +
> >>  tests/qtest/libqos/qgraph_internal.h | 1 +
> >>  2 files changed, 2 insertions(+)
> > 
> > So what shall happen with these libqos patches 1..7? Is that a nack, or
> > postpone for now?
> 
> I was hoping to see a review by Paolo or Laurent, who are much more familiar
> with qos than I am ... but after having a look at the patches, I think I
> can also give some feedback, too:
> 
> Patch 1 and 2 sound basically ok to me (should maybe be squashed together,
> though), but the qos_node_create_driver_named() function currently seems to
> be unused so far? So I'd postpone these two patches to the point in time
> when you really need the qos_node_create_driver_named() function.

I did use patches 1 & 2 in v1 of this series, as of v2 and higher I used a
workaround for the actual 9pfs test case patches not to depend on these 2
libqos patches. This happened after Paolo's feedback, who wrote that qos
patches 1 & 2 would be useful for other, future use cases, but argued it
would not be appropriate for my intended use case:

https://lore.kernel.org/qemu-devel/95ef57d0-b35e-f16a-f957-06bc3692cb7c@redhat.com/

I preserved patches 1 & 2 in this series though as he noted they might be
useful for future purposes and applied his requested changes. I personally
probably won't need thise 2 patches any time soon. So it's up to you what
shall happen with them. I don't mind if you postpone or nack them.

> The other patches are basically fine with me, too, but please avoid the
> hard-coded ESC codes that only work with certain terminals.
> 
>  Thomas

I'll respond to that on your patch 4 response.

Best regards,
Christian Schoenebeck
diff mbox series

Patch

diff --git a/tests/qtest/libqos/qgraph.c b/tests/qtest/libqos/qgraph.c
index fc49cfa879..e42f3eaafa 100644
--- a/tests/qtest/libqos/qgraph.c
+++ b/tests/qtest/libqos/qgraph.c
@@ -153,6 +153,7 @@  static QOSGraphNode *create_node(const char *name, QOSNodeType type)
 static void destroy_node(void *val)
 {
     QOSGraphNode *node = val;
+    g_free(node->qemu_name);
     g_free(node->command_line);
     g_free(node);
 }
diff --git a/tests/qtest/libqos/qgraph_internal.h b/tests/qtest/libqos/qgraph_internal.h
index 968fa69450..974985dce9 100644
--- a/tests/qtest/libqos/qgraph_internal.h
+++ b/tests/qtest/libqos/qgraph_internal.h
@@ -56,6 +56,7 @@  struct QOSGraphNode {
     bool available;     /* set by QEMU via QMP, used during graph walk */
     bool visited;       /* used during graph walk */
     char *name;         /* used to identify the node */
+    char *qemu_name;    /* optional: see qos_node_create_driver_named() */
     char *command_line; /* used to start QEMU at test execution */
     union {
         struct {