Re: [libvirt] [PATCH 3/4] snapshot: expose location through virsh snapshot-info

On 11/13/12 20:09, Eric Blake wrote: > Now that we can filter on this information, we should also make > it easy to get at. > > * tools/virsh-snapshot.c (cmdSnapshotInfo): Add another output > row. > --- > tools/virsh-snapshot.c | 32 +++++++++++++++++++++++++++----- > 1 file changed, 27 insertions(+), 5 deletions(-)

> - tmp = strstr(doc, "<state>"); > - if (!tmp) { > + state = strstr(doc, "<state>"); > + if (!state) { > vshError(ctl, "%s", > _("unexpected problem reading snapshot xml")); > goto cleanup; > } > - tmp += strlen("<state>"); > + state += strlen("<state>"); > vshPrint(ctl, "%-15s %.*s\n", _("State:"), > - (int) (strchr(tmp, '<') - tmp), tmp); > + (int) (strchr(state, '<') - state), state); > + > + /* In addition to state, location is useful. If the snapshot has > + * a <memory> element, then the existence of snapshot='external' > + * prior to <domain> is the deciding factor; for snapshots > + * created prior to 1.0.1, a state of disk-only is the only > + * external snapshot. */ > + if (strstr(state, "<memory")) { > + char *domain = strstr(state, "<domain"); Bleah. Raw XML parsing. Wouldn't it be easier in and cleaner convert this piece code to use the XML parser and xpath?

The code looks OK in what it should be doing, but I don't like the raw XML parse-hacking stuff. The only reason to put this in as-is would be if it would be really complicated/overheading to use xpath here.