On 2013年02月11日 21:10, Peter Krempa wrote:

This patch simplifies the creation of XML, some error paths and adds correct approach to check for virBuffer errors. --- tools/virsh-snapshot.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 66776e2..fe1cee9 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -427,19 +427,16 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "live")) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_LIVE;

- dom = vshCommandOptDomain(ctl, cmd, NULL); - if (dom == NULL) - goto cleanup; + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + return false;

if (vshCommandOptStringReq(ctl, cmd, "name",&name)< 0 || vshCommandOptStringReq(ctl, cmd, "description",&desc)< 0) goto cleanup;

virBufferAddLit(&buf, "<domainsnapshot>\n"); - if (name) - virBufferEscapeString(&buf, "<name>%s</name>\n", name); - if (desc) - virBufferEscapeString(&buf, "<description>%s</description>\n", desc); + virBufferEscapeString(&buf, "<name>%s</name>\n", name); + virBufferEscapeString(&buf, "<description>%s</description>\n", desc);

if (vshCommandOptStringReq(ctl, cmd, "memspec",&memspec)< 0) goto cleanup; @@ -457,12 +454,13 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) } virBufferAddLit(&buf, "</domainsnapshot>\n");

- buffer = virBufferContentAndReset(&buf); - if (buffer == NULL) { + if (virBufferError(&buf)) { vshError(ctl, "%s", _("Out of memory")); goto cleanup; }

+ buffer = virBufferContentAndReset(&buf); + if (vshCommandOptBool(cmd, "print-xml")) { vshPrint(ctl, "%s\n", buffer); ret = true; @@ -474,8 +472,7 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) cleanup: virBufferFreeAndReset(&buf); VIR_FREE(buffer); - if (dom) - virDomainFree(dom); + virDomainFree(dom);

return ret; }

ACK

On 2013年02月11日 21:10, Peter Krempa wrote:

Manual for "virsh snapshot-create-as" states that --no-metadata and --print-xml are incompatible. Honor this detail in the code. --- tools/virsh-snapshot.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index fe1cee9..d9659d4 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -412,8 +412,14 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) unsigned int flags = 0; const vshCmdOpt *opt = NULL;

- if (vshCommandOptBool(cmd, "no-metadata")) + if (vshCommandOptBool(cmd, "no-metadata")) { + if (vshCommandOptBool(cmd, "print-xml")) {

Considering using a variable to store the return value instead of calling the function twice.

+ vshError(ctl, "%s", + _("--print-xml is incompatible with --no-metadata")); + return false; + } flags |= VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA; + } if (vshCommandOptBool(cmd, "halt")) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_HALT; if (vshCommandOptBool(cmd, "disk-only"))

But I'm fine if you keep the twice calling, as it's trivial. ACK.