Let's move some variables from an inside loop to global function declaration header block. It's going to be easier for next patches. At the same time, order the cleanup calls at the function's end so it's easier to track which variables are freed and which not. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- tools/virsh-domain-monitor.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 340a8e2..d4e500b 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -549,24 +544,28 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd) if (details) { vshPrint(ctl, "%-10s %-10s %-10s %s\n", type, device, target, source ? source : "-"); - VIR_FREE(type); - VIR_FREE(device); } else { vshPrint(ctl, "%-10s %s\n", target, source ? source : "-"); } - VIR_FREE(target); VIR_FREE(source); + VIR_FREE(target); + VIR_FREE(device); + VIR_FREE(type); } ret = true; cleanup: + VIR_FREE(source); + VIR_FREE(target); + VIR_FREE(device); + VIR_FREE(type); VIR_FREE(disks); - virDomainFree(dom); - VIR_FREE(xml); - xmlFreeDoc(xmldoc); xmlXPathFreeContext(ctxt); + xmlFreeDoc(xmldoc); + VIR_FREE(xml); + virDomainFree(dom);

return ret; } -- 2.4.6 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 09/02/2015 11:58 AM, Michal Privoznik wrote: > From: Lin Ma <lma(a)suse.com&gt; > > Format & output more detailed information about networked source > > e.g: The output without the patch: > $ virsh domblklist $DOMAIN --details > Type Device Target Source > ------------------------------------------------ > network disk vda test-pool/image > network disk vdb iqn.2015-08.org.example:sn01/0 > network disk vdc /image.raw > network disk vdd - > network disk vde - > network disk vdf image1 > network disk vdg test-volume/image.raw > > The output with the patch: > $ virsh domblklist $DOMAIN --details > Type Device Target Source > ------------------------------------------------ > network disk vda rbd://monitor1.example.org:6321/test-pool/image > network disk vdb iscsi://192.168.124.200:3260/iqn.2015-08.org.example:sn01/0 > network disk vdc http://192.168.124.200:80/image.raw > network disk vdd nbd+unix:///var/run/nbdsock > network disk vde nbd://192.168.124.200:12345 > network disk vdf sheepdog://192.168.124.200:6000/image1 > network disk vdg gluster://192.168.124.200/test-volume/image.raw Is the goal to just format in some "standard" format or to use the format that would be used by qemu in the command line?

If I look at the code in qemuBuildNetworkDriveURI, it seems there's a provision for nbd, sheepdog, and rbd to be "different" with nbd having my least favorite construct to read "if (!((multiple choice || constructs)))" It seems nbd can either have uri format with "://" found or a format such as "nbd:unix:$socket" or "nbd:$hostname:$port". It seems sheepdog doesn't have to have a $host value and thus would have just "sheepdog:$source-path". It seems rbd doesn't conform to any norm - good luck with that one! While I don't disagree this is better than what is currently provided, I worry whether all 'options' are technically correct given what we send to qemu or conversely what perhaps someone provided to qemu and we attached to (whether that's possible still currently I'm not sure). See also qemuParseNBDString, qemuParseRBDString, qemuParseGlusterString, etc from qemu_command.c BTW: If the goal is to just format it some consistent manner, then the code seems fine to me... John > Signed-off-by: Lin Ma <lma(a)suse.com&gt; > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > --- > tools/virsh-domain-monitor.c | 64 ++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 59 insertions(+), 5 deletions(-) > > diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c > index d4e500b..6446549 100644 > --- a/tools/virsh-domain-monitor.c > +++ b/tools/virsh-domain-monitor.c > @@ -486,10 +486,17 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd) > xmlNodePtr *disks = NULL; > size_t i; > bool details = false; > + virBuffer buf = VIR_BUFFER_INITIALIZER; > char *type = NULL; > char *device = NULL; > char *target = NULL; > char *source = NULL; > + char *protocol = NULL; > + char *transport = NULL; > + char *host_name = NULL; > + char *host_port = NULL; > + char *socket = NULL; > + char *name = NULL; > > if (vshCommandOptBool(cmd, "inactive")) > flags |= VIR_DOMAIN_XML_INACTIVE; > @@ -536,11 +543,45 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd) > vshError(ctl, "unable to query block list"); > goto cleanup; > } > - source = virXPathString("string(./source/@file" > - "|./source/@dev" > - "|./source/@dir" > - "|./source/@name" > - "|./source/@volume)", ctxt); > + if (type && STREQ(type, "network")) { > + protocol = virXPathString("string(./source/@protocol)", ctxt); > + name = virXPathString("string(./source/@name)", ctxt); > + transport = virXPathString("string(./source/host/@transport)", ctxt); > + socket = virXPathString("string(./source/host/@socket)", ctxt); > + host_name = virXPathString("string(./source/host/@name)", ctxt); > + host_port = virXPathString("string(./source/host/@port)", ctxt); > + > + virBufferAddStr(&buf, protocol); > + > + if (transport) > + virBufferAsprintf(&buf, "+%s", transport); > + virBufferAddLit(&buf, "://"); > + if (host_name) { > + virBufferAddStr(&buf, host_name); > + if (host_port) > + virBufferAsprintf(&buf, ":%s", host_port); > + } > + if (name) { > + if (!STRPREFIX(name, "/")) > + virBufferAddChar(&buf, '/'); > + virBufferAddStr(&buf, name); > + } else if (socket) { > + if (!STRPREFIX(socket, "/")) > + virBufferAddChar(&buf, '/'); > + virBufferAddStr(&buf, socket); > + } > + if (virBufferError(&buf)) { > + virReportOOMError(); > + goto cleanup; > + } > + source = virBufferContentAndReset(&buf); > + } else { > + source = virXPathString("string(./source/@file" > + "|./source/@dev" > + "|./source/@dir" > + "|./source/@name" > + "|./source/@volume)", ctxt); > + } > if (details) { > vshPrint(ctl, "%-10s %-10s %-10s %s\n", type, device, > target, source ? source : "-"); > @@ -548,6 +589,12 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd) > vshPrint(ctl, "%-10s %s\n", target, source ? source : "-"); > } > > + VIR_FREE(name); > + VIR_FREE(socket); > + VIR_FREE(host_port); > + VIR_FREE(host_name); > + VIR_FREE(transport); > + VIR_FREE(protocol); > VIR_FREE(source); > VIR_FREE(target); > VIR_FREE(device); > @@ -557,10 +604,17 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd) > ret = true; > > cleanup: > + VIR_FREE(name); > + VIR_FREE(socket); > + VIR_FREE(host_port); > + VIR_FREE(host_name); > + VIR_FREE(transport); > + VIR_FREE(protocol); > VIR_FREE(source); > VIR_FREE(target); > VIR_FREE(device); > VIR_FREE(type); > + virBufferFreeAndReset(&buf); > VIR_FREE(disks); > xmlXPathFreeContext(ctxt); > xmlFreeDoc(xmldoc); > -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

