Re: [PATCH v2] docs: Mark "gluster" support in QEMU as deprecated
Daniel P. Berrangé writes: > On Wed, Sep 25, 2024 at 09:15:14AM +0200, Thomas Huth wrote: >> According to https://marc.info/?l=fedora-devel-list&m=171934833215726 >> the GlusterFS development effectively ended. Thus mark it as deprecated >> in QEMU, so we can remove it in a future release if the project does >> not gain momentum again. >> >> Acked-by: Niels de Vos >> Signed-off-by: Thomas Huth >> --- >> v2: Mark it as deprecated in the QAPI and print a warning once, too >> >> docs/about/deprecated.rst | 9 + >> qapi/block-core.json | 7 ++- >> block/gluster.c | 2 ++ >> 3 files changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst >> index ed31d4b0b2..b231aa3948 100644 >> --- a/docs/about/deprecated.rst >> +++ b/docs/about/deprecated.rst >> @@ -395,6 +395,15 @@ Specifying the iSCSI password in plain text on the >> command line using the >> used instead, to refer to a ``--object secret...`` instance that provides >> a password via a file, or encrypted. >> >> +``gluster`` backend (since 9.2) >> +^^^ >> + >> +According to https://marc.info/?l=fedora-devel-list&m=171934833215726 >> +the GlusterFS development effectively ended. Unless the development >> +gains momentum again, the QEMU project might remove the gluster backend >> +in a future release. > > I'd suggest the second half of the sentance can be simplified: > >", the QEMU project will remove the gluster backend/" > > since marking something as deprecated is a stronger than "might". > We /will/ remove it, unless new informaton comes to light that > makes us re-evaluate the plans. Good point.
Re: [PATCH v2] docs: Mark "gluster" support in QEMU as deprecated
Thomas Huth writes:
> According to https://marc.info/?l=fedora-devel-list&m=171934833215726
> the GlusterFS development effectively ended. Thus mark it as deprecated
> in QEMU, so we can remove it in a future release if the project does
> not gain momentum again.
>
> Acked-by: Niels de Vos
> Signed-off-by: Thomas Huth
> ---
> v2: Mark it as deprecated in the QAPI and print a warning once, too
>
> docs/about/deprecated.rst | 9 +
> qapi/block-core.json | 7 ++-
> block/gluster.c | 2 ++
> 3 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index ed31d4b0b2..b231aa3948 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -395,6 +395,15 @@ Specifying the iSCSI password in plain text on the
> command line using the
> used instead, to refer to a ``--object secret...`` instance that provides
> a password via a file, or encrypted.
>
> +``gluster`` backend (since 9.2)
> +^^^
> +
> +According to https://marc.info/?l=fedora-devel-list&m=171934833215726
> +the GlusterFS development effectively ended. Unless the development
> +gains momentum again, the QEMU project might remove the gluster backend
> +in a future release.
> +
> +
> Character device options
> ''''''''''''''''''''''''
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 9f6dd59298..cb7cb1c0ed 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3187,12 +3187,17 @@
> #
> # @snapshot-access: Since 7.0
> #
> +# Features:
> +#
> +# @deprecated: Member @gluster is deprecated since GlusterFS ceased
> development
End the sentence with full stop, and wrap the line. Suggest:
# @deprecated: Member @gluster is deprecated because GlusterFS
# development ceased.
> +#
> # Since: 2.9
> ##
> { 'enum': 'BlockdevDriver',
>'data': [ 'blkdebug', 'blklogwrites', 'blkreplay', 'blkverify', 'bochs',
> 'cloop', 'compress', 'copy-before-write', 'copy-on-read', 'dmg',
> -'file', 'snapshot-access', 'ftp', 'ftps', 'gluster',
> +'file', 'snapshot-access', 'ftp', 'ftps',
> +{'name': 'gluster', 'features': [ 'deprecated' ] },
> {'name': 'host_cdrom', 'if': 'HAVE_HOST_BLOCK_DEVICE' },
> {'name': 'host_device', 'if': 'HAVE_HOST_BLOCK_DEVICE' },
> 'http', 'https',
I wonder why it's not 'if': 'CONFIG_GLUSTERFS'. Probably not worth
exploring now.
With the doc comment tidied up:
Acked-by: Markus Armbruster
[...]
-chardev with a JSON argument (was: [PATCH] chardev: introduce 'reconnect-ms' and deprecate 'reconnect')
Peter Krempa writes: > This is a little off-topic: > > So I wanted to make libvirt use the new parameter to stay ahead > deprecation. I've applied this patch to qemu, dumped capabilities and > pretty much expected a bunch of test cases in libvirt fail as they'd be > using a deprecated field as libvirt is supposed to validate everything. > > And the test suite passed unexpectedly. I've dug further and noticed > that for some reason libvirt doesn't still use JSON parameters for > -chardev (which is the pre-requisite for validation). > > I've also noticed that at some point I attempted to convert it over > witnessed by having an (unused) capability named QEMU_CAPS_CHARDEV_JSON > that I've introduced. > > My questions are: > 1) Does '-chardev' accept JSON identical to 'chardev-add' QMP command? Sadly, no. How badly do you want it? > If yes: If we implemented it: > 2) Since when can that be used? (What can I use as a witness) I figure we'd provide a witness the same way we did when we added JSON support to -device: add a feature @json-cli to chardev-add. > 3) Are there any gotchas? Not aware of any. Can't be 100% sure until we try. > I wonder this as I'd love to finish that out, but I really don't fancy > digging into qemu to find a gotcha 3/4 of the way there. Understandable :) > Anyways, as I've already stated, this patch is okay for libvirt, but I > didn't review the implementation, thus, on behalf of libvirt: > > ACKed-by: Peter Krempa Thanks!
Re: [PATCH v2] chardev: introduce 'reconnect-ms' and deprecate 'reconnect'
Daniil Tatianin writes: > The 'reconnect' option only allows to specify the time in seconds, > which is way too long for certain workflows. > > We have a lightweight disk backend server, which takes about 20ms to > live update, but due to this limitation in QEMU, previously the guest > disk controller would hang for one second because it would take this > long for QEMU to reinitialize the socket connection. > > Introduce a new option called 'reconnect-ms', which is the same as > 'reconnect', except the value is treated as milliseconds. These are > mutually exclusive and specifying both results in an error. > > 'reconnect' is also deprecated by this commit to make it possible to > remove it in the future as to not keep two options that control the > same thing. > > Reviewed-by: Vladimir Sementsov-Ogievskiy > Acked-by: Peter Krempa > Signed-off-by: Daniil Tatianin Tested-by: Markus Armbruster Acked-by: Markus Armbruster
Re: [PATCH v1] chardev: introduce 'reconnect-ms' and deprecate 'reconnect'
Daniil Tatianin writes:
> The 'reconnect' option only allows to specify the time in seconds,
> which is way too long for certain workflows.
>
> We have a lightweight disk backend server, which takes about 20ms to
> live update, but due to this limitation in QEMU, previously the guest
> disk controller would hang for one second because it would take this
> long for QEMU to reinitialize the socket connection.
>
> Introduce a new option called 'reconnect-ms', which is the same as
> 'reconnect', except the value is treated as milliseconds. These are
> mutually exclusive and specifying both results in an error.
Good:
$ upstream-qemu -nodefaults -chardev
socket,id=chr0,path=test-hmp,server=off,reconnect=1,reconnect-ms=2
upstream-qemu: -chardev
socket,id=chr0,path=test-hmp,server=off,reconnect=1,reconnect-ms=2: 'reconnect'
and 'reconnect-ms' are mutually exclusive
Bad:
$ upstream-qemu -nodefaults -S -display none -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 1, "major": 9},
"package": "v9.1.0-211-ga0866249bd"}, "capabilities": ["oob"]}}
{"execute": "qmp_capabilities", "arguments": {"enable": ["oob"]}}
{"return": {}}
{"execute":"chardev-add", "arguments": {"id":"chr0", "backend": {"type":
"socket", "data": {"server": false, "addr": {"type": "unix", "data": {"path":
"xyz"}}, "reconnect": 1, "reconnect-ms": 2
{"return": {}}
upstream-qemu: Unable to connect character device chr0: Failed to connect
to 'xyz': No such file or directory
We're not rejecting simultaneous use of @reconnect and @reconnect-ms
here.
Moreover, you somehow regressed the handling of the "unable to connect"
error. Before the patch, behavior is correct:
$ upstream-qemu -nodefaults -S -display none -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 1, "major": 9},
"package": "v9.1.0-210-g4b7ea33074"}, "capabilities": ["oob"]}}
{"execute": "qmp_capabilities", "arguments": {"enable": ["oob"]}}
{"return": {}}
{"execute":"chardev-add", "arguments": {"id":"chr0", "backend": {"type":
"socket", "data": {"server": false, "addr": {"type": "unix", "data": {"path":
"xyz"}}
{"error": {"class": "GenericError", "desc": "Failed to add chardev 'chr0':
Failed to connect to 'xyz': No such file or directory"}}
> 'reconnect' is also deprecated by this commit to make it possible to
> remove it in the future as to not keep two options that control the
> same thing.
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy
> Acked-by: Peter Krempa
> Signed-off-by: Daniil Tatianin
Re: [PATCH] chardev: allow specifying finer-grained reconnect timeouts
Eric Blake writes: > On Thu, Aug 29, 2024 at 01:56:43PM GMT, Markus Armbruster wrote: >> Daniil Tatianin writes: >> >> > The "reconnect" option only allows to specify the time in seconds, >> > which is way too long for certain workflows. > > ... >> > @@ -287,7 +292,8 @@ >> > '*telnet': 'bool', >> > '*tn3270': 'bool', >> > '*websocket': 'bool', >> > -'*reconnect': 'int' }, >> > +'*reconnect': 'int', >> > +'*reconnect-is-ms': 'bool' }, >> >'base': 'ChardevCommon' } >> > >> > ## >> >> I don't like this interface. >> >>PRO: compatible extension; no management application updates needed >>unless they want to support sub-second delays. >> >>CON: specifying a sub-second delay takes two parameters, which is >>awkward. >> >>CON: trap in combination with -set. Before the patch, something like >>-set chardev.ID.reconnect=N means N seconds no matter what. >>Afterwards, it depends on the value of reconnect-is-ms, which may be >>set far away. Mitigating factor: -set is obscure. > > I also dislike this interface. Having only one number plus an > optional boolean that controls its scale is not as easy to reason > about. > >> >> Alternatives: >> >> 1. Change @reconnect to 'number', specify sub-second delays as >>fractions. >> >>PRO: compatible extension; no management application updates needed >>unless they want to support sub-second delays. >> >>CON: first use of floating-point for time in seconds in QAPI, as far >>as I can see. >> >>CON: QemuOpts can't do floating-point. > > Eww. I don't see this as the compelling reason to switch to floating point. > >> >> 2. Deprecate @reconnect in favour of a new member with a more suitable >>unit. Error if both are present. >> >>PRO: after @reconnect is gone, the interface is what it arguably >>should've been from the start. >> >>CON: incompatible change. Management application updates needed >>within the deprecation grace period. > > This seems reasonable to me. To Daniil as well. Since Peter is okay with it on behalf of libvirt, so am I. > We have enough places in QAPI where we > want mutual exclusion (we mark both fields optional, but expect the > user to provide exactly one or get an error), that I wonder if it is > worth making it a first-class construct in QAPI (maybe I'm spoiled by > the OneOf designation[1] in protobuf[2] used by gRPC[3] in > kubernetes). "One of a set of optional members" is a constraint the QAPI language cannot express, so we have to leave enforcing it to handwritten code, and documenting it to handwritten comments. If it could express it, the constraint would be visible in introspection, and we'd have less handwritten code. The price is additional QAPI language and generator complexity. Sensible tradeoff? Hard to tell without patches. Risk: we pay for patches only to find out the answer is no. Another risk: we pay for patches, then take them just because we already paid for them, then continue to pay upkeep. I don't mean to say such QAPI language enhancements are a bad idea. I'm just pointing out the tradeoff to weigh, and the risks to take. There are more constraints that could be supported by the language, e.g. integer ranges. > Including the scale in the name is a bonus reason to > switch the interface. Yes, because the unit isn't obvious from the type. It is for things like byte counts, where we've succeeded at sticking to one plain unit[*]. We failed for time, so there it isn't obvious. > [1] https://protobuf.dev/programming-guides/proto3/#oneof > [2] https://protobuf.dev/overview/ > [3] https://grpc.io/ > >> >> Let's get additional input from management application developers. I >> cc'ed some. >> >> Related: NetdevSocketOptions member @reconnect. Could use the same treatment for consistency. Not a demand. [*] With few exceptions, such certain rate limits that use MBytes/s.
Re: [PATCH] chardev: allow specifying finer-grained reconnect timeouts
Daniil Tatianin writes: > The "reconnect" option only allows to specify the time in seconds, > which is way too long for certain workflows. > > We have a lightweight disk backend server, which takes about 20ms to > live update, but due to this limitation in QEMU, previously the guest > disk controller would hang for one second because it would take this > long for QEMU to reinitialize the socket connection. > > Make it possible to specify a smaller timeout by treating the value in > "reconnect" as milliseconds via the new "reconnect-is-ms" option. > > Signed-off-by: Daniil Tatianin Your use case demonstrates that a granularity of seconds was the wrong choice for the reconnection delay. [...] > diff --git a/qapi/char.json b/qapi/char.json > index ef58445cee..61aeccf09d 100644 > --- a/qapi/char.json > +++ b/qapi/char.json > @@ -272,8 +272,13 @@ > # (default: false) (Since: 3.1) > # > # @reconnect: For a client socket, if a socket is disconnected, then > -# attempt a reconnect after the given number of seconds. Setting > -# this to zero disables this function. (default: 0) (Since: 2.2) > +# attempt a reconnect after the given number of seconds (unless > +# @reconnect-is-ms is set to true, in that case the number is > +# treated as milliseconds). Setting this to zero disables > +# this function. (default: 0) (Since: 2.2) > +# > +# @reconnect-is-ms: The value specified in @reconnect should be treated > +# as milliseconds. (default: false) (Since: 9.2) > # > # Since: 1.4 > ## > @@ -287,7 +292,8 @@ > '*telnet': 'bool', > '*tn3270': 'bool', > '*websocket': 'bool', > -'*reconnect': 'int' }, > +'*reconnect': 'int', > +'*reconnect-is-ms': 'bool' }, >'base': 'ChardevCommon' } > > ## I don't like this interface. PRO: compatible extension; no management application updates needed unless they want to support sub-second delays. CON: specifying a sub-second delay takes two parameters, which is awkward. CON: trap in combination with -set. Before the patch, something like -set chardev.ID.reconnect=N means N seconds no matter what. Afterwards, it depends on the value of reconnect-is-ms, which may be set far away. Mitigating factor: -set is obscure. Alternatives: 1. Change @reconnect to 'number', specify sub-second delays as fractions. PRO: compatible extension; no management application updates needed unless they want to support sub-second delays. CON: first use of floating-point for time in seconds in QAPI, as far as I can see. CON: QemuOpts can't do floating-point. 2. Deprecate @reconnect in favour of a new member with a more suitable unit. Error if both are present. PRO: after @reconnect is gone, the interface is what it arguably should've been from the start. CON: incompatible change. Management application updates needed within the deprecation grace period. Let's get additional input from management application developers. I cc'ed some. Related: NetdevSocketOptions member @reconnect.
Re: [PATCH v2 6/7] qapi/block-core: derpecate block-job-change
Typo in subject: it's "deprecate".
Re: [PATCH v2 6/7] qapi/block-core: derpecate block-job-change
Vladimir Sementsov-Ogievskiy writes: > On 18.07.24 14:01, Markus Armbruster wrote: >> Vladimir Sementsov-Ogievskiy writes: >> >>> That's a first step to move on newer job-* APIs. >>> >>> The difference between block-job-change and job-change is in >>> find_block_job_locked() vs find_job_locked() functions. What's >>> different? >>> >>> 1. find_block_job_locked() do check, is found job a block-job. This OK >> >> Do you mean something like find_block_job_locked() finds only block >> jobs, whereas find_job_locked() finds any kind of job? > > Yes Thanks! >>> when moving to more generic API, no needs to document this change. >>> >>> 2. find_block_job_locked() reports DeviceNotActive on failure, when >>>find_job_locked() reports GenericError. Still, for block-job-change >>>errors are not documented at all, so be silent in deprecated.txt as >>>well. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy Suggest: 1. find_block_job_locked() finds only block jobs, whereas find_job_locked() finds any kind of job. job-change is a compatible extension of block-job-change. 2. find_block_job_locked() reports DeviceNotActive on failure, when find_job_locked() reports GenericError. Since the kind of error reported isn't documented for either command, and clients shouldn't rely on undocumented error details, job-change is a compatible replacement for block-job-change.
Re: [PATCH v2 6/7] qapi/block-core: derpecate block-job-change
Vladimir Sementsov-Ogievskiy writes: > That's a first step to move on newer job-* APIs. > > The difference between block-job-change and job-change is in > find_block_job_locked() vs find_job_locked() functions. What's > different? > > 1. find_block_job_locked() do check, is found job a block-job. This OK Do you mean something like find_block_job_locked() finds only block jobs, whereas find_job_locked() finds any kind of job? >when moving to more generic API, no needs to document this change. > > 2. find_block_job_locked() reports DeviceNotActive on failure, when >find_job_locked() reports GenericError. Still, for block-job-change >errors are not documented at all, so be silent in deprecated.txt as >well. > > Signed-off-by: Vladimir Sementsov-Ogievskiy
Re: [PATCH v2 5/7] qapi: add job-change
Vladimir Sementsov-Ogievskiy writes:
> Add a new-style command job-change, doing same thing as
> block-job-change. The aim is finally deprecate block-job-* APIs and
> move to job-* APIs.
>
> We add a new command to qapi/block-core.json, not to
> qapi/job.json to avoid resolving json file including loops for now.
> This all would be a lot simple to refactor when we finally drop
> deprecated block-job-* APIs.
>
> @type argument of the new command immediately becomes deprecated.
Where?
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> job-qmp.c| 14 ++
> qapi/block-core.json | 10 ++
> 2 files changed, 24 insertions(+)
>
> diff --git a/job-qmp.c b/job-qmp.c
> index c764bd3801..248e68f554 100644
> --- a/job-qmp.c
> +++ b/job-qmp.c
> @@ -139,6 +139,20 @@ void qmp_job_dismiss(const char *id, Error **errp)
> job_dismiss_locked(&job, errp);
> }
>
> +void qmp_job_change(JobChangeOptions *opts, Error **errp)
> +{
> +Job *job;
> +
> +JOB_LOCK_GUARD();
> +job = find_job_locked(opts->id, errp);
> +
> +if (!job) {
> +return;
> +}
> +
> +job_change_locked(job, opts, errp);
> +}
> +
> /* Called with job_mutex held. */
> static JobInfo *job_query_single_locked(Job *job, Error **errp)
> {
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 660c7f4a48..9087ce300c 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3104,6 +3104,16 @@
> { 'command': 'block-job-change',
>'data': 'JobChangeOptions', 'boxed': true }
>
> +##
> +# @job-change:
> +#
> +# Change the block job's options.
> +#
> +# Since: 9.1
> +##
> +{ 'command': 'job-change',
> + 'data': 'JobChangeOptions', 'boxed': true }
> +
> ##
> # @BlockdevDiscardOptions:
> #
Re: [PATCH v2 3/7] qapi: block-job-change: make copy-mode parameter optional
Vladimir Sementsov-Ogievskiy writes:
> We are going to add more parameters to change. We want to make possible
> to change only one or any subset of available options. So all the
> options should be optional.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> block/mirror.c | 4
> qapi/block-core.json | 3 ++-
> 2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index 2816bb1042..60e8d83e4f 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1272,6 +1272,10 @@ static void mirror_change(BlockJob *job,
> JobChangeOptions *opts,
>
> GLOBAL_STATE_CODE();
>
> +if (!change_opts->has_copy_mode) {
> +return;
> +}
> +
> if (qatomic_read(&s->copy_mode) == change_opts->copy_mode) {
> return;
> }
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 4ec5632596..660c7f4a48 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3071,11 +3071,12 @@
> #
> # @copy-mode: Switch to this copy mode. Currently, only the switch
> # from 'background' to 'write-blocking' is implemented.
> +# If absent, copy mode remains the same. (optional since 9.1)
> #
> # Since: 8.2
> ##
> { 'struct': 'JobChangeOptionsMirror',
> - 'data': { 'copy-mode' : 'MirrorCopyMode' } }
> + 'data': { '*copy-mode' : 'MirrorCopyMode' } }
>
> ##
> # @JobChangeOptions:
Acked-by: Markus Armbruster
Re: [PATCH v2 1/7] qapi: rename BlockJobChangeOptions to JobChangeOptions
Vladimir Sementsov-Ogievskiy writes: > We are going to move change action from block-job to job > implementation, and then move to job-* extenral APIs, deprecating > block-job-* APIs. This commit simplifies further transition. > > The commit is made by command > > git grep -l BlockJobChangeOptions | \ > xargs sed -i 's/BlockJobChangeOptions/JobChangeOptions/g' > > Signed-off-by: Vladimir Sementsov-Ogievskiy Acked-by: Markus Armbruster
Re: [PATCH 0/4] hw/s390x: Alias @dump-skeys -> @dump-s390-skey and deprecate
"Dr. David Alan Gilbert" writes: > * Markus Armbruster (arm...@redhat.com) wrote: >> Philippe Mathieu-Daudé writes: >> >> > Hi Daniel, Dave, Markus & Thomas. >> > >> > On 4/6/24 06:58, Markus Armbruster wrote: >> >> "Dr. David Alan Gilbert" writes: >> >>> * Daniel P. Berrangé (berra...@redhat.com) wrote: >> >>>> On Fri, May 31, 2024 at 06:47:45AM +0200, Thomas Huth wrote: >> >>>>> On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote: >> >>>>>> We are trying to unify all qemu-system-FOO to a single binary. >> >>>>>> In order to do that we need to remove QAPI target specific code. >> >>>>>> >> >>>>>> @dump-skeys is only available on qemu-system-s390x. This series >> >>>>>> rename it as @dump-s390-skey, making it available on other >> >>>>>> binaries. We take care of backward compatibility via deprecation. >> >>>>>> >> >>>>>> Philippe Mathieu-Daudé (4): >> >>>>>> hw/s390x: Introduce the @dump-s390-skeys QMP command >> >>>>>> hw/s390x: Introduce the 'dump_s390_skeys' HMP command >> >>>>>> hw/s390x: Deprecate the HMP 'dump_skeys' command >> >>>>>> hw/s390x: Deprecate the QMP @dump-skeys command >> >>>>> >> >>>>> Why do we have to rename the command? Just for the sake of it? I think >> >>>>> renaming HMP commands is maybe ok, but breaking the API in QMP is >> >>>>> something >> >>>>> you should consider twice. >> > >> > I'm looking at how to include this command in the new "single binary". >> > >> > Markus explained in an earlier series, just expanding this command as >> > stub to targets that don't implement it is not backward compatible and >> > breaks QMP introspection. Currently on s390x we get a result, on other >> > targets the command doesn't exist. If we add a stubs, then other targets >> > return something (even if it is an empty list), confusing management >> > interface. >> >> Loss of introspection precision is a concern, not a hard "no". >> >> We weigh all the concerns, and pick a solution we hate the least :) >> >> > So this approach use to deprecate process to include a new command >> > which behaves differently on non-s390x targets. >> > >> > If we don't care for this particular case, better. However I'd still >> > like to discuss this approach for other target-specific commands. >> > >> >> PRO rename: the command's tie to S390 is them immediately obvious, which >> >> may be useful when the command becomes available in qemu-systems capable >> >> of running other targets. >> >> >> >> CON rename: users need to adapt. >> >> >> >> What are the users? Not libvirt, as far as I can tell. >> > >> > Years ago we said, "all HMP must be based on QMP". >> >> In practice, it's closer to "HMP must be base on QMP when the >> functionality does or should exist in QMP." >> >> >Now we realize HMP >> > became stable because QMP-exposed, although not consumed externally... >> >> I'm afraid I didn't get this part. >> >> > Does the concept of "internal QMP commands" makes sense for HMP debug >> > ones? (Looking at a way to not expose them). We could use the "x-" >> > prefix to not care about stable / backward compat, but what is the point >> > of exposing to QMP commands that will never be accessed there? >> > >> >>>> That was going to be my question too. Seems like its possible to simply >> >>>> stub out the existing command for other targets. >> >> >> >> That's going to happen whether we rename the commands or not. >> >> >> >>> Are these commands really supposed to be stable, or are they just debug >> >>> commands? If they are debug, then add the x- and don't worry too much. >> > >> > OK. >> > >> >> docs/devel/qapi-code-gen.rst: >> >> >> >> Names beginning with ``x-`` used to signify "experimental". This >> >> convention has been
Re: [PATCH 0/4] hw/s390x: Alias @dump-skeys -> @dump-s390-skey and deprecate
Philippe Mathieu-Daudé writes: > Hi Daniel, Dave, Markus & Thomas. > > On 4/6/24 06:58, Markus Armbruster wrote: >> "Dr. David Alan Gilbert" writes: >>> * Daniel P. Berrangé (berra...@redhat.com) wrote: >>>> On Fri, May 31, 2024 at 06:47:45AM +0200, Thomas Huth wrote: >>>>> On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote: >>>>>> We are trying to unify all qemu-system-FOO to a single binary. >>>>>> In order to do that we need to remove QAPI target specific code. >>>>>> >>>>>> @dump-skeys is only available on qemu-system-s390x. This series >>>>>> rename it as @dump-s390-skey, making it available on other >>>>>> binaries. We take care of backward compatibility via deprecation. >>>>>> >>>>>> Philippe Mathieu-Daudé (4): >>>>>> hw/s390x: Introduce the @dump-s390-skeys QMP command >>>>>> hw/s390x: Introduce the 'dump_s390_skeys' HMP command >>>>>> hw/s390x: Deprecate the HMP 'dump_skeys' command >>>>>> hw/s390x: Deprecate the QMP @dump-skeys command >>>>> >>>>> Why do we have to rename the command? Just for the sake of it? I think >>>>> renaming HMP commands is maybe ok, but breaking the API in QMP is >>>>> something >>>>> you should consider twice. > > I'm looking at how to include this command in the new "single binary". > > Markus explained in an earlier series, just expanding this command as > stub to targets that don't implement it is not backward compatible and > breaks QMP introspection. Currently on s390x we get a result, on other > targets the command doesn't exist. If we add a stubs, then other targets > return something (even if it is an empty list), confusing management > interface. Loss of introspection precision is a concern, not a hard "no". We weigh all the concerns, and pick a solution we hate the least :) > So this approach use to deprecate process to include a new command > which behaves differently on non-s390x targets. > > If we don't care for this particular case, better. However I'd still > like to discuss this approach for other target-specific commands. > >> PRO rename: the command's tie to S390 is them immediately obvious, which >> may be useful when the command becomes available in qemu-systems capable >> of running other targets. >> >> CON rename: users need to adapt. >> >> What are the users? Not libvirt, as far as I can tell. > > Years ago we said, "all HMP must be based on QMP". In practice, it's closer to "HMP must be base on QMP when the functionality does or should exist in QMP." >Now we realize HMP > became stable because QMP-exposed, although not consumed externally... I'm afraid I didn't get this part. > Does the concept of "internal QMP commands" makes sense for HMP debug > ones? (Looking at a way to not expose them). We could use the "x-" > prefix to not care about stable / backward compat, but what is the point > of exposing to QMP commands that will never be accessed there? > >>>> That was going to be my question too. Seems like its possible to simply >>>> stub out the existing command for other targets. >> >> That's going to happen whether we rename the commands or not. >> >>> Are these commands really supposed to be stable, or are they just debug >>> commands? If they are debug, then add the x- and don't worry too much. > > OK. > >> docs/devel/qapi-code-gen.rst: >> >> Names beginning with ``x-`` used to signify "experimental". This >> convention has been replaced by special feature "unstable". >> >> Feature "unstable" is what makes something unstable, and is what >> machines should check. > > What I mentioned earlier could be 'Feature "internal" or "debug"'. What's the difference to "unstable"? >> An "x-" prefix may still be useful for humans. Machines should *not* >> key on the prefix. It's unreliable anyway: InputBarrierProperties >> member @x-origin is stable despite it's name. Renames to gain or lose >> the prefix may or may not be worth the bother. > > Could follow the rules and be renamed as "origin-coordinate-x". I don't think it's worth the trouble. The "x-" prefix is now strictly for humans, and humans can figure out what the x- in @x-origin, @y-origin means. [...]
Re: [PATCH 0/4] hw/s390x: Alias @dump-skeys -> @dump-s390-skey and deprecate
"Dr. David Alan Gilbert" writes: > * Daniel P. Berrangé (berra...@redhat.com) wrote: >> On Fri, May 31, 2024 at 06:47:45AM +0200, Thomas Huth wrote: >> > On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote: >> > > We are trying to unify all qemu-system-FOO to a single binary. >> > > In order to do that we need to remove QAPI target specific code. >> > > >> > > @dump-skeys is only available on qemu-system-s390x. This series >> > > rename it as @dump-s390-skey, making it available on other >> > > binaries. We take care of backward compatibility via deprecation. >> > > >> > > Philippe Mathieu-Daudé (4): >> > >hw/s390x: Introduce the @dump-s390-skeys QMP command >> > >hw/s390x: Introduce the 'dump_s390_skeys' HMP command >> > >hw/s390x: Deprecate the HMP 'dump_skeys' command >> > >hw/s390x: Deprecate the QMP @dump-skeys command >> > >> > Why do we have to rename the command? Just for the sake of it? I think >> > renaming HMP commands is maybe ok, but breaking the API in QMP is something >> > you should consider twice. PRO rename: the command's tie to S390 is them immediately obvious, which may be useful when the command becomes available in qemu-systems capable of running other targets. CON rename: users need to adapt. What are the users? Not libvirt, as far as I can tell. >> That was going to be my question too. Seems like its possible to simply >> stub out the existing command for other targets. That's going to happen whether we rename the commands or not. > Are these commands really supposed to be stable, or are they just debug > commands? If they are debug, then add the x- and don't worry too much. docs/devel/qapi-code-gen.rst: Names beginning with ``x-`` used to signify "experimental". This convention has been replaced by special feature "unstable". Feature "unstable" is what makes something unstable, and is what machines should check. An "x-" prefix may still be useful for humans. Machines should *not* key on the prefix. It's unreliable anyway: InputBarrierProperties member @x-origin is stable despite it's name. Renames to gain or lose the prefix may or may not be worth the bother. Making an existing part of the interface unstable should be treated similar to deprecating it: we keep it stable for at least the deprecation period. Not written policy, because we didn't consider such changes when we documented our deprecation policy in docs/about/deprecated.rst. Alternative path to a renamed command (*if* we want that): 1. Make it unstable. 2. But keep it stable for two releases. 3. Rename. [...]
Re: [PATCH 0/2] qapi: Remove deprecated events
Philippe Mathieu-Daudé writes: > Remove MEM_UNPLUG_ERROR and 'vcpu' field in TRACE events, > all deprecated since long enough. Thank you! Reviewed-by: Markus Armbruster
Re: [PATCH v3 3/6] migration: Remove 'blk/-b' option from migrate commands
Fabiano Rosas writes:
> The block migration is considered obsolete and has been deprecated in
> 8.2. Remove the migrate command option that enables it. This only
> affects the QMP and HMP commands, the feature can still be accessed by
> setting the migration 'block' capability. The whole feature will be
> removed in a future patch.
>
> Deprecation commit 8846b5bfca ("migration: migrate 'blk' command
> option is deprecated.").
>
> Reviewed-by: Markus Armbruster
> Signed-off-by: Fabiano Rosas
[...]
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 7978302949..ebca2cdced 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -909,21 +909,17 @@ ERST
>
> {
> .name = "migrate",
> -.args_type = "detach:-d,blk:-b,resume:-r,uri:s",
> -.params = "[-d] [-b] [-r] uri",
> +.args_type = "detach:-d,resume:-r,uri:s",
> +.params = "[-d] [-r] uri",
> .help = "migrate to URI (using -d to not wait for completion)"
> - "\n\t\t\t -b for migration without shared storage with"
> - " full copy of disk\n\t\t\t -r to resume a paused
> migration",
> + "\n\t\t\t -r to resume a paused migration",
> .cmd= hmp_migrate,
> },
>
>
> SRST
> -``migrate [-d] [-b]`` *uri*
> +``migrate [-d]`` *uri*
>Migrate to *uri* (using -d to not wait for completion).
> -
> - ``-b``
> -for migration with full copy of disk
> ERST
Not this patch's fault, but here goes anyway: -r is undocumented here.
>
> {
[...]
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH-for-9.1 v2 2/3] migration: Remove RDMA protocol handling
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. [...] ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH 5/6] migration: Remove non-multifd compression
0; i < thread_count; i++) {
> -while (!decomp_param[i].done) {
> -qemu_cond_wait(&decomp_done_cond, &decomp_done_lock);
> -}
> -}
> -qemu_mutex_unlock(&decomp_done_lock);
> -return qemu_file_get_error(decomp_file);
> -}
> -
> -void compress_threads_load_cleanup(void)
> -{
> -int i, thread_count;
> -
> -if (!migrate_compress()) {
> -return;
> -}
> -thread_count = migrate_decompress_threads();
> -for (i = 0; i < thread_count; i++) {
> -/*
> - * we use it as a indicator which shows if the thread is
> - * properly init'd or not
> - */
> -if (!decomp_param[i].compbuf) {
> -break;
> -}
> -
> -qemu_mutex_lock(&decomp_param[i].mutex);
> -decomp_param[i].quit = true;
> -qemu_cond_signal(&decomp_param[i].cond);
> -qemu_mutex_unlock(&decomp_param[i].mutex);
> -}
> -for (i = 0; i < thread_count; i++) {
> -if (!decomp_param[i].compbuf) {
> -break;
> -}
> -
> -qemu_thread_join(decompress_threads + i);
> -qemu_mutex_destroy(&decomp_param[i].mutex);
> -qemu_cond_destroy(&decomp_param[i].cond);
> -inflateEnd(&decomp_param[i].stream);
> -g_free(decomp_param[i].compbuf);
> -decomp_param[i].compbuf = NULL;
> -}
> -g_free(decompress_threads);
> -g_free(decomp_param);
> -decompress_threads = NULL;
> -decomp_param = NULL;
> -decomp_file = NULL;
> -}
> -
> -int compress_threads_load_setup(QEMUFile *f)
> -{
> -int i, thread_count;
> -
> -if (!migrate_compress()) {
> -return 0;
> -}
> -
> -/*
> - * set compression_counters memory to zero for a new migration
> - */
> -memset(&compression_counters, 0, sizeof(compression_counters));
> -
> -thread_count = migrate_decompress_threads();
> -decompress_threads = g_new0(QemuThread, thread_count);
> -decomp_param = g_new0(DecompressParam, thread_count);
> -qemu_mutex_init(&decomp_done_lock);
> -qemu_cond_init(&decomp_done_cond);
> -decomp_file = f;
> -for (i = 0; i < thread_count; i++) {
> -if (inflateInit(&decomp_param[i].stream) != Z_OK) {
> -goto exit;
> -}
> -
> -size_t compbuf_size = compressBound(qemu_target_page_size());
> -decomp_param[i].compbuf = g_malloc0(compbuf_size);
> -qemu_mutex_init(&decomp_param[i].mutex);
> -qemu_cond_init(&decomp_param[i].cond);
> -decomp_param[i].done = true;
> -decomp_param[i].quit = false;
> -qemu_thread_create(decompress_threads + i, "decompress",
> - do_data_decompress, decomp_param + i,
> - QEMU_THREAD_JOINABLE);
> -}
> -return 0;
> -exit:
> -compress_threads_load_cleanup();
> -return -1;
> -}
> -
> -void decompress_data_with_multi_threads(QEMUFile *f, void *host, int len)
> -{
> -int thread_count = migrate_decompress_threads();
> -QEMU_LOCK_GUARD(&decomp_done_lock);
> -while (true) {
> -for (int i = 0; i < thread_count; i++) {
> -if (decomp_param[i].done) {
> -decomp_param[i].done = false;
> -qemu_mutex_lock(&decomp_param[i].mutex);
> -qemu_get_buffer(f, decomp_param[i].compbuf, len);
> -decomp_param[i].des = host;
> -decomp_param[i].len = len;
> -qemu_cond_signal(&decomp_param[i].cond);
> -qemu_mutex_unlock(&decomp_param[i].mutex);
> -return;
> -}
> -}
> -qemu_cond_wait(&decomp_done_cond, &decomp_done_lock);
> -}
> -}
> -
> -void populate_compress(MigrationInfo *info)
> -{
> -if (!migrate_compress()) {
> -return;
> -}
> -info->compression = g_malloc0(sizeof(*info->compression));
> -info->compression->pages = compression_counters.pages;
> -info->compression->busy = compression_counters.busy;
> -info->compression->busy_rate = compression_counters.busy_rate;
> -info->compression->compressed_size =
> compression_counters.compressed_size;
> -info->compression->compression_rate =
> compression_counters.compression_rate;
> -}
Please also drop the declaration from migration/ram-compress.h.
Not nothing again.
The entire header may well need to go.
Please double-check the entire series for missing header updates.
[...]
QAPI schema
Acked-by: Markus Armbruster
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH 4/6] migration: Remove block migration
Fabiano Rosas writes:
> The block migration has been considered obsolete since QEMU 8.2 in
> favor of the more flexible storage migration provided by the
> blockdev-mirror driver. Two releases have passed so now it's time to
> remove it.
>
> Deprecation commit 66db46ca83 ("migration: Deprecate block
> migration").
>
> Signed-off-by: Fabiano Rosas
[...]
> diff --git a/migration/migration.c b/migration/migration.c
> index a3dc8a7974..0f4df893e5 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
[...]
> @@ -1997,8 +1983,6 @@ static bool migrate_prepare(MigrationState *s, bool
> resume, Error **errp)
> }
> }
>
> -s->must_remove_block_options = true;
> -
> if (migrate_init(s, errp)) {
> return false;
> }
> @@ -2080,7 +2064,6 @@ void qmp_migrate(const char *uri, bool has_channels,
> "a valid migration protocol");
> migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
>MIGRATION_STATUS_FAILED);
> -block_cleanup_parameters();
> }
>
> if (local_err) {
> diff --git a/migration/options.c b/migration/options.c
> index 638eeeb9a0..5049bfb78e 100644
> --- a/migration/options.c
> +++ b/migration/options.c
[...]
> @@ -942,17 +917,6 @@ ZeroPageDetection migrate_zero_page_detection(void)
>
> /* parameters helpers */
>
> -void block_cleanup_parameters(void)
> -{
> -MigrationState *s = migrate_get_current();
> -
> -if (s->must_remove_block_options) {
> -/* setting to false can never fail */
> -migrate_cap_set(MIGRATION_CAPABILITY_BLOCK, false, &error_abort);
> -s->must_remove_block_options = false;
> -}
> -}
> -
MigrationState member @must_remove_block_options is now unused. Please
delete it.
> AnnounceParameters *migrate_announce_params(void)
> {
> static AnnounceParameters ap;
[...]
With that:
Reviewed-by: Markus Armbruster
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH 2/6] migration: Remove 'inc' option from migrate command
Markus Armbruster writes:
> Fabiano Rosas writes:
>
>> The block incremental option for block migration has been deprecated
>> in 8.2 in favor of using the block-mirror feature. Remove it now.
>>
>> Deprecation commit 40101f320d ("migration: migrate 'inc' command
>> option is deprecated.").
>>
>> Signed-off-by: Fabiano Rosas
I think you missed the update to hmp-commands.hx. I almost missed it,
too :)
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH 3/6] migration: Remove 'blk/-b' option from migrate commands
Markus Armbruster writes:
> Fabiano Rosas writes:
>
>> The block migration is considered obsolete and has been deprecated in
>> 8.2. Remove the migrate command option that enables it. This only
>> affects the QMP and HMP commands, the feature can still be accessed by
>> setting the migration 'block' capability. The whole feature will be
>> removed in a future patch.
>>
>> Deprecation commit 8846b5bfca ("migration: migrate 'blk' command
>> option is deprecated.").
>>
>> Signed-off-by: Fabiano Rosas
>
> Reviewed-by: Markus Armbruster
I think you missed the update to hmp-commands.hx. I almost missed it,
too :)
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH 3/6] migration: Remove 'blk/-b' option from migrate commands
Fabiano Rosas writes:
> The block migration is considered obsolete and has been deprecated in
> 8.2. Remove the migrate command option that enables it. This only
> affects the QMP and HMP commands, the feature can still be accessed by
> setting the migration 'block' capability. The whole feature will be
> removed in a future patch.
>
> Deprecation commit 8846b5bfca ("migration: migrate 'blk' command
> option is deprecated.").
>
> Signed-off-by: Fabiano Rosas
Reviewed-by: Markus Armbruster
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH 2/6] migration: Remove 'inc' option from migrate command
Fabiano Rosas writes:
> The block incremental option for block migration has been deprecated
> in 8.2 in favor of using the block-mirror feature. Remove it now.
>
> Deprecation commit 40101f320d ("migration: migrate 'inc' command
> option is deprecated.").
>
> Signed-off-by: Fabiano Rosas
> ---
> docs/about/deprecated.rst | 9 --
> docs/about/removed-features.rst | 14 +
> migration/block.c | 1 -
> migration/migration-hmp-cmds.c | 18 ++-
> migration/migration.c | 24 +--
> migration/options.c | 30 +-
> qapi/migration.json | 54 +++--
> 7 files changed, 35 insertions(+), 115 deletions(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 4d9d6bf2da..25b309dde4 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -488,15 +488,6 @@ option).
> Migration
> -
>
> -``inc`` migrate command option (since 8.2)
> -''''''''''''''''''''''''''''''''''''''''''
> -
> -Use blockdev-mirror with NBD instead.
> -
> -As an intermediate step the ``inc`` functionality can be achieved by
> -setting the ``block-incremental`` migration parameter to ``true``.
> -But this parameter is also deprecated.
> -
> ``blk`` migrate command option (since 8.2)
> ''''''''''''''''''''''''''''''''''''''''''
>
> diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
> index 9873f59bee..e62fc760f1 100644
> --- a/docs/about/removed-features.rst
> +++ b/docs/about/removed-features.rst
> @@ -620,6 +620,13 @@ was superseded by ``sections``.
> Member ``skipped`` of the ``MigrationStats`` struct hasn't been used
> for more than 10 years. Removed with no replacement.
>
> +``migrate`` command option ``inc`` (removed in 9.1)
> +'''''''''''''''''''''''''''''''''''''''''''''''''''
> +
> +Use blockdev-mirror with NBD instead. See "QMP invocation for live
> +storage migration with ``blockdev-mirror`` + NBD" in
> +docs/interop/live-block-operations.rst for a detailed explanation.
You didn't just copy the text from deprecated.rst, you made it more
useful. Appreciated!
> +
> Human Monitor Protocol (HMP) commands
> -
>
> @@ -680,6 +687,13 @@ This command didn't produce any output already. Removed
> with no replacement.
> The ``singlestep`` command has been replaced by the ``one-insn-per-tb``
> command, which has the same behaviour but a less misleading name.
>
> +``migrate`` command ``-i`` option (removed in 9.1)
> +''''''''''''''''''''''''''''''''''''''''''''''''''
> +
> +Use blockdev-mirror with NBD instead. See "QMP invocation for live
> +storage migration with ``blockdev-mirror`` + NBD" in
> +docs/interop/live-block-operations.rst for a detailed explanation.
> +
> Host Architectures
> --
>
[...]
Reviewed-by: Markus Armbruster
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH 0/6] migration removals & deprecations
Fabiano Rosas writes:
> Markus Armbruster writes:
>
>> Doesn't apply for me. What's your base?
>
> 88daa112d4 ("Merge tag 'migration-20240423-pull-request' of
> https://gitlab.com/peterx/qemu into staging")
>
> Probably clashed with the other removals from Philippe.
Thanks!
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH 0/6] migration removals & deprecations
Doesn't apply for me. What's your base? ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH 1/6] migration: Remove 'skipped' field from MigrationStats
Fabiano Rosas writes:
> The 'skipped' field of the MigrationStats struct has been deprecated
> in 8.1. Time to remove it.
>
> Deprecation commit 7b24d32634 ("migration: skipped field is really
> obsolete.").
>
> Signed-off-by: Fabiano Rosas
> ---
> docs/about/deprecated.rst | 6 --
> docs/about/removed-features.rst | 6 ++
> migration/migration-hmp-cmds.c | 2 --
> migration/migration.c | 2 --
> qapi/migration.json | 8
> 5 files changed, 6 insertions(+), 18 deletions(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 7b548519b5..4d9d6bf2da 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -488,12 +488,6 @@ option).
> Migration
> -
>
> -``skipped`` MigrationStats field (since 8.1)
> -''''''''''''''''''''''''''''''''''''''''''''
> -
> -``skipped`` field in Migration stats has been deprecated. It hasn't
> -been used for more than 10 years.
> -
> ``inc`` migrate command option (since 8.2)
> ''''''''''''''''''''''''''''''''''''''''''
>
> diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
> index f9cf874f7b..9873f59bee 100644
> --- a/docs/about/removed-features.rst
> +++ b/docs/about/removed-features.rst
> @@ -614,6 +614,12 @@ was superseded by ``sections``.
> Member ``section-size`` in the return value of ``query-sgx-capabilities``
> was superseded by ``sections``.
>
> +``query-migrate`` return value member ``skipped`` (removed in 9.1)
> +''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> +
> +Member ``skipped`` of the ``MigrationStats`` struct hasn't been used
> +for more than 10 years. Removed with no replacement.
> +
> Human Monitor Protocol (HMP) commands
> -
>
> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index 7e96ae6ffd..28f776d06d 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -105,8 +105,6 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
> info->ram->total >> 10);
> monitor_printf(mon, "duplicate: %" PRIu64 " pages\n",
> info->ram->duplicate);
> -monitor_printf(mon, "skipped: %" PRIu64 " pages\n",
> - info->ram->skipped);
> monitor_printf(mon, "normal: %" PRIu64 " pages\n",
> info->ram->normal);
> monitor_printf(mon, "normal bytes: %" PRIu64 " kbytes\n",
> diff --git a/migration/migration.c b/migration/migration.c
> index 696762bc64..3b433fdb31 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1149,8 +1149,6 @@ static void populate_ram_info(MigrationInfo *info,
> MigrationState *s)
> info->ram->transferred = migration_transferred_bytes();
> info->ram->total = ram_bytes_total();
> info->ram->duplicate = stat64_get(&mig_stats.zero_pages);
> -/* legacy value. It is not used anymore */
> -info->ram->skipped = 0;
> info->ram->normal = stat64_get(&mig_stats.normal_pages);
> info->ram->normal_bytes = info->ram->normal * page_size;
> info->ram->mbps = s->mbps;
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 8c65b90328..401b8e24ac 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -23,9 +23,6 @@
> #
> # @duplicate: number of duplicate (zero) pages (since 1.2)
> #
> -# @skipped: number of skipped zero pages. Always zero, only provided
> -# for compatibility (since 1.5)
> -#
> # @normal: number of normal pages (since 1.2)
> #
> # @normal-bytes: number of normal bytes sent (since 1.2)
> @@ -63,16 +60,11 @@
> # between 0 and @dirty-sync-count * @multifd-channels. (since
> # 7.1)
> #
> -# Features:
> -#
> -# @deprecated: Member @skipped is always zero since 1.5.3
> -#
> # Since: 0.14
> ##
> { 'struct': 'MigrationStats',
>'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
> 'duplicate': 'int',
> - 'skipped': { 'type': 'int', 'features': [ 'deprecated' ] },
> 'normal': 'int',
> 'normal-bytes': 'int', 'dirty-pages-rate': 'int',
> 'mbps': 'number', 'dirty-sync-count': 'int',
Reviewed-by: Markus Armbruster
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH-for-9.1 v2 2/3] migration: Remove RDMA protocol handling
Peter Xu writes: > On Mon, Apr 08, 2024 at 04:07:20PM +0200, Jinpu Wang wrote: >> Hi Peter, > > Jinpu, > > Thanks for joining the discussion. > >> >> On Tue, Apr 2, 2024 at 11:24 PM Peter Xu wrote: >> > >> > On Mon, Apr 01, 2024 at 11:26:25PM +0200, Yu Zhang wrote: >> > > Hello Peter und Zhjian, >> > > >> > > Thank you so much for letting me know about this. I'm also a bit >> > > surprised at >> > > the plan for deprecating the RDMA migration subsystem. >> > >> > It's not too late, since it looks like we do have users not yet notified >> > from this, we'll redo the deprecation procedure even if it'll be the final >> > plan, and it'll be 2 releases after this. [...] >> > Per our best knowledge, RDMA users are rare, and please let anyone know if >> > you are aware of such users. IIUC the major reason why RDMA stopped being >> > the trend is because the network is not like ten years ago; I don't think I >> > have good knowledge in RDMA at all nor network, but my understanding is >> > it's pretty easy to fetch modern NIC to outperform RDMAs, then it may make >> > little sense to maintain multiple protocols, considering RDMA migration >> > code is so special so that it has the most custom code comparing to other >> > protocols. >> +cc some guys from Huawei. >> >> I'm surprised RDMA users are rare, I guess maybe many are just >> working with different code base. > > Yes, please cc whoever might be interested (or surprised.. :) to know this, > and let's be open to all possibilities. > > I don't think it makes sense if there're a lot of users of a feature then > we deprecate that without a good reason. However there's always the > resource limitation issue we're facing, so it could still have the > possibility that this gets deprecated if nobody is working on our upstream > branch. Say, if people use private branches anyway to support rdma without > collaborating upstream, keeping such feature upstream then may not make > much sense either, unless there's some way to collaborate. We'll see. > > It seems there can still be people joining this discussion. I'll hold off > a bit on merging this patch to provide enough window for anyone to chim in. Users are not enough. Only maintainers are. At some point, people cared enough about RDMA in QEMU to contribute the code. That's why have the code. To keep the code, we need people who care enough about RDMA in QEMU to maintain it. Without such people, the case for keeping it remains dangerously weak, and no amount of talk or even benchmarks can change that. ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [RFC 01/15] scripts/qapi: support type-based unions
Subject: all unions are type-based. Perhaps "support implicit union tags on the wire"? Do you need this schema language feature for folding block jobs into the jobs abstraction, or is it just for making the wire protocol nicer in places? ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [RFC 04/15] qapi: block-job-change: make copy-mode parameter optional
Vladimir Sementsov-Ogievskiy writes:
> We are going to add more parameters to change. We want to make possible
> to change only one or any subset of available options. So all the
> options should be optional.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> block/mirror.c | 5 +
> qapi/block-core.json | 2 +-
> 2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index a177502e4f..2d0cd22c06 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1265,6 +1265,11 @@ static void mirror_change(BlockJob *job,
> JobChangeOptions *opts,
>
> GLOBAL_STATE_CODE();
>
> +if (!change_opts->has_copy_mode) {
> +/* Nothing to do */
I doubt the comment is useful.
> +return;
> +}
> +
> if (qatomic_read(&s->copy_mode) == change_opts->copy_mode) {
> return;
> }
if (change_opts->copy_mode != MIRROR_COPY_MODE_WRITE_BLOCKING) {
error_setg(errp, "Change to copy mode '%s' is not implemented",
MirrorCopyMode_str(change_opts->copy_mode));
return;
}
current = qatomic_cmpxchg(&s->copy_mode, MIRROR_COPY_MODE_BACKGROUND,
change_opts->copy_mode);
if (current != MIRROR_COPY_MODE_BACKGROUND) {
error_setg(errp, "Expected current copy mode '%s', got '%s'",
MirrorCopyMode_str(MIRROR_COPY_MODE_BACKGROUND),
MirrorCopyMode_str(current));
}
Now I'm curious: what could be changing @copy_mode behind our backs
here?
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 67dd0ef038..6041e7bd8f 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3071,7 +3071,7 @@
##
# @BlockJobChangeOptionsMirror:
#
# @copy-mode: Switch to this copy mode. Currently, only the switch
# from 'background' to 'write-blocking' is implemented.
#
> # Since: 8.2
> ##
> { 'struct': 'JobChangeOptionsMirror',
> - 'data': { 'copy-mode' : 'MirrorCopyMode' } }
> + 'data': { '*copy-mode' : 'MirrorCopyMode' } }
>
> ##
> # @JobChangeOptions:
A member becoming optional is backward compatible. Okay.
We may want to document "(optional since 9.1)". We haven't done so in
the past.
The doc comment needs to spell out how absent members are handled.
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [RFC 01/15] scripts/qapi: support type-based unions
Vladimir Sementsov-Ogievskiy writes:
> Look at block-job-change command: we have to specify both 'id' to chose
> the job to operate on and 'type' for QAPI union be parsed. But for user
> this looks redundant: when we specify 'id', QEMU should be able to get
> corresponding job's type.
>
> This commit brings such a possibility: just specify some Enum type as
> 'discriminator', and define a function somewhere with prototype
>
> bool YourUnionType_mapper(YourUnionType *, EnumType *out, Error **errp)
>
> Further commits will use this functionality to upgrade block-job-change
> interface and introduce some new interfaces.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> scripts/qapi/introspect.py | 5 +++-
> scripts/qapi/schema.py | 50 +++---
> scripts/qapi/types.py | 3 ++-
> scripts/qapi/visit.py | 43 ++--
> 4 files changed, 73 insertions(+), 28 deletions(-)
I believe you need to update docs/devel/qapi-code-gen.rst.
Current text:
Union types
---
Syntax::
UNION = { 'union': STRING,
'base': ( MEMBERS | STRING ),
'discriminator': STRING,
'data': BRANCHES,
'*if': COND,
'*features': FEATURES }
BRANCHES = { BRANCH, ... }
BRANCH = STRING : TYPE-REF
| STRING : { 'type': TYPE-REF, '*if': COND }
Member 'union' names the union type.
The 'base' member defines the common members. If it is a MEMBERS_
object, it defines common members just like a struct type's 'data'
member defines struct type members. If it is a STRING, it names a
struct type whose members are the common members.
Member 'discriminator' must name a non-optional enum-typed member of
the base struct. That member's value selects a branch by its name.
If no such branch exists, an empty branch is assumed.
If I understand your commit message correctly, this paragraph is no
longer true.
Each BRANCH of the 'data' object defines a branch of the union. A
union must have at least one branch.
The BRANCH's STRING name is the branch name. It must be a value of
the discriminator enum type.
The BRANCH's value defines the branch's properties, in particular its
type. The type must a struct type. The form TYPE-REF_ is shorthand
for :code:`{ 'type': TYPE-REF }`.
In the Client JSON Protocol, a union is represented by an object with
the common members (from the base type) and the selected branch's
members. The two sets of member names must be disjoint.
Example::
{ 'enum': 'BlockdevDriver', 'data': [ 'file', 'qcow2' ] }
{ 'union': 'BlockdevOptions',
'base': { 'driver': 'BlockdevDriver', '*read-only': 'bool' },
'discriminator': 'driver',
'data': { 'file': 'BlockdevOptionsFile',
'qcow2': 'BlockdevOptionsQcow2' } }
Resulting in these JSON objects::
{ "driver": "file", "read-only": true,
"filename": "/some/place/my-image" }
{ "driver": "qcow2", "read-only": false,
"backing": "/some/place/my-image", "lazy-refcounts": true }
The order of branches need not match the order of the enum values.
The branches need not cover all possible enum values. In the
resulting generated C data types, a union is represented as a struct
with the base members in QAPI schema order, and then a union of
structures for each branch of the struct.
The optional 'if' member specifies a conditional. See `Configuring
the schema`_ below for more on this.
The optional 'features' member specifies features. See Features_
below for more on this.
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH 1/4] chardev/parallel: Don't close stdin on inappropriate device
Markus Armbruster writes:
> The __linux__ version of qemu_chr_open_pp_fd() tries to claim the
> parport device with a PPCLAIM ioctl(). On success, it stores the file
> descriptor in the chardev object, and returns success. On failure, it
> closes the file descriptor, and returns failure.
>
> chardev_new() then passes the Chardev to object_unref(). This duly
> calls char_parallel_finalize(), which closes the file descriptor
> stored in the chardev object. Since qemu_chr_open_pp_fd() didn't
> store it, it's still zero, so this closes standard input. Ooopsie.
>
> To demonstate, add a unit test. With the bug above unfixed, running
> this test closes standard input. char_hotswap_test() happens to run
> next. It opens a socket, duly gets file descriptor 0, and since it
> tests for success with > 0 instead of >= 0, it fails.
>
> The test needs to be conditional exactly like the chardev it tests.
> Since the condition is rather complicated, steal the solution from the
> serial chardev: define HAVE_CHARDEV_PARALLEL in qemu/osdep.h. This
> also permits simplifying chardev/meson.build a bit.
>
> The bug fix is easy enough: store the file descriptor, and leave
> closing it to char_parallel_finalize().
>
> Signed-off-by: Markus Armbruster
[...]
> diff --git a/chardev/char-parallel.c b/chardev/char-parallel.c
> index a5164f975a..78697d7522 100644
> --- a/chardev/char-parallel.c
> +++ b/chardev/char-parallel.c
> @@ -164,13 +164,13 @@ static void qemu_chr_open_pp_fd(Chardev *chr,
> {
> ParallelChardev *drv = PARALLEL_CHARDEV(chr);
>
> +drv->fd = fd;
> +
> if (ioctl(fd, PPCLAIM) < 0) {
> error_setg_errno(errp, errno, "not a parallel port");
> -close(fd);
> return;
> }
>
> -drv->fd = fd;
> drv->mode = IEEE1284_MODE_COMPAT;
> }
> #endif /* __linux__ */
> @@ -238,6 +238,7 @@ static void qemu_chr_open_pp_fd(Chardev *chr,
> }
> #endif
>
> +#ifdef HAVE_CHARDEV_PARALLEL
> static void qmp_chardev_open_parallel(Chardev *chr,
>ChardevBackend *backend,
>bool *be_opened,
> @@ -306,3 +307,5 @@ static void register_types(void)
> }
>
> type_init(register_types);
> +
> +#endif /* HAVE_CHARDEV_PARALLEL */
> diff --git a/tests/unit/test-char.c b/tests/unit/test-char.c
> index 649fdf64e1..76946e6f90 100644
> --- a/tests/unit/test-char.c
> +++ b/tests/unit/test-char.c
> @@ -1203,6 +1203,24 @@ static void char_serial_test(void)
> }
> #endif
>
> +#if defined(HAVE_CHARDEV_PARALLEL) && !defined(WIN32)
> +static void char_parallel_test(void)
> +{
> +QemuOpts *opts;
> +Chardev *chr;
> +
> +opts = qemu_opts_create(qemu_find_opts("chardev"), "parallel-id",
> +1, &error_abort);
> +qemu_opt_set(opts, "backend", "parallel", &error_abort);
> +qemu_opt_set(opts, "path", "/dev/null", &error_abort);
> +
> +chr = qemu_chr_new_from_opts(opts, NULL, NULL);
> +g_assert_null(chr);
This is wrong.
On a Linux host, qemu_chr_new_from_opts() fails, because
qemu_chr_open_pp_fd()'s attempt to PPCLAIM fails.
On a BSD host, it succeeds.
Proposed fixup appended. Marc-André, is respinning the PR with the
fixup okay, or would you prefer a v2?
> +
> +qemu_opts_del(opts);
> +}
> +#endif
> +
> #ifndef _WIN32
> static void char_file_fifo_test(void)
> {
> @@ -1544,6 +1562,9 @@ int main(int argc, char **argv)
> g_test_add_func("/char/udp", char_udp_test);
> #if defined(HAVE_CHARDEV_SERIAL) && !defined(WIN32)
> g_test_add_func("/char/serial", char_serial_test);
> +#endif
> +#if defined(HAVE_CHARDEV_PARALLEL) && !defined(WIN32)
> +g_test_add_func("/char/parallel", char_parallel_test);
> #endif
> g_test_add_func("/char/hotswap", char_hotswap_test);
> g_test_add_func("/char/websocket", char_websock_test);
[...]
diff --git a/tests/unit/test-char.c b/tests/unit/test-char.c
index e3b783c06b..f273ce5226 100644
--- a/tests/unit/test-char.c
+++ b/tests/unit/test-char.c
@@ -1215,7 +1215,13 @@ static void char_parallel_test(void)
qemu_opt_set(opts, "path", "/dev/null", &error_abort);
chr = qemu_chr_new_from_opts(opts, NULL, NULL);
+#ifdef __linux__
+/* fails to PPCLAIM, see qemu_chr_open_pp_fd() */
g_assert_null(chr);
+#else
+g_assert_nonnull(chr);
+object_unparent(OBJECT(chr));
+#endif
qemu_opts_del(opts);
}
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH 2/4] tests/unit/test-char: Fix qemu_socket(), make_udp_socket() check
Eric Blake writes: > On Sat, Feb 03, 2024 at 09:02:26AM +0100, Markus Armbruster wrote: >> qemu_socket() and make_udp_socket() return a file descriptor on >> success, -1 on failure. The check misinterprets 0 as failure. Fix >> that. >> >> Signed-off-by: Markus Armbruster >> --- >> tests/unit/test-char.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) > > Reviewed-by: Eric Blake > > Might be worth amending the commit message of 1/4 where you called out > this bug to mention it will be fixed in the next patch. Yes. Thanks! ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH 1/4] chardev/parallel: Don't close stdin on inappropriate device
Eric Blake writes: > On Sat, Feb 03, 2024 at 09:02:25AM +0100, Markus Armbruster wrote: >> The __linux__ version of qemu_chr_open_pp_fd() tries to claim the >> parport device with a PPCLAIM ioctl(). On success, it stores the file >> descriptor in the chardev object, and returns success. On failure, it >> closes the file descriptor, and returns failure. >> >> chardev_new() then passes the Chardev to object_unref(). This duly >> calls char_parallel_finalize(), which closes the file descriptor >> stored in the chardev object. Since qemu_chr_open_pp_fd() didn't >> store it, it's still zero, so this closes standard input. Ooopsie. >> >> To demonstate, add a unit test. With the bug above unfixed, running >> this test closes standard input. char_hotswap_test() happens to run >> next. It opens a socket, duly gets file descriptor 0, and since it >> tests for success with > 0 instead of >= 0, it fails. > > Two bugs for the price of one! > >> >> The test needs to be conditional exactly like the chardev it tests. >> Since the condition is rather complicated, steal the solution from the >> serial chardev: define HAVE_CHARDEV_PARALLEL in qemu/osdep.h. This >> also permits simplifying chardev/meson.build a bit. >> >> The bug fix is easy enough: store the file descriptor, and leave >> closing it to char_parallel_finalize(). >> >> Signed-off-by: Markus Armbruster >> --- > >> +++ b/include/qemu/osdep.h >> @@ -508,11 +508,18 @@ void qemu_anon_ram_free(void *ptr, size_t size); >> >> #ifdef _WIN32 >> #define HAVE_CHARDEV_SERIAL 1 >> -#elif defined(__linux__) || defined(__sun__) || defined(__FreeBSD__)\ >> +#define HAVE_CHARDEV_PARALLEL 1 >> +#else >> +#if defined(__linux__) || defined(__sun__) || defined(__FreeBSD__) \ >> || defined(__NetBSD__) || defined(__OpenBSD__) || >> defined(__DragonFly__) \ >> || defined(__GLIBC__) || defined(__APPLE__) >> #define HAVE_CHARDEV_SERIAL 1 >> #endif >> +#if defined(__linux__) || defined(__FreeBSD__) \ >> +|| defined(__FreeBSD_kernel__) || defined(__DragonFly__) >> +#define HAVE_CHARDEV_PARALLEL 1 >> +#endif >> +#endif > > Not for this patch, but I've grown to like a preprocessor style I've > seen in other projects to make it easier to read nested #if: > > #ifdef _WIN32 > # define HAVE_CHARDEV_SERIAL 1 > # define HAVE_CHARDEV_PARALLEL 1 > #else > # if defined(__linux__) ... defined(__APPLE__) > # define HAVE_CHARDEV_SERIAL 1 > # endif > # if defined(__linux__) ... defined(__DragonFly__) > # define HAVE_CHARDEV_PARALLEL 1 > # endif > #endif I like this style, too. >> +++ b/chardev/meson.build >> @@ -21,11 +21,9 @@ if host_os == 'windows' >> else >>chardev_ss.add(files( >>'char-fd.c', >> +'char-parallel.c', >>'char-pty.c', > > Indentation looks off. Otherwise, Will fix. > Reviewed-by: Eric Blake Thanks! ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH 0/4] char: Minor fixes, and a tighter QAPI schema
Markus Armbruster (4): chardev/parallel: Don't close stdin on inappropriate device tests/unit/test-char: Fix qemu_socket(), make_udp_socket() check qapi/char: Make backend types properly conditional qapi/char: Deprecate backend type "memory" docs/about/deprecated.rst | 8 qapi/char.json| 28 +--- include/qemu/osdep.h | 9 - chardev/char-parallel.c | 7 +-- tests/unit/test-char.c| 25 +++-- chardev/meson.build | 4 +--- 6 files changed, 62 insertions(+), 19 deletions(-) -- 2.43.0 ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH 4/4] qapi/char: Deprecate backend type "memory"
It's an alias for "ringbuf" we kept for backward compatibility; see
commit 3a1da42eb35 (qapi: Rename ChardevBackend member "memory" to
"ringbuf"). Deprecation is long overdue.
Signed-off-by: Markus Armbruster
---
docs/about/deprecated.rst | 8
qapi/char.json| 8 +---
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index d4492b9460..ae1a520c26 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -371,6 +371,14 @@ Specifying the iSCSI password in plain text on the command
line using the
used instead, to refer to a ``--object secret...`` instance that provides
a password via a file, or encrypted.
+Character device options
+''''''''''''''''''''''''
+
+Backend ``memory`` (since 9.0)
+^^
+
+``memory`` is a deprecated synonym for ``ringbuf``.
+
CPU device properties
'''''''''''''''''''''
diff --git a/qapi/char.json b/qapi/char.json
index 2d74e66746..75a7e057f0 100644
--- a/qapi/char.json
+++ b/qapi/char.json
@@ -468,6 +468,10 @@
#
# @memory: Since 1.5
#
+# Features:
+#
+# @deprecated: Member @memory is deprecated. Use @ringbuf instead.
+#
# Since: 1.4
##
{ 'enum': 'ChardevBackendKind',
@@ -492,8 +496,7 @@
{ 'name': 'dbus', 'if': 'CONFIG_DBUS_DISPLAY' },
'vc',
'ringbuf',
-# next one is just for compatibility
-'memory' ] }
+{ 'name': 'memory', 'features': [ 'deprecated' ] } ] }
##
# @ChardevFileWrapper:
@@ -642,7 +645,6 @@
'if': 'CONFIG_DBUS_DISPLAY' },
'vc': 'ChardevVCWrapper',
'ringbuf': 'ChardevRingbufWrapper',
-# next one is just for compatibility
'memory': 'ChardevRingbufWrapper' } }
##
--
2.43.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH 1/4] chardev/parallel: Don't close stdin on inappropriate device
The __linux__ version of qemu_chr_open_pp_fd() tries to claim the
parport device with a PPCLAIM ioctl(). On success, it stores the file
descriptor in the chardev object, and returns success. On failure, it
closes the file descriptor, and returns failure.
chardev_new() then passes the Chardev to object_unref(). This duly
calls char_parallel_finalize(), which closes the file descriptor
stored in the chardev object. Since qemu_chr_open_pp_fd() didn't
store it, it's still zero, so this closes standard input. Ooopsie.
To demonstate, add a unit test. With the bug above unfixed, running
this test closes standard input. char_hotswap_test() happens to run
next. It opens a socket, duly gets file descriptor 0, and since it
tests for success with > 0 instead of >= 0, it fails.
The test needs to be conditional exactly like the chardev it tests.
Since the condition is rather complicated, steal the solution from the
serial chardev: define HAVE_CHARDEV_PARALLEL in qemu/osdep.h. This
also permits simplifying chardev/meson.build a bit.
The bug fix is easy enough: store the file descriptor, and leave
closing it to char_parallel_finalize().
Signed-off-by: Markus Armbruster
---
include/qemu/osdep.h| 9 -
chardev/char-parallel.c | 7 +--
tests/unit/test-char.c | 21 +
chardev/meson.build | 4 +---
4 files changed, 35 insertions(+), 6 deletions(-)
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index c9692cc314..3b0f3389d3 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -508,11 +508,18 @@ void qemu_anon_ram_free(void *ptr, size_t size);
#ifdef _WIN32
#define HAVE_CHARDEV_SERIAL 1
-#elif defined(__linux__) || defined(__sun__) || defined(__FreeBSD__)\
+#define HAVE_CHARDEV_PARALLEL 1
+#else
+#if defined(__linux__) || defined(__sun__) || defined(__FreeBSD__) \
|| defined(__NetBSD__) || defined(__OpenBSD__) || defined(__DragonFly__) \
|| defined(__GLIBC__) || defined(__APPLE__)
#define HAVE_CHARDEV_SERIAL 1
#endif
+#if defined(__linux__) || defined(__FreeBSD__) \
+|| defined(__FreeBSD_kernel__) || defined(__DragonFly__)
+#define HAVE_CHARDEV_PARALLEL 1
+#endif
+#endif
#if defined(__HAIKU__)
#define SIGIO SIGPOLL
diff --git a/chardev/char-parallel.c b/chardev/char-parallel.c
index a5164f975a..78697d7522 100644
--- a/chardev/char-parallel.c
+++ b/chardev/char-parallel.c
@@ -164,13 +164,13 @@ static void qemu_chr_open_pp_fd(Chardev *chr,
{
ParallelChardev *drv = PARALLEL_CHARDEV(chr);
+drv->fd = fd;
+
if (ioctl(fd, PPCLAIM) < 0) {
error_setg_errno(errp, errno, "not a parallel port");
-close(fd);
return;
}
-drv->fd = fd;
drv->mode = IEEE1284_MODE_COMPAT;
}
#endif /* __linux__ */
@@ -238,6 +238,7 @@ static void qemu_chr_open_pp_fd(Chardev *chr,
}
#endif
+#ifdef HAVE_CHARDEV_PARALLEL
static void qmp_chardev_open_parallel(Chardev *chr,
ChardevBackend *backend,
bool *be_opened,
@@ -306,3 +307,5 @@ static void register_types(void)
}
type_init(register_types);
+
+#endif /* HAVE_CHARDEV_PARALLEL */
diff --git a/tests/unit/test-char.c b/tests/unit/test-char.c
index 649fdf64e1..76946e6f90 100644
--- a/tests/unit/test-char.c
+++ b/tests/unit/test-char.c
@@ -1203,6 +1203,24 @@ static void char_serial_test(void)
}
#endif
+#if defined(HAVE_CHARDEV_PARALLEL) && !defined(WIN32)
+static void char_parallel_test(void)
+{
+QemuOpts *opts;
+Chardev *chr;
+
+opts = qemu_opts_create(qemu_find_opts("chardev"), "parallel-id",
+1, &error_abort);
+qemu_opt_set(opts, "backend", "parallel", &error_abort);
+qemu_opt_set(opts, "path", "/dev/null", &error_abort);
+
+chr = qemu_chr_new_from_opts(opts, NULL, NULL);
+g_assert_null(chr);
+
+qemu_opts_del(opts);
+}
+#endif
+
#ifndef _WIN32
static void char_file_fifo_test(void)
{
@@ -1544,6 +1562,9 @@ int main(int argc, char **argv)
g_test_add_func("/char/udp", char_udp_test);
#if defined(HAVE_CHARDEV_SERIAL) && !defined(WIN32)
g_test_add_func("/char/serial", char_serial_test);
+#endif
+#if defined(HAVE_CHARDEV_PARALLEL) && !defined(WIN32)
+g_test_add_func("/char/parallel", char_parallel_test);
#endif
g_test_add_func("/char/hotswap", char_hotswap_test);
g_test_add_func("/char/websocket", char_websock_test);
diff --git a/chardev/meson.build b/chardev/meson.build
index c80337d15f..b39028b7b0 100644
--- a/chardev/meson.build
+++ b/chardev/meson.build
@@ -21,11 +21,9 @@ if host_os == 'windows'
else
chardev_ss.add(files(
'char-fd.c',
+'char-parallel.c',
'char-pty.c',
), util)
- if host_os in ['linux', 'gnu/kf
[PATCH 3/4] qapi/char: Make backend types properly conditional
Character backends are actually QOM types. When a backend's
compile-time conditional QOM type is not compiled in, creation fails
with "'FOO' is not a valid char driver name". Okay, except
introspecting chardev-add with query-qmp-schema doesn't work then: the
backend type is there even though the QOM type isn't.
A management application can work around this issue by using
qom-list-types instead.
Fix the issue anyway: add the conditionals to the QAPI schema.
Signed-off-by: Markus Armbruster
---
qapi/char.json | 20
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/qapi/char.json b/qapi/char.json
index 6c6ad3b10c..2d74e66746 100644
--- a/qapi/char.json
+++ b/qapi/char.json
@@ -472,8 +472,8 @@
##
{ 'enum': 'ChardevBackendKind',
'data': [ 'file',
-'serial',
-'parallel',
+{ 'name': 'serial', 'if': 'HAVE_CHARDEV_SERIAL' },
+{ 'name': 'parallel', 'if': 'HAVE_CHARDEV_PARALLEL' },
'pipe',
'socket',
'udp',
@@ -482,10 +482,10 @@
'mux',
'msmouse',
'wctablet',
-'braille',
+{ 'name': 'braille', 'if': 'CONFIG_BRLAPI' },
'testdev',
'stdio',
-'console',
+{ 'name': 'console', 'if': 'CONFIG_WIN32' },
{ 'name': 'spicevmc', 'if': 'CONFIG_SPICE' },
{ 'name': 'spiceport', 'if': 'CONFIG_SPICE' },
{ 'name': 'qemu-vdagent', 'if': 'CONFIG_SPICE_PROTOCOL' },
@@ -614,8 +614,10 @@
'base': { 'type': 'ChardevBackendKind' },
'discriminator': 'type',
'data': { 'file': 'ChardevFileWrapper',
-'serial': 'ChardevHostdevWrapper',
-'parallel': 'ChardevHostdevWrapper',
+'serial': { 'type': 'ChardevHostdevWrapper',
+'if': 'HAVE_CHARDEV_SERIAL' },
+'parallel': { 'type': 'ChardevHostdevWrapper',
+ 'if': 'HAVE_CHARDEV_PARALLEL' },
'pipe': 'ChardevHostdevWrapper',
'socket': 'ChardevSocketWrapper',
'udp': 'ChardevUdpWrapper',
@@ -624,10 +626,12 @@
'mux': 'ChardevMuxWrapper',
'msmouse': 'ChardevCommonWrapper',
'wctablet': 'ChardevCommonWrapper',
-'braille': 'ChardevCommonWrapper',
+'braille': { 'type': 'ChardevCommonWrapper',
+ 'if': 'CONFIG_BRLAPI' },
'testdev': 'ChardevCommonWrapper',
'stdio': 'ChardevStdioWrapper',
-'console': 'ChardevCommonWrapper',
+'console': { 'type': 'ChardevCommonWrapper',
+ 'if': 'CONFIG_WIN32' },
'spicevmc': { 'type': 'ChardevSpiceChannelWrapper',
'if': 'CONFIG_SPICE' },
'spiceport': { 'type': 'ChardevSpicePortWrapper',
--
2.43.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH 2/4] tests/unit/test-char: Fix qemu_socket(), make_udp_socket() check
qemu_socket() and make_udp_socket() return a file descriptor on
success, -1 on failure. The check misinterprets 0 as failure. Fix
that.
Signed-off-by: Markus Armbruster
---
tests/unit/test-char.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/unit/test-char.c b/tests/unit/test-char.c
index 76946e6f90..e3b783c06b 100644
--- a/tests/unit/test-char.c
+++ b/tests/unit/test-char.c
@@ -556,7 +556,7 @@ static int make_udp_socket(int *port)
socklen_t alen = sizeof(addr);
int ret, sock = qemu_socket(PF_INET, SOCK_DGRAM, 0);
-g_assert_cmpint(sock, >, 0);
+g_assert_cmpint(sock, >=, 0);
addr.sin_family = AF_INET ;
addr.sin_addr.s_addr = htonl(INADDR_ANY);
addr.sin_port = 0;
@@ -1401,7 +1401,7 @@ static void char_hotswap_test(void)
int port;
int sock = make_udp_socket(&port);
-g_assert_cmpint(sock, >, 0);
+g_assert_cmpint(sock, >=, 0);
chr_args = g_strdup_printf("udp:127.0.0.1:%d", port);
--
2.43.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH 0/3] qapi/hmp/cli: Remove the deprecated 'singlestep'
Philippe Mathieu-Daudé writes: > Deprecated for 8.1, good to go for 9.0. > > Based-on: <20240112100059.965041-5-th...@redhat.com> > "Remove deprecated command line options" Series Reviewed-by: Markus Armbruster ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
