Re: [libvirt] [PATCH 3/5] snapshot: refactor virsh snapshot parent computation

Make parent computation reusable, using virDomainSnapshotGetParent when possible. * tools/virsh.c (vshGetSnapshotParent): New helper. (cmdSnapshotParent): Use it. --- tools/virsh.c | 66 ++++++++++++++++++++++++++++++++++++++++++++------------- 1 files changed, 51 insertions(+), 15 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 7b0533d..035b209 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -246,6 +246,8 @@ typedef struct __vshControl { char *historyfile; /* readline history file name */ bool useGetInfo; /* must use virDomainGetInfo, since virDomainGetState is not supported */ + bool useSnapshotGetXML; /* must use virDomainSnapshotGetXMLDesc, since + virDomainSnapshotGetParent is missing */ } __vshControl; typedef struct vshCmdGrp { @@ -613,6 +615,7 @@ vshReconnect(vshControl *ctl) vshError(ctl, "%s", _("Reconnected to the hypervisor")); disconnected = 0; ctl->useGetInfo = false; + ctl->useSnapshotGetXML = false; } /* --------------- @@ -760,6 +763,7 @@ cmdConnect(vshControl *ctl, const vshCmd *cmd) ctl->name = vshStrdup(ctl, name); ctl->useGetInfo = false; + ctl->useSnapshotGetXML = false; ctl->readonly = ro; ctl->conn = virConnectOpenAuth(ctl->name, virConnectAuthPtrDefault, @@ -12967,6 +12971,52 @@ cleanup: return ret; } +/* Helper function to get the name of a snapshot's parent. Caller + * must free the result. */ +static char * +vshGetSnapshotParent(vshControl *ctl, virDomainSnapshotPtr snapshot) +{ + virDomainSnapshotPtr parent = NULL; + char *xml = NULL; + xmlDocPtr xmldoc = NULL; + xmlXPathContextPtr ctxt = NULL; + char *parent_name = NULL; + + /* Try new API, since it is faster. */ + if (!ctl->useSnapshotGetXML) { + parent = virDomainSnapshotGetParent(snapshot, 0); + if (parent) { + /* API works, and virDomainSnapshotGetName will succeed */ + parent_name = vshStrdup(ctl, virDomainSnapshotGetName(snapshot)); + goto cleanup; + } + if (last_error->code == VIR_ERR_NO_DOMAIN_SNAPSHOT) { + /* API works, and we found a root with no parent */ + goto cleanup; + } + /* API didn't work, fall back to XML scraping. */ + ctl->useSnapshotGetXML = true; + } + + xml = virDomainSnapshotGetXMLDesc(snapshot, 0); + if (!xml) + goto cleanup; + + xmldoc = virXMLParseStringCtxt(xml, _("(domain_snapshot)"), &ctxt); + if (!xmldoc) + goto cleanup; + + parent_name = virXPathString("string(/domainsnapshot/parent/name)", ctxt); + +cleanup: + if (parent) + virDomainSnapshotFree(parent); + xmlXPathFreeContext(ctxt); + xmlFreeDoc(xmldoc); + VIR_FREE(xml); + return parent_name; +} + /* * "snapshot-list" command */ @@ -13220,10 +13270,7 @@ cmdSnapshotParent(vshControl *ctl, const vshCmd *cmd) bool ret = false; const char *name = NULL; virDomainSnapshotPtr snapshot = NULL; - char *xml = NULL; char *parent = NULL; - xmlDocPtr xmldoc = NULL; - xmlXPathContextPtr ctxt = NULL; if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -13239,15 +13286,7 @@ cmdSnapshotParent(vshControl *ctl, const vshCmd *cmd) if (snapshot == NULL) goto cleanup; - xml = virDomainSnapshotGetXMLDesc(snapshot, 0); - if (!xml) - goto cleanup; - - xmldoc = virXMLParseStringCtxt(xml, _("(domain_snapshot)"), &ctxt); - if (!xmldoc) - goto cleanup; - - parent = virXPathString("string(/domainsnapshot/parent/name)", ctxt); + parent = vshGetSnapshotParent(ctl, snapshot); if (!parent) goto cleanup; @@ -13257,9 +13296,6 @@ cmdSnapshotParent(vshControl *ctl, const vshCmd *cmd) cleanup: VIR_FREE(parent); - xmlXPathFreeContext(ctxt); - xmlFreeDoc(xmldoc); - VIR_FREE(xml); if (snapshot) virDomainSnapshotFree(snapshot); if (dom)