From: Lin Ma <lma(a)suse.com&gt; Format & output more detailed information about networked source e.g: The output without the patch: $ virsh domblklist $DOMAIN --details Type Device Target Source ------------------------------------------------ network disk vda test-pool/image network disk vdb iqn.2015-08.org.example:sn01/0 network disk vdc /image.raw network disk vdd - network disk vde - network disk vdf image1 network disk vdg test-volume/image.raw The output with the patch: $ virsh domblklist $DOMAIN --details Type Device Target Source ------------------------------------------------ network disk vda rbd://monitor1.example.org:6321/test-pool/image network disk vdb iscsi://192.168.124.200:3260/iqn.2015-08.org.example:sn01/0 network disk vdc http://192.168.124.200:80/image.raw network disk vdd nbd+unix:///var/run/nbdsock network disk vde nbd://192.168.124.200:12345 network disk vdf sheepdog://192.168.124.200:6000/image1 network disk vdg gluster://192.168.124.200/test-volume/image.raw Signed-off-by: Lin Ma <lma(a)suse.com&gt; Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- tools/virsh-domain-monitor.c | 64 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 59 insertions(+), 5 deletions(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index d4e500b..6446549 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -486,10 +486,17 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd) xmlNodePtr *disks = NULL; size_t i; bool details = false; + virBuffer buf = VIR_BUFFER_INITIALIZER; char *type = NULL; char *device = NULL; char *target = NULL; char *source = NULL; + char *protocol = NULL; + char *transport = NULL; + char *host_name = NULL; + char *host_port = NULL; + char *socket = NULL; + char *name = NULL; if (vshCommandOptBool(cmd, "inactive")) flags |= VIR_DOMAIN_XML_INACTIVE; @@ -536,11 +543,45 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd) vshError(ctl, "unable to query block list"); goto cleanup; } - source = virXPathString("string(./source/@file" - "|./source/@dev" - "|./source/@dir" - "|./source/@name" - "|./source/@volume)", ctxt); + if (type && STREQ(type, "network")) { + protocol = virXPathString("string(./source/@protocol)", ctxt); + name = virXPathString("string(./source/@name)", ctxt); + transport = virXPathString("string(./source/host/@transport)", ctxt); + socket = virXPathString("string(./source/host/@socket)", ctxt); + host_name = virXPathString("string(./source/host/@name)", ctxt); + host_port = virXPathString("string(./source/host/@port)", ctxt); + + virBufferAddStr(&buf, protocol); + + if (transport) + virBufferAsprintf(&buf, "+%s", transport); + virBufferAddLit(&buf, "://"); + if (host_name) { + virBufferAddStr(&buf, host_name); + if (host_port) + virBufferAsprintf(&buf, ":%s", host_port); + } + if (name) { + if (!STRPREFIX(name, "/")) + virBufferAddChar(&buf, '/'); + virBufferAddStr(&buf, name); + } else if (socket) { + if (!STRPREFIX(socket, "/")) + virBufferAddChar(&buf, '/'); + virBufferAddStr(&buf, socket); + } + if (virBufferError(&buf)) { + virReportOOMError(); + goto cleanup; + } + source = virBufferContentAndReset(&buf); + } else { + source = virXPathString("string(./source/@file" + "|./source/@dev" + "|./source/@dir" + "|./source/@name" + "|./source/@volume)", ctxt); + } if (details) { vshPrint(ctl, "%-10s %-10s %-10s %s\n", type, device, target, source ? source : "-"); @@ -548,6 +589,12 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd) vshPrint(ctl, "%-10s %s\n", target, source ? source : "-"); } + VIR_FREE(name); + VIR_FREE(socket); + VIR_FREE(host_port); + VIR_FREE(host_name); + VIR_FREE(transport); + VIR_FREE(protocol); VIR_FREE(source); VIR_FREE(target); VIR_FREE(device); @@ -557,10 +604,17 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: + VIR_FREE(name); + VIR_FREE(socket); + VIR_FREE(host_port); + VIR_FREE(host_name); + VIR_FREE(transport); + VIR_FREE(protocol); VIR_FREE(source); VIR_FREE(target); VIR_FREE(device); VIR_FREE(type); + virBufferFreeAndReset(&buf); VIR_FREE(disks); xmlXPathFreeContext(ctxt); xmlFreeDoc(xmldoc);