Re: [PATCH v5 09/13] osdep: move O_DSYNC and O_DIRECT defines from file-posix
On Thu, May 23, 2024 at 04:55:18PM +0200, Stefano Garzarella wrote:
> These defines are also useful for vhost-user-blk when it is compiled
> in some POSIX systems that do not define them, so let's move them to
> “qemu/osdep.h”.
>
> Suggested-by: Philippe Mathieu-Daudé
> Signed-off-by: Stefano Garzarella
> ---
> include/qemu/osdep.h | 14 ++
> block/file-posix.c | 14 --
> 2 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index f61edcfdc2..e165b5cb1b 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -325,6 +325,20 @@ void QEMU_ERROR("code path is reachable")
> #define ESHUTDOWN 4099
> #endif
>
> +/* OS X does not have O_DSYNC */
> +#ifndef O_DSYNC
> +#ifdef O_SYNC
> +#define O_DSYNC O_SYNC
> +#elif defined(O_FSYNC)
> +#define O_DSYNC O_FSYNC
> +#endif
> +#endif
> +
> +/* Approximate O_DIRECT with O_DSYNC if O_DIRECT isn't available */
> +#ifndef O_DIRECT
> +#define O_DIRECT O_DSYNC
> +#endif
Please don't do this - we can't be confident that all code in
QEMU will be OK with O_DIRECT being simulated in this way.
I'm not convinced that the O_DSYNC simulation is a good idea
to do tree-wide either.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 2/2] iotests: test NBD+TLS+iothread
On Fri, May 17, 2024 at 10:08:08PM -0500, Eric Blake wrote:
> Adding a bit of self-review (in case you want to amend this before
> pushing, instead of waiting for me to get back online),
>
> On Fri, May 17, 2024 at 09:50:15PM GMT, Eric Blake wrote:
> > Prevent regressions when using NBD with TLS in the presence of
> > iothreads, adding coverage the fix to qio channels made in the
> > previous patch.
> >
> > CC: qemu-sta...@nongnu.org
> > Signed-off-by: Eric Blake
> > ---
> > tests/qemu-iotests/tests/nbd-tls-iothread | 170 ++
> > tests/qemu-iotests/tests/nbd-tls-iothread.out | 54 ++
> > 2 files changed, 224 insertions(+)
> > create mode 100755 tests/qemu-iotests/tests/nbd-tls-iothread
> > create mode 100644 tests/qemu-iotests/tests/nbd-tls-iothread.out
> > +
> > +# pick_unused_port
> > +# Copied from nbdkit/tests/functions.sh.in with compatible 2-clause BSD
> > license
>
> I'm not sure if I have to include the license text verbatim in this
> file, and/or have this function moved to a helper utility file. The
> original source file that I borrowed pick_unused_port from has:
>
> # Copyright Red Hat
I checked most of the relevant history, and this Copyright statement
does indeed appear correct - the code was all written by Richard.
Thus, you could invoke Red Hat's right to re-license and just declare
this copy to be under QEMU's normal GPL license, avoiding the question
of copying the license text.
> > +#
> > +# Picks and returns an "unused" port, setting the global variable
> > +# $port.
> > +#
> > +# This is inherently racy, but we need it because qemu does not currently
> > +# permit NBD+TLS over a Unix domain socket
> > +pick_unused_port ()
> > +{
> > +if ! (ss --version) >/dev/null 2>&1; then
> > +_notrun "ss utility required, skipped this test"
> > +fi
> > +
> > +# Start at a random port to make it less likely that two parallel
> > +# tests will conflict.
> > +port=$(( 5 + (RANDOM%15000) ))
> > +while ss -ltn | grep -sqE ":$port\b"; do
> > +((port++))
> > +if [ $port -eq 65000 ]; then port=5; fi
>
> Also, common.nbd only probes:
> for ((port = 10809; port <= 10909; port++))
> and nbdkit's choice of starting with a random offset is interesting.
Yes, a random offset is a nice idea, massively reducing risk of
clashes through (un)lucky concurrent execution.
> > +echo
> > +echo === Cleaning up ===
> > +echo
> > +
> > +_send_qemu_cmd $h1 '{"execute":"quit"}' ''
> > +_send_qemu_cmd $h2 '{"execute":"quit"}' ''
>
> Since the bug was exposed by this point, I didn't bother to do a clean
> shutdown of the mirror job or NBD export. As is, testing that we shut
> down cleanly despite abandoning a job is probably not a bad idea.
Yeah, perhaps worthwhile, if you can get something that works reliably.
A reliable partial test is better than an unreliable full test, as we'll
just end up killing the latter.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH] qio: Inherit follow_coroutine_ctx across TLS
On Wed, May 15, 2024 at 09:14:06PM -0500, Eric Blake wrote:
> Since qemu 8.2, the combination of NBD + TLS + iothread crashes on an
> assertion failure:
>
> qemu-kvm: ../io/channel.c:534: void qio_channel_restart_read(void *):
> Assertion `qemu_get_current_aio_context() ==
> qemu_coroutine_get_aio_context(co)' failed.
>
> It turns out that when we removed AioContext locking, we did so by
> having NBD tell its qio channels that it wanted to opt in to
> qio_channel_set_follow_coroutine_ctx(); but while we opted in on the
> main channel, we did not opt in on the TLS wrapper channel.
> qemu-iotests has coverage of NBD+iothread and NBD+TLS, but apparently
> no coverage of NBD+TLS+iothread, or we would have noticed this
> regression sooner. (I'll add that in the next patch)
>
> But while we could manually opt in to the TLS thread in nbd/server.c,
> it is more generic if all qio channels that wrap other channels
> inherit the follow status, in the same way that they inherit feature
> bits.
>
> CC: Stefan Hajnoczi
> CC: Daniel P. Berrangé
> CC: qemu-sta...@nongnu.org
> Fixes: https://issues.redhat.com/browse/RHEL-34786
> Fixes: 06e0f098 ("io: follow coroutine AioContext in qio_channel_yield()",
> v8.2.0)
> Signed-off-by: Eric Blake
Reviewed-by: Daniel P. Berrangé
> ---
>
> Maybe we should turn ioc->follow_coroutine_ctx into a
> QIO_CHANNEL_FEATURE_* bit?
The features thus far have all been about properties of the underlying
channel, rather than properties related to the API usage pattern. So
I think it is fine to have it separate.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH-for-9.1 v2 2/3] migration: Remove RDMA protocol handling
On Tue, May 07, 2024 at 06:52:50AM +0200, Jinpu Wang wrote:
> Hi Peter, hi Daniel,
> On Mon, May 6, 2024 at 5:29 PM Peter Xu wrote:
> >
> > On Mon, May 06, 2024 at 12:08:43PM +0200, Jinpu Wang wrote:
> > > Hi Peter, hi Daniel,
> >
> > Hi, Jinpu,
> >
> > Thanks for sharing this test results. Sounds like a great news.
> >
> > What's your plan next? Would it then be worthwhile / possible moving QEMU
> > into that direction? Would that greatly simplify rdma code as Dan
> > mentioned?
> I'm rather not familiar with QEMU migration yet, from the test
> result, I think it's a possible direction,
> just we need to at least based on a rather recent release like
> rdma-core v33 with proper 'fork' support.
>
> Maybe Dan or you could give more detail about what you have in mind
> for using rsocket as a replacement for the future.
> We will also look into the implementation details in the meantime.
The migration/socket.c file is the entrypoint for traditional TCP
based migration code. It uses the QIOChannelSocket class which is
written against the traditional sockets APIs, and uses the QAPI
SocketAddress data type to configure it..
My thought was that potentially SocketAddress could be extended to
offer RDMA addressing eg
{ 'union': 'SocketAddress',
'base': { 'type': 'SocketAddressType' },
'discriminator': 'type',
'data': { 'inet': 'InetSocketAddress',
'unix': 'UnixSocketAddress',
'vsock': 'VsockSocketAddress',
'fd': 'FdSocketAddress',
'rdma': 'InetSocketAddress' } }
And then QIOChannelSocket could be also extended to call the
alternative 'rsockets' APIs where needed. That would mean that
existing sockets migration code would almost "just work" with
RDMA. Theoreticaly any other part of QEMU using QIOChannelSocket
would also then magically support RDMA too, with very little (if
any) extra work.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v4 04/12] vhost-user-server: do not set memory fd non-blocking
On Wed, May 08, 2024 at 09:44:48AM +0200, Stefano Garzarella wrote: > In vhost-user-server we set all fd received from the other peer > in non-blocking mode. For some of them (e.g. memfd, shm_open, etc.) > it's not really needed, because we don't use these fd with blocking > operations, but only to map memory. > > In addition, in some systems this operation can fail (e.g. in macOS > setting an fd returned by shm_open() non-blocking fails with errno > = ENOTTY). > > So, let's avoid setting fd non-blocking for those messages that we > know carry memory fd (e.g. VHOST_USER_ADD_MEM_REG, > VHOST_USER_SET_MEM_TABLE). > > Signed-off-by: Stefano Garzarella > --- > v3: > - avoiding setting fd non-blocking for messages where we have memory fd > (Eric) > --- > util/vhost-user-server.c | 12 ++++++++ > 1 file changed, 12 insertions(+) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v4 02/12] libvhost-user: fail vu_message_write() if sendmsg() is failing
On Wed, May 08, 2024 at 09:44:46AM +0200, Stefano Garzarella wrote: > In vu_message_write() we use sendmsg() to send the message header, > then a write() to send the payload. > > If sendmsg() fails we should avoid sending the payload, since we > were unable to send the header. > > Discovered before fixing the issue with the previous patch, where > sendmsg() failed on macOS due to wrong parameters, but the frontend > still sent the payload which the backend incorrectly interpreted > as a wrong header. > > Signed-off-by: Stefano Garzarella > --- > subprojects/libvhost-user/libvhost-user.c | 5 + > 1 file changed, 5 insertions(+) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v4 01/12] libvhost-user: set msg.msg_control to NULL when it is empty
On Wed, May 08, 2024 at 09:44:45AM +0200, Stefano Garzarella wrote:
> On some OS (e.g. macOS) sendmsg() returns -1 (errno EINVAL) if
> the `struct msghdr` has the field `msg_controllen` set to 0, but
> `msg_control` is not NULL.
>
> Reviewed-by: Eric Blake
> Reviewed-by: David Hildenbrand
> Reviewed-by: Philippe Mathieu-Daud??
Philippe's name has got mangled here
> Signed-off-by: Stefano Garzarella
> ---
> subprojects/libvhost-user/libvhost-user.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/subprojects/libvhost-user/libvhost-user.c
> b/subprojects/libvhost-user/libvhost-user.c
> index a879149fef..22bea0c775 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -632,6 +632,7 @@ vu_message_write(VuDev *dev, int conn_fd, VhostUserMsg
> *vmsg)
> memcpy(CMSG_DATA(cmsg), vmsg->fds, fdsize);
> } else {
> msg.msg_controllen = 0;
> +msg.msg_control = NULL;
> }
>
> do {
> --
> 2.45.0
>
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH-for-9.1 v2 2/3] migration: Remove RDMA protocol handling
On Wed, May 01, 2024 at 11:31:13AM -0400, Peter Xu wrote: > What I worry more is whether this is really what we want to keep rdma in > qemu, and that's also why I was trying to request for some serious > performance measurements comparing rdma v.s. nics. And here when I said > "we" I mean both QEMU community and any company that will support keeping > rdma around. > > The problem is if NICs now are fast enough to perform at least equally > against rdma, and if it has a lower cost of overall maintenance, does it > mean that rdma migration will only be used by whoever wants to keep them in > the products and existed already? In that case we should simply ask new > users to stick with tcp, and rdma users should only drop but not increase. > > It seems also destined that most new migration features will not support > rdma: see how much we drop old features in migration now (which rdma > _might_ still leverage, but maybe not), and how much we add mostly multifd > relevant which will probably not apply to rdma at all. So in general what > I am worrying is a both-loss condition, if the company might be easier to > either stick with an old qemu (depending on whether other new features are > requested to be used besides RDMA alone), or do periodic rebase with RDMA > downstream only. I don't know much about the originals of RDMA support in QEMU and why this particular design was taken. It is indeed a huge maint burden to have a completely different code flow for RDMA with 4000+ lines of custom protocol signalling which is barely understandable. I would note that /usr/include/rdma/rsocket.h provides a higher level API that is a 1-1 match of the normal kernel 'sockets' API. If we had leveraged that, then QIOChannelSocket class and the QAPI SocketAddress type could almost[1] trivially have supported RDMA. There would have been almost no RDMA code required in the migration subsystem, and all the modern features like compression, multifd, post-copy, etc would "just work". I guess the 'rsocket.h' shim may well limit some of the possible performance gains, but it might still have been a better tradeoff to have not quite so good peak performance, but with massively less maint burden. With regards, Daniel [1] "almost" trivially, because the poll() integration for rsockets requires a bit more magic sauce since rsockets FDs are not really FDs from the kernel's POV. Still, QIOCHannel likely can abstract that probme. -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH-for-9.1 v2 2/3] migration: Remove RDMA protocol handling
On Tue, Apr 30, 2024 at 09:15:03AM +0200, Markus Armbruster wrote: > Peter Xu writes: > > > On Mon, Apr 29, 2024 at 08:08:10AM -0500, Michael Galaxy wrote: > >> Hi All (and Peter), > > > > Hi, Michael, > > > >> > >> My name is Michael Galaxy (formerly Hines). Yes, I changed my last name > >> (highly irregular for a male) and yes, that's my real last name: > >> https://www.linkedin.com/in/mrgalaxy/) > >> > >> I'm the original author of the RDMA implementation. I've been discussing > >> with Yu Zhang for a little bit about potentially handing over > >> maintainership > >> of the codebase to his team. > >> > >> I simply have zero access to RoCE or Infiniband hardware at all, > >> unfortunately. so I've never been able to run tests or use what I wrote at > >> work, and as all of you know, if you don't have a way to test something, > >> then you can't maintain it. > >> > >> Yu Zhang put a (very kind) proposal forward to me to ask the community if > >> they feel comfortable training his team to maintain the codebase (and run > >> tests) while they learn about it. > > > > The "while learning" part is fine at least to me. IMHO the "ownership" to > > the code, or say, taking over the responsibility, may or may not need 100% > > mastering the code base first. There should still be some fundamental > > confidence to work on the code though as a starting point, then it's about > > serious use case to back this up, and careful testings while getting more > > familiar with it. > > How much experience we expect of maintainers depends on the subsystem > and other circumstances. The hard requirement isn't experience, it's > trust. See the recent attack on xz. > > I do not mean to express any doubts whatsoever on Yu Zhang's integrity! > I'm merely reminding y'all what's at stake. I think we shouldn't overly obsess[1] about 'xz', because the overwhealmingly common scenario is that volunteer maintainers are honest people. QEMU is in a massively better peer review situation. With xz there was basically no oversight of the new maintainer. With QEMU, we have oversight from 1000's of people on the list, a huge pool of general maintainers, the specific migration maintainers, and the release manager merging code. With a lack of historical experiance with QEMU maintainership, I'd suggest that new RDMA volunteers would start by adding themselves to the "MAINTAINERS" file with only the 'Reviewer' classification. The main migration maintainers would still handle pull requests, but wait for a R-b from one of the RMDA volunteers. After some period of time the RDMA folks could graduate to full maintainer status if the migration maintainers needed to reduce their load. I suspect that might prove unneccesary though, given RDMA isn't an area of code with a high turnover of patches. With regards, Daniel [1] If we do want to obsess about something bad though, we should look at our handling of binary blobs in the repo and tarballs. ie the firmware binaries that all get built in an arbitrary environment of their respective maintainer. If we need firmware blobs in tree, we should strive to come up with a reprodicble build environment that gives us byte-for-byte identical results, so the blobs can be verified. This is rather a tangent from this thread though :) -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 00/12] misc: Remove sprintf() due to macOS deprecation
On Thu, Apr 11, 2024 at 11:38:41AM +0200, Philippe Mathieu-Daudé wrote: > On 11/4/24 00:27, BALATON Zoltan wrote: > > On Wed, 10 Apr 2024, Richard Henderson wrote: > > > On 4/10/24 06:06, Philippe Mathieu-Daudé wrote: > > > > Hi, > > > > > > > > sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1, > > > > resulting in painful developper experience. > > > > > > Is snprintf also deprecated? > > > It might be easier to convert some of these fixed buffer cases that > > > way, if allowed. > > > > I had the same thought as some of these might also have performance > > implications (although most of them are in rarely called places). > > I thought GLib/GString was recommended for formatting (IIRC some > previous discussion with Alex / Daniel), so I switched to this > API for style, rather than thinking of performance. Anyway, I'll > respin using sprintf() when the buffer size maths are already done. There are places in QEMU where the strings end up living in a fixed size struct fields, and those would be candidates for sticking with snprint(). For stack allocated string buffers, it is preferrable to switch to g_autofree + g_strdup_printf(), unless there's a compelling performance reason to avoid allocation in a hot path IMHO. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 04/13] tests: Update our CI to use CentOS Stream 9 instead of 8
On Fri, Apr 12, 2024 at 03:24:06PM +0200, Thomas Huth wrote:
> RHEL 9 (and thus also the derivatives) are available since two years
> now, so according to QEMU's support policy, we can drop the active
> support for the previous major version 8 now.
> Thus upgrade our CentOS Stream container to major version 9 now.
The second reason for doing this is that Centos Stream 8
will go EOL in about 1 month:
https://blog.centos.org/2023/04/end-dates-are-coming-for-centos-stream-8-and-centos-linux-7/
"After May 31, 2024, CentOS Stream 8 will be archived
and no further updates will be provided."
I'm seeking confirmation, but I suspect after that date we
will be unable to build centos8 containers, as the package
repos will likely be archived.
RHEL-8 and other derivatives (Alma Linux, Rocky Linux,
etc) remain actively supported by their respective vendors
/ communities. Only CentOS Stream EOLs.
This has implications for our CI on stable branches. It is
valid for our stable branches to continue targetting the
RHEL-8 family of distros, as a 2 year cutoff in our support
policy is evaluated at time of each given major release.
IOW, cherry-picking this change to switch to CentOS Stream
9 is possibly inappropriate for stable branches.
lcitool supports Alma Linux as target, so we could switch
stable branches to Alma Linux 8 if desired to keep CI
coverage of RHEL-8 family.
Thoughts ?
>
> Signed-off-by: Thomas Huth
> ---
> .gitlab-ci.d/buildtest.yml| 16 -
> .gitlab-ci.d/container-core.yml | 4 +--
> .../{centos8.docker => centos9.docker}| 34 +++
> tests/lcitool/mappings.yml| 20 ---
> tests/lcitool/refresh | 2 +-
> tests/vm/centos | 4 +--
> 6 files changed, 26 insertions(+), 54 deletions(-)
> rename tests/docker/dockerfiles/{centos8.docker => centos9.docker} (82%)
>
> diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
> index cfdff175c3..9f34c650d6 100644
> --- a/.gitlab-ci.d/buildtest.yml
> +++ b/.gitlab-ci.d/buildtest.yml
> @@ -158,9 +158,9 @@ build-system-centos:
> - .native_build_job_template
> - .native_build_artifact_template
>needs:
> -job: amd64-centos8-container
> +job: amd64-centos9-container
>variables:
> -IMAGE: centos8
> +IMAGE: centos9
> CONFIGURE_ARGS: --disable-nettle --enable-gcrypt
> --enable-vfio-user-server
>--enable-modules --enable-trace-backends=dtrace --enable-docs
> TARGETS: ppc64-softmmu or1k-softmmu s390x-softmmu
> @@ -242,7 +242,7 @@ check-system-centos:
> - job: build-system-centos
>artifacts: true
>variables:
> -IMAGE: centos8
> +IMAGE: centos9
> MAKE_CHECK_ARGS: check
>
> avocado-system-centos:
> @@ -251,7 +251,7 @@ avocado-system-centos:
> - job: build-system-centos
>artifacts: true
>variables:
> -IMAGE: centos8
> +IMAGE: centos9
> MAKE_CHECK_ARGS: check-avocado
> AVOCADO_TAGS: arch:ppc64 arch:or1k arch:s390x arch:x86_64 arch:rx
>arch:sh4 arch:nios2
> @@ -327,9 +327,9 @@ avocado-system-flaky:
> build-tcg-disabled:
>extends: .native_build_job_template
>needs:
> -job: amd64-centos8-container
> +job: amd64-centos9-container
>variables:
> -IMAGE: centos8
> +IMAGE: centos9
>script:
> - mkdir build
> - cd build
> @@ -651,9 +651,9 @@ build-tci:
> build-without-defaults:
>extends: .native_build_job_template
>needs:
> -job: amd64-centos8-container
> +job: amd64-centos9-container
>variables:
> -IMAGE: centos8
> +IMAGE: centos9
> CONFIGURE_ARGS:
>--without-default-devices
>--without-default-features
> diff --git a/.gitlab-ci.d/container-core.yml b/.gitlab-ci.d/container-core.yml
> index 08f8450fa1..5459447676 100644
> --- a/.gitlab-ci.d/container-core.yml
> +++ b/.gitlab-ci.d/container-core.yml
> @@ -1,10 +1,10 @@
> include:
>- local: '/.gitlab-ci.d/container-template.yml'
>
> -amd64-centos8-container:
> +amd64-centos9-container:
>extends: .container_job_template
>variables:
> -NAME: centos8
> +NAME: centos9
>
> amd64-fedora-container:
>extends: .container_job_template
> diff --git a/tests/docker/dockerfiles/centos8.docker
> b/tests/docker/dockerfiles/centos9.docker
> similarity index 82%
> rename from tests/docker/dockerfiles/centos8.docker
> rename to tests/docker/dockerfiles/centos9.docker
> index ea618bf352..6cf47ce786 100644
> --- a/tests/docker/dockerfiles/centos8.docker
> +++ b/tests/docker/dockerfiles/centos9.docker
> @@ -1,15 +1,14 @@
> # THIS FILE WAS AUTO-GENERATED
> #
> -# $ lcitool dockerfile --layers all centos-stream-8 qemu
> +# $ lcitool dockerfile --layers all centos-stream-9 qemu
> #
> # https://gitlab.com/libvirt/libvirt-ci
>
> -FROM quay.io/centos/centos:stream8
> +FROM quay.io/centos/centos:stream9
>
> RUN dnf distro-sync -y && \
>
Re: [PATCH v2 09/13] block/gluster: Use URI parsing code from glib
On Fri, Apr 12, 2024 at 09:40:11AM -0500, Eric Blake wrote:
> On Fri, Apr 12, 2024 at 03:24:11PM +0200, Thomas Huth wrote:
> > Since version 2.66, glib has useful URI parsing functions, too.
> > Use those instead of the QEMU-internal ones to be finally able
> > to get rid of the latter.
> >
> > Signed-off-by: Thomas Huth
> > ---
> > block/gluster.c | 71 -
> > 1 file changed, 35 insertions(+), 36 deletions(-)
> >
> > diff --git a/block/gluster.c b/block/gluster.c
> > index cc74af06dc..1c9505f8bb 100644
> > --- a/block/gluster.c
> > +++ b/block/gluster.c
> > @@ -17,7 +17,6 @@
> > #include "qapi/error.h"
> > #include "qapi/qmp/qdict.h"
> > #include "qapi/qmp/qerror.h"
> > -#include "qemu/uri.h"
> > #include "qemu/error-report.h"
> > #include "qemu/module.h"
> > #include "qemu/option.h"
> > @@ -289,9 +288,9 @@ static void glfs_clear_preopened(glfs_t *fs)
> > }
> > }
> >
> > -static int parse_volume_options(BlockdevOptionsGluster *gconf, char *path)
> > +static int parse_volume_options(BlockdevOptionsGluster *gconf, const char
> > *path)
>
> Is it worth mentioning in the commit message that this includes a
> const-correctness tweak?
>
> > @@ -364,57 +363,57 @@ static int
> > qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf,
> > QAPI_LIST_PREPEND(gconf->server, gsconf);
> >
> > /* transport */
> > -if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
> > +uri_scheme = g_uri_get_scheme(uri);
> > +if (!uri_scheme || !strcmp(uri_scheme, "gluster")) {
>
> Pre-existing, but per RFC 3986, we should probably be using strcasecmp
> for scheme comparisons (I'm not sure if g_uri_parse guarantees a
> lower-case return, even when the user passed in upper case). That can
> be a separate patch.
Docs say it is lowercase:
https://developer-old.gnome.org/glib/stable/glib-URI-Functions.html
"on return, contains the scheme (converted to lowercase), or NULL."
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 09/13] block/gluster: Use URI parsing code from glib
On Fri, Apr 12, 2024 at 03:24:11PM +0200, Thomas Huth wrote: > Since version 2.66, glib has useful URI parsing functions, too. > Use those instead of the QEMU-internal ones to be finally able > to get rid of the latter. > > Signed-off-by: Thomas Huth > --- > block/gluster.c | 71 - > 1 file changed, 35 insertions(+), 36 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 08/13] Remove glib compatibility code that is not required anymore
On Fri, Apr 12, 2024 at 03:24:10PM +0200, Thomas Huth wrote: > Now that we bumped the minumum glib version to 2.66, we can drop > the old code. > > Suggested-by: Paolo Bonzini > Signed-off-by: Thomas Huth > --- > qga/commands-posix-ssh.c | 8 > util/error-report.c | 10 -- > 2 files changed, 18 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 07/13] Bump minimum glib version to v2.66
On Fri, Apr 12, 2024 at 03:24:09PM +0200, Thomas Huth wrote: > Now that we dropped support for CentOS 8 and Ubuntu 20.04, we can > look into bumping the glib version to a new minimum for further > clean-ups. According to repology.org, available versions are: > > CentOS Stream 9: 2.66.7 > Debian 11: 2.66.8 > Fedora 38: 2.74.1 > Freebsd: 2.78.4 > Homebrew: 2.80.0 > Openbsd: 2.78.4 > OpenSuse leap 15.5:2.70.5 > pkgsrc_current:2.78.4 > Ubuntu 22.04: 2.72.1 > > Thus it should be safe to bump the minimum glib version to 2.66 now. > Version 2.66 comes with new functions for URI parsing which will > allow further clean-ups in the following patches. > > Signed-off-by: Thomas Huth > --- > meson.build | 16 +--- > include/glib-compat.h| 27 ++- > qga/commands-posix-ssh.c | 4 ++-- > 3 files changed, 5 insertions(+), 42 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 06/13] ci: move external build environment setups to CentOS Stream 9
On Fri, Apr 12, 2024 at 03:24:08PM +0200, Thomas Huth wrote:
> From: Paolo Bonzini
>
> RHEL 9 (and thus also the derivatives) are available since two years
> now, so according to QEMU's support policy, we can drop the active
> support for the previous major version 8 now.
>
> Thus upgrade our CentOS Stream build environment playbooks to major
> version 9 now.
>
> Signed-off-by: Paolo Bonzini
> Reviewed-by: Thomas Huth
> Message-ID: <20240412103708.27650-1-pbonz...@redhat.com>
> Signed-off-by: Thomas Huth
> ---
> .../stream/{8 => 9}/build-environment.yml | 31 ++---
> .../stream/{8 => 9}/x86_64/configure | 4 +-
> .../stream/{8 => 9}/x86_64/test-avocado | 0
> scripts/ci/setup/build-environment.yml| 44 +++
> 4 files changed, 34 insertions(+), 45 deletions(-)
> rename scripts/ci/org.centos/stream/{8 => 9}/build-environment.yml (75%)
> rename scripts/ci/org.centos/stream/{8 => 9}/x86_64/configure (98%)
> rename scripts/ci/org.centos/stream/{8 => 9}/x86_64/test-avocado (100%)
Reviewed-by: Daniel P. Berrangé
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 05/13] .travis.yml: Update the jobs to Ubuntu 22.04
On Fri, Apr 12, 2024 at 03:24:07PM +0200, Thomas Huth wrote:
> According to our support policy, we'll soon drop our official support
> for Ubuntu 20.04 ("Focal Fossa") in QEMU. Thus we should update the
> Travis jobs now to a newer release (Ubuntu 22.04 - "Jammy Jellyfish")
> for future testing. Since all jobs are using this release now, we
> can drop the entries from the individual jobs and use the global
> setting again.
>
> Signed-off-by: Thomas Huth
> ---
> .travis.yml | 13 +++--
> 1 file changed, 3 insertions(+), 10 deletions(-)
Reviewed-by: Daniel P. Berrangé
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 04/13] tests: Update our CI to use CentOS Stream 9 instead of 8
On Fri, Apr 12, 2024 at 03:24:06PM +0200, Thomas Huth wrote:
> RHEL 9 (and thus also the derivatives) are available since two years
> now, so according to QEMU's support policy, we can drop the active
> support for the previous major version 8 now.
> Thus upgrade our CentOS Stream container to major version 9 now.
>
> Signed-off-by: Thomas Huth
> ---
> .gitlab-ci.d/buildtest.yml| 16 -
> .gitlab-ci.d/container-core.yml | 4 +--
> .../{centos8.docker => centos9.docker}| 34 +++
> tests/lcitool/mappings.yml| 20 ---
> tests/lcitool/refresh | 2 +-
> tests/vm/centos | 4 +--
> 6 files changed, 26 insertions(+), 54 deletions(-)
> rename tests/docker/dockerfiles/{centos8.docker => centos9.docker} (82%)
Reviewed-by: Daniel P. Berrangé
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 03/13] tests/docker/dockerfiles: Run lcitool-refresh after the lcitool update
On Fri, Apr 12, 2024 at 03:24:05PM +0200, Thomas Huth wrote: > This update adds the removing of the EXTERNALLY-MANAGED marker files > that has been added to the lcitool recently. For those who don't know, python now commonly blocks the ability to run 'pip install' outside of a venv. This generally makes sense for a precious installation environment. Our containers are disposable though, so a venv has no benefit. Removing the 'EXTERNALLY-MANAGED' allows the historical arbitrary use of 'pip' outside a venv. lcitool just does this unconditionally given the containers are not precious. > > Signed-off-by: Thomas Huth > --- > tests/docker/dockerfiles/alpine.docker| 3 ++- > tests/docker/dockerfiles/centos8.docker | 1 + > tests/docker/dockerfiles/debian-amd64-cross.docker| 3 ++- > tests/docker/dockerfiles/debian-arm64-cross.docker| 3 ++- > tests/docker/dockerfiles/debian-armel-cross.docker| 3 ++- > tests/docker/dockerfiles/debian-armhf-cross.docker| 3 ++- > tests/docker/dockerfiles/debian-i686-cross.docker | 3 ++- > tests/docker/dockerfiles/debian-mips64el-cross.docker | 3 ++- > tests/docker/dockerfiles/debian-mipsel-cross.docker | 3 ++- > tests/docker/dockerfiles/debian-ppc64el-cross.docker | 3 ++- > tests/docker/dockerfiles/debian-riscv64-cross.docker | 3 ++- > tests/docker/dockerfiles/debian-s390x-cross.docker| 3 ++- > tests/docker/dockerfiles/debian.docker| 1 + > tests/docker/dockerfiles/fedora-win64-cross.docker| 3 ++- > tests/docker/dockerfiles/fedora.docker| 1 + > tests/docker/dockerfiles/opensuse-leap.docker | 1 + > tests/docker/dockerfiles/ubuntu2204.docker| 1 + > 17 files changed, 29 insertions(+), 12 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 02/13] tests/lcitool/libvirt-ci: Update to the latest master branch
On Fri, Apr 12, 2024 at 03:24:04PM +0200, Thomas Huth wrote: > We need the latest fixes for the lcitool to be able to properly > update our CentOS docker file to CentOS Stream 9. > > Signed-off-by: Thomas Huth > --- > tests/lcitool/libvirt-ci | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH-for-9.1 v2 2/3] migration: Remove RDMA protocol handling
On Fri, Mar 29, 2024 at 11:28:54AM +0100, Philippe Mathieu-Daudé wrote:
> Hi Zhijian,
>
> On 29/3/24 02:53, Zhijian Li (Fujitsu) wrote:
> >
> >
> > On 28/03/2024 23:01, Peter Xu wrote:
> > > On Thu, Mar 28, 2024 at 11:18:04AM -0300, Fabiano Rosas wrote:
> > > > Philippe Mathieu-Daudé writes:
> > > >
> > > > > The whole RDMA subsystem was deprecated in commit e9a54265f5
> > > > > ("hw/rdma: Deprecate the pvrdma device and the rdma subsystem")
> > > > > released in v8.2.
> > > > >
> > > > > Remove:
> > > > >- RDMA handling from migration
> > > > >- dependencies on libibumad, libibverbs and librdmacm
> > > > >
> > > > > Keep the RAM_SAVE_FLAG_HOOK definition since it might appears
> > > > > in old migration streams.
> > > > >
> > > > > Cc: Peter Xu
> > > > > Cc: Li Zhijian
> > > > > Acked-by: Fabiano Rosas
> > > > > Signed-off-by: Philippe Mathieu-Daudé
> > > >
> > > > Just to be clear, because people raised the point in the last version,
> > > > the first link in the deprecation commit links to a thread comprising
> > > > entirely of rdma migration patches. I don't see any ambiguity on whether
> > > > the deprecation was intended to include migration. There's even an ack
> > > > from Juan.
> > >
> > > Yes I remember that's the plan.
> > >
> > > >
> > > > So on the basis of not reverting the previous maintainer's decision, my
> > > > Ack stands here.
> > > >
> > > > We also had pretty obvious bugs ([1], [2]) in the past that would have
> > > > been caught if we had any kind of testing for the feature, so I can't
> > > > even say this thing works currently.
> > > >
> > > > @Peter Xu, @Li Zhijian, what are your thoughts on this?
> > >
> > > Generally I definitely agree with such a removal sooner or later, as
> > > that's
> > > how deprecation works, and even after Juan's left I'm not aware of any
> > > other new RDMA users. Personally, I'd slightly prefer postponing it one
> > > more release which might help a bit of our downstream maintenance, however
> > > I assume that's not a blocker either, as I think we can also manage it.
> > >
> > > IMHO it's more important to know whether there are still users and whether
> > > they would still like to see it around. That's also one thing I notice
> > > that
> > > e9a54265f533f didn't yet get acks from RDMA users that we are aware, even
> > > if they're rare. According to [2] it could be that such user may only rely
> > > on the release versions of QEMU when it broke things.
> > >
> > > So I'm copying Yu too (while Zhijian is already in the loop), just in case
> > > someone would like to stand up and speak.
> >
> >
> > I admit RDMA migration was lack of testing(unit/CI test), which led to the
> > a few
> > obvious bugs being noticed too late.
> > However I was a bit surprised when I saw the removal of the RDMA migration.
> > I wasn't
> > aware that this feature has not been marked as deprecated(at least there is
> > no
> > prompt to end-user).
> >
> >
> > > IMHO it's more important to know whether there are still users and whether
> > > they would still like to see it around.
> >
> > Agree.
> > I didn't immediately express my opinion in V1 because I'm also consulting
> > our
> > customers for this feature in the future.
> >
> > Personally, I agree with Perter's idea that "I'd slightly prefer postponing
> > it one
> > more release which might help a bit of our downstream maintenance"
>
> Do you mind posting a deprecation patch to clarify the situation?
The key thing the first deprecation patch missed was that it failed
to issue a warning message when RDMA migration was actually used.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH for-9.1 6/9] block/nbd: Use URI parsing code from glib
On Thu, Mar 28, 2024 at 09:54:49AM -0500, Eric Blake wrote:
> On Thu, Mar 28, 2024 at 03:06:03PM +0100, Thomas Huth wrote:
> > Since version 2.66, glib has useful URI parsing functions, too.
> > Use those instead of the QEMU-internal ones to be finally able
> > to get rid of the latter. The g_uri_get_host() also takes care
> > of removing the square brackets from IPv6 addresses, so we can
> > drop that part of the QEMU code now, too.
> >
> > Signed-off-by: Thomas Huth
> > ---
> > block/nbd.c | 66 ++---
> > 1 file changed, 38 insertions(+), 28 deletions(-)
> >
> > diff --git a/block/nbd.c b/block/nbd.c
> > index ef05f7cdfd..95b507f872 100644
> > --- a/block/nbd.c
> > +++ b/block/nbd.c
> > @@ -31,7 +31,6 @@
> > #include "qemu/osdep.h"
> >
> > #include "trace.h"
> > -#include "qemu/uri.h"
> > #include "qemu/option.h"
> > #include "qemu/cutils.h"
> > #include "qemu/main-loop.h"
> > @@ -1514,30 +1513,34 @@ static void nbd_client_close(BlockDriverState *bs)
> >
> > static int nbd_parse_uri(const char *filename, QDict *options)
> > {
> > -URI *uri;
> > +GUri *uri;
>
> Is it worth using 'g_autoptr(GUri) uri = NULL;' here, to simplify cleanup
> later?
>
> > const char *p;
> > -QueryParams *qp = NULL;
> > +GHashTable *qp = NULL;
>
> Presumably would be easier if qp is also auto-free.
>
> > +int qp_n;
> > int ret = 0;
> > bool is_unix;
> > +const char *uri_scheme, *uri_query, *uri_server;
> > +int uri_port;
> >
> > -uri = uri_parse(filename);
> > +uri = g_uri_parse(filename, G_URI_FLAGS_NONE, NULL);
>
> The glib API is fairly close to what we have in qemu, making this a
> nice switchover.
>
> > /* nbd[+tcp]://host[:port]/export */
> > -if (!uri->server) {
> > +if (!uri_server) {
> > ret = -EINVAL;
> > goto out;
> > }
> >
> > -/* strip braces from literal IPv6 address */
> > -if (uri->server[0] == '[') {
> > -host = qstring_from_substr(uri->server, 1,
> > - strlen(uri->server) - 1);
> > -} else {
> > -host = qstring_from_str(uri->server);
> > -}
> > -
> > qdict_put_str(options, "server.type", "inet");
> > -qdict_put(options, "server.host", host);
> > +qdict_put_str(options, "server.host", uri_server);
> >
> > -port_str = g_strdup_printf("%d", uri->port ?: NBD_DEFAULT_PORT);
> > +port_str = g_strdup_printf("%d", uri_port != -1 ? uri_port
> > +:
> > NBD_DEFAULT_PORT);
> > qdict_put_str(options, "server.port", port_str);
>
> If a user requests nbd://hostname:0/export, this now sets server.port
> to "0" instead of "10809". Is that an intentional change? No one
> actually passes an explicit ":0" port on purpose, but we do have to
> worry about malicious URIs.
Passing '0' will cause the kernel to allocate a random free
port, so that is potentially introducing new semantics ?
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH-for-9.1] rdma: Remove RDMA subsystem and pvrdma device
On Thu, Mar 28, 2024 at 01:57:27PM +0100, Philippe Mathieu-Daudé wrote:
> On 28/3/24 10:06, Daniel P. Berrangé wrote:
> > On Wed, Mar 27, 2024 at 11:55:48AM +0100, Philippe Mathieu-Daudé wrote:
> > > The whole RDMA subsystem was deprecated in commit e9a54265f5
> > > ("hw/rdma: Deprecate the pvrdma device and the rdma subsystem")
> > > released in v8.2. Time to remove it.
> > >
> > > Keep the RAM_SAVE_FLAG_HOOK definition since it might appears
> > > in old migration streams.
> > >
> > > Remove the dependencies on libibumad and libibverbs.
> >
> > >
> > > Remove the generated vmw_pvrdma/ directory from linux-headers.
> > >
> > > Remove RDMA handling from migration.
> > >
> > > Remove RDMA handling in GlusterFS block driver.
> >
> > The RDMA support in GlusterFS is completely opaque to QEMU.
> > All we have there is the CLI syntax to enable use of the
> > RDMA support inside libglusterfs. I'm not convinced that
> > the justification for deprecation (lack of maintanier)
> > applies to this scenario.
>
> I'll quote commit 0552ff2465 from 2016 then:
>
> block/gluster: deprecate rdma support
>
> gluster volfile server fetch happens through unix and/or tcp,
> it doesn't support volfile fetch over rdma. The rdma code may
> actually mislead, so to make sure things do not break, for now
> we fallback to tcp when requested for rdma, with a warning.
>
> If you are wondering how this worked all these days, its the gluster
> libgfapi code which handles anything other than unix transport as
> socket/tcp, sad but true.
Ok, that should have been mentioned in the commit message
then, and is another reason for removing each functional
area in a separate commit, with the relevant notes for each.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH-for-9.1] rdma: Remove RDMA subsystem and pvrdma device
On Wed, Mar 27, 2024 at 11:55:48AM +0100, Philippe Mathieu-Daudé wrote:
> The whole RDMA subsystem was deprecated in commit e9a54265f5
> ("hw/rdma: Deprecate the pvrdma device and the rdma subsystem")
> released in v8.2. Time to remove it.
>
> Keep the RAM_SAVE_FLAG_HOOK definition since it might appears
> in old migration streams.
>
> Remove the dependencies on libibumad and libibverbs.
>
> Remove the generated vmw_pvrdma/ directory from linux-headers.
>
> Remove RDMA handling from migration.
>
> Remove RDMA handling in GlusterFS block driver.
The RDMA support in GlusterFS is completely opaque to QEMU.
All we have there is the CLI syntax to enable use of the
RDMA support inside libglusterfs. I'm not convinced that
the justification for deprecation (lack of maintanier)
applies to this scenario.
> Remove rdmacm-mux tool from contrib/.
>
> Remove PVRDMA device.
I agree with Thomas that each functional area listed here
should be handld in a separate patch, since they're all
independant.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PULL 1/1] coroutine: cap per-thread local pool size
Sending this PULL feels little rushed, as I still have
un-answered questions on the inital patch posting just
a few hours ago
On Tue, Mar 19, 2024 at 11:09:38AM -0400, Stefan Hajnoczi wrote:
> The coroutine pool implementation can hit the Linux vm.max_map_count
> limit, causing QEMU to abort with "failed to allocate memory for stack"
> or "failed to set up stack guard page" during coroutine creation.
>
> This happens because per-thread pools can grow to tens of thousands of
> coroutines. Each coroutine causes 2 virtual memory areas to be created.
> Eventually vm.max_map_count is reached and memory-related syscalls fail.
> The per-thread pool sizes are non-uniform and depend on past coroutine
> usage in each thread, so it's possible for one thread to have a large
> pool while another thread's pool is empty.
>
> Switch to a new coroutine pool implementation with a global pool that
> grows to a maximum number of coroutines and per-thread local pools that
> are capped at hardcoded small number of coroutines.
>
> This approach does not leave large numbers of coroutines pooled in a
> thread that may not use them again. In order to perform well it
> amortizes the cost of global pool accesses by working in batches of
> coroutines instead of individual coroutines.
>
> The global pool is a list. Threads donate batches of coroutines to when
> they have too many and take batches from when they have too few:
>
> .---.
> | Batch 1 | Batch 2 | Batch 3 | ... | global_pool
> `---'
>
> Each thread has up to 2 batches of coroutines:
>
> .---.
> | Batch 1 | Batch 2 | per-thread local_pool (maximum 2 batches)
> `---'
>
> The goal of this change is to reduce the excessive number of pooled
> coroutines that cause QEMU to abort when vm.max_map_count is reached
> without losing the performance of an adequately sized coroutine pool.
>
> Here are virtio-blk disk I/O benchmark results:
>
> RW BLKSIZE IODEPTHOLDNEW CHANGE
> randread 4k 1 113725 117451 +3.3%
> randread 4k 8 192968 198510 +2.9%
> randread 4k 16 207138 209429 +1.1%
> randread 4k 32 212399 215145 +1.3%
> randread 4k 64 218319 221277 +1.4%
> randread128k 1 17587 17535 -0.3%
> randread128k 8 17614 17616 +0.0%
> randread128k 16 17608 17609 +0.0%
> randread128k 32 17552 17553 +0.0%
> randread128k 64 17484 17484 +0.0%
>
> See files/{fio.sh,test.xml.j2} for the benchmark configuration:
> https://gitlab.com/stefanha/virt-playbooks/-/tree/coroutine-pool-fix-sizing
>
> Buglink: https://issues.redhat.com/browse/RHEL-28947
> Reported-by: Sanjay Rao
> Reported-by: Boaz Ben Shabat
> Reported-by: Joe Mario
> Reviewed-by: Kevin Wolf
> Signed-off-by: Stefan Hajnoczi
> Message-ID: <20240318183429.1039340-1-stefa...@redhat.com>
> ---
> util/qemu-coroutine.c | 282 +-
> 1 file changed, 223 insertions(+), 59 deletions(-)
>
> diff --git a/util/qemu-coroutine.c b/util/qemu-coroutine.c
> index 5fd2dbaf8b..2790959eaf 100644
> --- a/util/qemu-coroutine.c
> +++ b/util/qemu-coroutine.c
> @@ -18,39 +18,200 @@
> #include "qemu/atomic.h"
> #include "qemu/coroutine_int.h"
> #include "qemu/coroutine-tls.h"
> +#include "qemu/cutils.h"
> #include "block/aio.h"
>
> -/**
> - * The minimal batch size is always 64, coroutines from the release_pool are
> - * reused as soon as there are 64 coroutines in it. The maximum pool size
> starts
> - * with 64 and is increased on demand so that coroutines are not deleted
> even if
> - * they are not immediately reused.
> - */
> enum {
> -POOL_MIN_BATCH_SIZE = 64,
> -POOL_INITIAL_MAX_SIZE = 64,
> +COROUTINE_POOL_BATCH_MAX_SIZE = 128,
> };
>
> -/** Free list to speed up creation */
> -static QSLIST_HEAD(, Coroutine) release_pool = QSLIST_HEAD_INITIALIZER(pool);
> -static unsigned int pool_max_size = POOL_INITIAL_MAX_SIZE;
> -static unsigned int release_pool_size;
> +/*
> + * Coroutine creation and deletion is expensive so a pool of unused
> coroutines
> + * is kept as a cache. When the pool has coroutines available, they are
> + * recycled instead of creating new ones from scratch. Coroutines are added
> to
> + * the pool upon termination.
> + *
> + * The pool is global but each thread maintains a small local pool to avoid
> + * global pool contention. Threads fetch and return batches of coroutines
> from
> + * the global pool to maintain their local pool. The local pool holds up to
> two
> + * batches whereas the maximum size of the global pool is controlled by the
> + * qemu_coroutine_inc_pool_size() API.
> + *
> + * .---.
> + * | Batch 1 | Batch 2 | Batch 3 | ... | global_pool
> + * `---'
> + *
> + * .---.
> + * | Batch 1 | Batch 2 | per-thread local_pool (maximum 2 batches)
> + *
Re: [PATCH v3] block: Use LVM tools for LV block device truncation
On Fri, Mar 15, 2024 at 09:58:38AM +0100, Alexander Ivanov wrote:
> If a block device is an LVM logical volume we can resize it using
> standard LVM tools.
>
> Add a helper to detect if a device is a DM device. In raw_co_truncate()
> check if the block device is DM and resize it executing lvresize.
>
> Signed-off-by: Alexander Ivanov
> ---
> block/file-posix.c | 61 ++
> 1 file changed, 61 insertions(+)
Reviewed-by: Daniel P. Berrangé
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 35684f7e21..af17a43fe9 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -2642,6 +2642,38 @@ raw_regular_truncate(BlockDriverState *bs, int fd,
> int64_t offset,
> return raw_thread_pool_submit(handle_aiocb_truncate, );
> }
>
> +static bool device_is_dm(struct stat *st)
> +{
> +unsigned int maj, maj2;
> +char line[32], devname[16];
> +bool ret = false;
> +FILE *f;
> +
> +if (!S_ISBLK(st->st_mode)) {
> +return false;
> +}
> +
> +f = fopen("/proc/devices", "r");
> +if (!f) {
> +return false;
> +}
> +
> +maj = major(st->st_rdev);
> +
> +while (fgets(line, sizeof(line), f)) {
> +if (sscanf(line, "%u %15s", , devname) != 2) {
> +continue;
> +}
> +if (strcmp(devname, "device-mapper") == 0) {
> +ret = (maj == maj2);
> +break;
> +}
> +}
> +
> +fclose(f);
> +return ret;
> +}
> +
> static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset,
> bool exact, PreallocMode prealloc,
> BdrvRequestFlags flags, Error **errp)
> @@ -2670,6 +2702,35 @@ static int coroutine_fn
> raw_co_truncate(BlockDriverState *bs, int64_t offset,
> if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
> int64_t cur_length = raw_getlength(bs);
>
> +/*
> + * Try to resize an LVM device using LVM tools.
> + */
> +if (device_is_dm() && offset > 0) {
> +int spawn_flags = G_SPAWN_SEARCH_PATH |
> G_SPAWN_STDOUT_TO_DEV_NULL;
> +int status;
> +bool success;
> +char *err;
> +GError *gerr = NULL, *gerr_exit = NULL;
> +g_autofree char *size_str = g_strdup_printf("%" PRId64 "B",
> offset);
> +const char *cmd[] = {"lvresize", "-f", "-L",
> + size_str, bs->filename, NULL};
> +
> +success = g_spawn_sync(NULL, (gchar **)cmd, NULL, spawn_flags,
> + NULL, NULL, NULL, , , );
> +
> +if (success && g_spawn_check_exit_status(status, _exit)) {
> +return 0;
> +}
> +
> +if (success) {
> +error_setg(errp, "%s: %s", gerr_exit->message, err);
> +} else {
> +error_setg(errp, "lvresize execution error: %s",
> gerr->message);
> +}
> +
> +return -EINVAL;
> +}
> +
> if (offset != cur_length && exact) {
> error_setg(errp, "Cannot resize device files");
> return -ENOTSUP;
> --
> 2.40.1
>
>
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2] block: Use LVM tools for LV block device truncation
On Thu, Mar 14, 2024 at 06:25:00PM +0100, Alexander Ivanov wrote:
>
>
> On 3/14/24 13:44, Daniel P. Berrangé wrote:
> > On Wed, Mar 13, 2024 at 11:43:27AM +0100, Alexander Ivanov wrote:
> > > If a block device is an LVM logical volume we can resize it using
> > > standard LVM tools.
> > >
> > > Add a helper to detect if a device is a DM device. In raw_co_truncate()
> > > check if the block device is DM and resize it executing lvresize.
> > >
> > > Signed-off-by: Alexander Ivanov
> > > ---
> > > block/file-posix.c | 61 ++
> > > 1 file changed, 61 insertions(+)
> > >
> > > diff --git a/block/file-posix.c b/block/file-posix.c
> > > index 35684f7e21..5f07d98aa5 100644
> > > --- a/block/file-posix.c
> > > +++ b/block/file-posix.c
> > > @@ -2642,6 +2642,38 @@ raw_regular_truncate(BlockDriverState *bs, int fd,
> > > int64_t offset,
> > > return raw_thread_pool_submit(handle_aiocb_truncate, );
> > > }
> > > static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t
> > > offset,
> > > bool exact, PreallocMode
> > > prealloc,
> > > BdrvRequestFlags flags, Error
> > > **errp)
> > > @@ -2670,6 +2702,35 @@ static int coroutine_fn
> > > raw_co_truncate(BlockDriverState *bs, int64_t offset,
> > > if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
> > > int64_t cur_length = raw_getlength(bs);
> > > +/*
> > > + * Try to resize an LVM device using LVM tools.
> > > + */
> > > +if (device_is_dm() && offset > 0) {
> > > +int spawn_flags = G_SPAWN_SEARCH_PATH |
> > > G_SPAWN_STDOUT_TO_DEV_NULL;
> > > +int status;
> > > +bool success;
> > > +char *err;
> > > +GError *gerr = NULL;
> > > +g_autofree char *size_str = g_strdup_printf("%ldB", offset);
> > offset is 64-bit, but '%ld' is not guaranteed to be 64-bit. I expect
> > this will break on 32-bit platforms. Try PRId64 instead.
> >
> > > +const char *cmd[] = {"lvresize", "-f", "-L",
> > > + size_str, bs->filename, NULL};
> > > +
> > > +success = g_spawn_sync(NULL, (gchar **)cmd, NULL,
> > > spawn_flags,
> > > + NULL, NULL, NULL, , ,
> > > );
> > > +
> > > +if (success && WEXITSTATUS(status) == 0) {
> > > +return 0;
> > > +}
> > We should probably check 'g_spawn_check_wait_status' rather than
> > WEXITSTATUS, as this then gives us further eror message details
> > that
> Thank you.
> I think it would be better to use 'g_spawn_check_exit_status' because there
> is no
> 'g_spawn_check_wait_status' in glib before 2.70 and even in 2.78 it leads to
> 'g_spawn_check_wait_status is deprecated: Not available before 2.70' error.
Ah yes, well spotted.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2] block: Use LVM tools for LV block device truncation
On Wed, Mar 13, 2024 at 11:43:27AM +0100, Alexander Ivanov wrote:
> If a block device is an LVM logical volume we can resize it using
> standard LVM tools.
>
> Add a helper to detect if a device is a DM device. In raw_co_truncate()
> check if the block device is DM and resize it executing lvresize.
>
> Signed-off-by: Alexander Ivanov
> ---
> block/file-posix.c | 61 ++
> 1 file changed, 61 insertions(+)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 35684f7e21..5f07d98aa5 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -2642,6 +2642,38 @@ raw_regular_truncate(BlockDriverState *bs, int fd,
> int64_t offset,
> return raw_thread_pool_submit(handle_aiocb_truncate, );
> }
> static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset,
> bool exact, PreallocMode prealloc,
> BdrvRequestFlags flags, Error **errp)
> @@ -2670,6 +2702,35 @@ static int coroutine_fn
> raw_co_truncate(BlockDriverState *bs, int64_t offset,
> if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
> int64_t cur_length = raw_getlength(bs);
>
> +/*
> + * Try to resize an LVM device using LVM tools.
> + */
> +if (device_is_dm() && offset > 0) {
> +int spawn_flags = G_SPAWN_SEARCH_PATH |
> G_SPAWN_STDOUT_TO_DEV_NULL;
> +int status;
> +bool success;
> +char *err;
> +GError *gerr = NULL;
> +g_autofree char *size_str = g_strdup_printf("%ldB", offset);
offset is 64-bit, but '%ld' is not guaranteed to be 64-bit. I expect
this will break on 32-bit platforms. Try PRId64 instead.
> +const char *cmd[] = {"lvresize", "-f", "-L",
> + size_str, bs->filename, NULL};
> +
> +success = g_spawn_sync(NULL, (gchar **)cmd, NULL, spawn_flags,
> + NULL, NULL, NULL, , , );
> +
> +if (success && WEXITSTATUS(status) == 0) {
> +return 0;
> +}
We should probably check 'g_spawn_check_wait_status' rather than
WEXITSTATUS, as this then gives us further eror message details
that
> +
> +if (!success) {
> +error_setg(errp, "lvresize execution error: %s",
> gerr->message);
> +} else {
> +error_setg(errp, "%s", err);
...we would also include here, such as the exit code or terminal
signal.
> +}
> +
> +return -EINVAL;
> +}
> +
> if (offset != cur_length && exact) {
> error_setg(errp, "Cannot resize device files");
> return -ENOTSUP;
> --
> 2.40.1
>
>
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH] block: Use LVM tools for LV block device truncation
On Tue, Mar 12, 2024 at 06:04:26PM +0100, Alexander Ivanov wrote:
> Thank you for the review.
>
> On 3/11/24 19:24, Daniel P. Berrangé wrote:
> > On Mon, Mar 11, 2024 at 06:40:44PM +0100, Alexander Ivanov wrote:
> > > If a block device is an LVM logical volume we can resize it using
> > > standard LVM tools.
> > >
> > > In raw_co_truncate() check if the block device is a LV using lvdisplay
> > > and resize it executing lvresize.
> > >
> > > Signed-off-by: Alexander Ivanov
> > > ---
> > > block/file-posix.c | 27 +++
> > > 1 file changed, 27 insertions(+)
> > >
> > > diff --git a/block/file-posix.c b/block/file-posix.c
> > > index 35684f7e21..cf8a04e6f7 100644
> > > --- a/block/file-posix.c
> > > +++ b/block/file-posix.c
> > > @@ -2670,6 +2670,33 @@ static int coroutine_fn
> > > raw_co_truncate(BlockDriverState *bs, int64_t offset,
> > > if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
> > > int64_t cur_length = raw_getlength(bs);
> > > +/*
> > > + * Check if the device is an LVM logical volume and try to resize
> > > + * it using LVM tools.
> > > + */
> > > +if (S_ISBLK(st.st_mode) && offset > 0) {
> > > +char cmd[PATH_MAX + 32];
> > > +
> > > +snprintf(cmd, sizeof(cmd), "lvdisplay %s > /dev/null",
> > > + bs->filename);
> > PATH_MAX + snprint is a bad practice - g_strdup_printf() is recommended
> > for dynamic allocation, along with g_autofree for release.
> >
> > > +ret = system(cmd);
> > IMHO using 'system' for spawning processes is dubious in any non-trivial
> > program.
> >
> > Historically at least it does not have well defined behaviour wrt signal
> > handling in the child, before execve is called. ie potentially a signal
> > handler registered by QEMU in the parent could run in the child having
> > ill-effects.
> >
> > 'system' also executes via the shell which opens up many risks with
> > unsanitized files path being substituted into the command line.
> > > +if (ret != 0) {
> > > +error_setg(errp, "lvdisplay returned %d error for '%s'",
> > > + ret, bs->filename);
> > > +return ret;
> > > +}
> > Calling 'lvdisplay' doesn't seem to be needed. Surely 'lvresize' is
> > going to report errors if it isn't an LVM device.
> The problem is that we don't know if 'lvresize' returned an error because of
> non-LVM device or there was another reason. For the first variant we should
> continue original code execution, for the second - return an error.
> >
> > If we want to detect an LVM device though, couldn't we lookup
> > 'device-mapper' in /proc/devices and then major the device major
> > node number.
> It will require more code for getting device major, file reading and
> parsing.
> Why this way is better?
There are a variety of reasons why these LVM commands could fail.
Directly detecting whether or not this is an LVM device, rather
than inferring it from 'lvdisplay' result is a more precise check
which will allow for clearer error reporting IMHO.
Ideally we wouldn't even spawn a process for the resize operation,
but instead call into the device mapper library API. That would
let it work even when the seccomp sandbox is enabled and blocking
process spawning. IME the device mapper API is not too friendly
though, so I didn't want to suggest that as a blocker.
> > > +snprintf(cmd, sizeof(cmd), "lvresize -f -L %ldB %s >
> > > /dev/null",
> > > + offset, bs->filename);
> > > +ret = system(cmd);
> > > +if (ret != 0) {
> > > +error_setg(errp, "lvresize returned %d error for '%s'",
> > > + ret, bs->filename);
> > lvresize might display an message on stderr on failure but that's at best
> > going to QEMU's stderr. Any error needs to be captured and put into
> > this error message that's fed back to the user / QMP client.
> It seems I need to implement a high level function for programs execution.
> Looked at
> g_spawn_sync(), but it uses fork() under the hood and we could have a
> performance
> issue. Haven't found a high level function that uses vfork() and allows to
> catch stderr.
GLib should be using posix_spawn internally on all modern platforms.
Resizing LVM volumes does not sound like a hot code path, so I'm
seeing what performance concerns we would have.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH] block: Use LVM tools for LV block device truncation
On Mon, Mar 11, 2024 at 06:40:44PM +0100, Alexander Ivanov wrote:
> If a block device is an LVM logical volume we can resize it using
> standard LVM tools.
>
> In raw_co_truncate() check if the block device is a LV using lvdisplay
> and resize it executing lvresize.
>
> Signed-off-by: Alexander Ivanov
> ---
> block/file-posix.c | 27 +++
> 1 file changed, 27 insertions(+)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 35684f7e21..cf8a04e6f7 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -2670,6 +2670,33 @@ static int coroutine_fn
> raw_co_truncate(BlockDriverState *bs, int64_t offset,
> if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
> int64_t cur_length = raw_getlength(bs);
>
> +/*
> + * Check if the device is an LVM logical volume and try to resize
> + * it using LVM tools.
> + */
> +if (S_ISBLK(st.st_mode) && offset > 0) {
> +char cmd[PATH_MAX + 32];
> +
> +snprintf(cmd, sizeof(cmd), "lvdisplay %s > /dev/null",
> + bs->filename);
PATH_MAX + snprint is a bad practice - g_strdup_printf() is recommended
for dynamic allocation, along with g_autofree for release.
> +ret = system(cmd);
IMHO using 'system' for spawning processes is dubious in any non-trivial
program.
Historically at least it does not have well defined behaviour wrt signal
handling in the child, before execve is called. ie potentially a signal
handler registered by QEMU in the parent could run in the child having
ill-effects.
'system' also executes via the shell which opens up many risks with
unsanitized files path being substituted into the command line.
> +if (ret != 0) {
> +error_setg(errp, "lvdisplay returned %d error for '%s'",
> + ret, bs->filename);
> +return ret;
> +}
Calling 'lvdisplay' doesn't seem to be needed. Surely 'lvresize' is
going to report errors if it isn't an LVM device.
If we want to detect an LVM device though, couldn't we lookup
'device-mapper' in /proc/devices and then major the device major
node number.
> +
> +snprintf(cmd, sizeof(cmd), "lvresize -f -L %ldB %s > /dev/null",
> + offset, bs->filename);
> +ret = system(cmd);
> +if (ret != 0) {
> +error_setg(errp, "lvresize returned %d error for '%s'",
> + ret, bs->filename);
lvresize might display an message on stderr on failure but that's at best
going to QEMU's stderr. Any error needs to be captured and put into
this error message that's fed back to the user / QMP client.
> +}
> +
> +return ret;
> +}
> +
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 9/9] hostmem-file: support POSIX shm_open()
On Wed, Feb 28, 2024 at 12:47:59PM +0100, Stefano Garzarella wrote: > Add a new `shm` bool option for `-object memory-backend-file`. > > When this option is set to true, the POSIX shm_open(3) is used instead > of open(2). > > So a file will not be created in the filesystem, but a "POSIX shared > memory object" will be instantiated. In Linux this turns into a file > in /dev/shm, but in other OSes this may not happen (for example in > macOS or FreeBSD nothing is shown in any filesystem). > > This new feature is useful when we need to share guest memory with > another process (e.g. vhost-user backend), but we don't have > memfd_create() or any special filesystems (e.g. /dev/shm) available > as in macOS. > > Signed-off-by: Stefano Garzarella > --- > I am not sure this is the best way to support shm_open() in QEMU. > > Other solutions I had in mind were: > > - create a new memory-backend-shm > > - extend memory-backend-memfd to use shm_open() on systems where memfd is > not available (problem: shm_open wants a name to assign to the object, but > we can do a workaround by using a random name and do the unlink right away) IMHO, create a new memory-backend-shm, don't overload memory-backend-memfd, as this lets users choose between shm & memfd, even on Linux. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 04/28] qemu-img: global option processing and error printing
On Thu, Feb 22, 2024 at 12:15:45AM +0300, Michael Tokarev wrote:
> In order to correctly print executable name in various
> error messages, pass argv[0] to error_exit() function.
> This way, error messages will refer to actual executable
> name, which may be different from 'qemu-img'.
>
> For subcommands, pass whole argv[] array, so argv[0] is
> the executable name, not subcommand name. In order to
> do that, avoid resetting optind but continue with the
> next option. Also don't require at least 3 options on
> the command line: it makes no sense with options before
> subcommand.
>
> Before invoking a subcommand, replace argv[0] to include
> the subcommand name.
>
> Introduce tryhelp() function which just prints
>
> try 'command-name --help' for more info
>
> and exits. When tryhelp() is called from within a subcommand
> handler, the message will look like:
>
> try 'command-name subcommand --help' for more info
>
> qemu-img uses getopt_long() with ':' as the first char in
> optstring parameter, which means it doesn't print error
> messages but return ':' or '?' instead, and qemu-img uses
> unrecognized_option() or missing_argument() function to
> print error messages. But it doesn't quite work:
>
> $ ./qemu-img -xx
> qemu-img: unrecognized option './qemu-img'
>
> so the aim is to let getopt_long() to print regular error
> messages instead (removing ':' prefix from optstring) and
> remove handling of '?' and ':' "options" entirely. With
> concatenated argv[0] and the subcommand, it all finally
> does the right thing in all cases. This will be done in
> subsequent changes command by command, with main() done
> last.
>
> unrecognized_option() and missing_argument() functions
> prototypes aren't changed by this patch, since they're
> called from many places and will be removed a few patches
> later. Only artifical "qemu-img" argv0 is provided in
> there for now.
>
> Signed-off-by: Michael Tokarev
> ---
> qemu-img.c | 75 +++---
> 1 file changed, 38 insertions(+), 37 deletions(-)
I'm not sure how, but this change seems to have broken the iotests.
Just one example:
$ (cd tests/qemu-iotests/ && ./check -qcow2 249)
QEMU -- "/var/home/berrange/src/virt/qemu/build/qemu-system-x86_64"
-nodefaults -display none -accel qtest
QEMU_IMG -- "/var/home/berrange/src/virt/qemu/build/qemu-img"
QEMU_IO -- "/var/home/berrange/src/virt/qemu/build/qemu-io" --cache
writeback --aio threads -f qcow2
QEMU_NBD -- "/var/home/berrange/src/virt/qemu/build/qemu-nbd"
IMGFMT-- qcow2
IMGPROTO -- file
PLATFORM -- Linux/x86_64 toolbox 6.6.12-200.fc39.x86_64
TEST_DIR --
/var/home/berrange/src/virt/qemu/build/tests/qemu-iotests/scratch
SOCK_DIR -- /tmp/qemu-iotests-0t8h94bu
GDB_OPTIONS --
VALGRIND_QEMU --
PRINT_QEMU_OUTPUT --
249 fail [15:39:25] [15:39:25] 0.2s (last: 0.4s) failed, exit
status 1
--- /var/home/berrange/src/virt/qemu/tests/qemu-iotests/249.out
+++
/var/home/berrange/src/virt/qemu/build/tests/qemu-iotests/scratch/qcow2-file-249/249.out.bad
@@ -1,47 +1,7 @@
QA output created by 249
Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=1048576
-Formatting 'TEST_DIR/t.IMGFMT.int', fmt=IMGFMT size=1048576
backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
backing_file=TEST_DIR/t.IMGFMT.int backing_fmt=IMGFMT
-{ 'execute': 'qmp_capabilities' }
-{"return": {}}
...snip
-*** done
+qemu-img: Invalid image size specified. You may use k, M, G, T, P or E
suffixes for
+qemu-img: kilobytes, megabytes, gigabytes, terabytes, petabytes and exabytes.
+qemu-img: Invalid image size specified. You may use k, M, G, T, P or E
suffixes for
+qemu-img: kilobytes, megabytes, gigabytes, terabytes, petabytes and exabytes.
+Timeout waiting for capabilities on handle 0
Failures: 249
Failed 1 of 1 iotests
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 28/28] qemu-img: extend cvtnum() and use it in more places
On Thu, Feb 22, 2024 at 12:16:09AM +0300, Michael Tokarev wrote: > cvtnum() expects input string to specify some sort of size > (optionally with KMG... suffix). However, there are a lot > of other number conversions in there (using qemu_strtol ), > also, not all conversions which use cvtnum, actually expects > size, - like dd count=nn. > > Add bool issize argument to cvtnum() to specify if it should > treat the argument as a size or something else, - this changes > conversion routine in use and error text. > > Use the new cvtnum() in more places (like where strtol were used), > since it never return negative number in successful conversion. > When it makes sense, also specify upper or lower bounds at the > same time. This simplifies option processing in multiple places, > removing the need of local temporary variables and longer error > reporting code. > > While at it, fix errors, like depth in measure must be >= 1, > while the previous code allowed it to be 0. > > In a few places, change unsigned variables (like of type size_t) > to be signed instead, - to avoid the need of temporary conversion > variable. All these variables are okay to be signed, we never > assign <0 value to them except of the cases of conversion error, > where we return immediately. > > Signed-off-by: Michael Tokarev > --- > qemu-img.c | 118 --------- > 1 file changed, 44 insertions(+), 74 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 27/28] qemu-img: inline list of supported commands, remove qemu-img-cmds.h include
On Thu, Feb 22, 2024 at 12:16:08AM +0300, Michael Tokarev wrote: > also add short description to each command and use it in --help > > Signed-off-by: Michael Tokarev > --- > qemu-img.c | 41 ++--- > 1 file changed, 34 insertions(+), 7 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 19/28] qemu-img: resize: do not always eat last argument
On Thu, Feb 22, 2024 at 12:16:00AM +0300, Michael Tokarev wrote:
> 'qemu-img resize --help' does not work, since it wants more
> arguments. Also it -size is only recognized as a very last
> argument, but it is common for tools to handle other options
> after positional arguments too.
>
> Tell getopt_long() to return non-options together with options,
> and process filename and size in the loop, and check if there's
> an argument right after filename which looks like -N (number),
> and treat it as size (decrement). This way we can handle --help,
> and we can also have options after filename and size, and `--'
> will be handled fine too.
>
> The only case which is not handled right is when there's an option
> between filename and size, and size is given as decrement, - in
> this case -size will be treated as option, not as size.
>
> Signed-off-by: Michael Tokarev
> ---
> qemu-img.c | 41 +++--
> 1 file changed, 27 insertions(+), 14 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 2a4bff2872..c8b0b68d67 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -4296,7 +4296,7 @@ static int img_resize(const img_cmd_t *ccmd, int argc,
> char **argv)
> {
> Error *err = NULL;
> int c, ret, relative;
> -const char *filename, *fmt, *size;
> +const char *filename = NULL, *fmt = NULL, *size = NULL;
> int64_t n, total_size, current_size;
> bool quiet = false;
> BlockBackend *blk = NULL;
> @@ -4319,17 +4319,7 @@ static int img_resize(const img_cmd_t *ccmd, int argc,
> char **argv)
> bool image_opts = false;
> bool shrink = false;
>
> -/* Remove size from argv manually so that negative numbers are not
> treated
> - * as options by getopt. */
> -if (argc < 3) {
> -error_exit(argv[0], "Not enough arguments");
> -return 1;
> -}
> -
> -size = argv[--argc];
> -
> /* Parse getopt arguments */
> -fmt = NULL;
> for(;;) {
> static const struct option long_options[] = {
> {"help", no_argument, 0, 'h'},
> @@ -4339,7 +4329,7 @@ static int img_resize(const img_cmd_t *ccmd, int argc,
> char **argv)
> {"shrink", no_argument, 0, OPTION_SHRINK},
> {0, 0, 0, 0}
> };
> -c = getopt_long(argc, argv, ":f:hq",
> +c = getopt_long(argc, argv, "-:f:hq",
In other patches you removed the initial ':' from gopt_long arg strings.
> long_options, NULL);
> if (c == -1) {
> break;
> @@ -4377,12 +4367,35 @@ static int img_resize(const img_cmd_t *ccmd, int
> argc, char **argv)
> case OPTION_SHRINK:
> shrink = true;
> break;
> +case 1: /* a non-optional argument */
> +if (!filename) {
> +filename = optarg;
> +/* see if we have -size (number) next to filename */
> +if (optind < argc) {
> +size = argv[optind];
> +if (size[0] == '-' && size[1] >= '0' && size[1] <= '9') {
> +++optind;
> +} else {
> +size = NULL;
> +}
> +}
> +} else if (!size) {
> +size = optarg;
> +} else {
> +error_exit(argv[0], "Extra argument(s) in command line");
> +}
> +break;
Can you say what scenario exercises this code 'case 1' ? I couldn't
get it to run in any scenarios i tried, and ineed removing this,
and removing the 'getopt_long' change, I could still run 'qemu-img resize
--help'
OK, and also run 'qemu-img resize foo -43' too.
> }
> }
> -if (optind != argc - 1) {
> +if (!filename && optind < argc) {
> +filename = argv[optind++];
> +}
> +if (!size && optind < argc) {
> +size = argv[optind++];
> +}
> +if (!filename || !size || optind < argc) {
> error_exit(argv[0], "Expecting image file name and size");
> }
> -filename = argv[optind++];
>
> /* Choose grow, shrink, or absolute resize mode */
> switch (size[0]) {
> --
> 2.39.2
>
>
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 26/28] qemu-img: implement short --help, remove global help() function
On Thu, Feb 22, 2024 at 12:16:07AM +0300, Michael Tokarev wrote: > now once all individual subcommands has --help support, remove > the large unreadable help() thing and replace it with small > global --help, which refers to individual command --help for > more info. > > While at it, also line-wrap list of formats after 75 chars. > > Since missing_argument() and unrecognized_option() are now unused, > remove them. > > Signed-off-by: Michael Tokarev > --- > qemu-img.c | 172 - > 1 file changed, 39 insertions(+), 133 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 16/28] qemu-img: snapshot: make -l (list) the default, simplify option handling
On Thu, Feb 22, 2024 at 12:15:57AM +0300, Michael Tokarev wrote: > When no -l/-a/-c/-d specified, assume -l (list). > > Use the same values for SNAPSHOT_LIST/etc constants as the > option chars (lacd), this makes it possible to simplify > option handling a lot, combining cases for 4 options into > one. > > Also remove bdrv_oflags handling (only list can use RO mode). > > Signed-off-by: Michael Tokarev > --- > docs/tools/qemu-img.rst | 2 +- > qemu-img.c | 52 ++--- > 2 files changed, 19 insertions(+), 35 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 06/28] qemu-img: create: refresh options/--help
On Thu, Feb 22, 2024 at 12:15:47AM +0300, Michael Tokarev wrote:
> Create helper function cmd_help() to display command-specific
> help text, and use it to print --help for 'create' subcommand.
>
> Add missing long options (eg --format) in img_create().
>
> Remove usage of missing_argument()/unrecognized_option() in
> img_create().
>
> Signed-off-by: Michael Tokarev
> ---
> qemu-img.c | 68 +++---
> 1 file changed, 60 insertions(+), 8 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 38ac0f1845..7e4c993b9c 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -132,6 +132,31 @@ void unrecognized_option(const char *option)
> error_exit("qemu-img", "unrecognized option '%s'", option);
> }
>
> +/*
> + * Print --help output for a command and exit.
> + * syntax and description are multi-line with trailing EOL
> + * (to allow easy extending of the text)
> + * syntax has each subsequent line indented by 8 chars.
> + * desrciption is indented by 2 chars for argument on each own line,
> + * and with 5 chars for argument description (like -h arg below).
> + */
> +static G_NORETURN
> +void cmd_help(const img_cmd_t *ccmd,
> + const char *syntax, const char *arguments)
> +{
> +printf(
> +"Usage:\n"
> +" %s %s %s"
For the global help there's an extra '\n' after 'Usage'. It would be
good go be consistent in this between global and per-command help.
$ ./build/qemu-img --help
qemu-img version 8.2.50 (v8.2.0-1677-g81b20f4b55)
Copyright (c) 2003-2023 Fabrice Bellard and the QEMU Project developers
QEMU disk image utility. Usage:
qemu-img [standard options] COMMAND [--help | command options]
...snip...
vs
$ ./build/qemu-img info --help
Display information about image. Usage:
qemu-img info [-f FMT | --image-opts] [-b] [-U] [--object OBJDEF]
[--output human|json] FILENAME
...snip...
I wonder if we should repeat '[standard options]' for the
per-command help too ?
> +"\n"
> +"Arguments:\n"
In the global help you called it 'Standard options', so for
consistency lets use 'Options:' here too.
> +" -h, --help\n"
> +" print this help and exit\n"
> +"%s\n",
> + "qemu-img", ccmd->name,
> + syntax, arguments);
> +exit(EXIT_SUCCESS);
> +}
> +
> /* Please keep in synch with docs/tools/qemu-img.rst */
> static G_NORETURN
> void help(void)
> @@ -530,23 +555,48 @@ static int img_create(const img_cmd_t *ccmd, int argc,
> char **argv)
> for(;;) {
> static const struct option long_options[] = {
> {"help", no_argument, 0, 'h'},
> +{"quiet", no_argument, 0, 'q'},
> {"object", required_argument, 0, OPTION_OBJECT},
> +{"format", required_argument, 0, 'f'},
> +{"backing", required_argument, 0, 'b'},
> +{"backing-format", required_argument, 0, 'F'},
> +{"backing-unsafe", no_argument, 0, 'u'},
> +{"options", required_argument, 0, 'o'},
> {0, 0, 0, 0}
> };
> -c = getopt_long(argc, argv, ":F:b:f:ho:qu",
> +c = getopt_long(argc, argv, "F:b:f:ho:qu",
> long_options, NULL);
> if (c == -1) {
> break;
> }
> switch(c) {
> -case ':':
> -missing_argument(argv[optind - 1]);
> -break;
> -case '?':
> -unrecognized_option(argv[optind - 1]);
> -break;
> case 'h':
> -help();
> +cmd_help(ccmd,
> +"[-f FMT] [-o FMT_OPTS] [-b BACKING_FILENAME [-F BACKING_FMT]]\n"
> +"[--object OBJDEF] [-u] FILENAME [SIZE[bkKMGTPE]]\n"
> +,
> +" -q, --quiet\n"
> +" quiet operations\n"
> +" -f, --format FMT\n"
> +" specifies format of the new image, default is raw\n"
> +" -o, --options FMT_OPTS\n"
> +" format-specific options ('-o list' for list)\n"
> +" -b, --backing BACKING_FILENAME\n"
> +" stack new image on top of BACKING_FILENAME\n"
> +" (for formats which support stacking)\n"
> +" -F, --backing-format BACKING_FMT\n"
> +" specify format of BACKING_FILENAME\n"
> +" -u, --backing-unsafe\n"
> +" do not fail if BACKING_FMT can not be read\n"
> +" --object OBJDEF\n"
> +" QEMU user-creatable object (eg encryption key)\n"
> +" FILENAME\n"
> +" image file to create. It will be overridden if exists\n"
> +" SIZE\n"
> +" image size with optional suffix (multiplies in 1024)\n"
> +" SIZE is required unless BACKING_IMG is specified,\n"
> +" in which case it will be the same as size of BACKING_IMG\n"
> +);
> break;
> case 'F':
> base_fmt = optarg;
> @@ -571,6 +621,8 @@ static int img_create(const img_cmd_t *ccmd, int argc,
> char **argv)
> case OPTION_OBJECT:
> user_creatable_process_cmdline(optarg);
> break;
> +default:
> +tryhelp(argv[0]);
> }
> }
>
> --
> 2.39.2
>
>
With
Re: [PATCH 05/28] qemu-img: pass current cmd info into command handlers
On Thu, Feb 22, 2024 at 12:15:46AM +0300, Michael Tokarev wrote: > This info will be used to generate --help output. > > Signed-off-by: Michael Tokarev > --- > qemu-img.c | 34 +- > 1 file changed, 17 insertions(+), 17 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 04/28] qemu-img: global option processing and error printing
On Thu, Feb 22, 2024 at 12:15:45AM +0300, Michael Tokarev wrote:
> In order to correctly print executable name in various
> error messages, pass argv[0] to error_exit() function.
> This way, error messages will refer to actual executable
> name, which may be different from 'qemu-img'.
>
> For subcommands, pass whole argv[] array, so argv[0] is
> the executable name, not subcommand name. In order to
> do that, avoid resetting optind but continue with the
> next option. Also don't require at least 3 options on
> the command line: it makes no sense with options before
> subcommand.
>
> Before invoking a subcommand, replace argv[0] to include
> the subcommand name.
>
> Introduce tryhelp() function which just prints
>
> try 'command-name --help' for more info
>
> and exits. When tryhelp() is called from within a subcommand
> handler, the message will look like:
>
> try 'command-name subcommand --help' for more info
>
> qemu-img uses getopt_long() with ':' as the first char in
> optstring parameter, which means it doesn't print error
> messages but return ':' or '?' instead, and qemu-img uses
> unrecognized_option() or missing_argument() function to
> print error messages. But it doesn't quite work:
>
> $ ./qemu-img -xx
> qemu-img: unrecognized option './qemu-img'
>
> so the aim is to let getopt_long() to print regular error
> messages instead (removing ':' prefix from optstring) and
> remove handling of '?' and ':' "options" entirely. With
> concatenated argv[0] and the subcommand, it all finally
> does the right thing in all cases. This will be done in
> subsequent changes command by command, with main() done
> last.
>
> unrecognized_option() and missing_argument() functions
> prototypes aren't changed by this patch, since they're
> called from many places and will be removed a few patches
> later. Only artifical "qemu-img" argv0 is provided in
> there for now.
>
> Signed-off-by: Michael Tokarev
> ---
> qemu-img.c | 75 +++---
> 1 file changed, 38 insertions(+), 37 deletions(-)
> @@ -5602,10 +5602,11 @@ int main(int argc, char **argv)
> /* find the command */
> for (cmd = img_cmds; cmd->name != NULL; cmd++) {
> if (!strcmp(cmdname, cmd->name)) {
> +argv[0] = g_strdup_printf("%s %s", argv[0], cmdname);
> return cmd->handler(argc, argv);
This is going to result in valgrind warning that argv[0] is leaked.
How about:
g_autofree char *cmdargv0 = g_strdup_printf("%s %s", argv[0], cmdname);
argv[0] = cmdargv0;
return cmd->handler(argc, argv);
> }
> }
>
> /* not found */
> -error_exit("Command not found: %s", cmdname);
> +error_exit(argv[0], "Command not found: %s", cmdname);
> }
> --
> 2.39.2
>
>
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 02/28] qemu-img: measure: convert img_size to signed, simplify handling
On Thu, Feb 22, 2024 at 12:15:43AM +0300, Michael Tokarev wrote: > qemu_opt_set_number() expects signed int64_t. > > Use int64_t instead of uint64_t for img_size, use -1 as "unset" > value instead of UINT64_MAX, and do not require temporary sval > for conversion from string. > > Signed-off-by: Michael Tokarev > --- > qemu-img.c | 19 +++ > 1 file changed, 7 insertions(+), 12 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 03/28] qemu-img: create: convert img_size to signed, simplify handling
On Thu, Feb 22, 2024 at 12:15:44AM +0300, Michael Tokarev wrote: > Initializing an unsigned as -1, or using temporary > sval for conversion is awkward. Since we don't allow > other "negative" values anyway, use signed value and > pass it to bdrv_img_create() (where it is properly > converted to unsigned), simplifying code. > > Signed-off-by: Michael Tokarev > --- > qemu-img.c | 9 +++-- > 1 file changed, 3 insertions(+), 6 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 01/28] qemu-img: stop printing error twice in a few places
On Thu, Feb 22, 2024 at 12:15:42AM +0300, Michael Tokarev wrote: > Currently we have: > > ./qemu-img resize none +10 > qemu-img: Could not open 'none': Could not open 'none': No such file or > directory > > stop printing the message twice, - local_err already has > all the info, no need to prepend additional text there. > > There are a few other places like this, but I'm unsure > about these. > > Signed-off-by: Michael Tokarev > --- > qemu-img.c | 8 +++- > 1 file changed, 3 insertions(+), 5 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 23/23] qemu-img: inline list of supported commands, remove qemu-img-cmds.h include
On Wed, Feb 21, 2024 at 07:31:42PM +0300, Michael Tokarev wrote:
> 20.02.2024 21:48, Daniel P. Berrangé:
>
> > This ends up looking a bit muddled together. I don't think we
> > need repeat 'qemu-img ' twice, and could add a little
> > more whitespace
> >
> > eg instead of:
> >
> > $ ./build/qemu-img check --help
> > qemu-img check: Check basic image integrity. Usage:
> > qemu-img check [-f FMT | --image-opts] [-T CACHE_MODE] [-r] [-u]
> > [--output human|json] [--object OBJDEF] FILENAME
> > Arguments:
> > ...snip...
> >
> > have it look like
> >
> > $ ./build/qemu-img check --help
> > Check basic image integrity.
> >
> > Usage:
> >
> >qemu-img check [-f FMT | --image-opts] [-T CACHE_MODE] [-r] [-u]
> > [--output human|json] [--object OBJDEF] FILENAME
> >
> > Arguments:
> > ...snip...
>
> Here's the current way how `create' help text looks like:
>
> $ ./qemu-img create --help
> Create and format qemu image file. Usage:
> qemu-img create [-f FMT] [-o FMT_OPTS] [-b BACKING_FILENAME [-F
> BACKING_FMT]]
> [--object OBJDEF] [-u] FILENAME [SIZE[bkKMGTPE]]
> Arguments:
> -h, --help
> print this help and exit
> -q, --quiet
> quiet operations
> -f, --format FMT
> specifies format of the new image, default is raw
> -o, --options FMT_OPTS
> format-specific options ('-o list' for list)
> -b, --backing BACKING_FILENAME
> stack new image on top of BACKING_FILENAME
> (for formats which support stacking)
> -F, --backing-format BACKING_FMT
> specify format of BACKING_FILENAME
> -u, --backing-unsafe
> do not fail if BACKING_FMT can not be read
> --object OBJDEF
> QEMU user-creatable object (eg encryption key)
> FILENAME
> image file to create. It will be overridden if exists
> SIZE
> image size with optional suffix (multiplies in 1024)
> SIZE is required unless BACKING_IMG is specified,
> in which case it will be the same as size of BACKING_IMG
>
> Maybe it's a good idea to add newlines around the "syntax" part,
> ie, after "Usage:" and before "Arguments:". I don't think it needs
> extra newlines between each argument description though, - this way
> it becomes just too long.
>
> What do you think?
I still prefer to have more vertical whitespace, as I find it harder
to read through without it. I was using the typical man page option
/ usage formatting as a guide in my feedback.
Still, it would be useful to see what other maintainers think, as I'm
just one data point.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 23/23] qemu-img: inline list of supported commands, remove qemu-img-cmds.h include
On Tue, Feb 20, 2024 at 10:02:32PM +0300, Michael Tokarev wrote: > 20.02.2024 21:48, Daniel P. Berrangé: > ... > > $ ./build/qemu-img check --help > > Check basic image integrity. > > > > Usage: > > > >qemu-img check [-f FMT | --image-opts] [-T CACHE_MODE] [-r] [-u] > > [--output human|json] [--object OBJDEF] FILENAME > > > > Arguments: > > $ ./build/qemu-img check --help > Check basic image integrity. Usage: > >qemu-img check [-f FMT | --image-opts] [-T CACHE_MODE] [-r] [-u] > [--output human|json] [--object OBJDEF] FILENAME > > Arguments: > ... > > Or just: > > Check basic image integrity: > > qemu-img check... > > > In all cases I tried to make the whole thing as compact as possible, > to (almost) fit on a standard terminal. The extra empty lines between > different arguments makes it almost impossible. IMHO fitting on a "standard" terminal is OK in terms of width, but should be a non-goal in terms of height. Readability is more important than avoiding vertical scroll. > > > "Arguments:\n" > > > " -h|--help - print this help and exit\n" > > btw, the common way is to use comma here, not "|", -- > -h,--help - ... > > Again, I especially omitted space after "|" to make it > more compact. Maybe for no good. Yes, a comma with a space would look nicer. If we have the description on the following line, then there's no width limit problems there. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 00/23] qemu-img: refersh options and --help handling
On Sat, Feb 10, 2024 at 12:22:21AM +0300, Michael Tokarev wrote: > Quite big patchset implementing normal, readable qemu-img --help > (and qemu-img COMMAND --help) output with readable descriptions, > and adding many long options in the process. > > In the end I stopped using qemu-img-opts.hx in qemu-img.c, perhaps > this can be avoided, with only list of commands and their desrciptions > kept there, but I don't see big advantage here. The same list should > be included in docs/tools/qemu-img.rst, - this is not done now. I think it'd be nice for qemu-img.c to be the canonical source of truth, so we have the getopt short arg, long args, and help all in the same place & thus much less likely to get out of sync. Thus I like the approach you've taken here to stop using the .hx file. As a later work, it wouldn't be too terrible to have a python script that parses qemu-img.c to look for 'cmd_help(...)' calls and extra the help text, which could be used to feed into the qemu-img.rxt man page generation, thus fully eliminating the .hx file. > > Also each command syntax isn't reflected in the doc for now, because > I want to give good names for options first, - and there, we've quite > some inconsistences and questions. For example, measure --output=OFMT > -O OFMT, - this is priceless :) I've no idea why we have this ugly > --output=json thing, why not have --json? ;) I gave the desired > format long name --target-format to avoid clash with --output. > > For rebase, src vs tgt probably should be renamed in local variables > too, and I'm not even sure I've got the caches right. For caches, > the thing is inconsistent across commands. > > For compare, I used --a-format/--b-format (for -f/-F), - this can > be made --souce-format and --target-format, to compare source (file1) > with target (file2). > > For bitmap, things are scary, I'm not sure what -b SRC_FILENAME > really means, - for now I gave it --source option, but this does > not make it more clear, suggestions welcome. > > There are many other inconsistencies, I can't fix them all in one > go.. :) This is already a massive improvement on the status quo. My comments were mostly around whitespace/layout tweaks to the help output. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 23/23] qemu-img: inline list of supported commands, remove qemu-img-cmds.h include
On Sat, Feb 10, 2024 at 12:22:44AM +0300, Michael Tokarev wrote:
> also add short description to each command and use it in --help
>
> Signed-off-by: Michael Tokarev
> ---
> qemu-img.c | 42 +++---
> 1 file changed, 35 insertions(+), 7 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index d9c5c6078b..e57076738e 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -61,6 +61,7 @@
> typedef struct img_cmd_t {
> const char *name;
> int (*handler)(const struct img_cmd_t *ccmd, int argc, char **argv);
> +const char *description;
> } img_cmd_t;
>
> enum {
> @@ -130,11 +131,12 @@ static G_NORETURN
> void cmd_help(const img_cmd_t *ccmd,
>const char *syntax, const char *arguments)
> {
> -printf("qemu-img %s %s"
> +printf("qemu-img %s: %s. Usage:\n"
> + "qemu-img %s %s"
This ends up looking a bit muddled together. I don't think we
need repeat 'qemu-img ' twice, and could add a little
more whitespace
eg instead of:
$ ./build/qemu-img check --help
qemu-img check: Check basic image integrity. Usage:
qemu-img check [-f FMT | --image-opts] [-T CACHE_MODE] [-r] [-u]
[--output human|json] [--object OBJDEF] FILENAME
Arguments:
...snip...
have it look like
$ ./build/qemu-img check --help
Check basic image integrity.
Usage:
qemu-img check [-f FMT | --image-opts] [-T CACHE_MODE] [-r] [-u]
[--output human|json] [--object OBJDEF] FILENAME
Arguments:
...snip...
> "Arguments:\n"
> " -h|--help - print this help and exit\n"
> "%s",
> - ccmd->name, syntax, arguments);
> + ccmd->name, ccmd->description, ccmd->name, syntax, arguments);
> exit(EXIT_SUCCESS);
> }
>
> @@ -5746,10 +5748,36 @@ out:
> }
>
> static const img_cmd_t img_cmds[] = {
> -#define DEF(option, callback, arg_string)\
> -{ option, callback },
> -#include "qemu-img-cmds.h"
> -#undef DEF
> +{ "amend", img_amend,
> + "Update format-specific options of the image" },
> +{ "bench", img_bench,
> + "Run simple image benchmark" },
> +{ "bitmap", img_bitmap,
> + "Perform modifications of the persistent bitmap in the image" },
> +{ "check", img_check,
> + "Check basic image integrity" },
> +{ "commit", img_commit,
> + "Commit image to its backing file" },
> +{ "compare", img_compare,
> + "Check if two images have the same contents" },
> +{ "convert", img_convert,
> + "Copy one image to another with optional format conversion" },
> +{ "create", img_create,
> + "Create and format new image file" },
> +{ "dd", img_dd,
> + "Copy input to output with optional format conversion" },
> +{ "info", img_info,
> + "Display information about image" },
> +{ "map", img_map,
> + "Dump image metadata" },
> +{ "measure", img_measure,
> + "Calculate file size requred for a new image" },
> +{ "rebase", img_rebase,
> + "Change backing file of the image" },
> +{ "resize", img_resize,
> + "Resize the image to the new size" },
> +{ "snapshot", img_snapshot,
> + "List or manipulate snapshots within image" },
> { NULL, NULL, },
> };
>
> @@ -5813,7 +5841,7 @@ QEMU_IMG_VERSION
> " [[enable=]][,events=][,file=]\n"
> "Recognized commands (run qemu-img command --help for command-specific
> help):\n");
> for (cmd = img_cmds; cmd->name != NULL; cmd++) {
> -printf(" %s\n", cmd->name);
> +printf(" %s - %s\n", cmd->name, cmd->description);
> }
> c = printf("Supported image formats:");
> bdrv_iterate_format(format_print, , false);
> --
> 2.39.2
>
>
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 22/23] qemu-img: implement short --help, remove global help() function
On Sat, Feb 10, 2024 at 12:22:43AM +0300, Michael Tokarev wrote:
> now once all individual subcommands has --help support, remove
> the large unreadable help() thing and replace it with small
> global --help, which refers to individual command --help for
> more info.
>
> While at it, also line-wrap list of formats after 74 chars.
>
> Signed-off-by: Michael Tokarev
> ---
> qemu-img.c | 148 +++--
> 1 file changed, 30 insertions(+), 118 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index e2c8855ff5..d9c5c6078b 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -94,11 +94,6 @@ typedef enum OutputFormat {
> /* Default to cache=writeback as data integrity is not important for
> qemu-img */
> #define BDRV_DEFAULT_CACHE "writeback"
>
> -static void format_print(void *opaque, const char *name)
> -{
> -printf(" %s", name);
> -}
> -
> static G_NORETURN G_GNUC_PRINTF(2, 3)
> void error_exit(const img_cmd_t *ccmd, const char *fmt, ...)
> {
> @@ -154,114 +149,6 @@ static OutputFormat parse_output_format(const img_cmd_t
> *ccmd, const char *arg)
> }
> }
>
> -/* Please keep in synch with docs/tools/qemu-img.rst */
> -static G_NORETURN
> -void help(void)
> -{
> -const char *help_msg =
> - QEMU_IMG_VERSION
> - "usage: qemu-img [standard options] command [command options]\n"
> - "QEMU disk image utility\n"
> - "\n"
> - "'-h', '--help' display this help and exit\n"
> - "'-V', '--version'output version information and exit\n"
> - "'-T', '--trace'
> [[enable=]][,events=][,file=]\n"
> - " specify tracing options\n"
> - "\n"
> - "Command syntax:\n"
> -#define DEF(option, callback, arg_string)\
> - " " arg_string "\n"
> -#include "qemu-img-cmds.h"
> -#undef DEF
> - "\n"
> - "Command parameters:\n"
> - " 'filename' is a disk image filename\n"
> - " 'objectdef' is a QEMU user creatable object definition. See
> the qemu(1)\n"
> - "manual page for a description of the object properties. The
> most common\n"
> - "object type is a 'secret', which is used to supply passwords
> and/or\n"
> - "encryption keys.\n"
> - " 'fmt' is the disk image format. It is guessed automatically in
> most cases\n"
> - " 'cache' is the cache mode used to write the output disk image,
> the valid\n"
> - "options are: 'none', 'writeback' (default, except for
> convert), 'writethrough',\n"
> - "'directsync' and 'unsafe' (default for convert)\n"
> - " 'src_cache' is the cache mode used to read input disk images,
> the valid\n"
> - "options are the same as for the 'cache' option\n"
> - " 'size' is the disk image size in bytes. Optional suffixes\n"
> - "'k' or 'K' (kilobyte, 1024), 'M' (megabyte, 1024k), 'G'
> (gigabyte, 1024M),\n"
> - "'T' (terabyte, 1024G), 'P' (petabyte, 1024T) and 'E'
> (exabyte, 1024P) are\n"
> - "supported. 'b' is ignored.\n"
> - " 'output_filename' is the destination disk image filename\n"
> - " 'output_fmt' is the destination format\n"
> - " 'options' is a comma separated list of format specific options
> in a\n"
> - "name=value format. Use -o help for an overview of the
> options supported by\n"
> - "the used format\n"
> - " 'snapshot_param' is param used for internal snapshot, format\n"
> - "is 'snapshot.id=[ID],snapshot.name=[NAME]', or\n"
> - "'[ID_OR_NAME]'\n"
> - " '-c' indicates that target image must be compressed (qcow
> format only)\n"
> - " '-u' allows unsafe backing chains. For rebasing, it is assumed
> that old and\n"
> - " new backing file match exactly. The image doesn't need a
> working\n"
> - " backing file before rebasing in this case (useful for
> renaming the\n"
> - " backing file). For image creation, allow creating without
> attempting\n"
> - " to open the backing file.\n"
> - " '-h' with or without a command shows this help and lists the
> supported formats\n"
> - " '-p' show progress of command (only certain commands)\n"
> - " '-q' use Quiet mode - do not print any output (except
> errors)\n"
> - " '-S' indicates the consecutive number of bytes (defaults to
> 4k) that must\n"
> - " contain only zeros for qemu-img to create a sparse image
> during\n"
> - " conversion. If the number of bytes is 0, the source will
> not be scanned for\n"
> - " unallocated or zero sectors, and the destination image
> will always be\n"
> - " fully allocated\n"
> -
Re: [PATCH 02/23] qemu-img: refresh options/--help for "create" subcommand
On Sat, Feb 10, 2024 at 12:22:23AM +0300, Michael Tokarev wrote:
> Add missing long options (eg --format).
>
> Create helper function cmd_help() to display command-specific
> help text, and use it to print --help for 'create' subcommand.
>
> Signed-off-by: Michael Tokarev
> ---
> qemu-img.c | 45 -
> 1 file changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 05f80b6e5b..7edfc56572 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -126,6 +126,25 @@ void unrecognized_option(const img_cmd_t *ccmd, const
> char *option)
> error_exit(ccmd, "unrecognized option '%s'", option);
> }
>
> +/*
> + * Print --help output for a command and exit.
> + * syntax and description are multi-line with trailing EOL
> + * (to allow easy extending of the text)
> + * syntax has each subsequent line starting with \t
> + * desrciption is indented by one char
> + */
> +static G_NORETURN
> +void cmd_help(const img_cmd_t *ccmd,
> + const char *syntax, const char *arguments)
> +{
> +printf("qemu-img %s %s"
I think we want an extra "\n" before & after 'Arguments:'
> + "Arguments:\n"
> + " -h|--help - print this help and exit\n"
> + "%s",
> + ccmd->name, syntax, arguments);
> +exit(EXIT_SUCCESS);
> +}
> +
> /* Please keep in synch with docs/tools/qemu-img.rst */
> static G_NORETURN
> void help(void)
> @@ -524,7 +543,13 @@ static int img_create(const img_cmd_t *ccmd, int argc,
> char **argv)
> for(;;) {
> static const struct option long_options[] = {
> {"help", no_argument, 0, 'h'},
> +{"quiet", no_argument, 0, 'q'},
> {"object", required_argument, 0, OPTION_OBJECT},
> +{"format", required_argument, 0, 'f'},
> +{"backing", required_argument, 0, 'b'},
> +{"backing-format", required_argument, 0, 'F'},
> +{"backing-unsafe", no_argument, 0, 'u'},
> +{"options", required_argument, 0, 'o'},
> {0, 0, 0, 0}
> };
> c = getopt_long(argc, argv, ":F:b:f:ho:qu",
> @@ -540,7 +565,25 @@ static int img_create(const img_cmd_t *ccmd, int argc,
> char **argv)
> unrecognized_option(ccmd, argv[optind - 1]);
> break;
> case 'h':
> -help();
> +cmd_help(ccmd,
> +"[-f FMT] [-o FMT_OPTS] [-b BACKING_FILENAME [-F BACKING_FMT]]\n"
> +"[--object OBJDEF] [-u] FILENAME [SIZE[bkKMGTPE]]\n"
> +,
> +" -q|--quiet - quiet operations\n"
> +" -f|--format FMT - specifies format of the new image, default is raw\n"
> +" -o|--options FMT_OPTS - format-specific options ('-o list' for list)\n"
> +" -b|--backing BACKING_FILENAME - stack new image on top of
> BACKING_FILENAME\n"
> +" (for formats which support stacking)\n"
> +" -F|--backing-format BACKING_FMT - specify format of BACKING_FILENAME\n"
> +" -u|--backing-unsafe - do not fail if BACKING_FMT can not be read\n"
> +" --object OBJDEF - QEMU user-creatable object (eg encryption key)\n"
> +" FILENAME - image file to create. It will be overriden if exists\n"
> +" SIZE - image size with optional suffix: 'b' (byte, default), 'k' or\n"
> +" 'K' (kilobyte, 1024b), 'M' (megabyte, 1024K), 'G' (gigabyte, 1024M),\n"
> +" 'T' (terabyte, 1024G), 'P' (petabyte, 1024T), or 'E' (exabyte, 1024P)\n"
> +" SIZE is required unless BACKING_IMG is specified, in which case\n"
> +" it will be the same as size of BACKING_IMG\n"
This comes out as a bit of a wall of dense text.
I think we should have 2 space indent for options, and a further
4 space for continuations, and also put the description on its
own line.
eg so instead of getting:
$ ./build/qemu-img create --help
qemu-img create [-f FMT] [-o FMT_OPTS] [-b BACKING_FILENAME [-F BACKING_FMT]]
[--object OBJDEF] [-u] FILENAME [SIZE[bkKMGTPE]]
Arguments:
-h|--help - print this help and exit
-q|--quiet - quiet operations
-f|--format FMT - specifies format of the new image, default is raw
-o|--options FMT_OPTS - format-specific options ('-o list' for list)
-b|--backing BACKING_FILENAME - stack new image on top of BACKING_FILENAME
(for formats which support stacking)
-F|--backing-format BACKING_FMT - specify format of BACKING_FILENAME
-u|--backing-unsafe - do not fail if BACKING_FMT can not be read
--object OBJDEF - QEMU user-creatable object (eg encryption key)
FILENAME - image file to create. It will be overriden if exists
SIZE - image size with optional suffix: 'b' (byte, default), 'k' or
'K' (kilobyte, 1024b), 'M' (megabyte, 1024K), 'G' (gigabyte, 1024M),
'T' (terabyte, 1024G), 'P' (petabyte, 1024T), or 'E' (exabyte, 1024P)
SIZE is required unless BACKING_IMG is specified, in which case
it will be the same as size of BACKING_IMG
we would get:
$ ./build/qemu-img create --help
qemu-img create [-f FMT] [-o FMT_OPTS] [-b BACKING_FILENAME [-F BACKING_FMT]]
[--object OBJDEF] [-u] FILENAME
Re: [PATCH 15/23] qemu-img: resize: do not always eat last argument
On Sat, Feb 10, 2024 at 12:22:36AM +0300, Michael Tokarev wrote:
> 'qemu-img resize --help' does not work, since it wants more arguments.
> Only eat last option at the beginning if it starts like -N.., and allow
> getopt() to do its work, and eat it up at the end if not already eaten.
> This will not allow to mix options and size anyway, but it is better
> than now.
>
> Signed-off-by: Michael Tokarev
> ---
> qemu-img.c | 15 +--
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 69d41e0a92..929a25a021 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -4271,13 +4271,13 @@ static int img_resize(const img_cmd_t *ccmd, int
> argc, char **argv)
>
> /* Remove size from argv manually so that negative numbers are not
> treated
> * as options by getopt. */
> -if (argc < 3) {
> -error_exit(ccmd, "Not enough arguments");
> -return 1;
> +if (argc > 1 && argv[argc - 1][0] == '-'
> +&& argv[argc-1][1] >= '0' && argv[argc-1][1] <= '9') {
> +size = argv[--argc];
> +} else {
> +size = NULL;
> }
We already have a variable 'int relative' that is set to '-1'
or '+1' depending on whether we have a -ve or +ve size.
I think it is clearer to follow if we just set 'relative' much
earlier before parsing by moving this chunk of code to before
the getopt:
switch (size[0]) {
case '+':
relative = 1;
size++;
break;
case '-':
relative = -1;
size++;
break;
default:
relative = 0;
break;
}
once we've done that we can simply replace the '-' with '+'
to stop getopt getting upset.
>
> -size = argv[--argc];
> -
> /* Parse getopt arguments */
> fmt = NULL;
> for(;;) {
> @@ -4329,10 +4329,13 @@ static int img_resize(const img_cmd_t *ccmd, int
> argc, char **argv)
> break;
> }
> }
> -if (optind != argc - 1) {
> +if (optind + 1 + (size == NULL) != argc) {
> error_exit(ccmd, "Expecting image file name and size");
> }
> filename = argv[optind++];
> +if (!size) {
> +size = argv[optind++];
> +}
>
> /* Choose grow, shrink, or absolute resize mode */
> switch (size[0]) {
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 12/23] qemu-img: make -l (list) the default for "snapshot" subcommand
On Sat, Feb 10, 2024 at 12:22:33AM +0300, Michael Tokarev wrote:
> also remove bdrv_oflags handling (only list can use RO mode)
> ---
> qemu-img.c | 13 -
> 1 file changed, 8 insertions(+), 5 deletions(-)
I'd suggest docs/tools/qemu-img.rst should also be updated to say
Lists all snapshots in the given image (default action)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 1e09b78d00..d9dfff2f07 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3541,7 +3541,7 @@ static int img_snapshot(const img_cmd_t *ccmd, int
> argc, char **argv)
> BlockDriverState *bs;
> QEMUSnapshotInfo sn;
> char *filename, *fmt = NULL, *snapshot_name = NULL;
> -int c, ret = 0, bdrv_oflags;
> +int c, ret = 0;
> int action = 0;
> bool quiet = false;
> Error *err = NULL;
> @@ -3549,7 +3549,6 @@ static int img_snapshot(const img_cmd_t *ccmd, int
> argc, char **argv)
> bool force_share = false;
> int64_t rt;
>
> -bdrv_oflags = BDRV_O_RDWR;
> /* Parse commandline parameters */
> for(;;) {
> static const struct option long_options[] = {
> @@ -3583,7 +3582,6 @@ static int img_snapshot(const img_cmd_t *ccmd, int
> argc, char **argv)
> return 0;
> }
> action = SNAPSHOT_LIST;
> -bdrv_oflags &= ~BDRV_O_RDWR; /* no need for RW */
> break;
> case 'a':
> if (action) {
> @@ -3629,9 +3627,14 @@ static int img_snapshot(const img_cmd_t *ccmd, int
> argc, char **argv)
> }
> filename = argv[optind++];
>
> +if (!action) {
> +action = SNAPSHOT_LIST;
> +}
> +
> /* Open the image */
> -blk = img_open(image_opts, filename, fmt, bdrv_oflags, false, quiet,
> - force_share);
> +blk = img_open(image_opts, filename, fmt,
> + action == SNAPSHOT_LIST ? 0 : BDRV_O_RDWR,
> + false, quiet, force_share);
> if (!blk) {
> return 1;
> }
> --
> 2.39.2
>
>
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 11/23] qemu-img: allow specifying -f fmt for snapshot subcommand
On Sat, Feb 10, 2024 at 12:22:32AM +0300, Michael Tokarev wrote: > For consistency with other commands, and since it already > accepts --image-opts, allow specifying -f fmt too. > > Signed-off-by: Michael Tokarev > --- > docs/tools/qemu-img.rst | 2 +- > qemu-img-cmds.hx| 4 ++-- > qemu-img.c | 9 ++--- > 3 files changed, 9 insertions(+), 6 deletions(-) With the fix you already mentioned for getopt: Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 05/23] qemu-img: simplify --repair error message
On Sat, Feb 10, 2024 at 12:22:26AM +0300, Michael Tokarev wrote:
> Signed-off-by: Michael Tokarev
> ---
> qemu-img.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 3ae07bfae0..ad7fa033b1 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -843,8 +843,8 @@ static int img_check(const img_cmd_t *ccmd, int argc,
> char **argv)
> } else if (!strcmp(optarg, "all")) {
> fix = BDRV_FIX_LEAKS | BDRV_FIX_ERRORS;
> } else {
> -error_exit(ccmd, "Unknown option value for -r "
> - "(expecting 'leaks' or 'all'): %s", optarg);
> +error_exit(ccmd,
> + "--repair expects 'leaks' or 'all' not '%s'",
> optarg);
> }
Should we say '--repair/-r expects...' since we don't know which the
user passed
Either way
Reviewed-by: Daniel P. Berrangé
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 03/23] qemu-img: factor out parse_output_format() and use it in the code
On Sat, Feb 10, 2024 at 12:22:24AM +0300, Michael Tokarev wrote: > Use common code and simplify error message > > Signed-off-by: Michael Tokarev > --- > qemu-img.c | 63 -- > 1 file changed, 18 insertions(+), 45 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 01/23] qemu-img: pass current cmd info into command handlers
On Sat, Feb 10, 2024 at 12:22:22AM +0300, Michael Tokarev wrote: > In order to be able to give correct --help output, pass current cmd > information (img_cmd_t structure) to command handlers and to common > error reporting functions. After the change, in case of command-line > error, qemu-img will now print: > > Try 'qemu-img create --help' for more information. > > Current cmd info will be useful in --help output as well. > > Signed-off-by: Michael Tokarev > --- > qemu-img.c | 150 ++--- > 1 file changed, 75 insertions(+), 75 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH] iotests: adapt to output change for recently introduced 'detached header' field
On Fri, Feb 16, 2024 at 11:14:15AM +0100, Fiona Ebner wrote:
> Failure was noticed when running the tests for the qcow2 image format.
>
> Fixes: 0bd779e27e ("crypto: Introduce 'detached-header' field in
> QCryptoBlockInfoLUKS")
> Signed-off-by: Fiona Ebner
> ---
> tests/qemu-iotests/198.out | 2 ++
> tests/qemu-iotests/206.out | 1 +
> 2 files changed, 3 insertions(+)
Reviewed-by: Daniel P. Berrangé
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
[PULL 14/17] block: Support detached LUKS header creation using blockdev-create
From: Hyman Huang
Firstly, enable the ability to choose the block device containing
a detachable LUKS header by adding the 'header' parameter to
BlockdevCreateOptionsLUKS.
Secondly, when formatting the LUKS volume with a detachable header,
truncate the payload volume to length without a header size.
Using the qmp blockdev command, create the LUKS volume with a
detachable header as follows:
1. add the secret to lock/unlock the cipher stored in the
detached LUKS header
$ virsh qemu-monitor-command vm '{"execute":"object-add",
> "arguments":{"qom-type": "secret", "id": "sec0", "data": "foo"}}'
2. create a header img with 0 size
$ virsh qemu-monitor-command vm '{"execute":"blockdev-create",
> "arguments":{"job-id":"job0", "options":{"driver":"file",
> "filename":"/path/to/detached_luks_header.img", "size":0 }}}'
3. add protocol blockdev node for header
$ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> "arguments": {"driver":"file", "filename":
> "/path/to/detached_luks_header.img", "node-name":
> "detached-luks-header-storage"}}'
4. create a payload img with 0 size
$ virsh qemu-monitor-command vm '{"execute":"blockdev-create",
> "arguments":{"job-id":"job1", "options":{"driver":"file",
> "filename":"/path/to/detached_luks_payload_raw.img", "size":0}}}'
5. add protocol blockdev node for payload
$ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> "arguments": {"driver":"file", "filename":
> "/path/to/detached_luks_payload_raw.img", "node-name":
> "luks-payload-raw-storage"}}'
6. do the formatting with 128M size
$ virsh qemu-monitor-command c81_node1 '{"execute":"blockdev-create",
> "arguments":{"job-id":"job2", "options":{"driver":"luks", "header":
> "detached-luks-header-storage", "file":"luks-payload-raw-storage",
> "size":134217728, "preallocation":"full", "key-secret":"sec0" }}}'
Signed-off-by: Hyman Huang
Reviewed-by: Daniel P. Berrangé
Signed-off-by: Daniel P. Berrangé
---
block/crypto.c | 101 +++
qapi/block-core.json | 3 ++
2 files changed, 96 insertions(+), 8 deletions(-)
diff --git a/block/crypto.c b/block/crypto.c
index 1b3f87922a..8e7ee5e9ac 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -162,6 +162,48 @@ error:
return ret;
}
+static int coroutine_fn GRAPH_UNLOCKED
+block_crypto_co_format_luks_payload(BlockdevCreateOptionsLUKS *luks_opts,
+Error **errp)
+{
+BlockDriverState *bs = NULL;
+BlockBackend *blk = NULL;
+Error *local_error = NULL;
+int ret;
+
+if (luks_opts->size > INT64_MAX) {
+return -EFBIG;
+}
+
+bs = bdrv_co_open_blockdev_ref(luks_opts->file, errp);
+if (bs == NULL) {
+return -EIO;
+}
+
+blk = blk_co_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE,
+ BLK_PERM_ALL, errp);
+if (!blk) {
+ret = -EPERM;
+goto fail;
+}
+
+ret = blk_truncate(blk, luks_opts->size, true,
+ luks_opts->preallocation, 0, _error);
+if (ret < 0) {
+if (ret == -EFBIG) {
+/* Replace the error message with a better one */
+error_free(local_error);
+error_setg(errp, "The requested file size is too large");
+}
+goto fail;
+}
+
+ret = 0;
+
+fail:
+bdrv_co_unref(bs);
+return ret;
+}
static QemuOptsList block_crypto_runtime_opts_luks = {
.name = "crypto",
@@ -341,7 +383,9 @@ static int block_crypto_open_generic(QCryptoBlockFormat
format,
static int coroutine_fn GRAPH_UNLOCKED
block_crypto_co_create_generic(BlockDriverState *bs, int64_t size,
QCryptoBlockCreateOptions *opts,
- PreallocMode prealloc, Error **errp)
+ PreallocMode prealloc,
+ unsigned int flags,
+ Error **errp)
{
int ret;
BlockBackend *blk;
@@ -369,7 +413,7 @@ block_crypto_co_create_generic(BlockDriverState *bs,
int64_t size,
block_crypto_create_init_func,
block_crypto_create_write_func,
,
-
[PULL 16/17] crypto: Introduce 'detached-header' field in QCryptoBlockInfoLUKS
From: Hyman Huang
When querying the LUKS disk with the qemu-img tool or other APIs,
add information about whether the LUKS header is detached.
Additionally, update the test case with the appropriate
modification.
Signed-off-by: Hyman Huang
Reviewed-by: Daniel P. Berrangé
Signed-off-by: Daniel P. Berrangé
---
crypto/block-luks.c| 2 ++
qapi/crypto.json | 3 +++
tests/qemu-iotests/210.out | 4
3 files changed, 9 insertions(+)
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index ab52c9dce1..3ee928fb5a 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -1271,6 +1271,7 @@ qcrypto_block_luks_open(QCryptoBlock *block,
block->sector_size = QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
block->payload_offset = luks->header.payload_offset_sector *
block->sector_size;
+block->detached_header = (block->payload_offset == 0) ? true : false;
return 0;
@@ -1895,6 +1896,7 @@ static int qcrypto_block_luks_get_info(QCryptoBlock
*block,
info->u.luks.master_key_iters = luks->header.master_key_iterations;
info->u.luks.uuid = g_strndup((const char *)luks->header.uuid,
sizeof(luks->header.uuid));
+info->u.luks.detached_header = block->detached_header;
for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
slot = g_new0(QCryptoBlockInfoLUKSSlot, 1);
diff --git a/qapi/crypto.json b/qapi/crypto.json
index 22c6cce3ae..ad8dd37175 100644
--- a/qapi/crypto.json
+++ b/qapi/crypto.json
@@ -317,6 +317,8 @@
#
# @hash-alg: the master key hash algorithm
#
+# @detached-header: whether the LUKS header is detached (Since 9.0)
+#
# @payload-offset: offset to the payload data in bytes
#
# @master-key-iters: number of PBKDF2 iterations for key material
@@ -333,6 +335,7 @@
'ivgen-alg': 'QCryptoIVGenAlgorithm',
'*ivgen-hash-alg': 'QCryptoHashAlgorithm',
'hash-alg': 'QCryptoHashAlgorithm',
+ 'detached-header': 'bool',
'payload-offset': 'int',
'master-key-iters': 'int',
'uuid': 'str',
diff --git a/tests/qemu-iotests/210.out b/tests/qemu-iotests/210.out
index 96d9f749dd..94b29b2120 100644
--- a/tests/qemu-iotests/210.out
+++ b/tests/qemu-iotests/210.out
@@ -18,6 +18,7 @@ virtual size: 128 MiB (134217728 bytes)
encrypted: yes
Format specific information:
ivgen alg: plain64
+detached header: false
hash alg: sha256
cipher alg: aes-256
uuid: ----
@@ -70,6 +71,7 @@ virtual size: 64 MiB (67108864 bytes)
encrypted: yes
Format specific information:
ivgen alg: plain64
+detached header: false
hash alg: sha1
cipher alg: aes-128
uuid: ----
@@ -125,6 +127,7 @@ virtual size: 0 B (0 bytes)
encrypted: yes
Format specific information:
ivgen alg: plain64
+detached header: false
hash alg: sha256
cipher alg: aes-256
uuid: ----
@@ -195,6 +198,7 @@ virtual size: 0 B (0 bytes)
encrypted: yes
Format specific information:
ivgen alg: plain64
+detached header: false
hash alg: sha256
cipher alg: aes-256
uuid: ----
--
2.43.0
[PULL 09/17] chardev: close QIOChannel before unref'ing
The chardev socket backend will unref the QIOChannel object while
it is still potentially open. When using TLS there could be a
pending TLS handshake taking place. If the channel is left open
then when the TLS handshake callback runs, it can end up accessing
free'd memory in the tcp_chr_tls_handshake method.
Closing the QIOChannel will unregister any pending handshake
source.
Reported-by: jiangyegen
Reviewed-by: Marc-André Lureau
Signed-off-by: Daniel P. Berrangé
---
chardev/char-socket.c | 4
1 file changed, 4 insertions(+)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 73947da188..7105753815 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -378,6 +378,10 @@ static void tcp_chr_free_connection(Chardev *chr)
char_socket_yank_iochannel,
QIO_CHANNEL(s->sioc));
}
+
+if (s->ioc) {
+qio_channel_close(s->ioc, NULL);
+}
object_unref(OBJECT(s->sioc));
s->sioc = NULL;
object_unref(OBJECT(s->ioc));
--
2.43.0
[PULL 03/17] qemu_init: increase NOFILE soft limit on POSIX
From: Fiona Ebner
In many configurations, e.g. multiple vNICs with multiple queues or
with many Ceph OSDs, the default soft limit of 1024 is not enough.
QEMU is supposed to work fine with file descriptors >= 1024 and does
not use select() on POSIX. Bump the soft limit to the allowed hard
limit to avoid issues with the aforementioned configurations.
Of course the limit could be raised from the outside, but the man page
of systemd.exec states about 'LimitNOFILE=':
> Don't use.
> [...]
> Typically applications should increase their soft limit to the hard
> limit on their own, if they are OK with working with file
> descriptors above 1023,
If the soft limit is already the same as the hard limit, avoid the
superfluous setrlimit call. This can avoid a warning with a strict
seccomp filter blocking setrlimit if NOFILE was already raised before
executing QEMU.
Buglink: https://bugzilla.proxmox.com/show_bug.cgi?id=4507
Reviewed-by: Daniel P. Berrangé
Signed-off-by: Fiona Ebner
Signed-off-by: Daniel P. Berrangé
---
include/sysemu/os-posix.h | 1 +
include/sysemu/os-win32.h | 5 +
os-posix.c| 22 ++
system/vl.c | 2 ++
4 files changed, 30 insertions(+)
diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h
index dff32ae185..b881ac6c6f 100644
--- a/include/sysemu/os-posix.h
+++ b/include/sysemu/os-posix.h
@@ -51,6 +51,7 @@ bool is_daemonized(void);
void os_daemonize(void);
bool os_set_runas(const char *user_id);
void os_set_chroot(const char *path);
+void os_setup_limits(void);
void os_setup_post(void);
int os_mlock(void);
diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
index 1047d260cb..b82a5d3ad9 100644
--- a/include/sysemu/os-win32.h
+++ b/include/sysemu/os-win32.h
@@ -128,6 +128,11 @@ static inline int os_mlock(void)
return -ENOSYS;
}
+static inline void os_setup_limits(void)
+{
+return;
+}
+
#define fsync _commit
#if !defined(lseek)
diff --git a/os-posix.c b/os-posix.c
index 52ef6990ff..a4284e2c07 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -24,6 +24,7 @@
*/
#include "qemu/osdep.h"
+#include
#include
#include
#include
@@ -256,6 +257,27 @@ void os_daemonize(void)
}
}
+void os_setup_limits(void)
+{
+struct rlimit nofile;
+
+if (getrlimit(RLIMIT_NOFILE, ) < 0) {
+warn_report("unable to query NOFILE limit: %s", strerror(errno));
+return;
+}
+
+if (nofile.rlim_cur == nofile.rlim_max) {
+return;
+}
+
+nofile.rlim_cur = nofile.rlim_max;
+
+if (setrlimit(RLIMIT_NOFILE, ) < 0) {
+warn_report("unable to set NOFILE limit: %s", strerror(errno));
+return;
+}
+}
+
void os_setup_post(void)
{
int fd = 0;
diff --git a/system/vl.c b/system/vl.c
index 2a0bd08ff1..95cd2d91c4 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -2778,6 +2778,8 @@ void qemu_init(int argc, char **argv)
error_init(argv[0]);
qemu_init_exec_dir(argv[0]);
+os_setup_limits();
+
qemu_init_arch_modules();
qemu_init_subsystems();
--
2.43.0
[PULL 08/17] docs: re-generate x86_64 ABI compatibility CSV
This picks up the new EPYC-Genoa, SapphireRapids & GraniteRapids CPUs, removes the now deleted Icelake-Client CPU, and adds the newer versions of many existing CPUs. Signed-off-by: Daniel P. Berrangé --- docs/system/cpu-models-x86-abi.csv | 20 ++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/docs/system/cpu-models-x86-abi.csv b/docs/system/cpu-models-x86-abi.csv index f3f3b60be1..38b9bae310 100644 --- a/docs/system/cpu-models-x86-abi.csv +++ b/docs/system/cpu-models-x86-abi.csv @@ -8,27 +8,37 @@ Cascadelake-Server-v1,✅,✅,✅,✅ Cascadelake-Server-v2,✅,✅,✅,✅ Cascadelake-Server-v3,✅,✅,✅,✅ Cascadelake-Server-v4,✅,✅,✅,✅ +Cascadelake-Server-v5,✅,✅,✅,✅ Conroe-v1,✅,,, Cooperlake-v1,✅,✅,✅,✅ +Cooperlake-v2,✅,✅,✅,✅ Denverton-v1,✅,✅,, Denverton-v2,✅,✅,, +Denverton-v3,✅,✅,, Dhyana-v1,✅,✅,✅, +Dhyana-v2,✅,✅,✅, +EPYC-Genoa-v1,✅,✅,✅,✅ EPYC-Milan-v1,✅,✅,✅, +EPYC-Milan-v2,✅,✅,✅, EPYC-Rome-v1,✅,✅,✅, EPYC-Rome-v2,✅,✅,✅, +EPYC-Rome-v3,✅,✅,✅, +EPYC-Rome-v4,✅,✅,✅, EPYC-v1,✅,✅,✅, EPYC-v2,✅,✅,✅, EPYC-v3,✅,✅,✅, +EPYC-v4,✅,✅,✅, +GraniteRapids-v1,✅,✅,✅,✅ Haswell-v1,✅,✅,✅, Haswell-v2,✅,✅,✅, Haswell-v3,✅,✅,✅, Haswell-v4,✅,✅,✅, -Icelake-Client-v1,✅,✅,✅, -Icelake-Client-v2,✅,✅,✅, Icelake-Server-v1,✅,✅,✅,✅ Icelake-Server-v2,✅,✅,✅,✅ Icelake-Server-v3,✅,✅,✅,✅ Icelake-Server-v4,✅,✅,✅,✅ +Icelake-Server-v5,✅,✅,✅,✅ +Icelake-Server-v6,✅,✅,✅,✅ IvyBridge-v1,✅,✅,, IvyBridge-v2,✅,✅,, KnightsMill-v1,✅,✅,✅, @@ -42,15 +52,21 @@ Opteron_G5-v1,✅,✅,, Penryn-v1,✅,,, SandyBridge-v1,✅,✅,, SandyBridge-v2,✅,✅,, +SapphireRapids-v1,✅,✅,✅,✅ +SapphireRapids-v2,✅,✅,✅,✅ Skylake-Client-v1,✅,✅,✅, Skylake-Client-v2,✅,✅,✅, Skylake-Client-v3,✅,✅,✅, +Skylake-Client-v4,✅,✅,✅, Skylake-Server-v1,✅,✅,✅,✅ Skylake-Server-v2,✅,✅,✅,✅ Skylake-Server-v3,✅,✅,✅,✅ Skylake-Server-v4,✅,✅,✅,✅ +Skylake-Server-v5,✅,✅,✅,✅ Snowridge-v1,✅,✅,, Snowridge-v2,✅,✅,, +Snowridge-v3,✅,✅,, +Snowridge-v4,✅,✅,, Westmere-v1,✅,✅,, Westmere-v2,✅,✅,, athlon-v1 -- 2.43.0
[PULL 10/17] io: add trace event when cancelling TLS handshake
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Daniel P. Berrangé
---
io/channel-tls.c | 1 +
io/trace-events | 1 +
2 files changed, 2 insertions(+)
diff --git a/io/channel-tls.c b/io/channel-tls.c
index 58fe1aceee..1d9c9c72bf 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -381,6 +381,7 @@ static int qio_channel_tls_close(QIOChannel *ioc,
QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc);
if (tioc->hs_ioc_tag) {
+trace_qio_channel_tls_handshake_cancel(ioc);
g_clear_handle_id(>hs_ioc_tag, g_source_remove);
}
diff --git a/io/trace-events b/io/trace-events
index 3cc5cf1efd..d4c0f84a9a 100644
--- a/io/trace-events
+++ b/io/trace-events
@@ -43,6 +43,7 @@ qio_channel_tls_handshake_start(void *ioc) "TLS handshake
start ioc=%p"
qio_channel_tls_handshake_pending(void *ioc, int status) "TLS handshake
pending ioc=%p status=%d"
qio_channel_tls_handshake_fail(void *ioc) "TLS handshake fail ioc=%p"
qio_channel_tls_handshake_complete(void *ioc) "TLS handshake complete ioc=%p"
+qio_channel_tls_handshake_cancel(void *ioc) "TLS handshake cancel ioc=%p"
qio_channel_tls_credentials_allow(void *ioc) "TLS credentials allow ioc=%p"
qio_channel_tls_credentials_deny(void *ioc) "TLS credentials deny ioc=%p"
--
2.43.0
[PULL 11/17] crypto: Support LUKS volume with detached header
From: Hyman Huang
By enhancing the LUKS driver, it is possible to implement
the LUKS volume with a detached header.
Normally a LUKS volume has a layout:
disk: | header | key material | disk payload data |
With a detached LUKS header, you need 2 disks so getting:
disk1: | header | key material |
disk2: | disk payload data |
There are a variety of benefits to doing this:
* Secrecy - the disk2 cannot be identified as containing LUKS
volume since there's no header
* Control - if access to the disk1 is restricted, then even
if someone has access to disk2 they can't unlock
it. Might be useful if you have disks on NFS but
want to restrict which host can launch a VM
instance from it, by dynamically providing access
to the header to a designated host
* Flexibility - your application data volume may be a given
size and it is inconvenient to resize it to
add encryption.You can store the LUKS header
separately and use the existing storage
volume for payload
* Recovery - corruption of a bit in the header may make the
entire payload inaccessible. It might be
convenient to take backups of the header. If
your primary disk header becomes corrupt, you
can unlock the data still by pointing to the
backup detached header
Take the raw-format image as an example to introduce the usage
of the LUKS volume with a detached header:
1. prepare detached LUKS header images
$ dd if=/dev/zero of=test-header.img bs=1M count=32
$ dd if=/dev/zero of=test-payload.img bs=1M count=1000
$ cryptsetup luksFormat --header test-header.img test-payload.img
> --force-password --type luks1
2. block-add a protocol blockdev node of payload image
$ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> "arguments":{"node-name":"libvirt-1-storage", "driver":"file",
> "filename":"test-payload.img"}}'
3. block-add a protocol blockdev node of LUKS header as above.
$ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> "arguments":{"node-name":"libvirt-2-storage", "driver":"file",
> "filename": "test-header.img" }}'
4. object-add the secret for decrypting the cipher stored in
LUKS header above
$ virsh qemu-monitor-command vm '{"execute":"object-add",
> "arguments":{"qom-type":"secret", "id":
> "libvirt-2-storage-secret0", "data":"abc123"}}'
5. block-add the raw-drived blockdev format node
$ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> "arguments":{"node-name":"libvirt-1-format", "driver":"raw",
> "file":"libvirt-1-storage"}}'
6. block-add the luks-drived blockdev to link the raw disk
with the LUKS header by specifying the field "header"
$ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> "arguments":{"node-name":"libvirt-2-format", "driver":"luks",
> "file":"libvirt-1-format", "header":"libvirt-2-storage",
> "key-secret":"libvirt-2-format-secret0"}}'
7. hot-plug the virtio-blk device finally
$ virsh qemu-monitor-command vm '{"execute":"device_add",
> "arguments": {"num-queues":"1", "driver":"virtio-blk-pci",
> "drive": "libvirt-2-format", "id":"virtio-disk2"}}'
Starting a VM with a LUKS volume with detached header is
somewhat similar to hot-plug in that both maintaining the
same json command while the starting VM changes the
"blockdev-add/device_add" parameters to "blockdev/device".
Signed-off-by: Hyman Huang
Reviewed-by: Daniel P. Berrangé
Signed-off-by: Daniel P. Berrangé
---
block/crypto.c | 21 +++--
crypto/block-luks.c| 11 +++
include/crypto/block.h | 5 +
qapi/block-core.json | 5 -
4 files changed, 35 insertions(+), 7 deletions(-)
diff --git a/block/crypto.c b/block/crypto.c
index 921933a5e5..68656158e9 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -39,6 +39,7 @@ typedef struct BlockCrypto BlockCrypto;
struct BlockCrypto {
QCryptoBlock *block;
bool updating_keys;
+BdrvChild *header; /* Reference to the detached LUKS header */
};
@@ -63,12 +64,14 @@ static int block_crypto_read_func(QCryptoBlock *block,
Error **errp)
{
BlockDriverState *bs = opaque;
+BlockCrypto *crypto = bs->
[PULL 17/17] tests: Add case for LUKS volume with detached header
From: Hyman Huang
Also, add a section to the MAINTAINERS file for detached
LUKS header, it only has a test case in it currently.
Signed-off-by: Hyman Huang
Reviewed-by: Daniel P. Berrangé
Signed-off-by: Daniel P. Berrangé
---
MAINTAINERS | 5 +
tests/qemu-iotests/tests/luks-detached-header | 316 ++
.../tests/luks-detached-header.out| 5 +
3 files changed, 326 insertions(+)
create mode 100755 tests/qemu-iotests/tests/luks-detached-header
create mode 100644 tests/qemu-iotests/tests/luks-detached-header.out
diff --git a/MAINTAINERS b/MAINTAINERS
index 2f9741b898..f80db6a96a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3402,6 +3402,11 @@ F: migration/dirtyrate.c
F: migration/dirtyrate.h
F: include/sysemu/dirtyrate.h
+Detached LUKS header
+M: Hyman Huang
+S: Maintained
+F: tests/qemu-iotests/tests/luks-detached-header
+
D-Bus
M: Marc-André Lureau
S: Maintained
diff --git a/tests/qemu-iotests/tests/luks-detached-header
b/tests/qemu-iotests/tests/luks-detached-header
new file mode 100755
index 00..3455fd8de1
--- /dev/null
+++ b/tests/qemu-iotests/tests/luks-detached-header
@@ -0,0 +1,316 @@
+#!/usr/bin/env python3
+# group: rw auto
+#
+# Test LUKS volume with detached header
+#
+# Copyright (C) 2024 SmartX Inc.
+#
+# Authors:
+# Hyman Huang
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program 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 General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+import json
+import iotests
+from iotests import (
+imgfmt,
+qemu_img_create,
+qemu_img_info,
+QMPTestCase,
+)
+
+
+image_size = 128 * 1024 * 1024
+
+luks_img = os.path.join(iotests.test_dir, "luks.img")
+detached_header_img1 = os.path.join(iotests.test_dir, "detached_header.img1")
+detached_header_img2 = os.path.join(iotests.test_dir, "detached_header.img2")
+detached_payload_raw_img = os.path.join(
+iotests.test_dir, "detached_payload_raw.img"
+)
+detached_payload_qcow2_img = os.path.join(
+iotests.test_dir, "detached_payload_qcow2.img"
+)
+detached_header_raw_img = "json:" + json.dumps(
+{
+"driver": "luks",
+"file": {"filename": detached_payload_raw_img},
+"header": {
+"filename": detached_header_img1,
+},
+}
+)
+detached_header_qcow2_img = "json:" + json.dumps(
+{
+"driver": "luks",
+"file": {"filename": detached_payload_qcow2_img},
+"header": {"filename": detached_header_img2},
+}
+)
+
+secret_obj = "secret,id=sec0,data=foo"
+luks_opts = "key-secret=sec0"
+
+
+class TestDetachedLUKSHeader(QMPTestCase):
+def setUp(self) -> None:
+self.vm = iotests.VM()
+self.vm.add_object(secret_obj)
+self.vm.launch()
+
+# 1. Create the normal LUKS disk with 128M size
+self.vm.blockdev_create(
+{"driver": "file", "filename": luks_img, "size": 0}
+)
+self.vm.qmp_log(
+"blockdev-add",
+driver="file",
+filename=luks_img,
+node_name="luks-1-storage",
+)
+result = self.vm.blockdev_create(
+{
+"driver": imgfmt,
+"file": "luks-1-storage",
+"key-secret": "sec0",
+"size": image_size,
+"iter-time": 10,
+}
+)
+# None is expected
+self.assertEqual(result, None)
+
+# 2. Create the LUKS disk with detached header (raw)
+
+# Create detached LUKS header
+self.vm.blockdev_create(
+{"driver": "file", "filename": detached_header_img1, "size": 0}
+)
+self.vm.qmp_log(
+"blockdev-add",
+driver="file",
+filename=detached_header_img1,
+node_name="luks-2-header-storage",
+)
+
+# Create detached LUKS raw payload
+self.vm.blockdev_create(
+{"driver": "file", "filename": detached_payload_raw_img
[PULL 01/17] meson: sort C warning flags alphabetically
When scanning the list of warning flags to see if one is present, it is helpful if they are in alphabetical order. It is further helpful to separate out the 'no-' prefixed warnings. Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Daniel P. Berrangé --- meson.build | 40 +--- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/meson.build b/meson.build index e5d6f2d057..74d3aa0b12 100644 --- a/meson.build +++ b/meson.build @@ -571,36 +571,38 @@ qemu_common_flags += cc.get_supported_arguments(hardening_flags) add_global_arguments(qemu_common_flags, native: false, language: all_languages) add_global_link_arguments(qemu_ldflags, native: false, language: all_languages) -# Collect warnings that we want to enable - +# Collect warning flags we want to set, sorted alphabetically warn_flags = [ - '-Wundef', - '-Wwrite-strings', - '-Wmissing-prototypes', - '-Wstrict-prototypes', - '-Wredundant-decls', - '-Wold-style-declaration', - '-Wold-style-definition', - '-Wtype-limits', - '-Wformat-security', - '-Wformat-y2k', - '-Winit-self', - '-Wignored-qualifiers', + # First enable interesting warnings '-Wempty-body', - '-Wnested-externs', '-Wendif-labels', '-Wexpansion-to-defined', + '-Wformat-security', + '-Wformat-y2k', + '-Wignored-qualifiers', '-Wimplicit-fallthrough=2', + '-Winit-self', '-Wmissing-format-attribute', + '-Wmissing-prototypes', + '-Wnested-externs', + '-Wold-style-declaration', + '-Wold-style-definition', + '-Wredundant-decls', + '-Wshadow=local', + '-Wstrict-prototypes', + '-Wtype-limits', + '-Wundef', + '-Wwrite-strings', + + # Then disable some undesirable warnings + '-Wno-gnu-variable-sized-type-not-at-end', '-Wno-initializer-overrides', '-Wno-missing-include-dirs', + '-Wno-psabi', '-Wno-shift-negative-value', '-Wno-string-plus-int', - '-Wno-typedef-redefinition', '-Wno-tautological-type-limit-compare', - '-Wno-psabi', - '-Wno-gnu-variable-sized-type-not-at-end', - '-Wshadow=local', + '-Wno-typedef-redefinition', ] if host_os != 'darwin' -- 2.43.0
[PULL 02/17] crypto: Introduce SM4 symmetric cipher algorithm
From: Hyman Huang
Introduce the SM4 cipher algorithms (OSCCA GB/T 32907-2016).
SM4 (GBT.32907-2016) is a cryptographic standard issued by the
Organization of State Commercial Administration of China (OSCCA)
as an authorized cryptographic algorithms for the use within China.
Detect the SM4 cipher algorithms and enable the feature silently
if it is available.
Signed-off-by: Hyman Huang
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Daniel P. Berrangé
Signed-off-by: Daniel P. Berrangé
---
crypto/block-luks.c | 11
crypto/cipher-gcrypt.c.inc | 8 ++
crypto/cipher-nettle.c.inc | 49 +
crypto/cipher.c | 6
meson.build | 26 +
qapi/crypto.json| 5 +++-
tests/unit/test-crypto-cipher.c | 13 +
7 files changed, 117 insertions(+), 1 deletion(-)
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index fb01ec38bb..f0813d69b4 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -95,12 +95,23 @@ qcrypto_block_luks_cipher_size_map_twofish[] = {
{ 0, 0 },
};
+#ifdef CONFIG_CRYPTO_SM4
+static const QCryptoBlockLUKSCipherSizeMap
+qcrypto_block_luks_cipher_size_map_sm4[] = {
+{ 16, QCRYPTO_CIPHER_ALG_SM4},
+{ 0, 0 },
+};
+#endif
+
static const QCryptoBlockLUKSCipherNameMap
qcrypto_block_luks_cipher_name_map[] = {
{ "aes", qcrypto_block_luks_cipher_size_map_aes },
{ "cast5", qcrypto_block_luks_cipher_size_map_cast5 },
{ "serpent", qcrypto_block_luks_cipher_size_map_serpent },
{ "twofish", qcrypto_block_luks_cipher_size_map_twofish },
+#ifdef CONFIG_CRYPTO_SM4
+{ "sm4", qcrypto_block_luks_cipher_size_map_sm4},
+#endif
};
QEMU_BUILD_BUG_ON(sizeof(struct QCryptoBlockLUKSKeySlot) != 48);
diff --git a/crypto/cipher-gcrypt.c.inc b/crypto/cipher-gcrypt.c.inc
index a6a0117717..1377cbaf14 100644
--- a/crypto/cipher-gcrypt.c.inc
+++ b/crypto/cipher-gcrypt.c.inc
@@ -35,6 +35,9 @@ bool qcrypto_cipher_supports(QCryptoCipherAlgorithm alg,
case QCRYPTO_CIPHER_ALG_SERPENT_256:
case QCRYPTO_CIPHER_ALG_TWOFISH_128:
case QCRYPTO_CIPHER_ALG_TWOFISH_256:
+#ifdef CONFIG_CRYPTO_SM4
+case QCRYPTO_CIPHER_ALG_SM4:
+#endif
break;
default:
return false;
@@ -219,6 +222,11 @@ static QCryptoCipher
*qcrypto_cipher_ctx_new(QCryptoCipherAlgorithm alg,
case QCRYPTO_CIPHER_ALG_TWOFISH_256:
gcryalg = GCRY_CIPHER_TWOFISH;
break;
+#ifdef CONFIG_CRYPTO_SM4
+case QCRYPTO_CIPHER_ALG_SM4:
+gcryalg = GCRY_CIPHER_SM4;
+break;
+#endif
default:
error_setg(errp, "Unsupported cipher algorithm %s",
QCryptoCipherAlgorithm_str(alg));
diff --git a/crypto/cipher-nettle.c.inc b/crypto/cipher-nettle.c.inc
index 24cc61f87b..42b39e18a2 100644
--- a/crypto/cipher-nettle.c.inc
+++ b/crypto/cipher-nettle.c.inc
@@ -33,6 +33,9 @@
#ifndef CONFIG_QEMU_PRIVATE_XTS
#include
#endif
+#ifdef CONFIG_CRYPTO_SM4
+#include
+#endif
static inline bool qcrypto_length_check(size_t len, size_t blocksize,
Error **errp)
@@ -426,6 +429,30 @@ DEFINE_ECB_CBC_CTR_XTS(qcrypto_nettle_twofish,
QCryptoNettleTwofish, TWOFISH_BLOCK_SIZE,
twofish_encrypt_native, twofish_decrypt_native)
+#ifdef CONFIG_CRYPTO_SM4
+typedef struct QCryptoNettleSm4 {
+QCryptoCipher base;
+struct sm4_ctx key[2];
+} QCryptoNettleSm4;
+
+static void sm4_encrypt_native(void *ctx, size_t length,
+ uint8_t *dst, const uint8_t *src)
+{
+struct sm4_ctx *keys = ctx;
+sm4_crypt([0], length, dst, src);
+}
+
+static void sm4_decrypt_native(void *ctx, size_t length,
+ uint8_t *dst, const uint8_t *src)
+{
+struct sm4_ctx *keys = ctx;
+sm4_crypt([1], length, dst, src);
+}
+
+DEFINE_ECB(qcrypto_nettle_sm4,
+ QCryptoNettleSm4, SM4_BLOCK_SIZE,
+ sm4_encrypt_native, sm4_decrypt_native)
+#endif
bool qcrypto_cipher_supports(QCryptoCipherAlgorithm alg,
QCryptoCipherMode mode)
@@ -443,6 +470,9 @@ bool qcrypto_cipher_supports(QCryptoCipherAlgorithm alg,
case QCRYPTO_CIPHER_ALG_TWOFISH_128:
case QCRYPTO_CIPHER_ALG_TWOFISH_192:
case QCRYPTO_CIPHER_ALG_TWOFISH_256:
+#ifdef CONFIG_CRYPTO_SM4
+case QCRYPTO_CIPHER_ALG_SM4:
+#endif
break;
default:
return false;
@@ -701,6 +731,25 @@ static QCryptoCipher
*qcrypto_cipher_ctx_new(QCryptoCipherAlgorithm alg,
return >base;
}
+#ifdef CONFIG_CRYPTO_SM4
+case QCRYPTO_CIPHER_ALG_SM4:
+{
+QCryptoNettleSm4 *ctx = g_new0(QCryptoNettleSm4, 1);
+
+switch (mode) {
+case QCRYPTO_CIPHER_MODE_ECB:
+ctx->base.driver = _nettle_sm4_driver_ecb;
+
[PULL 15/17] block: Support detached LUKS header creation using qemu-img
From: Hyman Huang
Even though a LUKS header might be created with cryptsetup,
qemu-img should be enhanced to accommodate it as well.
Add the 'detached-header' option to specify the creation of
a detached LUKS header. This is how it is used:
$ qemu-img create --object secret,id=sec0,data=abc123 -f luks
> -o cipher-alg=aes-256,cipher-mode=xts -o key-secret=sec0
> -o detached-header=true header.luks
Using qemu-img or cryptsetup tools to query information of
an LUKS header image as follows:
Assume a detached LUKS header image has been created by:
$ dd if=/dev/zero of=test-header.img bs=1M count=32
$ dd if=/dev/zero of=test-payload.img bs=1M count=1000
$ cryptsetup luksFormat --header test-header.img test-payload.img
> --force-password --type luks1
Header image information could be queried using cryptsetup:
$ cryptsetup luksDump test-header.img
or qemu-img:
$ qemu-img info 'json:{"driver":"luks","file":{"filename":
> "test-payload.img"},"header":{"filename":"test-header.img"}}'
When using qemu-img, keep in mind that the entire disk
information specified by the JSON-format string above must be
supplied on the commandline; if not, an overlay check will reveal
a problem with the LUKS volume check logic.
Signed-off-by: Hyman Huang
Reviewed-by: Daniel P. Berrangé
[changed to pass 'cflags' to block_crypto_co_create_generic]
Signed-off-by: Daniel P. Berrangé
---
block.c | 5 -
block/crypto.c | 12 ++--
block/crypto.h | 8
qapi/crypto.json | 5 -
4 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/block.c b/block.c
index 30afdcbba6..1ed9214f66 100644
--- a/block.c
+++ b/block.c
@@ -7357,7 +7357,10 @@ void bdrv_img_create(const char *filename, const char
*fmt,
goto out;
}
-if (size == -1) {
+/* Parameter 'size' is not needed for detached LUKS header */
+if (size == -1 &&
+!(!strcmp(fmt, "luks") &&
+ qemu_opt_get_bool(opts, "detached-header", false))) {
error_setg(errp, "Image creation needs a size parameter");
goto out;
}
diff --git a/block/crypto.c b/block/crypto.c
index 8e7ee5e9ac..21eed909c1 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -231,6 +231,7 @@ static QemuOptsList block_crypto_create_opts_luks = {
BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG(""),
BLOCK_CRYPTO_OPT_DEF_LUKS_HASH_ALG(""),
BLOCK_CRYPTO_OPT_DEF_LUKS_ITER_TIME(""),
+BLOCK_CRYPTO_OPT_DEF_LUKS_DETACHED_HEADER(""),
{ /* end of list */ }
},
};
@@ -405,7 +406,7 @@ block_crypto_co_create_generic(BlockDriverState *bs,
int64_t size,
data = (struct BlockCryptoCreateData) {
.blk = blk,
-.size = size,
+.size = flags & QCRYPTO_BLOCK_CREATE_DETACHED ? 0 : size,
.prealloc = prealloc,
};
@@ -791,6 +792,9 @@ block_crypto_co_create_opts_luks(BlockDriver *drv, const
char *filename,
PreallocMode prealloc;
char *buf = NULL;
int64_t size;
+bool detached_hdr =
+qemu_opt_get_bool(opts, "detached-header", false);
+unsigned int cflags = 0;
int ret;
Error *local_err = NULL;
@@ -830,9 +834,13 @@ block_crypto_co_create_opts_luks(BlockDriver *drv, const
char *filename,
goto fail;
}
+if (detached_hdr) {
+cflags |= QCRYPTO_BLOCK_CREATE_DETACHED;
+}
+
/* Create format layer */
ret = block_crypto_co_create_generic(bs, size, create_opts,
- prealloc, 0, errp);
+ prealloc, cflags, errp);
if (ret < 0) {
goto fail;
}
diff --git a/block/crypto.h b/block/crypto.h
index 72e792c9af..dc3d2d5ed9 100644
--- a/block/crypto.h
+++ b/block/crypto.h
@@ -41,6 +41,7 @@
#define BLOCK_CRYPTO_OPT_LUKS_IVGEN_HASH_ALG "ivgen-hash-alg"
#define BLOCK_CRYPTO_OPT_LUKS_HASH_ALG "hash-alg"
#define BLOCK_CRYPTO_OPT_LUKS_ITER_TIME "iter-time"
+#define BLOCK_CRYPTO_OPT_LUKS_DETACHED_HEADER "detached-header"
#define BLOCK_CRYPTO_OPT_LUKS_KEYSLOT "keyslot"
#define BLOCK_CRYPTO_OPT_LUKS_STATE "state"
#define BLOCK_CRYPTO_OPT_LUKS_OLD_SECRET "old-secret"
@@ -100,6 +101,13 @@
.help = "Select new state of affected keyslots (active/inactive)",\
}
+#define BLOCK_CRYPTO_OPT_DEF_LUKS_DETACHED_HEADER(prefix) \
+{ \
+.name = prefix BLOCK_CRYPTO_OPT_LUKS_DETACHED_HEADER, \
+.type = QEMU_OPT_BOOL,\
+.help = "Create a detached LUKS header", \
+}
+
#define BLOCK_CRYPTO_OPT_DEF_LUKS_KEYSLOT(prefix) \
{
[PULL 12/17] qapi: Make parameter 'file' optional for BlockdevCreateOptionsLUKS
From: Hyman Huang
To support detached LUKS header creation, make the existing 'file'
field in BlockdevCreateOptionsLUKS optional.
Signed-off-by: Hyman Huang
Reviewed-by: Daniel P. Berrangé
Signed-off-by: Daniel P. Berrangé
---
block/crypto.c | 21 ++---
qapi/block-core.json | 5 +++--
2 files changed, 17 insertions(+), 9 deletions(-)
diff --git a/block/crypto.c b/block/crypto.c
index 68656158e9..e87dc84111 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -663,9 +663,9 @@ block_crypto_co_create_luks(BlockdevCreateOptions
*create_options, Error **errp)
assert(create_options->driver == BLOCKDEV_DRIVER_LUKS);
luks_opts = _options->u.luks;
-bs = bdrv_co_open_blockdev_ref(luks_opts->file, errp);
-if (bs == NULL) {
-return -EIO;
+if (luks_opts->file == NULL) {
+error_setg(errp, "Formatting LUKS disk requires parameter 'file'");
+return -EINVAL;
}
create_opts = (QCryptoBlockCreateOptions) {
@@ -677,10 +677,17 @@ block_crypto_co_create_luks(BlockdevCreateOptions
*create_options, Error **errp)
preallocation = luks_opts->preallocation;
}
-ret = block_crypto_co_create_generic(bs, luks_opts->size, _opts,
- preallocation, errp);
-if (ret < 0) {
-goto fail;
+if (luks_opts->file) {
+bs = bdrv_co_open_blockdev_ref(luks_opts->file, errp);
+if (bs == NULL) {
+return -EIO;
+}
+
+ret = block_crypto_co_create_generic(bs, luks_opts->size, _opts,
+ preallocation, errp);
+if (ret < 0) {
+goto fail;
+}
}
ret = 0;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index e7d4fb539b..77d1f50d55 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4955,7 +4955,8 @@
#
# Driver specific image creation options for LUKS.
#
-# @file: Node to create the image format on
+# @file: Node to create the image format on, mandatory except when
+#'preallocation' is not requested
#
# @size: Size of the virtual disk in bytes
#
@@ -4966,7 +4967,7 @@
##
{ 'struct': 'BlockdevCreateOptionsLUKS',
'base': 'QCryptoBlockCreateOptionsLUKS',
- 'data': { 'file': 'BlockdevRef',
+ 'data': { '*file':'BlockdevRef',
'size': 'size',
'*preallocation': 'PreallocMode' } }
--
2.43.0
[PULL 13/17] crypto: Modify the qcrypto_block_create to support creation flags
From: Hyman Huang
Expand the signature of qcrypto_block_create to enable the
formation of LUKS volumes with detachable headers. To accomplish
that, introduce QCryptoBlockCreateFlags to instruct the creation
process to set the payload_offset_sector to 0.
Signed-off-by: Hyman Huang
Reviewed-by: Daniel P. Berrangé
Signed-off-by: Daniel P. Berrangé
---
block/crypto.c | 1 +
block/qcow.c | 2 +-
block/qcow2.c | 2 +-
crypto/block-luks.c| 28 +---
crypto/block.c | 4 +++-
crypto/blockpriv.h | 2 ++
include/crypto/block.h | 11 +++
tests/unit/test-crypto-block.c | 2 ++
8 files changed, 42 insertions(+), 10 deletions(-)
diff --git a/block/crypto.c b/block/crypto.c
index e87dc84111..1b3f87922a 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -369,6 +369,7 @@ block_crypto_co_create_generic(BlockDriverState *bs,
int64_t size,
block_crypto_create_init_func,
block_crypto_create_write_func,
,
+ 0,
errp);
if (!crypto) {
diff --git a/block/qcow.c b/block/qcow.c
index c6d0e15f1e..ca8e1d5ec8 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -885,7 +885,7 @@ qcow_co_create(BlockdevCreateOptions *opts, Error **errp)
header.crypt_method = cpu_to_be32(QCOW_CRYPT_AES);
crypto = qcrypto_block_create(qcow_opts->encrypt, "encrypt.",
- NULL, NULL, NULL, errp);
+ NULL, NULL, NULL, 0, errp);
if (!crypto) {
ret = -EINVAL;
goto exit;
diff --git a/block/qcow2.c b/block/qcow2.c
index 9bee66fff5..204f5854cf 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3216,7 +3216,7 @@ qcow2_set_up_encryption(BlockDriverState *bs,
crypto = qcrypto_block_create(cryptoopts, "encrypt.",
qcow2_crypto_hdr_init_func,
qcow2_crypto_hdr_write_func,
- bs, errp);
+ bs, 0, errp);
if (!crypto) {
return -EINVAL;
}
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 7e1235c213..ab52c9dce1 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -1315,6 +1315,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
const char *hash_alg;
g_autofree char *cipher_mode_spec = NULL;
uint64_t iters;
+uint64_t detached_header_size;
memcpy(_opts, >u.luks, sizeof(luks_opts));
if (!luks_opts.has_iter_time) {
@@ -1543,19 +1544,32 @@ qcrypto_block_luks_create(QCryptoBlock *block,
slot->stripes = QCRYPTO_BLOCK_LUKS_STRIPES;
}
-/* The total size of the LUKS headers is the partition header + key
- * slot headers, rounded up to the nearest sector, combined with
- * the size of each master key material region, also rounded up
- * to the nearest sector */
-luks->header.payload_offset_sector = header_sectors +
-QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS * split_key_sectors;
+if (block->detached_header) {
+/*
+ * For a detached LUKS header image, set the payload_offset_sector
+ * to 0 to specify the starting point for read/write
+ */
+luks->header.payload_offset_sector = 0;
+} else {
+/*
+ * The total size of the LUKS headers is the partition header + key
+ * slot headers, rounded up to the nearest sector, combined with
+ * the size of each master key material region, also rounded up
+ * to the nearest sector
+ */
+luks->header.payload_offset_sector = header_sectors +
+QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS * split_key_sectors;
+}
block->sector_size = QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
block->payload_offset = luks->header.payload_offset_sector *
block->sector_size;
+detached_header_size =
+(header_sectors + QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS *
+ split_key_sectors) * block->sector_size;
/* Reserve header space to match payload offset */
-initfunc(block, block->payload_offset, opaque, _err);
+initfunc(block, detached_header_size, opaque, _err);
if (local_err) {
error_propagate(errp, local_err);
goto error;
diff --git a/crypto/block.c b/crypto/block.c
index 7bb4b74a37..506ea1d1a3 100644
--- a/crypto/block.c
+++ b/crypto/block.c
@@ -87,6 +87,7 @@ QCryptoBlock *qcrypto_block_create(QCryptoBlockCreateOptions
*options,
QCryptoBlockInitFunc initfunc,
QCryptoBlockWriteFunc writefunc,
void *opaque,
+ unsigned
[PULL 05/17] softmmu: remove obsolete comment about libvirt timeouts
For a long time now, libvirt has pre-created the monitor connection
socket and passed the pre-opened FD into QEMU during startup. Thus
libvirt does not have any timeouts waiting for the monitor socket
to appear, it is immediately connected.
Reviewed-by: Zhao Liu
Signed-off-by: Daniel P. Berrangé
---
system/vl.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/system/vl.c b/system/vl.c
index 95cd2d91c4..a82555ae15 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -1914,7 +1914,6 @@ static bool object_create_early(const char *type)
* Allocation of large amounts of memory may delay
* chardev initialization for too long, and trigger timeouts
* on software that waits for a monitor socket to be created
- * (e.g. libvirt).
*/
if (g_str_has_prefix(type, "memory-backend-")) {
return false;
--
2.43.0
[PULL 07/17] docs: fix highlighting of CPU ABI header rows
The 'header-rows' directive indicates how many rows in the generated table are to be highlighted as headers. We only have one such row in the CSV file included. This removes the accident bold highlighting of the 'i486' CPU model. Signed-off-by: Daniel P. Berrangé --- docs/system/cpu-models-x86.rst.inc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/system/cpu-models-x86.rst.inc b/docs/system/cpu-models-x86.rst.inc index 7f6368f999..ba27b5683f 100644 --- a/docs/system/cpu-models-x86.rst.inc +++ b/docs/system/cpu-models-x86.rst.inc @@ -58,7 +58,7 @@ depending on the machine type is in use. .. csv-table:: x86-64 ABI compatibility levels :file: cpu-models-x86-abi.csv :widths: 40,15,15,15,15 - :header-rows: 2 + :header-rows: 1 Preferred CPU models for Intel x86 hosts -- 2.43.0
[PULL 06/17] scripts: drop comment about autogenerated CPU API file
The RST doc include can't be made to skip the comment indicating the CPU
CSV file is auto-generated when importing it. This comment line was
previously manually removed from the generated output that was committed.
Signed-off-by: Daniel P. Berrangé
---
scripts/cpu-x86-uarch-abi.py | 1 -
1 file changed, 1 deletion(-)
diff --git a/scripts/cpu-x86-uarch-abi.py b/scripts/cpu-x86-uarch-abi.py
index 052ddd7514..7360e55c6e 100644
--- a/scripts/cpu-x86-uarch-abi.py
+++ b/scripts/cpu-x86-uarch-abi.py
@@ -179,7 +179,6 @@
models[name]["delta"][level] = delta
def print_uarch_abi_csv():
-print("# Automatically generated from '%s'" % __file__)
print("Model,baseline,v2,v3,v4")
for name in models.keys():
print(name, end="")
--
2.43.0
[PULL 04/17] ui: drop VNC feature _MASK constants
Each VNC feature enum entry has a corresponding _MASK constant
which is the bit-shifted value. It is very easy for contributors
to accidentally use the _MASK constant, instead of the non-_MASK
constant, or the reverse. No compiler warning is possible and
it'll just silently do the wrong thing at runtime.
By introducing the vnc_set_feature helper method, we can drop
all the _MASK constants and thus prevent any future accidents.
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Daniel P. Berrangé
---
ui/vnc.c | 34 +-
ui/vnc.h | 22 +-
2 files changed, 22 insertions(+), 34 deletions(-)
diff --git a/ui/vnc.c b/ui/vnc.c
index 4f23a0fa79..3db87fd89c 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2144,16 +2144,16 @@ static void set_encodings(VncState *vs, int32_t
*encodings, size_t n_encodings)
vs->vnc_encoding = enc;
break;
case VNC_ENCODING_HEXTILE:
-vs->features |= VNC_FEATURE_HEXTILE_MASK;
+vnc_set_feature(vs, VNC_FEATURE_HEXTILE);
vs->vnc_encoding = enc;
break;
case VNC_ENCODING_TIGHT:
-vs->features |= VNC_FEATURE_TIGHT_MASK;
+vnc_set_feature(vs, VNC_FEATURE_TIGHT);
vs->vnc_encoding = enc;
break;
#ifdef CONFIG_PNG
case VNC_ENCODING_TIGHT_PNG:
-vs->features |= VNC_FEATURE_TIGHT_PNG_MASK;
+vnc_set_feature(vs, VNC_FEATURE_TIGHT_PNG);
vs->vnc_encoding = enc;
break;
#endif
@@ -2163,57 +2163,57 @@ static void set_encodings(VncState *vs, int32_t
*encodings, size_t n_encodings)
* So prioritize ZRLE, even if the client hints that it prefers
* ZLIB.
*/
-if ((vs->features & VNC_FEATURE_ZRLE_MASK) == 0) {
-vs->features |= VNC_FEATURE_ZLIB_MASK;
+if (!vnc_has_feature(vs, VNC_FEATURE_ZRLE)) {
+vnc_set_feature(vs, VNC_FEATURE_ZLIB);
vs->vnc_encoding = enc;
}
break;
case VNC_ENCODING_ZRLE:
-vs->features |= VNC_FEATURE_ZRLE_MASK;
+vnc_set_feature(vs, VNC_FEATURE_ZRLE);
vs->vnc_encoding = enc;
break;
case VNC_ENCODING_ZYWRLE:
-vs->features |= VNC_FEATURE_ZYWRLE_MASK;
+vnc_set_feature(vs, VNC_FEATURE_ZYWRLE);
vs->vnc_encoding = enc;
break;
case VNC_ENCODING_DESKTOPRESIZE:
-vs->features |= VNC_FEATURE_RESIZE_MASK;
+vnc_set_feature(vs, VNC_FEATURE_RESIZE);
break;
case VNC_ENCODING_DESKTOP_RESIZE_EXT:
-vs->features |= VNC_FEATURE_RESIZE_EXT_MASK;
+vnc_set_feature(vs, VNC_FEATURE_RESIZE_EXT);
break;
case VNC_ENCODING_POINTER_TYPE_CHANGE:
-vs->features |= VNC_FEATURE_POINTER_TYPE_CHANGE_MASK;
+vnc_set_feature(vs, VNC_FEATURE_POINTER_TYPE_CHANGE);
break;
case VNC_ENCODING_RICH_CURSOR:
-vs->features |= VNC_FEATURE_RICH_CURSOR_MASK;
+vnc_set_feature(vs, VNC_FEATURE_RICH_CURSOR);
break;
case VNC_ENCODING_ALPHA_CURSOR:
-vs->features |= VNC_FEATURE_ALPHA_CURSOR_MASK;
+vnc_set_feature(vs, VNC_FEATURE_ALPHA_CURSOR);
break;
case VNC_ENCODING_EXT_KEY_EVENT:
send_ext_key_event_ack(vs);
break;
case VNC_ENCODING_AUDIO:
if (vs->vd->audio_state) {
-vs->features |= VNC_FEATURE_AUDIO_MASK;
+vnc_set_feature(vs, VNC_FEATURE_AUDIO);
send_ext_audio_ack(vs);
}
break;
case VNC_ENCODING_WMVi:
-vs->features |= VNC_FEATURE_WMVI_MASK;
+vnc_set_feature(vs, VNC_FEATURE_WMVI);
break;
case VNC_ENCODING_LED_STATE:
-vs->features |= VNC_FEATURE_LED_STATE_MASK;
+vnc_set_feature(vs, VNC_FEATURE_LED_STATE);
break;
case VNC_ENCODING_XVP:
if (vs->vd->power_control) {
-vs->features |= VNC_FEATURE_XVP_MASK;
+vnc_set_feature(vs, VNC_FEATURE_XVP);
send_xvp_message(vs, VNC_XVP_CODE_INIT);
}
break;
case VNC_ENCODING_CLIPBOARD_EXT:
-vs->features |= VNC_FEATURE_CLIPBOARD_EXT_MASK;
+vnc_set_feature(vs, VNC_FEATURE_CLIPBOARD_EXT);
vnc_server_cut_text_caps(vs);
break;
case VNC_ENCODING_COMPRESSLEVEL0 ... VNC_ENCODING_COMPRESSLEVEL0 + 9:
diff --git a/ui/vnc.h b/ui/vnc.h
index 96d19dce19..4521dc88f7 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -467,23 +467,6 @@ enum VncFeatures {
VNC_FEATURE_AUDIO,
};
-#define VNC_FEATURE_RESIZE_MASK (1 << VNC_FEATUR
[PULL 00/17] Misc fixes patches
The following changes since commit 9e34f127f419b3941b36dfdfac79640dc81e97e2: Merge tag 'pull-request-2024-02-06' of https://gitlab.com/thuth/qemu into staging (2024-02-08 11:59:28 +) are available in the Git repository at: https://gitlab.com/berrange/qemu tags/misc-fixes-pull-request for you to fetch changes up to d87b258b75498d3e8563ec8ebaaf67efc27be945: tests: Add case for LUKS volume with detached header (2024-02-09 12:50:38 +) - LUKS support for detached headers - Update x86 CPU model docs and script - Add missing close of chardev QIOChannel - More trace events o nTKS handshake - Drop unsafe VNC constants - Increase NOFILE limit during startup Daniel P. Berrangé (8): meson: sort C warning flags alphabetically ui: drop VNC feature _MASK constants softmmu: remove obsolete comment about libvirt timeouts scripts: drop comment about autogenerated CPU API file docs: fix highlighting of CPU ABI header rows docs: re-generate x86_64 ABI compatibility CSV chardev: close QIOChannel before unref'ing io: add trace event when cancelling TLS handshake Fiona Ebner (1): qemu_init: increase NOFILE soft limit on POSIX Hyman Huang (8): crypto: Introduce SM4 symmetric cipher algorithm crypto: Support LUKS volume with detached header qapi: Make parameter 'file' optional for BlockdevCreateOptionsLUKS crypto: Modify the qcrypto_block_create to support creation flags block: Support detached LUKS header creation using blockdev-create block: Support detached LUKS header creation using qemu-img crypto: Introduce 'detached-header' field in QCryptoBlockInfoLUKS tests: Add case for LUKS volume with detached header MAINTAINERS | 5 + block.c | 5 +- block/crypto.c| 144 +++- block/crypto.h| 8 + block/qcow.c | 2 +- block/qcow2.c | 2 +- chardev/char-socket.c | 4 + crypto/block-luks.c | 52 ++- crypto/block.c| 4 +- crypto/blockpriv.h| 2 + crypto/cipher-gcrypt.c.inc| 8 + crypto/cipher-nettle.c.inc| 49 +++ crypto/cipher.c | 6 + docs/system/cpu-models-x86-abi.csv| 20 +- docs/system/cpu-models-x86.rst.inc| 2 +- include/crypto/block.h| 16 + include/sysemu/os-posix.h | 1 + include/sysemu/os-win32.h | 5 + io/channel-tls.c | 1 + io/trace-events | 1 + meson.build | 66 ++-- os-posix.c| 22 ++ qapi/block-core.json | 13 +- qapi/crypto.json | 13 +- scripts/cpu-x86-uarch-abi.py | 1 - system/vl.c | 3 +- tests/qemu-iotests/210.out| 4 + tests/qemu-iotests/tests/luks-detached-header | 316 ++ .../tests/luks-detached-header.out| 5 + tests/unit/test-crypto-block.c| 2 + tests/unit/test-crypto-cipher.c | 13 + ui/vnc.c | 34 +- ui/vnc.h | 22 +- 33 files changed, 760 insertions(+), 91 deletions(-) create mode 100755 tests/qemu-iotests/tests/luks-detached-header create mode 100644 tests/qemu-iotests/tests/luks-detached-header.out -- 2.43.0
Re: [RFC/INCOMPLETE PATCH 0/8] Attempt to make qemu-img options consistent and --help working
On Wed, Feb 07, 2024 at 08:58:09PM +0300, Michael Tokarev wrote: > - create more or less consistent set of options between different >subcommands > - provide long options which can be used without figuring out which >-T/-t, -f|-F|-O etc to use for which of the two images given > - have qemu-img --help provide just a list of subcommands > - have qemu-img COMMAND --help to describe just this subcommand > - get rid of qemu-img-opts.hx, instead finish documentation in >qemu-img.rst based on the actual options implemented in >qemu-img.c. Yes, yes, yes, yes & more yes :-) I cry every time I have to read the qemu-img --help output, and I'm not much of a fan of the man page either to be fair, as I don't like the global list of options at the top which is divorced from which commands actually use them. These days I see many programs with subcommands switching to a separate man page per sub-command. Still, I'm not asking you todo that too, its just an idea for the gallery. > I started converting subcommands one by one, providing long options > and --help output. And immediately faced some questions which needs > wider discussion. > > o We have --image-opts and --target-image-opts. Do we really need both? >In my view, --image-opts should be sort of global, turning *all* >filenames on this command line into complete image specifications, >there's no need to have separate image-opts and --target-image-opts. >Don't know what to do wrt compatibility though. It shouldn't be made >this way from the beginning. As a possible solution, introduce a new >option and deprecate current set. This is basically a crutch for incomplete conversion of the block layer APIs, which meant we had a situation where we wanted to use image opts syntax for the source, but were unable todo so for the target: commit 305b4c60f200ee8e6267ac75f3f5b5d09fda1079 Author: Daniel P. Berrangé Date: Mon May 15 17:47:11 2017 +0100 qemu-img: introduce --target-image-opts for 'convert' command The '--image-opts' flag indicates whether the source filename includes options. The target filename has to remain in the plain filename format though, since it needs to be passed to bdrv_create(). When using --skip-create though, it would be possible to use image-opts syntax. This adds --target-image-opts to indicate that the target filename includes options. Currently this mandates use of the --skip-create flag too. we do have internal support for creating block devices using the full QAPI schema, via BlockdevCreateOptions. I'm not sure if bdrv_create can be made to create using the image-opts syntax. If it can, there is still the additional problem that after creation it then needs to re-open the file, and the image-opts for open is defined by BlockdevOptions not BlockdevCreateOptions. So we would need a way to convert from the latter to the former. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 15/15] qapi: Add missing union tag documentation
On Mon, Feb 05, 2024 at 08:47:09AM +0100, Markus Armbruster wrote: > Low-hanging fruit, and except for StatsFilter, the only members of > these unions lacking documentation. > > Signed-off-by: Markus Armbruster > --- > qapi/block-core.json | 12 > qapi/block-export.json | 2 ++ > qapi/char.json | 2 ++ > qapi/crypto.json | 2 ++ > qapi/machine.json | 4 > qapi/migration.json| 2 ++ > qapi/pragma.json | 16 > qapi/sockets.json | 2 ++ > qapi/stats.json| 2 ++ > qapi/transaction.json | 2 ++ > qapi/ui.json | 2 ++ > qapi/yank.json | 2 ++ > 12 files changed, 34 insertions(+), 16 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 14/15] qapi: Move @String out of common.json to discourage reuse
On Mon, Feb 05, 2024 at 08:47:08AM +0100, Markus Armbruster wrote: > Use of String is problematic, because it results in awkward interface > documentation. The previous commit cleaned up one instance. > > Move String out of common.json next to its remaining users in net.json > to discourage reuse elsewhere. > > Signed-off-by: Markus Armbruster > --- > qapi/common.json | 11 --- > qapi/net.json| 12 +++- > include/net/filter.h | 2 +- > 3 files changed, 12 insertions(+), 13 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 13/15] qapi: Improve documentation of file descriptor socket addresses
On Mon, Feb 05, 2024 at 08:47:07AM +0100, Markus Armbruster wrote: > SocketAddress branch @fd is documented in enum SocketAddressType, > unlike the other branches. That's because the branch's type is String > from common.json. > > Use a local copy of String, so we can put the documentation in the > usual place. > > Signed-off-by: Markus Armbruster > --- > qapi/sockets.json | 40 +- > include/hw/virtio/vhost-vsock-common.h | 1 + > chardev/char-socket.c | 2 +- > util/qemu-sockets.c| 3 +- > 4 files changed, 31 insertions(+), 15 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 12/15] qapi: Plug trivial documentation holes around former simple unions
On Mon, Feb 05, 2024 at 08:47:06AM +0100, Markus Armbruster wrote: > The conversion of simple to flat unions left the @data members > undocumented. Add documentation where it's trivial. Copy verbatim > from the wrapped type's description where possible. > > Leftovers: String (to be taken care of in the next commit), and > TransActionAction (left for another day). > > Signed-off-by: Markus Armbruster > --- > qapi/block-core.json | 10 ++ > qapi/char.json | 26 ++ > qapi/machine.json| 10 ++ > qapi/pragma.json | 34 -- > qapi/sockets.json| 6 ++ > qapi/tpm.json| 4 > qapi/ui.json | 12 > 7 files changed, 68 insertions(+), 34 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 11/15] qapi/dump: Clean up documentation of DumpGuestMemoryCapability
On Mon, Feb 05, 2024 at 08:47:05AM +0100, Markus Armbruster wrote: > The type's doc comment describes its member, but it's not marked up as > such. Easy enough to fix. > > Signed-off-by: Markus Armbruster > --- > qapi/dump.json | 2 +- > qapi/pragma.json | 1 - > 2 files changed, 1 insertion(+), 2 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 10/15] qapi/yank: Clean up documentaion of yank
On Mon, Feb 05, 2024 at 08:47:04AM +0100, Markus Armbruster wrote: > The command's doc comment describes the argument, but it's not marked > up as such. Easy enough to fix. > > Signed-off-by: Markus Armbruster > --- > qapi/pragma.json | 3 +-- > qapi/yank.json | 2 +- > 2 files changed, 2 insertions(+), 3 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 09/15] qga/qapi-schema: Plug trivial documentation holes
On Mon, Feb 05, 2024 at 08:47:03AM +0100, Markus Armbruster wrote: > Add missing return member documentation of guest-get-disks, > guest-get-devices, guest-get-diskstats, and guest-get-cpustats. > > The NVMe SMART information returned by guest-getdisks remains > undocumented. Add a TODO there. > > Signed-off-by: Markus Armbruster > --- > qga/qapi-schema.json | 24 ++-- > 1 file changed, 14 insertions(+), 10 deletions(-) Reviewed-by: Daniel P. Berrangé > @@ -978,7 +974,7 @@ > # > # Disk type related smart information. > # > -# - @nvme: NVMe disk smart > +# @type: disk bus type > # > # Since: 7.1 > ## Fun, so not only were the fields that exist not documented, but we've documented fields which don't exist. > @@ -1780,7 +1784,7 @@ > # > # Get statistics of each CPU in millisecond. > # > -# - @linux: Linux style CPU statistics > +# @type: guest operating system > # > # Since: 7.1 > ## And more things which don't exist ! With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 08/15] qga/qapi-schema: Clean up documentation of guest-set-vcpus
On Mon, Feb 05, 2024 at 08:47:02AM +0100, Markus Armbruster wrote: > The command's doc comment describes the argument, but it's not marked > up as such. Easy enough to fix. > > Signed-off-by: Markus Armbruster > --- > qga/qapi-schema.json | 20 ++-- > 1 file changed, 10 insertions(+), 10 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 07/15] qga/qapi-schema: Clean up documentation of guest-set-memory-blocks
On Mon, Feb 05, 2024 at 08:47:01AM +0100, Markus Armbruster wrote: > The command's doc comment describes the argument, but it's not marked > up as such. Easy enough to fix. > > Signed-off-by: Markus Armbruster > --- > qga/qapi-schema.json | 19 ++- > 1 file changed, 10 insertions(+), 9 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 06/15] qapi: Require member documentation (with loophole)
On Mon, Feb 05, 2024 at 08:47:00AM +0100, Markus Armbruster wrote: > The QAPI generator forces you to document your stuff. Except for > command arguments, event data, and members of enum and object types: > these the generator silently "documents" as "Not documented". > > We can't require proper documentation there without first fixing all > the offenders. We've always had too many offenders to pull that off. > Right now, we have more than 500. Worse, we seem to fix old ones no > faster than we add new ones: in the past year, we fixed 22 ones, but > added 26 new ones. > > To help arrest the backsliding, make missing documentation an error > unless the command, type, or event is in listed in new pragma > documentation-exceptions. If we want to really annoy people we could print a warning to stderr during docs generation, for each undocumented field. The many pages of warnings would likely trigger a much quicker series to patches to fix it to eliminate the annoying warnings :-) > > List all the current offenders: 117 commands and types in qapi/, and 9 > in qga/. > > Signed-off-by: Markus Armbruster > --- > docs/devel/qapi-code-gen.rst | 5 + > qapi/pragma.json | 119 ++ > qga/qapi-schema.json | 13 +- > scripts/qapi/parser.py| 7 +- > scripts/qapi/source.py| 2 + > .../qapi-schema/doc-bad-alternate-member.json | 2 + > tests/qapi-schema/doc-good.json | 4 +- > 7 files changed, 149 insertions(+), 3 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 05/15] sphinx/qapidoc: Drop code to generate doc for simple union tag
On Mon, Feb 05, 2024 at 08:46:59AM +0100, Markus Armbruster wrote: > QAPISchemaGenRSTVisitor._nodes_for_members() has a special case to > auto-generate documentation for a union tag member of implicit (enum) > type that lacks documentation. > > This was useful for simple unions, where the tag member's type was > implicitly. The only implicit enum type left today is 'QType'. Not > worth a special case. Drop. No change to generated documentation. > > Signed-off-by: Markus Armbruster > --- > docs/sphinx/qapidoc.py | 6 -- > 1 file changed, 6 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 04/15] qapi: Indent tagged doc comment sections properly
On Mon, Feb 05, 2024 at 08:46:58AM +0100, Markus Armbruster wrote: > docs/devel/qapi-code-gen demands that the "second and subsequent lines > of sections other than "Example"/"Examples" should be indented". > Commit a937b6aa739q (qapi: Reformat doc comments to conform to current > conventions) missed a few instances, and messed up a few others. > Clean that up. > > Signed-off-by: Markus Armbruster > --- > qapi/migration.json | 46 - > qapi/misc.json | 12 + > qapi/qdev.json | 12 - > tests/qapi-schema/doc-good.json | 10 +++ > tests/qapi-schema/doc-good.out | 2 +- > 5 files changed, 42 insertions(+), 40 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 03/15] qapi/block-core: Fix BlockLatencyHistogramInfo doc markup
On Mon, Feb 05, 2024 at 08:46:57AM +0100, Markus Armbruster wrote: > The description of @bins ends with a literal block: > > # @bins: list of io request counts corresponding to histogram > # intervals, one more element than @boundaries has. For the > # example above, @bins may be something like [3, 1, 5, 2], and > # corresponding histogram looks like: > # > # :: > # > #5| * > > Except it actually ends *before* the block: the unindented '::' line > starts a new section. Makes no sense. > > We could fix this by indenting the '::' line. Instead, double the > colon at the end of the preceding paragraph, and drop the '::' line. > > This shifts the box for the literal block right in generated > documentation, so it lines up with the description. > > Fixes: commit a0fcff383b34 (qapi: Use rST markup for literal blocks) > Signed-off-by: Markus Armbruster > --- > qapi/block-core.json | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 02/15] docs/devel/qapi-code-gen: Tweak doc comment whitespace
On Mon, Feb 05, 2024 at 08:46:56AM +0100, Markus Armbruster wrote: > Missed in commit a937b6aa739 (qapi: Reformat doc comments to conform > to current conventions). > > Signed-off-by: Markus Armbruster > --- > docs/devel/qapi-code-gen.rst | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 01/15] docs/devel/qapi-code-gen: Normalize version refs x.y.0 to just x.y
On Mon, Feb 05, 2024 at 08:46:55AM +0100, Markus Armbruster wrote: > Missed in commit 9bc6e893b72 (qapi: Normalize version references x.y.0 > to just x.y). > > Signed-off-by: Markus Armbruster > --- > docs/devel/qapi-code-gen.rst | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH trivial] qemu-nbd: mention --tls-hostname option in qemu-nbd --help
On Wed, Feb 07, 2024 at 10:32:31AM +0300, Michael Tokarev wrote: > This option was not documented. > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1240 > Signed-off-by: Michael Tokarev > --- > qemu-nbd.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/qemu-nbd.c b/qemu-nbd.c > index bac0b5e3ec..d7b3ccab21 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -114,6 +114,7 @@ static void usage(const char *name) > " --tls-creds=IDuse id of an earlier --object to provide TLS\n" > " --tls-authz=IDuse id of an earlier --object to provide\n" > "authorization\n" > +" --tls-hostname=HOSTNAME override hostname used to check x509 > certificate\n" > " -T, --trace [[enable=]][,events=][,file=]\n" > "specify tracing options\n" > " --forkfork off the server process and exit the > parent\n" Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
[PATCH] iotests: give tempdir an identifying name
If something goes wrong causing the iotests not to cleanup their
temporary directory, it is useful if the dir had an identifying
name to show what is to blame.
Signed-off-by: Daniel P. Berrangé
---
tests/qemu-iotests/testenv.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 3ff38f2661..588f30a4f1 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -126,7 +126,7 @@ def init_directories(self) -> None:
self.tmp_sock_dir = False
Path(self.sock_dir).mkdir(parents=True, exist_ok=True)
except KeyError:
-self.sock_dir = tempfile.mkdtemp()
+self.sock_dir = tempfile.mkdtemp(prefix="qemu-iotests-")
self.tmp_sock_dir = True
self.sample_img_dir = os.getenv('SAMPLE_IMG_DIR',
--
2.43.0
[PATCH] iotests: fix leak of tmpdir in dry-run mode
Creating an instance of the 'TestEnv' class will create a temporary
directory. This dir is only deleted, however, in the __exit__ handler
invoked by a context manager.
In dry-run mode, we don't use the TestEnv via a context manager, so
were leaking the temporary directory. Since meson invokes 'check'
5 times on each configure run, developers /tmp was filling up with
empty temporary directories.
Signed-off-by: Daniel P. Berrangé
---
tests/qemu-iotests/check | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index f2e9d27dcf..56d88ca423 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -184,7 +184,8 @@ if __name__ == '__main__':
sys.exit(str(e))
if args.dry_run:
-print('\n'.join([os.path.basename(t) for t in tests]))
+with env:
+print('\n'.join([os.path.basename(t) for t in tests]))
else:
with TestRunner(env, tap=args.tap,
color=args.color) as tr:
--
2.43.0
Re: [PATCH v3] tests/qemu-iotests/149: Use more inclusive language in this test
On Wed, Nov 22, 2023 at 09:40:00AM +0100, Thomas Huth wrote: > Let's use 'unsupported_configs' and 'tested_configs' here > instead of non-inclusive words. > > Signed-off-by: Thomas Huth > --- > v3: Rewording according to the suggestions of Daniel: > - Replaced "cipher not supported" with "config not supported" > - Replaced "not in LUKS_CONFIG" with "by user request" > > tests/qemu-iotests/149 | 16 +--- > tests/qemu-iotests/149.out | 8 > 2 files changed, 13 insertions(+), 11 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2] tests/qemu-iotests/149: Use more inclusive language in this test
On Mon, Nov 13, 2023 at 05:56:42PM +0100, Thomas Huth wrote:
> Let's use 'unsupported_configs' and 'tested_configs' here instead
> of non-inclusive words.
>
> Suggested-by: Paolo Bonzini
> Signed-off-by: Thomas Huth
> ---
> v2: Use different wordings (suggested by Paolo)
>
> tests/qemu-iotests/149 | 16 +---
> tests/qemu-iotests/149.out | 8
> 2 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
> index 2ae318f16f..2a8bb5787f 100755
> --- a/tests/qemu-iotests/149
> +++ b/tests/qemu-iotests/149
> @@ -518,7 +518,7 @@ configs = [
>
> ]
>
> -blacklist = [
> +unsupported_configs = [
> # We don't have a cast-6 cipher impl for QEMU yet
> "cast6-256-xts-plain64-sha1",
> "cast6-128-xts-plain64-sha1",
> @@ -528,17 +528,19 @@ blacklist = [
> "twofish-192-xts-plain64-sha1",
> ]
>
> -whitelist = []
> +# Optionally test only the configurations in the LUKS_CONFIG
> +# environment variable
> +tested_configs = None
> if "LUKS_CONFIG" in os.environ:
> -whitelist = os.environ["LUKS_CONFIG"].split(",")
> +tested_configs = os.environ["LUKS_CONFIG"].split(",")
>
> for config in configs:
> -if config.name in blacklist:
> -iotests.log("Skipping %s in blacklist" % config.name)
> +if config.name in unsupported_configs:
> +iotests.log("Skipping %s (cipher not supported)" % config.name)
s/cipher/config/ - this is about more than just ciphers - it is the
combination of algorithms (cipher, hash, ivgen).
> continue
>
> -if len(whitelist) > 0 and config.name not in whitelist:
> -iotests.log("Skipping %s not in whitelist" % config.name)
> +if tested_configs is not None and config.name not in tested_configs:
> +iotests.log("Skipping %s (not in LUKS_CONFIG)" % config.name)
This is essentially at the demand of the person invoking it, so I'd
say
s/not in LUKS_CONFIG/by user request/
> continue
>
> test_once(config, qemu_img=False)
> diff --git a/tests/qemu-iotests/149.out b/tests/qemu-iotests/149.out
> index 2cc5b82f7c..3c731bdf95 100644
> --- a/tests/qemu-iotests/149.out
> +++ b/tests/qemu-iotests/149.out
> @@ -470,7 +470,7 @@ sudo cryptsetup -q -v luksClose
> qiotest-145-cast5-128-cbc-plain64-sha1
> # Delete image
> unlink TEST_DIR/luks-cast5-128-cbc-plain64-sha1.img
>
> -Skipping cast6-256-xts-plain64-sha1 in blacklist
> +Skipping cast6-256-xts-plain64-sha1 (cipher not supported)
> # = dm-crypt aes-256-cbc-plain-sha1 =
> # Create image
> truncate TEST_DIR/luks-aes-256-cbc-plain-sha1.img --size 4194304MB
> @@ -1297,7 +1297,7 @@ sudo cryptsetup -q -v luksClose
> qiotest-145-twofish-128-xts-plain64-sha1
> # Delete image
> unlink TEST_DIR/luks-twofish-128-xts-plain64-sha1.img
>
> -Skipping twofish-192-xts-plain64-sha1 in blacklist
> +Skipping twofish-192-xts-plain64-sha1 (cipher not supported)
> # = dm-crypt serpent-128-xts-plain64-sha1 =
> # Create image
> truncate TEST_DIR/luks-serpent-128-xts-plain64-sha1.img --size 4194304MB
> @@ -1534,8 +1534,8 @@ sudo cryptsetup -q -v luksClose
> qiotest-145-serpent-192-xts-plain64-sha1
> # Delete image
> unlink TEST_DIR/luks-serpent-192-xts-plain64-sha1.img
>
> -Skipping cast6-128-xts-plain64-sha1 in blacklist
> -Skipping cast6-192-xts-plain64-sha1 in blacklist
> +Skipping cast6-128-xts-plain64-sha1 (cipher not supported)
> +Skipping cast6-192-xts-plain64-sha1 (cipher not supported)
> # = dm-crypt aes-256-xts-plain64-sha224 =
> # Create image
> truncate TEST_DIR/luks-aes-256-xts-plain64-sha224.img --size 4194304MB
> --
> 2.41.0
>
>
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 1/1] qemu-img: do not erase destination file in qemu-img dd command
On Wed, Nov 01, 2023 at 06:03:36PM +0100, Denis V. Lunev wrote: > On 11/1/23 17:51, Daniel P. Berrangé wrote: > > On Tue, Oct 31, 2023 at 03:33:52PM +0100, Hanna Czenczek wrote: > > > On 01.10.23 22:46, Denis V. Lunev wrote: > > > > Can you please not top-post. This makes the discussion complex. This > > > > approach is followed in this mailing list and in other similar lists > > > > like LKML. > > > > > > > > On 10/1/23 19:08, Mike Maslenkin wrote: > > > > > I thought about "conv=notrunc", but my main concern is changed virtual > > > > > disk metadata. > > > > > It depends on how qemu-img used. > > > > > May be I followed to wrong pattern, but pros and cons of adding "conv" > > > > > parameter was not in my mind in scope of the first patch version. > > > > > I see 4 obvious ways of using `qemu-img dd`: > > > > > 1. Copy virtual disk data between images of same format. I think disk > > > > > geometry must be preserved in this case. > > > > > 2. Copy virtual disk data between different formats. It is a valid > > > > > pattern? May be `qemu-img convert` should to be used instead? > > > > > 3. Merge snapshots to specified disk image, i.e read current state and > > > > > write it to new disk image. > > > > > 4. Copy virtual disk data to raw binary file. Actually this patch > > > > > breaks 'dd' behavior for this case when source image is less (in terms > > > > > of logical blocks) than existed raw binary file. > > > > > May be for this case condition can be improved to smth like > > > > > if (strcmp(fmt, "raw") || !g_file_test(out.filename, > > > > > G_FILE_TEST_EXISTS)) . And parameter "conv=notrunc" may be implemented > > > > > additionally for this case. > > > > My personal opinion is that qemu dd when you will need to > > > > extract the SOME data from the original image and process > > > > it further. Thus I use it to copy some data into raw binary > > > > file. My next goal here would add ability to put data into > > > > stdout that would be beneficial for. Though this is out of the > > > > equation at the moment. > > > > > > > > Though, speaking about the approach, I would say that the > > > > patch changes current behavior which is not totally buggy > > > > under a matter of this or that taste. It should be noted that > > > > we are here in Linux world, not in the Mac world where we > > > > were in position to avoid options and selections. > > > > > > > > Thus my opinion that original behavior is to be preserved > > > > as somebody is relying on it. The option you are proposing > > > > seems valuable to me also and thus the switch is to be added. > > > > The switch is well-defined in the original 'dd' world thus > > > > either conv= option would be good, either nocreat or notrunc. > > > > For me 'nocreat' seems more natural. > > > > > > > > Anyway, the last word here belongs to either Hanna or Kevin ;) > > > Personally, and honestly, I see no actual use for qemu-img dd at all, > > > because we’re trying to mimic a subset of an interface of a rather complex > > > program that has been designed to do what it does. We can only fail at > > > that. Personally, whenever I need dd functionality, I use > > > qemu-storage-daemon’s fuse export, and then use the actual dd program on > > > top. Alternatively, qemu-img convert is our native interface; > > > unfortunately, its feature set is lacking when compared to qemu-img dd, > > > but > > > I think it would be better to improve that rather than working on qemu-img > > > dd. > > Is there a clear view of what gaps exist in 'qemu-img convert', and more > > importantly, how much work is it to close the gaps, such that 'dd' could > > potentially be deprecated & eventually removed ? > > > I am using 'qemu-img dd' as a way to get (some) content > from the image. I have dreamed about getting it to > stdout. FWIW, you can use qemu-img convert to extract a subset of data from an image by layering in the 'raw' format. qemu-img convert --image-opts \ driver=raw,offset=1024,size=512,file.driver=file,file.filename=demo.img data.bin somewhat annoyingly it forces 'size' to be a multiple of 512. That makes sense if you're using the output as backing for a VM disk, but for simply extracting data, conceptually it shouldn't be needed. Yes, the syntax gets hairy with image opts :-) With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 1/1] qemu-img: do not erase destination file in qemu-img dd command
On Tue, Oct 31, 2023 at 03:33:52PM +0100, Hanna Czenczek wrote: > On 01.10.23 22:46, Denis V. Lunev wrote: > > Can you please not top-post. This makes the discussion complex. This > > approach is followed in this mailing list and in other similar lists > > like LKML. > > > > On 10/1/23 19:08, Mike Maslenkin wrote: > > > I thought about "conv=notrunc", but my main concern is changed virtual > > > disk metadata. > > > It depends on how qemu-img used. > > > May be I followed to wrong pattern, but pros and cons of adding "conv" > > > parameter was not in my mind in scope of the first patch version. > > > I see 4 obvious ways of using `qemu-img dd`: > > > 1. Copy virtual disk data between images of same format. I think disk > > > geometry must be preserved in this case. > > > 2. Copy virtual disk data between different formats. It is a valid > > > pattern? May be `qemu-img convert` should to be used instead? > > > 3. Merge snapshots to specified disk image, i.e read current state and > > > write it to new disk image. > > > 4. Copy virtual disk data to raw binary file. Actually this patch > > > breaks 'dd' behavior for this case when source image is less (in terms > > > of logical blocks) than existed raw binary file. > > > May be for this case condition can be improved to smth like > > > if (strcmp(fmt, "raw") || !g_file_test(out.filename, > > > G_FILE_TEST_EXISTS)) . And parameter "conv=notrunc" may be implemented > > > additionally for this case. > > My personal opinion is that qemu dd when you will need to > > extract the SOME data from the original image and process > > it further. Thus I use it to copy some data into raw binary > > file. My next goal here would add ability to put data into > > stdout that would be beneficial for. Though this is out of the > > equation at the moment. > > > > Though, speaking about the approach, I would say that the > > patch changes current behavior which is not totally buggy > > under a matter of this or that taste. It should be noted that > > we are here in Linux world, not in the Mac world where we > > were in position to avoid options and selections. > > > > Thus my opinion that original behavior is to be preserved > > as somebody is relying on it. The option you are proposing > > seems valuable to me also and thus the switch is to be added. > > The switch is well-defined in the original 'dd' world thus > > either conv= option would be good, either nocreat or notrunc. > > For me 'nocreat' seems more natural. > > > > Anyway, the last word here belongs to either Hanna or Kevin ;) > > Personally, and honestly, I see no actual use for qemu-img dd at all, > because we’re trying to mimic a subset of an interface of a rather complex > program that has been designed to do what it does. We can only fail at > that. Personally, whenever I need dd functionality, I use > qemu-storage-daemon’s fuse export, and then use the actual dd program on > top. Alternatively, qemu-img convert is our native interface; > unfortunately, its feature set is lacking when compared to qemu-img dd, but > I think it would be better to improve that rather than working on qemu-img > dd. Is there a clear view of what gaps exist in 'qemu-img convert', and more importantly, how much work is it to close the gaps, such that 'dd' could potentially be deprecated & eventually removed ? With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 4/4] qapi: introduce CONFIG_READ event
On Wed, Oct 18, 2023 at 02:02:08PM +0200, Markus Armbruster wrote: > Daniel P. Berrangé writes: > > > On Wed, Oct 18, 2023 at 06:51:41AM -0400, Michael S. Tsirkin wrote: > >> On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote: > >> > > x- seems safer for management tool that doesn't know about "unstable" > >> > > properties.. > >> > > >> > Easy, traditional, and unreliable :) > >> > >> > > But on the other hand, changing from x- to no-prefix is already > >> > > done when the feature is stable, and thouse who use it already > >> > > use the latest version of interface, so, removing the prefix is > >> > > just extra work. > >> > > >> > Exactly. > >> > > >> > >> I think "x-" is still better for command line use of properties - we > >> don't have an API to mark things unstable there, do we? > > > > Personally I like to see "x-" prefix present *everywhere* there is > > an unstable feature, and consider the need to rename when declaring > > it stable to be good thing as it sets an easily identifiable line > > in the sand and is self-evident to outside observers. > > > > The self-documenting nature of the "x-" prefer is what makes it most > > compelling to me. A patch submission, or command line invokation or > > an example QMP command, or a bug report, that exhibit an 'x-' prefix > > are an immediate red flag to anyone who sees them. > > Except when it isn't, like in "x-origin". > > > If someone sees a QMP comamnd / a typical giant QEMU command line, > > they are never going to go look at the QAPI schema to check whether > > any feature used had an 'unstable' marker. The 'unstable' marker > > might as well not exist in most cases. > > > > IOW, having the 'unstable' flag in the QAPI schema is great for machine > > introspection, but it isn't a substitute for having an 'x-' prefix used > > for the benefit of humans IMHO. > > I'm not sure there's disagreement. Quoting myself: > > The "x-" can remind humans "this is unstable" better than a feature > flag can (for machines, it's the other way round). > > CLI and HMP are for humans. We continue to use "x-" there. > > QMP is for machines. The feature flag is the sole source of truth. > Additional use of "x-" is fine, but not required. I guess we have different defintions of "for humans" in this context. I consider QMP data still relevant for humans, because humans are reviewing patches to libvirt that add usage of QMP features, or triaging bug reports that include examples of usage, and in both cases it is pretty relevant to make unstable features stand out to the human via the x- prefix IMHO. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 4/4] qapi: introduce CONFIG_READ event
On Wed, Oct 18, 2023 at 06:51:41AM -0400, Michael S. Tsirkin wrote: > On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote: > > > x- seems safer for management tool that doesn't know about "unstable" > > > properties.. > > > > Easy, traditional, and unreliable :) > > > > But on the other hand, changing from x- to no-prefix is already > > > done when the feature is stable, and thouse who use it already > > > use the latest version of interface, so, removing the prefix is > > > just extra work. > > > > Exactly. > > > > I think "x-" is still better for command line use of properties - we > don't have an API to mark things unstable there, do we? Personally I like to see "x-" prefix present *everywhere* there is an unstable feature, and consider the need to rename when declaring it stable to be good thing as it sets an easily identifiable line in the sand and is self-evident to outside observers. The self-documenting nature of the "x-" prefer is what makes it most compelling to me. A patch submission, or command line invokation or an example QMP command, or a bug report, that exhibit an 'x-' prefix are an immediate red flag to anyone who sees them. If someone sees a QMP comamnd / a typical giant QEMU command line, they are never going to go look at the QAPI schema to check whether any feature used had an 'unstable' marker. The 'unstable' marker might as well not exist in most cases. IOW, having the 'unstable' flag in the QAPI schema is great for machine introspection, but it isn't a substitute for having an 'x-' prefix used for the benefit of humans IMHO. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v7 0/4] Migration deprecated parts
On Wed, Oct 18, 2023 at 12:38:10PM +0200, Juan Quintela wrote:
> Juan Quintela wrote:
> > Based on: Message-ID: <20231018100651.32674-1-quint...@redhat.com>
> > [PULL 00/11] Migration 20231018 patches
> >
> > And here we are, at v7:
> > - drop black line at the end of deprecated.rst
> > - change qemu-iotest output due to warnings for deprecation.
> >
> > The only real change is the output of the qemu-iotest. That is the
> > reason why I maintained the reviewed-by. But will be happy if anyone
> > of the block people ack the changes.
>
> I forgot to include the link to the CI of the previous failure.
>
> https://gitlab.com/juan.quintela/qemu/-/jobs/5314070229
>
> tput mismatch (see
> /builds/juan.quintela/qemu/build/tests/qemu-iotests/scratch/raw-file-183/183.out.bad)
> --- /builds/juan.quintela/qemu/tests/qemu-iotests/183.out
> +++
> /builds/juan.quintela/qemu/build/tests/qemu-iotests/scratch/raw-file-183/183.out.bad
> @@ -28,6 +28,8 @@
> { 'execute': 'migrate',
> 'arguments': { 'uri': 'unix:SOCK_DIR/migrate', 'blk': true } }
> +warning: parameter 'blk is deprecated; use blockdev-mirror with NBD instead
> +warning: block migration is deprecated; use blockdev-mirror with NBD instead
> {"return": {}}
> { 'execute': 'query-status' }
> {"return": {"status": "postmigrate", "singlestep": false, "running":
> false}}
IIUC, the JSON bits are being written on stdout, and the warnings
are being written on stderr. The interleaving of the data is
potentially going to be non-deterministic in the .out file.
Generally you'd want a filter in the iotests that culls the
warning: lines to avoid this mixing of stdout/err streams.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
