Re: [PATCH 20/20] qapi: convert "Example" sections to rST
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(-)
>
> 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.
>
We can create a custom directive that assumes a default caption; e.g.
.. qapi:example::
... blah blah blah ...
And if you want to override it, you'd just
.. qapi:example::
:caption: Lorem Ipsum ...
... blah blah blah ...
Interested? (I kept this patch vanilla to avoid getting fancy, but there
are fanciness options available if you'd like them.)
>
> > #
> > # -> { "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.
>
We can change that with styling
Re: [PATCH 19/20] qapi: convert "Note" sections to plain rST
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:", "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 > > Done. > > > > 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? > Retrieve paragraph from L1 cache. Nope!
Re: [PATCH 18/20] qapi: ensure all errors sections are uniformly typset
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. > > Signed-off-by: John Snow > >
Re: [PATCH 14/20] qapi: fix non-compliant JSON examples
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 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.))) > > 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. 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 13/20] docs/qapidoc: fix nested parsing under untagged sections
On Fri, Jun 14, 2024, 5:46 AM Markus Armbruster wrote:
> 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?
>
Yes - this is necessary for captioned example blocks that appear in
untagged sections, because those have titles.
When the sphinx html writer encounters a title under a section without a
title field, it malforms the tree (I cannot give you an example of this
easily, it's deep in the bowels) and produces an assertion error.
If you want to see it explode for yourself, just modify any untagged
section to include a captioned codeblock and watch it die.
If you apply either the note or Example conversion patches without this
fix, the old generator will choke. (Note patch dies because of my use of
".. admonition:: Notes", which also creates a title element.)
Simply put - docutils can tolerate title-less sections, Sphinx cannot. (And
it is not graceful about it.)
> > 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.
>
Recommend just observing a before/after; the hash changes but the output
doesn't meaningfully change.
I intend to remove the old generator when we're done, so I think this is
probably safe to wave through with an ACK so long as there isn't
tremendously obvious regression (And, I have tested these patches from 3.x
to 7.x so I do not believe there is any compat risk.)
>
Re: [PATCH 11/20] qapi/schema: add doc_visible property to QAPISchemaDefinition
On Fri, Jun 14, 2024, 5:40 AM Markus Armbruster wrote: > 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? > Yes. If a type is only ever used as a branch type or a base type and never named as a field type, it can be safely omitted from the rendered docs. (If a type is not named in any fashion by and command or event or recursive structure therein, it's an internal type not used in QMP at all and can also be excluded safely.) > > 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. > What I mean to say is that anywhere in the generated documentation where we need to cross-reference a type name should be "doc visible". This means, currently: - All commands - All events - All member field types (including branch field types) of commands/events or nested fields therein (i.e. CommandA takes an arg "foo" of type "FooA" which in turn has a field "bar" of type "BarB"; CommandA, FooA and BarB are all doc visible types.) - All return types and nested types therein; identical to the input rules above (You've expressed a desire to change this. Future work.) - All alternate types (which are never inlined but instead are referenced. This can be changed later but it's just not how it works currently.) but excludes: - Arg types for events/commands - Inherited object types - Any data type that does not appear as a field/member type for a command/event input/output - Array/List types: only the primitive types are documented with a section, not the List/Array container. This is in contrast to Alternates, where we do not perform a similar destructuring. So, if you look at the WIP render on gitlab.io, any (type) parenthetical links to a "qapi object" (Sphinx parlance: an indexable qapi domain *thingie*) that exists as its own section with a name and docs attached. Those are the ones that are "doc visible". This differs from introspection in that some (but not all) of the shared types get sections in the HTML output, but these are not API and not inherently stable with regards to their name(s) or factorings. Introspection by contrast strips more of the names out than we do here. > > 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? > Not quite; in the generator, we visit only entities that *have docs*, so the true visibility check is (pseudocode): ent.doc_visible() && ent.has_docs() but we guarantee this does not cull anything inappropriate because any hanging reference is a warning that will cause a build failure. This should not be possible to trigger in the doc builder under normal circumstances. I just mean to say: "I believe the algorithm in this patch is correct because the build would not tolerate a hanging reference, and the doc generator does in fact succeed." Notably, the doc_visible algorithm may mark some built-in types as "visible", but they're still excluded from the html render because they simply have no docs to show. (The reference checker will not yelp for these built-in types.) > > Signed-off-by: John Snow > >
Re: [PATCH 09/20] qapi/parser: add undocumented stub members to all_sections
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.)
>
> 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 insertion will place the member documentation
prior to that section.
The loop doesn't break when it finds intro-paragraph or members, so it'll
continue to tick upwards until it reaches the end of the list or it finds
something disqualifying.
>
> 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
>
Wellp, yeah. That's certainly less code :)
I tossed in your algorithm alongside mine and asserted they were always
equal, and they are, so... yup. I think the only possible concern here is
if there is precisely one and only one section and 1 is beyond EOL, but
that's easy to fix. It apparently doesn't happen in practice, but I can't
presently imagine why it *couldn't* happen.
Re: [PATCH 07/20] qapi/parser: add semantic 'kind' parameter to QAPIDoc.Section
On Thu, Jun 13, 2024 at 10:46 AM Markus Armbruster
wrote:
> 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.
>
OK, knock yourself out :)
>
> > 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, which
> we
> > ignore.
> >
> > Uhm, can I name it paragraphs? :) or open to other suggestions, incl.
> > untagged if that's still your preference.
>
> 'untagged' is more consistent with existing names and comments:
> .ensure_untagged_section(), "additional (non-argument) sections,
> possibly tagged", ...
>
> When all tags but 'TODO'
Re: [PATCH 06/20] qapi/parser: fix comment parsing immediately following a doc block
On Thu, Jun 13, 2024 at 10:32 AM Markus Armbruster
wrote:
> 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:
> #
>
Okey-dokey, all set.
--js
Re: [PATCH RESEND v7 00/12] vhost-user: support any POSIX system (tested on macOS, FreeBSD, OpenBSD)
On Mon, Jun 17, 2024 at 09:42:21AM GMT, Michael S. Tsirkin wrote: On Mon, Jun 17, 2024 at 02:59:14PM +0200, Stefano Garzarella wrote: On Mon, Jun 17, 2024 at 02:02:30PM GMT, Markus Armbruster wrote: > 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> Yep! > > > 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. > I can also include it in v8 if it helps. Thanks, Stefano If I'm to merge it, pls do. Much less error prone. Okay, I'll include it in v8. I'll wait until tomorrow to see if there's any objection on the tree, but I think yours is the most suitable. Thanks, Stefano
Re: [PATCH RESEND v7 00/12] vhost-user: support any POSIX system (tested on macOS, FreeBSD, OpenBSD)
On Mon, Jun 17, 2024 at 02:59:14PM +0200, Stefano Garzarella wrote: > On Mon, Jun 17, 2024 at 02:02:30PM GMT, Markus Armbruster wrote: > > 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> > > Yep! > > > > > > 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. > > > > I can also include it in v8 if it helps. > > Thanks, > Stefano If I'm to merge it, pls do. Much less error prone. -- MST
Re: [PATCH RESEND v7 00/12] vhost-user: support any POSIX system (tested on macOS, FreeBSD, OpenBSD)
On Mon, Jun 17, 2024 at 02:02:30PM GMT, Markus Armbruster wrote: 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> Yep! 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. I can also include it in v8 if it helps. Thanks, Stefano
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 RESEND v7 00/12] vhost-user: support any POSIX system (tested on macOS, FreeBSD, OpenBSD)
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)? In the last case I would ask you to queue up the first 9 patches of this series if that is okay with you. Thanks, Stefano Thanks, Stefano Changelog v1: https://patchew.org/QEMU/20240228114759.44758-1-sgarz...@redhat.com/ v2: https://patchew.org/QEMU/20240326133936.125332-1-sgarz...@redhat.com/ v3: https://patchew.org/QEMU/20240404122330.92710-1-sgarz...@redhat.com/ v4: https://patchew.org/QEMU/20240508074457.12367-1-sgarz...@redhat.com/ v5: https://patchew.org/QEMU/20240523145522.313012-1-sgarz...@redhat.com/ v6: https://patchew.org/QEMU/20240528103543.145412-1-sgarz...@redhat.com/ v7: - rebased on https://patchew.org/QEMU/20240611130231.83152-1-sgarz...@redhat.com/ That patch is queued by Markus and only Patch 10 of this series depends on it. - changed default value documentation for @share [Markus] - used `memory-backend-shm` instead of `shm` and wrapped to 70 columns [Markus] - added 'if': 'CONFIG_POSIX' to MemoryBackendShmProperties [Markus] Description The vhost-user protocol is not really Linux-specific, so let's try support QEMU's frontends and backends (including libvhost-user) in any POSIX system with this series. The main use case is to be able to use virtio devices that we don't have built-in in QEMU (e.g. virtiofsd, vhost-user-vsock, etc.) even in non-Linux systems. The first 5 patches are more like fixes discovered at runtime on macOS or FreeBSD that could go even independently of this series. Patches 6, 7, 8, 9 enable building of frontends and backends (including libvhost-user) with associated code changes to succeed in compilation. Patch 10 adds `memory-backend-shm` that uses the POSIX shm_open() API to create shared memory which is identified by an fd that can be shared with vhost-user backends. This is useful on those systems (like macOS) where we don't have memfd_create() or special filesystems like "/dev/shm". Patches 11 and 12 use `memory-backend-shm` in some vhost-user tests. Maybe the first 5 patches can go separately, but I only discovered those problems after testing patches 6 - 9, so I have included them in this series for now. Please let me know if you prefer that I send them separately. I tested this series using vhost-user-blk and QSD on macOS Sonoma 14.4 (aarch64), FreeBSD 14 (x86_64), OpenBSD 7.4 (x86_64), and Fedora 40 (x86_64) in this way: - Start vhost-user-blk or QSD (same commands for all systems) vhost-user-blk -s /tmp/vhost.socket \ -b Fedora-Cloud-Base-39-1.5.x86_64.raw qemu-storage-daemon \ --blockdev file,filename=Fedora-Cloud-Base-39-1.5.x86_64.qcow2,node-name=file \ --blockdev qcow2,file=file,node-name=qcow2 \ --export vhost-user-blk,addr.type=unix,addr.path=/tmp/vhost.socket,id=vub,num-queues=1,node-name=qcow2,writable=on - macOS (aarch64): start QEMU (using hvf accelerator) qemu-system-aarch64 -smp 2 -cpu host -M virt,accel=hvf,memory-backend=mem \ -drive file=./build/pc-bios/edk2-aarch64-code.fd,if=pflash,format=raw,readonly=on \ -device virtio-net-device,netdev=net0 -netdev user,id=net0 \ -device ramfb -device usb-ehci -device usb-kbd \ -object memory-backend-shm,id=mem,size=512M \ -device vhost-user-blk-pci,num-queues=1,disable-legacy=on,chardev=char0 \ -chardev socket,id=char0,path=/tmp/vhost.socket - FreeBSD/OpenBSD (x86_64): start QEMU (no accelerators available) qemu-system-x86_64 -smp 2 -M q35,memory-backend=mem \ -object memory-backend-shm,id=mem,size="512M" \ -device vhost-user-blk-pci,num-queues=1,chardev=char0 \ -chardev socket,id=char0,path=/tmp/vhost.socket - Fedora (x86_64): start QEMU (using kvm accelerator) qemu-system-x86_64 -smp 2 -M q35,accel=kvm,memory-backend=mem \ -object memory-backend-shm,size="512M" \ -device vhost-user-blk-pci,num-queues=1,chardev=char0 \ -chardev socket,id=char0,path=/tmp/vhost.socket Branch pushed (and CI started) at https://gitlab.com/sgarzarella/qemu/-/tree/macos-vhost-user?ref_type=heads Based-on: 20240611130231.83152-1-sgarz...@redhat.com Stefano Garzarella (12): libvhost-user: set msg.msg_control to NULL when it is empty libvhost-user: fail vu_message_write() if sendmsg() is failing libvhost-user: mask F_INFLIGHT_SHMFD if memfd is not supported vhost-user-server: do not set memory fd non-blocking contrib/vhost-user-blk: fix bind() using the right size of the address contrib/vhost-user-*: use QEMU bswap helper functions vhost-user: enable frontends on any POSIX system libvhost-user: enable it on any POSIX system contrib/vhost-user-blk: enable it on any POSIX system hostmem: add a new memory backend based on POSIX
Re: [PATCH v2 00/11] qcow2: make subclusters discardable
On 6/10/24 11:53 AM, Andrey Drobyshev wrote: > On 6/3/24 12:19 PM, Andrey Drobyshev wrote: >> On 5/13/24 9:31 AM, Andrey Drobyshev wrote: >>> v1: https://lists.nongnu.org/archive/html/qemu-devel/2023-10/msg07223.html >>> >>> Andrey Drobyshev (11): >>> qcow2: make function update_refcount_discard() global >>> qcow2: simplify L2 entries accounting for discard-no-unref >>> qcow2: put discard requests in the common queue when discard-no-unref >>> enabled >>> block/file-posix: add trace event for fallocate() calls >>> iotests/common.rc: add disk_usage function >>> iotests/290: add test case to check 'discard-no-unref' option behavior >>> qcow2: add get_sc_range_info() helper for working with subcluster >>> ranges >>> qcow2: zeroize the entire cluster when there're no non-zero >>> subclusters >>> qcow2: make subclusters discardable >>> qcow2: zero_l2_subclusters: fall through to discard operation when >>> requested >>> iotests/271: add test cases for subcluster-based discard/unmap >>> >>> block/file-posix.c | 1 + >>> block/qcow2-cluster.c| 346 --- >>> block/qcow2-refcount.c | 8 +- >>> block/qcow2-snapshot.c | 6 +- >>> block/qcow2.c| 25 +-- >>> block/qcow2.h| 6 +- >>> block/trace-events | 1 + >>> tests/qemu-iotests/250 | 5 - >>> tests/qemu-iotests/271 | 72 ++-- >>> tests/qemu-iotests/271.out | 69 ++- >>> tests/qemu-iotests/290 | 34 >>> tests/qemu-iotests/290.out | 28 +++ >>> tests/qemu-iotests/common.rc | 6 + >>> 13 files changed, 490 insertions(+), 117 deletions(-) >>> >> >> Friendly ping > > Ping Friendly ping
