Make it possible to grab all snapshot XMLs via a single API
call, by adding a new internal flag. All callers are adjusted,
for now passing NULL and not using the new flag. A new wrapper
virDomainDefFormatFull() is added to make it easier for the
next patch to add a few callers without having to revisit all
existing clients of virDomainDefFormat().
Signed-off-by: Eric Blake <eblake@redhat.com>
---
src/conf/domain_conf.h | 8 +++++
src/conf/domain_conf.c | 58 +++++++++++++++++++++++++++++++------
src/conf/snapshot_conf.c | 4 +--
src/network/bridge_driver.c | 3 +-
src/qemu/qemu_domain.c | 2 +-
5 files changed, 62 insertions(+), 13 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 9bccd8bcd1..9b13c147da 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3047,6 +3047,7 @@ typedef enum {
VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM = 1 << 6,
VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT = 1 << 7,
VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST = 1 << 8,
+ VIR_DOMAIN_DEF_FORMAT_SNAPSHOTS = 1 << 9,
} virDomainDefFormatFlags;
/* Use these flags to skip specific domain ABI consistency checks done
@@ -3124,12 +3125,19 @@ unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags);
char *virDomainDefFormat(virDomainDefPtr def,
virCapsPtr caps,
unsigned int flags);
+char *virDomainDefFormatFull(virDomainDefPtr def,
+ virCapsPtr caps,
+ virDomainSnapshotObjListPtr snapshots,
+ virDomainSnapshotObjPtr current_snapshot,
+ unsigned int flags);
char *virDomainObjFormat(virDomainXMLOptionPtr xmlopt,
virDomainObjPtr obj,
virCapsPtr caps,
unsigned int flags);
int virDomainDefFormatInternal(virDomainDefPtr def,
virCapsPtr caps,
+ virDomainSnapshotObjListPtr snapshots,
+ virDomainSnapshotObjPtr current_snapshot,
unsigned int flags,
virBufferPtr buf,
virDomainXMLOptionPtr xmlopt);
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index ceeb247ef4..46ae79e738 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1,7 +1,7 @@
/*
* domain_conf.c: domain XML processing
*
- * Copyright (C) 2006-2016 Red Hat, Inc.
+ * Copyright (C) 2006-2019 Red Hat, Inc.
* Copyright (C) 2006-2008 Daniel P. Berrange
* Copyright (c) 2015 SUSE LINUX Products GmbH, Nuernberg, Germany.
*
@@ -28212,6 +28212,8 @@ virDomainVsockDefFormat(virBufferPtr buf,
int
virDomainDefFormatInternal(virDomainDefPtr def,
virCapsPtr caps,
+ virDomainSnapshotObjListPtr snapshots,
+ virDomainSnapshotObjPtr current_snapshot,
unsigned int flags,
virBufferPtr buf,
virDomainXMLOptionPtr xmlopt)
@@ -28229,9 +28231,16 @@ virDomainDefFormatInternal(virDomainDefPtr def,
VIR_DOMAIN_DEF_FORMAT_STATUS |
VIR_DOMAIN_DEF_FORMAT_ACTUAL_NET |
VIR_DOMAIN_DEF_FORMAT_PCI_ORIG_STATES |
- VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST,
+ VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST |
+ VIR_DOMAIN_DEF_FORMAT_SNAPSHOTS,
-1);
+ if ((flags & VIR_DOMAIN_DEF_FORMAT_SNAPSHOTS) && !snapshots) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("snapshots requested but not provided"));
+ goto error;
+ }
+
if (!(type = virDomainVirtTypeToString(def->virtType))) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected domain type %d"), def->virtType);
@@ -29016,6 +29025,23 @@ virDomainDefFormatInternal(virDomainDefPtr def,
virDomainSEVDefFormat(buf, def->sev);
+ if (flags & VIR_DOMAIN_DEF_FORMAT_SNAPSHOTS) {
+ unsigned int snapflags = flags & VIR_DOMAIN_DEF_FORMAT_SECURE ?
+ VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE : 0;
+
+ virBufferAddLit(buf, "<snapshots");
+ if (current_snapshot)
+ virBufferEscapeString(buf, " current='%s'",
+ current_snapshot->def->name);
+ virBufferAddLit(buf, ">\n");
+ virBufferAdjustIndent(buf, 2);
+ if (virDomainSnapshotObjListFormat(buf, uuidstr, snapshots, caps,
+ xmlopt, snapflags))
+ goto error;
+ virBufferAdjustIndent(buf, -2);
+ virBufferAddLit(buf, "</snapshots>\n");
+ }
+
virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "</domain>\n");
@@ -29051,16 +29077,29 @@ unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags)
}
+char *
+virDomainDefFormatFull(virDomainDefPtr def, virCapsPtr caps,
+ virDomainSnapshotObjListPtr snapshots,
+ virDomainSnapshotObjPtr current_snapshot,
+ unsigned int flags)
+{
+ virBuffer buf = VIR_BUFFER_INITIALIZER;
+
+ virCheckFlags(VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS |
+ VIR_DOMAIN_DEF_FORMAT_SNAPSHOTS, NULL);
+ if (virDomainDefFormatInternal(def, caps, snapshots, current_snapshot,
+ flags, &buf, NULL) < 0)
+ return NULL;
+
+ return virBufferContentAndReset(&buf);
+}
+
+
char *
virDomainDefFormat(virDomainDefPtr def, virCapsPtr caps, unsigned int flags)
{
- virBuffer buf = VIR_BUFFER_INITIALIZER;
-
virCheckFlags(VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS, NULL);
- if (virDomainDefFormatInternal(def, caps, flags, &buf, NULL) < 0)
- return NULL;
-
- return virBufferContentAndReset(&buf);
+ return virDomainDefFormatFull(def, caps, NULL, NULL, flags);
}
@@ -29092,7 +29131,8 @@ virDomainObjFormat(virDomainXMLOptionPtr xmlopt,
xmlopt->privateData.format(&buf, obj) < 0)
goto error;
- if (virDomainDefFormatInternal(obj->def, caps, flags, &buf, xmlopt) < 0)
+ if (virDomainDefFormatInternal(obj->def, caps, NULL, NULL, flags, &buf,
+ xmlopt) < 0)
goto error;
virBufferAdjustIndent(&buf, -2);
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index a572da5b58..963dc10247 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -754,8 +754,8 @@ virDomainSnapshotDefFormatInternal(virBufferPtr buf,
}
if (def->dom) {
- if (virDomainDefFormatInternal(def->dom, caps, domainflags, buf,
- xmlopt) < 0)
+ if (virDomainDefFormatInternal(def->dom, caps, NULL, NULL, domainflags,
+ buf, xmlopt) < 0)
goto error;
} else if (uuidstr) {
virBufferAddLit(buf, "<domain>\n");
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index b3ca5b8a15..bf2045a753 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -226,7 +226,8 @@ networkRunHook(virNetworkObjPtr obj,
goto cleanup;
if (virNetworkDefFormatBuf(&buf, def, 0) < 0)
goto cleanup;
- if (dom && virDomainDefFormatInternal(dom, NULL, 0, &buf, NULL) < 0)
+ if (dom && virDomainDefFormatInternal(dom, NULL, NULL, NULL, 0, &buf,
+ NULL) < 0)
goto cleanup;
virBufferAdjustIndent(&buf, -2);
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 84b923fbbb..cb1665c8f9 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7881,7 +7881,7 @@ qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver,
}
format:
- ret = virDomainDefFormatInternal(def, caps,
+ ret = virDomainDefFormatInternal(def, caps, NULL, NULL,
virDomainDefFormatConvertXMLFlags(flags),
buf, driver->xmlopt);
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 2/23/19 4:24 PM, Eric Blake wrote:
> Make it possible to grab all snapshot XMLs via a single API
> call, by adding a new internal flag. All callers are adjusted,
> for now passing NULL and not using the new flag. A new wrapper
> virDomainDefFormatFull() is added to make it easier for the
Not really the next any more it seems - definitely in future ones though.
> next patch to add a few callers without having to revisit all
> existing clients of virDomainDefFormat().
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> src/conf/domain_conf.h | 8 +++++
> src/conf/domain_conf.c | 58 +++++++++++++++++++++++++++++++------
> src/conf/snapshot_conf.c | 4 +--
> src/network/bridge_driver.c | 3 +-
> src/qemu/qemu_domain.c | 2 +-
> 5 files changed, 62 insertions(+), 13 deletions(-)
>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 9bccd8bcd1..9b13c147da 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -3047,6 +3047,7 @@ typedef enum {
> VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM = 1 << 6,
> VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT = 1 << 7,
> VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST = 1 << 8,
> + VIR_DOMAIN_DEF_FORMAT_SNAPSHOTS = 1 << 9,
> } virDomainDefFormatFlags;
>
> /* Use these flags to skip specific domain ABI consistency checks done
> @@ -3124,12 +3125,19 @@ unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags);
> char *virDomainDefFormat(virDomainDefPtr def,
> virCapsPtr caps,
> unsigned int flags);
> +char *virDomainDefFormatFull(virDomainDefPtr def,
> + virCapsPtr caps,
> + virDomainSnapshotObjListPtr snapshots,
> + virDomainSnapshotObjPtr current_snapshot,
> + unsigned int flags);
> char *virDomainObjFormat(virDomainXMLOptionPtr xmlopt,
> virDomainObjPtr obj,
> virCapsPtr caps,
> unsigned int flags);
> int virDomainDefFormatInternal(virDomainDefPtr def,
> virCapsPtr caps,
> + virDomainSnapshotObjListPtr snapshots,
> + virDomainSnapshotObjPtr current_snapshot,
> unsigned int flags,
> virBufferPtr buf,
> virDomainXMLOptionPtr xmlopt);
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index ceeb247ef4..46ae79e738 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1,7 +1,7 @@
> /*
> * domain_conf.c: domain XML processing
> *
> - * Copyright (C) 2006-2016 Red Hat, Inc.
> + * Copyright (C) 2006-2019 Red Hat, Inc.
> * Copyright (C) 2006-2008 Daniel P. Berrange
> * Copyright (c) 2015 SUSE LINUX Products GmbH, Nuernberg, Germany.
> *
> @@ -28212,6 +28212,8 @@ virDomainVsockDefFormat(virBufferPtr buf,
> int
> virDomainDefFormatInternal(virDomainDefPtr def,
> virCapsPtr caps,
> + virDomainSnapshotObjListPtr snapshots,
> + virDomainSnapshotObjPtr current_snapshot,
> unsigned int flags,
> virBufferPtr buf,
> virDomainXMLOptionPtr xmlopt)
Rather than possibly continually adding parameters to
virDomainDefFormatInternal, maybe we should take the plunge now to
create a local struct that would be passed along that would fill in
fields based on what "extra" format flags beyond the common set are
being used.
struct virDomainDefFormatInternalFlagsData {
virDomainSnapshotObjListPtr snapshots;
virDomainSnapshotObjPtr current_snapshot;
};
I am of course assuming checkpoints in the future will do something similar.
> @@ -28229,9 +28231,16 @@ virDomainDefFormatInternal(virDomainDefPtr def,
> VIR_DOMAIN_DEF_FORMAT_STATUS |
> VIR_DOMAIN_DEF_FORMAT_ACTUAL_NET |
> VIR_DOMAIN_DEF_FORMAT_PCI_ORIG_STATES |
> - VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST,
> + VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST |
> + VIR_DOMAIN_DEF_FORMAT_SNAPSHOTS,
> -1);
>
> + if ((flags & VIR_DOMAIN_DEF_FORMAT_SNAPSHOTS) && !snapshots) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("snapshots requested but not provided"));
> + goto error;
> + }
> +
> if (!(type = virDomainVirtTypeToString(def->virtType))) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("unexpected domain type %d"), def->virtType);
> @@ -29016,6 +29025,23 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>
> virDomainSEVDefFormat(buf, def->sev);
>
I think all of the next hunk should be in it's own helper method
> + if (flags & VIR_DOMAIN_DEF_FORMAT_SNAPSHOTS) {
> + unsigned int snapflags = flags & VIR_DOMAIN_DEF_FORMAT_SECURE ?
> + VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE : 0;
> +
> + virBufferAddLit(buf, "<snapshots");
> + if (current_snapshot)
> + virBufferEscapeString(buf, " current='%s'",
> + current_snapshot->def->name);
> + virBufferAddLit(buf, ">\n");
> + virBufferAdjustIndent(buf, 2);
> + if (virDomainSnapshotObjListFormat(buf, uuidstr, snapshots, caps,
> + xmlopt, snapflags))
So this answers my question from patch4...
Anyway, the return checking should be < 0 and I believe the @childrenBuf
is how other similar constructs attempted for format the child XML
output - I believe it could be used in this case as well or at least a
similarly named/used childrenBuf in a method/helper.
> + goto error;
> + virBufferAdjustIndent(buf, -2);
> + virBufferAddLit(buf, "</snapshots>\n");
> + }
> +
> virBufferAdjustIndent(buf, -2);
> virBufferAddLit(buf, "</domain>\n");
>
> @@ -29051,16 +29077,29 @@ unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags)
> }
>
>
> +char *
> +virDomainDefFormatFull(virDomainDefPtr def, virCapsPtr caps,
> + virDomainSnapshotObjListPtr snapshots,
> + virDomainSnapshotObjPtr current_snapshot,
> + unsigned int flags)
Naming is hard, but is "Full" just another way of saying "all flags" not
just the common ones?
Not that we necessarily want to see N different implementations of
virDomainDefFormatXXXX; however, if at some point in the future someone
wanted to list all Checkpoints (;-)), but not all Snapshots - then would
*Full still fit the bill or would we need a new method.
Just want to be sure we rightly name it considering your knowledge of
intended future changes.
It almost seems DefFormat is "format only common flags" while
DefFormatFull would be format all flags that
virDomainDefFormatConvertXMLFlags can handle beyond the original set.
If you feel *Full is fine - I'm not against it - just throwing out a
something to consider...
> +{
> + virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> + virCheckFlags(VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS |
> + VIR_DOMAIN_DEF_FORMAT_SNAPSHOTS, NULL);
> + if (virDomainDefFormatInternal(def, caps, snapshots, current_snapshot,
> + flags, &buf, NULL) < 0)
> + return NULL;
> +
> + return virBufferContentAndReset(&buf);
> +}
> +
> +
> char *
> virDomainDefFormat(virDomainDefPtr def, virCapsPtr caps, unsigned int flags)
> {
> - virBuffer buf = VIR_BUFFER_INITIALIZER;
> -
> virCheckFlags(VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS, NULL);
Considering virDomainDefFormatFull does check this - do we need to also
check it?
John
> - if (virDomainDefFormatInternal(def, caps, flags, &buf, NULL) < 0)
> - return NULL;
> -
> - return virBufferContentAndReset(&buf);
> + return virDomainDefFormatFull(def, caps, NULL, NULL, flags);
> }
>
>
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 2/27/19 10:27 AM, John Ferlan wrote:
>
>
> On 2/23/19 4:24 PM, Eric Blake wrote:
>> Make it possible to grab all snapshot XMLs via a single API
>> call, by adding a new internal flag. All callers are adjusted,
>> for now passing NULL and not using the new flag. A new wrapper
>> virDomainDefFormatFull() is added to make it easier for the
>
> Not really the next any more it seems - definitely in future ones though.
>
>> next patch to add a few callers without having to revisit all
>> existing clients of virDomainDefFormat().
>>
>> @@ -28212,6 +28212,8 @@ virDomainVsockDefFormat(virBufferPtr buf,
>> int
>> virDomainDefFormatInternal(virDomainDefPtr def,
>> virCapsPtr caps,
>> + virDomainSnapshotObjListPtr snapshots,
>> + virDomainSnapshotObjPtr current_snapshot,
>> unsigned int flags,
>> virBufferPtr buf,
>> virDomainXMLOptionPtr xmlopt)
>
> Rather than possibly continually adding parameters to
> virDomainDefFormatInternal, maybe we should take the plunge now to
> create a local struct that would be passed along that would fill in
> fields based on what "extra" format flags beyond the common set are
> being used.
>
Ooh, nice idea, especially since I _will_ be adding more parameters for
bulk listing of checkpoints :) Will do for v3 (since, as you point out,
everything beyond patch 1 is now 5.2 material).
> struct virDomainDefFormatInternalFlagsData {
> virDomainSnapshotObjListPtr snapshots;
> virDomainSnapshotObjPtr current_snapshot;
> };
>
> I am of course assuming checkpoints in the future will do something similar.
Yep.
>> @@ -29016,6 +29025,23 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>>
>> virDomainSEVDefFormat(buf, def->sev);
>>
>
> I think all of the next hunk should be in it's own helper method
>
>> + if (flags & VIR_DOMAIN_DEF_FORMAT_SNAPSHOTS) {
>> + unsigned int snapflags = flags & VIR_DOMAIN_DEF_FORMAT_SECURE ?
>> + VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE : 0;
>> +
>> + virBufferAddLit(buf, "<snapshots");
>> + if (current_snapshot)
>> + virBufferEscapeString(buf, " current='%s'",
>> + current_snapshot->def->name);
>> + virBufferAddLit(buf, ">\n");
>> + virBufferAdjustIndent(buf, 2);
>> + if (virDomainSnapshotObjListFormat(buf, uuidstr, snapshots, caps,
>> + xmlopt, snapflags))
Can do. Or maybe virDomainSnapshotObjListFormat() should do ALL of the
work (but then it needs a pointer to the current snapshot - but then
again, you also made me question whether the current snapshot should be
an internal detail to virDomainSnapshotObjList rather than tracked
separately, so that alone is a separate cleanup potentially worth doing
first).
>
> So this answers my question from patch4...
>
> Anyway, the return checking should be < 0 and I believe the @childrenBuf
> is how other similar constructs attempted for format the child XML
> output - I believe it could be used in this case as well or at least a
> similarly named/used childrenBuf in a method/helper.
I'm not sure a childrenBuf adds much. After all, whether we do:
most output direct to main buf
snapshot output to children buf
concatenate children buf to main buf
or
most output direct to main buf
snapshot output to main buf
we get the same result: on success, main buf is populated; on failure,
main buf may be half-built but it is going to be thrown away.
>> +char *
>> +virDomainDefFormatFull(virDomainDefPtr def, virCapsPtr caps,
>> + virDomainSnapshotObjListPtr snapshots,
>> + virDomainSnapshotObjPtr current_snapshot,
>> + unsigned int flags)
>
> Naming is hard, but is "Full" just another way of saying "all flags" not
> just the common ones?
>
> Not that we necessarily want to see N different implementations of
> virDomainDefFormatXXXX; however, if at some point in the future someone
> wanted to list all Checkpoints (;-)), but not all Snapshots - then would
> *Full still fit the bill or would we need a new method.
The alternative is to fix all current callers to pass the extra
parameter(s) even when they don't use them. Which may not be too bad, if
I take your advice to use a struct (so all existing callers pass NULL,
and don't have to be touched again), instead of my current approach of
adding yet more parameters for every additional thing to be printed. I
don't necessarily need this helper function (test_driver.c was the only
immediate beneficiary), but having it reduced the churn (all other
non-qemu drivers have to be touched due to an added parameter, if I
don't add it).
>
> Just want to be sure we rightly name it considering your knowledge of
> intended future changes.
>
> It almost seems DefFormat is "format only common flags" while
> DefFormatFull would be format all flags that
> virDomainDefFormatConvertXMLFlags can handle beyond the original set.
>
> If you feel *Full is fine - I'm not against it - just throwing out a
> something to consider...
It may go away once I implement your idea of an auxiliary struct (where
we have the one-time hit to other drivers to pass NULL, but then avoid
further churn).
>
>> +{
>> + virBuffer buf = VIR_BUFFER_INITIALIZER;
>> +
>> + virCheckFlags(VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS |
>> + VIR_DOMAIN_DEF_FORMAT_SNAPSHOTS, NULL);
>> + if (virDomainDefFormatInternal(def, caps, snapshots, current_snapshot,
>> + flags, &buf, NULL) < 0)
>> + return NULL;
>> +
>> + return virBufferContentAndReset(&buf);
>> +}
>> +
>> +
>> char *
>> virDomainDefFormat(virDomainDefPtr def, virCapsPtr caps, unsigned int flags)
>> {
>> - virBuffer buf = VIR_BUFFER_INITIALIZER;
>> -
>> virCheckFlags(VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS, NULL);
>
> Considering virDomainDefFormatFull does check this - do we need to also
> check it?
This is slightly different than FormatFull() - that one accepts
VIR_DOMAIN_XML_SNAPSHOTS, while this one rejects it. (In other words, it
is one last safety check that no one requests snapshot XML without also
passing in a snapshots objlist to be printed).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.