Re: [PATCH 09/13] qapi: convert "Note" sections to plain rST
John Snow writes: > On Fri, Jun 21, 2024 at 8:23 AM Markus Armbruster wrote: [...] >> My reason for four spaces is reducing churn. To see by how much, I >> redid your change. I found a few more notes that don't start with a >> capital letter, or don't end with a period. >> > > ^ Guess I'll re-audit for v2. Hang on to the list of cases you found. Happy to share my patch. > (Sorry for the churn, though. I obviously don't mind it as much as you do, > but I suspect I'm a lot less nimble with fiddling through git history than > you are and find the value of avoiding churn to be ... lower than you do, > in general. Respecting reviewer time is a strong argument, I apologize that > some non-mechanical changes snuck into the patch. The downside of hacking > together a very large series.) You did a good job splitting it up. Minor mistakes are bound to happen. Got to give the reviewer soemthing to find ;) [...]
Re: [PATCH 04/13] qapi/parser: preserve indentation in QAPIDoc sections
John Snow writes: > On Fri, Jun 21, 2024 at 2:38 AM Markus Armbruster wrote: [...] >> I'd like you to express more clearly that you're talking about an >> alternative you rejected. Perhaps like this: >> >> block-level constructs such as code blocks, lists, and other such >> markup. >> >> The alternative would be to somehow undo .get_doc_indented()'s >> indentation changes in the new generator. Much messier. >> >> Feel free to add more detail to the last paragraph. >> > > Eh, I just deleted it. I recall running into troubles but I can't > articulate the precise conditions because as you point out, it's a doomed > strategy for other reasons - you can't reconstruct the proper indentation. > > This patch is still the correct way to go, so I don't have to explain my > failures at length in the commit message ... I just like giving people > clues for *why* I decided to implement things a certain way, because I > often find that more instructive than the "how". "Why" tends to be much more useful in a commit message than "how". I should be able to figure out "how" by reading the patch, whereas for "why", I may have to read the author's mind. > In this case, the "why" is > probably more properly summarized as "it's a total shitshow in that > direction, trust me" The right amount of detail is often not obvious. Use your judgement.
Re: [PATCH 09/13] qapi: convert "Note" sections to plain rST
John Snow writes: > On Thu, Jun 20, 2024 at 11:46 AM John Snow wrote: > >> >> >> On Thu, Jun 20, 2024, 9:35 AM Markus Armbruster wrote: >> >>> Markus Armbruster writes: >>> >>> > John Snow writes: >>> >>> [...] >>> >>> >> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json >>> >> index b3de1fb6b3a..57598331c5c 100644 >>> >> --- a/qga/qapi-schema.json >>> >> +++ b/qga/qapi-schema.json >>> >>> [...] >>> >>> >> @@ -631,8 +632,8 @@ >>> >> # Errors: >>> >> # - If hybrid suspend is not supported, Unsupported >>> >> # >>> >> -# Notes: It's strongly recommended to issue the guest-sync command >>> >> -# before sending commands when the guest resumes >>> >> +# .. note:: It's strongly recommended to issue the guest-sync command >>> >> +#before sending commands when the guest resumes. >>> >> # >>> >> # Since: 1.1 >>> >> ## >>> >> @@ -1461,16 +1462,15 @@ >>> >> # * POSIX: as defined by os-release(5) >>> >> # * Windows: contains string "server" or "client" >>> >> # >>> >> -# Notes: On POSIX systems the fields @id, @name, @pretty-name, >>> >> -# @version, @version-id, @variant and @variant-id follow the >>> >> -# definition specified in os-release(5). Refer to the manual page >>> >> -# for exact description of the fields. Their values are taken >>> >> -# from the os-release file. If the file is not present in the >>> >> -# system, or the values are not present in the file, the fields >>> >> -# are not included. >>> >> +# .. note:: On POSIX systems the fields @id, @name, @pretty-name, >>> >> +#@version, @version-id, @variant and @variant-id follow the >>> >> +#definition specified in os-release(5). Refer to the manual page >>> for >>> >> +#exact description of the fields. Their values are taken from the >>> >> +#os-release file. If the file is not present in the system, or >>> the >>> >> +#values are not present in the file, the fields are not included. >>> >> # >>> >> -# On Windows the values are filled from information gathered from >>> >> -# the system. >>> >> +#On Windows the values are filled from information gathered from >>> >> +#the system. >>> > >>> > Please don't change the indentation here. I get the same output with >>> > >>> > @@ -1461,7 +1462,7 @@ >>> ># * POSIX: as defined by os-release(5) >>> ># * Windows: contains string "server" or "client" >>> ># >>> > -# Notes: On POSIX systems the fields @id, @name, @pretty-name, >>> > +# .. note:: On POSIX systems the fields @id, @name, @pretty-name, >>> ># @version, @version-id, @variant and @variant-id follow the >>> ># definition specified in os-release(5). Refer to the manual page >>> ># for exact description of the fields. Their values are taken >>> >>> I'm blind. Actually, you change indentation of subsequent lines from 4 >>> to 3 *everywhere*. I guess you do that to make subsequent lines line up >>> with the directive, here "note". >>> >>> Everywhere else, we indent such lines by 4. Hmm. How terrible would it >>> be not to mess with the alignment? >>> >>> If we want to use 3 for directives, is it worth pointing out in the >>> commit message? >>> >>> [...] >>> >> >> Let me look up some conventions and see what's the most prominent... as >> well as testing what emacs does by default (or if emacs can be configured >> to do what we want with in-tree style config. Warning: I am functionally >> inept at emacs lisp. Warning 2x: [neo]vi[m] users, you're entirely on your >> own. I'm sorry.) >> >> I use three myself by force of habit and have some mild reluctance to >> respin for that reason, but ... guess we ought to be consistent if we can. >> >> (No idea where my habit came from. Maybe it is just because it looks nice >> to me and no other reason.) >> >> ((I have no plans, nor desire, to write any kind of checker to enforce >> this, thou
Re: [PATCH 09/13] qapi: convert "Note" sections to plain rST
John Snow writes:
> We do not need a dedicated section for notes. By eliminating a specially
> parsed section, these notes can be treated as normal rST paragraphs in
> the new QMP reference manual, and can be placed and styled much more
> flexibly.
>
> Convert all existing "Note" and "Notes" sections to pure rST. As part of
> the conversion, capitalize the first letter of each sentence and add
> trailing punctuation where appropriate to ensure notes look sensible and
> consistent in rendered HTML documentation.
>
> Update docs/devel/qapi-code-gen.rst to reflect the new paradigm, and ...
>
> ... Update the QAPI parser to prohibit "Note" sections while suggesting
> a new syntax. The exact formatting to use is a matter of taste, but a
> good candidate is simply:
>
> .. note:: lorem ipsum ...
>
> ... but there are other choices, too. The Sphinx readthedocs theme
> offers theming for the following forms (capitalization unimportant); all
> are adorned with a (!) symbol in the title bar for rendered HTML docs.
>
> See
> https://sphinx-rtd-theme.readthedocs.io/en/stable/demo/demo.html#admonitions
> for examples of each directive/admonition in use.
>
> These are rendered in orange:
>
> .. Attention:: ...
> .. Caution:: ...
> .. WARNING:: ...
>
> These are rendered in red:
>
> .. DANGER:: ...
> .. Error:: ...
>
> These are rendered in green:
>
> .. Hint:: ...
> .. Important:: ...
> .. Tip:: ...
>
> These are rendered in blue:
>
> .. Note:: ...
> .. admonition:: custom title
>
>admonition body text
>
> This patch uses ".. note::" almost everywhere, with just two "caution"
> directives. ".. admonition:: notes" is used in a few places where we had
> an ordered list of multiple notes that would not make sense as
> standalone/separate admonitions.
>
> Signed-off-by: John Snow
> Acked-by: Stefan Hajnoczi [for block*.json]
[...]
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 8bd299265e3..5bfa0ded42c 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -195,12 +195,12 @@
> #
> # @typename: the type name of an object
> #
> -# Note: objects can create properties at runtime, for example to
> -# describe links between different devices and/or objects. These
> -# properties are not included in the output of this command.
> -#
> # Returns: a list of ObjectPropertyInfo describing object properties
> #
> +# .. note:: Objects can create properties at runtime, for example to
> +#describe links between different devices and/or objects. These
> +#properties are not included in the output of this command.
> +#
> # Since: 2.12
> ##
You move the note. Commit message doesn't tell why.
> { 'command': 'qom-list-properties',
[...]
Re: [PATCH 09/13] qapi: convert "Note" sections to plain rST
John Snow writes: > On Wed, Jun 19, 2024, 8:49 AM Markus Armbruster wrote: > >> John Snow writes: [...] >> > diff --git a/tests/qapi-schema/doc-interleaved-section.json >> > b/tests/qapi-schema/doc-interleaved-section.json >> > index adb29e98daa..b26bc0bbb79 100644 >> > --- a/tests/qapi-schema/doc-interleaved-section.json >> > +++ b/tests/qapi-schema/doc-interleaved-section.json >> > @@ -10,7 +10,7 @@ >> > # >> > # bao >> > # >> > -# Note: a section. >> > +# Returns: a section. >> > # >> > # @foobar: catch this >> > # >> >> "Returns:" is only valid for commands, and this is a struct. Let's use >> "TODO:" instead. >> > > Weird that it didn't prohibit it. Bug? No: it simply chokes on "description of '@foobar:' follows a section" before it can choke on "'Returns' section is only valid for commands".
Re: [PATCH 09/13] qapi: convert "Note" sections to plain rST
John Snow writes: > Apologies, I meant to do this but forgot there were two cases and only > nabbed one. > > Fixing. No problem at all!
Re: [PATCH 04/13] qapi/parser: preserve indentation in QAPIDoc sections
John Snow writes: > On Thu, Jun 20, 2024, 11:07 AM Markus Armbruster wrote: > >> John Snow writes: >> >> > On Wed, Jun 19, 2024, 8:03 AM Markus Armbruster wrote: >> > >> >> John Snow writes: >> >> >> >> > Change get_doc_indented() to preserve indentation on all subsequent text >> >> > lines, and create a compatibility dedent() function for qapidoc.py to >> >> > remove that indentation. This is being done for the benefit of a new >> >> >> >> Suggest "remove indentation the same way get_doc_indented() did." >> >> >> > >> > Aight. >> > >> > >> >> > qapidoc generator which requires that indentation in argument and >> >> > features sections are preserved. >> >> > >> >> > Prior to this patch, a section like this: >> >> > >> >> > ``` >> >> > @name: lorem ipsum >> >> >dolor sit amet >> >> > consectetur adipiscing elit >> >> > ``` >> >> > >> >> > would have its body text be parsed as: >> >> >> >> Suggest "parsed into". >> >> >> > >> > Why? (I mean, I'll do it, but I don't see the semantic difference >> > personally) >> > >> >> "Parse as " vs. "Parse into ". >> >> >> > (first and final newline only for presentation) >> >> > >> >> > ``` >> >> > lorem ipsum >> >> > dolor sit amet >> >> > consectetur adipiscing elit >> >> > ``` >> >> > >> >> > We want to preserve the indentation for even the first body line so that >> >> > the entire block can be parsed directly as rST. This patch would now >> >> > parse that segment as: >> >> >> >> If you change "parsed as" to "parsed into" above, then do it here, too. >> >> >> >> > >> >> > ``` >> >> > lorem ipsum >> >> >dolor sit amet >> >> > consectetur adipiscing elit >> >> > ``` >> >> > >> >> > This is helpful for formatting arguments and features as field lists in >> >> > rST, where the new generator will format this information as: >> >> > >> >> > ``` >> >> > :arg type name: lorem ipsum >> >> >dolor sit amet >> >> > consectetur apidiscing elit >> >> > ``` >> >> > >> >> > ...and can be formed by the simple concatenation of the field list >> >> > construct and the body text. The indents help preserve the continuation >> >> > of a block-level element, and further allow the use of additional rST >> >> > block-level constructs such as code blocks, lists, and other such >> >> > markup. Avoiding reflowing the text conditionally also helps preserve >> >> > source line context for better rST error reporting from sphinx through >> >> > generated source, too. >> >> >> >> What do you mean by "reflowing"? >> >> >> > >> > Poorly phrased, was thinking about emacs too much. I mean munging the text >> > post-hoc for the doc generator such that newlines are added or removed in >> > the process of re-formatting text to get the proper indentation for the new >> > rST form. >> > >> > In prototyping, this got messy very quickly and was difficult to correlate >> > source line numbers across the transformation. >> > >> > It was easier to just not munge the text at all instead of munging it and >> > then un-munging it. >> > >> > (semantic satiation: munge munge munge munge.) >> >> Is this about a possible alternative solution you explored? Keeping >> .get_doc_indented() as is, and then try to undo its damage? >> > > precisamente. That solution was categorically worse. Since .get_doc_indented() removes N spaces of indentation, we'd want to add back N spaces of indentation. But we can't know N, so I guess we'd make do with an arbitrary number. Where would reflowing come it? I'd like you to express more clearly that you're talking about an alternative you rejected. Perhaps like this: block-level constructs such as code blocks, lists, and other such markup. The alternative would be to somehow undo .get_doc_indented()'s indentation changes in the new generator. Much messier. Feel free to add more detail to the last paragraph. >> [...]
Re: [PATCH 04/13] qapi/parser: preserve indentation in QAPIDoc sections
John Snow writes: > On Wed, Jun 19, 2024, 8:03 AM Markus Armbruster wrote: > >> John Snow writes: >> >> > Change get_doc_indented() to preserve indentation on all subsequent text >> > lines, and create a compatibility dedent() function for qapidoc.py to >> > remove that indentation. This is being done for the benefit of a new >> >> Suggest "remove indentation the same way get_doc_indented() did." >> > > Aight. > > >> > qapidoc generator which requires that indentation in argument and >> > features sections are preserved. >> > >> > Prior to this patch, a section like this: >> > >> > ``` >> > @name: lorem ipsum >> >dolor sit amet >> > consectetur adipiscing elit >> > ``` >> > >> > would have its body text be parsed as: >> >> Suggest "parsed into". >> > > Why? (I mean, I'll do it, but I don't see the semantic difference > personally) > "Parse as " vs. "Parse into ". >> > (first and final newline only for presentation) >> > >> > ``` >> > lorem ipsum >> > dolor sit amet >> > consectetur adipiscing elit >> > ``` >> > >> > We want to preserve the indentation for even the first body line so that >> > the entire block can be parsed directly as rST. This patch would now >> > parse that segment as: >> >> If you change "parsed as" to "parsed into" above, then do it here, too. >> >> > >> > ``` >> > lorem ipsum >> >dolor sit amet >> > consectetur adipiscing elit >> > ``` >> > >> > This is helpful for formatting arguments and features as field lists in >> > rST, where the new generator will format this information as: >> > >> > ``` >> > :arg type name: lorem ipsum >> >dolor sit amet >> > consectetur apidiscing elit >> > ``` >> > >> > ...and can be formed by the simple concatenation of the field list >> > construct and the body text. The indents help preserve the continuation >> > of a block-level element, and further allow the use of additional rST >> > block-level constructs such as code blocks, lists, and other such >> > markup. Avoiding reflowing the text conditionally also helps preserve >> > source line context for better rST error reporting from sphinx through >> > generated source, too. >> >> What do you mean by "reflowing"? >> > > Poorly phrased, was thinking about emacs too much. I mean munging the text > post-hoc for the doc generator such that newlines are added or removed in > the process of re-formatting text to get the proper indentation for the new > rST form. > > In prototyping, this got messy very quickly and was difficult to correlate > source line numbers across the transformation. > > It was easier to just not munge the text at all instead of munging it and > then un-munging it. > > (semantic satiation: munge munge munge munge.) Is this about a possible alternative solution you explored? Keeping .get_doc_indented() as is, and then try to undo its damage? [...]
Re: [PATCH 09/13] qapi: convert "Note" sections to plain rST
John Snow writes: > We do not need a dedicated section for notes. By eliminating a specially > parsed section, these notes can be treated as normal rST paragraphs in > the new QMP reference manual, and can be placed and styled much more > flexibly. > > Convert all existing "Note" and "Notes" sections to pure rST. As part of > the conversion, capitalize the first letter of each sentence and add > trailing punctuation where appropriate to ensure notes look sensible and > consistent in rendered HTML documentation. > > Update docs/devel/qapi-code-gen.rst to reflect the new paradigm, and ... > > ... Update the QAPI parser to prohibit "Note" sections while suggesting > a new syntax. The exact formatting to use is a matter of taste, but a > good candidate is simply: > > .. note:: lorem ipsum ... > > ... but there are other choices, too. The Sphinx readthedocs theme > offers theming for the following forms (capitalization unimportant); all > are adorned with a (!) symbol in the title bar for rendered HTML docs. > > See > https://sphinx-rtd-theme.readthedocs.io/en/stable/demo/demo.html#admonitions > for examples of each directive/admonition in use. > > These are rendered in orange: > > .. Attention:: ... > .. Caution:: ... > .. WARNING:: ... > > These are rendered in red: > > .. DANGER:: ... > .. Error:: ... > > These are rendered in green: > > .. Hint:: ... > .. Important:: ... > .. Tip:: ... > > These are rendered in blue: > > .. Note:: ... > .. admonition:: custom title > >admonition body text > > This patch uses ".. note::" almost everywhere, Not mentioned, and may or may not be worth mentioning: both "Note:" and "Notes:" become ".. note::", which renders as "Note". One instance quoted below. No objection to the change; you obviously double-checked it reads okay that way. >with just two "caution" > directives. ".. admonition:: notes" is used in a few places where we had > an ordered list of multiple notes that would not make sense as > standalone/separate admonitions. > > Signed-off-by: John Snow > Acked-by: Stefan Hajnoczi [for block*.json] [...] > diff --git a/qapi/block-core.json b/qapi/block-core.json > index df5e07debd2..cacedfb771c 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json [...] > @@ -6048,9 +6048,9 @@ > # > # @name: the name of the internal snapshot to be created > # > -# Notes: In transaction, if @name is empty, or any snapshot matching > -# @name exists, the operation will fail. Only some image formats > -# support it, for example, qcow2, and rbd. > +# .. note:: In transaction, if @name is empty, or any snapshot matching > +#@name exists, the operation will fail. Only some image formats > +#support it, for example, qcow2, and rbd. > # > # Since: 1.7 > ## [...]
Re: [PATCH 09/13] qapi: convert "Note" sections to plain rST
Markus Armbruster writes: > John Snow writes: [...] >> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json >> index b3de1fb6b3a..57598331c5c 100644 >> --- a/qga/qapi-schema.json >> +++ b/qga/qapi-schema.json [...] >> @@ -631,8 +632,8 @@ >> # Errors: >> # - If hybrid suspend is not supported, Unsupported >> # >> -# Notes: It's strongly recommended to issue the guest-sync command >> -# before sending commands when the guest resumes >> +# .. note:: It's strongly recommended to issue the guest-sync command >> +#before sending commands when the guest resumes. >> # >> # Since: 1.1 >> ## >> @@ -1461,16 +1462,15 @@ >> # * POSIX: as defined by os-release(5) >> # * Windows: contains string "server" or "client" >> # >> -# Notes: On POSIX systems the fields @id, @name, @pretty-name, >> -# @version, @version-id, @variant and @variant-id follow the >> -# definition specified in os-release(5). Refer to the manual page >> -# for exact description of the fields. Their values are taken >> -# from the os-release file. If the file is not present in the >> -# system, or the values are not present in the file, the fields >> -# are not included. >> +# .. note:: On POSIX systems the fields @id, @name, @pretty-name, >> +#@version, @version-id, @variant and @variant-id follow the >> +#definition specified in os-release(5). Refer to the manual page for >> +#exact description of the fields. Their values are taken from the >> +#os-release file. If the file is not present in the system, or the >> +#values are not present in the file, the fields are not included. >> # >> -# On Windows the values are filled from information gathered from >> -# the system. >> +#On Windows the values are filled from information gathered from >> +#the system. > > Please don't change the indentation here. I get the same output with > > @@ -1461,7 +1462,7 @@ ># * POSIX: as defined by os-release(5) ># * Windows: contains string "server" or "client" ># > -# Notes: On POSIX systems the fields @id, @name, @pretty-name, > +# .. note:: On POSIX systems the fields @id, @name, @pretty-name, ># @version, @version-id, @variant and @variant-id follow the ># definition specified in os-release(5). Refer to the manual page ># for exact description of the fields. Their values are taken I'm blind. Actually, you change indentation of subsequent lines from 4 to 3 *everywhere*. I guess you do that to make subsequent lines line up with the directive, here "note". Everywhere else, we indent such lines by 4. Hmm. How terrible would it be not to mess with the alignment? If we want to use 3 for directives, is it worth pointing out in the commit message? [...]
Re: [PATCH 13/13] qapi: convert "Example" sections to rST
John Snow writes:
> Eliminate the "Example" sections in QAPI doc blocks, converting them
> into QMP example code blocks. This is generally done in this patch by
> converting "Example:" or "Examples:" lines into ".. code-block:: QMP"
> lines.
>
> The old "Example:" or "Examples:" syntax is now caught as an error; but
> with the previous commit, "Example::" is still permitted as explicit rST
> syntax. ('Example' is not special in this case; any sentence that ends
> with "::" will start an indented code block in rST.)
>
> The ".. code-block:: QMP" form explicitly applies the QMP lexer and will
> loosely validate an example as valid QMP/JSON. The "::" form does not
> apply any lexer in particular and will not emit any errors.
>
> It is possible to choose the QMP lexer with the "::" form by using the
> Sphinx directive ".. highlight:: QMP" in the document above where the
> example appears; but this changes the lexer for *all* subsequent "::"
> style code-blocks in the document thereafter.
>
> This patch does not change the default lexer for the legacy qapidoc
> generator documents; future patches for the new qapidoc generator *may*
> change this default.
>
> This patch has several benefits:
>
> 1. Example sections can now be written more arbitrarily, mixing
>explanatory paragraphs and code blocks however desired.
>
> 2. "Example sections" can now use fully arbitrary rST.
Do the double-quotes signify something I'm missing?
>
> 3. All code blocks are now lexed and validated as QMP; increasing
>usability of the docs and ensuring validity of example snippets.
>
>(To some extent - This patch only gaurantees it lexes correctly, not
>that it's valid under the JSON or QMP grammars. It will catch most
>small mistakes, however.)
>
> 4. Each code-block can be captioned independently without bypassing the
>QMP lexer/validator.
>
>(i.e. code blocks are now for *code* only, so we don't have to
>sacrifice annotations/captions for having lexicographically correct
>examples.)
>
> For any sections with more than one example, examples are split up into
> multiple code-block regions. If annotations are present, those
> annotations are converted into code-block captions instead, e.g.
>
> ```
> Examples:
>
>1. Lorem Ipsum
>
>-> { "foo": "bar" }
> ```
>
> Is rewritten as:
>
> ```
> .. code-block:: QMP
>:caption: Example: Lorem Ipsum
>
>-> { "foo": "bar" }
> ```
>
> This process was only semi-automated:
>
> 1. Replace "Examples?:" sections with sed:
>
> sed -i 's|# Example:|# .. code-block:: QMP|' *.json
> sed -i 's|# Examples:|# .. code-block:: QMP|' *.json
>
> 2. Identify sections that no longer parse successfully by attempting the
>doc build, convert annotations into captions manually.
>(Tedious, oh well.)
>
> 3. Add captions where still needed:
>
> sed -zi 's|# .. code-block:: QMP\n#\n|# .. code-block:: QMP\n#:caption:
> Example\n#\n|g' *.json
>
> Not fully ideal, but hopefully not something that has to be done very
> often. (Or ever again.)
>
> Signed-off-by: John Snow
> Acked-by: Stefan Hajnoczi [for block*.json]
[...]
> diff --git a/qapi/pci.json b/qapi/pci.json
> index f51159a2c4c..9192212661b 100644
> --- a/qapi/pci.json
> +++ b/qapi/pci.json
> @@ -182,7 +182,8 @@
> #
> # Since: 0.14
> #
> -# Example:
> +# .. code-block:: QMP
> +#:caption: Example
> #
> # -> { "execute": "query-pci" }
> # <- { "return": [
> @@ -311,8 +312,7 @@
> # ]
> #}
> #
> -# Note: This example has been shortened as the real response is too
> -# long.
> +# This example has been shortened as the real response is too long.
Squash into PATCH 09.
> #
> ##
> { 'command': 'query-pci', 'returns': ['PciInfo'] }
Otherwise looks good to me except for the somewhat ugly rendering in
HTML mentioned in the commit message.
[...]
Re: [PATCH 09/13] qapi: convert "Note" sections to plain rST
John Snow writes:
> We do not need a dedicated section for notes. By eliminating a specially
> parsed section, these notes can be treated as normal rST paragraphs in
> the new QMP reference manual, and can be placed and styled much more
> flexibly.
>
> Convert all existing "Note" and "Notes" sections to pure rST. As part of
> the conversion, capitalize the first letter of each sentence and add
> trailing punctuation where appropriate to ensure notes look sensible and
> consistent in rendered HTML documentation.
>
> Update docs/devel/qapi-code-gen.rst to reflect the new paradigm, and ...
>
> ... Update the QAPI parser to prohibit "Note" sections while suggesting
> a new syntax. The exact formatting to use is a matter of taste, but a
> good candidate is simply:
>
> .. note:: lorem ipsum ...
>
> ... but there are other choices, too. The Sphinx readthedocs theme
> offers theming for the following forms (capitalization unimportant); all
> are adorned with a (!) symbol in the title bar for rendered HTML docs.
>
> See
> https://sphinx-rtd-theme.readthedocs.io/en/stable/demo/demo.html#admonitions
> for examples of each directive/admonition in use.
>
> These are rendered in orange:
>
> .. Attention:: ...
> .. Caution:: ...
> .. WARNING:: ...
>
> These are rendered in red:
>
> .. DANGER:: ...
> .. Error:: ...
>
> These are rendered in green:
>
> .. Hint:: ...
> .. Important:: ...
> .. Tip:: ...
>
> These are rendered in blue:
>
> .. Note:: ...
> .. admonition:: custom title
>
>admonition body text
>
> This patch uses ".. note::" almost everywhere, with just two "caution"
> directives. ".. admonition:: notes" is used in a few places where we had
> an ordered list of multiple notes that would not make sense as
> standalone/separate admonitions.
>
> Signed-off-by: John Snow
> Acked-by: Stefan Hajnoczi [for block*.json]
[...]
> diff --git a/qapi/control.json b/qapi/control.json
> index 10c906fa0e7..59d5e00c151 100644
> --- a/qapi/control.json
> +++ b/qapi/control.json
> @@ -22,14 +22,13 @@
> # "arguments": { "enable": [ "oob" ] } }
> # <- { "return": {} }
> #
> -# Notes: This command is valid exactly when first connecting: it must
> -# be issued before any other command will be accepted, and will
> -# fail once the monitor is accepting other commands. (see qemu
> -# docs/interop/qmp-spec.rst)
> +# .. note:: This command is valid exactly when first connecting: it must
> +#be issued before any other command will be accepted, and will fail
> +#once the monitor is accepting other commands. (see qemu
> +#docs/interop/qmp-spec.rst)
> #
> -# The QMP client needs to explicitly enable QMP capabilities,
> -# otherwise all the QMP capabilities will be turned off by
> -# default.
> +# .. note:: The QMP client needs to explicitly enable QMP capabilities,
> +#otherwise all the QMP capabilities will be turned off by default.
> #
> # Since: 0.13
> ##
> @@ -150,8 +149,8 @@
> # ]
> #}
> #
> -# Note: This example has been shortened as the real response is too
> -# long.
> +# This example has been shortened as the real response is too long.
> +#
Here's one way to transform the elision note, ...
> ##
> { 'command': 'query-commands', 'returns': ['CommandInfo'],
>'allow-preconfig': true }
[...]
> diff --git a/qapi/pci.json b/qapi/pci.json
> index 08bf6958634..f51159a2c4c 100644
> --- a/qapi/pci.json
> +++ b/qapi/pci.json
> @@ -146,8 +146,8 @@
> #
> # @regions: a list of the PCI I/O regions associated with the device
> #
> -# Notes: the contents of @class_info.desc are not stable and should
> -# only be treated as informational.
> +# .. note:: The contents of @class_info.desc are not stable and should
> +#only be treated as informational.
> #
> # Since: 0.14
> ##
> @@ -311,7 +311,8 @@
> # ]
> #}
> #
> -# Note: This example has been shortened as the real response is too
> +# Note: This example has been shortened as the real response is too
> # long.
> +#
... and here's another way. Same way would be better, wouldn't it?
They actually are after PATCH 13:
-# Note: This example has been shortened as the real response is too
-# long.
+# This example has been shortened as the real response is too long.
Move that hunk here, please.
> ##
> { 'command': 'query-pci', 'returns': ['PciInfo'] }
[...]
Re: [PATCH 12/13] qapi/parser: don't parse rST markup as section headers
John Snow writes: > The double-colon synax is rST formatting that precedes a literal code > block. We do not want to capture these as QAPI-specific sections. > > Coerce blocks that start with e.g. "Example::" to be parsed as untagged > paragraphs instead of special tagged sections. > > Signed-off-by: John Snow > --- > scripts/qapi/parser.py | 9 +++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py > index 53b06a94508..971fdf61a09 100644 > --- a/scripts/qapi/parser.py > +++ b/scripts/qapi/parser.py > @@ -545,10 +545,15 @@ def get_doc(self) -> 'QAPIDoc': > line = self.get_doc_indented(doc) > no_more_args = True > elif match := re.match( > -r'(Returns|Errors|Since|Notes?|Examples?|TODO): *', > -line): > +r'(Returns|Errors|Since|Notes?|Examples?|TODO)' > +r'(?!::): *', > +line, > +): > # tagged section > > +# Note: "sections" with two colons are left alone as > +# rST markup and not interpreted as a section heading. > + > # TODO: Remove this error sometime in 2025 or so > # after we've fully transitioned to the new qapidoc > # generator. Patch looks good, but let's add a positive test to doc-good.json.
Re: [PATCH 09/13] qapi: convert "Note" sections to plain rST
John Snow writes:
> We do not need a dedicated section for notes. By eliminating a specially
> parsed section, these notes can be treated as normal rST paragraphs in
> the new QMP reference manual, and can be placed and styled much more
> flexibly.
>
> Convert all existing "Note" and "Notes" sections to pure rST. As part of
> the conversion, capitalize the first letter of each sentence and add
> trailing punctuation where appropriate to ensure notes look sensible and
> consistent in rendered HTML documentation.
>
> Update docs/devel/qapi-code-gen.rst to reflect the new paradigm, and ...
>
> ... Update the QAPI parser to prohibit "Note" sections while suggesting
Scratch "... ..." and downcase "Update"?
> a new syntax. The exact formatting to use is a matter of taste, but a
> good candidate is simply:
>
> .. note:: lorem ipsum ...
>
> ... but there are other choices, too. The Sphinx readthedocs theme
> offers theming for the following forms (capitalization unimportant); all
> are adorned with a (!) symbol in the title bar for rendered HTML docs.
>
> See
> https://sphinx-rtd-theme.readthedocs.io/en/stable/demo/demo.html#admonitions
> for examples of each directive/admonition in use.
>
> These are rendered in orange:
>
> .. Attention:: ...
> .. Caution:: ...
> .. WARNING:: ...
>
> These are rendered in red:
>
> .. DANGER:: ...
> .. Error:: ...
>
> These are rendered in green:
>
> .. Hint:: ...
> .. Important:: ...
> .. Tip:: ...
>
> These are rendered in blue:
>
> .. Note:: ...
> .. admonition:: custom title
>
>admonition body text
>
> This patch uses ".. note::" almost everywhere, with just two "caution"
> directives. ".. admonition:: notes" is used in a few places where we had
> an ordered list of multiple notes that would not make sense as
> standalone/separate admonitions.
>
> Signed-off-by: John Snow
> Acked-by: Stefan Hajnoczi [for block*.json]
[...]
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index b3de1fb6b3a..57598331c5c 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -422,8 +422,9 @@
> # Returns: GuestFsfreezeStatus ("thawed", "frozen", etc., as defined
> # below)
> #
> -# Note: This may fail to properly report the current state as a result
> -# of some other guest processes having issued an fs freeze/thaw.
> +# .. note:: This may fail to properly report the current state as a
> +#result of some other guest processes having issued an fs
> +#freeze/thaw.
> #
> # Since: 0.15.0
> ##
> @@ -443,9 +444,9 @@
> #
> # Returns: Number of file systems currently frozen.
> #
> -# Note: On Windows, the command is implemented with the help of a
> -# Volume Shadow-copy Service DLL helper. The frozen state is
> -# limited for up to 10 seconds by VSS.
> +# .. note:: On Windows, the command is implemented with the help of a
> +#Volume Shadow-copy Service DLL helper. The frozen state is limited
> +#for up to 10 seconds by VSS.
> #
> # Since: 0.15.0
> ##
> @@ -479,10 +480,10 @@
> #
> # Returns: Number of file systems thawed by this call
> #
> -# Note: if return value does not match the previous call to
> -# guest-fsfreeze-freeze, this likely means some freezable
> -# filesystems were unfrozen before this call, and that the
> -# filesystem state may have changed before issuing this command.
> +# .. note:: If return value does not match the previous call to
> +#guest-fsfreeze-freeze, this likely means some freezable filesystems
> +#were unfrozen before this call, and that the filesystem state may
> +#have changed before issuing this command.
> #
> # Since: 0.15.0
> ##
> @@ -560,8 +561,8 @@
> # Errors:
> # - If suspend to disk is not supported, Unsupported
> #
> -# Notes: It's strongly recommended to issue the guest-sync command
> -# before sending commands when the guest resumes
> +# .. note:: It's strongly recommended to issue the guest-sync command
> +#before sending commands when the guest resumes.
> #
> # Since: 1.1
> ##
> @@ -596,8 +597,8 @@
> # Errors:
> # - If suspend to ram is not supported, Unsupported
> #
> -# Notes: It's strongly recommended to issue the guest-sync command
> -# before sending commands when the guest resumes
> +# .. note:: It's strongly recommended to issue the guest-sync command
> +#before sending commands when the guest resumes.
> #
> # Since: 1.1
> ##
> @@ -631,8 +632,8 @@
> # Errors:
> # - If hybrid suspend is not supported, Unsupported
> #
> -# Notes: It's strongly recommended to issue the guest-sync command
> -# before sending commands when the guest resumes
> +# .. note:: It's strongly recommended to issue the guest-sync command
> +#before sending commands when the guest resumes.
> #
> # Since: 1.1
> ##
> @@ -1461,16 +1462,15 @@
> # * POSIX: as defined by os-release(5)
> # * Windows: contains string "server" or "client"
> #
> -# Notes: On POSIX systems the fields @id, @name, @pretty-name,
> -#
Re: [PATCH 11/13] qapi: add markup to note blocks
John Snow writes: > Generally, surround command-line options with ``literal`` markup to help > it stand out from prose in rendered HTML, and add cross-references to > replace "see also" messages. > > References to types, values, and other QAPI definitions are not yet > adjusted here; they will be converted en masse in a subsequent patch > after the new QAPI doc generator is merged. > > Signed-off-by: John Snow Reviewed-by: Markus Armbruster
Re: [PATCH 10/13] qapi: update prose in note blocks
John Snow writes: > Where I've noticed, rephrase the note to read more fluently. > > Signed-off-by: John Snow > --- > qapi/block-core.json | 4 ++-- > qga/qapi-schema.json | 2 +- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index cacedfb771c..9ef23ec02ae 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -6048,9 +6048,9 @@ > # > # @name: the name of the internal snapshot to be created > # > -# .. note:: In transaction, if @name is empty, or any snapshot matching > +# .. note:: In a transaction, if @name is empty or any snapshot matching > #@name exists, the operation will fail. Only some image formats > -#support it, for example, qcow2, and rbd. > +#support it; for example, qcow2, and rbd. > # > # Since: 1.7 > ## > diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json > index 57598331c5c..1273d85bb5f 100644 > --- a/qga/qapi-schema.json > +++ b/qga/qapi-schema.json > @@ -480,7 +480,7 @@ > # > # Returns: Number of file systems thawed by this call > # > -# .. note:: If return value does not match the previous call to > +# .. note:: If the return value does not match the previous call to > #guest-fsfreeze-freeze, this likely means some freezable filesystems > #were unfrozen before this call, and that the filesystem state may > #have changed before issuing this command. Reviewed-by: Markus Armbruster
Re: [PATCH 08/13] qapi: ensure all errors sections are uniformly typset
John Snow writes: > Transactions have the only instance of an Errors section that isn't a > rST list; turn it into one. > > Signed-off-by: John Snow Let;s explain the "why" a bit more clearly. Maybe qapi: Nail down convention that Errors sections are lists By unstated convention, Errors sections are rST lists. Document the convention, and make the one exception conform. > --- > docs/devel/qapi-code-gen.rst | 7 +++ > qapi/transaction.json| 2 +- > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst > index f453bd35465..cee43222f19 100644 > --- a/docs/devel/qapi-code-gen.rst > +++ b/docs/devel/qapi-code-gen.rst > @@ -1011,6 +1011,13 @@ like this:: > "Returns" and "Errors" sections are only valid for commands. They > document the success and the error response, respectively. > > +"Errors" sections should be formatted as an rST list, each entry > +detailing a relevant error condition. For example:: > + > + # Errors: > + # - If @device does not exist, DeviceNotFound > + # - Any other error returns a GenericError. > + > A "Since: x.y.z" tagged section lists the release that introduced the > definition. > > diff --git a/qapi/transaction.json b/qapi/transaction.json > index 5749c133d4a..07afc269d54 100644 > --- a/qapi/transaction.json > +++ b/qapi/transaction.json > @@ -235,7 +235,7 @@ > # additional detail. > # > # Errors: > -# Any errors from commands in the transaction > +# - Any errors from commands in the transaction > # > # Note: The transaction aborts on the first failure. Therefore, there > # will be information on only one failed operation returned in an Preferably with an improved commit message Reviewed-by: Markus Armbruster
Re: [PATCH 07/13] qapi: fix non-compliant JSON examples
John Snow writes: > The new QMP documentation generator wants to parse all examples as > "QMP". We have an existing QMP lexer in docs/sphinx/qmp_lexer.py (Seen > in-use here: https://qemu-project.gitlab.io/qemu/interop/bitmaps.html) > that allows the use of "->", "<-" and "..." tokens to denote QMP > protocol flow with elisions, but otherwise defers to the JSON lexer. > > To utilize this lexer for the existing QAPI documentation, we need them > to conform to a standard so that they lex and render correctly. Once the > QMP lexer is active for examples, errant QMP/JSON will produce warning > messages and fail the build. > > Fix any invalid JSON found in QAPI documentation (identified by > attempting to lex all examples as QMP; see subsequent commits). Further, > the QMP lexer still supports elisions, but they must be represented as > the value "...", so three examples have been adjusted to support that > format here. > > Signed-off-by: John Snow Reviewed-by: Markus Armbruster
Re: [PATCH 06/13] docs/qapidoc: fix nested parsing under untagged sections
John Snow writes:
> Sphinx does not like sections without titles, because it wants to
> convert every section into a reference. When there is no title, it
> struggles to do this and transforms the tree inproperly.
>
> Depending on the rST used, this may result in an assertion error deep in
> the docutils HTMLWriter.
>
> (Observed when using ".. admonition:: Notes" under such a section - When
> this is transformed with its own element, Sphinx is fooled into
> believing this title belongs to the section and incorrect mutates the
> docutils tree, leading to errors during rendering time.)
>
> When parsing an untagged section (free paragraphs), skip making a hollow
> section and instead append the parse results to the prior section.
>
> Many Bothans died to bring us this information.
>
> Signed-off-by: John Snow
> ---
> docs/sphinx/qapidoc.py | 16 +++-
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index f2f2005dd5f..66cf254a624 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -286,14 +286,20 @@ def _nodes_for_sections(self, doc):
> if section.tag and section.tag == 'TODO':
> # Hide TODO: sections
> continue
> +
> +if not section.tag:
> +# Sphinx cannot handle sectionless titles;
> +# Instead, just append the results to the prior section.
> +container = nodes.container()
> +self._parse_text_into_node(section.text, container)
> +nodelist += container.children
> +continue
> +
> snode = self._make_section(section.tag)
> -if section.tag and section.tag.startswith('Example'):
> +if section.tag.startswith('Example'):
> snode += self._nodes_for_example(dedent(section.text))
> else:
> -self._parse_text_into_node(
> -dedent(section.text) if section.tag else section.text,
> -snode,
> - )
> +self._parse_text_into_node(dedent(section.text), snode)
> nodelist.append(snode)
> return nodelist
Acked-by: Markus Armbruster
Re: [PATCH 05/13] qapi/parser: fix comment parsing immediately following a doc block
John Snow writes:
> If a comment immediately follows a doc block, the parser doesn't ignore
> that token appropriately. Fix that.
>
> e.g.
>
>> ##
>> # = Hello World!
>> ##
>>
>> # I'm a comment!
>
> will break the parser, because it does not properly ignore the comment
> token if it immediately follows a doc block.
>
> Fixes: 3d035cd2cca6 (qapi: Rewrite doc comment parser)
> Signed-off-by: John Snow
> ---
> scripts/qapi/parser.py | 2 +-
> tests/qapi-schema/doc-good.json | 2 ++
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 43167ef0ab3..dfd6a6c5bee 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -584,7 +584,7 @@ def get_doc(self) -> 'QAPIDoc':
> line = self.get_doc_line()
> first = False
>
> -self.accept(False)
> +self.accept()
> doc.end()
> return doc
>
> diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json
> index de38a386e8f..8b39eb946af 100644
> --- a/tests/qapi-schema/doc-good.json
> +++ b/tests/qapi-schema/doc-good.json
> @@ -55,6 +55,8 @@
> # - {braces}
> ##
>
> +# Not a doc comment
> +
> ##
> # @Enum:
> #
Reviewed-by: Markus Armbruster
Re: [PATCH 04/13] qapi/parser: preserve indentation in QAPIDoc sections
term = self._nodes_for_one_member(section.member)
> # TODO drop fallbacks when undocumented members are outlawed
> if section.text:
> -defn = section.text
> +defn = dedent(section.text)
> else:
> defn = [nodes.Text('Not documented')]
>
> @@ -214,7 +230,7 @@ def _nodes_for_enum_values(self, doc):
>
> termtext.extend(self._nodes_for_ifcond(section.member.ifcond))
> # TODO drop fallbacks when undocumented members are outlawed
> if section.text:
> -defn = section.text
> +defn = dedent(section.text)
> else:
> defn = [nodes.Text('Not documented')]
>
> @@ -249,7 +265,7 @@ def _nodes_for_features(self, doc):
> dlnode = nodes.definition_list()
> for section in doc.features.values():
> dlnode += self._make_dlitem(
> -[nodes.literal('', section.member.name)], section.text)
> +[nodes.literal('', section.member.name)],
> dedent(section.text))
> seen_item = True
>
> if not seen_item:
> @@ -272,9 +288,12 @@ def _nodes_for_sections(self, doc):
> continue
> snode = self._make_section(section.tag)
> if section.tag and section.tag.startswith('Example'):
> -snode += self._nodes_for_example(section.text)
> +snode += self._nodes_for_example(dedent(section.text))
> else:
> -self._parse_text_into_node(section.text, snode)
> +self._parse_text_into_node(
> +dedent(section.text) if section.tag else section.text,
> +snode,
> +)
> nodelist.append(snode)
> return nodelist
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 7b13a583ac1..43167ef0ab3 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -437,6 +437,7 @@ def _match_at_name_colon(string: str) ->
> Optional[Match[str]]:
> return re.match(r'@([^:]*): *', string)
>
> def get_doc_indented(self, doc: 'QAPIDoc') -> Optional[str]:
> +"""get_doc_indented preserves indentation for later rST parsing."""
A proper function comment explains what the function does. This one
merely points out one minor aspect. Easy fix: delete it. Alternative
fix: write a proper function comment.
> self.accept(False)
> line = self.get_doc_line()
> while line == '':
[...]
Just commit message and doc nitpicks, so
Reviewed-by: Markus Armbruster
Re: [PATCH 03/13] docs/qapidoc: delint a tiny portion of the module
equired}
> has_content = False
>
> def new_serialno(self):
> """Return a unique new ID string suitable for use as a node's ID"""
> env = self.state.document.settings.env
> -return 'qapidoc-%d' % env.new_serialno('qapidoc')
> +return "qapidoc-%d" % env.new_serialno("qapidoc")
>
> def run(self):
> env = self.state.document.settings.env
> -qapifile = env.config.qapidoc_srctree + '/' + self.arguments[0]
> +qapifile = env.config.qapidoc_srctree + "/" + self.arguments[0]
> qapidir = os.path.dirname(qapifile)
>
> try:
> @@ -523,13 +536,14 @@ def do_parse(self, rstlist, node):
> # plain self.state.nested_parse(), and so we can drop the saving
> # of title_styles and section_level that kerneldoc.py does,
> # because nested_parse_with_titles() does that for us.
> -if Use_SSI:
> +if USE_SSI:
> with switch_source_input(self.state, rstlist):
> nested_parse_with_titles(self.state, rstlist, node)
> else:
> save = self.state.memo.reporter
> self.state.memo.reporter = AutodocReporter(
> -rstlist, self.state.memo.reporter)
> +rstlist, self.state.memo.reporter
> +)
> try:
> nested_parse_with_titles(self.state, rstlist, node)
> finally:
> @@ -537,12 +551,12 @@ def do_parse(self, rstlist, node):
>
>
> def setup(app):
> -""" Register qapi-doc directive with Sphinx"""
> -app.add_config_value('qapidoc_srctree', None, 'env')
> -app.add_directive('qapi-doc', QAPIDocDirective)
> +"""Register qapi-doc directive with Sphinx"""
> +app.add_config_value("qapidoc_srctree", None, "env")
> +app.add_directive("qapi-doc", QAPIDocDirective)
>
> -return dict(
> -version=__version__,
> -parallel_read_safe=True,
> -parallel_write_safe=True
> -)
> +return {
> +"version": __version__,
> +"parallel_read_safe": True,
> +"parallel_write_safe": True,
> +}
With intersperse() deleted
Reviewed-by: Markus Armbruster
Re: [PATCH 09/20] qapi/parser: add undocumented stub members to all_sections
John Snow writes:
> On Fri, Jun 14, 2024 at 4:53 AM Markus Armbruster wrote:
>
>> John Snow writes:
>>
>> > This helps simplify the doc generator if it doesn't have to check for
>> > undocumented members.
>> >
>> > Signed-off-by: John Snow
>> > ---
>> > scripts/qapi/parser.py | 20 ++--
>> > 1 file changed, 18 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>> > index b1794f71e12..3cd8e7ee295 100644
>> > --- a/scripts/qapi/parser.py
>> > +++ b/scripts/qapi/parser.py
>> > @@ -740,8 +740,24 @@ def connect_member(self, member: 'QAPISchemaMember')
>> > -> None:
>> > raise QAPISemError(member.info,
>> > "%s '%s' lacks documentation"
>> > % (member.role, member.name))
>> > -self.args[member.name] = QAPIDoc.ArgSection(
>> > -self.info, '@' + member.name, 'member')
>> > +
>> > +# Insert stub documentation section for missing member docs.
>> > +section = QAPIDoc.ArgSection(
>> > +self.info, f"@{member.name}", "member")
>>
>> Although I like f-strings in general, I'd pefer to stick to '@' +
>> member.name here, because it's simpler.
>
> Tomayto, Tomahto. (OK.)
Apropos healthy vegetables: at some time, we might want to mass-convert
to f-strings where they are easier to read.
>> Also, let's not change 'member' to "member". Existing practice: single
>> quotes for string literals unless double quotes avoid escapes. Except
>> English prose (like error messages) is always in double quotes.
>>
>
> OK. I realize I'm not consistent in this patch either, but I'll explain
> that my using double quotes here is a black-ism that is sneaking in the
> more I use it to auto-format my patches :)
>
> Maybe time for a flag day when I move scripts/qapi to python/qemu/qapi ...
>
> (Sorry, this type of stuff is ... invisible to me, and I really do rely on
> the linters to make sure I don't do this kind of thing.)
>
>
>>
>> > +self.args[member.name] = section
>> > +
>> > +# Determine where to insert stub doc.
>>
>> If we have some member documentation, the member doc stubs clearly must
>> go there. Inserting them at the end makes sense.
>>
>> Else we want to put them where the parser would accept real member
>> documentation.
>>
>> "The parser" is .get_doc(). This is what it accepts (I'm prepared to
>> explain this in detail if necessary):
>>
>> One untagged section
>>
>> Member documentation, if any
>>
>> Zero ore more tagged or untagged sections
>>
>> Feature documentation, if any
>>
>> Zero or more tagged or untagged sections
>>
>> If we there is no member documentation, this is
>>
>> One untagged section
>>
>> Zero ore more tagged or untagged sections
>>
>> Feature documentation, if any
>>
>> Zero or more tagged or untagged sections
>>
>> Note that we cannot have two adjacent untagged sections (we only create
>> one if the current section isn't untagged; if it is, we extend it
>> instead). Thus, the second section must be tagged or feature
>> documentation.
>>
>> Therefore, the member doc stubs must go right after the first section.
>>
>> This is also where qapidoc.py inserts member documentation.
>>
>> > +index = 0
>> > +for i, sect in enumerate(self.all_sections):
>> > +# insert after these:
>> > +if sect.kind in ('intro-paragraph', 'member'):
>> > +index = i + 1
>> > +# but before these:
>> > +elif sect.kind in ('tagged', 'feature',
>> > 'outro-paragraph'):
>> > +index = i
>> > +break
>>
>> Can you describe what this does in English? As a specification; simply
>> paraphrasing the code is cheating. I tried, and gave up.
>>
>
> It inserts after any intro-paragraph or member section it finds, but before
> any tagged, feature, or outro-paragraph it finds.
>
> The loop breaks on the very first instance of tagged/feature/outro, exiting
> immediately and leaving the insertion index set to the first occurrence of
> such a section, so that the in
Re: [PATCH 20/20] qapi: convert "Example" sections to rST
John Snow writes:
> On Fri, Jun 14, 2024 at 10:39 AM Markus Armbruster
> wrote:
>
>> John Snow writes:
>>
>> > Eliminate the "Example" sections in QAPI doc blocks, converting them
>> > into QMP example code blocks. This is generally done by converting
>> > "Example:" or "Examples:" lines into ".. code-block:: QMP" lines.
>> >
>> > This patch does also allow for the use of the rST syntax "Example::" by
>> > exempting double-colon syntax from the QAPI doc parser, but that form is
>> > not used by this conversion patch. The phrase "Example" here is not
>> > special, it is the double-colon syntax that transforms the following
>> > block into a code-block. By default, *this* form does not apply QMP
>> > highlighting.
>> >
>> > This patch has several benefits:
>> >
>> > 1. Example sections can now be written more arbitrarily, mixing
>> >explanatory paragraphs and code blocks however desired.
>> >
>> > 2. Example sections can now use fully arbitrary rST.
>> >
>> > 3. All code blocks are now lexed and validated as QMP; increasing
>> >usability of the docs and ensuring validity of example snippets.
>> >
>> > 4. Each code-block can be captioned independently without bypassing the
>> >QMP lexer/validator.
>> >
>> > For any sections with more than one example, examples are split up into
>> > multiple code-block regions. If annotations are present, those
>> > annotations are converted into code-block captions instead, e.g.
>> >
>> > ```
>> > Examples:
>> >
>> >1. Lorem Ipsum
>> >
>> >-> { "foo": "bar" }
>> > ```
>> >
>> > Is rewritten as:
>> >
>> > ```
>> > .. code-block:: QMP
>> >:caption: Example: Lorem Ipsum
>> >
>> >-> { "foo": "bar" }
>> > ```
>> >
>> > This process was only semi-automated:
>> >
>> > 1. Replace "Examples?:" sections with sed:
>> >
>> > sed -i 's|# Example:|# .. code-block:: QMP|' *.json
>> > sed -i 's|# Examples:|# .. code-block:: QMP|' *.json
>> >
>> > 2. Identify sections that no longer parse successfully by attempting the
>> >doc build, convert annotations into captions manually.
>> >(Tedious, oh well.)
>> >
>> > 3. Add captions where still needed:
>> >
>> > sed -zi 's|# .. code-block:: QMP\n#\n|# .. code-block:: QMP\n# :caption:
>> > Example\n#\n|g' *.json
>> >
>> > Not fully ideal, but hopefully not something that has to be done very
>> > often. (Or ever again.)
>> >
>> > Signed-off-by: John Snow
>> > ---
>> > qapi/acpi.json | 6 +-
>> > qapi/block-core.json| 120 --
>> > qapi/block.json | 60 +++--
>> > qapi/char.json | 36 ++--
>> > qapi/control.json | 16 ++--
>> > qapi/dump.json | 12 ++-
>> > qapi/machine-target.json| 3 +-
>> > qapi/machine.json | 79 ++---
>> > qapi/migration.json | 145 +++-
>> > qapi/misc-target.json | 33 +---
>> > qapi/misc.json | 48 +++
>> > qapi/net.json | 30 +--
>> > qapi/pci.json | 6 +-
>> > qapi/qapi-schema.json | 6 +-
>> > qapi/qdev.json | 15 +++-
>> > qapi/qom.json | 20 +++--
>> > qapi/replay.json| 12 ++-
>> > qapi/rocker.json| 12 ++-
>> > qapi/run-state.json | 45 ++
>> > qapi/tpm.json | 9 +-
>> > qapi/trace.json | 6 +-
>> > qapi/transaction.json | 3 +-
>> > qapi/ui.json| 62 +-
>> > qapi/virtio.json| 38 +
>> > qapi/yank.json | 6 +-
>> > scripts/qapi/parser.py | 15 +++-
>> > tests/qapi-schema/doc-good.json | 12 +--
>> > tests/qapi-schema/doc-good.out | 17 ++--
>> > tests/qapi-schema/doc-good.txt | 17 +---
>> > 29 files changed, 574 insertions(+), 315 deletions(-)
Re: [PATCH 19/20] qapi: convert "Note" sections to plain rST
John Snow writes: > On Fri, Jun 14, 2024, 9:44 AM Markus Armbruster wrote: > >> John Snow writes: >> >> > We do not need a dedicated section for notes. By eliminating a specially >> > parsed section, these notes can be treated as normal rST paragraphs in >> > the new QMP reference manual, and can be placed and styled much more >> > flexibly. >> > >> > Update the QAPI parser to now prohibit special "Note" sections while >> > suggesting a new syntax. >> > >> > The exact formatting to use is a matter of taste, but a good candidate >> > is simply: >> > >> > .. note:: lorem ipsum ... >> > >> > but there are other choices, too. The Sphinx readthedocs theme offers >> > theming for the following forms (capitalization unimportant); all are >> > adorned with a (!) symbol in the title bar for rendered HTML docs. >> > > Store this paragraph in your L1 cache for a moment ... > >> >> > These are rendered in orange: >> > >> > .. Attention:: ... >> > .. Caution:: ... >> > .. WARNING:: ... >> > >> > These are rendered in red: >> > >> > .. DANGER:: ... >> > .. Error:: ... >> > >> > These are rendered in green: >> > >> > .. Hint:: ... >> > .. Important:: ... >> > .. Tip:: ... >> > >> > These are rendered in blue: >> > >> > .. Note:: ... >> > .. admonition:: custom title >> > >> >admonition body text >> > >> > This patch uses ".. notes::" almost everywhere, with just two "caution" >> > directives. ".. admonition:: notes" is used in a few places where we had >> > an ordered list of multiple notes. >> > >> > Signed-off-by: John Snow >> > --- >> > qapi/block-core.json | 30 +++ >> > qapi/block.json | 2 +- >> > qapi/char.json| 12 +-- >> > qapi/control.json | 15 ++-- >> > qapi/dump.json| 2 +- >> > qapi/introspect.json | 6 +- >> > qapi/machine-target.json | 26 +++--- >> > qapi/machine.json | 47 +- >> > qapi/migration.json | 12 +-- >> > qapi/misc.json| 88 +-- >> > qapi/net.json | 6 +- >> > qapi/pci.json | 7 +- >> > qapi/qdev.json| 30 +++ >> > qapi/qom.json | 19 ++-- >> > qapi/rocker.json | 16 ++-- >> > qapi/run-state.json | 18 ++-- >> > qapi/sockets.json | 10 +-- >> > qapi/stats.json | 22 ++--- >> > qapi/transaction.json | 8 +- >> > qapi/ui.json | 29 +++--- >> > qapi/virtio.json | 12 +-- >> > qga/qapi-schema.json | 48 +- >> > scripts/qapi/parser.py| 9 ++ >> > tests/qapi-schema/doc-empty-section.err | 2 +- >> > tests/qapi-schema/doc-empty-section.json | 2 +- >> > tests/qapi-schema/doc-good.json | 6 +- >> > tests/qapi-schema/doc-good.out| 10 ++- >> > tests/qapi-schema/doc-good.txt| 14 ++- >> > .../qapi-schema/doc-interleaved-section.json | 2 +- >> > 29 files changed, 258 insertions(+), 252 deletions(-) >> >> Missing: update of docs/devel/qapi-code-gen.rst. Something like >> >> diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst >> index f453bd3546..1a4e240adb 100644 >> --- a/docs/devel/qapi-code-gen.rst >> +++ b/docs/devel/qapi-code-gen.rst >> @@ -995,14 +995,13 @@ line "Features:", like this:: >># @feature: Description text >> >> A tagged section begins with a paragraph that starts with one of the >> -following words: "Note:"/"Notes:", "Since:", "Example:"/"Examples:", >> -"Returns:", "Errors:", "TODO:". It ends with the start of a new >> -section. >> +following words: "Since:", "
Re: [PATCH 18/20] qapi: ensure all errors sections are uniformly typset
John Snow writes: > On Fri, Jun 14, 2024, 7:24 AM Markus Armbruster wrote: > >> John Snow writes: >> >> > Transactions have the only instance of an Errors section that isn't a >> > rST list; turn it into one. >> >> Just for consistency? Or do you have other shenanigans up your sleeve? > > Just consistency at this precise moment in time, but it's *possible* I may > introduce shenanigans for visual consistency in the rendered output, for > which having a uniform format would make mechanical conversions in the > generator easier/possible. > > It's an idea I had but didn't implement yet. I figured I'd write this patch > anyway because it isn't wrong, and you yourself seemed to believe it would > *always* be a RST list, when that isn't strictly true. > > >> If we want the Errors sections to remain all rST lists, we should update >> docs/devel/qapi-code-gen.rst to say so. >> > > OK, will do. With such an update, we could perhaps sell the patch like qapi: Nail down convention that Errors sections are lists By unstated convention, Errors sections are rST lists. Document the convention, and make the one exception conform. > > >> > Signed-off-by: John Snow >> >>
Re: [PATCH 14/20] qapi: fix non-compliant JSON examples
John Snow writes: > On Fri, Jun 14, 2024, 6:55 AM Markus Armbruster wrote: > >> John Snow writes: >> >> > If we parse all examples as QMP, we need them to conform to a standard >> > so that they render correctly. Once the QMP lexer is active for >> > examples, these will produce warning messages and fail the build. >> > >> > The QMP lexer still supports elisions, but they must be represented as >> > the value "...", so two examples have been adjusted to support that >> > format here. >> >> I think this could use a bit more context. I believe you're referring >> to docs/sphinx/qmp_lexer.py. It describes itself as "a Sphinx extension >> that provides a QMP lexer for code blocks." >> > > That's our guy! I explain its use a bit more in ... some other patch, > somewhere... > > >> "If we parse all examples as QMP" and "Once the QMP lexer is active for >> examples" suggests we're *not* using it for (some?) examples. So what >> are we using it for? >> > > My incremental backup doc makes use of it; you have to "opt in" to using > the QMP lexer instead of the generic lexer. The ".. code-block:: QMP" lines I can see in a few files? Namely: docs/devel/s390-cpu-topology.rst docs/interop/bitmaps.rst docs/interop/qmp-spec.rst > The example conversion patch later in this series opts all of the qapi docs > into using it. > > ((Later, it's possible to make "Example::" choose the QMP lexer by default > on any of our generated QMP pages. (and opting out would require explicit > code-block syntax with the lexer of choice named.))) The patch does two related things: 1. Fix invalid JSON. Doesn't need justification. 2. Normalize elisions to ... You pick ... because that's what qmp_lexer.py wants. Doing both in one patch is fine. Perhaps qapi: Fix invalid JSON in examples, and normalize elisions A few examples elide part of the output. Normalize elision to exactly ... Together with the JSON fixing, this enables use of docs/sphinx/qmp_lexer.py to highlight the examples in a later patch. >> > Signed-off-by: John Snow >> >> Patch looks lovely. >> >> Hat tip to Victor Toso, who fixed up most examples two years ago. Back >> then we couldn't decide how to do elisions, so we left some unfixed. >> > > Sorry I didn't chime in back then! "..." is arbitrary, but it's what we > already use for the qmp lexer and in the incremental backup/bitmap docs, so > I figured consistency was good. It is. > The QMP lexer has syntax support for ->, <- and ... and otherwise requires > the examples to be valid JSON. It doesn't understand grammar, though, so > it's kind of "dumb", but this is one small protection.
Re: [PATCH RESEND v7 00/12] vhost-user: support any POSIX system (tested on macOS, FreeBSD, OpenBSD)
Stefano Garzarella writes: > Hi Michael, > > On Wed, Jun 12, 2024 at 03:01:28PM GMT, Stefano Garzarella wrote: >>This series should be in a good shape, in which tree should we queue it? >>@Micheal would your tree be okay? > > Markus suggested a small change to patch 10, so do you want me to resend the > whole series, or is it okay to resend just the last 3 patches (which are also > the ones that depend on the other patch queued by Markus)? I guess you mean [PATCH v2] qapi: clarify that the default is backend dependent Message-ID: <20240611130231.83152-1-sgarz...@redhat.com> > In the last case I would ask you to queue up the first 9 patches of this > series if that is okay with you. Michael, feel free to merge the patch I queued.
Re: [PATCH 20/20] qapi: convert "Example" sections to rST
John Snow writes:
> Eliminate the "Example" sections in QAPI doc blocks, converting them
> into QMP example code blocks. This is generally done by converting
> "Example:" or "Examples:" lines into ".. code-block:: QMP" lines.
>
> This patch does also allow for the use of the rST syntax "Example::" by
> exempting double-colon syntax from the QAPI doc parser, but that form is
> not used by this conversion patch. The phrase "Example" here is not
> special, it is the double-colon syntax that transforms the following
> block into a code-block. By default, *this* form does not apply QMP
> highlighting.
>
> This patch has several benefits:
>
> 1. Example sections can now be written more arbitrarily, mixing
>explanatory paragraphs and code blocks however desired.
>
> 2. Example sections can now use fully arbitrary rST.
>
> 3. All code blocks are now lexed and validated as QMP; increasing
>usability of the docs and ensuring validity of example snippets.
>
> 4. Each code-block can be captioned independently without bypassing the
>QMP lexer/validator.
>
> For any sections with more than one example, examples are split up into
> multiple code-block regions. If annotations are present, those
> annotations are converted into code-block captions instead, e.g.
>
> ```
> Examples:
>
>1. Lorem Ipsum
>
>-> { "foo": "bar" }
> ```
>
> Is rewritten as:
>
> ```
> .. code-block:: QMP
>:caption: Example: Lorem Ipsum
>
>-> { "foo": "bar" }
> ```
>
> This process was only semi-automated:
>
> 1. Replace "Examples?:" sections with sed:
>
> sed -i 's|# Example:|# .. code-block:: QMP|' *.json
> sed -i 's|# Examples:|# .. code-block:: QMP|' *.json
>
> 2. Identify sections that no longer parse successfully by attempting the
>doc build, convert annotations into captions manually.
>(Tedious, oh well.)
>
> 3. Add captions where still needed:
>
> sed -zi 's|# .. code-block:: QMP\n#\n|# .. code-block:: QMP\n#:caption:
> Example\n#\n|g' *.json
>
> Not fully ideal, but hopefully not something that has to be done very
> often. (Or ever again.)
>
> Signed-off-by: John Snow
> ---
> qapi/acpi.json | 6 +-
> qapi/block-core.json| 120 --
> qapi/block.json | 60 +++--
> qapi/char.json | 36 ++--
> qapi/control.json | 16 ++--
> qapi/dump.json | 12 ++-
> qapi/machine-target.json| 3 +-
> qapi/machine.json | 79 ++---
> qapi/migration.json | 145 +++-
> qapi/misc-target.json | 33 +---
> qapi/misc.json | 48 +++
> qapi/net.json | 30 +--
> qapi/pci.json | 6 +-
> qapi/qapi-schema.json | 6 +-
> qapi/qdev.json | 15 +++-
> qapi/qom.json | 20 +++--
> qapi/replay.json| 12 ++-
> qapi/rocker.json| 12 ++-
> qapi/run-state.json | 45 ++
> qapi/tpm.json | 9 +-
> qapi/trace.json | 6 +-
> qapi/transaction.json | 3 +-
> qapi/ui.json| 62 +-
> qapi/virtio.json| 38 +
> qapi/yank.json | 6 +-
> scripts/qapi/parser.py | 15 +++-
> tests/qapi-schema/doc-good.json | 12 +--
> tests/qapi-schema/doc-good.out | 17 ++--
> tests/qapi-schema/doc-good.txt | 17 +---
> 29 files changed, 574 insertions(+), 315 deletions(-)
Missing: update of docs/devel/qapi-code-gen.rst.
> diff --git a/qapi/acpi.json b/qapi/acpi.json
> index aa4dbe57943..3da01f1b7fc 100644
> --- a/qapi/acpi.json
> +++ b/qapi/acpi.json
> @@ -111,7 +111,8 @@
> #
> # Since: 2.1
> #
> -# Example:
> +# .. code-block:: QMP
> +#:caption: Example
I wish this was a bit less verbose. Oh well, we'll live.
> #
> # -> { "execute": "query-acpi-ospm-status" }
> # <- { "return": [ { "device": "d1", "slot": "0", "slot-type": "DIMM",
> "source": 1, "status": 0},
This is rendered as a light green box with the caption on top, in
italics and centered. I'm not sure I like the use of the caption. The
previous patch's Note boxes look nicer.
The contents of the box is highlighted. I am sure I like that.
> @@ -131,7 +132,8 @@
> #
> # Since: 2.1
> #
> -# Example:
> +# .. code-block:: QMP
> +#:caption: Example
> #
> # <- { "event": "ACPI_DEVICE_OST",
> # "data": { "info": { "device": "d1", "slot": "0",
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 530af40404d..bb0447207df 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -763,7 +763,8 @@
> #
> # Since: 0.14
> #
> -# Example:
> +# .. code-block:: QMP
> +#:caption: Example
> #
> # -> { "execute": "query-block" }
> # <- {
> @@ -1167,7 +1168,8 @@
> #
> # Since: 0.14
> #
> -# Example:
> +# ..
Re: [PATCH 19/20] qapi: convert "Note" sections to plain rST
John Snow writes:
> We do not need a dedicated section for notes. By eliminating a specially
> parsed section, these notes can be treated as normal rST paragraphs in
> the new QMP reference manual, and can be placed and styled much more
> flexibly.
>
> Update the QAPI parser to now prohibit special "Note" sections while
> suggesting a new syntax.
>
> The exact formatting to use is a matter of taste, but a good candidate
> is simply:
>
> .. note:: lorem ipsum ...
>
> but there are other choices, too. The Sphinx readthedocs theme offers
> theming for the following forms (capitalization unimportant); all are
> adorned with a (!) symbol in the title bar for rendered HTML docs.
>
> These are rendered in orange:
>
> .. Attention:: ...
> .. Caution:: ...
> .. WARNING:: ...
>
> These are rendered in red:
>
> .. DANGER:: ...
> .. Error:: ...
>
> These are rendered in green:
>
> .. Hint:: ...
> .. Important:: ...
> .. Tip:: ...
>
> These are rendered in blue:
>
> .. Note:: ...
> .. admonition:: custom title
>
>admonition body text
>
> This patch uses ".. notes::" almost everywhere, with just two "caution"
> directives. ".. admonition:: notes" is used in a few places where we had
> an ordered list of multiple notes.
>
> Signed-off-by: John Snow
> ---
> qapi/block-core.json | 30 +++
> qapi/block.json | 2 +-
> qapi/char.json| 12 +--
> qapi/control.json | 15 ++--
> qapi/dump.json| 2 +-
> qapi/introspect.json | 6 +-
> qapi/machine-target.json | 26 +++---
> qapi/machine.json | 47 +-
> qapi/migration.json | 12 +--
> qapi/misc.json| 88 +--
> qapi/net.json | 6 +-
> qapi/pci.json | 7 +-
> qapi/qdev.json| 30 +++
> qapi/qom.json | 19 ++--
> qapi/rocker.json | 16 ++--
> qapi/run-state.json | 18 ++--
> qapi/sockets.json | 10 +--
> qapi/stats.json | 22 ++---
> qapi/transaction.json | 8 +-
> qapi/ui.json | 29 +++---
> qapi/virtio.json | 12 +--
> qga/qapi-schema.json | 48 +-
> scripts/qapi/parser.py| 9 ++
> tests/qapi-schema/doc-empty-section.err | 2 +-
> tests/qapi-schema/doc-empty-section.json | 2 +-
> tests/qapi-schema/doc-good.json | 6 +-
> tests/qapi-schema/doc-good.out| 10 ++-
> tests/qapi-schema/doc-good.txt| 14 ++-
> .../qapi-schema/doc-interleaved-section.json | 2 +-
> 29 files changed, 258 insertions(+), 252 deletions(-)
Missing: update of docs/devel/qapi-code-gen.rst. Something like
diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
index f453bd3546..1a4e240adb 100644
--- a/docs/devel/qapi-code-gen.rst
+++ b/docs/devel/qapi-code-gen.rst
@@ -995,14 +995,13 @@ line "Features:", like this::
# @feature: Description text
A tagged section begins with a paragraph that starts with one of the
-following words: "Note:"/"Notes:", "Since:", "Example:"/"Examples:",
-"Returns:", "Errors:", "TODO:". It ends with the start of a new
-section.
+following words: "Since:", "Example:"/"Examples:", "Returns:",
+"Errors:", "TODO:". It ends with the start of a new section.
The second and subsequent lines of tagged sections must be indented
like this::
- # Note: Ut enim ad minim veniam, quis nostrud exercitation ullamco
+ # TODO: Ut enim ad minim veniam, quis nostrud exercitation ullamco
# laboris nisi ut aliquip ex ea commodo consequat.
#
# Duis aute irure dolor in reprehenderit in voluptate velit esse
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 64fe5240cc9..530af40404d 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1510,7 +1510,7 @@
> # @mode: whether and how QEMU should create a new image, default is
> # 'absolute-paths'.
> #
> -# Note: Either @device or @node-name must be set but not both.
> +# .. note:: Either @device or @node-name must be set but not both.
The commit message talks about ".. Note::", but you actually use
".. note::". Is there a difference?
> #
> ##
> { 'struct': 'BlockdevSnapshotSync',
> @@ -1616,9 +1616,9 @@
> #
> # @unstable: Member @x-perf is experimental.
> #
> -# Note: @on-source-error and @on-target-error only affect background
> -# I/O. If an error occurs during a guest write request, the
> -# device's rerror/werror actions will be used.
> +# .. note:: @on-source-error and @on-target-error only affect
Re: [PATCH 18/20] qapi: ensure all errors sections are uniformly typset
John Snow writes: > Transactions have the only instance of an Errors section that isn't a > rST list; turn it into one. Just for consistency? Or do you have other shenanigans up your sleeve? If we want the Errors sections to remain all rST lists, we should update docs/devel/qapi-code-gen.rst to say so. > Signed-off-by: John Snow
Re: [PATCH 14/20] qapi: fix non-compliant JSON examples
John Snow writes: > If we parse all examples as QMP, we need them to conform to a standard > so that they render correctly. Once the QMP lexer is active for > examples, these will produce warning messages and fail the build. > > The QMP lexer still supports elisions, but they must be represented as > the value "...", so two examples have been adjusted to support that > format here. I think this could use a bit more context. I believe you're referring to docs/sphinx/qmp_lexer.py. It describes itself as "a Sphinx extension that provides a QMP lexer for code blocks." "If we parse all examples as QMP" and "Once the QMP lexer is active for examples" suggests we're *not* using it for (some?) examples. So what are we using it for? > Signed-off-by: John Snow Patch looks lovely. Hat tip to Victor Toso, who fixed up most examples two years ago. Back then we couldn't decide how to do elisions, so we left some unfixed.
Re: [PATCH 13/20] docs/qapidoc: fix nested parsing under untagged sections
John Snow writes:
> Sphinx does not like sections without titles, because it wants to
> convert every section into a reference. When there is no title, it
> struggles to do this and transforms the tree inproperly.
>
> Depending on the rST used, this may result in an assertion error deep in
> the docutils HTMLWriter.
I'm getting vibes of someone having had hours of "fun" with Sphinx...
Can you give you an idea of how a reproducer would look like?
> When parsing an untagged section (free paragraphs), skip making a hollow
> section and instead append the parse results to the prior section.
>
> Many Bothans died to bring us this information.
Terribly sad.
> Signed-off-by: John Snow
> ---
> docs/sphinx/qapidoc.py | 16 +++-
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index 34e95bd168d..cfc0cf169ef 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -286,14 +286,20 @@ def _nodes_for_sections(self, doc):
> if section.tag and section.tag == 'TODO':
> # Hide TODO: sections
> continue
> +
> +if not section.tag:
> +# Sphinx cannot handle sectionless titles;
> +# Instead, just append the results to the prior section.
> +container = nodes.container()
> +self._parse_text_into_node(section.text, container)
> +nodelist += container.children
> +continue
> +
> snode = self._make_section(section.tag)
> -if section.tag and section.tag.startswith('Example'):
> +if section.tag.startswith('Example'):
> snode += self._nodes_for_example(dedent(section.text))
> else:
> -self._parse_text_into_node(
> -dedent(section.text) if section.tag else section.text,
> -snode,
> -)
> +self._parse_text_into_node(dedent(section.text), snode)
> nodelist.append(snode)
> return nodelist
Looks plausible. I lack the Sphinx-fu to say more.
Re: [PATCH 11/20] qapi/schema: add doc_visible property to QAPISchemaDefinition
John Snow writes: > The intent here is to mark only certain definitions as visible in the > end-user docs. > > All commands and events are inherently visible. Everything else is > visible only if it is a member (or a branch member) of a type that is > visible, or if it is named as a return type for a command. > > Notably, this excludes arg_type for commands and events, and any > base_types specified for structures/unions. Those objects may still be > marked visible if they are named as members from a visible type. Why? I figure the answer is "because the transmogrifier inlines the things excluded". Correct? > This does not necessarily match the data revealed by introspection: in > this case, we want anything that we are cross-referencing in generated > documentation to be available to target. I don't get the part after the colon. > Some internal and built-in types may be marked visible with this > approach, but if they do not have a documentation block, they'll be > skipped by the generator anyway. This includes array types and built-in > primitives which do not get their own documentation objects. > > This information is not yet used by qapidoc, which continues to render > documentation exactly as it has. This information will be used by the > new qapidoc (the "transmogrifier"), to be introduced later. The new > generator verifies that all of the objects that should be rendered *are* > by failing if any cross-references are missing, verifying everything is > in place. So... we decide "doc should be visible" here, and then the transmogrifier decides again, and we check the two decisions match? > Signed-off-by: John Snow
Re: [PATCH 09/20] qapi/parser: add undocumented stub members to all_sections
John Snow writes:
> This helps simplify the doc generator if it doesn't have to check for
> undocumented members.
>
> Signed-off-by: John Snow
> ---
> scripts/qapi/parser.py | 20 ++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index b1794f71e12..3cd8e7ee295 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -740,8 +740,24 @@ def connect_member(self, member: 'QAPISchemaMember') ->
> None:
> raise QAPISemError(member.info,
> "%s '%s' lacks documentation"
> % (member.role, member.name))
> -self.args[member.name] = QAPIDoc.ArgSection(
> -self.info, '@' + member.name, 'member')
> +
> +# Insert stub documentation section for missing member docs.
> +section = QAPIDoc.ArgSection(
> +self.info, f"@{member.name}", "member")
Although I like f-strings in general, I'd pefer to stick to '@' +
member.name here, because it's simpler.
Also, let's not change 'member' to "member". Existing practice: single
quotes for string literals unless double quotes avoid escapes. Except
English prose (like error messages) is always in double quotes.
> +self.args[member.name] = section
> +
> +# Determine where to insert stub doc.
If we have some member documentation, the member doc stubs clearly must
go there. Inserting them at the end makes sense.
Else we want to put them where the parser would accept real member
documentation.
"The parser" is .get_doc(). This is what it accepts (I'm prepared to
explain this in detail if necessary):
One untagged section
Member documentation, if any
Zero ore more tagged or untagged sections
Feature documentation, if any
Zero or more tagged or untagged sections
If we there is no member documentation, this is
One untagged section
Zero ore more tagged or untagged sections
Feature documentation, if any
Zero or more tagged or untagged sections
Note that we cannot have two adjacent untagged sections (we only create
one if the current section isn't untagged; if it is, we extend it
instead). Thus, the second section must be tagged or feature
documentation.
Therefore, the member doc stubs must go right after the first section.
This is also where qapidoc.py inserts member documentation.
> +index = 0
> +for i, sect in enumerate(self.all_sections):
> +# insert after these:
> +if sect.kind in ('intro-paragraph', 'member'):
> +index = i + 1
> +# but before these:
> +elif sect.kind in ('tagged', 'feature', 'outro-paragraph'):
> +index = i
> +break
Can you describe what this does in English? As a specification; simply
paraphrasing the code is cheating. I tried, and gave up.
Above, I derived what I believe we need to do. It's simple enough: if
we have member documentation, it starts right after the first (untagged)
section, and the stub goes to the end of the member documentation.
Else, the stub goes right after the first section.
Code:
index = 1;
while self.all_sections[index].kind == 'member':
index += 1
Of course future patches I haven't seen might change the invariants in
ways that break my simple code. We'll see.
> +self.all_sections.insert(index, section)
> +
> self.args[member.name].connect(member)
>
> def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
Re: [PATCH 07/20] qapi/parser: add semantic 'kind' parameter to QAPIDoc.Section
John Snow writes:
> On Thu, May 16, 2024, 2:18 AM Markus Armbruster wrote:
>
>> John Snow writes:
>>
>> > When iterating all_sections, this is helpful to be able to distinguish
>> > "members" from "features"; the only other way to do so is to
>> > cross-reference these sections against QAPIDoc.args or QAPIDoc.features,
>> > but if the desired end goal for QAPIDoc is to remove everything except
>> > all_sections, we need *something* accessible to distinguish them.
>> >
>> > To keep types simple, add this semantic parameter to the base Section
>> > and not just ArgSection; we can use this to filter out paragraphs and
>> > tagged sections, too.
>> >
>> > Signed-off-by: John Snow
>> > ---
>> > scripts/qapi/parser.py | 25 -
>> > 1 file changed, 16 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>> > index 161768b8b96..cf4cbca1c1f 100644
>> > --- a/scripts/qapi/parser.py
>> > +++ b/scripts/qapi/parser.py
>> > @@ -613,21 +613,27 @@ class QAPIDoc:
>> >
>> > class Section:
>> > # pylint: disable=too-few-public-methods
>> > -def __init__(self, info: QAPISourceInfo,
>> > - tag: Optional[str] = None):
>> > +def __init__(
>> > +self,
>> > +info: QAPISourceInfo,
>> > +tag: Optional[str] = None,
>> > +kind: str = 'paragraph',
>> > +):
>> > # section source info, i.e. where it begins
>> > self.info = info
>> > # section tag, if any ('Returns', '@name', ...)
>> > self.tag = tag
>> > # section text without tag
>> > self.text = ''
>> > +# section type - {paragraph, feature, member, tagged}
>> > +self.kind = kind
>>
>> Hmm. .kind is almost redundant with .tag.
>>
>
> Almost, yes. But the crucial bit is members/features as you notice. That's
> the real necessity here that saves a lot of code when relying on *only*
> all_sections.
>
> (If you want to remove the other fields leaving only all_sections behind,
> this is strictly necessary.)
>
>
>> Untagged section:.kind is 'paragraph', .tag is None
>>
>> Member description: .kind is 'member', .tag matches @NAME
>>
>> Feature description: .kind is 'feature', .tag matches @NAME
>
>
>> Tagged section: .kind is 'tagged', .tag matches
>> r'Returns|Errors|Since|Notes?|Examples?|TODO'
>>
>> .kind can directly be derived from .tag except for member and feature
>> descriptions. And you want to tell these two apart in a straightforward
>> manner in later patches, as you explain in your commit message.
>>
>> If .kind is 'member' or 'feature', then self must be an ArgSection.
>> Worth a comment? An assertion?
>>
>
> No real need. The classes don't differ much in practice so there's not much
> benefit, and asserting it won't help the static typer out anyway because it
> can't remember the inference from string->type anyway.
>
> If you wanted to be FANCY, we could use string literal typing on the field
> and restrict valid values per-class, but that's so needless not even I'm
> tempted by it.
>
>
>> Some time back, I considered changing .tag for member and feature
>> descriptions to suitable strings, like your 'member' and 'feature', and
>> move the member / feature name into ArgSection. I didn't, because the
>> benefit wasn't worth the churn at the time. Perhaps it's worth it now.
>> Would it result in simpler code than your solution?
>>
>
> Not considerably, I think. Would just be shuffling around which field names
> I touch and where/when.
The way .tag works irks me. I might explore the change I proposed just
to see whether I hate the result less. On top of your work, to not
annoy you without need.
> It might actually just add some lines where I have to assert isinstance to
> do type narrowing in the generator.
>
>> Terminology nit: the section you call 'paragraph' isn't actually a
>> paragraph: it could be several paragraphs. Best to call it 'untagged',
>> as in .ensure_untagged_section().
>>
>
> Oh, I hate when you make a good point. I was avoiding the term because I'm
> removing Notes and Examples, and we have plans to eliminate Since ... the
> tagged sections are almost going away entirely, leaving just TODO, wh
Re: [PATCH 06/20] qapi/parser: fix comment parsing immediately following a doc block
John Snow writes:
> On Thu, May 16, 2024 at 2:01 AM Markus Armbruster wrote:
>
>> John Snow writes:
>>
>> > If a comment immediately follows a doc block, the parser doesn't ignore
>> > that token appropriately. Fix that.
>>
>> Reproducer?
>>
>> >
>> > Signed-off-by: John Snow
>> > ---
>> > scripts/qapi/parser.py | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>> > index 41b9319e5cb..161768b8b96 100644
>> > --- a/scripts/qapi/parser.py
>> > +++ b/scripts/qapi/parser.py
>> > @@ -587,7 +587,7 @@ def get_doc(self) -> 'QAPIDoc':
>> > line = self.get_doc_line()
>> > first = False
>> >
>> > -self.accept(False)
>> > +self.accept()
>> > doc.end()
>> > return doc
>>
>> Can't judge the fix without understanding the problem, and the problem
>> will be easier to understand for me with a reproducer.
>>
>
> audio.json:
>
> ```
> ##
> # = Audio
> ##
>
> ##
> # @AudiodevPerDirectionOptions:
> #
> # General audio backend options that are used for both playback and
> # recording.
> #
> ```
>
> Modify this excerpt to have a comment after the "= Audio" header, say for
> instance if you were to take out that intro paragraph and transform it into
> a comment that preceded the AudiodevPerDirectionOptions doc block.
>
> e.g.
>
> ```
> ##
> # = Audio
> ##
>
> # Lorem Ipsum
>
> ##
> # @AudiodevPerDirectionOptions:
> ```
>
> the parser breaks because the line I changed that primes the next token is
> still set to "not ignore comments", but that breaks the parser and gives a
> rather unhelpful message:
>
> ../qapi/audio.json:13:1: junk after '##' at start of documentation comment
When ._parse()'s loop sees a comment token in .tok, it expects it to be
the start of a doc comment block (because any other comments are to be
skipped). It then calls .get_doc(), which expects the same. The
unexpected comment token then triggers .get_doc()'s check for junk after
'##'.
> Encountered when converting developer commentary from documentation
> paragraphs to mere QAPI comments.
Your fix is correct.
It's actually a regression. Please add
Fixes: 3d035cd2cca6 (qapi: Rewrite doc comment parser)
to the commit message.
Let's add a reproducer to tests/qapi-schema/doc-good.json right in this
patch, e.g.
diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json
index de38a386e8..8b39eb946a 100644
--- a/tests/qapi-schema/doc-good.json
+++ b/tests/qapi-schema/doc-good.json
@@ -55,6 +55,8 @@
# - {braces}
##
+# Not a doc comment
+
##
# @Enum:
#
Re: [PATCH RESEND v7 10/12] hostmem: add a new memory backend based on POSIX shm_open()
Stefano Garzarella writes:
> shm_open() creates and opens a new POSIX shared memory object.
> A POSIX shared memory object allows creating memory backend with an
> associated file descriptor that can be shared with external processes
> (e.g. vhost-user).
>
> The new `memory-backend-shm` can be used as an alternative when
> `memory-backend-memfd` is not available (Linux only), since shm_open()
> should be provided by any POSIX-compliant operating system.
>
> This backend mimics memfd, allocating memory that is practically
> anonymous. In theory shm_open() requires a name, but this is allocated
> for a short time interval and shm_unlink() is called right after
> shm_open(). After that, only fd is shared with external processes
> (e.g., vhost-user) as if it were associated with anonymous memory.
>
> In the future we may also allow the user to specify the name to be
> passed to shm_open(), but for now we keep the backend simple, mimicking
> anonymous memory such as memfd.
>
> Acked-by: David Hildenbrand
> Acked-by: Stefan Hajnoczi
> Signed-off-by: Stefano Garzarella
[...]
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 9b8f6a7ab5..94e4458288 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -601,8 +601,8 @@
> #
> # @share: if false, the memory is private to QEMU; if true, it is
> # shared (default false for backends memory-backend-file and
> -# memory-backend-ram, true for backends memory-backend-epc and
> -# memory-backend-memfd)
> +# memory-backend-ram, true for backends memory-backend-epc,
> +# memory-backend-memfd, and memory-backend-shm)
> #
> # @reserve: if true, reserve swap space (or huge pages) if applicable
> # (default: true) (since 6.1)
> @@ -721,6 +721,22 @@
> '*hugetlbsize': 'size',
> '*seal': 'bool' } }
>
> +##
> +# @MemoryBackendShmProperties:
> +#
> +# Properties for memory-backend-shm objects.
> +#
> +# Setting @share boolean option (defined in the base type) to false
> +# will cause a failure during allocation because it is not
> +# supported by this backend.
This is QMP reference documentation. "Failure during allocation" feels
like unnecessary detail there. Maybe "This memory backend support only
shared memory, which is the default."
> +#
> +# Since: 9.1
> +##
> +{ 'struct': 'MemoryBackendShmProperties',
> + 'base': 'MemoryBackendProperties',
> + 'data': { },
> + 'if': 'CONFIG_POSIX' }
> +
> ##
> # @MemoryBackendEpcProperties:
> #
> @@ -1049,6 +1065,8 @@
> { 'name': 'memory-backend-memfd',
>'if': 'CONFIG_LINUX' },
> 'memory-backend-ram',
> +{ 'name': 'memory-backend-shm',
> + 'if': 'CONFIG_POSIX' },
> 'pef-guest',
> { 'name': 'pr-manager-helper',
>'if': 'CONFIG_LINUX' },
> @@ -1121,6 +1139,8 @@
>'memory-backend-memfd': { 'type': 'MemoryBackendMemfdProperties',
>'if': 'CONFIG_LINUX' },
>'memory-backend-ram': 'MemoryBackendProperties',
> + 'memory-backend-shm': { 'type': 'MemoryBackendShmProperties',
> + 'if': 'CONFIG_POSIX' },
> 'pr-manager-helper': { 'type': 'PrManagerHelperProperties',
>'if': 'CONFIG_LINUX' },
>'qtest': 'QtestProperties',
[...]
Other than that, QAPI schema
Acked-by: Markus Armbruster
Re: [RFC PATCH v2 21/22] qapi: Inline and remove QERR_UNSUPPORTED definition
Philippe Mathieu-Daudé writes: > Michael, Konstantin, QERR_UNSUPPORTED is only used by QGA. > > Do you mind giving our maintainer's position on Markus > analysis so we can know how to proceed with this definition? Daniel Berrangé recently posted patches that get rid of most instances of QERR_UNSUPPORTED: [PATCH 00/20] qga: clean up command source locations and conditionals https://lore.kernel.org/qemu-devel/20240604134933.220112-1-berra...@redhat.com/ I pointed out a possible opportunity to remove even more. I think we should let the dust settle there, then figure out how to eliminate remaining QERR_UNSUPPORTED, if any.
Re: [PATCH v2 2/2] backup: add minimum cluster size to performance options
Fiona Ebner writes:
> In the context of backup fleecing, discarding the source will not work
> when the fleecing image has a larger granularity than the one used for
> block-copy operations (can happen if the backup target has smaller
> cluster size), because cbw_co_pdiscard_snapshot() will align down the
> discard requests and thus effectively ignore then.
>
> To make @discard-source work in such a scenario, allow specifying the
> minimum cluster size used for block-copy operations and thus in
> particular also the granularity for discard requests to the source.
>
> Suggested-by: Vladimir Sementsov-Ogievskiy
> Signed-off-by: Fiona Ebner
[...]
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 8fc0a4b234..f1219a9dfb 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1551,11 +1551,16 @@
> # it should not be less than job cluster size which is calculated
> # as maximum of target image cluster size and 64k. Default 0.
> #
> +# @min-cluster-size: Minimum size of blocks used by copy-before-write
> +# and background copy operations. Has to be a power of 2. No
> +# effect if smaller than the maximum of the target's cluster size
> +# and 64 KiB. Default 0. (Since 9.1)
> +#
> # Since: 6.0
> ##
> { 'struct': 'BackupPerf',
> - 'data': { '*use-copy-range': 'bool',
> -'*max-workers': 'int', '*max-chunk': 'int64' } }
> + 'data': { '*use-copy-range': 'bool', '*max-workers': 'int',
> +'*max-chunk': 'int64', '*min-cluster-size': 'size' } }
>
> ##
> # @BackupCommon:
QAPI schema
Acked-by: Markus Armbruster
Re: [PATCH v2 1/2] copy-before-write: allow specifying minimum cluster size
errp);
This is now more obviously safe. Thanks!
> if (cluster_size < 0) {
> return NULL;
> }
> diff --git a/block/copy-before-write.c b/block/copy-before-write.c
> index cd65524e26..ef0bc4dcfe 100644
> --- a/block/copy-before-write.c
> +++ b/block/copy-before-write.c
> @@ -417,6 +417,7 @@ static BlockdevOptions *cbw_parse_options(QDict *options,
> Error **errp)
> qdict_extract_subqdict(options, NULL, "bitmap");
> qdict_del(options, "on-cbw-error");
> qdict_del(options, "cbw-timeout");
> +qdict_del(options, "min-cluster-size");
>
> out:
> visit_free(v);
> @@ -432,6 +433,7 @@ static int cbw_open(BlockDriverState *bs, QDict *options,
> int flags,
> BDRVCopyBeforeWriteState *s = bs->opaque;
> BdrvDirtyBitmap *bitmap = NULL;
> int64_t cluster_size;
> +uint64_t min_cluster_size = 0;
> g_autoptr(BlockdevOptions) full_opts = NULL;
> BlockdevOptionsCbw *opts;
> int ret;
> @@ -476,8 +478,14 @@ static int cbw_open(BlockDriverState *bs, QDict
> *options, int flags,
> bs->file->bs->supported_zero_flags);
>
> s->discard_source = flags & BDRV_O_CBW_DISCARD_SOURCE;
> +
> +if (opts->has_min_cluster_size) {
> +min_cluster_size = opts->min_cluster_size;
> +}
> +
> s->bcs = block_copy_state_new(bs->file, s->target, bs, bitmap,
> - flags & BDRV_O_CBW_DISCARD_SOURCE, errp);
> + flags & BDRV_O_CBW_DISCARD_SOURCE,
> + min_cluster_size, errp);
> if (!s->bcs) {
> error_prepend(errp, "Cannot create block-copy-state: ");
> return -EINVAL;
You can pass opts->min_cluster_size directly, as in v1. When
!opts->has_min_cluster_size, then opts->min_cluster_size is zero.
> diff --git a/include/block/block-copy.h b/include/block/block-copy.h
> index bdc703bacd..dd5cc82f3b 100644
> --- a/include/block/block-copy.h
> +++ b/include/block/block-copy.h
> @@ -28,6 +28,7 @@ BlockCopyState *block_copy_state_new(BdrvChild *source,
> BdrvChild *target,
> BlockDriverState *copy_bitmap_bs,
> const BdrvDirtyBitmap *bitmap,
> bool discard_source,
> + uint64_t min_cluster_size,
> Error **errp);
>
> /* Function should be called prior any actual copy request */
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index df5e07debd..8fc0a4b234 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4642,12 +4642,18 @@
> # @on-cbw-error parameter will decide how this failure is handled.
> # Default 0. (Since 7.1)
> #
> +# @min-cluster-size: Minimum size of blocks used by copy-before-write
> +# operations. Has to be a power of 2. No effect if smaller than
> +# the maximum of the target's cluster size and 64 KiB. Default 0.
> +# (Since 9.1)
> +#
> # Since: 6.2
> ##
> { 'struct': 'BlockdevOptionsCbw',
>'base': 'BlockdevOptionsGenericFormat',
>'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap',
> -'*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32' } }
> +'*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32',
> +'*min-cluster-size': 'size' } }
>
> ##
> # @BlockdevOptions:
QAPI schema
Acked-by: Markus Armbruster
Re: [PATCH v6 10/12] hostmem: add a new memory backend based on POSIX shm_open()
Stefano Garzarella writes: > On Wed, May 29, 2024 at 04:50:20PM GMT, Markus Armbruster wrote: >>Stefano Garzarella writes: >> >>> shm_open() creates and opens a new POSIX shared memory object. >>> A POSIX shared memory object allows creating memory backend with an >>> associated file descriptor that can be shared with external processes >>> (e.g. vhost-user). >>> >>> The new `memory-backend-shm` can be used as an alternative when >>> `memory-backend-memfd` is not available (Linux only), since shm_open() >>> should be provided by any POSIX-compliant operating system. >>> >>> This backend mimics memfd, allocating memory that is practically >>> anonymous. In theory shm_open() requires a name, but this is allocated >>> for a short time interval and shm_unlink() is called right after >>> shm_open(). After that, only fd is shared with external processes >>> (e.g., vhost-user) as if it were associated with anonymous memory. >>> >>> In the future we may also allow the user to specify the name to be >>> passed to shm_open(), but for now we keep the backend simple, mimicking >>> anonymous memory such as memfd. >>> >>> Acked-by: David Hildenbrand >>> Acked-by: Stefan Hajnoczi >>> Signed-off-by: Stefano Garzarella >>> --- >>> v5 >>> - fixed documentation in qapi/qom.json and qemu-options.hx [Markus] >>> v4 >>> - fail if we find "share=off" in shm_backend_memory_alloc() [David] >>> v3 >>> - enriched commit message and documentation to highlight that we >>> want to mimic memfd (David) >>> --- >>> docs/system/devices/vhost-user.rst | 5 +- >>> qapi/qom.json | 19 + >>> backends/hostmem-shm.c | 123 + >>> backends/meson.build | 1 + >>> qemu-options.hx| 16 >>> 5 files changed, 162 insertions(+), 2 deletions(-) >>> create mode 100644 backends/hostmem-shm.c >>> >>> diff --git a/docs/system/devices/vhost-user.rst >>> b/docs/system/devices/vhost-user.rst >>> index 9b2da106ce..35259d8ec7 100644 >>> --- a/docs/system/devices/vhost-user.rst >>> +++ b/docs/system/devices/vhost-user.rst >>> @@ -98,8 +98,9 @@ Shared memory object >>> >>> In order for the daemon to access the VirtIO queues to process the >>> requests it needs access to the guest's address space. This is >>> -achieved via the ``memory-backend-file`` or ``memory-backend-memfd`` >>> -objects. A reference to a file-descriptor which can access this object >>> +achieved via the ``memory-backend-file``, ``memory-backend-memfd``, or >>> +``memory-backend-shm`` objects. >>> +A reference to a file-descriptor which can access this object >>> will be passed via the socket as part of the protocol negotiation. >>> >>> Currently the shared memory object needs to match the size of the main >>> diff --git a/qapi/qom.json b/qapi/qom.json >>> index 38dde6d785..d40592d863 100644 >>> --- a/qapi/qom.json >>> +++ b/qapi/qom.json >>> @@ -721,6 +721,21 @@ >>> '*hugetlbsize': 'size', >>> '*seal': 'bool' } } >>> >>> +## >>> +# @MemoryBackendShmProperties: >>> +# >>> +# Properties for memory-backend-shm objects. >>> +# >>> +# The @share boolean option is true by default with shm. Setting it to >>> false >>> +# will cause a failure during allocation because it is not supported by >>> this >>> +# backend. >> >>docs/devel/qapi-code-gen.rst: >> >>For legibility, wrap text paragraphs so every line is at most 70 >>characters long. >> >>Separate sentences with two spaces. >> >>Result: >> >> # Properties for memory-backend-shm objects. >> # >> # The @share boolean option is true by default with shm. Setting it >> # to false will cause a failure during allocation because it is not >> # supported by this backend. > > Ops, sorry, I'll fix! > >> >>However, this contradicts the doc comment for @share: >> >> # @share: if false, the memory is private to QEMU; if true, it is >> # shared (default: false) >> >>Your intention is to override that text. But that's less than clear. >>Moreover, the documentation of @share is pretty far from this override. >>John Snow is working on patches that'll pull it
Re: [PATCH v6 10/12] hostmem: add a new memory backend based on POSIX shm_open()
Stefano Garzarella writes:
> shm_open() creates and opens a new POSIX shared memory object.
> A POSIX shared memory object allows creating memory backend with an
> associated file descriptor that can be shared with external processes
> (e.g. vhost-user).
>
> The new `memory-backend-shm` can be used as an alternative when
> `memory-backend-memfd` is not available (Linux only), since shm_open()
> should be provided by any POSIX-compliant operating system.
>
> This backend mimics memfd, allocating memory that is practically
> anonymous. In theory shm_open() requires a name, but this is allocated
> for a short time interval and shm_unlink() is called right after
> shm_open(). After that, only fd is shared with external processes
> (e.g., vhost-user) as if it were associated with anonymous memory.
>
> In the future we may also allow the user to specify the name to be
> passed to shm_open(), but for now we keep the backend simple, mimicking
> anonymous memory such as memfd.
>
> Acked-by: David Hildenbrand
> Acked-by: Stefan Hajnoczi
> Signed-off-by: Stefano Garzarella
> ---
> v5
> - fixed documentation in qapi/qom.json and qemu-options.hx [Markus]
> v4
> - fail if we find "share=off" in shm_backend_memory_alloc() [David]
> v3
> - enriched commit message and documentation to highlight that we
> want to mimic memfd (David)
> ---
> docs/system/devices/vhost-user.rst | 5 +-
> qapi/qom.json | 19 +
> backends/hostmem-shm.c | 123 +
> backends/meson.build | 1 +
> qemu-options.hx| 16
> 5 files changed, 162 insertions(+), 2 deletions(-)
> create mode 100644 backends/hostmem-shm.c
>
> diff --git a/docs/system/devices/vhost-user.rst
> b/docs/system/devices/vhost-user.rst
> index 9b2da106ce..35259d8ec7 100644
> --- a/docs/system/devices/vhost-user.rst
> +++ b/docs/system/devices/vhost-user.rst
> @@ -98,8 +98,9 @@ Shared memory object
>
> In order for the daemon to access the VirtIO queues to process the
> requests it needs access to the guest's address space. This is
> -achieved via the ``memory-backend-file`` or ``memory-backend-memfd``
> -objects. A reference to a file-descriptor which can access this object
> +achieved via the ``memory-backend-file``, ``memory-backend-memfd``, or
> +``memory-backend-shm`` objects.
> +A reference to a file-descriptor which can access this object
> will be passed via the socket as part of the protocol negotiation.
>
> Currently the shared memory object needs to match the size of the main
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 38dde6d785..d40592d863 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -721,6 +721,21 @@
> '*hugetlbsize': 'size',
> '*seal': 'bool' } }
>
> +##
> +# @MemoryBackendShmProperties:
> +#
> +# Properties for memory-backend-shm objects.
> +#
> +# The @share boolean option is true by default with shm. Setting it to false
> +# will cause a failure during allocation because it is not supported by this
> +# backend.
docs/devel/qapi-code-gen.rst:
For legibility, wrap text paragraphs so every line is at most 70
characters long.
Separate sentences with two spaces.
Result:
# Properties for memory-backend-shm objects.
#
# The @share boolean option is true by default with shm. Setting it
# to false will cause a failure during allocation because it is not
# supported by this backend.
However, this contradicts the doc comment for @share:
# @share: if false, the memory is private to QEMU; if true, it is
# shared (default: false)
Your intention is to override that text. But that's less than clear.
Moreover, the documentation of @share is pretty far from this override.
John Snow is working on patches that'll pull it closer.
Hmm, MemoryBackendMemfdProperties has the same override.
I think we should change the doc comment for @share to something like
# @share: if false, the memory is private to QEMU; if true, it is
# shared (default depends on the backend type)
and then document the actual default with each backend type.
> +#
> +# Since: 9.1
> +##
> +{ 'struct': 'MemoryBackendShmProperties',
> + 'base': 'MemoryBackendProperties',
> + 'data': { } }
Let's add 'if': 'CONFIG_POSIX' here.
> +
> ##
> # @MemoryBackendEpcProperties:
> #
> @@ -985,6 +1000,8 @@
> { 'name': 'memory-backend-memfd',
>'if': 'CONFIG_LINUX' },
> 'memory-backend-ram',
> +{ 'name': 'memory-backend-shm',
> + 'if': 'CONFIG_POSIX' },
> 'pef-guest',
> { 'name': 'pr-manager-helper',
>'if': 'CONFIG_LINUX' },
> @@ -1056,6 +1073,8 @@
>'memory-backend-memfd': { 'type': 'MemoryBackendMemfdProperties',
>'if': 'CONFIG_LINUX' },
>'memory-backend-ram': 'MemoryBackendProperties',
> + 'memory-backend-shm': { 'type': 'MemoryBackendShmProperties',
> +
Re: [PATCH 05/20] qapi/parser: adjust info location for doc body section
John Snow writes:
> On Thu, May 16, 2024, 1:58 AM Markus Armbruster wrote:
>
>> John Snow writes:
>>
>> > Instead of using the info object for the doc block as a whole, update
>> > the info pointer for each call to ensure_untagged_section when the
>> > existing section is otherwise empty. This way, Sphinx error information
>> > will match precisely to where the text actually starts.
>> >
>> > Signed-off-by: John Snow
>> > ---
>> > scripts/qapi/parser.py | 9 +++--
>> > 1 file changed, 7 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>> > index 8cdd5334ec6..41b9319e5cb 100644
>> > --- a/scripts/qapi/parser.py
>> > +++ b/scripts/qapi/parser.py
>> > @@ -662,8 +662,13 @@ def end(self) -> None:
>> >
>> > def ensure_untagged_section(self, info: QAPISourceInfo) -> None:
>> > if self.all_sections and not self.all_sections[-1].tag:
>> > -# extend current section
>> > -self.all_sections[-1].text += '\n'
>>
>> Before, we always append a newline.
>>
>> > +section = self.all_sections[-1]
>> > +# Section is empty so far; update info to start *here*.
>> > +if not section.text:
>> > +section.info = info
>> > +else:
>> > +# extend current section
>> > +self.all_sections[-1].text += '\n'
>>
>> Afterwards, we append it only when the section already has some text.
>>
>> The commit message claims the patch only adjusts section.info. That's a
>> lie :)
>>
>
> Well. It wasn't intentional, so it wasn't a lie... it was just wrong :)
>
>
>> I believe the change makes no difference because .end() strips leading
>> and trailing newline.
>>
>> > return
>> > # start new section
>> > section = self.Section(info)
>>
>> You could fix the commit message, but I think backing out the
>> no-difference change is easier. The appended patch works in my testing.
>>
>> Next one. Your patch changes the meaning of section.info. Here's its
>> initialization:
>>
>> class Section:
>> # pylint: disable=too-few-public-methods
>> def __init__(self, info: QAPISourceInfo,
>> tag: Optional[str] = None):
>> ---># section source info, i.e. where it begins
>> self.info = info
>> # section tag, if any ('Returns', '@name', ...)
>> self.tag = tag
>> # section text without tag
>> self.text = ''
>>
>> The comment is now wrong. Calls for a thorough review of .info's uses.
>>
>
> Hmm... Did I really change its meaning? I guess it's debatable what "where
> it begins" means. Does the tagless section start...
>
> ## <-- Here?
> # Hello! <-- Or here?
> ##
>
> I assert the *section* starts wherever the first line of text it contains
> starts. Nothing else makes any sense.
>
> There is value in recording where the doc block starts, but that's not a
> task for the *section* info.
>
> I don't think I understand your feedback.
This was before my vacation, and my memory is foggy, ... I may have
gotten confused back then. Let me have a fresh look now.
self.info gets initialized in Section.__init__() to whatever info it
gets passed.
Your patch makes .ensure_untagged_section() overwrite this Section.info
when it extends an untagged section that is still empty. Hmmm. I'd
prefer .info to remain constant after initialization.
I figure this overwrite can happen only when extenting the body section
QAPIDoc.__init__() creates. In that case, it adjusts .info from
beginning of doc comment to first non-blank line.
Thoughts?
>> The alternative to changing .info's meaning is to add another member
>> with the meaning you need. Then we have to review .info's uses to find
>> out which ones to switch to the new one.
>
>
>> Left for later.
>>
>>
>> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>> index 8cdd5334ec..abeae1ca77 100644
>> --- a/scripts/qapi/parser.py
>> +++ b/scripts/qapi/parser.py
>> @@ -663,7 +663,10 @@ def end(self) -> None:
>> def ensure_untagged_section(self, info: QAPISourceInfo) -> None:
>> if self.all_sections and not self.all_sections[-1].tag:
>> # extend current section
>> -self.all_sections[-1].text += '\n'
>> +section = self.all_sections[-1]
>> +if not section.text:
>> +section.info = info
>> +section.text += '\n'
>> return
>> # start new section
>> section = self.Section(info)
>>
>>
Re: [PATCH 5/6] migration: Rephrase message on failure to save / load Xen device state
Peter Xu writes: > On Mon, May 13, 2024 at 04:17:02PM +0200, Markus Armbruster wrote: >> Functions that use an Error **errp parameter to return errors should >> not also report them to the user, because reporting is the caller's >> job. When the caller does, the error is reported twice. When it >> doesn't (because it recovered from the error), there is no error to >> report, i.e. the report is bogus. >> >> qmp_xen_save_devices_state() and qmp_xen_load_devices_state() violate >> this principle: they call qemu_save_device_state() and >> qemu_loadvm_state(), which call error_report_err(). >> >> I wish I could clean this up now, but migration's error reporting is >> too complicated (confused?) for me to mess with it. > > :-( If I understood how it's *supposed* to work, I might have a chance... I can see a mixture of reporting errors directly (with error_report() & friends), passing them to callers (via Error **errp), and storing them in / retrieving them from MigrationState member @error. This can't be right. I think a necessary first step towards getting it right is a shared understanding how errors are to be handled in migration code. This includes how error data should flow from error source to error sink, and what the possible sinks are. >> Instead, I'm merely improving the error reported by >> qmp_xen_load_devices_state() and qmp_xen_load_devices_state() to the >> QMP core from >> >> An IO error has occurred >> >> to >> saving Xen device state failed >> >> and >> >> loading Xen device state failed >> >> respectively. >> >> Signed-off-by: Markus Armbruster > > Acked-by: Peter Xu Thanks!
Re: [PATCH 4/6] cpus: Improve error messages on memsave, pmemsave write error
Philippe Mathieu-Daudé writes:
> On 13/5/24 16:45, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé writes:
>>
>>> On 13/5/24 16:17, Markus Armbruster wrote:
>>>> qmp_memsave() and qmp_pmemsave() report fwrite() error as
>>>>
>>>> An IO error has occurred
>>>>
>>>> Improve this to
>>>>
>>>> writing memory to '' failed
>>>>
>>>> Signed-off-by: Markus Armbruster
>>>> ---
>>>>system/cpus.c | 6 --
>>>>1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/system/cpus.c b/system/cpus.c
>>>> index 68d161d96b..f8fa78f33d 100644
>>>> --- a/system/cpus.c
>>>> +++ b/system/cpus.c
>>>> @@ -813,7 +813,8 @@ void qmp_memsave(int64_t addr, int64_t size, const
>>>> char *filename,
>>>> goto exit;
>>>> }
>>>> if (fwrite(buf, 1, l, f) != l) {
>>>> -error_setg(errp, QERR_IO_ERROR);
>>>> +error_setg(errp, "writing memory to '%s' failed",
>>>> + filename);
>>>> goto exit;
>>>> }
>>>> addr += l;
>>>> @@ -843,7 +844,8 @@ void qmp_pmemsave(int64_t addr, int64_t size, const
>>>> char *filename,
>>>> l = size;
>>>> cpu_physical_memory_read(addr, buf, l);
>>>> if (fwrite(buf, 1, l, f) != l) {
>>>> -error_setg(errp, QERR_IO_ERROR);
>>>> +error_setg(errp, "writing memory to '%s' failed",
>>>> + filename);
>>>
>>> What about including errno with error_setg_errno()?
>>
>> Sure fwrite() fails with errno reliably set? The manual page doesn't
>> mention it...
>
> Indeed. I can see some uses in the code base:
>
> qemu-io-cmds.c:409:if (ferror(f)) {
> qemu-io-cmds.c-410-perror(file_name);
This is after fread(), which isn't specified to set errno, either.
> qga/commands-posix.c-632-write_count = fwrite(buf, 1, count, fh);
> qga/commands-posix.c:633:if (ferror(fh)) {
> qga/commands-posix.c-634-error_setg_errno(errp, errno, "failed to
> write to file");
This one is after fwrite(), like the code I'm changing.
> util/qemu-config.c:152:if (ferror(fp)) {
> util/qemu-config.c-153-loc_pop();
> util/qemu-config.c-154-error_setg_errno(errp, errno, "Cannot read
> config file");
This is after fgets(), which isn't specified to set errno, either.
All three uses feel iffy to me. They work if the stream's error
indicator is clear before fread() / fwrite() / fgets(), and it is set
there, and the reason for it being set is something that sets errno
(such as a failed system call, which seems likely), and errno remains
untouched until after ferror(). Too much "if", "seems likely" for my
taste.
> Regardless,
>
> Reviewed-by: Philippe Mathieu-Daudé
Thanks!
Re: [PATCH 08/20] qapi/parser: differentiate intro and outro paragraphs
John Snow writes:
> Add a semantic tag to paragraphs that appear *before* tagged
> sections/members/features and those that appear after. This will control
> how they are inlined when doc sections are merged and flattened.
This future use is not obvious to me now. I guess the effective way to
help me see it is actual patches, which will come in due time.
> Signed-off-by: John Snow
> ---
> scripts/qapi/parser.py | 22 +-
> 1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index cf4cbca1c1f..b1794f71e12 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -503,6 +503,10 @@ def get_doc(self) -> 'QAPIDoc':
> self.accept(False)
> line = self.get_doc_line()
> no_more_args = False
> +# Paragraphs before members/features/tagged are "intro"
> paragraphs.
> +# Any appearing subsequently are "outro" paragraphs.
> +# This is only semantic metadata for the doc generator.
Not sure about the last sentence. Isn't it true for almost everything
around here?
Also, long line.
> +intro = True
>
> while line is not None:
> # Blank lines
> @@ -532,6 +536,7 @@ def get_doc(self) -> 'QAPIDoc':
> raise QAPIParseError(
> self, 'feature descriptions expected')
> no_more_args = True
> +intro = False
After feature descriptions.
> elif match := self._match_at_name_colon(line):
> # description
> if no_more_args:
> @@ -547,6 +552,7 @@ def get_doc(self) -> 'QAPIDoc':
> doc.append_line(text)
> line = self.get_doc_indented(doc)
> no_more_args = True
> +intro = False
Or after member descriptions.
> elif match := re.match(
> r'(Returns|Errors|Since|Notes?|Examples?|TODO): *',
> line):
> @@ -557,13 +563,14 @@ def get_doc(self) -> 'QAPIDoc':
> doc.append_line(text)
> line = self.get_doc_indented(doc)
> no_more_args = True
> +intro = False
Or after the first tagged section.
Okay, it does what it says on the tin.
> elif line.startswith('='):
> raise QAPIParseError(
> self,
> "unexpected '=' markup in definition documentation")
> else:
> # tag-less paragraph
> -doc.ensure_untagged_section(self.info)
> +doc.ensure_untagged_section(self.info, intro)
> doc.append_line(line)
> line = self.get_doc_paragraph(doc)
> else:
> @@ -617,7 +624,7 @@ def __init__(
> self,
> info: QAPISourceInfo,
> tag: Optional[str] = None,
> -kind: str = 'paragraph',
> +kind: str = 'intro-paragraph',
The question "why is this optional?" crossed my mind when reviewing the
previous patch. I left it unasked, because I felt challenging the
overlap between @kind and @tag was more useful. However, the new
default value 'intro-paragraph' feels more arbitrary to me than the old
one 'paragraph', and that makes the question pop right back into my
mind.
Unless I'm mistaken, all calls but one @tag and @kind. Making that one
pass it too feels simpler to me.
Moot if we fuse @tag and @kind, of course.
> ):
> # section source info, i.e. where it begins
> self.info = info
> @@ -625,7 +632,7 @@ def __init__(
> self.tag = tag
> # section text without tag
> self.text = ''
> -# section type - {paragraph, feature, member, tagged}
> +# section type - {-paragraph, feature, member,
> tagged}
Long line.
> self.kind = kind
>
> def append_line(self, line: str) -> None:
> @@ -666,7 +673,11 @@ def end(self) -> None:
> raise QAPISemError(
> section.info, "text required after '%s:'" % section.tag)
>
> -def ensure_untagged_section(self, info: QAPISourceInfo) -> None:
> +def ensure_untagged_section(
> +self,
> +info: QAPISourceInfo,
> +intro: bool = True,
Two callers, one passes @info, one doesn't. Passing it always might be
simpler.
> +) -> None:
> if self.all_sections and not self.all_sections[-1].tag:
> section = self.all_sections[-1]
> # Section is empty so far; update info to start *here*.
> @@ -677,7 +688,8 @@ def ensure_untagged_section(self, info: QAPISourceInfo)
> -> None:
> self.all_sections[-1].text += '\n'
> return
Re: [PATCH 07/20] qapi/parser: add semantic 'kind' parameter to QAPIDoc.Section
John Snow writes:
> When iterating all_sections, this is helpful to be able to distinguish
> "members" from "features"; the only other way to do so is to
> cross-reference these sections against QAPIDoc.args or QAPIDoc.features,
> but if the desired end goal for QAPIDoc is to remove everything except
> all_sections, we need *something* accessible to distinguish them.
>
> To keep types simple, add this semantic parameter to the base Section
> and not just ArgSection; we can use this to filter out paragraphs and
> tagged sections, too.
>
> Signed-off-by: John Snow
> ---
> scripts/qapi/parser.py | 25 -
> 1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 161768b8b96..cf4cbca1c1f 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -613,21 +613,27 @@ class QAPIDoc:
>
> class Section:
> # pylint: disable=too-few-public-methods
> -def __init__(self, info: QAPISourceInfo,
> - tag: Optional[str] = None):
> +def __init__(
> +self,
> +info: QAPISourceInfo,
> +tag: Optional[str] = None,
> +kind: str = 'paragraph',
> +):
> # section source info, i.e. where it begins
> self.info = info
> # section tag, if any ('Returns', '@name', ...)
> self.tag = tag
> # section text without tag
> self.text = ''
> +# section type - {paragraph, feature, member, tagged}
> +self.kind = kind
Hmm. .kind is almost redundant with .tag.
Untagged section:.kind is 'paragraph', .tag is None
Member description: .kind is 'member', .tag matches @NAME
Feature description: .kind is 'feature', .tag matches @NAME
Tagged section: .kind is 'tagged', .tag matches
r'Returns|Errors|Since|Notes?|Examples?|TODO'
.kind can directly be derived from .tag except for member and feature
descriptions. And you want to tell these two apart in a straightforward
manner in later patches, as you explain in your commit message.
If .kind is 'member' or 'feature', then self must be an ArgSection.
Worth a comment? An assertion?
Some time back, I considered changing .tag for member and feature
descriptions to suitable strings, like your 'member' and 'feature', and
move the member / feature name into ArgSection. I didn't, because the
benefit wasn't worth the churn at the time. Perhaps it's worth it now.
Would it result in simpler code than your solution?
Terminology nit: the section you call 'paragraph' isn't actually a
paragraph: it could be several paragraphs. Best to call it 'untagged',
as in .ensure_untagged_section().
>
> def append_line(self, line: str) -> None:
> self.text += line + '\n'
>
> class ArgSection(Section):
> -def __init__(self, info: QAPISourceInfo, tag: str):
> -super().__init__(info, tag)
> +def __init__(self, info: QAPISourceInfo, tag: str, kind: str):
> +super().__init__(info, tag, kind)
> self.member: Optional['QAPISchemaMember'] = None
>
> def connect(self, member: 'QAPISchemaMember') -> None:
[...]
Re: [PATCH 06/20] qapi/parser: fix comment parsing immediately following a doc block
John Snow writes: > If a comment immediately follows a doc block, the parser doesn't ignore > that token appropriately. Fix that. Reproducer? > > Signed-off-by: John Snow > --- > scripts/qapi/parser.py | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py > index 41b9319e5cb..161768b8b96 100644 > --- a/scripts/qapi/parser.py > +++ b/scripts/qapi/parser.py > @@ -587,7 +587,7 @@ def get_doc(self) -> 'QAPIDoc': > line = self.get_doc_line() > first = False > > -self.accept(False) > +self.accept() > doc.end() > return doc Can't judge the fix without understanding the problem, and the problem will be easier to understand for me with a reproducer.
Re: [PATCH 05/20] qapi/parser: adjust info location for doc body section
John Snow writes:
> Instead of using the info object for the doc block as a whole, update
> the info pointer for each call to ensure_untagged_section when the
> existing section is otherwise empty. This way, Sphinx error information
> will match precisely to where the text actually starts.
>
> Signed-off-by: John Snow
> ---
> scripts/qapi/parser.py | 9 +++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 8cdd5334ec6..41b9319e5cb 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -662,8 +662,13 @@ def end(self) -> None:
>
> def ensure_untagged_section(self, info: QAPISourceInfo) -> None:
> if self.all_sections and not self.all_sections[-1].tag:
> -# extend current section
> -self.all_sections[-1].text += '\n'
Before, we always append a newline.
> +section = self.all_sections[-1]
> +# Section is empty so far; update info to start *here*.
> +if not section.text:
> +section.info = info
> +else:
> +# extend current section
> +self.all_sections[-1].text += '\n'
Afterwards, we append it only when the section already has some text.
The commit message claims the patch only adjusts section.info. That's a
lie :)
I believe the change makes no difference because .end() strips leading
and trailing newline.
> return
> # start new section
> section = self.Section(info)
You could fix the commit message, but I think backing out the
no-difference change is easier. The appended patch works in my testing.
Next one. Your patch changes the meaning of section.info. Here's its
initialization:
class Section:
# pylint: disable=too-few-public-methods
def __init__(self, info: QAPISourceInfo,
tag: Optional[str] = None):
---># section source info, i.e. where it begins
self.info = info
# section tag, if any ('Returns', '@name', ...)
self.tag = tag
# section text without tag
self.text = ''
The comment is now wrong. Calls for a thorough review of .info's uses.
The alternative to changing .info's meaning is to add another member
with the meaning you need. Then we have to review .info's uses to find
out which ones to switch to the new one.
Left for later.
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 8cdd5334ec..abeae1ca77 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -663,7 +663,10 @@ def end(self) -> None:
def ensure_untagged_section(self, info: QAPISourceInfo) -> None:
if self.all_sections and not self.all_sections[-1].tag:
# extend current section
-self.all_sections[-1].text += '\n'
+section = self.all_sections[-1]
+if not section.text:
+section.info = info
+section.text += '\n'
return
# start new section
section = self.Section(info)
Re: [PATCH 03/20] docs/qapidoc: delint a tiny portion of the module
John Snow writes: > On Wed, May 15, 2024 at 5:17 AM Markus Armbruster wrote: > >> John Snow writes: >> >> > In the coming patches, it's helpful to have a linting baseline. However, >> > there's no need to shuffle around the deck chairs too much, because most >> > of this code will be removed once the new qapidoc generator (the >> > "transmogrifier") is in place. >> > >> > To ease my pain: just turn off the black auto-formatter for most, but >> > not all, of qapidoc.py. This will help ensure that *new* code follows a >> > coding standard without bothering too much with cleaning up the existing >> > code. >> > >> > For manual checking for now, try "black --check qapidoc.py" from the >> > docs/sphinx directory. "pip install black" (without root permissions) if >> > you do not have it installed otherwise. >> > >> > Signed-off-by: John Snow >> > --- >> > docs/sphinx/qapidoc.py | 16 +--- >> > 1 file changed, 9 insertions(+), 7 deletions(-) >> > >> > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py >> > index f270b494f01..1655682d4c7 100644 >> > --- a/docs/sphinx/qapidoc.py >> > +++ b/docs/sphinx/qapidoc.py >> > @@ -28,28 +28,30 @@ >> > import re >> > >> > from docutils import nodes >> > +from docutils.parsers.rst import Directive, directives >> > from docutils.statemachine import ViewList >> > -from docutils.parsers.rst import directives, Directive >> > -from sphinx.errors import ExtensionError >> > -from sphinx.util.nodes import nested_parse_with_titles >> > -import sphinx >> > -from qapi.gen import QAPISchemaVisitor >> > from qapi.error import QAPIError, QAPISemError >> > +from qapi.gen import QAPISchemaVisitor >> > from qapi.schema import QAPISchema >> > >> > +import sphinx >> > +from sphinx.errors import ExtensionError >> > +from sphinx.util.nodes import nested_parse_with_titles >> > + >> >> Exchanges old pylint gripe >> >> docs/sphinx/qapidoc.py:45:4: C0412: Imports from package sphinx are >> not grouped (ungrouped-imports) >> >> for new gripes >> >> docs/sphinx/qapidoc.py:37:0: C0411: third party import "import sphinx" >> should be placed before "from qapi.error import QAPIError, QAPISemError" >> (wrong-import-order) >> docs/sphinx/qapidoc.py:38:0: C0411: third party import "from >> sphinx.errors import ExtensionError" should be placed before "from >> qapi.error import QAPIError, QAPISemError" (wrong-import-order) >> docs/sphinx/qapidoc.py:39:0: C0411: third party import "from >> sphinx.util.nodes import nested_parse_with_titles" should be placed before >> "from qapi.error import QAPIError, QAPISemError" (wrong-import-order) >> >> Easy enough to fix. >> > > I believe these errors are caused by the fact that the tools are confused > about the "sphinx" namespace - some interpret them as being the local > "module", the docs/sphinx/ directory, and others believe them to be the > third party external package. > > I have not been using pylint on docs/sphinx/ files because of the > difficulty of managing imports - this environment is generally beyond the > reaches of my python borgcube and at present I don't have plans to > integrate it. > > At the moment, I am using black, isort and flake8 for qapidoc.py and > they're happy with it. I am not using mypy because I never did the typing > boogaloo with qapidoc.py and I won't be bothering - except for any new code > I write, which *will* bother. By the end of the new transmogrifier, > qapidoc.py *will* strictly typecheck. > > pylint may prove to be an issue with the imports, though. isort also seems > to misunderstand "sphinx, the stuff in this folder" and "sphinx, the stuff > in a third party package" and so I'm afraid I don't have any good ability > to get pylint to play along, here. > > Pleading for "Sorry, this sucks and I can't figure out how to solve it > quickly". Maybe a future project, apologies. Is this pain we inflict on ourselves by naming the directory "sphinx"? >> > >> > # Sphinx up to 1.6 uses AutodocReporter; 1.7 and later >> > # use switch_source_input. Check borrowed from kerneldoc.py. >> > -Use_SSI = sphinx.__version__[:3] >= '1.7' >> > +Use_SSI = sphinx.__version__[:3] >= "1.7" >> > if Use_SSI: >> > from sphinx.util.docutils import switch_source_input >> > else: >> > from sphinx.ext.autodoc import AutodocReporter >> > >> > >> > -__version__ = '1.0' >> > +__version__ = "1.0" >> > >> > >> > +# fmt: off >> >> I figure this tells black to keep quiet for the remainder of the file. >> Worth a comment, I think. >> >> > # Function borrowed from pydash, which is under the MIT license >> > def intersperse(iterable, separator): >> > """Yield the members of *iterable* interspersed with *separator*.""" >> >> With my comments addressed >> Reviewed-by: Markus Armbruster >> > > ^ Dropping this unless you're okay with the weird import orders owing to > the strange import paradigm in the sphinx folder.r Feel free to keep it.
Re: [PATCH 04/20] qapi/parser: preserve indentation in QAPIDoc sections
John Snow writes: > On Wed, May 15, 2024, 7:50 AM Markus Armbruster wrote: > >> John Snow writes: >> >> > Prior to this patch, a section like this: >> > >> > @name: lorem ipsum >> >dolor sit amet >> > consectetur adipiscing elit >> > >> > would be parsed as: >> > >> > "lorem ipsum\ndolor sit amet\n consectetur adipiscing elit" >> > >> > We want to preserve the indentation for even the first body line so that >> > the entire block can be parsed directly as rST. This patch would now >> > parse that segment as: >> > >> > "lorem ipsum\n dolor sit amet\n consectetur adipiscing elit" >> >> I'm afraid it's less than clear *why* we want to parse the entire block >> directly as rST. I have just enough insight into what you've built on >> top of this series to hazard a guess. Bear with me while I try to >> explain it. >> > > My own summary: qapidoc expects a paragraph, the new generator expects a > block. > > >> We first parse the doc comment with parser.py into an internal >> representation. The structural parts become objects, and the remainder >> becomes text attributes of these objects. Currently, parser.py >> carefully adjusts indentation for these text attributes. Why? I'll get >> to that. >> >> For your example, parser.py creates an ArgSection object, and sets its >> @text member to >> >> "lorem ipsum\ndolor sit amet\n consectetur adipiscing elit" >> >> Printing this string gives us >> >> lorem ipsum >> dolor sit amet >> consectetur adipiscing elit >> >> qapidoc.py then transforms parser.py's IR into Sphinx IR. The objects >> become (more complicated) Sphinx objects, and their text attributes get >> re-parsed as rST text into still more Sphinx objects. >> >> This re-parse rejects your example with "Unexpected indentation." >> > > Specifically, it'd be an unexpected *unindent*; the indent lacking on the > first *two* lines is the problem. > > >> Let me use a slightly different one: >> >> # @name: lorem ipsum >> #dolor sit amet >> #consectetur adipiscing elit >> >> Results in this @text member >> >> lorem ipsum >> dolor sit amet >> consectetur adipiscing elit >> >> which is re-parsed as paragraph, i.e. exactly what we want. >> > > It's what we used to want, anyway. Yes, I'm describing the current state here. >> > This understandably breaks qapidoc.py; >> >> Without indentation adjustment, we'd get >> >> lorem ipsum >>dolor sit amet >>consectetur adipiscing elit >> >> which would be re-parsed as a definition list, I guess. This isn't what >> we want. >> >> >so a new function is added there >> > to re-dedent the text. >> >> Your patch moves the indentation adjustment to another place. No >> functional change. >> >> You move it so you can branch off your new rendering pipeline before the >> indentation adjustment, because ... >> >> >Once the new generator is merged, this function >> > will not be needed any longer and can be dropped. >> >> ... yours doesn't want it. >> >> I believe it doesn't want it, because it generates rST (with a QAPI >> extension) instead of Sphinx objects. For your example, something like >> >> :arg name: lorem ipsum >>dolor sit amet >> consectetur adipiscing elit >> >> For mine: >> >> :arg name: lorem ipsum >>dolor sit amet >>consectetur adipiscing elit >> >> Fair? >> > > Not quite; > > Old parsing, new generator: > > :arg type name: lorem ipsum > dolor sit amet > consectetur apidiscing elit > > This is wrong - continuations of a field list must be indented. Unlike > paragraphs, we want indents to "keep the block". > > New parsing, new generator: > > :arg type name: lorem ipsum >dolor sit amet > consectetur apidiscing elit > > indent is preserved, maintaining the block-level element. > > I don't have to re-add indents and any nested block elements will be > preserved correctly. i.e. you can use code examples, nested lists, etc. in > argument definitions. > > The goal here was "Do not treat this as a paragraph, treat it directly as > rST and do not modify i
Re: [PATCH 04/20] qapi/parser: preserve indentation in QAPIDoc sections
John Snow writes:
> Prior to this patch, a section like this:
>
> @name: lorem ipsum
>dolor sit amet
> consectetur adipiscing elit
>
> would be parsed as:
>
> "lorem ipsum\ndolor sit amet\n consectetur adipiscing elit"
>
> We want to preserve the indentation for even the first body line so that
> the entire block can be parsed directly as rST. This patch would now
> parse that segment as:
>
> "lorem ipsum\n dolor sit amet\n consectetur adipiscing elit"
I'm afraid it's less than clear *why* we want to parse the entire block
directly as rST. I have just enough insight into what you've built on
top of this series to hazard a guess. Bear with me while I try to
explain it.
We first parse the doc comment with parser.py into an internal
representation. The structural parts become objects, and the remainder
becomes text attributes of these objects. Currently, parser.py
carefully adjusts indentation for these text attributes. Why? I'll get
to that.
For your example, parser.py creates an ArgSection object, and sets its
@text member to
"lorem ipsum\ndolor sit amet\n consectetur adipiscing elit"
Printing this string gives us
lorem ipsum
dolor sit amet
consectetur adipiscing elit
qapidoc.py then transforms parser.py's IR into Sphinx IR. The objects
become (more complicated) Sphinx objects, and their text attributes get
re-parsed as rST text into still more Sphinx objects.
This re-parse rejects your example with "Unexpected indentation."
Let me use a slightly different one:
# @name: lorem ipsum
#dolor sit amet
#consectetur adipiscing elit
Results in this @text member
lorem ipsum
dolor sit amet
consectetur adipiscing elit
which is re-parsed as paragraph, i.e. exactly what we want.
> This understandably breaks qapidoc.py;
Without indentation adjustment, we'd get
lorem ipsum
dolor sit amet
consectetur adipiscing elit
which would be re-parsed as a definition list, I guess. This isn't what
we want.
>so a new function is added there
> to re-dedent the text.
Your patch moves the indentation adjustment to another place. No
functional change.
You move it so you can branch off your new rendering pipeline before the
indentation adjustment, because ...
>Once the new generator is merged, this function
> will not be needed any longer and can be dropped.
... yours doesn't want it.
I believe it doesn't want it, because it generates rST (with a QAPI
extension) instead of Sphinx objects. For your example, something like
:arg name: lorem ipsum
dolor sit amet
consectetur adipiscing elit
For mine:
:arg name: lorem ipsum
dolor sit amet
consectetur adipiscing elit
Fair?
The transition from the doc comment to (extended) rST is straightforward
for these examples.
I hope it'll be as straightforward (and thus predictable) in other
cases, too.
> (I verified this patch changes absolutely nothing by comparing the
> md5sums of the QMP ref html pages both before and after the change, so
> it's certified inert. QAPI test output has been updated to reflect the
> new strategy of preserving indents for rST.)
>
> Signed-off-by: John Snow
> ---
> docs/sphinx/qapidoc.py | 36 +-
> scripts/qapi/parser.py | 8 ++--
> tests/qapi-schema/doc-good.out | 32 +++---
> 3 files changed, 53 insertions(+), 23 deletions(-)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index 1655682d4c7..2e3ffcbafb7 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -26,6 +26,7 @@
>
> import os
> import re
> +import textwrap
>
> from docutils import nodes
> from docutils.parsers.rst import Directive, directives
> @@ -51,6 +52,28 @@
> __version__ = "1.0"
>
>
> +def dedent(text: str) -> str:
> +# Temporary: In service of the new QAPI domain, the QAPI doc parser
> +# now preserves indents in args/members/features text. QAPIDoc does
> +# not handle this well, so undo that change here.
> +
> +# QAPIDoc is being rewritten and will be replaced soon,
> +# but this function is here in the interim as transition glue.
I'm not sure we need the comment.
> +
> +lines = text.splitlines(True)
> +if len(lines) > 1:
> +if re.match(r"\s+", lines[0]):
> +# First line is indented; description started on
> +# the line after the name. dedent the whole block.
> +return textwrap.dedent(text)
> +else:
pylint gripes
docs/sphinx/qapidoc.py:65:8: R1705: Unnecessary "else" after "return",
remove the "else" and de-indent the code inside it (no-else-return)
> +# Descr started on same line. Dedent line 2+.
> +return lines[0] + textwrap.dedent("".join(lines[1:]))
> +else:
> +# Descr was a single line; dedent entire line.
> +return
Re: [PATCH 03/20] docs/qapidoc: delint a tiny portion of the module
John Snow writes: > In the coming patches, it's helpful to have a linting baseline. However, > there's no need to shuffle around the deck chairs too much, because most > of this code will be removed once the new qapidoc generator (the > "transmogrifier") is in place. > > To ease my pain: just turn off the black auto-formatter for most, but > not all, of qapidoc.py. This will help ensure that *new* code follows a > coding standard without bothering too much with cleaning up the existing > code. > > For manual checking for now, try "black --check qapidoc.py" from the > docs/sphinx directory. "pip install black" (without root permissions) if > you do not have it installed otherwise. > > Signed-off-by: John Snow > --- > docs/sphinx/qapidoc.py | 16 +--- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py > index f270b494f01..1655682d4c7 100644 > --- a/docs/sphinx/qapidoc.py > +++ b/docs/sphinx/qapidoc.py > @@ -28,28 +28,30 @@ > import re > > from docutils import nodes > +from docutils.parsers.rst import Directive, directives > from docutils.statemachine import ViewList > -from docutils.parsers.rst import directives, Directive > -from sphinx.errors import ExtensionError > -from sphinx.util.nodes import nested_parse_with_titles > -import sphinx > -from qapi.gen import QAPISchemaVisitor > from qapi.error import QAPIError, QAPISemError > +from qapi.gen import QAPISchemaVisitor > from qapi.schema import QAPISchema > > +import sphinx > +from sphinx.errors import ExtensionError > +from sphinx.util.nodes import nested_parse_with_titles > + Exchanges old pylint gripe docs/sphinx/qapidoc.py:45:4: C0412: Imports from package sphinx are not grouped (ungrouped-imports) for new gripes docs/sphinx/qapidoc.py:37:0: C0411: third party import "import sphinx" should be placed before "from qapi.error import QAPIError, QAPISemError" (wrong-import-order) docs/sphinx/qapidoc.py:38:0: C0411: third party import "from sphinx.errors import ExtensionError" should be placed before "from qapi.error import QAPIError, QAPISemError" (wrong-import-order) docs/sphinx/qapidoc.py:39:0: C0411: third party import "from sphinx.util.nodes import nested_parse_with_titles" should be placed before "from qapi.error import QAPIError, QAPISemError" (wrong-import-order) Easy enough to fix. > > # Sphinx up to 1.6 uses AutodocReporter; 1.7 and later > # use switch_source_input. Check borrowed from kerneldoc.py. > -Use_SSI = sphinx.__version__[:3] >= '1.7' > +Use_SSI = sphinx.__version__[:3] >= "1.7" > if Use_SSI: > from sphinx.util.docutils import switch_source_input > else: > from sphinx.ext.autodoc import AutodocReporter > > > -__version__ = '1.0' > +__version__ = "1.0" > > > +# fmt: off I figure this tells black to keep quiet for the remainder of the file. Worth a comment, I think. > # Function borrowed from pydash, which is under the MIT license > def intersperse(iterable, separator): > """Yield the members of *iterable* interspersed with *separator*.""" With my comments addressed Reviewed-by: Markus Armbruster
Re: [PATCH 02/20] qapi: linter fixups
John Snow writes:
> Fix minor irritants to pylint/flake8 et al.
>
> (Yes, these need to be guarded by the Python tests. That's a work in
> progress, a series that's quite likely to follow once I finish this
> Sphinx project. Please pardon the temporary irritation.)
No worries; one step at a time.
> Signed-off-by: John Snow
> ---
> scripts/qapi/introspect.py | 8
> scripts/qapi/schema.py | 6 +++---
> scripts/qapi/visit.py | 5 +++--
> 3 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 86c075a6ad2..ac14b20f308 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -27,8 +27,8 @@
> from .schema import (
> QAPISchema,
> QAPISchemaAlternatives,
> -QAPISchemaBranches,
> QAPISchemaArrayType,
> +QAPISchemaBranches,
> QAPISchemaBuiltinType,
> QAPISchemaEntity,
> QAPISchemaEnumMember,
> @@ -233,9 +233,9 @@ def _use_type(self, typ: QAPISchemaType) -> str:
> typ = type_int
> elif (isinstance(typ, QAPISchemaArrayType) and
>typ.element_type.json_type() == 'int'):
> -type_intList = self._schema.lookup_type('intList')
> -assert type_intList
> -typ = type_intList
> +type_intlist = self._schema.lookup_type('intList')
> +assert type_intlist
> +typ = type_intlist
I named the variable after the type. Suppressing the linter complaint
just to keep that name isn't worthwhile. I might have picked
type_int_list instead. No need to change it now.
> # Add type to work queue if new
> if typ not in self._used_types:
> self._used_types.append(typ)
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 721c470d2b8..d65c35f6ee6 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -730,6 +730,7 @@ def set_defined_in(self, name: str) -> None:
> for v in self.variants:
> v.set_defined_in(name)
>
> +# pylint: disable=unused-argument
> def check(
> self, schema: QAPISchema, seen: Dict[str, QAPISchemaMember]
> ) -> None:
> @@ -1166,7 +1167,7 @@ def _def_definition(self, defn: QAPISchemaDefinition)
> -> None:
> defn.info, "%s is already defined" % other_defn.describe())
> self._entity_dict[defn.name] = defn
>
> -def lookup_entity(self,name: str) -> Optional[QAPISchemaEntity]:
> +def lookup_entity(self, name: str) -> Optional[QAPISchemaEntity]:
> return self._entity_dict.get(name)
>
> def lookup_type(self, name: str) -> Optional[QAPISchemaType]:
> @@ -1302,11 +1303,10 @@ def _make_implicit_object_type(
> name = 'q_obj_%s-%s' % (name, role)
> typ = self.lookup_entity(name)
> if typ:
> -assert(isinstance(typ, QAPISchemaObjectType))
> +assert isinstance(typ, QAPISchemaObjectType)
> # The implicit object type has multiple users. This can
> # only be a duplicate definition, which will be flagged
> # later.
> -pass
> else:
> self._def_definition(QAPISchemaObjectType(
> name, info, None, ifcond, None, None, members, None))
> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> index e766acaac92..12f92e429f6 100644
> --- a/scripts/qapi/visit.py
> +++ b/scripts/qapi/visit.py
> @@ -280,8 +280,9 @@ def gen_visit_alternate(name: str,
> abort();
> default:
> assert(visit_is_input(v));
> -error_setg(errp, "Invalid parameter type for '%%s', expected:
> %(name)s",
> - name ? name : "null");
> +error_setg(errp,
> + "Invalid parameter type for '%%s', expected: %(name)s",
> + name ? name : "null");
> /* Avoid passing invalid *obj to qapi_free_%(c_name)s() */
> g_free(*obj);
> *obj = NULL;
This is mostly lint I neglected to pick off in my recent work. Thanks
for taking care of it!
Reviewed-by: Markus Armbruster
Re: [PATCH 1/2] copy-before-write: allow specifying minimum cluster size
Fiona Ebner writes:
> Am 26.03.24 um 10:06 schrieb Markus Armbruster:
>>> @@ -365,7 +368,13 @@ BlockCopyState *block_copy_state_new(BdrvChild
>>> *source, BdrvChild *target,
>>>
>>> GLOBAL_STATE_CODE();
>>>
>>> -cluster_size = block_copy_calculate_cluster_size(target->bs, errp);
>>> +if (min_cluster_size && !is_power_of_2(min_cluster_size)) {
>>
>> min_cluster_size is int64_t, is_power_of_2() takes uint64_t. Bad if
>> min_cluster_size is negative. Could this happen?
>
> No, because it comes in as a uint32_t via the QAPI (the internal caller
> added by patch 2/2 from the backup code also gets the value via QAPI and
> there uint32_t is used too).
Good.
Is there a practical way to tweak the types so it's more obvious?
>>> +error_setg(errp, "min-cluster-size needs to be a power of 2");
>>> +return NULL;
>>> +}
>>> +
>>> +cluster_size = block_copy_calculate_cluster_size(target->bs,
>>> + min_cluster_size,
>>> errp);
>>> if (cluster_size < 0) {
>>> return NULL;
>>> }
>
> ---snip---
>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 0a72c590a8..85c8f88f6e 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -4625,12 +4625,18 @@
>>> # @on-cbw-error parameter will decide how this failure is handled.
>>> # Default 0. (Since 7.1)
>>> #
>>> +# @min-cluster-size: Minimum size of blocks used by copy-before-write
>>> +# operations. Has to be a power of 2. No effect if smaller than
>>> +# the maximum of the target's cluster size and 64 KiB. Default 0.
>>> +# (Since 9.0)
>>> +#
>>> # Since: 6.2
>>> ##
>>> { 'struct': 'BlockdevOptionsCbw',
>>>'base': 'BlockdevOptionsGenericFormat',
>>>'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap',
>>> -'*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32' } }
>>> +'*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32',
>>> +'*min-cluster-size': 'uint32' } }
>>
>> Elsewhere in the schema, we use either 'int' or 'size' for cluster-size.
>> Why the difference?
>
> The motivation was to disallow negative values up front and have it work
> with block_copy_calculate_cluster_size(), whose result is an int64_t.
Let's see whether I understand.
cbw_open() passes the new member @min-cluster-size to
block_copy_state_new().
block_copy_state_new() checks it, and passes it on to
block_copy_calculate_cluster_size(). This is the C code shown above.
block_copy_calculate_cluster_size() uses it like
return MAX(min_cluster_size, BLOCK_COPY_CLUSTER_SIZE_DEFAULT);
and
return MAX(min_cluster_size,
MAX(BLOCK_COPY_CLUSTER_SIZE_DEFAULT, bdi.cluster_size));
BLOCK_COPY_CLUSTER_SIZE_DEFAULT and bdi.cluster_size are int. The
return type is int64_t.
Correct?
I don't like mixing signed and unsigned in MAX() like this. Figuring
out whether it's safe takes a C language lawyer. I'd rather avoid such
subtle code. Can we please compute these maxima entirely in the
destination type int64_t?
> If
> I go with 'int', I'll have to add a check to disallow negative values.
> If I go with 'size', I'll have to add a check for to disallow too large
> values.
>
> Which approach should I go with?
For me, reducing the need for manual checking is important, but
cleanliness of the external interface trumps it. I'd use 'size'.
Not the first time I see friction between QAPI 'size' or 'uint64' and
the block layer's use of int64_t.
The block layer likes to use int64_t even when the value must not be
negative. There are good reasons for that.
Perhaps a QAPI type for "non-negative int64_t" could be useful. Weird,
but useful.
Re: [PATCH 2/6] dump/win_dump: Improve error messages on write error
Philippe Mathieu-Daudé writes:
> On 13/5/24 16:16, Markus Armbruster wrote:
>> create_win_dump() and write_run report qemu_write_full() failure to
>> their callers as
>> An IO error has occurred
>> The errno set by qemu_write_full() is lost.
>> Improve this to
>> win-dump: failed to write header:
>> and
>> win-dump: failed to save memory:
>> This matches how dump.c reports similar errors.
>> Signed-off-by: Markus Armbruster
>> ---
>> dump/win_dump.c | 7 ---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>> diff --git a/dump/win_dump.c b/dump/win_dump.c
>> index b7bfaff379..0e4fe692ce 100644
>> --- a/dump/win_dump.c
>> +++ b/dump/win_dump.c
>> @@ -12,7 +12,6 @@
>> #include "sysemu/dump.h"
>> #include "qapi/error.h"
>> #include "qemu/error-report.h"
>> -#include "qapi/qmp/qerror.h"
>> #include "exec/cpu-defs.h"
>> #include "hw/core/cpu.h"
>> #include "qemu/win_dump_defs.h"
>> @@ -52,6 +51,7 @@ static size_t write_run(uint64_t base_page, uint64_t
>> page_count,
>> uint64_t addr = base_page << TARGET_PAGE_BITS;
>> uint64_t size = page_count << TARGET_PAGE_BITS;
>> uint64_t len, l;
>> +int eno;
>> size_t total = 0;
>> while (size) {
>> @@ -65,9 +65,10 @@ static size_t write_run(uint64_t base_page, uint64_t
>> page_count,
>> }
>> l = qemu_write_full(fd, buf, len);
>> +eno = errno;
>
> Hmm this show the qemu_write_full() API isn't ideal.
> Maybe we could pass as argument and return errno.
> There are only 20 calls.
qemu_write_full() is a drop-in replacement for write().
>> cpu_physical_memory_unmap(buf, addr, false, len);
>> if (l != len) {
>> -error_setg(errp, QERR_IO_ERROR);
>> +error_setg_errno(errp, eno, "win-dump: failed to save memory");
>> return 0;
>> }
>> @@ -459,7 +460,7 @@ void create_win_dump(DumpState *s, Error **errp)
>> s->written_size = qemu_write_full(s->fd, h, hdr_size);
>> if (s->written_size != hdr_size) {
>> -error_setg(errp, QERR_IO_ERROR);
>> +error_setg_errno(errp, errno, "win-dump: failed to write header");
>> goto out_restore;
>> }
>>
Re: [PATCH 4/6] cpus: Improve error messages on memsave, pmemsave write error
Philippe Mathieu-Daudé writes:
> On 13/5/24 16:17, Markus Armbruster wrote:
>> qmp_memsave() and qmp_pmemsave() report fwrite() error as
>> An IO error has occurred
>> Improve this to
>> writing memory to '' failed
>> Signed-off-by: Markus Armbruster
>> ---
>> system/cpus.c | 6 --
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>> diff --git a/system/cpus.c b/system/cpus.c
>> index 68d161d96b..f8fa78f33d 100644
>> --- a/system/cpus.c
>> +++ b/system/cpus.c
>> @@ -813,7 +813,8 @@ void qmp_memsave(int64_t addr, int64_t size, const char
>> *filename,
>> goto exit;
>> }
>> if (fwrite(buf, 1, l, f) != l) {
>> -error_setg(errp, QERR_IO_ERROR);
>> +error_setg(errp, "writing memory to '%s' failed",
>> + filename);
>> goto exit;
>> }
>> addr += l;
>> @@ -843,7 +844,8 @@ void qmp_pmemsave(int64_t addr, int64_t size, const char
>> *filename,
>> l = size;
>> cpu_physical_memory_read(addr, buf, l);
>> if (fwrite(buf, 1, l, f) != l) {
>> -error_setg(errp, QERR_IO_ERROR);
>> +error_setg(errp, "writing memory to '%s' failed",
>> + filename);
>
> What about including errno with error_setg_errno()?
Sure fwrite() fails with errno reliably set? The manual page doesn't
mention it...
>> goto exit;
>> }
>> addr += l;
[PATCH 4/6] cpus: Improve error messages on memsave, pmemsave write error
qmp_memsave() and qmp_pmemsave() report fwrite() error as
An IO error has occurred
Improve this to
writing memory to '' failed
Signed-off-by: Markus Armbruster
---
system/cpus.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/system/cpus.c b/system/cpus.c
index 68d161d96b..f8fa78f33d 100644
--- a/system/cpus.c
+++ b/system/cpus.c
@@ -813,7 +813,8 @@ void qmp_memsave(int64_t addr, int64_t size, const char
*filename,
goto exit;
}
if (fwrite(buf, 1, l, f) != l) {
-error_setg(errp, QERR_IO_ERROR);
+error_setg(errp, "writing memory to '%s' failed",
+ filename);
goto exit;
}
addr += l;
@@ -843,7 +844,8 @@ void qmp_pmemsave(int64_t addr, int64_t size, const char
*filename,
l = size;
cpu_physical_memory_read(addr, buf, l);
if (fwrite(buf, 1, l, f) != l) {
-error_setg(errp, QERR_IO_ERROR);
+error_setg(errp, "writing memory to '%s' failed",
+ filename);
goto exit;
}
addr += l;
--
2.45.0
[PATCH 5/6] migration: Rephrase message on failure to save / load Xen device state
Functions that use an Error **errp parameter to return errors should
not also report them to the user, because reporting is the caller's
job. When the caller does, the error is reported twice. When it
doesn't (because it recovered from the error), there is no error to
report, i.e. the report is bogus.
qmp_xen_save_devices_state() and qmp_xen_load_devices_state() violate
this principle: they call qemu_save_device_state() and
qemu_loadvm_state(), which call error_report_err().
I wish I could clean this up now, but migration's error reporting is
too complicated (confused?) for me to mess with it.
Instead, I'm merely improving the error reported by
qmp_xen_load_devices_state() and qmp_xen_load_devices_state() to the
QMP core from
An IO error has occurred
to
saving Xen device state failed
and
loading Xen device state failed
respectively.
Signed-off-by: Markus Armbruster
---
migration/savevm.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/migration/savevm.c b/migration/savevm.c
index 4509482ec4..a4a856982a 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -45,7 +45,6 @@
#include "qapi/qapi-commands-migration.h"
#include "qapi/clone-visitor.h"
#include "qapi/qapi-builtin-visit.h"
-#include "qapi/qmp/qerror.h"
#include "qemu/error-report.h"
#include "sysemu/cpus.h"
#include "exec/memory.h"
@@ -3208,7 +3207,7 @@ void qmp_xen_save_devices_state(const char *filename,
bool has_live, bool live,
object_unref(OBJECT(ioc));
ret = qemu_save_device_state(f);
if (ret < 0 || qemu_fclose(f) < 0) {
-error_setg(errp, QERR_IO_ERROR);
+error_setg(errp, "saving Xen device state failed");
} else {
/* libxl calls the QMP command "stop" before calling
* "xen-save-devices-state" and in case of migration failure, libxl
@@ -3257,7 +3256,7 @@ void qmp_xen_load_devices_state(const char *filename,
Error **errp)
ret = qemu_loadvm_state(f);
qemu_fclose(f);
if (ret < 0) {
-error_setg(errp, QERR_IO_ERROR);
+error_setg(errp, "loading Xen device state failed");
}
migration_incoming_state_destroy();
}
--
2.45.0
[PATCH 1/6] block: Improve error message when external snapshot can't flush
external_snapshot_action() reports bdrv_flush() failure to its caller
as
An IO error has occurred
The errno code returned by bdrv_flush() is lost.
Improve this to
Write to node '' failed:
Signed-off-by: Markus Armbruster
---
blockdev.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 08eccc9052..528db3452f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1406,8 +1406,10 @@ static void external_snapshot_action(TransactionAction
*action,
}
if (!bdrv_is_read_only(state->old_bs)) {
-if (bdrv_flush(state->old_bs)) {
-error_setg(errp, QERR_IO_ERROR);
+ret = bdrv_flush(state->old_bs);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Write to node '%s' failed",
+ bdrv_get_device_or_node_name(state->old_bs));
return;
}
}
--
2.45.0
[PATCH 0/6] error: Eliminate QERR_IO_ERROR
Markus Armbruster (6): block: Improve error message when external snapshot can't flush dump/win_dump: Improve error messages on write error block/vmdk: Improve error messages on extent write error cpus: Improve error messages on memsave, pmemsave write error migration: Rephrase message on failure to save / load Xen device state qerror: QERR_IO_ERROR is no longer used, drop include/qapi/qmp/qerror.h | 3 --- block/vmdk.c | 10 +- blockdev.c| 6 -- dump/win_dump.c | 7 --- migration/savevm.c| 5 ++--- system/cpus.c | 6 -- 6 files changed, 19 insertions(+), 18 deletions(-) -- 2.45.0
[PATCH 3/6] block/vmdk: Improve error messages on extent write error
vmdk_init_extent() reports blk_co_pwrite() failure to its caller as
An IO error has occurred
The errno code returned by blk_co_pwrite() is lost.
Improve this to
failed to write VMDK :
Signed-off-by: Markus Armbruster
---
block/vmdk.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 3b82979fdf..78f6433607 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -28,7 +28,6 @@
#include "block/block_int.h"
#include "sysemu/block-backend.h"
#include "qapi/qmp/qdict.h"
-#include "qapi/qmp/qerror.h"
#include "qemu/error-report.h"
#include "qemu/module.h"
#include "qemu/option.h"
@@ -2278,12 +2277,12 @@ vmdk_init_extent(BlockBackend *blk, int64_t filesize,
bool flat, bool compress,
/* write all the data */
ret = blk_co_pwrite(blk, 0, sizeof(magic), , 0);
if (ret < 0) {
-error_setg(errp, QERR_IO_ERROR);
+error_setg_errno(errp, -ret, "failed to write VMDK magic");
goto exit;
}
ret = blk_co_pwrite(blk, sizeof(magic), sizeof(header), , 0);
if (ret < 0) {
-error_setg(errp, QERR_IO_ERROR);
+error_setg_errno(errp, -ret, "failed to write VMDK header");
goto exit;
}
@@ -2303,7 +2302,7 @@ vmdk_init_extent(BlockBackend *blk, int64_t filesize,
bool flat, bool compress,
ret = blk_co_pwrite(blk, le64_to_cpu(header.rgd_offset) * BDRV_SECTOR_SIZE,
gd_buf_size, gd_buf, 0);
if (ret < 0) {
-error_setg(errp, QERR_IO_ERROR);
+error_setg_errno(errp, -ret, "failed to write VMDK grain directory");
goto exit;
}
@@ -2315,7 +2314,8 @@ vmdk_init_extent(BlockBackend *blk, int64_t filesize,
bool flat, bool compress,
ret = blk_co_pwrite(blk, le64_to_cpu(header.gd_offset) * BDRV_SECTOR_SIZE,
gd_buf_size, gd_buf, 0);
if (ret < 0) {
-error_setg(errp, QERR_IO_ERROR);
+error_setg_errno(errp, -ret,
+ "failed to write VMDK backup grain directory");
}
ret = 0;
--
2.45.0
[PATCH 6/6] qerror: QERR_IO_ERROR is no longer used, drop
Signed-off-by: Markus Armbruster --- include/qapi/qmp/qerror.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h index 00b18e9082..bc9116f76a 100644 --- a/include/qapi/qmp/qerror.h +++ b/include/qapi/qmp/qerror.h @@ -20,9 +20,6 @@ #define QERR_INVALID_PARAMETER_VALUE \ "Parameter '%s' expects %s" -#define QERR_IO_ERROR \ -"An IO error has occurred" - #define QERR_MISSING_PARAMETER \ "Parameter '%s' is missing" -- 2.45.0
[PATCH 2/6] dump/win_dump: Improve error messages on write error
create_win_dump() and write_run report qemu_write_full() failure to
their callers as
An IO error has occurred
The errno set by qemu_write_full() is lost.
Improve this to
win-dump: failed to write header:
and
win-dump: failed to save memory:
This matches how dump.c reports similar errors.
Signed-off-by: Markus Armbruster
---
dump/win_dump.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/dump/win_dump.c b/dump/win_dump.c
index b7bfaff379..0e4fe692ce 100644
--- a/dump/win_dump.c
+++ b/dump/win_dump.c
@@ -12,7 +12,6 @@
#include "sysemu/dump.h"
#include "qapi/error.h"
#include "qemu/error-report.h"
-#include "qapi/qmp/qerror.h"
#include "exec/cpu-defs.h"
#include "hw/core/cpu.h"
#include "qemu/win_dump_defs.h"
@@ -52,6 +51,7 @@ static size_t write_run(uint64_t base_page, uint64_t
page_count,
uint64_t addr = base_page << TARGET_PAGE_BITS;
uint64_t size = page_count << TARGET_PAGE_BITS;
uint64_t len, l;
+int eno;
size_t total = 0;
while (size) {
@@ -65,9 +65,10 @@ static size_t write_run(uint64_t base_page, uint64_t
page_count,
}
l = qemu_write_full(fd, buf, len);
+eno = errno;
cpu_physical_memory_unmap(buf, addr, false, len);
if (l != len) {
-error_setg(errp, QERR_IO_ERROR);
+error_setg_errno(errp, eno, "win-dump: failed to save memory");
return 0;
}
@@ -459,7 +460,7 @@ void create_win_dump(DumpState *s, Error **errp)
s->written_size = qemu_write_full(s->fd, h, hdr_size);
if (s->written_size != hdr_size) {
-error_setg(errp, QERR_IO_ERROR);
+error_setg_errno(errp, errno, "win-dump: failed to write header");
goto out_restore;
}
--
2.45.0
Re: [PATCH] hw/nvme: Add CLI options for PCI vendor/device IDs and IEEE-OUI ID
Saif Abrar writes:
> Add CLI options for user specified
> - PCI vendor, device, subsystem vendor and subsystem IDs
> - IEEE-OUI ID
>
> e.g. PCI IDs to be specified as follows:
> -device
> nvme,id_vendor=0xABCD,id_device=0xA0B0,id_subsys_vendor=0xEF00,id_subsys=0xEF01
>
> IEEE-OUI ID (Identify Controller bytes 75:73) is to be
> specified in LE format.
> (e.g. ieee_oui=0xABCDEF => Byte[73]=0xEF, Byte[74]=0xCD, Byte[75]=0xAB).
>
> Signed-off-by: Saif Abrar
[...]
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 127c3d2383..35aeb48e0b 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
[...]
> @@ -8451,6 +8480,13 @@ static Property nvme_props[] = {
>params.sriov_max_vq_per_vf, 0),
> DEFINE_PROP_BOOL("msix-exclusive-bar", NvmeCtrl,
> params.msix_exclusive_bar,
> false),
> +DEFINE_PROP_UINT16("id_vendor", NvmeCtrl, params.id_vendor, 0),
> +DEFINE_PROP_UINT16("id_device", NvmeCtrl, params.id_device, 0),
> +DEFINE_PROP_UINT16("id_subsys_vendor", NvmeCtrl,
> +params.id_subsys_vendor,
> 0),
> +DEFINE_PROP_UINT16("id_subsys", NvmeCtrl, params.id_subsys, 0),
> +DEFINE_PROP("ieee_oui", NvmeCtrl, params.ieee_oui, nvme_prop_ieee,
> +
> uint32_t),
> DEFINE_PROP_END_OF_LIST(),
> };
You add properties, not CLI options. Properties are accessible via CLI
-device, but also via monitor device_add, qom-set, qom-get.
Please rephrase your commit message like "Add properties for".
Re: [PATCH v3 3/5] mirror: allow specifying working bitmap
bitmap's granularity is used as the job's granularity. When
> +# the target does not support COW and is a diff image, i.e. one
> +# that should only contain the delta and was not synced to
> +# previously, the target's cluster size must not be larger than
> +# the bitmap's granularity and copy-mode=write-blocking should not
Comma before "and", please.
> +# be used. That is, because unaligned writes will lead to
Two spaces between sentences for consistency, please.
Suggest "Otherwise, unaligned writes ..."
> +# allocated clusters with partial data in the target image!
> +# (Since 9.1)
> +#
> # @granularity: granularity of the dirty bitmap, default is 64K if the
> # image format doesn't have clusters, 4K if the clusters are
> # smaller than that, else the cluster size. Must be a power of 2
> @@ -2228,12 +2240,18 @@
> # disappear from the query list without user intervention.
> # Defaults to true. (Since 3.1)
> #
> +# Features:
> +#
> +# @unstable: Member @bitmap is experimental.
> +#
> # Since: 1.3
> ##
> { 'struct': 'DriveMirror',
>'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
> '*format': 'str', '*node-name': 'str', '*replaces': 'str',
> -'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
> +'sync': 'MirrorSyncMode',
> +'*bitmap': { 'type': 'str', 'features': [ 'unstable' ] },
> +'*mode': 'NewImageMode',
> '*speed': 'int', '*granularity': 'uint32',
> '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
> '*on-target-error': 'BlockdevOnError',
> @@ -2513,6 +2531,18 @@
> # destination (all the disk, only the sectors allocated in the
> # topmost image, or only new I/O).
> #
> +# @bitmap: The name of a bitmap to use as a working bitmap for
> +# sync=full mode. This argument must be not be present for other
> +# sync modes and not at the same time as @granularity. The
> +# bitmap's granularity is used as the job's granularity. When
> +# the target does not support COW and is a diff image, i.e. one
> +# that should only contain the delta and was not synced to
> +# previously, the target's cluster size must not be larger than
> +# the bitmap's granularity and copy-mode=write-blocking should not
> +# be used. That is, because unaligned writes will lead to
> +# allocated clusters with partial data in the target image!
> +# (Since 9.1)
Comments above apply.
> +#
> # @granularity: granularity of the dirty bitmap, default is 64K if the
> # image format doesn't have clusters, 4K if the clusters are
> # smaller than that, else the cluster size. Must be a power of 2
> @@ -2548,6 +2578,10 @@
> # disappear from the query list without user intervention.
> # Defaults to true. (Since 3.1)
> #
> +# Features:
> +#
> +# @unstable: Member @bitmap is experimental.
> +#
> # Since: 2.6
> #
> # Example:
> @@ -2562,6 +2596,7 @@
>'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
> '*replaces': 'str',
> 'sync': 'MirrorSyncMode',
> +'*bitmap': { 'type': 'str', 'features': [ 'unstable' ] },
> '*speed': 'int', '*granularity': 'uint32',
> '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
> '*on-target-error': 'BlockdevOnError',
[...]
With my comments addressed,
Acked-by: Markus Armbruster
Re: [PATCH v4 10/12] hostmem: add a new memory backend based on POSIX shm_open()
Stefano Garzarella writes:
> shm_open() creates and opens a new POSIX shared memory object.
> A POSIX shared memory object allows creating memory backend with an
> associated file descriptor that can be shared with external processes
> (e.g. vhost-user).
>
> The new `memory-backend-shm` can be used as an alternative when
> `memory-backend-memfd` is not available (Linux only), since shm_open()
> should be provided by any POSIX-compliant operating system.
>
> This backend mimics memfd, allocating memory that is practically
> anonymous. In theory shm_open() requires a name, but this is allocated
> for a short time interval and shm_unlink() is called right after
> shm_open(). After that, only fd is shared with external processes
> (e.g., vhost-user) as if it were associated with anonymous memory.
>
> In the future we may also allow the user to specify the name to be
> passed to shm_open(), but for now we keep the backend simple, mimicking
> anonymous memory such as memfd.
>
> Acked-by: David Hildenbrand
> Signed-off-by: Stefano Garzarella
> ---
> v4
> - fail if we find "share=off" in shm_backend_memory_alloc() [David]
> v3
> - enriched commit message and documentation to highlight that we
> want to mimic memfd (David)
> ---
> docs/system/devices/vhost-user.rst | 5 +-
> qapi/qom.json | 17
> backends/hostmem-shm.c | 123 +
> backends/meson.build | 1 +
> qemu-options.hx| 13 +++
> 5 files changed, 157 insertions(+), 2 deletions(-)
> create mode 100644 backends/hostmem-shm.c
>
> diff --git a/docs/system/devices/vhost-user.rst
> b/docs/system/devices/vhost-user.rst
> index 9b2da106ce..35259d8ec7 100644
> --- a/docs/system/devices/vhost-user.rst
> +++ b/docs/system/devices/vhost-user.rst
> @@ -98,8 +98,9 @@ Shared memory object
>
> In order for the daemon to access the VirtIO queues to process the
> requests it needs access to the guest's address space. This is
> -achieved via the ``memory-backend-file`` or ``memory-backend-memfd``
> -objects. A reference to a file-descriptor which can access this object
> +achieved via the ``memory-backend-file``, ``memory-backend-memfd``, or
> +``memory-backend-shm`` objects.
> +A reference to a file-descriptor which can access this object
> will be passed via the socket as part of the protocol negotiation.
>
> Currently the shared memory object needs to match the size of the main
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 38dde6d785..52df052df8 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -721,6 +721,19 @@
> '*hugetlbsize': 'size',
> '*seal': 'bool' } }
>
> +##
> +# @MemoryBackendShmProperties:
> +#
> +# Properties for memory-backend-shm objects.
> +#
> +# The @share boolean option is true by default with shm.
This contradicts the doc comment for @share:
# @share: if false, the memory is private to QEMU; if true, it is
# shared (default: false)
Your intention is to override that text. But that's less than clear.
Moreover, the documentation of @share is pretty far from this override.
John Snow is working on patches that'll pull it closer.
Hmm, MemoryBackendMemfdProperties has the same override.
I think we should change the doc comment for @share to something like
# @share: if false, the memory is private to QEMU; if true, it is
# shared (default depends on the backend type)
and then document the actual default with each backend type.
> +#
> +# Since: 9.1
> +##
> +{ 'struct': 'MemoryBackendShmProperties',
> + 'base': 'MemoryBackendProperties',
> + 'data': { } }
Let's add 'if': 'CONFIG_POSIX' here.
> +
> ##
> # @MemoryBackendEpcProperties:
> #
> @@ -985,6 +998,8 @@
> { 'name': 'memory-backend-memfd',
>'if': 'CONFIG_LINUX' },
> 'memory-backend-ram',
> +{ 'name': 'memory-backend-shm',
> + 'if': 'CONFIG_POSIX' },
> 'pef-guest',
> { 'name': 'pr-manager-helper',
>'if': 'CONFIG_LINUX' },
> @@ -1056,6 +1071,8 @@
>'memory-backend-memfd': { 'type': 'MemoryBackendMemfdProperties',
>'if': 'CONFIG_LINUX' },
>'memory-backend-ram': 'MemoryBackendProperties',
> + 'memory-backend-shm': { 'type': 'MemoryBackendShmProperties',
> + 'if': 'CONFIG_POSIX' },
>'pr-manager-helper': { 'type': 'PrManagerHelperProperties',
>'if': 'CONFIG_LINUX' },
>'qtest': 'QtestProperties',
[...]
> diff --git a/qemu-options.hx b/qemu-options.hx
> index cf61f6b863..2226172fb0 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -5227,6 +5227,19 @@ SRST
>
> The ``share`` boolean option is on by default with memfd.
>
> +``-object
>
Re: [PATCH v4 3/3] qapi: introduce device-sync-config
Vladimir Sementsov-Ogievskiy writes:
> On 30.04.24 11:31, Vladimir Sementsov-Ogievskiy wrote:
>> On 30.04.24 11:19, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy writes:
>>>
>>>> Add command to sync config from vhost-user backend to the device. It
>>>> may be helpful when VHOST_USER_SLAVE_CONFIG_CHANGE_MSG failed or not
>>>> triggered interrupt to the guest or just not available (not supported
>>>> by vhost-user server).
>>>>
>>>> Command result is racy if allow it during migration. Let's allow the
>>>> sync only in RUNNING state.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
[...]
>>>> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
>>>> index 264978aa40..47bfc0506e 100644
>>>> --- a/system/qdev-monitor.c
>>>> +++ b/system/qdev-monitor.c
>>>> @@ -23,6 +23,7 @@
>>>> #include "monitor/monitor.h"
>>>> #include "monitor/qdev.h"
>>>> #include "sysemu/arch_init.h"
>>>> +#include "sysemu/runstate.h"
>>>> #include "qapi/error.h"
>>>> #include "qapi/qapi-commands-qdev.h"
>>>> #include "qapi/qmp/dispatch.h"
>>>> @@ -971,6 +972,53 @@ void qmp_device_del(const char *id, Error **errp)
>>>> }
>>>> }
>>>>
>>>> +int qdev_sync_config(DeviceState *dev, Error **errp)
>>>> +{
>>>> + DeviceClass *dc = DEVICE_GET_CLASS(dev);
>>>> +
>>>> + if (!dc->sync_config) {
>>>> + error_setg(errp, "device-sync-config is not supported for '%s'",
>>>> + object_get_typename(OBJECT(dev)));
>>>> + return -ENOTSUP;
>>>> + }
>>>> +
>>>> + return dc->sync_config(dev, errp);
>>>> +}
>>>> +
>>>> +void qmp_device_sync_config(const char *id, Error **errp)
>>>> +{
>>>> + DeviceState *dev;
>>>> +
>>>> + /*
>>>> + * During migration there is a race between syncing`configuration and
>>>> + * migrating it (if migrate first, that target would get outdated
>>>> version),
>>>> + * so let's just not allow it.
>>>
>>> Wrap comment lines around column 70 for legibility, please.
>>>
>>>> + *
>>>> + * Moreover, let's not rely on setting up interrupts in paused
>>>> + * state, which may be a part of migration process.
>>>
>>> We discussed this in review of v3. You wanted to check whether the
>>> problem is real. Is it?
>>
>> We discussed it later than I've sent v4 :) No, I didn't check yet.
>
> Checked. Seems this works (in scheme pause -> migrate -> resume, interrupts
> are migrated and triggered on target after resume), so my doubt was wrong.
Thanks!
[...]
Re: [PATCH v4 2/3] vhost-user-blk: split vhost_user_blk_sync_config()
Vladimir Sementsov-Ogievskiy writes:
> On 30.04.24 11:15, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy writes:
>>
>>> Split vhost_user_blk_sync_config() out from
>>> vhost_user_blk_handle_config_change(), to be reused in the following
>>> commit.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>> ---
>>> hw/block/vhost-user-blk.c | 26 +++---
>>> 1 file changed, 19 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>>> index 9e6bbc6950..091d2c6acf 100644
>>> --- a/hw/block/vhost-user-blk.c
>>> +++ b/hw/block/vhost-user-blk.c
>>> @@ -88,27 +88,39 @@ static void vhost_user_blk_set_config(VirtIODevice
>>> *vdev, const uint8_t *config)
>>> s->blkcfg.wce = blkcfg->wce;
>>> }
>>> +static int vhost_user_blk_sync_config(DeviceState *dev, Error **errp)
>>> +{
>>> +int ret;
>>> +VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>>
>> Note for later: all this function does with paramter @dev is cast it to
>> VirtIODevice *.
>>
>>> +VHostUserBlk *s = VHOST_USER_BLK(vdev);
>>> +
>>> +ret = vhost_dev_get_config(>dev, (uint8_t *)>blkcfg,
>>> + vdev->config_len, errp);
>>> +if (ret < 0) {
>>> +return ret;
>>> +}
>>> +
>>> +memcpy(vdev->config, >blkcfg, vdev->config_len);
>>> +virtio_notify_config(vdev);
>>> +
>>> +return 0;
>>> +}
>>> +
>>> static int vhost_user_blk_handle_config_change(struct vhost_dev *dev)
>>> {
>>> int ret;
>>> -VirtIODevice *vdev = dev->vdev;
>>> -VHostUserBlk *s = VHOST_USER_BLK(dev->vdev);
>>> Error *local_err = NULL;
>>>
>>> if (!dev->started) {
>>> return 0;
>>> }
>>>
>>> -ret = vhost_dev_get_config(dev, (uint8_t *)>blkcfg,
>>> - vdev->config_len, _err);
>>> +ret = vhost_user_blk_sync_config(DEVICE(dev->vdev), _err);
>>
>> dev->vdev is a VirtIODevice *. You cast it to DeviceState * for
>> vhost_user_blk_sync_config(), which casts it right back.
>> Could you simply pass it as is instead?
>
> vhost_user_blk_sync_config() is generic handler, which will be used as
> ->sync_config() in the following commit, so it's good and convenient for it
> to have DeviceState* argument.
Ah, that's what I missed.
>>> if (ret < 0) {
>>> error_report_err(local_err);
>>> return ret;
>>> }
>>>
>>> -memcpy(dev->vdev->config, >blkcfg, vdev->config_len);
>>> -virtio_notify_config(dev->vdev);
>>> -
>>> return 0;
>>> }
Re: [PATCH v4 3/3] qapi: introduce device-sync-config
Vladimir Sementsov-Ogievskiy writes:
> Add command to sync config from vhost-user backend to the device. It
> may be helpful when VHOST_USER_SLAVE_CONFIG_CHANGE_MSG failed or not
> triggered interrupt to the guest or just not available (not supported
> by vhost-user server).
>
> Command result is racy if allow it during migration. Let's allow the
> sync only in RUNNING state.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> hw/block/vhost-user-blk.c | 1 +
> hw/virtio/virtio-pci.c| 9
> include/hw/qdev-core.h| 3 +++
> qapi/qdev.json| 23 +++
> system/qdev-monitor.c | 48 +++
> 5 files changed, 84 insertions(+)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 091d2c6acf..2f301f380c 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -588,6 +588,7 @@ static void vhost_user_blk_class_init(ObjectClass *klass,
> void *data)
>
> device_class_set_props(dc, vhost_user_blk_properties);
> dc->vmsd = _vhost_user_blk;
> +dc->sync_config = vhost_user_blk_sync_config;
> set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> vdc->realize = vhost_user_blk_device_realize;
> vdc->unrealize = vhost_user_blk_device_unrealize;
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index b1d02f4b3d..0d91e8b5dc 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -2351,6 +2351,14 @@ static void virtio_pci_dc_realize(DeviceState *qdev,
> Error **errp)
> vpciklass->parent_dc_realize(qdev, errp);
> }
>
> +static int virtio_pci_sync_config(DeviceState *dev, Error **errp)
> +{
> +VirtIOPCIProxy *proxy = VIRTIO_PCI(dev);
> +VirtIODevice *vdev = virtio_bus_get_device(>bus);
> +
> +return qdev_sync_config(DEVICE(vdev), errp);
> +}
> +
> static void virtio_pci_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -2367,6 +2375,7 @@ static void virtio_pci_class_init(ObjectClass *klass,
> void *data)
> device_class_set_parent_realize(dc, virtio_pci_dc_realize,
> >parent_dc_realize);
> rc->phases.hold = virtio_pci_bus_reset_hold;
> +dc->sync_config = virtio_pci_sync_config;
> }
>
> static const TypeInfo virtio_pci_info = {
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 9228e96c87..87135bdcdf 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -95,6 +95,7 @@ typedef void (*DeviceUnrealize)(DeviceState *dev);
> typedef void (*DeviceReset)(DeviceState *dev);
> typedef void (*BusRealize)(BusState *bus, Error **errp);
> typedef void (*BusUnrealize)(BusState *bus);
> +typedef int (*DeviceSyncConfig)(DeviceState *dev, Error **errp);
>
> /**
> * struct DeviceClass - The base class for all devices.
> @@ -162,6 +163,7 @@ struct DeviceClass {
> DeviceReset reset;
> DeviceRealize realize;
> DeviceUnrealize unrealize;
> +DeviceSyncConfig sync_config;
>
> /**
> * @vmsd: device state serialisation description for
> @@ -546,6 +548,7 @@ bool qdev_hotplug_allowed(DeviceState *dev, Error **errp);
> */
> HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
> void qdev_unplug(DeviceState *dev, Error **errp);
> +int qdev_sync_config(DeviceState *dev, Error **errp);
> void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
>DeviceState *dev, Error **errp);
> void qdev_machine_creation_done(void);
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index facaa0bc6a..fc5e125a45 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -161,3 +161,26 @@
> ##
> { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>'data': { '*device': 'str', 'path': 'str' } }
> +
> +##
> +# @device-sync-config:
> +#
> +# Synchronize device configuration from host to guest part. First,
> +# copy the configuration from the host part (backend) to the guest
> +# part (frontend). Then notify guest software that device
> +# configuration changed.
Blank line here, please.
> +# The command may be used to notify the guest about block device
> +# capcity change. Currently only vhost-user-blk device supports
> +# this.
> +#
> +# @id: the device's ID or QOM path
> +#
> +# Features:
> +#
> +# @unstable: The command is experimental.
> +#
> +# Since: 9.1
> +##
> +{ 'command': 'device-sync-config',
> + 'features': [ 'unstable' ],
> + 'data': {'id': 'str'} }
> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> index 264978aa40..47bfc0506e 100644
> --- a/system/qdev-monitor.c
> +++ b/system/qdev-monitor.c
> @@ -23,6 +23,7 @@
> #include "monitor/monitor.h"
> #include "monitor/qdev.h"
> #include "sysemu/arch_init.h"
> +#include "sysemu/runstate.h"
> #include "qapi/error.h"
> #include "qapi/qapi-commands-qdev.h"
> #include "qapi/qmp/dispatch.h"
> @@ -971,6 +972,53 @@ void qmp_device_del(const char *id, Error **errp)
Re: [PATCH v4 2/3] vhost-user-blk: split vhost_user_blk_sync_config()
Vladimir Sementsov-Ogievskiy writes:
> Split vhost_user_blk_sync_config() out from
> vhost_user_blk_handle_config_change(), to be reused in the following
> commit.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> hw/block/vhost-user-blk.c | 26 +++---
> 1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 9e6bbc6950..091d2c6acf 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -88,27 +88,39 @@ static void vhost_user_blk_set_config(VirtIODevice *vdev,
> const uint8_t *config)
> s->blkcfg.wce = blkcfg->wce;
> }
>
> +static int vhost_user_blk_sync_config(DeviceState *dev, Error **errp)
> +{
> +int ret;
> +VirtIODevice *vdev = VIRTIO_DEVICE(dev);
Note for later: all this function does with paramter @dev is cast it to
VirtIODevice *.
> +VHostUserBlk *s = VHOST_USER_BLK(vdev);
> +
> +ret = vhost_dev_get_config(>dev, (uint8_t *)>blkcfg,
> + vdev->config_len, errp);
> +if (ret < 0) {
> +return ret;
> +}
> +
> +memcpy(vdev->config, >blkcfg, vdev->config_len);
> +virtio_notify_config(vdev);
> +
> +return 0;
> +}
> +
> static int vhost_user_blk_handle_config_change(struct vhost_dev *dev)
> {
> int ret;
> -VirtIODevice *vdev = dev->vdev;
> -VHostUserBlk *s = VHOST_USER_BLK(dev->vdev);
> Error *local_err = NULL;
>
> if (!dev->started) {
> return 0;
> }
>
> -ret = vhost_dev_get_config(dev, (uint8_t *)>blkcfg,
> - vdev->config_len, _err);
> +ret = vhost_user_blk_sync_config(DEVICE(dev->vdev), _err);
dev->vdev is a VirtIODevice *. You cast it to DeviceState * for
vhost_user_blk_sync_config(), which casts it right back.
Could you simply pass it as is instead?
> if (ret < 0) {
> error_report_err(local_err);
> return ret;
> }
>
> -memcpy(dev->vdev->config, >blkcfg, vdev->config_len);
> -virtio_notify_config(dev->vdev);
> -
> return 0;
> }
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. [...]
Re: [PATCH v3 4/5] qapi: introduce device-sync-config
Vladimir Sementsov-Ogievskiy writes:
> On 29.04.24 13:51, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy writes:
>>
>>> On 24.04.24 14:48, Markus Armbruster wrote:
>>>> Vladimir Sementsov-Ogievskiy writes:
>>>>
>>>>> Add command to sync config from vhost-user backend to the device. It
>>>>> may be helpful when VHOST_USER_SLAVE_CONFIG_CHANGE_MSG failed or not
>>>>> triggered interrupt to the guest or just not available (not supported
>>>>> by vhost-user server).
>>>>>
>>>>> Command result is racy if allow it during migration. Let's allow the
>>>>> sync only in RUNNING state.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>
>> [...]
>>
>>>>> diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
>>>>> index 0117d243c4..296af52322 100644
>>>>> --- a/include/sysemu/runstate.h
>>>>> +++ b/include/sysemu/runstate.h
>>>>> @@ -5,6 +5,7 @@
>>>>>#include "qemu/notify.h"
>>>>>
>>>>>bool runstate_check(RunState state);
>>>>> +const char *current_run_state_str(void);
>>>>>void runstate_set(RunState new_state);
>>>>>RunState runstate_get(void);
>>>>>bool runstate_is_running(void);
>>>>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>>>>> index facaa0bc6a..e8be79c3d5 100644
>>>>> --- a/qapi/qdev.json
>>>>> +++ b/qapi/qdev.json
>>>>> @@ -161,3 +161,24 @@
>>>>>##
>>>>>{ 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>>>>> 'data': { '*device': 'str', 'path': 'str' } }
>>>>> +
>>>>> +##
>>>>> +# @device-sync-config:
>>>>> +#
>>>>> +# Synchronize config from backend to the guest. The command notifies
>>>>> +# re-read the device config from the backend and notifies the guest
>>>>> +# to re-read the config. The command may be used to notify the guest
>>>>> +# about block device capcity change. Currently only vhost-user-blk
>>>>> +# device supports this.
>>>>
>>>> I'm not sure I understand this. To work towards an understanding, I
>>>> rephrase it, and you point out the errors.
>>>>
>>>>Synchronize device configuration from host to guest part. First,
>>>>copy the configuration from the host part (backend) to the guest
>>>>part (frontend). Then notify guest software that device
>>>>configuration changed.
>>>
>>> Correct, thanks
>>
>> Perhaps
>>
>>Synchronize guest-visible device configuration with the backend's
>>configuration, and notify guest software that device configuration
>>changed.
>>
>>This may be useful to notify the guest of a block device capacity
>>change. Currenrly, only vhost-user-blk devices support this.
>
> Sounds good
Except I fat-fingered "Currently".
>>
>> Next question: what happens when the device *doesn't* support this?
>
> An error "device-sync-config is not supported ..."
Okay.
>>>> I wonder how configuration can get out of sync. Can you explain?
>>>>
>>>
>>> The example (and the original feature, which triggered developing this) is
>>> vhost disk resize. If vhost-server (backend) doesn't support
>>> VHOST_USER_SLAVE_CONFIG_CHANGE_MSG, neither QEMU nor guest will know that
>>> disk capacity changed.
>>
>> Sounds like we wouldn't need this command if we could make the
>> vhost-server support VHOST_USER_SLAVE_CONFIG_CHANGE_MSG. Is making it
>> support it impractical? Or are there other uses for this command?
>
> Qemu's internal vhost-server do support it. But that's not the only
> vhost-user server) So the command is useful for those servers which doesn't
> support VHOST_USER_SLAVE_CONFIG_CHANGE_MSG. Note, that this message requires
> setting up additional channel of server -> client communication. That was the
> reason, why the "change-msg" solution was rejected in our downstream: it's
> safer to reuse existing channel (QMP), than to add and support an additional
> channel.
>
> Also, the command may help to debug the system, when
> VHOST_USER_SLAVE_CONFIG_CHANGE_MSG doesn't work for some reason.
Suggest to work this into the commit message.
>>>>> +#
Re: [PATCH v3 4/5] qapi: introduce device-sync-config
Vladimir Sementsov-Ogievskiy writes:
> On 24.04.24 14:48, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy writes:
>>
>>> Add command to sync config from vhost-user backend to the device. It
>>> may be helpful when VHOST_USER_SLAVE_CONFIG_CHANGE_MSG failed or not
>>> triggered interrupt to the guest or just not available (not supported
>>> by vhost-user server).
>>>
>>> Command result is racy if allow it during migration. Let's allow the
>>> sync only in RUNNING state.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
[...]
>>> diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
>>> index 0117d243c4..296af52322 100644
>>> --- a/include/sysemu/runstate.h
>>> +++ b/include/sysemu/runstate.h
>>> @@ -5,6 +5,7 @@
>>> #include "qemu/notify.h"
>>>
>>> bool runstate_check(RunState state);
>>> +const char *current_run_state_str(void);
>>> void runstate_set(RunState new_state);
>>> RunState runstate_get(void);
>>> bool runstate_is_running(void);
>>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>>> index facaa0bc6a..e8be79c3d5 100644
>>> --- a/qapi/qdev.json
>>> +++ b/qapi/qdev.json
>>> @@ -161,3 +161,24 @@
>>> ##
>>> { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>>> 'data': { '*device': 'str', 'path': 'str' } }
>>> +
>>> +##
>>> +# @device-sync-config:
>>> +#
>>> +# Synchronize config from backend to the guest. The command notifies
>>> +# re-read the device config from the backend and notifies the guest
>>> +# to re-read the config. The command may be used to notify the guest
>>> +# about block device capcity change. Currently only vhost-user-blk
>>> +# device supports this.
>>
>> I'm not sure I understand this. To work towards an understanding, I
>> rephrase it, and you point out the errors.
>>
>> Synchronize device configuration from host to guest part. First,
>> copy the configuration from the host part (backend) to the guest
>> part (frontend). Then notify guest software that device
>> configuration changed.
>
> Correct, thanks
Perhaps
Synchronize guest-visible device configuration with the backend's
configuration, and notify guest software that device configuration
changed.
This may be useful to notify the guest of a block device capacity
change. Currenrly, only vhost-user-blk devices support this.
Next question: what happens when the device *doesn't* support this?
>> I wonder how configuration can get out of sync. Can you explain?
>>
>
> The example (and the original feature, which triggered developing this) is
> vhost disk resize. If vhost-server (backend) doesn't support
> VHOST_USER_SLAVE_CONFIG_CHANGE_MSG, neither QEMU nor guest will know that
> disk capacity changed.
Sounds like we wouldn't need this command if we could make the
vhost-server support VHOST_USER_SLAVE_CONFIG_CHANGE_MSG. Is making it
support it impractical? Or are there other uses for this command?
>>> +#
>>> +# @id: the device's ID or QOM path
>>> +#
>>> +# Features:
>>> +#
>>> +# @unstable: The command is experimental.
>>> +#
>>> +# Since: 9.1
>>> +##
>>> +{ 'command': 'device-sync-config',
>>> + 'features': [ 'unstable' ],
>>> + 'data': {'id': 'str'} }
>>> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
>>> index 7e075d91c1..cb35ea0b86 100644
>>> --- a/system/qdev-monitor.c
>>> +++ b/system/qdev-monitor.c
>>> @@ -23,6 +23,7 @@
>>> #include "monitor/monitor.h"
>>> #include "monitor/qdev.h"
>>> #include "sysemu/arch_init.h"
>>> +#include "sysemu/runstate.h"
>>> #include "qapi/error.h"
>>> #include "qapi/qapi-commands-qdev.h"
>>> #include "qapi/qmp/dispatch.h"
>>> @@ -969,6 +970,52 @@ void qmp_device_del(const char *id, Error **errp)
>>> }
>>> }
>>>
>>> +int qdev_sync_config(DeviceState *dev, Error **errp)
>>> +{
>>> +DeviceClass *dc = DEVICE_GET_CLASS(dev);
>>> +
>>> +if (!dc->sync_config) {
>>> +error_setg(errp, "device-sync-config is not supported for '%s'",
>>> + object_get_typename(OBJECT(dev)));
>>> +return -ENOTSUP;
>>> +}
>>> +
>>> +return dc->sync_config(dev, errp);
>>>
Re: [PATCH v3 5/5] qapi: introduce CONFIG_READ event
Vladimir Sementsov-Ogievskiy writes:
> Send a new event when guest reads virtio-pci config after
> virtio_notify_config() call.
>
> That's useful to check that guest fetched modified config, for example
> after resizing disk backend.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> hw/virtio/virtio-pci.c | 9 +
> include/monitor/qdev.h | 2 ++
> monitor/monitor.c | 1 +
> qapi/qdev.json | 33 +
> stubs/qdev.c | 6 ++
> system/qdev-monitor.c | 6 ++
> 6 files changed, 57 insertions(+)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 92afbae71c..c0c158dae2 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -23,6 +23,7 @@
> #include "hw/boards.h"
> #include "hw/virtio/virtio.h"
> #include "migration/qemu-file-types.h"
> +#include "monitor/qdev.h"
> #include "hw/pci/pci.h"
> #include "hw/pci/pci_bus.h"
> #include "hw/qdev-properties.h"
> @@ -530,6 +531,10 @@ static uint64_t virtio_pci_config_read(void *opaque,
> hwaddr addr,
> }
> addr -= config;
>
> +if (vdev->generation > 0) {
> +qdev_virtio_config_read_event(DEVICE(proxy));
> +}
> +
> switch (size) {
> case 1:
> val = virtio_config_readb(vdev, addr);
> @@ -1884,6 +1889,10 @@ static uint64_t virtio_pci_device_read(void *opaque,
> hwaddr addr,
> return UINT64_MAX;
> }
>
> +if (vdev->generation > 0) {
> +qdev_virtio_config_read_event(DEVICE(proxy));
> +}
> +
> switch (size) {
> case 1:
> val = virtio_config_modern_readb(vdev, addr);
> diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
> index 1d57bf6577..fc9a834dca 100644
> --- a/include/monitor/qdev.h
> +++ b/include/monitor/qdev.h
> @@ -36,4 +36,6 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
> */
> const char *qdev_set_id(DeviceState *dev, char *id, Error **errp);
>
> +void qdev_virtio_config_read_event(DeviceState *dev);
> +
> #endif
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 01ede1babd..5b06146503 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -316,6 +316,7 @@ static MonitorQAPIEventConf
> monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
> [QAPI_EVENT_VSERPORT_CHANGE] = { 1000 * SCALE_MS },
> [QAPI_EVENT_MEMORY_DEVICE_SIZE_CHANGE] = { 1000 * SCALE_MS },
> [QAPI_EVENT_HV_BALLOON_STATUS_REPORT] = { 1000 * SCALE_MS },
> +[QAPI_EVENT_VIRTIO_CONFIG_READ] = { 300 * SCALE_MS },
All the other rate-limited events use 1s. Why 0.3s for this one?
> };
>
> /*
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index e8be79c3d5..29a4f47360 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -182,3 +182,36 @@
> { 'command': 'device-sync-config',
>'features': [ 'unstable' ],
>'data': {'id': 'str'} }
> +
> +##
> +# @VIRTIO_CONFIG_READ:
> +#
> +# Emitted whenever guest reads virtio device configuration after
> +# configuration change.
Is it emitted whenever the guest reads, or only when it reads after a
configuration change?
> +#
> +# The event may be used in pair with device-sync-config. It shows
> +# that guest has re-read updated configuration. It doesn't
> +# guarantee that guest successfully handled it and updated the
> +# view of the device for the user, but still it's a kind of
> +# success indicator.
The event is virtio-only. device-sync-config isn't. Why?
> +#
> +# @device: device name
> +#
> +# @path: device path
> +#
> +# Features:
> +#
> +# @unstable: The event is experimental.
> +#
Missing:
# Note: This event is rate-limited.
#
> +# Since: 9.1
> +#
> +# Example:
> +#
> +# <- { "event": "VIRTIO_CONFIG_READ",
> +# "data": { "device": "virtio-net-pci-0",
> +#"path": "/machine/peripheral/virtio-net-pci-0" },
> +# "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> +##
> +{ 'event': 'VIRTIO_CONFIG_READ',
> + 'features': [ 'unstable' ],
> + 'data': { '*device': 'str', 'path': 'str' } }
> diff --git a/stubs/qdev.c b/stubs/qdev.c
> index 6869f6f90a..ab6c4afe0b 100644
> --- a/stubs/qdev.c
> +++ b/stubs/qdev.c
> @@ -26,3 +26,9 @@ void qapi_event_send_device_unplug_guest_error(const char
> *device,
> {
> /* Nothing to do. */
> }
> +
> +void qapi_event_send_virtio_config_read(const char *device,
> +const char *path)
> +{
> +/* Nothing to do. */
> +}
> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> index cb35ea0b86..8a2ca77fde 100644
> --- a/system/qdev-monitor.c
> +++ b/system/qdev-monitor.c
> @@ -26,6 +26,7 @@
> #include "sysemu/runstate.h"
> #include "qapi/error.h"
> #include "qapi/qapi-commands-qdev.h"
> +#include "qapi/qapi-events-qdev.h"
> #include "qapi/qmp/dispatch.h"
> #include "qapi/qmp/qdict.h"
> #include "qapi/qmp/qerror.h"
> @@ -1206,3 +1207,8 @@ bool qmp_command_available(const QmpCommand *cmd, Error
> **errp)
> }
>
Re: [PATCH v3 4/5] qapi: introduce device-sync-config
Vladimir Sementsov-Ogievskiy writes:
> Add command to sync config from vhost-user backend to the device. It
> may be helpful when VHOST_USER_SLAVE_CONFIG_CHANGE_MSG failed or not
> triggered interrupt to the guest or just not available (not supported
> by vhost-user server).
>
> Command result is racy if allow it during migration. Let's allow the
> sync only in RUNNING state.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
[...]
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 9228e96c87..87135bdcdf 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -95,6 +95,7 @@ typedef void (*DeviceUnrealize)(DeviceState *dev);
> typedef void (*DeviceReset)(DeviceState *dev);
> typedef void (*BusRealize)(BusState *bus, Error **errp);
> typedef void (*BusUnrealize)(BusState *bus);
> +typedef int (*DeviceSyncConfig)(DeviceState *dev, Error **errp);
>
> /**
> * struct DeviceClass - The base class for all devices.
> @@ -162,6 +163,7 @@ struct DeviceClass {
> DeviceReset reset;
> DeviceRealize realize;
> DeviceUnrealize unrealize;
> +DeviceSyncConfig sync_config;
I get
include/hw/qdev-core.h:179: warning: Function parameter or member
'sync_config' not described in 'DeviceClass'
To fix this, cover the new member in the doc comment.
>
> /**
> * @vmsd: device state serialisation description for
[...]
Re: [PATCH v3 4/5] qapi: introduce device-sync-config
Vladimir Sementsov-Ogievskiy writes:
> Add command to sync config from vhost-user backend to the device. It
> may be helpful when VHOST_USER_SLAVE_CONFIG_CHANGE_MSG failed or not
> triggered interrupt to the guest or just not available (not supported
> by vhost-user server).
>
> Command result is racy if allow it during migration. Let's allow the
> sync only in RUNNING state.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> hw/block/vhost-user-blk.c | 27 --
> hw/virtio/virtio-pci.c| 9
> include/hw/qdev-core.h| 3 +++
> include/sysemu/runstate.h | 1 +
> qapi/qdev.json| 21 +
> system/qdev-monitor.c | 47 +++
> system/runstate.c | 5 +
> 7 files changed, 106 insertions(+), 7 deletions(-)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 9e6bbc6950..2f301f380c 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -88,27 +88,39 @@ static void vhost_user_blk_set_config(VirtIODevice *vdev,
> const uint8_t *config)
> s->blkcfg.wce = blkcfg->wce;
> }
>
> +static int vhost_user_blk_sync_config(DeviceState *dev, Error **errp)
> +{
> +int ret;
> +VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> +VHostUserBlk *s = VHOST_USER_BLK(vdev);
> +
> +ret = vhost_dev_get_config(>dev, (uint8_t *)>blkcfg,
> + vdev->config_len, errp);
> +if (ret < 0) {
> +return ret;
> +}
> +
> +memcpy(vdev->config, >blkcfg, vdev->config_len);
> +virtio_notify_config(vdev);
> +
> +return 0;
> +}
> +
> static int vhost_user_blk_handle_config_change(struct vhost_dev *dev)
> {
> int ret;
> -VirtIODevice *vdev = dev->vdev;
> -VHostUserBlk *s = VHOST_USER_BLK(dev->vdev);
> Error *local_err = NULL;
>
> if (!dev->started) {
> return 0;
> }
>
> -ret = vhost_dev_get_config(dev, (uint8_t *)>blkcfg,
> - vdev->config_len, _err);
> +ret = vhost_user_blk_sync_config(DEVICE(dev->vdev), _err);
> if (ret < 0) {
> error_report_err(local_err);
> return ret;
> }
>
> -memcpy(dev->vdev->config, >blkcfg, vdev->config_len);
> -virtio_notify_config(dev->vdev);
> -
> return 0;
> }
This factors vhost_user_blk_sync_config() out of
vhost_user_blk_handle_config_change() for reuse. Correct?
>
> @@ -576,6 +588,7 @@ static void vhost_user_blk_class_init(ObjectClass *klass,
> void *data)
>
> device_class_set_props(dc, vhost_user_blk_properties);
> dc->vmsd = _vhost_user_blk;
> +dc->sync_config = vhost_user_blk_sync_config;
> set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> vdc->realize = vhost_user_blk_device_realize;
> vdc->unrealize = vhost_user_blk_device_unrealize;
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index eaaf86402c..92afbae71c 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -2501,6 +2501,14 @@ static void virtio_pci_dc_realize(DeviceState *qdev,
> Error **errp)
> vpciklass->parent_dc_realize(qdev, errp);
> }
>
> +static int virtio_pci_sync_config(DeviceState *dev, Error **errp)
> +{
> +VirtIOPCIProxy *proxy = VIRTIO_PCI(dev);
> +VirtIODevice *vdev = virtio_bus_get_device(>bus);
> +
> +return qdev_sync_config(DEVICE(vdev), errp);
> +}
> +
> static void virtio_pci_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -2517,6 +2525,7 @@ static void virtio_pci_class_init(ObjectClass *klass,
> void *data)
> device_class_set_parent_realize(dc, virtio_pci_dc_realize,
> >parent_dc_realize);
> rc->phases.hold = virtio_pci_bus_reset_hold;
> +dc->sync_config = virtio_pci_sync_config;
> }
>
I tried to follow the callbacks, but quickly gave up. Leaving to a
reviewer who understands virtio.
> static const TypeInfo virtio_pci_info = {
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 9228e96c87..87135bdcdf 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -95,6 +95,7 @@ typedef void (*DeviceUnrealize)(DeviceState *dev);
> typedef void (*DeviceReset)(DeviceState *dev);
> typedef void (*BusRealize)(BusState *bus, Error **errp);
> typedef void (*BusUnrealize)(BusState *bus);
> +typedef int (*DeviceSyncConfig)(DeviceState *dev, Error **errp);
>
> /**
> * struct DeviceClass - The base class for all devices.
> @@ -162,6 +163,7 @@ struct DeviceClass {
> DeviceReset reset;
> DeviceRealize realize;
> DeviceUnrealize unrealize;
> +DeviceSyncConfig sync_config;
>
> /**
> * @vmsd: device state serialisation description for
> @@ -546,6 +548,7 @@ bool qdev_hotplug_allowed(DeviceState *dev, Error **errp);
> */
> HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
> void qdev_unplug(DeviceState *dev, Error
Re: [PATCH v2 12/13] block/ssh: Use URI parsing code from glib
Eric Blake writes:
> On Fri, Apr 12, 2024 at 03:24:14PM +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.
>>
>> Reviewed-by: Richard W.M. Jones
>> Signed-off-by: Thomas Huth
>> ---
>> block/ssh.c | 75 -
>> 1 file changed, 40 insertions(+), 35 deletions(-)
>>
>
>>
>> -if (g_strcmp0(uri->scheme, "ssh") != 0) {
>> +if (g_strcmp0(g_uri_get_scheme(uri), "ssh") != 0) {
>
> Yet another case-sensitive spot to consider.
>
>>
>> -qdict_put_str(options, "path", uri->path);
>> -
>> -/* Pick out any query parameters that we understand, and ignore
>> - * the rest.
>> - */
>> -for (i = 0; i < qp->n; ++i) {
>> -if (strcmp(qp->p[i].name, "host_key_check") == 0) {
>> -qdict_put_str(options, "host_key_check", qp->p[i].value);
>> +qdict_put_str(options, "path", uri_path);
>> +
>> +uri_query = g_uri_get_query(uri);
>> +if (uri_query) {
>> +g_uri_params_iter_init(, uri_query, -1, "&", G_URI_PARAMS_NONE);
>> +while (g_uri_params_iter_next(, _name, _value, )) {
>> +if (!qp_name || !qp_value || gerror) {
>> +warn_report("Failed to parse SSH URI parameters '%s'.",
>> +uri_query);
>> +break;
>> +}
>> +/*
>> + * Pick out the query parameters that we understand, and ignore
>> + * (or rather warn about) the rest.
>> + */
>> +if (g_str_equal(qp_name, "host_key_check")) {
>> +qdict_put_str(options, "host_key_check", qp_value);
>> +} else {
>> +warn_report("Unsupported parameter '%s' in URI.", qp_name);
>
> Do we want the trailing '.' in warn_report?
We do not.
> The warning is new; it was not in the old code, nor mentioned in the
> commit message. It seems like a good idea, but we should be more
> intentional if we intend to make that change.
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.
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?
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(>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(>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.
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.
Re: [PATCH 1/2] copy-before-write: allow specifying minimum cluster size
Fiona Ebner writes:
> Useful to make discard-source work in the context of backup fleecing
> when the fleecing image has a larger granularity than the backup
> target.
>
> Copy-before-write operations will use at least this granularity and in
> particular, discard requests to the source node will too. If the
> granularity is too small, they will just be aligned down in
> cbw_co_pdiscard_snapshot() and thus effectively ignored.
>
> The QAPI uses uint32 so the value will be non-negative, but still fit
> into a uint64_t.
>
> Suggested-by: Vladimir Sementsov-Ogievskiy
> Signed-off-by: Fiona Ebner
> ---
> block/block-copy.c | 17 +
> block/copy-before-write.c | 3 ++-
> include/block/block-copy.h | 1 +
> qapi/block-core.json | 8 +++-
> 4 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/block/block-copy.c b/block/block-copy.c
> index 7e3b378528..adb1cbb440 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -310,6 +310,7 @@ void block_copy_set_copy_opts(BlockCopyState *s, bool
> use_copy_range,
> }
>
> static int64_t block_copy_calculate_cluster_size(BlockDriverState *target,
> + int64_t min_cluster_size,
> Error **errp)
> {
> int ret;
> @@ -335,7 +336,7 @@ static int64_t
> block_copy_calculate_cluster_size(BlockDriverState *target,
> "used. If the actual block size of the target exceeds "
> "this default, the backup may be unusable",
> BLOCK_COPY_CLUSTER_SIZE_DEFAULT);
> -return BLOCK_COPY_CLUSTER_SIZE_DEFAULT;
> +return MAX(min_cluster_size, BLOCK_COPY_CLUSTER_SIZE_DEFAULT);
min_cluster_size is int64_t, BLOCK_COPY_CLUSTER_SIZE_DEFAULT is int, and
gets converted to int64_t. The return type is int64_t. Okay.
> } else if (ret < 0 && !target_does_cow) {
> error_setg_errno(errp, -ret,
> "Couldn't determine the cluster size of the target image, "
> @@ -345,16 +346,18 @@ static int64_t
> block_copy_calculate_cluster_size(BlockDriverState *target,
> return ret;
> } else if (ret < 0 && target_does_cow) {
> /* Not fatal; just trudge on ahead. */
> -return BLOCK_COPY_CLUSTER_SIZE_DEFAULT;
> +return MAX(min_cluster_size, BLOCK_COPY_CLUSTER_SIZE_DEFAULT);
Same.
> }
>
> -return MAX(BLOCK_COPY_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
> +return MAX(min_cluster_size,
> + MAX(BLOCK_COPY_CLUSTER_SIZE_DEFAULT, bdi.cluster_size));
Similar: bdi.cluster_size is int.
> }
>
> BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
> BlockDriverState *copy_bitmap_bs,
> const BdrvDirtyBitmap *bitmap,
> bool discard_source,
> + int64_t min_cluster_size,
> Error **errp)
> {
> ERRP_GUARD();
> @@ -365,7 +368,13 @@ BlockCopyState *block_copy_state_new(BdrvChild *source,
> BdrvChild *target,
>
> GLOBAL_STATE_CODE();
>
> -cluster_size = block_copy_calculate_cluster_size(target->bs, errp);
> +if (min_cluster_size && !is_power_of_2(min_cluster_size)) {
min_cluster_size is int64_t, is_power_of_2() takes uint64_t. Bad if
min_cluster_size is negative. Could this happen?
> +error_setg(errp, "min-cluster-size needs to be a power of 2");
> +return NULL;
> +}
> +
> +cluster_size = block_copy_calculate_cluster_size(target->bs,
> + min_cluster_size, errp);
min_cluster_size is int64_t, block_copy_calculate_cluster_size() takes
int64_t, returns int64_t, and cluster_size is int64_t. Good.
> if (cluster_size < 0) {
> return NULL;
> }
> diff --git a/block/copy-before-write.c b/block/copy-before-write.c
> index dac57481c5..f9896c6c1e 100644
> --- a/block/copy-before-write.c
> +++ b/block/copy-before-write.c
> @@ -476,7 +476,8 @@ static int cbw_open(BlockDriverState *bs, QDict *options,
> int flags,
>
> s->discard_source = flags & BDRV_O_CBW_DISCARD_SOURCE;
> s->bcs = block_copy_state_new(bs->file, s->target, bs, bitmap,
> - flags & BDRV_O_CBW_DISCARD_SOURCE, errp);
> + flags & BDRV_O_CBW_DISCARD_SOURCE,
> + opts->min_cluster_size, errp);
@opts is BlockdevOptionsCbw *, opts->min_cluster_size is uint32_t (see
last hunk), block_copy_state_new() takes int64_t: opts->min_cluster_size
gets zero-extended. Okay.
> if (!s->bcs) {
> error_prepend(errp, "Cannot create block-copy-state: ");
> return -EINVAL;
> diff --git a/include/block/block-copy.h b/include/block/block-copy.h
> index bdc703bacd..77857c6c68 100644
> ---
Re: [PATCH] qapi/block-core: improve Qcow2OverlapCheckFlags documentation
Squashing in diff --git a/qapi/pragma.json b/qapi/pragma.json index 99e4052ab3..9e28de1721 100644 --- a/qapi/pragma.json +++ b/qapi/pragma.json @@ -72,7 +72,6 @@ 'QCryptoAkCipherKeyType', 'QCryptodevBackendServiceType', 'QKeyCode', -'Qcow2OverlapCheckFlags', 'RbdAuthMode', 'RbdImageEncryptionFormat', 'String',
Re: [PATCH] qapi/block-core: improve Qcow2OverlapCheckFlags documentation
Vladimir Sementsov-Ogievskiy writes: > Most of fields have no description at all. Let's fix that. Still, no > reason to place here more detailed descriptions of what these > structures are, as we have public Qcow2 format specification. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > qapi/block-core.json | 25 + > 1 file changed, 21 insertions(+), 4 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 1874f880a8..b9fb994a66 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -3403,14 +3403,31 @@ > # @Qcow2OverlapCheckFlags: > # > # Structure of flags for each metadata structure. Setting a field to > -# 'true' makes qemu guard that structure against unintended > -# overwriting. The default value is chosen according to the template > -# given. > +# 'true' makes qemu guard that Qcow2 format structure against Mind if I use the occasion to correct the spelling of QEMU? > +# unintended overwriting. See Qcow2 format specification for detailed > +# information on these structures. The default value is chosen > +# according to the template given. > # > # @template: Specifies a template mode which can be adjusted using the > # other flags, defaults to 'cached' > # > -# @bitmap-directory: since 3.0 > +# @main-header: Qcow2 format header > +# > +# @active-l1: Qcow2 active L1 table > +# > +# @active-l2: Qcow2 active L2 table > +# > +# @refcount-table: Qcow2 refcount table > +# > +# @refcount-block: Qcow2 refcount blocks > +# > +# @snapshot-table: Qcow2 snapshot table > +# > +# @inactive-l1: Qcow2 inactive L1 tables > +# > +# @inactive-l2: Qcow2 inactive L2 tables > +# > +# @bitmap-directory: Qcow2 bitmap directory (since 3.0) > # > # Since: 2.9 > ## Acked-by: Markus Armbruster
Re: [PATCH 00/10] Reduce usage of QERR_ macros further
Markus Armbruster writes: > Philippe posted "[PATCH v2 00/22] qapi: Kill 'qapi/qmp/qerror.h' for > good" a couple of months ago. I cherry-picked just its simplest parts > for now. > > Markus Armbruster (1): > error: Drop superfluous #include "qapi/qmp/qerror.h" Queued for 9.1 with PATCH 08's new error message fixed. Thanks!
Re: [PATCH 08/10] qapi: Correct error message for 'vcpu_dirty_limit' parameter
Philippe Mathieu-Daudé writes:
> On 12/3/24 16:26, Zhao Liu wrote:
>> On Tue, Mar 12, 2024 at 03:13:41PM +0100, Markus Armbruster wrote:
>>> Date: Tue, 12 Mar 2024 15:13:41 +0100
>>> From: Markus Armbruster
>>> Subject: [PATCH 08/10] qapi: Correct error message for 'vcpu_dirty_limit'
>>> parameter
>>>
>>> From: Philippe Mathieu-Daudé
>>>
>>> QERR_INVALID_PARAMETER_VALUE is defined as:
>>>
>>>#define QERR_INVALID_PARAMETER_VALUE \
>>>"Parameter '%s' expects %s"
>>>
>>> The current error is formatted as:
>>>
>>>"Parameter 'vcpu_dirty_limit' expects is invalid, it must greater then 1
>>> MB/s"
>>>
>>> Replace by:
>>>
>>>"Parameter 'vcpu_dirty_limit' is invalid, it must greater than 1 MB/s"
>>
>> Is there a grammar error here? Maybe
>> s/it must greater/it must be greater/
>
> Oops indeed!
What about dropping "is invalid, "? I.e. go with
"Parameter 'vcpu_dirty_limit' must be greater than 1 MB/s"
>>> Signed-off-by: Philippe Mathieu-Daudé
>>> Reviewed-by: Juan Quintela
>>> Signed-off-by: Markus Armbruster
>>> ---
>>> migration/options.c | 5 ++---
>>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/migration/options.c b/migration/options.c
>>> index 40eb930940..c5115f1b5c 100644
>>> --- a/migration/options.c
>>> +++ b/migration/options.c
>>> @@ -1264,9 +1264,8 @@ bool migrate_params_check(MigrationParameters
>>> *params, Error **errp)
>>> if (params->has_vcpu_dirty_limit &&
>>> (params->vcpu_dirty_limit < 1)) {
>>> -error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>>> - "vcpu_dirty_limit",
>>> - "is invalid, it must greater then 1 MB/s");
>>> +error_setg(errp, "Parameter 'vcpu_dirty_limit' is invalid,"
>>> + " it must greater than 1 MB/s");
>
> So s/it must greater/it must be greater/ :)
>
>>> return false;
>>> }
>>>
>> Otherwise,
>> Reviewed-by: Zhao Liu
>>
Re: [PATCH v2 3/6] qdev-monitor: add option to report GenericError from find_device_state
Sorry for the late answer.
Vladimir Sementsov-Ogievskiy writes:
> On 07.03.24 12:46, Markus Armbruster wrote:
[...]
>> I appreciate the attempt to curb the spread of DeviceNotFound errors.
>> Two issues:
>>
>> * Copy-pasting find_device_state() with a false argument is an easy
>> error to make.
>>
>> * Most uses of find_device_state() are via blk_by_qdev_id() and
>> qmp_get_blk(). Any new uses of qemu_get_blk() will still produce
>> DeviceNotFound.
>>
>> Hmm.
>
> Hmm. Right. Wait a bit, I can make the change stricter.
>
> Could you still explain (or give a link), why and when we decided to use only
> GenericError? I think, having different "error-codes" is a good thing, why we
> are trying to get rid of it?
We actually got rid of most of them years ago :)
But you deserve a more complete answer.
QMP initially specified the following error response[1]:
2.4.2 error
---
The error response is issued when the command execution could not be
completed because of an error condition.
The format is:
{ "error": { "class": json-string, "data": json-value }, "id": json-value }
Where,
- The "class" member contains the error class name (eg.
"ServiceUnavailable")
- The "data" member contains specific error data and is defined in a
per-command basis, it will be an empty json-object if the error has no
data
- The "id" member contains the transaction identification associated with
the command execution (if issued by the Client)
Note the structure of @data depends on @class. We documented a
command's possible error classes (well, we tried), but never bothered to
document the @data it comes with.
Documentation deficits aside, this is looks quite expressive. There are
issues, though:
1. Formatting errors in human-readable form is bothersome, and creates a
tight coupling between QMP server and client.
Consider:
{"class": "DeviceNotFound", "data": {"device": "ide1-cd0"}}
To format this in human-readable form, you need to know the error.
The server does. Fine print: it has a table mapping JSON templates
to human-readable error message templates.
The client needs to duplicate this somehow. If it receives an error
it doesn't know, all it can do is barf (possibly dressed up) JSON at
the human. To avoid that, clients need to track the server closely:
tight coupling.
2. Errors have notational overhead, which leads to bad errors.
To create a new error, you have to edit two source files (not
counting clients). Strong incentive to reuse existing errors. Even
when they don't quite fit. When a name provided by the user couldn't
be resolved, reusing DeviceNotFound is easier than creating a new
error that is more precise.
3. The human-readable error message is hidden from the programmer's
view, which leads to bad error messages.
At the point in the source where the error is created, we see
something like QERR_DEVICE_NOT_FOUND, name. To see the
human-readable message, we have to look up macro
QERR_DEVICE_NOT_FOUND's error message template in the table, or
actually test (*gasp*) the error. People generally do neither, and
bad error messages proliferate.
4. Too little gain for the pain
Clients rarely want to handle different errors differently. More
often than not, all they do with @class and @data is log them. Only
occasionally do they switch on @class, and basically never on @data.
It me took a considerable fight to get the protocol amended to include a
human-readable message[2]. This addressed issue 1.
Over the next two years or so, issues 2. to 4. slowly sank in. We
eventually tired of the complexity, ripped out @data, and dumbed down
all error classes to GenericError, except for the few clients actually
cared for[3]. We also mandated that new errors avoid the QERR_ macros.
Eliminating the existing QERR_ macros has been slow. We're down to 13
in master, with patches deleting 7 on the list.
This has served us reasonably well.
Questions?
[1] Commit f544d174dfc
QMP: Introduce specification
Dec 2009
[2] Commit 77e595e7c61q
QMP: add human-readable description to error response
Dec 2009
[3] Commit de253f14912
qmp: switch to the new error format on the wire
Aug 2012
Re: [PATCH v4 4/5] qapi: blockdev-backup: add discard-source parameter
Vladimir Sementsov-Ogievskiy writes:
> Add a parameter that enables discard-after-copy. That is mostly useful
> in "push backup with fleecing" scheme, when source is snapshot-access
> format driver node, based on copy-before-write filter snapshot-access
> API:
>
> [guest] [snapshot-access] ~~ blockdev-backup ~~> [backup target]
>||
>| root | file
>vv
> [copy-before-write]
>| |
>| file| target
>v v
> [active disk] [temp.img]
>
> In this case discard-after-copy does two things:
>
> - discard data in temp.img to save disk space
> - avoid further copy-before-write operation in discarded area
>
> Note that we have to declare WRITE permission on source in
> copy-before-write filter, for discard to work. Still we can't take it
> unconditionally, as it will break normal backup from RO source. So, we
> have to add a parameter and pass it thorough bdrv_open flags.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> Reviewed-by: Fiona Ebner
> Tested-by: Fiona Ebner
[...]
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 1874f880a8..2ef52ae9a7 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1610,6 +1610,9 @@
> # node specified by @drive. If this option is not given, a node
> # name is autogenerated. (Since: 4.2)
> #
> +# @discard-source: Discard blocks on source which are already copied
"have been copied"?
> +# to the target. (Since 9.1)
> +#
> # @x-perf: Performance options. (Since 6.0)
> #
> # Features:
> @@ -1631,6 +1634,7 @@
> '*on-target-error': 'BlockdevOnError',
> '*auto-finalize': 'bool', '*auto-dismiss': 'bool',
> '*filter-node-name': 'str',
> + '*discard-source': 'bool',
> '*x-perf': { 'type': 'BackupPerf',
> 'features': [ 'unstable' ] } } }
QAPI schema
Acked-by: Markus Armbruster
Re: [PATCH for 9.0] migration: Skip only empty block devices
Peter Xu writes: > On Tue, Mar 12, 2024 at 05:34:26PM -0400, Stefan Hajnoczi wrote: >> I understand now. I missed that returning from init_blk_migration_it() >> did not abort iteration. Thank you! > > I queued it, thanks both! Thanks for cleaning up the mess I made!
[PATCH 07/10] qapi: Inline and remove QERR_INVALID_PARAMETER_TYPE definition
From: Philippe Mathieu-Daudé
Address the comment added in commit 4629ed1e98
("qerror: Finally unused, clean up"), from 2015:
/*
* These macros will go away, please don't use
* in new code, and do not add new ones!
*/
Manual changes (escaping the format in qapi/visit.py).
Signed-off-by: Philippe Mathieu-Daudé
Signed-off-by: Markus Armbruster
---
include/qapi/qmp/qerror.h | 3 ---
qom/object.c | 4 ++--
scripts/qapi/visit.py | 5 ++---
3 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 63ab775dc5..b723830eff 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -17,9 +17,6 @@
* add new ones!
*/
-#define QERR_INVALID_PARAMETER_TYPE \
-"Invalid parameter type for '%s', expected: %s"
-
#define QERR_INVALID_PARAMETER_VALUE \
"Parameter '%s' expects %s"
diff --git a/qom/object.c b/qom/object.c
index 3d96818f7d..44ec8f6460 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -23,7 +23,6 @@
#include "qapi/qobject-input-visitor.h"
#include "qapi/forward-visitor.h"
#include "qapi/qapi-builtin-visit.h"
-#include "qapi/qmp/qerror.h"
#include "qapi/qmp/qjson.h"
#include "trace.h"
@@ -1912,7 +1911,8 @@ static Object *object_resolve_link(Object *obj, const
char *name,
} else if (!target) {
target = object_resolve_path(path, );
if (target || ambiguous) {
-error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name, target_type);
+error_setg(errp, "Invalid parameter type for '%s', expected: %s",
+ name, target_type);
} else {
error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
"Device '%s' not found", path);
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index c56ea4d724..a21b7b1468 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -278,8 +278,8 @@ def gen_visit_alternate(name: str, variants:
QAPISchemaVariants) -> str:
abort();
default:
assert(visit_is_input(v));
-error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
- "%(name)s");
+error_setg(errp, "Invalid parameter type for '%%s', expected:
%(name)s",
+ name ? name : "null");
/* Avoid passing invalid *obj to qapi_free_%(c_name)s() */
g_free(*obj);
*obj = NULL;
@@ -356,7 +356,6 @@ def _begin_user_module(self, name: str) -> None:
self._genc.preamble_add(mcgen('''
#include "qemu/osdep.h"
#include "qapi/error.h"
-#include "qapi/qmp/qerror.h"
#include "%(visit)s.h"
''',
visit=visit))
--
2.44.0
[PATCH 06/10] qapi: Inline QERR_INVALID_PARAMETER_TYPE definition (constant value)
From: Philippe Mathieu-Daudé
Address the comment added in commit 4629ed1e98
("qerror: Finally unused, clean up"), from 2015:
/*
* These macros will go away, please don't use
* in new code, and do not add new ones!
*/
Mechanical transformation using the following
coccinelle semantic patch:
@match@
expression errp;
expression param;
constant value;
@@
error_setg(errp, QERR_INVALID_PARAMETER_TYPE, param, value);
@script:python strformat depends on match@
value << match.value;
fixedfmt; // new var
@@
fixedfmt = f'"Invalid parameter type for \'%s\', expected: {value[1:-1]}"'
coccinelle.fixedfmt = cocci.make_ident(fixedfmt)
@replace@
expression match.errp;
expression match.param;
constant match.value;
identifier strformat.fixedfmt;
@@
-error_setg(errp, QERR_INVALID_PARAMETER_TYPE, param, value);
+error_setg(errp, fixedfmt, param);
Signed-off-by: Philippe Mathieu-Daudé
Signed-off-by: Markus Armbruster
---
qapi/qobject-input-visitor.c | 32
qapi/string-input-visitor.c | 8
qom/object.c | 12
3 files changed, 28 insertions(+), 24 deletions(-)
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 3e8aca6b15..f110a804b2 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -288,8 +288,8 @@ static bool qobject_input_start_struct(Visitor *v, const
char *name, void **obj,
return false;
}
if (qobject_type(qobj) != QTYPE_QDICT) {
-error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
- full_name(qiv, name), "object");
+error_setg(errp, "Invalid parameter type for '%s', expected: object",
+ full_name(qiv, name));
return false;
}
@@ -326,8 +326,8 @@ static bool qobject_input_start_list(Visitor *v, const char
*name,
return false;
}
if (qobject_type(qobj) != QTYPE_QLIST) {
-error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
- full_name(qiv, name), "array");
+error_setg(errp, "Invalid parameter type for '%s', expected: array",
+ full_name(qiv, name));
return false;
}
@@ -405,8 +405,8 @@ static bool qobject_input_type_int64(Visitor *v, const char
*name, int64_t *obj,
}
qnum = qobject_to(QNum, qobj);
if (!qnum || !qnum_get_try_int(qnum, obj)) {
-error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
- full_name(qiv, name), "integer");
+error_setg(errp, "Invalid parameter type for '%s', expected: integer",
+ full_name(qiv, name));
return false;
}
return true;
@@ -494,8 +494,8 @@ static bool qobject_input_type_bool(Visitor *v, const char
*name, bool *obj,
}
qbool = qobject_to(QBool, qobj);
if (!qbool) {
-error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
- full_name(qiv, name), "boolean");
+error_setg(errp, "Invalid parameter type for '%s', expected: boolean",
+ full_name(qiv, name));
return false;
}
@@ -534,8 +534,8 @@ static bool qobject_input_type_str(Visitor *v, const char
*name, char **obj,
}
qstr = qobject_to(QString, qobj);
if (!qstr) {
-error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
- full_name(qiv, name), "string");
+error_setg(errp, "Invalid parameter type for '%s', expected: string",
+ full_name(qiv, name));
return false;
}
@@ -565,8 +565,8 @@ static bool qobject_input_type_number(Visitor *v, const
char *name, double *obj,
}
qnum = qobject_to(QNum, qobj);
if (!qnum) {
-error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
- full_name(qiv, name), "number");
+error_setg(errp, "Invalid parameter type for '%s', expected: number",
+ full_name(qiv, name));
return false;
}
@@ -587,8 +587,8 @@ static bool qobject_input_type_number_keyval(Visitor *v,
const char *name,
if (qemu_strtod_finite(str, NULL, )) {
/* TODO report -ERANGE more nicely */
-error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
- full_name(qiv, name), "number");
+error_setg(errp, "Invalid parameter type for '%s', expected: number",
+ full_name(qiv, name));
return false;
}
@@ -623,8 +623,8 @@ static bool qobject_input_type_null(Visitor *v, const char
*name,
}
if (qobject_type(qobj) != QTYPE_QNULL) {
-error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
- full_name(qiv, name), "null");
+error_setg(errp, "Invalid parameter type for '%s', expected: null&qu
[PATCH 00/10] Reduce usage of QERR_ macros further
Philippe posted "[PATCH v2 00/22] qapi: Kill 'qapi/qmp/qerror.h' for good" a couple of months ago. I cherry-picked just its simplest parts for now. Markus Armbruster (1): error: Drop superfluous #include "qapi/qmp/qerror.h" Philippe Mathieu-Daudé (9): qapi: Inline and remove QERR_BUS_NO_HOTPLUG definition qapi: Inline and remove QERR_DEVICE_HAS_NO_MEDIUM definition qapi: Inline and remove QERR_DEVICE_NO_HOTPLUG definition qapi: Inline and remove QERR_INVALID_PARAMETER definition qapi: Inline QERR_INVALID_PARAMETER_TYPE definition (constant value) qapi: Inline and remove QERR_INVALID_PARAMETER_TYPE definition qapi: Correct error message for 'vcpu_dirty_limit' parameter qapi: Inline and remove QERR_MIGRATION_ACTIVE definition qapi: Inline and remove QERR_PROPERTY_VALUE_BAD definition include/qapi/qmp/qerror.h| 21 - backends/iommufd.c | 1 - block/snapshot.c | 7 --- blockdev.c | 2 +- chardev/char-fe.c| 1 - hw/core/qdev-properties.c| 3 +-- hw/core/qdev.c | 4 ++-- hw/ppc/spapr_pci.c | 5 ++--- migration/migration.c| 2 +- migration/options.c | 9 - migration/savevm.c | 2 +- qapi/opts-visitor.c | 2 +- qapi/qobject-input-visitor.c | 32 qapi/string-input-visitor.c | 8 qom/object.c | 16 ++-- system/qdev-monitor.c| 10 ++ system/rtc.c | 1 - util/qemu-option.c | 10 +- scripts/qapi/visit.py| 5 ++--- 19 files changed, 60 insertions(+), 81 deletions(-) -- 2.44.0
[PATCH 02/10] qapi: Inline and remove QERR_BUS_NO_HOTPLUG definition
From: Philippe Mathieu-Daudé
Address the comment added in commit 4629ed1e98
("qerror: Finally unused, clean up"), from 2015:
/*
* These macros will go away, please don't use
* in new code, and do not add new ones!
*/
Mechanical transformation using sed, and manual cleanup.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Signed-off-by: Markus Armbruster
---
include/qapi/qmp/qerror.h | 3 ---
hw/ppc/spapr_pci.c| 5 ++---
system/qdev-monitor.c | 8 +---
3 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 0c2689cf8a..06a1d2248e 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -17,9 +17,6 @@
* add new ones!
*/
-#define QERR_BUS_NO_HOTPLUG \
-"Bus '%s' does not support hotplugging"
-
#define QERR_DEVICE_HAS_NO_MEDIUM \
"Device '%s' has no medium"
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 25e0295d6f..72cfba419a 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -39,7 +39,6 @@
#include "trace.h"
#include "qemu/error-report.h"
#include "qemu/module.h"
-#include "qapi/qmp/qerror.h"
#include "hw/ppc/fdt.h"
#include "hw/pci/pci_bridge.h"
#include "hw/pci/pci_bus.h"
@@ -1554,7 +1553,7 @@ static void spapr_pci_pre_plug(HotplugHandler
*plug_handler,
* we need to let them know it's not enabled
*/
if (plugged_dev->hotplugged) {
-error_setg(errp, QERR_BUS_NO_HOTPLUG,
+error_setg(errp, "Bus '%s' does not support hotplugging",
phb->parent_obj.bus->qbus.name);
return;
}
@@ -1675,7 +1674,7 @@ static void spapr_pci_unplug_request(HotplugHandler
*plug_handler,
SpaprDrc *drc = drc_from_dev(phb, pdev);
if (!phb->dr_enabled) {
-error_setg(errp, QERR_BUS_NO_HOTPLUG,
+error_setg(errp, "Bus '%s' does not support hotplugging",
phb->parent_obj.bus->qbus.name);
return;
}
diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index 09e07cab9b..842c142c79 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -661,7 +661,8 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
if (qdev_should_hide_device(opts, from_json, errp)) {
if (bus && !qbus_is_hotpluggable(bus)) {
-error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
+error_setg(errp, "Bus '%s' does not support hotplugging",
+ bus->name);
}
return NULL;
} else if (*errp) {
@@ -669,7 +670,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
}
if (phase_check(PHASE_MACHINE_READY) && bus && !qbus_is_hotpluggable(bus))
{
-error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
+error_setg(errp, "Bus '%s' does not support hotplugging", bus->name);
return NULL;
}
@@ -911,7 +912,8 @@ void qdev_unplug(DeviceState *dev, Error **errp)
}
if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) {
-error_setg(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
+error_setg(errp, "Bus '%s' does not support hotplugging",
+ dev->parent_bus->name);
return;
}
--
2.44.0
[PATCH 04/10] qapi: Inline and remove QERR_DEVICE_NO_HOTPLUG definition
From: Philippe Mathieu-Daudé
Address the comment added in commit 4629ed1e98
("qerror: Finally unused, clean up"), from 2015:
/*
* These macros will go away, please don't use
* in new code, and do not add new ones!
*/
Mechanical transformation using sed, and manual cleanup.
Signed-off-by: Philippe Mathieu-Daudé
Signed-off-by: Markus Armbruster
---
include/qapi/qmp/qerror.h | 3 ---
hw/core/qdev.c| 4 ++--
system/qdev-monitor.c | 2 +-
3 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index daa889809b..e93211085a 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -17,9 +17,6 @@
* add new ones!
*/
-#define QERR_DEVICE_NO_HOTPLUG \
-"Device '%s' does not support hotplugging"
-
#define QERR_INVALID_PARAMETER \
"Invalid parameter '%s'"
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index c68d0f7c51..00efaf1bd1 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -29,7 +29,6 @@
#include "qapi/error.h"
#include "qapi/qapi-events-qdev.h"
#include "qapi/qmp/qdict.h"
-#include "qapi/qmp/qerror.h"
#include "qapi/visitor.h"
#include "qemu/error-report.h"
#include "qemu/option.h"
@@ -479,7 +478,8 @@ static void device_set_realized(Object *obj, bool value,
Error **errp)
static int unattached_count;
if (dev->hotplugged && !dc->hotpluggable) {
-error_setg(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));
+error_setg(errp, "Device '%s' does not support hotplugging",
+ object_get_typename(obj));
return;
}
diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index 842c142c79..e2eea7d96e 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -918,7 +918,7 @@ void qdev_unplug(DeviceState *dev, Error **errp)
}
if (!dc->hotpluggable) {
-error_setg(errp, QERR_DEVICE_NO_HOTPLUG,
+error_setg(errp, "Device '%s' does not support hotplugging",
object_get_typename(OBJECT(dev)));
return;
}
--
2.44.0
[PATCH 08/10] qapi: Correct error message for 'vcpu_dirty_limit' parameter
From: Philippe Mathieu-Daudé
QERR_INVALID_PARAMETER_VALUE is defined as:
#define QERR_INVALID_PARAMETER_VALUE \
"Parameter '%s' expects %s"
The current error is formatted as:
"Parameter 'vcpu_dirty_limit' expects is invalid, it must greater then 1 MB/s"
Replace by:
"Parameter 'vcpu_dirty_limit' is invalid, it must greater than 1 MB/s"
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Juan Quintela
Signed-off-by: Markus Armbruster
---
migration/options.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/migration/options.c b/migration/options.c
index 40eb930940..c5115f1b5c 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -1264,9 +1264,8 @@ bool migrate_params_check(MigrationParameters *params,
Error **errp)
if (params->has_vcpu_dirty_limit &&
(params->vcpu_dirty_limit < 1)) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
- "vcpu_dirty_limit",
- "is invalid, it must greater then 1 MB/s");
+error_setg(errp, "Parameter 'vcpu_dirty_limit' is invalid,"
+ " it must greater than 1 MB/s");
return false;
}
--
2.44.0
[PATCH 03/10] qapi: Inline and remove QERR_DEVICE_HAS_NO_MEDIUM definition
From: Philippe Mathieu-Daudé
Address the comment added in commit 4629ed1e98
("qerror: Finally unused, clean up"), from 2015:
/*
* These macros will go away, please don't use
* in new code, and do not add new ones!
*/
Mechanical transformation using sed, and manual cleanup.
Signed-off-by: Philippe Mathieu-Daudé
Signed-off-by: Markus Armbruster
---
include/qapi/qmp/qerror.h | 3 ---
block/snapshot.c | 7 ---
blockdev.c| 2 +-
3 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 06a1d2248e..daa889809b 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -17,9 +17,6 @@
* add new ones!
*/
-#define QERR_DEVICE_HAS_NO_MEDIUM \
-"Device '%s' has no medium"
-
#define QERR_DEVICE_NO_HOTPLUG \
"Device '%s' does not support hotplugging"
diff --git a/block/snapshot.c b/block/snapshot.c
index 8694fc0a3e..e2c18d3f8f 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -28,7 +28,6 @@
#include "block/qdict.h"
#include "qapi/error.h"
#include "qapi/qmp/qdict.h"
-#include "qapi/qmp/qerror.h"
#include "qapi/qmp/qstring.h"
#include "qemu/option.h"
#include "sysemu/block-backend.h"
@@ -359,7 +358,8 @@ int bdrv_snapshot_delete(BlockDriverState *bs,
GLOBAL_STATE_CODE();
if (!drv) {
-error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));
+error_setg(errp, "Device '%s' has no medium",
+ bdrv_get_device_name(bs));
return -ENOMEDIUM;
}
if (!snapshot_id && !name) {
@@ -437,7 +437,8 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
GRAPH_RDLOCK_GUARD_MAINLOOP();
if (!drv) {
-error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));
+error_setg(errp, "Device '%s' has no medium",
+ bdrv_get_device_name(bs));
return -ENOMEDIUM;
}
if (!snapshot_id && !name) {
diff --git a/blockdev.c b/blockdev.c
index d8fb3399f5..bd408e3e75 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1395,7 +1395,7 @@ static void external_snapshot_action(TransactionAction
*action,
bdrv_drained_begin(state->old_bs);
if (!bdrv_is_inserted(state->old_bs)) {
-error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
+error_setg(errp, "Device '%s' has no medium", device);
return;
}
--
2.44.0
[PATCH 10/10] qapi: Inline and remove QERR_PROPERTY_VALUE_BAD definition
From: Philippe Mathieu-Daudé
Address the comment added in commit 4629ed1e98
("qerror: Finally unused, clean up"), from 2015:
/*
* These macros will go away, please don't use
* in new code, and do not add new ones!
*/
Manual change. Remove the definition in
include/qapi/qmp/qerror.h.
Signed-off-by: Philippe Mathieu-Daudé
Signed-off-by: Markus Armbruster
---
include/qapi/qmp/qerror.h | 3 ---
hw/core/qdev-properties.c | 3 +--
2 files changed, 1 insertion(+), 5 deletions(-)
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 385a4876d6..00b18e9082 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -26,9 +26,6 @@
#define QERR_MISSING_PARAMETER \
"Parameter '%s' is missing"
-#define QERR_PROPERTY_VALUE_BAD \
-"Property '%s.%s' doesn't take value '%s'"
-
#define QERR_PROPERTY_VALUE_OUT_OF_RANGE \
"Property %s.%s doesn't take value %" PRId64 " (minimum: %" PRId64 ",
maximum: %" PRId64 ")"
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 7d6fa726fd..86a583574d 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -2,7 +2,6 @@
#include "hw/qdev-properties.h"
#include "qapi/error.h"
#include "qapi/qapi-types-misc.h"
-#include "qapi/qmp/qerror.h"
#include "qapi/qmp/qlist.h"
#include "qemu/ctype.h"
#include "qemu/error-report.h"
@@ -792,7 +791,7 @@ void error_set_from_qdev_prop_error(Error **errp, int ret,
Object *obj,
break;
default:
case -EINVAL:
-error_setg(errp, QERR_PROPERTY_VALUE_BAD,
+error_setg(errp, "Property '%s.%s' doesn't take value '%s'",
object_get_typename(obj), name, value);
break;
case -ENOENT:
--
2.44.0
[PATCH 05/10] qapi: Inline and remove QERR_INVALID_PARAMETER definition
From: Philippe Mathieu-Daudé
Address the comment added in commit 4629ed1e98
("qerror: Finally unused, clean up"), from 2015:
/*
* These macros will go away, please don't use
* in new code, and do not add new ones!
*/
Mechanical transformation using:
$ sed -i -e "s/QERR_INVALID_PARAMETER,/\"Invalid parameter '%s'\",/" \
$(git grep -lw QERR_INVALID_PARAMETER)
Manually simplify qemu_opts_create(), and remove the macro definition
in include/qapi/qmp/qerror.h.
Signed-off-by: Philippe Mathieu-Daudé
Signed-off-by: Markus Armbruster
---
include/qapi/qmp/qerror.h | 3 ---
qapi/opts-visitor.c | 2 +-
util/qemu-option.c| 10 +-
3 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index e93211085a..63ab775dc5 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -17,9 +17,6 @@
* add new ones!
*/
-#define QERR_INVALID_PARAMETER \
-"Invalid parameter '%s'"
-
#define QERR_INVALID_PARAMETER_TYPE \
"Invalid parameter type for '%s', expected: %s"
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 8f1efab8b9..3d1a28b419 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -184,7 +184,7 @@ opts_check_struct(Visitor *v, Error **errp)
const QemuOpt *first;
first = g_queue_peek_head(any);
-error_setg(errp, QERR_INVALID_PARAMETER, first->name);
+error_setg(errp, "Invalid parameter '%s'", first->name);
return false;
}
return true;
diff --git a/util/qemu-option.c b/util/qemu-option.c
index eedd08929b..201f7a87f3 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -498,7 +498,7 @@ static bool opt_validate(QemuOpt *opt, Error **errp)
desc = find_desc_by_name(list->desc, opt->name);
if (!desc && !opts_accepts_any(list)) {
-error_setg(errp, QERR_INVALID_PARAMETER, opt->name);
+error_setg(errp, "Invalid parameter '%s'", opt->name);
return false;
}
@@ -531,7 +531,7 @@ bool qemu_opt_set_bool(QemuOpts *opts, const char *name,
bool val,
desc = find_desc_by_name(list->desc, name);
if (!desc && !opts_accepts_any(list)) {
-error_setg(errp, QERR_INVALID_PARAMETER, name);
+error_setg(errp, "Invalid parameter '%s'", name);
return false;
}
@@ -554,7 +554,7 @@ bool qemu_opt_set_number(QemuOpts *opts, const char *name,
int64_t val,
desc = find_desc_by_name(list->desc, name);
if (!desc && !opts_accepts_any(list)) {
-error_setg(errp, QERR_INVALID_PARAMETER, name);
+error_setg(errp, "Invalid parameter '%s'", name);
return false;
}
@@ -612,7 +612,7 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char
*id,
if (list->merge_lists) {
if (id) {
-error_setg(errp, QERR_INVALID_PARAMETER, "id");
+error_setg(errp, "Invalid parameter 'id'");
return NULL;
}
opts = qemu_opts_find(list, NULL);
@@ -1103,7 +1103,7 @@ bool qemu_opts_validate(QemuOpts *opts, const QemuOptDesc
*desc, Error **errp)
QTAILQ_FOREACH(opt, >head, next) {
opt->desc = find_desc_by_name(desc, opt->name);
if (!opt->desc) {
-error_setg(errp, QERR_INVALID_PARAMETER, opt->name);
+error_setg(errp, "Invalid parameter '%s'", opt->name);
return false;
}
--
2.44.0
