qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Halil Pasic <pasic@linux.ibm.com>
To: Gerd Hoffmann <kraxel@redhat.com>
Cc: Thomas Huth <thuth@redhat.com>,
	David Hildenbrand <david@redhat.com>,
	Cornelia Huck <cohuck@redhat.com>,
	Richard Henderson <richard.henderson@linaro.org>,
	qemu-devel@nongnu.org,
	Christian Borntraeger <borntraeger@de.ibm.com>,
	qemu-s390x@nongnu.org, "Michael S. Tsirkin" <mst@redhat.com>
Subject: Re: [PATCH 0/3] s390x: modularize virtio-gpu-ccw
Date: Mon, 22 Mar 2021 14:12:48 +0100	[thread overview]
Message-ID: <20210322141248.031f7538.pasic@linux.ibm.com> (raw)
In-Reply-To: <20210317095622.2839895-1-kraxel@redhat.com>

On Wed, 17 Mar 2021 10:56:19 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

> Maybe not the most elegant but rather simple approach to the "parent
> object missing" problem: Use a symbol reference to make sure ccw modules
> load only in case ccw support is present.

[..]

Hi Gerd! I've tested this and I think mostly does what it should. The
only thing that I'm inclined to consider a little quirky is the first
message:

$ ./qemu-system-x86_64 -device virtio-gpu-ccw
Failed to open module: /home/pasic/build/qemu/hw-s390x-virtio-gpu-ccw.so: undefined symbol: have_virtio_ccw
qemu-system-x86_64: -device virtio-gpu-ccw: 'virtio-gpu-ccw' is not a valid device model name

But with something like --help we don't see it, and I assume neither do
we when probing, because there the modules are loaded with mayfail. So
for me this is acceptable, because it happens only if one tries to use a
device that ain't advertised.

Compared to Daniels suggestion, I find that one conceptually clearer,
and even more flexible. Implementation-wise I find this patch-set
simpler. I don't know how would it scale to modules depending on modules
(and it feels a little hackish), but we can address such problems as they
emerge if they emerge, so I did not think too hard.

Let me also note, that you took authorship of all three patches, which
I'm fine with. All I really care about at this stage is getting this
fixed in a remotely sane way, and this is definitely one. I'm also
willing to invest more work in that symlink based approach, if that is
what the community wants, but I would prefer this fixed as soon as
possible.

If you keep the authorship for all the patches, I would like to offer:
Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
Tested-by: Halil Pasic <pasic@linux.ibm.com>
for all three patches. (If I'm going to be the author of some of the
patches, those tags don't make sense for those).

Thanks for your work!

Regards,
Halil


  parent reply	other threads:[~2021-03-22 13:14 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-17  9:56 [PATCH 0/3] s390x: modularize virtio-gpu-ccw Gerd Hoffmann
2021-03-17  9:56 ` [PATCH 1/3] s390x: move S390_ADAPTER_SUPPRESSIBLE Gerd Hoffmann
2021-03-17 11:30   ` Philippe Mathieu-Daudé
2021-03-17  9:56 ` [PATCH 2/3] s390x: add have_virtio_ccw Gerd Hoffmann
2021-03-17  9:56 ` [PATCH 3/3] s390x: modularize virtio-gpu-ccw Gerd Hoffmann
2021-03-22 13:12 ` Halil Pasic [this message]
2021-03-26  8:40   ` [PATCH 0/3] " Gerd Hoffmann

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=20210322141248.031f7538.pasic@linux.ibm.com \
    --to=pasic@linux.ibm.com \
    --cc=borntraeger@de.ibm.com \
    --cc=cohuck@redhat.com \
    --cc=david@redhat.com \
    --cc=kraxel@redhat.com \
    --cc=mst@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-s390x@nongnu.org \
    --cc=richard.henderson@linaro.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).