qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Christian Schoenebeck <qemu_oss@crudebyte.com>
To: qemu-devel@nongnu.org
Cc: "Thomas Huth" <thuth@redhat.com>,
	"Laurent Vivier" <lvivier@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Daniel P. Berrangé" <berrange@redhat.com>,
	"Emanuele Giuseppe Esposito" <e.emanuelegiuseppe@gmail.com>,
	"Greg Kurz" <groug@kaod.org>
Subject: Re: [PATCH v4 01/12] libqos/qgraph: add qemu_name to QOSGraphNode
Date: Sat, 24 Oct 2020 12:39:35 +0200	[thread overview]
Message-ID: <1877224.zvXS8ShSZb@silver> (raw)
In-Reply-To: <a3f22a0d-ad06-d227-e9ee-ce0e9c629f96@redhat.com>

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




  reply	other threads:[~2020-10-24 10:40 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-08 18:49 [PATCH v4 00/12] 9pfs: add tests using local fs driver Christian Schoenebeck
2020-10-08 18:34 ` [PATCH v4 05/12] tests/qtest/qos-test: dump qos graph if verbose Christian Schoenebeck
2020-10-24  6:01   ` Thomas Huth
2020-10-24 11:34     ` Christian Schoenebeck
2020-10-08 18:34 ` [PATCH v4 02/12] libqos/qgraph: add qos_node_create_driver_named() Christian Schoenebeck
2020-10-08 18:34 ` [PATCH v4 12/12] tests/9pfs: add local Tmkdir test Christian Schoenebeck
2020-10-08 18:34 ` [PATCH v4 11/12] tests/9pfs: add virtio_9p_test_path() Christian Schoenebeck
2020-10-08 18:34 ` [PATCH v4 08/12] tests/9pfs: change qtest name prefix to synth Christian Schoenebeck
2020-10-14 15:25   ` Christian Schoenebeck
2020-10-14 19:38     ` Greg Kurz
2020-10-15  9:16       ` Christian Schoenebeck
2020-10-08 18:34 ` [PATCH v4 07/12] tests/qtest/qos-test: dump QEMU command if verbose Christian Schoenebeck
2020-10-08 18:34 ` [PATCH v4 03/12] libqos/qgraph_internal: add qos_printf() and qos_printf_literal() Christian Schoenebeck
2020-10-08 18:34 ` [PATCH v4 09/12] tests/9pfs: introduce local tests Christian Schoenebeck
2020-10-08 18:34 ` [PATCH v4 01/12] libqos/qgraph: add qemu_name to QOSGraphNode Christian Schoenebeck
2020-10-19 10:35   ` Christian Schoenebeck
2020-10-24  6:08     ` Thomas Huth
2020-10-24 10:39       ` Christian Schoenebeck [this message]
2020-10-08 18:34 ` [PATCH v4 10/12] tests/9pfs: wipe local 9pfs test directory Christian Schoenebeck
2020-10-08 18:34 ` [PATCH v4 06/12] tests/qtest/qos-test: dump environment variables if verbose Christian Schoenebeck
2020-10-24  5:56   ` Thomas Huth
2020-10-24 10:57     ` Christian Schoenebeck
2020-10-08 18:34 ` [PATCH v4 04/12] libqos/qgraph: add qos_dump_graph() Christian Schoenebeck
2020-10-24  6:04   ` Thomas Huth
2020-10-24 11:24     ` Christian Schoenebeck
2020-10-28  5:51       ` Thomas Huth
2020-10-28 13:00         ` Eric Blake
2020-10-28 13:28           ` Christian Schoenebeck

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1877224.zvXS8ShSZb@silver \
    --to=qemu_oss@crudebyte.com \
    --cc=berrange@redhat.com \
    --cc=e.emanuelegiuseppe@gmail.com \
    --cc=groug@kaod.org \
    --cc=lvivier@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=thuth@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).