[libvirt] [PATCH 0/2] snapshot: allow editing disk-only snapshots
I tested that both of these patches in isolation were sufficient to fix 'virsh snapshot-current dom name' when running the same version of virsh as libvirtd (what will become 0.9.7). However, both patches are required, so that both virsh 0.9.5 and libvirtd 0.9.7, as well as virsh 0.9.7 and libvirt 0.9.5, will work (leaving only the combination of 0.9.5 in both virsh and libvirtd broken). Eric Blake (2): snapshot: let virsh edit disk snapshots snapshot: simplify redefinition of disk snapshot src/conf/domain_conf.c | 44 +++++++++++++++++++++++--------------------- tools/virsh.c | 5 +++++ 2 files changed, 28 insertions(+), 21 deletions(-) -- 1.7.4.4
It was impossible for 'virsh snapshot-current dom name' to set name as the current snapshot, if name is a disk-only snapshot. Using strstr rather than full-blown xml parsing is safe, since the xml is assumed to be well-formed coming from libvirtd rather than arbitrary text coming from the user. * tools/virsh.c (cmdSnapshotCurrent, cmdSnapshotEdit): Pass disk_only flag when redefining a disk snapshot. --- tools/virsh.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 48f2b8a..70a4078 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -12869,6 +12869,9 @@ cmdSnapshotEdit(vshControl *ctl, const vshCmd *cmd) virDomainSnapshotFree(snapshot); snapshot = NULL; + if (strstr(doc, "<state>disk-snapshot</state>")) + define_flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY; + /* Create and open the temporary file. */ tmp = editWriteToTempFile(ctl, doc); if (!tmp) @@ -12978,6 +12981,8 @@ cmdSnapshotCurrent(vshControl *ctl, const vshCmd *cmd) xml = virDomainSnapshotGetXMLDesc(snapshot, VIR_DOMAIN_XML_SECURE); if (!xml) goto cleanup; + if (strstr(xml, "<state>disk-snapshot</state>")) + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY; snapshot2 = virDomainSnapshotCreateXML(dom, xml, flags); if (snapshot2 == NULL) goto cleanup; -- 1.7.4.4
On Thu, Oct 06, 2011 at 05:18:33PM -0600, Eric Blake wrote:
It was impossible for 'virsh snapshot-current dom name' to set name as the current snapshot, if name is a disk-only snapshot.
Using strstr rather than full-blown xml parsing is safe, since the xml is assumed to be well-formed coming from libvirtd rather than arbitrary text coming from the user.
* tools/virsh.c (cmdSnapshotCurrent, cmdSnapshotEdit): Pass disk_only flag when redefining a disk snapshot. --- tools/virsh.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 48f2b8a..70a4078 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -12869,6 +12869,9 @@ cmdSnapshotEdit(vshControl *ctl, const vshCmd *cmd) virDomainSnapshotFree(snapshot); snapshot = NULL;
+ if (strstr(doc, "<state>disk-snapshot</state>")) + define_flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY; +
ahum, but okay ... maybe a small comment in the code
/* Create and open the temporary file. */ tmp = editWriteToTempFile(ctl, doc); if (!tmp) @@ -12978,6 +12981,8 @@ cmdSnapshotCurrent(vshControl *ctl, const vshCmd *cmd) xml = virDomainSnapshotGetXMLDesc(snapshot, VIR_DOMAIN_XML_SECURE); if (!xml) goto cleanup; + if (strstr(xml, "<state>disk-snapshot</state>")) + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY;
same,
snapshot2 = virDomainSnapshotCreateXML(dom, xml, flags); if (snapshot2 == NULL) goto cleanup;
ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 10/07/2011 08:13 AM, Daniel Veillard wrote:
On Thu, Oct 06, 2011 at 05:18:33PM -0600, Eric Blake wrote:
It was impossible for 'virsh snapshot-current dom name' to set name as the current snapshot, if name is a disk-only snapshot.
Using strstr rather than full-blown xml parsing is safe, since the xml is assumed to be well-formed coming from libvirtd rather than arbitrary text coming from the user.
+ if (strstr(doc, "<state>disk-snapshot</state>")) + define_flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY; +
ahum, but okay ... maybe a small comment in the code
Sure. I added that,
ACK,
then pushed both patches. Thanks for the speedy review. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Redefining disk-only snapshot xml should work even if the user did not explicitly pass VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY; the flag is only required for conditions where the <state> subelement is not already present in parsing (that is, defining a new snapshot). Also, fix the error code of some user-visible errors (the remaining VIR_ERR_INTERNAL_ERROR should not be user-visible, since parsing of <active> is only done from internal code). * src/conf/domain_conf.c (virDomainSnapshotDefParseString): Allow disks during redefinition of disk snapshot. --- src/conf/domain_conf.c | 44 +++++++++++++++++++++++--------------------- 1 files changed, 23 insertions(+), 21 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f9007ce..34bf2d0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11696,25 +11696,6 @@ virDomainSnapshotDefParseString(const char *xmlStr, def->description = virXPathString("string(./description)", ctxt); - if ((i = virXPathNodeSet("./disks/*", ctxt, &nodes)) < 0) - goto cleanup; - if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_DISKS) { - def->ndisks = i; - if (def->ndisks && VIR_ALLOC_N(def->disks, def->ndisks) < 0) { - virReportOOMError(); - goto cleanup; - } - for (i = 0; i < def->ndisks; i++) { - if (virDomainSnapshotDiskDefParseXML(nodes[i], &def->disks[i]) < 0) - goto cleanup; - } - VIR_FREE(nodes); - } else if (i) { - virDomainReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("unable to handle disk requests in snapshot")); - goto cleanup; - } - if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { if (virXPathLongLong("string(./creationTime)", ctxt, &def->creationTime) < 0) { @@ -11730,13 +11711,13 @@ virDomainSnapshotDefParseString(const char *xmlStr, /* there was no state in an existing snapshot; this * should never happen */ - virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + virDomainReportError(VIR_ERR_XML_ERROR, "%s", _("missing state from existing snapshot")); goto cleanup; } def->state = virDomainSnapshotStateTypeFromString(state); if (def->state < 0) { - virDomainReportError(VIR_ERR_INTERNAL_ERROR, + virDomainReportError(VIR_ERR_XML_ERROR, _("Invalid state '%s' in domain snapshot XML"), state); goto cleanup; @@ -11768,6 +11749,27 @@ virDomainSnapshotDefParseString(const char *xmlStr, def->creationTime = tv.tv_sec; } + if ((i = virXPathNodeSet("./disks/*", ctxt, &nodes)) < 0) + goto cleanup; + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_DISKS || + (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE && + def->state == VIR_DOMAIN_DISK_SNAPSHOT)) { + def->ndisks = i; + if (def->ndisks && VIR_ALLOC_N(def->disks, def->ndisks) < 0) { + virReportOOMError(); + goto cleanup; + } + for (i = 0; i < def->ndisks; i++) { + if (virDomainSnapshotDiskDefParseXML(nodes[i], &def->disks[i]) < 0) + goto cleanup; + } + VIR_FREE(nodes); + } else if (i) { + virDomainReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("unable to handle disk requests in snapshot")); + goto cleanup; + } + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL) { if (virXPathInt("string(./active)", ctxt, &active) < 0) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", -- 1.7.4.4
On Thu, Oct 06, 2011 at 05:18:34PM -0600, Eric Blake wrote:
Redefining disk-only snapshot xml should work even if the user did not explicitly pass VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY; the flag is only required for conditions where the <state> subelement is not already present in parsing (that is, defining a new snapshot).
Also, fix the error code of some user-visible errors (the remaining VIR_ERR_INTERNAL_ERROR should not be user-visible, since parsing of <active> is only done from internal code).
* src/conf/domain_conf.c (virDomainSnapshotDefParseString): Allow disks during redefinition of disk snapshot. --- src/conf/domain_conf.c | 44 +++++++++++++++++++++++--------------------- 1 files changed, 23 insertions(+), 21 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f9007ce..34bf2d0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11696,25 +11696,6 @@ virDomainSnapshotDefParseString(const char *xmlStr,
def->description = virXPathString("string(./description)", ctxt);
- if ((i = virXPathNodeSet("./disks/*", ctxt, &nodes)) < 0) - goto cleanup; - if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_DISKS) { - def->ndisks = i; - if (def->ndisks && VIR_ALLOC_N(def->disks, def->ndisks) < 0) { - virReportOOMError(); - goto cleanup; - } - for (i = 0; i < def->ndisks; i++) { - if (virDomainSnapshotDiskDefParseXML(nodes[i], &def->disks[i]) < 0) - goto cleanup; - } - VIR_FREE(nodes); - } else if (i) { - virDomainReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("unable to handle disk requests in snapshot")); - goto cleanup; - } - if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { if (virXPathLongLong("string(./creationTime)", ctxt, &def->creationTime) < 0) { @@ -11730,13 +11711,13 @@ virDomainSnapshotDefParseString(const char *xmlStr, /* there was no state in an existing snapshot; this * should never happen */ - virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + virDomainReportError(VIR_ERR_XML_ERROR, "%s", _("missing state from existing snapshot")); goto cleanup; } def->state = virDomainSnapshotStateTypeFromString(state); if (def->state < 0) { - virDomainReportError(VIR_ERR_INTERNAL_ERROR, + virDomainReportError(VIR_ERR_XML_ERROR, _("Invalid state '%s' in domain snapshot XML"), state); goto cleanup; @@ -11768,6 +11749,27 @@ virDomainSnapshotDefParseString(const char *xmlStr, def->creationTime = tv.tv_sec; }
+ if ((i = virXPathNodeSet("./disks/*", ctxt, &nodes)) < 0) + goto cleanup; + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_DISKS || + (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE && + def->state == VIR_DOMAIN_DISK_SNAPSHOT)) { + def->ndisks = i; + if (def->ndisks && VIR_ALLOC_N(def->disks, def->ndisks) < 0) { + virReportOOMError(); + goto cleanup; + } + for (i = 0; i < def->ndisks; i++) { + if (virDomainSnapshotDiskDefParseXML(nodes[i], &def->disks[i]) < 0) + goto cleanup; + } + VIR_FREE(nodes); + } else if (i) { + virDomainReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("unable to handle disk requests in snapshot")); + goto cleanup; + } + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL) { if (virXPathInt("string(./active)", ctxt, &active) < 0) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s",
ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (2)
-
Daniel Veillard -
Eric Blake