6:18 p.m.
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
6:18 p.m.
New subject: [libvirt] [PATCH 1/2] snapshot: let virsh edit disk snapshots
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
9:13 a.m.
New subject: [libvirt] [PATCH 1/2] snapshot: let virsh edit disk snapshots
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:33 a.m.
New subject: [libvirt] [PATCH 1/2] snapshot: let virsh edit disk snapshots
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
6:18 p.m.
New subject: [libvirt] [PATCH 2/2] snapshot: simplify redefinition of disk snapshot
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
9:15 a.m.
New subject: [libvirt] [PATCH 2/2] snapshot: simplify redefinition of disk snapshot
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
4798
days inactive
4799
days old
5 comments
2 participants
participants (2)
-
Daniel Veillard -
Eric Blake