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
[PATCH 09/10] qapi: Inline and remove QERR_MIGRATION_ACTIVE 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, manually
removing the definition in include/qapi/qmp/qerror.h.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Juan Quintela
Signed-off-by: Markus Armbruster
---
include/qapi/qmp/qerror.h | 3 ---
migration/migration.c | 2 +-
migration/options.c | 4 ++--
migration/savevm.c| 2 +-
4 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index b723830eff..385a4876d6 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -23,9 +23,6 @@
#define QERR_IO_ERROR \
"An IO error has occurred"
-#define QERR_MIGRATION_ACTIVE \
-"There's a migration process in progress"
-
#define QERR_MISSING_PARAMETER \
"Parameter '%s' is missing"
diff --git a/migration/migration.c b/migration/migration.c
index a49fcd53ee..e4a68dfb62 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1961,7 +1961,7 @@ static bool migrate_prepare(MigrationState *s, bool blk,
bool blk_inc,
}
if (migration_is_running(s->state)) {
-error_setg(errp, QERR_MIGRATION_ACTIVE);
+error_setg(errp, "There's a migration process in progress");
return false;
}
diff --git a/migration/options.c b/migration/options.c
index c5115f1b5c..5a446e8925 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -682,7 +682,7 @@ bool migrate_cap_set(int cap, bool value, Error **errp)
bool new_caps[MIGRATION_CAPABILITY__MAX];
if (migration_is_running(s->state)) {
-error_setg(errp, QERR_MIGRATION_ACTIVE);
+error_setg(errp, "There's a migration process in progress");
return false;
}
@@ -726,7 +726,7 @@ void
qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
bool new_caps[MIGRATION_CAPABILITY__MAX];
if (migration_is_running(s->state) || migration_in_colo_state()) {
-error_setg(errp, QERR_MIGRATION_ACTIVE);
+error_setg(errp, "There's a migration process in progress");
return;
}
diff --git a/migration/savevm.c b/migration/savevm.c
index dc1fb9c0d3..19ca297e15 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1706,7 +1706,7 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
MigrationStatus status;
if (migration_is_running(ms->state)) {
-error_setg(errp, QERR_MIGRATION_ACTIVE);
+error_setg(errp, "There's a migration process in progress");
return -EINVAL;
}
--
2.44.0
[PATCH 01/10] error: Drop superfluous #include "qapi/qmp/qerror.h"
Signed-off-by: Markus Armbruster --- backends/iommufd.c | 1 - chardev/char-fe.c | 1 - system/rtc.c | 1 - 3 files changed, 3 deletions(-) diff --git a/backends/iommufd.c b/backends/iommufd.c index 1ef683c7b0..922d75e49e 100644 --- a/backends/iommufd.c +++ b/backends/iommufd.c @@ -13,7 +13,6 @@ #include "qemu/osdep.h" #include "sysemu/iommufd.h" #include "qapi/error.h" -#include "qapi/qmp/qerror.h" #include "qemu/module.h" #include "qom/object_interfaces.h" #include "qemu/error-report.h" diff --git a/chardev/char-fe.c b/chardev/char-fe.c index 66cee8475a..b214ba3802 100644 --- a/chardev/char-fe.c +++ b/chardev/char-fe.c @@ -24,7 +24,6 @@ #include "qemu/osdep.h" #include "qemu/error-report.h" #include "qapi/error.h" -#include "qapi/qmp/qerror.h" #include "sysemu/replay.h" #include "chardev/char-fe.h" diff --git a/system/rtc.c b/system/rtc.c index 4904581abe..dc44576686 100644 --- a/system/rtc.c +++ b/system/rtc.c @@ -25,7 +25,6 @@ #include "qemu/osdep.h" #include "qemu/cutils.h" #include "qapi/error.h" -#include "qapi/qmp/qerror.h" #include "qemu/error-report.h" #include "qemu/option.h" #include "qemu/timer.h" -- 2.44.0
Re: [PATCH] hw/ide/ahci: Rename ahci_internal.h to ahci-internal.h
BALATON Zoltan writes:
> On Tue, 27 Feb 2024, Philippe Mathieu-Daudé wrote:
>> On 27/2/24 14:13, BALATON Zoltan wrote:
>>> Other headers now use dash instead of underscore. Rename
>>> ahci_internal.h accordingly for consistency.
>>> Signed-off-by: BALATON Zoltan
>>> ---
>>> hw/ide/{ahci_internal.h => ahci-internal.h} | 0
>>> hw/ide/ahci.c | 2 +-
>>> hw/ide/ich.c| 2 +-
>>> 3 files changed, 2 insertions(+), 2 deletions(-)
>>> rename hw/ide/{ahci_internal.h => ahci-internal.h} (100%)
>>
>> Reviewed-by: Philippe Mathieu-Daudé
>
> Was this also queued somewhere? I haven't seen it merged neither with the
> trivial nor the misc-hw pull requests.
I figure John (our IDE odd fixer) wouldn't mind this going via
qemu-trivial. Cc'ed!
Re: [PATCH v2 2/4] mirror: allow specifying working bitmap
nt', '*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 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.
> +# For a diff image target, using copy-mode=write-blocking should
> +# not be used, because unaligned writes will lead to allocated
> +# clusters with partial data in the target image! The bitmap
> +# will be enabled after the job finishes. (Since 9.0)
> +#
> # @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',
Acked-by: Markus Armbruster
[...]
Re: [PATCH v2 1/4] qapi/block-core: avoid the re-use of MirrorSyncMode for backup
Fiona Ebner writes:
> Backup supports all modes listed in MirrorSyncMode, while mirror does
> not. Introduce BackupSyncMode by copying the current MirrorSyncMode
> and drop the variants mirror does not support from MirrorSyncMode as
> well as the corresponding manual check in mirror_start().
Results in tighter introspection: query-qmp-schema no longer reports
drive-mirror and blockdev-mirror accepting @sync values they actually
reject. Suggest to mention this in the commit message.
> Suggested-by: Vladimir Sementsov-Ogievskiy
> Signed-off-by: Fiona Ebner
> ---
>
> I felt like keeping the "Since: X.Y" as before makes the most sense as
> to not lose history. Or is it necessary to change this for
> BackupSyncMode (and its members) since it got a new name?
Doc comments are for users of the QMP interface. Type names do not
matter there. I agree with your decision not to update the "since"
tags.
[...]
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 1874f880a8..59d75b0793 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1304,10 +1304,10 @@
>'data': ['report', 'ignore', 'enospc', 'stop', 'auto'] }
>
> ##
> -# @MirrorSyncMode:
> +# @BackupSyncMode:
> #
> -# An enumeration of possible behaviors for the initial synchronization
> -# phase of storage mirroring.
> +# An enumeration of possible behaviors for image synchronization used
> +# by backup jobs.
> #
> # @top: copies data in the topmost image to the destination
> #
> @@ -1323,7 +1323,7 @@
> #
> # Since: 1.3
> ##
> -{ 'enum': 'MirrorSyncMode',
> +{ 'enum': 'BackupSyncMode',
>'data': ['top', 'full', 'none', 'incremental', 'bitmap'] }
>
> ##
> @@ -1347,6 +1347,23 @@
> { 'enum': 'BitmapSyncMode',
>'data': ['on-success', 'never', 'always'] }
>
> +##
> +# @MirrorSyncMode:
> +#
> +# An enumeration of possible behaviors for the initial synchronization
> +# phase of storage mirroring.
> +#
> +# @top: copies data in the topmost image to the destination
> +#
> +# @full: copies data from all images to the destination
> +#
> +# @none: only copy data written from now on
> +#
> +# Since: 1.3
> +##
> +{ 'enum': 'MirrorSyncMode',
> + 'data': ['top', 'full', 'none'] }
> +
> ##
> # @MirrorCopyMode:
> #
> @@ -1624,7 +1641,7 @@
> ##
> { 'struct': 'BackupCommon',
>'data': { '*job-id': 'str', 'device': 'str',
> -'sync': 'MirrorSyncMode', '*speed': 'int',
> + 'sync': 'BackupSyncMode', '*speed': 'int',
> '*bitmap': 'str', '*bitmap-mode': 'BitmapSyncMode',
> '*compress': 'bool',
> '*on-source-error': 'BlockdevOnError',
QAPI schema
Acked-by: Markus Armbruster
Re: [PATCH v2 6/6] 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
[...]
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 6ece164172..ffc5e3be18 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -179,3 +179,29 @@
> { 'command': 'device-sync-config',
>'features': [ 'unstable' ],
>'data': {'id': 'str'} }
> +
> +##
> +# @VIRTIO_CONFIG_READ:
> +#
> +# Emitted whenever guest reads virtio device config after config change.
Let's not abbreviate "configuration" to "config".
> +#
> +# @device: device name
> +#
> +# @path: device path
> +#
> +# Features:
> +#
> +# @unstable: The event is experimental.
> +#
> +# Since: 9.0
> +#
> +# Example:
> +#
> +# <- { "event": "VIRTIO_CONFIG_READ",
> +# "data": { "device": "virtio-net-pci-0",
> +#"path": "/machine/peripheral/virtio-net-pci-0" },
> +# "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> +##
As for PATCH 4, I'd like to see some guidance on intended use.
> +{ 'event': 'VIRTIO_CONFIG_READ',
> + 'features': [ 'unstable' ],
> + 'data': { '*device': 'str', 'path': 'str' } }
[...]
Re: [PATCH v2 5/6] qapi: device-sync-config: check runstate
Vladimir Sementsov-Ogievskiy writes:
> Command result is racy if allow it during migration. Let's allow the
> sync only in RUNNING state.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
If I understand this correctly, the previous commit introduces a race,
and this one fixes it.
We normally avoid such temporary bugs. When avoidance is impractical,
we point them out in commit message and FIXME comment.
> ---
> include/sysemu/runstate.h | 1 +
> system/qdev-monitor.c | 27 ++-
> system/runstate.c | 5 +
> 3 files changed, 32 insertions(+), 1 deletion(-)
>
> 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/system/qdev-monitor.c b/system/qdev-monitor.c
> index e3107a12d7..b83b5d23c9 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"
> @@ -983,7 +984,31 @@ int qdev_sync_config(DeviceState *dev, Error **errp)
>
> void qmp_device_sync_config(const char *id, Error **errp)
> {
> -DeviceState *dev = find_device_state(id, true, errp);
> +MigrationState *s = migrate_get_current();
> +DeviceState *dev;
> +
> +/*
> + * During migration there is a race between syncing`config and migrating
> it,
> + * so let's just not allow it.
> + *
> + * Moreover, let's not rely on setting up interrupts in paused state,
> which
> + * may be a part of migration process.
Wrap your comment lines around column 70, please.
> + */
> +
> +if (migration_is_running(s->state)) {
> +error_setg(errp, "Config synchronization is not allowed "
> + "during migration.");
> +return;
> +}
> +
> +if (!runstate_is_running()) {
> +error_setg(errp, "Config synchronization allowed only in '%s' state,
> "
> + "current state is '%s'", RunState_str(RUN_STATE_RUNNING),
> + current_run_state_str());
> +return;
> +}
> +
> +dev = find_device_state(id, true, errp);
> if (!dev) {
> return;
> }
> diff --git a/system/runstate.c b/system/runstate.c
> index d6ab860eca..8fd89172ae 100644
> --- a/system/runstate.c
> +++ b/system/runstate.c
> @@ -189,6 +189,11 @@ bool runstate_check(RunState state)
> return current_run_state == state;
> }
>
> +const char *current_run_state_str(void)
> +{
> +return RunState_str(current_run_state);
> +}
> +
> static void runstate_init(void)
> {
> const RunStateTransition *p;
Re: [PATCH v2 4/6] 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).
>
> 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 +++
> qapi/qdev.json| 17 +
> system/qdev-monitor.c | 23 +++
> 5 files changed, 72 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;
> }
>
> @@ -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 1a7039fb0c..1197341a3c 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -2318,6 +2318,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);
> @@ -2334,6 +2342,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 32ffaee644..6ece164172 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -162,3 +162,20 @@
> ##
Re: [PATCH v2 3/6] qdev-monitor: add option to report GenericError from find_device_state
Vladimir Sementsov-Ogievskiy writes:
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> system/qdev-monitor.c | 15 +++
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> index 9febb743f1..cf7481e416 100644
> --- a/system/qdev-monitor.c
> +++ b/system/qdev-monitor.c
> @@ -877,13 +877,20 @@ void qmp_device_add(QDict *qdict, QObject **ret_data,
> Error **errp)
> object_unref(OBJECT(dev));
> }
>
> -static DeviceState *find_device_state(const char *id, Error **errp)
> +/*
> + * Note that creating new APIs using error classes other than GenericError is
> + * not recommended. Set use_generic_error=true for new interfaces.
> + */
> +static DeviceState *find_device_state(const char *id, bool use_generic_error,
> + Error **errp)
> {
> Object *obj = object_resolve_path_at(qdev_get_peripheral(), id);
> DeviceState *dev;
>
> if (!obj) {
> -error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> +error_set(errp,
> + (use_generic_error ?
> + ERROR_CLASS_GENERIC_ERROR : ERROR_CLASS_DEVICE_NOT_FOUND),
>"Device '%s' not found", id);
> return NULL;
> }
> @@ -947,7 +954,7 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>
> void qmp_device_del(const char *id, Error **errp)
> {
> -DeviceState *dev = find_device_state(id, errp);
> +DeviceState *dev = find_device_state(id, false, errp);
> if (dev != NULL) {
> if (dev->pending_deleted_event &&
> (dev->pending_deleted_expires_ms == 0 ||
> @@ -1067,7 +1074,7 @@ BlockBackend *blk_by_qdev_id(const char *id, Error
> **errp)
>
> GLOBAL_STATE_CODE();
>
> -dev = find_device_state(id, errp);
> +dev = find_device_state(id, false, errp);
> if (dev == NULL) {
> return NULL;
> }
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.
Re: [PATCH v2 2/6] qdev-monitor: fix error message in find_device_state()
Vladimir Sementsov-Ogievskiy writes:
> This "hotpluggable" here is misleading. Actually we check is object a
> device or not. Let's drop the word.
>
> SUggested-by: Markus Armbruster
"Suggested"
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> system/qdev-monitor.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> index a13db763e5..9febb743f1 100644
> --- a/system/qdev-monitor.c
> +++ b/system/qdev-monitor.c
> @@ -890,7 +890,7 @@ static DeviceState *find_device_state(const char *id,
> Error **errp)
>
> dev = (DeviceState *)object_dynamic_cast(obj, TYPE_DEVICE);
> if (!dev) {
> -error_setg(errp, "%s is not a hotpluggable device", id);
> +error_setg(errp, "%s is not a device", id);
> return NULL;
> }
Perhaps including what @id resolved to could be useful, but that's
out of scope for this patch.
Reviewed-by: Markus Armbruster
[PATCH] blockdev: Fix blockdev-snapshot-sync error reporting for no medium
When external_snapshot_abort() rejects a BlockDriverState without a
medium, it creates an error like this:
error_setg(errp, "Device '%s' has no medium", device);
Trouble is @device can be null. My system formats null as "(null)",
but other systems might crash. Reproducer:
1. Create a block device without a medium
-> {"execute": "blockdev-add", "arguments": {"driver": "host_cdrom",
"node-name": "blk0", "filename": "/dev/sr0"}}
<- {"return": {}}
3. Attempt to snapshot it
-> {"execute":"blockdev-snapshot-sync", "arguments": { "node-name": "blk0",
"snapshot-file":"/tmp/foo.qcow2","format":"qcow2"}}
<- {"error": {"class": "GenericError", "desc": "Device '(null)' has no
medium"}}
Broken when commit 0901f67ecdb made @device optional.
Use bdrv_get_device_or_node_name() instead. Now it fails as it
should:
<- {"error": {"class": "GenericError", "desc": "Device 'blk0' has no
medium"}}
Fixes: 0901f67ecdb7 (qmp: Allow to take external snapshots on bs graphs node.)
Signed-off-by: Markus Armbruster
---
blockdev.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/blockdev.c b/blockdev.c
index d8fb3399f5..057601dcf0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1395,7 +1395,8 @@ 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, QERR_DEVICE_HAS_NO_MEDIUM,
+ bdrv_get_device_or_node_name(state->old_bs));
return;
}
--
2.44.0
[PATCH 2/2] qerror: QERR_DEVICE_IN_USE 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 8dd9fcb071..0c2689cf8a 100644 --- a/include/qapi/qmp/qerror.h +++ b/include/qapi/qmp/qerror.h @@ -23,9 +23,6 @@ #define QERR_DEVICE_HAS_NO_MEDIUM \ "Device '%s' has no medium" -#define QERR_DEVICE_IN_USE \ -"Device '%s' is in use" - #define QERR_DEVICE_NO_HOTPLUG \ "Device '%s' does not support hotplugging" -- 2.44.0
[PATCH 1/2] blockdev: Fix block_resize error reporting for op blockers
When block_resize() runs into an op blocker, it creates an error like
this:
error_setg(errp, "Device '%s' is in use", device);
Trouble is @device can be null. My system formats null as "(null)",
but other systems might crash. Reproducer:
1. Create two block devices
-> {"execute": "blockdev-add", "arguments": {"driver": "file", "node-name":
"blk0", "filename": "64k.img"}}
<- {"return": {}}
-> {"execute": "blockdev-add", "arguments": {"driver": "file", "node-name":
"blk1", "filename": "m.img"}}
<- {"return": {}}
2. Put a blocker on one them
-> {"execute": "blockdev-mirror", "arguments": {"job-id": "job0", "device":
"blk0", "target": "blk1", "sync": "full"}}
{"return": {}}
-> {"execute": "job-pause", "arguments": {"id": "job0"}}
{"return": {}}
-> {"execute": "job-complete", "arguments": {"id": "job0"}}
{"return": {}}
Note: job events elided for brevity.
3. Attempt to resize
-> {"execute": "block_resize", "arguments": {"node-name": "blk1",
"size":32768}}
<- {"error": {"class": "GenericError", "desc": "Device '(null)' is in use"}}
Broken when commit 3b1dbd11a60 made @device optional. Fixed in commit
ed3d2ec98a3 (block: Add errp to b{lk,drv}_truncate()), except for this
one instance.
Fix it by using the error message provided by the op blocker instead,
so it fails like this:
<- {"error": {"class": "GenericError", "desc": "Node 'blk1' is busy: block
device is in use by block job: mirror"}}
Fixes: 3b1dbd11a60d (qmp: Allow block_resize to manipulate bs graph nodes.)
Signed-off-by: Markus Armbruster
---
blockdev.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index f8bb0932f8..d8fb3399f5 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2252,8 +2252,7 @@ void coroutine_fn qmp_block_resize(const char *device,
const char *node_name,
}
bdrv_graph_co_rdlock();
-if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_RESIZE, NULL)) {
-error_setg(errp, QERR_DEVICE_IN_USE, device);
+if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_RESIZE, errp)) {
bdrv_graph_co_rdunlock();
return;
}
--
2.44.0
[PATCH 0/2] blockdev: Fix block_resize error reporting for op blockers
PATCH 2 requires my "[PATCH] char: Slightly better error reporting when chardev is in use" posted earlier today, PATCH 1 does not. Based-on: <20240306081505.2258405-1-arm...@redhat.com> Markus Armbruster (2): blockdev: Fix block_resize error reporting for op blockers qerror: QERR_DEVICE_IN_USE is no longer used, drop include/qapi/qmp/qerror.h | 3 --- blockdev.c| 3 +-- 2 files changed, 1 insertion(+), 5 deletions(-) -- 2.44.0
Re: [PATCH v2 00/10] mirror: allow switching from background to active mode
Kevin Wolf writes: > Am 28.02.2024 um 19:07 hat Vladimir Sementsov-Ogievskiy geschrieben: [...] >> About the APIs, I think, of course we should deprecate block-job-* API, >> because we already have jobs which are not block-jobs, so we can't deprecate >> job-* API. >> >> So I suggest a plan: >> >> 1. Add job-change command simply in block-core.json, as a simple copy >>of block-job-change, to not care with resolving inclusion loops. >>(ha we could simply name our block-job-change to be job-change and >>place it in block-core.json, but now is too late) >> >> 2. Support changing speed in a new job-chage command. (or both in >>block-job-change and job-change, keeping them equal) > > It should be both block-job-change and job-change. > > Having job-change in block-core.json rather than job.json is ugly, but > if Markus doesn't complain, why would I. What we have is ugly and confusing: two interfaces with insufficient guidance on which one to use. Unifying the interfaces will reduce confusion immediately, and may reduce ugliness eventually. I take it. [...]
Re: [PATCH 9/9] hostmem-file: support POSIX shm_open()
Stefano Garzarella writes: > On Wed, Feb 28, 2024 at 01:32:17PM +0100, Markus Armbruster wrote: >>Stefano Garzarella writes: [...] >>> +# @shm: if true, shm_open(3) is used to create/open POSIX shared memory >>> +# object; if false, an open(2) is used. (default: false) (since 9.0) >>> +# >> >>Please format like this for consistency: > > Sure. > >> >># @shm: if true, shm_open(3) is used to create/open POSIX shared memory >># object; if false, an open(2) is used (default: false) (since 9.0) > > I just noticed that I followed the property just above (@rom). Should we fix > that one? Yes, please. See commit a937b6aa739 (qapi: Reformat doc comments to conform to current conventions).
Re: [PATCH v2 00/10] mirror: allow switching from background to active mode
Vladimir Sementsov-Ogievskiy writes:
> On 03.11.23 18:56, Markus Armbruster wrote:
>> Is the job abstraction a failure?
>>
>> We have
>>
>> block-job- command since job- commandsince
>> -
>> block-job-set-speed 1.1
>> block-job-cancel1.1 job-cancel 3.0
>> block-job-pause 1.3 job-pause 3.0
>> block-job-resume1.3 job-resume 3.0
>> block-job-complete 1.3 job-complete3.0
>> block-job-dismiss 2.12job-dismiss 3.0
>> block-job-finalize 2.12job-finalize3.0
>> block-job-change8.2
>> query-block-jobs1.1 query-jobs
>>
>> I was under the impression that we added the (more general) job-
>> commands to replace the (less general) block-job commands, and we're
>> keeping the latter just for compatibility. Am I mistaken?
>>
>> Which one should be used?
>>
>> Why not deprecate the one that shouldn't be used?
>>
>> The addition of block-job-change without even trying to do job-change
>> makes me wonder: have we given up on the job- interface?
>>
>> I'm okay with giving up on failures. All I want is clarity. Right now,
>> I feel thoroughly confused about the status block-jobs and jobs, and how
>> they're related.
>
> Hi! I didn't notice, that the series was finally merged.
>
> About the APIs, I think, of course we should deprecate block-job-* API,
> because we already have jobs which are not block-jobs, so we can't deprecate
> job-* API.
>
> So I suggest a plan:
>
> 1. Add job-change command simply in block-core.json, as a simple copy of
> block-job-change, to not care with resolving inclusion loops. (ha we could
> simply name our block-job-change to be job-change and place it in
> block-core.json, but now is too late)
>
> 2. Support changing speed in a new job-chage command. (or both in
> block-job-change and job-change, keeping them equal)
>
> 3. Deprecate block-job-* APIs
>
> 4. Wait 3 releases
>
> 5. Drop block-job-* APIs
>
> 6. Move all job-related stuff to job.json, drop `{ 'include': 'job.json' }`
> from block-core.json, and instead include block-core.json into job.json
Sounds good to me.
> If it's OK, I can go through the steps.
Lovely!
Re: [PATCH v8 14/15] hw/pci: Determine if rombar is explicitly enabled
Akihiko Odaki writes: > vfio determines if rombar is explicitly enabled by inspecting QDict. > Inspecting QDict is not nice because QDict is untyped and depends on the > details on the external interface. Add an infrastructure to determine if > rombar is explicitly enabled to hw/pci. > > This changes the semantics of UINT32_MAX, which has always been a valid > value to explicitly say rombar is enabled to denote the implicit default > value. Nobody should have been set UINT32_MAX to rombar however, > considering that its meaning was no different from 1 and typing a > literal UINT32_MAX (0x or 4294967295) is more troublesome. > > Signed-off-by: Akihiko Odaki Remark on the previous patch applies. Reviewed-by: Markus Armbruster
Re: [PATCH v8 13/15] hw/pci: Use UINT32_MAX as a default value for rombar
Akihiko Odaki writes:
> Currently there is no way to distinguish the case that rombar is
> explicitly specified as 1 and the case that rombar is not specified.
>
> Set rombar UINT32_MAX by default to distinguish these cases just as it
> is done for addr and romsize. It was confirmed that changing the default
> value to UINT32_MAX will not change the behavior by looking at
> occurences of rom_bar.
>
> $ git grep -w rom_bar
> hw/display/qxl.c:328:QXLRom *rom = memory_region_get_ram_ptr(>rom_bar);
> hw/display/qxl.c:431:qxl_set_dirty(>rom_bar, 0, qxl->rom_size);
> hw/display/qxl.c:1048:QXLRom *rom =
> memory_region_get_ram_ptr(>rom_bar);
> hw/display/qxl.c:2131:memory_region_init_rom(>rom_bar, OBJECT(qxl),
> "qxl.vrom",
> hw/display/qxl.c:2154: PCI_BASE_ADDRESS_SPACE_MEMORY, >rom_bar);
> hw/display/qxl.h:101:MemoryRegion rom_bar;
> hw/pci/pci.c:74:DEFINE_PROP_UINT32("rombar", PCIDevice, rom_bar, 1),
> hw/pci/pci.c:2329:if (!pdev->rom_bar) {
> hw/vfio/pci.c:1019:if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
> hw/xen/xen_pt_load_rom.c:29:if (dev->romfile || !dev->rom_bar) {
> include/hw/pci/pci_device.h:150:uint32_t rom_bar;
>
> rom_bar refers to a different variable in qxl. It is only tested if
> the value is 0 or not in the other places.
>
> If a user explicitly set UINT32_MAX, we still cannot distinguish that
> from the implicit default. However, it is unlikely to be a problem as
> nobody would type literal UINT32_MAX (0x or 4294967295) by
> chance.
>
> Signed-off-by: Akihiko Odaki
Not exactly elegant, but I believe we have similar "default value means
not set by user (good enough because the default value is sufficiently
odd)" logic elsewhere.
Reviewed-by: Markus Armbruster
Re: [PATCH 9/9] hostmem-file: support POSIX shm_open()
Stefano Garzarella writes:
> Add a new `shm` bool option for `-object memory-backend-file`.
>
> When this option is set to true, the POSIX shm_open(3) is used instead
> of open(2).
>
> So a file will not be created in the filesystem, but a "POSIX shared
> memory object" will be instantiated. In Linux this turns into a file
> in /dev/shm, but in other OSes this may not happen (for example in
> macOS or FreeBSD nothing is shown in any filesystem).
>
> This new feature is useful when we need to share guest memory with
> another process (e.g. vhost-user backend), but we don't have
> memfd_create() or any special filesystems (e.g. /dev/shm) available
> as in macOS.
>
> Signed-off-by: Stefano Garzarella
> ---
> I am not sure this is the best way to support shm_open() in QEMU.
>
> Other solutions I had in mind were:
>
> - create a new memory-backend-shm
How would that look like? Would it involve duplicating code?
> - extend memory-backend-memfd to use shm_open() on systems where memfd is
> not available (problem: shm_open wants a name to assign to the object, but
> we can do a workaround by using a random name and do the unlink right away)
Hmm. Too much magic? I don't know...
> Any preference/suggestion?
[...]
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 2a6e49365a..bfb01b909f 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -682,6 +682,9 @@
# @mem-path: the path to either a shared memory or huge page
# filesystem mount
Does this need adjustment?
[...]
> # writable RAM instead of ROM, and want to set this property to 'off'.
> # (default: auto, since 8.2)
> #
> +# @shm: if true, shm_open(3) is used to create/open POSIX shared memory
> +# object; if false, an open(2) is used. (default: false) (since 9.0)
> +#
Please format like this for consistency:
# @shm: if true, shm_open(3) is used to create/open POSIX shared memory
# object; if false, an open(2) is used (default: false) (since 9.0)
> # Since: 2.1
> ##
> { 'struct': 'MemoryBackendFileProperties',
> @@ -692,6 +695,7 @@
> 'mem-path': 'str',
> '*pmem': { 'type': 'bool', 'if': 'CONFIG_LIBPMEM' },
> '*readonly': 'bool',
> +'*shm': 'bool',
> '*rom': 'OnOffAuto' } }
>
> ##
[...]
Re: [PATCH v7 15/16] vfio: Avoid inspecting option QDict for rombar
Akihiko Odaki writes:
> Use pci_rom_bar_explicitly_enabled() to determine if rombar is explicitly
> enabled.
>
> Signed-off-by: Akihiko Odaki
> ---
> hw/vfio/pci.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 4fa387f0430d..647f15b2a060 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1012,7 +1012,6 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
> {
> uint32_t orig, size = cpu_to_le32((uint32_t)PCI_ROM_ADDRESS_MASK);
> off_t offset = vdev->config_offset + PCI_ROM_ADDRESS;
> -DeviceState *dev = DEVICE(vdev);
> char *name;
> int fd = vdev->vbasedev.fd;
>
> @@ -1046,7 +1045,7 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
> }
>
> if (vfio_opt_rom_in_denylist(vdev)) {
> -if (dev->opts && qdict_haskey(dev->opts, "rombar")) {
> +if (pci_rom_bar_explicitly_enabled(>pdev)) {
> warn_report("Device at %s is known to cause system instability"
> " issues during option rom execution",
> vdev->vbasedev.name);
I'd squash the previous patch and this one together.
Re: [PATCH v7 14/16] hw/pci: Determine if rombar is explicitly enabled
Akihiko Odaki writes:
> vfio determines if rombar is explicitly enabled by inspecting QDict.
> Inspecting QDict is not nice because QDict is untyped and depends on the
> details on the external interface. Add an infrastructure to determine if
> rombar is explicitly enabled to hw/pci.
>
> Signed-off-by: Akihiko Odaki
> ---
> include/hw/pci/pci_device.h | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h
> index ca151325085d..6be0f989ebe0 100644
> --- a/include/hw/pci/pci_device.h
> +++ b/include/hw/pci/pci_device.h
> @@ -205,6 +205,11 @@ static inline uint16_t pci_get_bdf(PCIDevice *dev)
> return PCI_BUILD_BDF(pci_bus_num(pci_get_bus(dev)), dev->devfn);
> }
>
> +static inline bool pci_rom_bar_explicitly_enabled(PCIDevice *dev)
> +{
> +return dev->rom_bar && dev->rom_bar != UINT32_MAX;
> +}
> +
> static inline void pci_set_power(PCIDevice *pci_dev, bool state)
> {
> /*
I agree inspecting QDict is not nice. The replacement has its own small
drawback, though: an explicit "rombar=4294967296" is now misinterpreted
as "user did not set rombar". The previous commit argues that users are
really unlikely to do that. I don't disagree, but I think the drawback
is worth mentioning in the commit message.
Re: [PATCH v7 12/16] hw/pci: Replace -1 with UINT32_MAX for romsize
Akihiko Odaki writes:
> romsize is an uint32_t variable. Specifying -1 as an uint32_t value is
> obscure way to denote UINT32_MAX.
>
> Worse, if int is wider than 32-bit, it will change the behavior of a
> construct like the following:
> romsize = -1;
> if (romsize != -1) {
> ...
> }
>
> When -1 is assigned to romsize, -1 will be implicitly casted into
> uint32_t, resulting in UINT32_MAX. On contrary, when evaluating
> romsize != -1, romsize will be casted into int, and it will be a
> comparison of UINT32_MAX and -1, and result in false.
>
> Fix these issues by replacing -1 with UINT32_MAX for statements
> involving the variable.
Could be viewed as cleanup instead of fix, given how unlikely int wider
than 32 bits is. Observation, not a demand :)
> Signed-off-by: Akihiko Odaki
Reviewed-by: Markus Armbruster
Re: [PATCH] hw/ide/ahci: Rename ahci_internal.h to ahci-internal.h
BALATON Zoltan writes: > Other headers now use dash instead of underscore. Rename > ahci_internal.h accordingly for consistency. > > Signed-off-by: BALATON Zoltan Reviewed-by: Markus Armbruster
Re: [PATCH v3 3/3] hw/ide: Include 'ide_internal.h' from current path
Philippe Mathieu-Daudé writes: > Rename "internal.h" as "ide_internal.h", and include ide-internal.h > it via its relative local path, instead of absolute > to the project root path. > > Signed-off-by: Philippe Mathieu-Daudé The series is a lovely little cleanup; thanks, guys!
Re: [PATCH v2 3/3] hw/ide: Include 'ide_internal.h' from current path
BALATON Zoltan writes: > On Sun, 25 Feb 2024, Philippe Mathieu-Daudé wrote: >> Rename "internal.h" as "ide_internal.h", and include > > Is there a convention about using underscore or dash in file names? The > headers Thomas added are using - as well as ahci-allwinner.c, only > ahci_internal.h has _ (but there are others elsewhere such as pci_device.h). > Maybe we should be consistent at least within IDE and this series is now a > good opportunity for renaming these headers to match. But it's just a small > nit, thanks for picking this up. This is one of the many unnecessary inconsistencies we're inflicting on ourselves. We have more than 3600 file names containing '-', and more almost 2700 containing '_'. Bizarrely, 68 of them contain both. I strongly prefer '_' myself. Zoltan is making a local consistency argument for '-'. Let's use '-' here.
Re: [PATCH v6 15/15] hw/qdev: Remove opts member
Akihiko Odaki writes:
> It is no longer used.
>
> Signed-off-by: Akihiko Odaki
> Reviewed-by: Philippe Mathieu-Daudé
> ---
> include/hw/qdev-core.h | 4
> hw/core/qdev.c | 1 -
> system/qdev-monitor.c | 12 +++-
> 3 files changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 9228e96c87e9..5954404dcbfe 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -237,10 +237,6 @@ struct DeviceState {
> * @pending_deleted_expires_ms: optional timeout for deletion events
> */
> int64_t pending_deleted_expires_ms;
> -/**
> - * @opts: QDict of options for the device
> - */
> -QDict *opts;
> /**
> * @hotplugged: was device added after PHASE_MACHINE_READY?
> */
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index c68d0f7c512f..7349c9a86be8 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -706,7 +706,6 @@ static void device_finalize(Object *obj)
> dev->canonical_path = NULL;
> }
>
> -qobject_unref(dev->opts);
> g_free(dev->id);
> }
>
> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> index a13db763e5dd..71c00f62ee38 100644
> --- a/system/qdev-monitor.c
> +++ b/system/qdev-monitor.c
> @@ -625,6 +625,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
> char *id;
> DeviceState *dev = NULL;
> BusState *bus = NULL;
> +QDict *properties;
>
> driver = qdict_get_try_str(opts, "driver");
> if (!driver) {
> @@ -705,13 +706,14 @@ DeviceState *qdev_device_add_from_qdict(const QDict
> *opts,
> }
>
> /* set properties */
> -dev->opts = qdict_clone_shallow(opts);
> -qdict_del(dev->opts, "driver");
> -qdict_del(dev->opts, "bus");
> -qdict_del(dev->opts, "id");
> +properties = qdict_clone_shallow(opts);
> +qdict_del(properties, "driver");
> +qdict_del(properties, "bus");
> +qdict_del(properties, "id");
>
> -object_set_properties_from_keyval(>parent_obj, dev->opts, from_json,
> +object_set_properties_from_keyval(>parent_obj, properties,
> from_json,
>errp);
> +qobject_unref(properties);
> if (*errp) {
> goto err_del_dev;
> }
Reviewed-by: Markus Armbruster
Depends on the previous few patches, of course.
Re: [PATCH v6 14/15] vfio: Avoid inspecting option QDict for rombar
Akihiko Odaki writes:
> Use pci_rom_bar_explicitly_enabled() to determine if rombar is explicitly
> enabled.
>
> Signed-off-by: Akihiko Odaki
> ---
> hw/vfio/pci.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 4fa387f0430d..647f15b2a060 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1012,7 +1012,6 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
> {
> uint32_t orig, size = cpu_to_le32((uint32_t)PCI_ROM_ADDRESS_MASK);
> off_t offset = vdev->config_offset + PCI_ROM_ADDRESS;
> -DeviceState *dev = DEVICE(vdev);
> char *name;
> int fd = vdev->vbasedev.fd;
>
> @@ -1046,7 +1045,7 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
> }
>
> if (vfio_opt_rom_in_denylist(vdev)) {
> -if (dev->opts && qdict_haskey(dev->opts, "rombar")) {
> +if (pci_rom_bar_explicitly_enabled(>pdev)) {
> warn_report("Device at %s is known to cause system instability"
> " issues during option rom execution",
> vdev->vbasedev.name);
Consider -device ...,rombar=0x.
Before the patch, the condition is true.
Afterwards, it's false.
Do we care?
Re: [PATCH v6 13/15] hw/pci: Determine if rombar is explicitly enabled
Akihiko Odaki writes:
> vfio determines if rombar is explicitly enabled by inspecting QDict.
> Inspecting QDict is not nice because QDict is untyped and depends on the
> details on the external interface. Add an infrastructure to determine if
> rombar is explicitly enabled to hw/pci.
>
> Signed-off-by: Akihiko Odaki
> ---
> include/hw/pci/pci_device.h | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h
> index ca151325085d..c4fdc96ef50d 100644
> --- a/include/hw/pci/pci_device.h
> +++ b/include/hw/pci/pci_device.h
> @@ -205,6 +205,11 @@ static inline uint16_t pci_get_bdf(PCIDevice *dev)
> return PCI_BUILD_BDF(pci_bus_num(pci_get_bus(dev)), dev->devfn);
> }
>
> +static inline bool pci_rom_bar_explicitly_enabled(PCIDevice *dev)
> +{
> +return dev->rom_bar && dev->rom_bar != -1;
Compares signed with unsigned: rom_bar is uint32_t, -1 is int.
If int is at most 32 bits, the comparison converts -1 to
(uint32_t)0x.
If int is wider, it converts dev->rom_bar to int without changing its
value. In particular, it converts the default value 0x (written
as -1 in the previous patch) to (int)0x. Oops.
Best not to mix signed and unsigned in comparisons. Easy enough to
avoid here.
Still, we don't actually test "rom_bar has not been set". We test
"rom_bar has the default value". Nothing stops a user from passing
rombar=0x to -device. See my review of the next patch.
> +}
> +
> static inline void pci_set_power(PCIDevice *pci_dev, bool state)
> {
> /*
Re: [PATCH v6 12/15] hw/pci: Use -1 as a default value for rombar
Akihiko Odaki writes:
> Currently there is no way to distinguish the case that rombar is
> explicitly specified as 1 and the case that rombar is not specified.
>
> Set rombar -1 by default to distinguish these cases just as it is done
> for addr and romsize. It was confirmed that changing the default value
> to -1 will not change the behavior by looking at occurences of rom_bar.
>
> $ git grep -w rom_bar
> hw/display/qxl.c:328:QXLRom *rom = memory_region_get_ram_ptr(>rom_bar);
> hw/display/qxl.c:431:qxl_set_dirty(>rom_bar, 0, qxl->rom_size);
> hw/display/qxl.c:1048:QXLRom *rom =
> memory_region_get_ram_ptr(>rom_bar);
> hw/display/qxl.c:2131:memory_region_init_rom(>rom_bar, OBJECT(qxl),
> "qxl.vrom",
> hw/display/qxl.c:2154: PCI_BASE_ADDRESS_SPACE_MEMORY, >rom_bar);
> hw/display/qxl.h:101:MemoryRegion rom_bar;
> hw/pci/pci.c:74:DEFINE_PROP_UINT32("rombar", PCIDevice, rom_bar, 1),
> hw/pci/pci.c:2329:if (!pdev->rom_bar) {
> hw/vfio/pci.c:1019:if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
> hw/xen/xen_pt_load_rom.c:29:if (dev->romfile || !dev->rom_bar) {
> include/hw/pci/pci_device.h:150:uint32_t rom_bar;
>
> rom_bar refers to a different variable in qxl. It is only tested if
> the value is 0 or not in the other places.
Makes me wonder why it's uint32_t. Not this patch's problem.
> Signed-off-by: Akihiko Odaki
> ---
> hw/pci/pci.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 54b375da2d26..909c5b3ee4ee 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -71,7 +71,7 @@ static Property pci_props[] = {
> DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
> DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
> DEFINE_PROP_UINT32("romsize", PCIDevice, romsize, -1),
> -DEFINE_PROP_UINT32("rombar", PCIDevice, rom_bar, 1),
> +DEFINE_PROP_UINT32("rombar", PCIDevice, rom_bar, -1),
> DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present,
> QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false),
> DEFINE_PROP_BIT("x-pcie-lnksta-dllla", PCIDevice, cap_present,
Re: [RFC 1/4] drive-mirror: add support for sync=bitmap mode=never
Fiona Ebner writes:
> From: John Snow
>
> This patch adds support for the "BITMAP" sync mode to drive-mirror and
> blockdev-mirror. It adds support only for the BitmapSyncMode "never,"
> because it's the simplest mode.
>
> This mode simply uses a user-provided bitmap as an initial copy
> manifest, and then does not clear any bits in the bitmap at the
> conclusion of the operation.
>
> Any new writes dirtied during the operation are copied out, in contrast
> to backup. Note that whether these writes are reflected in the bitmap
> at the conclusion of the operation depends on whether that bitmap is
> actually recording!
>
> This patch was originally based on one by Ma Haocong, but it has since
> been modified pretty heavily.
>
> Suggested-by: Ma Haocong
> Signed-off-by: Ma Haocong
> Signed-off-by: John Snow
> [FG: switch to bdrv_dirty_bitmap_merge_internal]
> Signed-off-by: Fabian Grünbichler
> Signed-off-by: Thomas Lamprecht
> [FE: rebase for 9.0
> update version and formatting in QAPI]
> Signed-off-by: Fiona Ebner
[...]
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index ab5a93a966..ac05483958 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2181,6 +2181,15 @@
> # 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 for sync=bitmap mode. This
> +# argument must be present for bitmap mode and absent otherwise.
> +# The bitmap's granularity is used instead of @granularity.
> +# (Since 9.0).
What happens when the user specifies @granularity anyway? Error or
silently ignored?
> +#
> +# @bitmap-mode: Specifies the type of data the bitmap should contain
> +# after the operation concludes. Must be present if sync is
> +# "bitmap". Must NOT be present otherwise. (Since 9.0)
Members that must be present when and only when some enum member has a
certain value should perhaps be in a union branch. Perhaps the block
maintainers have an opinion here.
> +#
> # @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
> @@ -2223,7 +2232,9 @@
> { 'struct': 'DriveMirror',
>'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
> '*format': 'str', '*node-name': 'str', '*replaces': 'str',
> -'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
> +'sync': 'MirrorSyncMode', '*bitmap': 'str',
> +'*bitmap-mode': 'BitmapSyncMode',
> +'*mode': 'NewImageMode',
> '*speed': 'int', '*granularity': 'uint32',
> '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
> '*on-target-error': 'BlockdevOnError',
> @@ -2507,6 +2518,15 @@
> # 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 for sync=bitmap mode. This
> +# argument must be present for bitmap mode and absent otherwise.
> +# The bitmap's granularity is used instead of @granularity.
> +# (Since 9.0).
> +#
> +# @bitmap-mode: Specifies the type of data the bitmap should contain
> +# after the operation concludes. Must be present if sync is
> +# "bitmap". Must NOT be present otherwise. (Since 9.0)
> +#
> # @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
> @@ -2557,7 +2577,8 @@
> { 'command': 'blockdev-mirror',
>'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
> '*replaces': 'str',
> -'sync': 'MirrorSyncMode',
> +'sync': 'MirrorSyncMode', '*bitmap': 'str',
> +'*bitmap-mode': 'BitmapSyncMode',
> '*speed': 'int', '*granularity': 'uint32',
> '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
> '*on-target-error': 'BlockdevOnError',
[...]
Re: [PATCH v7 2/4] qcow2: add configurations for zoned format extension
Sam Li writes:
> Markus Armbruster 于2024年2月19日周一 21:42写道：
>>
>> Sam Li writes:
>>
>> > Markus Armbruster 于2024年2月19日周一 16:56写道：
>> >>
>> >> Sam Li writes:
>> >>
>> >> > Markus Armbruster 于2024年2月19日周一 15:40写道：
>> >> >>
>> >> >> Sam Li writes:
>> >> >>
>> >> >> > Markus Armbruster 于2024年2月19日周一 13:05写道：
>> >> >> >>
>> >> >> >> One more thing...
>> >> >> >>
>> >> >> >> Markus Armbruster writes:
>> >> >> >>
>> >> >> >> > I apologize for the delayed review.
>> >> >> >
>> >> >> > No problems. Thanks for reviewing!
>> >> >> >
>> >> >> >> >
>> >> >> >> > Sam Li writes:
>> >> >> >> >
>> >> >> >> >> To configure the zoned format feature on the qcow2 driver, it
>> >> >> >> >> requires settings as: the device size, zone model, zone size,
>> >> >> >> >> zone capacity, number of conventional zones, limits on zone
>> >> >> >> >> resources (max append bytes, max open zones, and
>> >> >> >> >> max_active_zones).
>> >> >> >> >>
>> >> >> >> >> To create a qcow2 image with zoned format feature, use command
>> >> >> >> >> like
>> >> >> >> >> this:
>> >> >> >> >> qemu-img create -f qcow2 zbc.qcow2 -o size=768M \
>> >> >> >> >> -o zone.size=64M -o zone.capacity=64M -o
>> >> >> >> >> zone.conventional_zones=0 \
>> >> >> >> >> -o zone.max_append_bytes=4096 -o zone.max_open_zones=6 \
>> >> >> >> >> -o zone.max_active_zones=8 -o zone.mode=host-managed
>> >> >> >> >>
>> >> >> >> >> Signed-off-by: Sam Li
>> >> >> >> >
>> >> >> >> > [...]
>> >> >> >> >
>> >> >> >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> >> >> >> >> index ca390c5700..e2e0ec21a5 100644
>> >> >> >> >> --- a/qapi/block-core.json
>> >> >> >> >> +++ b/qapi/block-core.json
>> >> >> >> >> @@ -5038,6 +5038,67 @@
>> >> >> >> >> { 'enum': 'Qcow2CompressionType',
>> >> >> >> >>'data': [ 'zlib', { 'name': 'zstd', 'if': 'CONFIG_ZSTD' } ] }
>> >> >> >> >>
>> >> >> >> >> +##
>> >> >> >> >> +# @Qcow2ZoneModel:
>> >> >> >> >> +#
>> >> >> >> >> +# Zoned device model used in qcow2 image file
>> >> >> >> >> +#
>> >> >> >> >> +# @host-managed: The host-managed model only allows sequential
>> >> >> >> >> write over the
>> >> >> >> >> +# device zones.
>> >> >> >> >> +#
>> >> >> >> >> +# Since 8.2
>> >> >> >> >> +##
>> >> >> >> >> +{ 'enum': 'Qcow2ZoneModel',
>> >> >> >> >> + 'data': [ 'host-managed'] }
>> >> >> >> >> +
>> >> >> >> >> +##
>> >> >> >> >> +# @Qcow2ZoneHostManaged:
>> >> >> >> >> +#
>> >> >> >> >> +# The host-managed zone model. It only allows sequential
>> >> >> >> >> writes.
>> >> >> >> >> +#
>> >> >> >> >> +# @size: Total number of bytes within zones.
>> >> >> >> >
>> >> >> >> > Default?
>> >> >> >
>> >> >> > It should be set by users. No default value provided. If it's unset
>> >> >> > then it is zero and an error will be returned.
>> >> >>
>> >> >> If the user must provide @size, why is it optional then?
>> >> >
>> >> > It is not optional when the zone model is host-managed. If it's
>> >> > non-zoned, then we don't care about zone info. I am not sure how to
>> >> > make it unoptional.
>> >>
>> >> We have:
>> >>
>> >>blockdev-create argument @options of type BlockdevCreateOptions
>> >>
>> >>BlockdevCreateOptions union branch @qcow2 of type
>> >>BlockdevCreateOptionsQcow2, union tag member is @driver
>> >>
>> >>BlockdevCreateOptionsQcow2 optional member @zone of type
>> >>Qcow2ZoneCreateOptions, default not zoned
>> >>
>> >>Qcow2ZoneCreateOptions union branch @host-managed of type
>> >>Qcow2ZoneHostManaged, union tag member is @mode
>> >>
>> >>Qcow2ZoneHostManaged optional member @size of type size.
>> >>
>> >> Making this member @size mandatory means we must specify it when
>> >> BlockdevCreateOptionsQcow2 member @zone is present and @zone's member
>> >> @mode is "host-managed". Feels right to me. Am I missing anything?
>> >
>> > That's right. And the checks when creating such an img can help do
>> > that. It's not specified in the .json file directly.
>>
>> What would break if we did specify it in the QAPI schema directly?
>
> Nothing I think. We can keep the current schema and add a default zone
> size like 131072.
I believe making the member mandatory makes a lot more sense.
I guess we can keep @capacity and @max-append-bytes keep optional *if*
we can come up with sensible defaults.
>> [...]
Re: [PATCH v7 2/4] qcow2: add configurations for zoned format extension
Sam Li writes:
> Markus Armbruster 于2024年2月19日周一 16:56写道：
>>
>> Sam Li writes:
>>
>> > Markus Armbruster 于2024年2月19日周一 15:40写道：
>> >>
>> >> Sam Li writes:
>> >>
>> >> > Markus Armbruster 于2024年2月19日周一 13:05写道：
>> >> >>
>> >> >> One more thing...
>> >> >>
>> >> >> Markus Armbruster writes:
>> >> >>
>> >> >> > I apologize for the delayed review.
>> >> >
>> >> > No problems. Thanks for reviewing!
>> >> >
>> >> >> >
>> >> >> > Sam Li writes:
>> >> >> >
>> >> >> >> To configure the zoned format feature on the qcow2 driver, it
>> >> >> >> requires settings as: the device size, zone model, zone size,
>> >> >> >> zone capacity, number of conventional zones, limits on zone
>> >> >> >> resources (max append bytes, max open zones, and max_active_zones).
>> >> >> >>
>> >> >> >> To create a qcow2 image with zoned format feature, use command like
>> >> >> >> this:
>> >> >> >> qemu-img create -f qcow2 zbc.qcow2 -o size=768M \
>> >> >> >> -o zone.size=64M -o zone.capacity=64M -o zone.conventional_zones=0 \
>> >> >> >> -o zone.max_append_bytes=4096 -o zone.max_open_zones=6 \
>> >> >> >> -o zone.max_active_zones=8 -o zone.mode=host-managed
>> >> >> >>
>> >> >> >> Signed-off-by: Sam Li
>> >> >> >
>> >> >> > [...]
>> >> >> >
>> >> >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> >> >> >> index ca390c5700..e2e0ec21a5 100644
>> >> >> >> --- a/qapi/block-core.json
>> >> >> >> +++ b/qapi/block-core.json
>> >> >> >> @@ -5038,6 +5038,67 @@
>> >> >> >> { 'enum': 'Qcow2CompressionType',
>> >> >> >>'data': [ 'zlib', { 'name': 'zstd', 'if': 'CONFIG_ZSTD' } ] }
>> >> >> >>
>> >> >> >> +##
>> >> >> >> +# @Qcow2ZoneModel:
>> >> >> >> +#
>> >> >> >> +# Zoned device model used in qcow2 image file
>> >> >> >> +#
>> >> >> >> +# @host-managed: The host-managed model only allows sequential
>> >> >> >> write over the
>> >> >> >> +# device zones.
>> >> >> >> +#
>> >> >> >> +# Since 8.2
>> >> >> >> +##
>> >> >> >> +{ 'enum': 'Qcow2ZoneModel',
>> >> >> >> + 'data': [ 'host-managed'] }
>> >> >> >> +
>> >> >> >> +##
>> >> >> >> +# @Qcow2ZoneHostManaged:
>> >> >> >> +#
>> >> >> >> +# The host-managed zone model. It only allows sequential writes.
>> >> >> >> +#
>> >> >> >> +# @size: Total number of bytes within zones.
>> >> >> >
>> >> >> > Default?
>> >> >
>> >> > It should be set by users. No default value provided. If it's unset
>> >> > then it is zero and an error will be returned.
>> >>
>> >> If the user must provide @size, why is it optional then?
>> >
>> > It is not optional when the zone model is host-managed. If it's
>> > non-zoned, then we don't care about zone info. I am not sure how to
>> > make it unoptional.
>>
>> We have:
>>
>>blockdev-create argument @options of type BlockdevCreateOptions
>>
>>BlockdevCreateOptions union branch @qcow2 of type
>>BlockdevCreateOptionsQcow2, union tag member is @driver
>>
>>BlockdevCreateOptionsQcow2 optional member @zone of type
>>Qcow2ZoneCreateOptions, default not zoned
>>
>>Qcow2ZoneCreateOptions union branch @host-managed of type
>>Qcow2ZoneHostManaged, union tag member is @mode
>>
>>Qcow2ZoneHostManaged optional member @size of type size.
>>
>> Making this member @size mandatory means we must specify it when
>> BlockdevCreateOptionsQcow2 member @zone is present and @zone's member
>> @mode is "host-managed". Feels right to me. Am I missing anything?
>
> That's right. And the checks when creating such an img can help do
> that. It's not specified in the .json file directly.
What would break if we did specify it in the QAPI schema directly?
[...]
Re: [PATCH v7 2/4] qcow2: add configurations for zoned format extension
Sam Li writes:
> Markus Armbruster 于2024年2月19日周一 15:40写道：
>>
>> Sam Li writes:
>>
>> > Markus Armbruster 于2024年2月19日周一 13:05写道：
>> >>
>> >> One more thing...
>> >>
>> >> Markus Armbruster writes:
>> >>
>> >> > I apologize for the delayed review.
>> >
>> > No problems. Thanks for reviewing!
>> >
>> >> >
>> >> > Sam Li writes:
>> >> >
>> >> >> To configure the zoned format feature on the qcow2 driver, it
>> >> >> requires settings as: the device size, zone model, zone size,
>> >> >> zone capacity, number of conventional zones, limits on zone
>> >> >> resources (max append bytes, max open zones, and max_active_zones).
>> >> >>
>> >> >> To create a qcow2 image with zoned format feature, use command like
>> >> >> this:
>> >> >> qemu-img create -f qcow2 zbc.qcow2 -o size=768M \
>> >> >> -o zone.size=64M -o zone.capacity=64M -o zone.conventional_zones=0 \
>> >> >> -o zone.max_append_bytes=4096 -o zone.max_open_zones=6 \
>> >> >> -o zone.max_active_zones=8 -o zone.mode=host-managed
>> >> >>
>> >> >> Signed-off-by: Sam Li
>> >> >
>> >> > [...]
>> >> >
>> >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> >> >> index ca390c5700..e2e0ec21a5 100644
>> >> >> --- a/qapi/block-core.json
>> >> >> +++ b/qapi/block-core.json
>> >> >> @@ -5038,6 +5038,67 @@
>> >> >> { 'enum': 'Qcow2CompressionType',
>> >> >>'data': [ 'zlib', { 'name': 'zstd', 'if': 'CONFIG_ZSTD' } ] }
>> >> >>
>> >> >> +##
>> >> >> +# @Qcow2ZoneModel:
>> >> >> +#
>> >> >> +# Zoned device model used in qcow2 image file
>> >> >> +#
>> >> >> +# @host-managed: The host-managed model only allows sequential write
>> >> >> over the
>> >> >> +# device zones.
>> >> >> +#
>> >> >> +# Since 8.2
>> >> >> +##
>> >> >> +{ 'enum': 'Qcow2ZoneModel',
>> >> >> + 'data': [ 'host-managed'] }
>> >> >> +
>> >> >> +##
>> >> >> +# @Qcow2ZoneHostManaged:
>> >> >> +#
>> >> >> +# The host-managed zone model. It only allows sequential writes.
>> >> >> +#
>> >> >> +# @size: Total number of bytes within zones.
>> >> >
>> >> > Default?
>> >
>> > It should be set by users. No default value provided. If it's unset
>> > then it is zero and an error will be returned.
>>
>> If the user must provide @size, why is it optional then?
>
> It is not optional when the zone model is host-managed. If it's
> non-zoned, then we don't care about zone info. I am not sure how to
> make it unoptional.
We have:
blockdev-create argument @options of type BlockdevCreateOptions
BlockdevCreateOptions union branch @qcow2 of type
BlockdevCreateOptionsQcow2, union tag member is @driver
BlockdevCreateOptionsQcow2 optional member @zone of type
Qcow2ZoneCreateOptions, default not zoned
Qcow2ZoneCreateOptions union branch @host-managed of type
Qcow2ZoneHostManaged, union tag member is @mode
Qcow2ZoneHostManaged optional member @size of type size.
Making this member @size mandatory means we must specify it when
BlockdevCreateOptionsQcow2 member @zone is present and @zone's member
@mode is "host-managed". Feels right to me. Am I missing anything?
>>
>> >> >
>> >> >> +#
>> >> >> +# @capacity: The number of usable logical blocks within zones
>> >> >> +# in bytes. A zone capacity is always smaller or equal to the
>> >> >> +# zone size.
>> >> >
>> >> > Default?
>> >
>> > Same.
>> >
>> >> >
>> >> >> +# @max-append-bytes: The maximal number of bytes of a zone
>> >> >> +# append request that can be issued to the device. It must be
>> >> >> +# 512-byte aligned and less than the zone capacity.
>> >> >
>> >> > Default?
>> >
>> > Same.
>> >
>> > For those values, I guess it could be set when users provide no
>> > information and still want a workable emulated zoned block device.
>> >
>> >> >
>> >> >> +#
>> >> >> +# Since 8.2
>> >> >> +##
>> >> >> +{ 'struct': 'Qcow2ZoneHostManaged',
>> >> >> + 'data': { '*size': 'size',
>> >> >> +'*capacity': 'size',
>> >> >> +'*conventional-zones': 'uint32',
>> >> >> +'*max-open-zones': 'uint32',
>> >> >> +'*max-active-zones': 'uint32',
>> >> >> +'*max-append-bytes': 'size' } }
>>
>> [...]
>>
Re: [PATCH v7 2/4] qcow2: add configurations for zoned format extension
Sam Li writes:
> Markus Armbruster 于2024年2月19日周一 13:05写道：
>>
>> One more thing...
>>
>> Markus Armbruster writes:
>>
>> > I apologize for the delayed review.
>
> No problems. Thanks for reviewing!
>
>> >
>> > Sam Li writes:
>> >
>> >> To configure the zoned format feature on the qcow2 driver, it
>> >> requires settings as: the device size, zone model, zone size,
>> >> zone capacity, number of conventional zones, limits on zone
>> >> resources (max append bytes, max open zones, and max_active_zones).
>> >>
>> >> To create a qcow2 image with zoned format feature, use command like
>> >> this:
>> >> qemu-img create -f qcow2 zbc.qcow2 -o size=768M \
>> >> -o zone.size=64M -o zone.capacity=64M -o zone.conventional_zones=0 \
>> >> -o zone.max_append_bytes=4096 -o zone.max_open_zones=6 \
>> >> -o zone.max_active_zones=8 -o zone.mode=host-managed
>> >>
>> >> Signed-off-by: Sam Li
>> >
>> > [...]
>> >
>> >> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> >> index ca390c5700..e2e0ec21a5 100644
>> >> --- a/qapi/block-core.json
>> >> +++ b/qapi/block-core.json
>> >> @@ -5038,6 +5038,67 @@
>> >> { 'enum': 'Qcow2CompressionType',
>> >>'data': [ 'zlib', { 'name': 'zstd', 'if': 'CONFIG_ZSTD' } ] }
>> >>
>> >> +##
>> >> +# @Qcow2ZoneModel:
>> >> +#
>> >> +# Zoned device model used in qcow2 image file
>> >> +#
>> >> +# @host-managed: The host-managed model only allows sequential write
>> >> over the
>> >> +# device zones.
>> >> +#
>> >> +# Since 8.2
>> >> +##
>> >> +{ 'enum': 'Qcow2ZoneModel',
>> >> + 'data': [ 'host-managed'] }
>> >> +
>> >> +##
>> >> +# @Qcow2ZoneHostManaged:
>> >> +#
>> >> +# The host-managed zone model. It only allows sequential writes.
>> >> +#
>> >> +# @size: Total number of bytes within zones.
>> >
>> > Default?
>
> It should be set by users. No default value provided. If it's unset
> then it is zero and an error will be returned.
If the user must provide @size, why is it optional then?
>> >
>> >> +#
>> >> +# @capacity: The number of usable logical blocks within zones
>> >> +# in bytes. A zone capacity is always smaller or equal to the
>> >> +# zone size.
>> >
>> > Default?
>
> Same.
>
>> >
>> >> +# @max-append-bytes: The maximal number of bytes of a zone
>> >> +# append request that can be issued to the device. It must be
>> >> +# 512-byte aligned and less than the zone capacity.
>> >
>> > Default?
>
> Same.
>
> For those values, I guess it could be set when users provide no
> information and still want a workable emulated zoned block device.
>
>> >
>> >> +#
>> >> +# Since 8.2
>> >> +##
>> >> +{ 'struct': 'Qcow2ZoneHostManaged',
>> >> + 'data': { '*size': 'size',
>> >> +'*capacity': 'size',
>> >> +'*conventional-zones': 'uint32',
>> >> +'*max-open-zones': 'uint32',
>> >> +'*max-active-zones': 'uint32',
>> >> +'*max-append-bytes': 'size' } }
[...]
Re: [PATCH v7 2/4] qcow2: add configurations for zoned format extension
One more thing...
Markus Armbruster writes:
> I apologize for the delayed review.
>
> Sam Li writes:
>
>> To configure the zoned format feature on the qcow2 driver, it
>> requires settings as: the device size, zone model, zone size,
>> zone capacity, number of conventional zones, limits on zone
>> resources (max append bytes, max open zones, and max_active_zones).
>>
>> To create a qcow2 image with zoned format feature, use command like
>> this:
>> qemu-img create -f qcow2 zbc.qcow2 -o size=768M \
>> -o zone.size=64M -o zone.capacity=64M -o zone.conventional_zones=0 \
>> -o zone.max_append_bytes=4096 -o zone.max_open_zones=6 \
>> -o zone.max_active_zones=8 -o zone.mode=host-managed
>>
>> Signed-off-by: Sam Li
>
> [...]
>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index ca390c5700..e2e0ec21a5 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -5038,6 +5038,67 @@
>> { 'enum': 'Qcow2CompressionType',
>>'data': [ 'zlib', { 'name': 'zstd', 'if': 'CONFIG_ZSTD' } ] }
>>
>> +##
>> +# @Qcow2ZoneModel:
>> +#
>> +# Zoned device model used in qcow2 image file
>> +#
>> +# @host-managed: The host-managed model only allows sequential write over
>> the
>> +# device zones.
>> +#
>> +# Since 8.2
>> +##
>> +{ 'enum': 'Qcow2ZoneModel',
>> + 'data': [ 'host-managed'] }
>> +
>> +##
>> +# @Qcow2ZoneHostManaged:
>> +#
>> +# The host-managed zone model. It only allows sequential writes.
>> +#
>> +# @size: Total number of bytes within zones.
>
> Default?
>
>> +#
>> +# @capacity: The number of usable logical blocks within zones
>> +# in bytes. A zone capacity is always smaller or equal to the
>> +# zone size.
>
> Default?
>
>> +#
>> +# @conventional-zones: The number of conventional zones of the
>> +# zoned device (default 0).
>> +#
>> +# @max-open-zones: The maximal number of open zones. It is less than
>> +# or equal to the number of sequential write required zones of
>> +# the device (default 0).
>> +#
>> +# @max-active-zones: The maximal number of zones in the implicit
>> +# open, explicit open or closed state. It is less than or equal
>> +# to the max open zones (default 0).
>> +#
>> +# @max-append-bytes: The maximal number of bytes of a zone
>> +# append request that can be issued to the device. It must be
>> +# 512-byte aligned and less than the zone capacity.
>
> Default?
>
>> +#
>> +# Since 8.2
>> +##
>> +{ 'struct': 'Qcow2ZoneHostManaged',
>> + 'data': { '*size': 'size',
>> +'*capacity': 'size',
>> +'*conventional-zones': 'uint32',
>> +'*max-open-zones': 'uint32',
>> +'*max-active-zones': 'uint32',
>> +'*max-append-bytes': 'size' } }
>> +
>> +##
>> +# @Qcow2ZoneCreateOptions:
>> +#
>> +# The zone device model for the qcow2 image.
Please document member @mode.
Fails to build since merge commit 61e7a0d27c1:
qapi/block-core.json: In union 'Qcow2ZoneCreateOptions':
qapi/block-core.json:5135: member 'mode' lacks documentation
>> +#
>> +# Since 8.2
>> +##
>> +{ 'union': 'Qcow2ZoneCreateOptions',
>> + 'base': { 'mode': 'Qcow2ZoneModel' },
>> + 'discriminator': 'mode',
>> + 'data': { 'host-managed': 'Qcow2ZoneHostManaged' } }
>> +
>> ##
>> # @BlockdevCreateOptionsQcow2:
>> #
>> @@ -5080,6 +5141,9 @@
>> # @compression-type: The image cluster compression method
>> # (default: zlib, since 5.1)
>> #
>> +# @zone: The zone device model modes. The default is that the device is
>> +# not zoned. (since 8.2)
>> +#
>> # Since: 2.12
>> ##
>> { 'struct': 'BlockdevCreateOptionsQcow2',
>> @@ -5096,7 +5160,8 @@
>> '*preallocation': 'PreallocMode',
>> '*lazy-refcounts': 'bool',
>> '*refcount-bits': 'int',
>> -'*compression-type':'Qcow2CompressionType' } }
>> +'*compression-type':'Qcow2CompressionType',
>> +'*zone':'Qcow2ZoneCreateOptions' } }
>>
>> ##
>> # @BlockdevCreateOptionsQed:
Re: [PATCH v7 2/4] qcow2: add configurations for zoned format extension
I apologize for the delayed review.
Sam Li writes:
> To configure the zoned format feature on the qcow2 driver, it
> requires settings as: the device size, zone model, zone size,
> zone capacity, number of conventional zones, limits on zone
> resources (max append bytes, max open zones, and max_active_zones).
>
> To create a qcow2 image with zoned format feature, use command like
> this:
> qemu-img create -f qcow2 zbc.qcow2 -o size=768M \
> -o zone.size=64M -o zone.capacity=64M -o zone.conventional_zones=0 \
> -o zone.max_append_bytes=4096 -o zone.max_open_zones=6 \
> -o zone.max_active_zones=8 -o zone.mode=host-managed
>
> Signed-off-by: Sam Li
[...]
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index ca390c5700..e2e0ec21a5 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -5038,6 +5038,67 @@
> { 'enum': 'Qcow2CompressionType',
>'data': [ 'zlib', { 'name': 'zstd', 'if': 'CONFIG_ZSTD' } ] }
>
> +##
> +# @Qcow2ZoneModel:
> +#
> +# Zoned device model used in qcow2 image file
> +#
> +# @host-managed: The host-managed model only allows sequential write over the
> +# device zones.
> +#
> +# Since 8.2
> +##
> +{ 'enum': 'Qcow2ZoneModel',
> + 'data': [ 'host-managed'] }
> +
> +##
> +# @Qcow2ZoneHostManaged:
> +#
> +# The host-managed zone model. It only allows sequential writes.
> +#
> +# @size: Total number of bytes within zones.
Default?
> +#
> +# @capacity: The number of usable logical blocks within zones
> +# in bytes. A zone capacity is always smaller or equal to the
> +# zone size.
Default?
> +#
> +# @conventional-zones: The number of conventional zones of the
> +# zoned device (default 0).
> +#
> +# @max-open-zones: The maximal number of open zones. It is less than
> +# or equal to the number of sequential write required zones of
> +# the device (default 0).
> +#
> +# @max-active-zones: The maximal number of zones in the implicit
> +# open, explicit open or closed state. It is less than or equal
> +# to the max open zones (default 0).
> +#
> +# @max-append-bytes: The maximal number of bytes of a zone
> +# append request that can be issued to the device. It must be
> +# 512-byte aligned and less than the zone capacity.
Default?
> +#
> +# Since 8.2
> +##
> +{ 'struct': 'Qcow2ZoneHostManaged',
> + 'data': { '*size': 'size',
> +'*capacity': 'size',
> +'*conventional-zones': 'uint32',
> +'*max-open-zones': 'uint32',
> +'*max-active-zones': 'uint32',
> +'*max-append-bytes': 'size' } }
> +
> +##
> +# @Qcow2ZoneCreateOptions:
> +#
> +# The zone device model for the qcow2 image.
> +#
> +# Since 8.2
> +##
> +{ 'union': 'Qcow2ZoneCreateOptions',
> + 'base': { 'mode': 'Qcow2ZoneModel' },
> + 'discriminator': 'mode',
> + 'data': { 'host-managed': 'Qcow2ZoneHostManaged' } }
> +
> ##
> # @BlockdevCreateOptionsQcow2:
> #
> @@ -5080,6 +5141,9 @@
> # @compression-type: The image cluster compression method
> # (default: zlib, since 5.1)
> #
> +# @zone: The zone device model modes. The default is that the device is
> +# not zoned. (since 8.2)
> +#
> # Since: 2.12
> ##
> { 'struct': 'BlockdevCreateOptionsQcow2',
> @@ -5096,7 +5160,8 @@
> '*preallocation': 'PreallocMode',
> '*lazy-refcounts': 'bool',
> '*refcount-bits': 'int',
> -'*compression-type':'Qcow2CompressionType' } }
> +'*compression-type':'Qcow2CompressionType',
> +'*zone':'Qcow2ZoneCreateOptions' } }
>
> ##
> # @BlockdevCreateOptionsQed:
Re: [PATCH 06/15] qapi: Require member documentation (with loophole)
Daniel P. Berrangé writes: > On Mon, Feb 05, 2024 at 08:47:00AM +0100, Markus Armbruster wrote: >> The QAPI generator forces you to document your stuff. Except for >> command arguments, event data, and members of enum and object types: >> these the generator silently "documents" as "Not documented". >> >> We can't require proper documentation there without first fixing all >> the offenders. We've always had too many offenders to pull that off. >> Right now, we have more than 500. Worse, we seem to fix old ones no >> faster than we add new ones: in the past year, we fixed 22 ones, but >> added 26 new ones. >> >> To help arrest the backsliding, make missing documentation an error >> unless the command, type, or event is in listed in new pragma >> documentation-exceptions. > > If we want to really annoy people we could print a warning to > stderr during docs generation, for each undocumented field. > The many pages of warnings would likely trigger a much quicker > series to patches to fix it to eliminate the annoying warnings :-) Heh. Let's give not annoying people that much a try. Can always escalate later :) [...]
Re: [PATCH 09/15] qga/qapi-schema: Plug trivial documentation holes
Daniel P. Berrangé writes: > On Mon, Feb 05, 2024 at 08:47:03AM +0100, Markus Armbruster wrote: >> Add missing return member documentation of guest-get-disks, >> guest-get-devices, guest-get-diskstats, and guest-get-cpustats. >> >> The NVMe SMART information returned by guest-getdisks remains >> undocumented. Add a TODO there. >> >> Signed-off-by: Markus Armbruster >> --- >> qga/qapi-schema.json | 24 ++-- >> 1 file changed, 14 insertions(+), 10 deletions(-) > > Reviewed-by: Daniel P. Berrangé > >> @@ -978,7 +974,7 @@ >> # >> # Disk type related smart information. >> # >> -# - @nvme: NVMe disk smart >> +# @type: disk bus type >> # >> # Since: 7.1 >> ## > > Fun, so not only were the fields that exist not documented, > but we've documented fields which don't exist. I think the @nvme line tried to document the branch. Not useful; the doc generator takes care of that: GuestDiskSmart (Object) --- Disk type related smart information. * nvme: NVMe disk smart Members ~~~ "type": "GuestDiskBusType" Not documented --> The members of "GuestNVMeSmart" when "type" is "nvme" >> @@ -1780,7 +1784,7 @@ >> # >> # Get statistics of each CPU in millisecond. >> # >> -# - @linux: Linux style CPU statistics >> +# @type: guest operating system >> # >> # Since: 7.1 >> ## > > And more things which don't exist ! Likewise.
Re: [PATCH 5/5] monitor: use aio_co_reschedule_self()
Stefan Hajnoczi writes: > The aio_co_reschedule_self() API is designed to avoid the race > condition between scheduling the coroutine in another AioContext and > yielding. > > The QMP dispatch code uses the open-coded version that appears > susceptible to the race condition at first glance: > > aio_co_schedule(qemu_get_aio_context(), qemu_coroutine_self()); > qemu_coroutine_yield(); > > The code is actually safe because the iohandler and qemu_aio_context > AioContext run under the Big QEMU Lock. Nevertheless, set a good example > and use aio_co_reschedule_self() so it's obvious that there is no race. > > Suggested-by: Hanna Reitz > Signed-off-by: Stefan Hajnoczi Acked-by: Markus Armbruster Feel free to merge this together with the remainder of the series.
[PATCH 11/15] qapi/dump: Clean up documentation of DumpGuestMemoryCapability
The type's doc comment describes its member, but it's not marked up as such. Easy enough to fix. Signed-off-by: Markus Armbruster --- qapi/dump.json | 2 +- qapi/pragma.json | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/qapi/dump.json b/qapi/dump.json index 5cbc237ad9..1997c1d1d4 100644 --- a/qapi/dump.json +++ b/qapi/dump.json @@ -186,7 +186,7 @@ ## # @DumpGuestMemoryCapability: # -# A list of the available formats for dump-guest-memory +# @formats: the available formats for dump-guest-memory # # Since: 2.0 ## diff --git a/qapi/pragma.json b/qapi/pragma.json index 544f138afa..aea6384255 100644 --- a/qapi/pragma.json +++ b/qapi/pragma.json @@ -74,7 +74,6 @@ 'DummyBlockCoreForceArrays', 'DummyForceArrays', 'DummyVirtioForceArrays', -'DumpGuestMemoryCapability', 'GrabToggleKeys', 'GuestPanicInformationHyperV', 'HotKeyMod', -- 2.43.0
[PATCH 04/15] qapi: Indent tagged doc comment sections properly
docs/devel/qapi-code-gen demands that the "second and subsequent lines of sections other than "Example"/"Examples" should be indented". Commit a937b6aa739q (qapi: Reformat doc comments to conform to current conventions) missed a few instances, and messed up a few others. Clean that up. Signed-off-by: Markus Armbruster --- qapi/migration.json | 46 - qapi/misc.json | 12 + qapi/qdev.json | 12 - tests/qapi-schema/doc-good.json | 10 +++ tests/qapi-schema/doc-good.out | 2 +- 5 files changed, 42 insertions(+), 40 deletions(-) diff --git a/qapi/migration.json b/qapi/migration.json index 819708321d..bf89765a26 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -1699,24 +1699,24 @@ # # Notes: # -# 1. The 'query-migrate' command should be used to check migration's -#progress and final result (this information is provided by the -#'status' member) +# 1. The 'query-migrate' command should be used to check +#migration's progress and final result (this information is +#provided by the 'status' member) # -# 2. All boolean arguments default to false +# 2. All boolean arguments default to false # -# 3. The user Monitor's "detach" argument is invalid in QMP and should -#not be used +# 3. The user Monitor's "detach" argument is invalid in QMP and +#should not be used # -# 4. The uri argument should have the Uniform Resource Identifier of -#default destination VM. This connection will be bound to default -#network. +# 4. The uri argument should have the Uniform Resource Identifier +#of default destination VM. This connection will be bound to +#default network. # -# 5. For now, number of migration streams is restricted to one, i.e -#number of items in 'channels' list is just 1. +# 5. For now, number of migration streams is restricted to one, +#i.e number of items in 'channels' list is just 1. # -# 6. The 'uri' and 'channels' arguments are mutually exclusive; -#exactly one of the two should be present. +# 6. The 'uri' and 'channels' arguments are mutually exclusive; +#exactly one of the two should be present. # # Example: # @@ -1781,20 +1781,20 @@ # # Notes: # -# 1. It's a bad idea to use a string for the uri, but it needs -#to stay compatible with -incoming and the format of the uri -#is already exposed above libvirt. +# 1. It's a bad idea to use a string for the uri, but it needs to +#stay compatible with -incoming and the format of the uri is +#already exposed above libvirt. # -# 2. QEMU must be started with -incoming defer to allow -#migrate-incoming to be used. +# 2. QEMU must be started with -incoming defer to allow +#migrate-incoming to be used. # -# 3. The uri format is the same as for -incoming +# 3. The uri format is the same as for -incoming # -# 5. For now, number of migration streams is restricted to one, i.e -#number of items in 'channels' list is just 1. +# 5. For now, number of migration streams is restricted to one, +#i.e number of items in 'channels' list is just 1. # -# 4. The 'uri' and 'channels' arguments are mutually exclusive; -#exactly one of the two should be present. +# 4. The 'uri' and 'channels' arguments are mutually exclusive; +#exactly one of the two should be present. # # Example: # diff --git a/qapi/misc.json b/qapi/misc.json index 2ca8c39874..4108a0c951 100644 --- a/qapi/misc.json +++ b/qapi/misc.json @@ -348,9 +348,10 @@ # - If file descriptor was not received, GenericError # - If @fdset-id is a negative value, GenericError # -# Notes: The list of fd sets is shared by all monitor connections. +# Notes: +# The list of fd sets is shared by all monitor connections. # -# If @fdset-id is not specified, a new fd set will be created. +# If @fdset-id is not specified, a new fd set will be created. # # Since: 1.2 # @@ -379,10 +380,11 @@ # # Since: 1.2 # -# Notes: The list of fd sets is shared by all monitor connections. +# Notes: +# The list of fd sets is shared by all monitor connections. # -# If @fd is not specified, all file descriptors in @fdset-id will be -# removed. +# If @fd is not specified, all file descriptors in @fdset-id will +# be removed. # # Example: # diff --git a/qapi/qdev.json b/qapi/qdev.json index 25bac5e611..3b3ccfa413 100644 --- a/qapi/qdev.json +++ b/qapi/qdev.json @@ -53,14 +53,14 @@ # # Notes: # -# 1. Additional arguments depend on the type. +# 1. Additional arguments depend on the type. # -# 2. For detailed information about this command, please refer to the -#'docs/qdev-device-use.txt' file. +# 2. For detailed information about this command, please refer to +#the 'docs/qdev-device-use.txt' file. # -# 3. It's possible to list device
[PATCH 13/15] qapi: Improve documentation of file descriptor socket addresses
SocketAddress branch @fd is documented in enum SocketAddressType,
unlike the other branches. That's because the branch's type is String
from common.json.
Use a local copy of String, so we can put the documentation in the
usual place.
Signed-off-by: Markus Armbruster
---
qapi/sockets.json | 40 +-
include/hw/virtio/vhost-vsock-common.h | 1 +
chardev/char-socket.c | 2 +-
util/qemu-sockets.c| 3 +-
4 files changed, 31 insertions(+), 15 deletions(-)
diff --git a/qapi/sockets.json b/qapi/sockets.json
index c3b616731d..5e6af5504d 100644
--- a/qapi/sockets.json
+++ b/qapi/sockets.json
@@ -5,8 +5,6 @@
# = Socket data types
##
-{ 'include': 'common.json' }
-
##
# @NetworkAddressFamily:
#
@@ -116,6 +114,24 @@
'cid': 'str',
'port': 'str' } }
+##
+# @FdSocketAddress:
+#
+# A file descriptor name or number.
+#
+# @str: decimal is for file descriptor number, otherwise it's a file
+# descriptor name. Named file descriptors are permitted in
+# monitor commands, in combination with the 'getfd' command.
+# Decimal file descriptors are permitted at startup or other
+# contexts where no monitor context is active.
+#
+#
+# Since: 1.2
+##
+{ 'struct': 'FdSocketAddress',
+ 'data': {
+'str': 'str' } }
+
##
# @InetSocketAddressWrapper:
#
@@ -147,12 +163,14 @@
'data': { 'data': 'VsockSocketAddress' } }
##
-# @StringWrapper:
+# @FdSocketAddressWrapper:
+#
+# @data: file descriptor name or number
#
# Since: 1.3
##
-{ 'struct': 'StringWrapper',
- 'data': { 'data': 'String' } }
+{ 'struct': 'FdSocketAddressWrapper',
+ 'data': { 'data': 'FdSocketAddress' } }
##
# @SocketAddressLegacy:
@@ -173,7 +191,7 @@
'inet': 'InetSocketAddressWrapper',
'unix': 'UnixSocketAddressWrapper',
'vsock': 'VsockSocketAddressWrapper',
-'fd': 'StringWrapper' } }
+'fd': 'FdSocketAddressWrapper' } }
##
# @SocketAddressType:
@@ -186,11 +204,7 @@
#
# @vsock: VMCI address
#
-# @fd: decimal is for file descriptor number, otherwise a file
-# descriptor name. Named file descriptors are permitted in
-# monitor commands, in combination with the 'getfd' command.
-# Decimal file descriptors are permitted at startup or other
-# contexts where no monitor context is active.
+# @fd: Socket file descriptor
#
# Since: 2.9
##
@@ -200,7 +214,7 @@
##
# @SocketAddress:
#
-# Captures the address of a socket, which could also be a named file
+# Captures the address of a socket, which could also be a socket file
# descriptor
#
# @type: Transport type
@@ -213,4 +227,4 @@
'data': { 'inet': 'InetSocketAddress',
'unix': 'UnixSocketAddress',
'vsock': 'VsockSocketAddress',
-'fd': 'String' } }
+'fd': 'FdSocketAddress' } }
diff --git a/include/hw/virtio/vhost-vsock-common.h
b/include/hw/virtio/vhost-vsock-common.h
index 93c782101d..75a74e8a99 100644
--- a/include/hw/virtio/vhost-vsock-common.h
+++ b/include/hw/virtio/vhost-vsock-common.h
@@ -11,6 +11,7 @@
#ifndef QEMU_VHOST_VSOCK_COMMON_H
#define QEMU_VHOST_VSOCK_COMMON_H
+#include "qapi/qapi-types-common.h"
#include "hw/virtio/virtio.h"
#include "hw/virtio/vhost.h"
#include "qom/object.h"
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 73947da188..ff8f845cca 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1504,7 +1504,7 @@ static void qemu_chr_parse_socket(QemuOpts *opts,
ChardevBackend *backend,
};
} else {
addr->type = SOCKET_ADDRESS_TYPE_FD;
-addr->u.fd.data = g_new(String, 1);
+addr->u.fd.data = g_new(FdSocketAddress, 1);
addr->u.fd.data->str = g_strdup(fd);
}
sock->addr = addr;
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 83e84b1186..60c44b2b56 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -1464,7 +1464,8 @@ SocketAddress *socket_address_flatten(SocketAddressLegacy
*addr_legacy)
break;
case SOCKET_ADDRESS_TYPE_FD:
addr->type = SOCKET_ADDRESS_TYPE_FD;
-QAPI_CLONE_MEMBERS(String, >u.fd, addr_legacy->u.fd.data);
+QAPI_CLONE_MEMBERS(FdSocketAddress, >u.fd,
+ addr_legacy->u.fd.data);
break;
default:
abort();
--
2.43.0
[PATCH 01/15] docs/devel/qapi-code-gen: Normalize version refs x.y.0 to just x.y
Missed in commit 9bc6e893b72 (qapi: Normalize version references x.y.0
to just x.y).
Signed-off-by: Markus Armbruster
---
docs/devel/qapi-code-gen.rst | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
index 76be722f4c..13d38dbb09 100644
--- a/docs/devel/qapi-code-gen.rst
+++ b/docs/devel/qapi-code-gen.rst
@@ -1023,7 +1023,7 @@ For example::
#
# ... more members ...
#
- # Since: 0.14.0
+ # Since: 0.14
##
{ 'struct': 'BlockStats',
'data': {'*device': 'str', '*node-name': 'str',
@@ -1039,7 +1039,7 @@ For example::
#
# Returns: A list of @BlockStats for each virtual block devices.
#
- # Since: 0.14.0
+ # Since: 0.14
#
# Example:
#
--
2.43.0
[PATCH 10/15] qapi/yank: Clean up documentaion of yank
The command's doc comment describes the argument, but it's not marked up as such. Easy enough to fix. Signed-off-by: Markus Armbruster --- qapi/pragma.json | 3 +-- qapi/yank.json | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/qapi/pragma.json b/qapi/pragma.json index 0fa64742b5..544f138afa 100644 --- a/qapi/pragma.json +++ b/qapi/pragma.json @@ -148,8 +148,7 @@ 'query-rocker', 'query-rocker-ports', 'query-stats-schemas', -'watchdog-set-action', -'yank' ], +'watchdog-set-action' ], # Externally visible types whose member names may use uppercase 'member-name-exceptions': [ # visible in: 'ACPISlotType', # query-acpi-ospm-status diff --git a/qapi/yank.json b/qapi/yank.json index 60eda20816..bfc71a07a6 100644 --- a/qapi/yank.json +++ b/qapi/yank.json @@ -74,7 +74,7 @@ # Try to recover from hanging QEMU by yanking the specified instances. # See @YankInstance for more information. # -# Takes a list of @YankInstance as argument. +# @instances: the instances to be yanked # # Returns: # - Nothing on success -- 2.43.0
[PATCH 09/15] qga/qapi-schema: Plug trivial documentation holes
Add missing return member documentation of guest-get-disks,
guest-get-devices, guest-get-diskstats, and guest-get-cpustats.
The NVMe SMART information returned by guest-getdisks remains
undocumented. Add a TODO there.
Signed-off-by: Markus Armbruster
---
qga/qapi-schema.json | 24 ++--
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index f3d168d542..b8efe31897 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -36,12 +36,6 @@
'guest-sync-delimited' ],
# Types and commands with undocumented members:
'documentation-exceptions': [
-'GuestCpuStats',
-'GuestCpuStatsType',
-'GuestDeviceId',
-'GuestDeviceType',
-'GuestDiskSmart',
-'GuestDiskStatsInfo',
'GuestNVMeSmart' ] } }
##
@@ -944,6 +938,8 @@
# NVMe smart information, based on NVMe specification, section
#
#
+# TODO: document members briefly
+#
# Since: 7.1
##
{ 'struct': 'GuestNVMeSmart',
@@ -978,7 +974,7 @@
#
# Disk type related smart information.
#
-# - @nvme: NVMe disk smart
+# @type: disk bus type
#
# Since: 7.1
##
@@ -1499,6 +1495,8 @@
##
# @GuestDeviceType:
+#
+# @pci: PCI device
##
{ 'enum': 'GuestDeviceType',
'data': [ 'pci' ] }
@@ -1518,7 +1516,9 @@
##
# @GuestDeviceId:
#
-# Id of the device - @pci: PCI ID, since: 5.2
+# Id of the device
+#
+# @type: device type
#
# Since: 5.2
##
@@ -1700,6 +1700,8 @@
# @major: major device number of disk
#
# @minor: minor device number of disk
+#
+# @stats: I/O statistics
##
{ 'struct': 'GuestDiskStatsInfo',
'data': {'name': 'str',
@@ -1723,7 +1725,9 @@
##
# @GuestCpuStatsType:
#
-# An enumeration of OS type
+# Guest operating systems supporting CPU statistics
+#
+# @linux: Linux
#
# Since: 7.1
##
@@ -1780,7 +1784,7 @@
#
# Get statistics of each CPU in millisecond.
#
-# - @linux: Linux style CPU statistics
+# @type: guest operating system
#
# Since: 7.1
##
--
2.43.0
