Re: [PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo
In the title: "qmp:"
Peter Maydell writes:
> The 'singlestep' member of StatusInfo has never done what
> the QMP documentation claims it does. What it actually
> reports is whether TCG is working in "one guest instruction
> per translation block" mode.
>
> Create a new 'one-insn-per-tb' member whose name matches
> what the field actually does and the new command line
> options. Deprecate the old 'singlestep' field.
>
> Signed-off-by: Peter Maydell
> ---
> docs/about/deprecated.rst | 10 ++
> docs/interop/qmp-intro.txt | 1 +
> qapi/run-state.json | 17 ++---
> softmmu/runstate-hmp-cmds.c | 2 +-
> softmmu/runstate.c | 6 --
> 5 files changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 6f5e689aa45..dd36becdf3b 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -199,6 +199,16 @@ accepted incorrect commands will return an error. Users
> should make sure that
> all arguments passed to ``device_add`` are consistent with the documented
> property types.
>
> +``StatusInfo`` member ``singlestep`` (since 8.1)
> +
> +
> +The ``singlestep`` member of the ``StatusInfo`` returned from
> +the ``query-status`` command is deprecated, because its name
> +is confusing and it never did what the documentation claimed
> +or what its name suggests. Use the ``one-insn-per-tb``
> +member instead, which reports the same information the old
> +``singlestep`` member did but under a clearer name.
> +
> Human Monitor Protocol (HMP) commands
> -
>
> diff --git a/docs/interop/qmp-intro.txt b/docs/interop/qmp-intro.txt
> index 1c745a7af04..b22916b23df 100644
> --- a/docs/interop/qmp-intro.txt
> +++ b/docs/interop/qmp-intro.txt
> @@ -73,6 +73,7 @@ Escape character is '^]'.
> { "execute": "query-status" }
> {
> "return": {
> +"one-insn-per-tb": false,
> "status": "prelaunch",
> "singlestep": false,
> "running": false
> diff --git a/qapi/run-state.json b/qapi/run-state.json
> index 9d34afa39e0..1de8c5c55d0 100644
> --- a/qapi/run-state.json
> +++ b/qapi/run-state.json
> @@ -104,16 +104,27 @@
> #
> # @running: true if all VCPUs are runnable, false if not runnable
> #
> -# @singlestep: true if VCPUs are in single-step mode
> +# @one-insn-per-tb: true if using TCG with one guest instruction
> +# per translation block
> +#
> +# @singlestep: deprecated synonym for @one-insn-per-tb
> #
> # @status: the virtual machine @RunState
> #
> +# Features:
> +# @deprecated: Member 'singlestep' is deprecated. Use @one-insn-per-tb
> instead.
Wrap this line, please.
> +#
> # Since: 0.14
> #
> -# Notes: @singlestep is enabled through the GDB stub
> +# Notes: @one-insn-per-tb is enabled on the command line with
> +#'-accel tcg,one-insn-per-tb=on', or with the HMP
> +#'one-insn-per-tb' command.
> ##
Hmm. We report it in query-status, which means it's relevant to QMP
clients. We provide the command to control it only in HMP, which means
it's not relevant to QMP clients.
Why is reading it relevant to QMP clients, but not writing?
Use cases for reading it via QMP query-status?
Have you considered tacking feature 'unstable' to it?
> { 'struct': 'StatusInfo',
> - 'data': {'running': 'bool', 'singlestep': 'bool', 'status': 'RunState'} }
> + 'data': {'running': 'bool',
> + 'singlestep': { 'type': 'bool', 'features': [ 'deprecated' ]},
> + 'one-insn-per-tb': 'bool',
> + 'status': 'RunState'} }
>
> ##
> # @query-status:
I see a bunch of query-status results in
tests/qemu-iotests/{183,234,262,280}.out. Do they need an update?
[...]
Re: [PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo
On Tue, 4 Apr 2023 at 09:25, Markus Armbruster wrote:
>
> In the title: "qmp:"
>
> Peter Maydell writes:
> > diff --git a/qapi/run-state.json b/qapi/run-state.json
> > index 9d34afa39e0..1de8c5c55d0 100644
> > --- a/qapi/run-state.json
> > +++ b/qapi/run-state.json
> > @@ -104,16 +104,27 @@
> > #
> > # @running: true if all VCPUs are runnable, false if not runnable
> > #
> > -# @singlestep: true if VCPUs are in single-step mode
> > +# @one-insn-per-tb: true if using TCG with one guest instruction
> > +# per translation block
> > +#
> > +# @singlestep: deprecated synonym for @one-insn-per-tb
> > #
> > # @status: the virtual machine @RunState
> > #
> > +# Features:
> > +# @deprecated: Member 'singlestep' is deprecated. Use @one-insn-per-tb
> > instead.
>
> Wrap this line, please.
>
> > +#
> > # Since: 0.14
> > #
> > -# Notes: @singlestep is enabled through the GDB stub
> > +# Notes: @one-insn-per-tb is enabled on the command line with
> > +#'-accel tcg,one-insn-per-tb=on', or with the HMP
> > +#'one-insn-per-tb' command.
> > ##
>
> Hmm. We report it in query-status, which means it's relevant to QMP
> clients. We provide the command to control it only in HMP, which means
> it's not relevant to QMP clients.
>
> Why is reading it relevant to QMP clients, but not writing?
I suspect that neither is very relevant to QMP clients, but I
thought we had a requirement that HMP interfaces went
via QMP ones ? If not, we could just make the HMP query
interface directly look at the TCG property, the way the
write interface does.
I don't want to add a QMP interface for writing it unless
there's somebody who actually has a use case for it.
> Use cases for reading it via QMP query-status?
>
> Have you considered tacking feature 'unstable' to it?
Nope, because I don't know anything about what that does :-)
> > { 'struct': 'StatusInfo',
> > - 'data': {'running': 'bool', 'singlestep': 'bool', 'status': 'RunState'} }
> > + 'data': {'running': 'bool',
> > + 'singlestep': { 'type': 'bool', 'features': [ 'deprecated' ]},
> > + 'one-insn-per-tb': 'bool',
> > + 'status': 'RunState'} }
> >
> > ##
> > # @query-status:
>
> I see a bunch of query-status results in
> tests/qemu-iotests/{183,234,262,280}.out. Do they need an update?
"make check" passed, so I guess not, unless those don't run
in "make check"...
Do those .out files need exact matching output, or can they
be written to say "we don't care about what value this field
has or whether it exists" ?
thanks
-- PMM
[PATCH] coding style: Follow our own rule on comment style
In our coding style document we have examples of good and bad
code, which we mark as:
// Good
// Bad
respectively. But in the very same document we advocate for using
C style of comments over C++. Follow our own advice and switch
annotation to:
/* Good */
/* Bad */
And while at it, align these annotations within their blocks for
better readability.
Signed-off-by: Michal Privoznik
---
docs/coding-style.rst | 76 +--
1 file changed, 38 insertions(+), 38 deletions(-)
diff --git a/docs/coding-style.rst b/docs/coding-style.rst
index 92f6099d82..fe5fe9a906 100644
--- a/docs/coding-style.rst
+++ b/docs/coding-style.rst
@@ -161,24 +161,24 @@ a single space following them before the opening bracket.
E.g.
::
- if(foo) // Bad
- if (foo) // Good
+ if(foo) /* Bad */
+ if (foo) /* Good */
Function implementations must **not** have any whitespace between
the function name and the opening bracket. E.g.
::
- int foo (int wizz) // Bad
- int foo(int wizz) // Good
+ int foo (int wizz) /* Bad */
+ int foo(int wizz) /* Good */
Function calls must **not** have any whitespace between the
function name and the opening bracket. E.g.
::
- bar = foo (wizz); // Bad
- bar = foo(wizz); // Good
+ bar = foo (wizz); /* Bad */
+ bar = foo(wizz); /* Good */
Function typedefs must **not** have any whitespace between the
closing bracket of the function name and opening bracket of the
@@ -186,16 +186,16 @@ arg list. E.g.
::
- typedef int (*foo) (int wizz); // Bad
- typedef int (*foo)(int wizz); // Good
+ typedef int (*foo) (int wizz); /* Bad */
+ typedef int (*foo)(int wizz); /* Good */
There must not be any whitespace immediately following any opening
bracket, or immediately prior to any closing bracket. E.g.
::
- int foo( int wizz ); // Bad
- int foo(int wizz);// Good
+ int foo( int wizz ); /* Bad */
+ int foo(int wizz);/* Good */
Commas
--
@@ -206,8 +206,8 @@ syntax-check'.
::
- call(a,b ,c);// Bad
- call(a, b, c); // Good
+ call(a,b ,c); /* Bad */
+ call(a, b, c);/* Good */
When declaring an enum or using a struct initializer that occupies
more than one line, use a trailing comma. That way, future edits
@@ -225,11 +225,11 @@ C99 allows trailing commas, remember that JSON and XDR do
not.
enum {
VALUE_ONE,
- VALUE_TWO // Bad
+ VALUE_TWO /* Bad */
};
enum {
VALUE_THREE,
- VALUE_FOUR, // Good
+ VALUE_FOUR, /* Good */
};
Semicolons
@@ -243,10 +243,10 @@ not enforced, loop counters generally use post-increment.
::
- for (i = 0 ;i < limit ; ++i) { // Bad
- for (i = 0; i < limit; i++) { // Good
- for (;;) { // ok
- while (1) { // Better
+ for (i = 0 ;i < limit ; ++i) {/* Bad */
+ for (i = 0; i < limit; i++) { /* Good */
+ for (;;) {/* ok */
+ while (1) { /* Better */
Empty loop bodies are better represented with curly braces and a
comment, although use of a semicolon is not currently rejected.
@@ -254,9 +254,9 @@ comment, although use of a semicolon is not currently
rejected.
::
while ((rc = waitpid(pid, &st, 0) == -1) &&
- errno == EINTR); // ok
+ errno == EINTR); /* ok */
while ((rc = waitpid(pid, &st, 0) == -1) &&
- errno == EINTR) { // Better
+ errno == EINTR) { /* Better */
/* nothing */
}
@@ -271,19 +271,19 @@ single-\ *statement* loop: each has only one *line* in
its body.
::
- while (expr) // single line body; {} is optional
+ while (expr) /* single line body; {} is optional */
single_line_stmt();
::
while (expr(arg1,
- arg2)) // indentation makes it obvious it is single line,
- single_line_stmt(); // {} is optional (not enforced either way)
+ arg2))/* indentation makes it obvious it is single line,
*/
+ single_line_stmt(); /* {} is optional (not enforced either way) */
::
while (expr1 &&
- expr2) { // multi-line, at same indentation, {} required
+ expr2) { /* multi-line, at same indentation, {} required */
single_line_stmt();
}
@@ -295,7 +295,7 @@ braces), thinking it is already a multi-statement loop:
::
- while (true) // BAD! multi-line body with no braces
+ while (true) /* BAD! multi-line body with no braces */
/* comment... */
single_line_stmt();
@@ -303,7 +303,7 @@ Do this instead:
::
- while (true) { // Always put braces around a multi-line body.
+ while (true) {/* Always put braces around a multi-line body. */
/* comment... */
single_line_stmt();
}
@@ -325,8 +325,8 @@ To reiterate, don't do this:
::
- if (expr)// BAD: no braces around...
- while (expr_2) { // ... a multi-line body
+ if (expr) /*
Re: [PATCH] coding style: Follow our own rule on comment style
On Tue, Apr 04, 2023 at 12:02:10 +0200, Michal Privoznik wrote: > In our coding style document we have examples of good and bad > code, which we mark as: > > // Good > // Bad > > respectively. But in the very same document we advocate for using > C style of comments over C++. Follow our own advice and switch > annotation to: > > /* Good */ > /* Bad */ > > And while at it, align these annotations within their blocks for > better readability. > > Signed-off-by: Michal Privoznik > --- > docs/coding-style.rst | 76 +-- > 1 file changed, 38 insertions(+), 38 deletions(-) Reviewed-by: Peter Krempa
Re: [PATCH] Conf: Move validation of Graphics Transport and StorageNetwork Socket out of parser
On 4/3/23 19:08, skran wrote:
> I have implemented your corrections successfully. Thank you.
This is not a commit message that describes what the patch does. If you
look at git log you'll see plenty of good examples. The idea is that
anybody reading through git log would understand what the commit does
without needing to read the actual diff.
> Signed-off-by: K Shiva
> ---
> src/conf/domain_conf.c | 42 +++-
> src/conf/domain_validate.c | 56 ++
> src/conf/domain_validate.h | 7 +
> 3 files changed, 67 insertions(+), 38 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 70e4d52ee6..1e0ac737bb 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5836,20 +5836,9 @@ virDomainStorageNetworkParseHost(xmlNodePtr hostnode,
>
> host->socket = virXMLPropString(hostnode, "socket");
>
> -if (host->transport == VIR_STORAGE_NET_HOST_TRANS_UNIX &&
> -host->socket == NULL) {
> -virReportError(VIR_ERR_XML_ERROR, "%s",
> - _("missing socket for unix transport"));
> +// Socket Validation
We don't use this style of comments.
https://libvirt.org/coding-style.html#code-formatting-especially-for-new-code
> +if (virDomainStorageNetHostSocketValidate(host, transport) < 0)
This doesn't really do what was suggested in the previous review though:
https://listman.redhat.com/archives/libvir-list/2023-April/239183.html
https://listman.redhat.com/archives/libvir-list/2023-April/239184.html
While it's okay to move the code into domain_validate.c file, it's also
necessary to stop calling the function from here and start calling it
from validation phase. Maybe my reasoning wasn't clear the first time.
One of the advantages of moving validation checks out from parsing phase
into validation phase is that we often introduce new validation checks.
Now, if there's an XML saved on a disk and these validation checks would
run in parsing phase we wouldn't even parse the XML document. BUT, if
they live in validation phase, then the document can be parsed and
later, when an user tries to start the domain with 'broken' XML, they
are given an error message, and can fix their XML, e.g. like this:
virsh edit myDomain
And it's true that especially for old code we still mix validation and
parsing (just like you can see in the code you're changing). The reason
is - nobody rolled up their sleeves and changed it yet. We're gradually
changing the code as we maintain it though. But there's another reason:
sometimes the cut isn't that clear. For instance, if an attribute must
be a power of two, should the check happen in parse phase or validation
phase? I'd vote for the latter, but sometimes we take a shortcut and
place it into the former.
> goto cleanup;
> -}
> -
> -if (host->transport != VIR_STORAGE_NET_HOST_TRANS_UNIX &&
> -host->socket != NULL) {
> -virReportError(VIR_ERR_XML_ERROR,
> - _("transport '%1$s' does not support socket
> attribute"),
> - transport);
> -goto cleanup;
> -}
>
> if (host->transport != VIR_STORAGE_NET_HOST_TRANS_UNIX) {
> if (!(host->name = virXMLPropString(hostnode, "name"))) {
> @@ -11004,7 +10993,6 @@
> virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDef *def,
> unsigned int flags)
> {
> int ret = -1;
> -const char *graphicsType = virDomainGraphicsTypeToString(graphics->type);
> g_autofree char *address = virXMLPropString(node, "address");
> g_autofree char *network = virXMLPropString(node, "network");
> g_autofree char *socketPath = virXMLPropString(node, "socket");
> @@ -11021,30 +11009,8 @@
> virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDef *def,
> VIR_XML_PROP_REQUIRED, &def->type) < 0)
> goto error;
>
> -switch (def->type) {
> -case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET:
> -if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
> -graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("listen type 'socket' is not available for
> graphics type '%1$s'"),
> - graphicsType);
> -goto error;
> -}
> -break;
> -case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE:
> -if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
> -graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("listen type 'none' is not available for
> graphics type '%1$s'"),
> - graphicsType);
> -goto error;
> -}
> -break;
> -case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS:
> -case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK:
Re: [libvirt PATCH 06/20] qemu_snapshot: use virDomainDiskByName while updating domain def
On Mon, Mar 13, 2023 at 16:42:07 +0100, Pavel Hrdina wrote: > When creating external snapshot this function is called only when the VM > is not running so there is only one definition to care about. However, > it will be used by external snapshot revert code for active and inactive > definition and they may be different if a disk was (un)plugged only for > the active or inactive definition. > > The current code would crash so use virDomainDiskByName() to get the > correct disk from the domain definition based on the disk name and make > sure it exists. Historically the snapshot code expects that virDomainSnapshotAlignDisks was ran and thus the disk and snapshot definitions are in sync. > Signed-off-by: Pavel Hrdina > --- > src/qemu/qemu_snapshot.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) Reviewed-by: Peter Krempa
Re: [libvirt PATCH 07/20] qemu_snapshot: introduce qemuSnapshotCreateQcow2Files
On Mon, Mar 13, 2023 at 16:42:08 +0100, Pavel Hrdina wrote: > Extract creation of qcow2 files for external snapshots to separate > function as we will need it for external snapshot revert code. Hmm, I don't think I like where this is going. I presume you want to use this code to create the new overlay images. If you want to use this also in cases where the VM was live you might run into scenarios where the qemu-img code will not be able to handle the overlay creation, specifically on networked storage. I think we'll need to create the overlay images as part of the startup of the reverted VM via QMP exactly as we are creating overlays currently for snapshots. Even when today's API will not allow reversion of non-local snapshots (I didn't check further in this series yet) I don't think we should go the way of using qemu-img at all.
Re: [libvirt PATCH 04/20] snapshot_conf: introduce metadata element
On Mon, Apr 03, 2023 at 03:45:50PM +0200, Peter Krempa wrote:
> On Mon, Mar 13, 2023 at 16:42:05 +0100, Pavel Hrdina wrote:
> > This new element will hold the new disk overlay created when reverting
> > to non-leaf snapshot in order to remember the files libvirt created.
> >
> > Signed-off-by: Pavel Hrdina
> > ---
> > src/conf/schemas/domainsnapshot.rng | 7 +++
> > src/conf/snapshot_conf.c| 27 +++
> > src/conf/snapshot_conf.h| 5 +
> > 3 files changed, 39 insertions(+)
>
> [...]
>
> > diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
> > index da1c694cb9..949104c1fd 100644
> > --- a/src/conf/snapshot_conf.c
> > +++ b/src/conf/snapshot_conf.c
>
> [...]
>
> > @@ -843,6 +859,17 @@ virDomainSnapshotDefFormatInternal(virBuffer *buf,
> > virBufferAddLit(buf, "\n");
> > }
> >
> > +if (def->nrevertdisks) {
>
> Always use explicit comparison against 0 for non-pointer, non-booleans.
>
> > +virBufferAddLit(buf, "\n");
> > +virBufferAdjustIndent(buf, 2);
> > +for (i = 0; i < def->nrevertdisks; i++) {
> > +if (virDomainSnapshotDiskDefFormat(buf, &def->revertdisks[i],
> > xmlopt) < 0)
> > +return -1;
> > +}
> > +virBufferAdjustIndent(buf, -2);
> > +virBufferAddLit(buf, "\n");
>
> Also preferrably use virXMLFormatElement.
I've basically copy&pasted what we do for the element. Will fix
it for this patch and post a separate patch fixing the other incorrect
comparisons and building xml.
> > +}
> > +
> > if (def->parent.dom) {
> > if (virDomainDefFormatInternal(def->parent.dom, xmlopt,
> > buf, domainflags) < 0)
>
> Reviewed-by: Peter Krempa
signature.asc
Description: PGP signature
Re: [libvirt PATCH 07/20] qemu_snapshot: introduce qemuSnapshotCreateQcow2Files
On Tue, Apr 04, 2023 at 01:09:41PM +0200, Peter Krempa wrote: > On Mon, Mar 13, 2023 at 16:42:08 +0100, Pavel Hrdina wrote: > > Extract creation of qcow2 files for external snapshots to separate > > function as we will need it for external snapshot revert code. > > Hmm, I don't think I like where this is going. I presume you want to use > this code to create the new overlay images. Correct > If you want to use this also in cases where the VM was live you might > run into scenarios where the qemu-img code will not be able to handle > the overlay creation, specifically on networked storage. We always kill the qemu process when reverting so I figured that qemu-img should be good enough to create the new overlay because the qemu process is not running at that point. Did not think we allow snapshot creation for qcow2 images on networked storage as it would not work for offline VM where we also use this qemu-img approach. Looking at the code for running VM we are using QMP to create the overlay files and I did not manage to find any check to limit it only for local images. If that's the case we have IMHO really bad inconsistent behavior where we would allow creating new snapshots for non-local images if the VM is running but fail to do so if the VM is offline. > I think we'll need to create the overlay images as part of the startup > of the reverted VM via QMP exactly as we are creating overlays currently > for snapshots. > > Even when today's API will not allow reversion of non-local snapshots (I > didn't check further in this series yet) I don't think we should go the > way of using qemu-img at all. There is no check for that as I assumed (probably incorrectly) that we allow creating snapshot only for local images. If that's not the case then yes we need to use the QMP commands to create overlays. Pavel signature.asc Description: PGP signature
Re: [libvirt PATCH 09/20] qemu_snapshot: introduce external snapshot revert support
On Mon, Mar 13, 2023 at 16:42:10 +0100, Pavel Hrdina wrote:
> When reverting to external snapshot we need to create new overlay qcow2
> files from the disk files the VM had when the snapshot was taken.
>
> There are some specifics and limitations when reverting to a snapshot:
>
> 1) When reverting to last snapshot we need to first create new overlay
>files before we can safely delete the old overlay files in case the
>creation fails so we have still recovery option when we error out.
>
>These new files will not have the suffix as when the snapshot was
>created as renaming the original files in order to use the same file
>names as when the snapshot was created would add unnecessary
>complexity to the code.
>
> 2) When reverting to any snapshot we will always create overlay files
>for every disk the VM had when the snapshot was done. Otherwise we
>would have to figure out if there is any other qcow2 image already
>using any of the VM disks as backing store and that itself might be
>extremely complex and in some cases impossible.
>
> 3) When reverting from any state the current overlay files will be
>always removed as that VM state is not meant to be saved. It's the
>same as with internal snapshots. If user want's to keep the current
>state before reverting they need to create a new snapshot. For now
>this will only work if the current snapshot is the last.
>
> Signed-off-by: Pavel Hrdina
> ---
> src/qemu/qemu_snapshot.c | 143 +--
> 1 file changed, 139 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index 9e4b978b1b..af4e2ea6aa 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -18,6 +18,8 @@
>
> #include
>
> +#include
> +
> #include "qemu_snapshot.h"
>
> #include "qemu_monitor.h"
> @@ -1968,6 +1970,119 @@ qemuSnapshotRevertWriteMetadata(virDomainObj *vm,
> }
>
>
> +static int
> +qemuSnapshotRevertExternal(virDomainObj *vm,
> + virDomainMomentObj *snap,
> + virDomainDef *config,
> + virDomainDef *inactiveConfig,
> + int *memsnapFD,
> + char **memsnapPath)
> +{
> +size_t i;
> +virDomainDef *domdef = NULL;
> +virDomainSnapshotLocation location =
> VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
> +virDomainMomentObj *curSnap = virDomainSnapshotGetCurrent(vm->snapshots);
> +virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap);
> +virDomainSnapshotDef *curdef = virDomainSnapshotObjGetDef(curSnap);
> +g_autoptr(virDomainSnapshotDef) tmpsnapdef = NULL;
> +g_autoptr(virBitmap) created = NULL;
> +int ret = -1;
> +
> +if (config) {
> +domdef = config;
> +} else {
> +domdef = inactiveConfig;
> +}
> +
> +if (!(tmpsnapdef = virDomainSnapshotDefNew()))
> +return -1;
> +
> +if (virDomainMomentDefPostParse(&tmpsnapdef->parent) < 0)
Weird at first glance. If we ever add something more to postparse that
might be a wrong thing to call here. Add a comment here that it's needed
_just_ for the timestamp stuff.
> +return -1;
> +
> +if (virDomainSnapshotAlignDisks(tmpsnapdef, domdef, location, false) < 0)
> +return -1;
So in the end you do align the definition, thus the modification to the
function you did should not be needed.
You also seem to rely on the fact that this auto-selects all
non-readonly disks for snapshot, but note that in case when the
definition has VIR_DOMAIN_SNAPSHOT_LOCATION_NO for some disks they will
not be selected. Having VIR_DOMAIN_SNAPSHOT_LOCATION_NO though doesn't
mean that there isn't a snapshot of that disk as it can be overriden
when specifying disks explicitly, and thus that image does have an
overlay. Reverting in the way implemented here would thus invalidate the
overlay. This contradicts point 2 from the commit message.
Also at this point this effectively limits all of this to work on local
files only as virDomainSnapshotDefAssignExternalNames works only on
local files ...
> +
> +created = virBitmapNew(tmpsnapdef->ndisks);
> +
> +if (qemuSnapshotCreateQcow2Files(vm, domdef, tmpsnapdef, created, false)
> < 0)
> +return -1;
... thus this will for this very specific moment work. But since you'll
most likely will be adding a proper revert API with XML which should
allow reversion also for network disks this is limiting that work.
> +
> +if (memsnapFD && memsnapPath && snapdef->memorysnapshotfile) {
> +virQEMUDriver *driver = ((qemuDomainObjPrivate *)
> vm->privateData)->driver;
> +g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
> +
> +*memsnapPath = snapdef->memorysnapshotfile;
> +*memsnapFD = qemuDomainOpenFile(cfg, NULL, *memsnapPath, O_RDONLY,
> NULL);
> +}
> +
> +if (config) {
> +
Re: [libvirt PATCH 10/20] qemu_snapshot: rename qemuSnapshotDeleteExternalPrepare
On Mon, Mar 13, 2023 at 16:42:11 +0100, Pavel Hrdina wrote: > The new name reflects that we prepare data for external snapshot > deletion and the old name will be used later for different part of code. > > Signed-off-by: Pavel Hrdina > --- > src/qemu/qemu_snapshot.c | 10 +- > 1 file changed, 5 insertions(+), 5 deletions(-) Reviewed-by: Peter Krempa
Re: [libvirt PATCH 11/20] qemu_snapshot: extract external snapshot delete prepare to function
On Mon, Mar 13, 2023 at 16:42:12 +0100, Pavel Hrdina wrote: > This part of code is about to grow to make deletion work when use > reverts to non-leaf snapshot. > > Signed-off-by: Pavel Hrdina > --- > src/qemu/qemu_snapshot.c | 89 +--- > 1 file changed, 55 insertions(+), 34 deletions(-) Reviewed-by: Peter Krempa
Re: [PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo
Peter Maydell writes:
> On Tue, 4 Apr 2023 at 09:25, Markus Armbruster wrote:
>>
>> In the title: "qmp:"
>>
>> Peter Maydell writes:
>> > diff --git a/qapi/run-state.json b/qapi/run-state.json
>> > index 9d34afa39e0..1de8c5c55d0 100644
>> > --- a/qapi/run-state.json
>> > +++ b/qapi/run-state.json
>> > @@ -104,16 +104,27 @@
>> > #
>> > # @running: true if all VCPUs are runnable, false if not runnable
>> > #
>> > -# @singlestep: true if VCPUs are in single-step mode
>> > +# @one-insn-per-tb: true if using TCG with one guest instruction
>> > +# per translation block
>> > +#
>> > +# @singlestep: deprecated synonym for @one-insn-per-tb
>> > #
>> > # @status: the virtual machine @RunState
>> > #
>> > +# Features:
>> > +# @deprecated: Member 'singlestep' is deprecated. Use @one-insn-per-tb
>> > instead.
>>
>> Wrap this line, please.
>>
>> > +#
>> > # Since: 0.14
>> > #
>> > -# Notes: @singlestep is enabled through the GDB stub
>> > +# Notes: @one-insn-per-tb is enabled on the command line with
>> > +#'-accel tcg,one-insn-per-tb=on', or with the HMP
>> > +#'one-insn-per-tb' command.
>> > ##
>>
>> Hmm. We report it in query-status, which means it's relevant to QMP
>> clients. We provide the command to control it only in HMP, which means
>> it's not relevant to QMP clients.
>>
>> Why is reading it relevant to QMP clients, but not writing?
>
> I suspect that neither is very relevant to QMP clients, but I
> thought we had a requirement that HMP interfaces went
> via QMP ones ?
Kind of. Here's my current boilerplate on the subject:
HMP commands without a QMP equivalent are okay if their
functionality makes no sense in QMP, or is of use only for human
users.
Example for "makes no sense in QMP": setting the current CPU,
because a QMP monitor doesn't have a current CPU.
Examples for "is of use only for human users": HMP command "help",
the integrated pocket calculator.
Debugging commands are kind of borderline. Debugging is commonly a
human activity, where HMP is just fine. However, humans create
tools to assist with their activities, and then QMP is useful.
While I wouldn't encourage HMP-only for the debugging use case, I
wouldn't veto it.
When adding an HMP-only command, explain why it is HMP-only in the
commit message.
>If not, we could just make the HMP query
> interface directly look at the TCG property, the way the
> write interface does.
How useful is it HMP?
> I don't want to add a QMP interface for writing it unless
> there's somebody who actually has a use case for it.
>
>> Use cases for reading it via QMP query-status?
>>
>> Have you considered tacking feature 'unstable' to it?
>
> Nope, because I don't know anything about what that does :-)
docs/devel/qapi-code-gen.rst explains:
Special features
Feature "deprecated" marks a command, event, enum value, or struct
member as deprecated. It is not supported elsewhere so far.
Interfaces so marked may be withdrawn in future releases in accordance
with QEMU's deprecation policy.
Feature "unstable" marks a command, event, enum value, or struct
member as unstable. It is not supported elsewhere so far. Interfaces
so marked may be withdrawn or changed incompatibly in future releases.
We use "unstable" for debugging aids, testing aids, experiments /
unfinished work.
>> > { 'struct': 'StatusInfo',
>> > - 'data': {'running': 'bool', 'singlestep': 'bool', 'status': 'RunState'}
>> > }
>> > + 'data': {'running': 'bool',
>> > + 'singlestep': { 'type': 'bool', 'features': [ 'deprecated' ]},
>> > + 'one-insn-per-tb': 'bool',
>> > + 'status': 'RunState'} }
>> >
>> > ##
>> > # @query-status:
>>
>> I see a bunch of query-status results in
>> tests/qemu-iotests/{183,234,262,280}.out. Do they need an update?
>
> "make check" passed, so I guess not, unless those don't run
> in "make check"...
Plenty of iotests don't run in "make check". Try
$ tests/qemu-iotests/check -qcow2 183 234 262 280
> Do those .out files need exact matching output, or can they
> be written to say "we don't care about what value this field
> has or whether it exists" ?
If (hazy) memory serves, there's some normalization. I doubt it'll
affect this member, i.e. you need to put it there.
Re: [RFC PATCH] docs/about/deprecated: Deprecate 32-bit host systems
On 05/02/2023 23.12, Mark Cave-Ayland wrote: On 30/01/2023 20:45, Alex Bennée wrote: Daniel P. Berrangé writes: On Mon, Jan 30, 2023 at 11:47:02AM +, Peter Maydell wrote: On Mon, 30 Jan 2023 at 11:44, Thomas Huth wrote: Testing 32-bit host OS support takes a lot of precious time during the QEMU contiguous integration tests, and considering that many OS vendors stopped shipping 32-bit variants of their OS distributions and most hardware from the past >10 years is capable of 64-bit True for x86, not necessarily true for other architectures. Are you proposing to deprecate x86 32-bit, or all 32-bit? I'm not entirely sure about whether we're yet at a point where I'd want to deprecate-and-drop 32-bit arm host support. Do we have a feeling on which aspects of 32-bit cause us the support burden ? The boring stuff like compiler errors from mismatched integer sizes is mostly quick & easy to detect simply through a cross compile. I vaguely recall someone mentioned problems with atomic ops in the past, or was it 128-bit ints, caused implications for the codebase ? Atomic operations on > TARGET_BIT_SIZE and cputlb when TCG_OVERSIZED_GUEST is set. Also the core TCG code and a bunch of the backends have TARGET_LONG_BITS > TCG_TARGET_REG_BITS ifdefs peppered throughout. I am one of an admittedly small group of people still interested in using KVM-PR on ppc32 to boot MacOS, although there is some interest on using 64-bit KVM-PR to run super-fast MacOS on modern Talos hardware. From my perspective losing the ability to run 64-bit guests on 32-bit hardware with TCG wouldn't be an issue, as long as it were still possible to use qemu-system-ppc on 32-bit hardware using both TCG and KVM to help debug the remaining issues. Hi Mark! Just out of curiosity (since we briefly talked about 32-bit KVM on ppc in today's QEMU/KVM call - in the context of whether qemu-system-ppc64 is a proper superset of qemu-system-ppc when it comes to building a unified qemu-system binary): What host machine are you using for running KVM-PR? And which QEMU machine are you using for running macOS? The mac99 or the g3beige machine? Unrelated to KVM: Do you happen to know whether there are any problems when running 32-bit guests with TCG with the mac99 or g3beige machine while using qemu-system-ppc64 ? Thanks, Thomas
Re: [PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo
On 4/4/23 11:17, Peter Maydell wrote: I don't want to add a QMP interface for writing it unless there's somebody who actually has a use case for it. We could place the accelerator at a well-known QOM path such as /machine/accel, and then qom-set can already be used to implement such an interface. Not that you have to do it---just an argument against adding a QMP version of singlestep/one-insn-per-tb. Paolo
Re: [PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo
On Tue, 4 Apr 2023 at 14:25, Markus Armbruster wrote: > > Peter Maydell writes: > > > On Tue, 4 Apr 2023 at 09:25, Markus Armbruster wrote: > >> Hmm. We report it in query-status, which means it's relevant to QMP > >> clients. We provide the command to control it only in HMP, which means > >> it's not relevant to QMP clients. > >> > >> Why is reading it relevant to QMP clients, but not writing? > > > > I suspect that neither is very relevant to QMP clients, but I > > thought we had a requirement that HMP interfaces went > > via QMP ones ? > > Kind of. Here's my current boilerplate on the subject: > > HMP commands without a QMP equivalent are okay if their > functionality makes no sense in QMP, or is of use only for human > users. > > Example for "makes no sense in QMP": setting the current CPU, > because a QMP monitor doesn't have a current CPU. > > Examples for "is of use only for human users": HMP command "help", > the integrated pocket calculator. > > Debugging commands are kind of borderline. Debugging is commonly a > human activity, where HMP is just fine. However, humans create > tools to assist with their activities, and then QMP is useful. > While I wouldn't encourage HMP-only for the debugging use case, I > wouldn't veto it. > > When adding an HMP-only command, explain why it is HMP-only in the > commit message. > > >If not, we could just make the HMP query > > interface directly look at the TCG property, the way the > > write interface does. > > How useful is it HMP? Well, as usual, we have no idea if anybody really uses any feature. I've never used it myself but I have a vague recollection of reading list mail once from somebody who used it. You can construct theoretical scenarios where it might be nice (eg "boot guest OS quickly and then turn on the one-insn-per-tb mode once you get to the point of interest"), I guess. These theoretical scenarios are equally valid (or esoteric) whether you're trying to control QEMU via QMP or HMP. I think on balance I would go for: * remove (ie deprecate-and-drop) 'singlestep' from the QMP struct, rather than merely renaming it * if anybody comes along and says they want to do this via QMP, implement Paolo's idea of putting the accelerator object somewhere they can get at it and use qom-get/qom-set on it [My guess is this is very unlikely: nobody's complained so far that QMP doesn't permit setting 'singlestep'; and wanting read without write seems even more marginal.] * keep the HMP commands, but have both read and write directly talk to the accel object. I favour splitting the 'read' part out into its own 'info one-insn-per-tb', for consistency (then 'info status' matches the QMP query-status) In particular, the fact that messing with this obscure debug functionality requires updating the reference-output for a bunch of io tests that have no interest at all in it rather suggests that even if we did want to expose this to QMP that the query-status command is the wrong place to do it. thanks -- PMM
Re: [libvirt PATCH 12/20] qemu_snapshot: add blockCommit to external snapshot delete prepare data
On Mon, Mar 13, 2023 at 16:42:13 +0100, Pavel Hrdina wrote: > Before external snapshot revert every delete operation did block commit > in order to delete a snapshot. But now when user reverts to non-leaf > snapshot deleting leaf snapshot will not have any overlay files so we > can just simply delete the snapshot images. > > Signed-off-by: Pavel Hrdina > --- > src/qemu/qemu_snapshot.c | 102 --- > 1 file changed, 62 insertions(+), 40 deletions(-) > > diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c > index d545c984ce..6ff18d7a9e 100644 > --- a/src/qemu/qemu_snapshot.c > +++ b/src/qemu/qemu_snapshot.c > @@ -2476,6 +2476,7 @@ qemuSnapshotFindParentSnapForDisk(virDomainMomentObj > *snap, > static int > qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm, >virDomainMomentObj *snap, > + bool blockCommit, >GSList **externalData) This function is starting to get into the region of needing documentation after the flag was added. Also I'd call it a bit differently to de-emphasise that block commit is used, but rather that we are merging images. Either way: Reviewed-by: Peter Krempa
Re: [libvirt PATCH 13/20] qemu_snapshot: prepare data for non-active leaf external snapshot deletion
On Mon, Mar 13, 2023 at 16:42:14 +0100, Pavel Hrdina wrote:
> In this case there is no need to run block commit and using qemu process
> at all.
>
> Signed-off-by: Pavel Hrdina
> ---
> src/qemu/qemu_snapshot.c | 52 ++--
> 1 file changed, 29 insertions(+), 23 deletions(-)
>
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index 6ff18d7a9e..be2e5c8cc4 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -2597,34 +2597,40 @@ qemuSnapshotDeleteExternalPrepare(virDomainObj *vm,
[...]
Please explain the logic and implications in a comment above this
condition.
> +if (snap != virDomainSnapshotGetCurrent(vm->snapshots) &&
> +snap->nchildren == 0) {
> +if (qemuSnapshotDeleteExternalPrepareData(vm, snap, false,
> externalData) < 0)
> return -1;
> } else {
Reviewed-by: Peter Krempa
Re: [libvirt PATCH 14/20] qemu_snapshot: add support to delete external snapshot without block commit
On Mon, Mar 13, 2023 at 16:42:15 +0100, Pavel Hrdina wrote:
> When block commit is not needed we can just simply unlink the
> disk files.
>
> Signed-off-by: Pavel Hrdina
> ---
> src/qemu/qemu_snapshot.c | 55 +---
> 1 file changed, 35 insertions(+), 20 deletions(-)
>
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index be2e5c8cc4..dbcdf56758 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
[...]
> @@ -2949,31 +2951,41 @@ qemuSnapshotDiscardExternal(virDomainObj *vm,
> virTristateBool autofinalize = VIR_TRISTATE_BOOL_NO;
> unsigned int commitFlags = VIR_DOMAIN_BLOCK_COMMIT_DELETE;
>
> -if (data->domDisk->src == data->diskSrc) {
> -commitFlags |= VIR_DOMAIN_BLOCK_COMMIT_ACTIVE;
> -autofinalize = VIR_TRISTATE_BOOL_YES;
> +if (data->blockCommit) {
> +if (data->domDisk->src == data->diskSrc) {
> +commitFlags |= VIR_DOMAIN_BLOCK_COMMIT_ACTIVE;
> +autofinalize = VIR_TRISTATE_BOOL_YES;
> +}
> +
> +if (qemuSnapshotSetInvalid(vm, data->parentSnap, data->snapDisk,
> true) < 0)
> +goto error;
> +
> +data->job = qemuBlockCommit(vm,
> +data->domDisk,
> +data->parentDiskSrc,
> +data->diskSrc,
> +data->prevDiskSrc,
> +0,
> +VIR_ASYNC_JOB_SNAPSHOT,
> +autofinalize,
> +commitFlags);
> +
> +if (!data->job)
> +goto error;
> +} else {
> +if (unlink(data->snapDisk->src->path) < 0) {
This can be done only when the 'src' object is "local storage"
(virStorageSourceIsLocalStorage)
> +VIR_WARN("Failed to remove snapshot image '%s'",
> + data->snapDisk->name);
> +}
Reviewed-by: Peter Krempa
Re: [libvirt PATCH 15/20] qemu_snapshot: delete: properly update parent snapshot with revert data
On Mon, Mar 13, 2023 at 16:42:16 +0100, Pavel Hrdina wrote:
> When deleting external snapshot and parent snapshot is the currently
> active snapshot as user reverted to it we need to properly update the
> parent snapshot metadata.
>
> After the delete is done the new overlay files will be the currently
> used files created when snapshot revert was done, replacing the original
> overlay files.
>
> Signed-off-by: Pavel Hrdina
> ---
> src/qemu/qemu_snapshot.c | 40
> 1 file changed, 40 insertions(+)
>
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index dbcdf56758..513bcb5a86 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -3063,6 +3063,41 @@ qemuSnapshotDiscardExternal(virDomainObj *vm,
> }
>
>
> +static int
> +qemuSnapshotDeleteUpdateParent(virDomainObj *vm,
> + virDomainMomentObj *parent)
> +{
> +size_t i;
> +virQEMUDriver *driver = ((qemuDomainObjPrivate *)
> vm->privateData)->driver;
You can use QEMU_DOMAIN_PRIVATE(vm)->driver here to hide the typecast.
> +g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
> +virDomainSnapshotDef *parentDef = virDomainSnapshotObjGetDef(parent);
> +
> +if (!parentDef)
> +return 0;
> +
> +if (!parentDef->revertdisks)
> +return 0;
Reviewed-by: Peter Krempa
Re: [libvirt PATCH 16/20] qemu_snapshot: don't allow creating external snapshot after reverting
On Mon, Mar 13, 2023 at 16:42:17 +0100, Pavel Hrdina wrote: > This would allow creating new branch in the external snapshot history > which we currently don't support. > > The issue with branching the external snapshot history is that deleting > some snapshot would require calling block-stream instead of > block-commit which is currently not implemented. If the problem is with deletion of the snapshot afterwards I strongly suggest detecting and limiting the deletion itself. Creation doesn't seem to be the problem and it feels weird to restrict it.
Re: [libvirt PATCH 17/20] virdomainmomentobjlist: introduce virDomainMomentIsAncestor
On Mon, Mar 13, 2023 at 16:42:18 +0100, Pavel Hrdina wrote:
> This new helper will allow us to check if we are able to delete external
> snapshot after user did revert to non-leaf snapshot.
>
> Signed-off-by: Pavel Hrdina
> ---
> src/conf/virdomainmomentobjlist.c | 21 +
> src/conf/virdomainmomentobjlist.h | 4
> src/libvirt_private.syms | 1 +
> 3 files changed, 26 insertions(+)
>
> diff --git a/src/conf/virdomainmomentobjlist.c
> b/src/conf/virdomainmomentobjlist.c
> index f19ec3319a..0c520fb173 100644
> --- a/src/conf/virdomainmomentobjlist.c
> +++ b/src/conf/virdomainmomentobjlist.c
> @@ -582,3 +582,24 @@ virDomainMomentFindLeaf(virDomainMomentObjList *list)
> return moment;
> return NULL;
> }
> +
> +
> +bool
> +virDomainMomentIsAncestor(virDomainMomentObj *moment,
> + virDomainMomentObj *ancestor)
> +{
> +if (!moment)
> +return false;
for (moment = moment->parent; moment; moment = moment->parent) {
if (moment == ancestor)
return true;
}
return false;
This should be sufficient IIUC to replace the condition and the loop,
right?
Either way:
Reviewed-by: Peter Krempa
Consider documenting expectations though.
Re: [libvirt PATCH 18/20] qemu_snapshot: check only once if snapshot is external
On Mon, Mar 13, 2023 at 16:42:19 +0100, Pavel Hrdina wrote: > There will be more external snapshot checks introduced by following > patch so group them together. > > Signed-off-by: Pavel Hrdina > --- > src/qemu/qemu_snapshot.c | 22 +++--- > 1 file changed, 11 insertions(+), 11 deletions(-) Reviewed-by: Peter Krempa
Re: [PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo
On 4/4/23 16:24, Peter Maydell wrote: I think on balance I would go for: * remove (ie deprecate-and-drop) 'singlestep' from the QMP struct, rather than merely renaming it * if anybody comes along and says they want to do this via QMP, implement Paolo's idea of putting the accelerator object somewhere they can get at it and use qom-get/qom-set on it [My guess is this is very unlikely: nobody's complained so far that QMP doesn't permit setting 'singlestep'; and wanting read without write seems even more marginal.] * keep the HMP commands, but have both read and write directly talk to the accel object. I favour splitting the 'read' part out into its own 'info one-insn-per-tb', for consistency (then 'info status' matches the QMP query-status) I think the read part could be added to 'info jit'. Paolo
Re: [RFC PATCH] docs/about/deprecated: Deprecate 32-bit host systems
On Tue, 4 Apr 2023, Thomas Huth wrote: On 05/02/2023 23.12, Mark Cave-Ayland wrote: On 30/01/2023 20:45, Alex Bennée wrote: Daniel P. Berrangé writes: On Mon, Jan 30, 2023 at 11:47:02AM +, Peter Maydell wrote: On Mon, 30 Jan 2023 at 11:44, Thomas Huth wrote: Testing 32-bit host OS support takes a lot of precious time during the QEMU contiguous integration tests, and considering that many OS vendors stopped shipping 32-bit variants of their OS distributions and most hardware from the past >10 years is capable of 64-bit True for x86, not necessarily true for other architectures. Are you proposing to deprecate x86 32-bit, or all 32-bit? I'm not entirely sure about whether we're yet at a point where I'd want to deprecate-and-drop 32-bit arm host support. Do we have a feeling on which aspects of 32-bit cause us the support burden ? The boring stuff like compiler errors from mismatched integer sizes is mostly quick & easy to detect simply through a cross compile. I vaguely recall someone mentioned problems with atomic ops in the past, or was it 128-bit ints, caused implications for the codebase ? Atomic operations on > TARGET_BIT_SIZE and cputlb when TCG_OVERSIZED_GUEST is set. Also the core TCG code and a bunch of the backends have TARGET_LONG_BITS > TCG_TARGET_REG_BITS ifdefs peppered throughout. I am one of an admittedly small group of people still interested in using KVM-PR on ppc32 to boot MacOS, although there is some interest on using 64-bit KVM-PR to run super-fast MacOS on modern Talos hardware. From my perspective losing the ability to run 64-bit guests on 32-bit hardware with TCG wouldn't be an issue, as long as it were still possible to use qemu-system-ppc on 32-bit hardware using both TCG and KVM to help debug the remaining issues. Hi Mark! Just out of curiosity (since we briefly talked about 32-bit KVM on ppc in today's QEMU/KVM call - in the context of whether qemu-system-ppc64 is a proper superset of qemu-system-ppc when it comes to building a unified qemu-system binary): What host machine are you using for running KVM-PR? And Another issue I know is that mac99 behaves differently in qemu-system-ppc and qemu-system-ppc64. This is not only confusing users but makes it more difficult to get rid of qemu-system-ppc. I've tried to solve that and sumbitted a patch to start deprecating these but I could not get that merged. That thread ended here: https://lists.nongnu.org/archive/html/qemu-ppc/2023-01/msg00406.html Regards, BALATON Zoltan
Re: [RFC PATCH] docs/about/deprecated: Deprecate 32-bit host systems
On Tue, 4 Apr 2023, Cédric Le Goater wrote: [ adding Zoltan ] On 4/4/23 16:00, Thomas Huth wrote: On 05/02/2023 23.12, Mark Cave-Ayland wrote: On 30/01/2023 20:45, Alex Bennée wrote: Daniel P. Berrangé writes: On Mon, Jan 30, 2023 at 11:47:02AM +, Peter Maydell wrote: On Mon, 30 Jan 2023 at 11:44, Thomas Huth wrote: Testing 32-bit host OS support takes a lot of precious time during the QEMU contiguous integration tests, and considering that many OS vendors stopped shipping 32-bit variants of their OS distributions and most hardware from the past >10 years is capable of 64-bit True for x86, not necessarily true for other architectures. Are you proposing to deprecate x86 32-bit, or all 32-bit? I'm not entirely sure about whether we're yet at a point where I'd want to deprecate-and-drop 32-bit arm host support. Do we have a feeling on which aspects of 32-bit cause us the support burden ? The boring stuff like compiler errors from mismatched integer sizes is mostly quick & easy to detect simply through a cross compile. I vaguely recall someone mentioned problems with atomic ops in the past, or was it 128-bit ints, caused implications for the codebase ? Atomic operations on > TARGET_BIT_SIZE and cputlb when TCG_OVERSIZED_GUEST is set. Also the core TCG code and a bunch of the backends have TARGET_LONG_BITS > TCG_TARGET_REG_BITS ifdefs peppered throughout. I am one of an admittedly small group of people still interested in using KVM-PR on ppc32 to boot MacOS, although there is some interest on using 64-bit KVM-PR to run super-fast MacOS on modern Talos hardware. From my perspective losing the ability to run 64-bit guests on 32-bit hardware with TCG wouldn't be an issue, as long as it were still possible to use qemu-system-ppc on 32-bit hardware using both TCG and KVM to help debug the remaining issues. Hi Mark! Just out of curiosity (since we briefly talked about 32-bit KVM on ppc in today's QEMU/KVM call - in the context of whether qemu-system-ppc64 is a proper superset of qemu-system-ppc when it comes to building a unified qemu-system binary): What host machine are you using for running KVM-PR? And which QEMU machine are you using for running macOS? The mac99 or the g3beige machine? Zoltan, what about the pegasos2 and sam460ex machines ? can they be run under KVM ? I don't know as I don't have PPC hardware to test on but theoretically they should work. Although BookE KVM was dropped from Linux I think so sam460ex could only work with an old kernel on a BookE host which is now rare but pegasos2 uses G4 CPU which is more likely to work on a host with the same CPU but I don't know anybody tested it yet. I know some people are interested to use it especially on G4 and G5 and some tested the latter but it does not work due to some differences that should be handled by KVM-PR but apparently they aren't (e.g. dcbz cache line size which I've seen debugged and identified by at least two sources that I sent references to this list before but to my knowledge no fix got upstream in Linux for this). AFAIK Mark has a G4 Mac mini on which KVM-PR works and it may also work on G5 when using 32 bit Kernel but running G4 guest on G5 with 64 bit kernel does not work. G5 host with 64 bit kernel may work with 64 bit guests but all Amiga like OSes I'm interested in are 32 bit so they either need 32 bit host kernel, preferably with same CPU as the guest (so G4 for pegasos2 or PPC440 for sam460ex) or fixing the patching and emulation of instructions in KVM-PR that behave differently between G4 and G5 which should be possible but nobody seems to have done it or bothered to upstream it and what's there may be incomplete or buggy. The only reports I've seen that said it works were either running 32bit guest on 32bit BookS host or 64bit guest on 64bit BookS host. (A special case is running 64bit aware 32bit guest such as newer MacOS or MorphOS versions on 64bit hosts which can run on real G5 so these may run as long as they correctly identify the CPU they are running on but the G5 Mac emulation in QEMU currently can't boot these because the machine is not emulated precisely enough for them.) Regards, BALATON Zoltan
Re: [RFC PATCH] docs/about/deprecated: Deprecate 32-bit host systems
On Tue, 4 Apr 2023, Cédric Le Goater wrote:
Unrelated to KVM: Do you happen to know whether there are any problems when
running 32-bit guests with TCG with the mac99 or g3beige machine while
using qemu-system-ppc64 ?
We removed rfi support for 64bit machine in QEMU 2.7. Commit a2e71b28e8
("ppc: Fix rfi/rfid/hrfi/... emulation"). So a mac99 machine using a 970
CPU needs a 64 bit kernel. The rfi insn was deleted from the ISA ...
I don't know about that issue but maybe this does not really answer the
question. AFAIK you could still run 32 bit guests with qemu-system-ppc64
and they seem to work. One could still get the qemu-system-ppc -M mac99
machine as qemu-system-ppc64 -M mac99 -cpu G4 but this is very confusing
and nobody could guess it. Other machines should work the same in
qemu-system-ppc64, the only concern is that some defines such as
TARGET_ULONG are different between qemu-system-ppc and qemu-system-ppc64
and it's not known if this could cause any problems. It's more frequently
happens that people try qemu-system-ppc64 -M mac99 when they want a G4 Mac
and get confused that it emulates a G5 Mac instead which does not even
work for anything than a Linux guest.
Regards,
BALATON Zoltan
Re: [libvirt PATCH 09/20] qemu_snapshot: introduce external snapshot revert support
On Tue, Apr 04, 2023 at 03:03:56PM +0200, Peter Krempa wrote:
> On Mon, Mar 13, 2023 at 16:42:10 +0100, Pavel Hrdina wrote:
> > When reverting to external snapshot we need to create new overlay qcow2
> > files from the disk files the VM had when the snapshot was taken.
> >
> > There are some specifics and limitations when reverting to a snapshot:
> >
> > 1) When reverting to last snapshot we need to first create new overlay
> >files before we can safely delete the old overlay files in case the
> >creation fails so we have still recovery option when we error out.
> >
> >These new files will not have the suffix as when the snapshot was
> >created as renaming the original files in order to use the same file
> >names as when the snapshot was created would add unnecessary
> >complexity to the code.
> >
> > 2) When reverting to any snapshot we will always create overlay files
> >for every disk the VM had when the snapshot was done. Otherwise we
> >would have to figure out if there is any other qcow2 image already
> >using any of the VM disks as backing store and that itself might be
> >extremely complex and in some cases impossible.
> >
> > 3) When reverting from any state the current overlay files will be
> >always removed as that VM state is not meant to be saved. It's the
> >same as with internal snapshots. If user want's to keep the current
> >state before reverting they need to create a new snapshot. For now
> >this will only work if the current snapshot is the last.
> >
> > Signed-off-by: Pavel Hrdina
> > ---
> > src/qemu/qemu_snapshot.c | 143 +--
> > 1 file changed, 139 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> > index 9e4b978b1b..af4e2ea6aa 100644
> > --- a/src/qemu/qemu_snapshot.c
> > +++ b/src/qemu/qemu_snapshot.c
> > @@ -18,6 +18,8 @@
> >
> > #include
> >
> > +#include
> > +
> > #include "qemu_snapshot.h"
> >
> > #include "qemu_monitor.h"
> > @@ -1968,6 +1970,119 @@ qemuSnapshotRevertWriteMetadata(virDomainObj *vm,
> > }
> >
> >
> > +static int
> > +qemuSnapshotRevertExternal(virDomainObj *vm,
> > + virDomainMomentObj *snap,
> > + virDomainDef *config,
> > + virDomainDef *inactiveConfig,
> > + int *memsnapFD,
> > + char **memsnapPath)
> > +{
> > +size_t i;
> > +virDomainDef *domdef = NULL;
> > +virDomainSnapshotLocation location =
> > VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
> > +virDomainMomentObj *curSnap =
> > virDomainSnapshotGetCurrent(vm->snapshots);
> > +virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap);
> > +virDomainSnapshotDef *curdef = virDomainSnapshotObjGetDef(curSnap);
> > +g_autoptr(virDomainSnapshotDef) tmpsnapdef = NULL;
> > +g_autoptr(virBitmap) created = NULL;
> > +int ret = -1;
> > +
> > +if (config) {
> > +domdef = config;
> > +} else {
> > +domdef = inactiveConfig;
> > +}
> > +
> > +if (!(tmpsnapdef = virDomainSnapshotDefNew()))
> > +return -1;
> > +
> > +if (virDomainMomentDefPostParse(&tmpsnapdef->parent) < 0)
>
> Weird at first glance. If we ever add something more to postparse that
> might be a wrong thing to call here. Add a comment here that it's needed
> _just_ for the timestamp stuff.
I can probably just rename the function to reflect that it just
generates creation time and uses it as default name so in the future if
we need to add more post parse functionality we would have to create
true post parse function calling this renamed helper.
> > +return -1;
> > +
> > +if (virDomainSnapshotAlignDisks(tmpsnapdef, domdef, location, false) <
> > 0)
> > +return -1;
>
> So in the end you do align the definition, thus the modification to the
> function you did should not be needed.
>
> You also seem to rely on the fact that this auto-selects all
> non-readonly disks for snapshot, but note that in case when the
> definition has VIR_DOMAIN_SNAPSHOT_LOCATION_NO for some disks they will
> not be selected. Having VIR_DOMAIN_SNAPSHOT_LOCATION_NO though doesn't
> mean that there isn't a snapshot of that disk as it can be overriden
> when specifying disks explicitly, and thus that image does have an
> overlay. Reverting in the way implemented here would thus invalidate the
> overlay. This contradicts point 2 from the commit message.
This should not be an issue as the tmpsnapdef is a new empty snapshot
definition so no disk should have VIR_DOMAIN_SNAPSHOT_LOCATION_NO.
This code should just take existing domain definition and fill in the
new empty snapshot definition with all disks the domain is currently
using and the domain definition is from the snapshot metadata that we
are reverting to. So this part should work correctly.
> Also at this point this effectively l
Re: [PATCH] Conf: Move validation of Graphics Transport and StorageNetwork Socket out of parser
Thank you for the corrections. I understand it now.
[1] I have made the Graphics Transport validation function static and moved its
call out of the parser, to virDomainGraphicsDefListensValidate() in
domain_validate.h. Is this a correct implementation?, it does pass the tests.
I have removed the *graphics def parameter from ParseXML function as it became
unused after moving the validation out of the Parser.
[2] I can't find a way to implement virDomainStorageNetHostSocketValidate() as
static. Could you please point as to which function within domain_validate.h
could be used to call this validation function? similar to
virDomainGraphicsDefListensValidate() for [1].
[3] How do I obtain char *transport within the validation function without
parameter passing?
It's used only to report this error in the below snippet
virReportError(VIR_ERR_XML_ERROR,
_("transport '%1$s' does not support socket attribute"),
transport);
Do I use virXMLPropString(hostnode, "transport") after passing xmlNodePtr
hostnode as a parameter? How do I obtain these in domain_validate.h.
Thank you for your help!
Signed-off-by: K Shiva
---
src/conf/domain_conf.c | 9 ++---
src/conf/domain_validate.c | 68 --
src/conf/domain_validate.h | 4 ---
3 files changed, 37 insertions(+), 44 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1e0ac737bb..746bb4efdf 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5836,7 +5836,7 @@ virDomainStorageNetworkParseHost(xmlNodePtr hostnode,
host->socket = virXMLPropString(hostnode, "socket");
-// Socket Validation
+/* Socket Validation */
if (virDomainStorageNetHostSocketValidate(host, transport) < 0)
goto cleanup;
@@ -10975,7 +10975,6 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr node,
/**
* virDomainGraphicsListenDefParseXML:
* @def: listen def pointer to be filled
- * @graphics: graphics def pointer
* @node: xml node of element
* @parent: xml node of element
* @flags: bit-wise or of VIR_DOMAIN_DEF_PARSE_*
@@ -10987,7 +10986,6 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr node,
*/
static int
virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDef *def,
- virDomainGraphicsDef *graphics,
xmlNodePtr node,
xmlNodePtr parent,
unsigned int flags)
@@ -11009,9 +11007,6 @@
virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDef *def,
VIR_XML_PROP_REQUIRED, &def->type) < 0)
goto error;
-if (virDomainGraphicsListenDefValidate(def, graphics) == -1)
-goto error;
-
if (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS) {
if (address && addressCompat && STRNEQ(address, addressCompat)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -4,7 +11109,7 @@ virDomainGraphicsListensParseXML(virDomainGraphicsDef
*def,
def->listens = g_new0(virDomainGraphicsListenDef, nListens);
for (i = 0; i < nListens; i++) {
-if (virDomainGraphicsListenDefParseXML(&def->listens[i], def,
+if (virDomainGraphicsListenDefParseXML(&def->listens[i],
listenNodes[i],
i == 0 ? node : NULL,
flags) < 0)
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 10f8242c84..a84dbe8bc9 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -2627,6 +2627,39 @@ virDomainAudioDefValidate(const virDomainDef *def,
return 0;
}
+static int
+virDomainGraphicsListenDefValidate(const virDomainGraphicsListenDef *def,
+ const virDomainGraphicsDef *graphics)
+{
+const char *graphicsType = virDomainGraphicsTypeToString(graphics->type);
+
+switch (def->type) {
+case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET:
+if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
+graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("listen type 'socket' is not available for
graphics type '%1$s'"),
+ graphicsType);
+return -1;
+}
+break;
+case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE:
+if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
+graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("listen type 'none' is not available for graphics
type '%1$s'"),
+ graphicsType);
+return -1;
+}
+break;
+case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS:
+case VIR_DOMA
[PATCH] Conf: Move validation of Graphics Transport and StorageNetwork Socket out of parser
Thank you for the corrections. I understand it now.
[1] I have made the Graphics Transport validation function static and moved its
call out of the parser, to virDomainGraphicsDefListensValidate() in
domain_validate.h. Is this a correct implementation?, it does pass the tests.
I have removed the *graphics def parameter from ParseXML function as it became
unused after moving the validation out of the Parser.
[2] I can't find a way to implement virDomainStorageNetHostSocketValidate() as
static. Could you please point as to which function within domain_validate.h
could be used to call this validation function? similar to
virDomainGraphicsDefListensValidate() for [1].
[3] How do I obtain char *transport within the validation function without
parameter passing?
It's used only to report this error in the below snippet
virReportError(VIR_ERR_XML_ERROR,
_("transport '%1$s' does not support socket attribute"),
transport);
Do I use virXMLPropString(hostnode, "transport") after passing xmlNodePtr
hostnode as a parameter? How do I obtain these in domain_validate.h.
Thank you for your help!
Signed-off-by: K Shiva
---
src/conf/domain_conf.c | 9 ++---
src/conf/domain_validate.c | 68 --
src/conf/domain_validate.h | 4 ---
3 files changed, 37 insertions(+), 44 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1e0ac737bb..746bb4efdf 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5836,7 +5836,7 @@ virDomainStorageNetworkParseHost(xmlNodePtr hostnode,
host->socket = virXMLPropString(hostnode, "socket");
-// Socket Validation
+/* Socket Validation */
if (virDomainStorageNetHostSocketValidate(host, transport) < 0)
goto cleanup;
@@ -10975,7 +10975,6 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr node,
/**
* virDomainGraphicsListenDefParseXML:
* @def: listen def pointer to be filled
- * @graphics: graphics def pointer
* @node: xml node of element
* @parent: xml node of element
* @flags: bit-wise or of VIR_DOMAIN_DEF_PARSE_*
@@ -10987,7 +10986,6 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr node,
*/
static int
virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDef *def,
- virDomainGraphicsDef *graphics,
xmlNodePtr node,
xmlNodePtr parent,
unsigned int flags)
@@ -11009,9 +11007,6 @@
virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDef *def,
VIR_XML_PROP_REQUIRED, &def->type) < 0)
goto error;
-if (virDomainGraphicsListenDefValidate(def, graphics) == -1)
-goto error;
-
if (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS) {
if (address && addressCompat && STRNEQ(address, addressCompat)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -4,7 +11109,7 @@ virDomainGraphicsListensParseXML(virDomainGraphicsDef
*def,
def->listens = g_new0(virDomainGraphicsListenDef, nListens);
for (i = 0; i < nListens; i++) {
-if (virDomainGraphicsListenDefParseXML(&def->listens[i], def,
+if (virDomainGraphicsListenDefParseXML(&def->listens[i],
listenNodes[i],
i == 0 ? node : NULL,
flags) < 0)
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 10f8242c84..a84dbe8bc9 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -2627,6 +2627,39 @@ virDomainAudioDefValidate(const virDomainDef *def,
return 0;
}
+static int
+virDomainGraphicsListenDefValidate(const virDomainGraphicsListenDef *def,
+ const virDomainGraphicsDef *graphics)
+{
+const char *graphicsType = virDomainGraphicsTypeToString(graphics->type);
+
+switch (def->type) {
+case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET:
+if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
+graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("listen type 'socket' is not available for
graphics type '%1$s'"),
+ graphicsType);
+return -1;
+}
+break;
+case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE:
+if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
+graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("listen type 'none' is not available for graphics
type '%1$s'"),
+ graphicsType);
+return -1;
+}
+break;
+case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS:
+case VIR_DOMA
