[libvirt] [PATCH v2 05/11] domain: Expand virDomainDefFormatInternal with snapshots
John Ferlan
jferlan at redhat.com
Wed Feb 27 16:27:39 UTC 2019
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 at 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);
> }
>
>
[...]
More information about the libvir-list
mailing list