Re: [libvirt] [PATCH 4/4 v5] virsh: Use virNodeDeviceLookupSCSIHostByWWN

On Mon, Feb 04, 2013 at 10:16:44PM +0800, Osier Yang wrote: > Only nodedev-destroy and nodedev-dumpxml can benifit from the > new API, other commands like nodedev-detach only works for > PCI devices, WWN makes no sense for them. > > --- > Rebased on Peter's virsh cleanup patches. > --- > tools/virsh-nodedev.c | 84 +++++++++++++++++++++++++++++++++++++++--------- > tools/virsh.pod | 15 +++++--- > 2 files changed, 77 insertions(+), 22 deletions(-) > > diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c > index f85bded..7c51a56 100644 > --- a/tools/virsh-nodedev.c > +++ b/tools/virsh-nodedev.c > @@ -101,9 +101,14 @@ static const vshCmdInfo info_node_device_destroy[] = { > > static const vshCmdOptDef opts_node_device_destroy[] = { > {.name = "name", > + .type = VSH_OT_ALIAS, > + .flags = 0, > + .help = "device" > + }, > + {.name = "device", > .type = VSH_OT_DATA, > .flags = VSH_OFLAG_REQ, > - .help = N_("name of the device to be destroyed") > + .help = N_("device name or wwn pair in 'wwnn,wwpn' format") > }, > {.name = NULL} > }; > @@ -112,21 +117,47 @@ static bool > cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd) > { > virNodeDevicePtr dev = NULL; > - bool ret = true; > - const char *name = NULL; > + bool ret = false; > + const char *device_value = NULL; > + char **arr = NULL; > + int narr; > > - if (vshCommandOptStringReq(ctl, cmd, "name",&name)< 0) > + if (vshCommandOptStringReq(ctl, cmd, "device",&device_value)< 0) > return false; > > - dev = virNodeDeviceLookupByName(ctl->conn, name); > + if (strchr(device_value, ',')) { > + narr = vshStringToArray(device_value,&arr); > + if (narr != 2) { > + vshError(ctl, _("Malformed device value '%s'"), device_value); > + goto cleanup; > + } > + > + if (!virValidateWWN(arr[0]) || !virValidateWWN(arr[1])) > + goto cleanup; > + > + dev = virNodeDeviceLookupSCSIHostByWWN(ctl->conn, arr[0], arr[1], 0); > + } else { > + dev = virNodeDeviceLookupByName(ctl->conn, device_value); > + } > + > + if (!dev) { > + vshError(ctl, "%s '%s'", _("Could not find matching device"), device_value); > + goto cleanup; > + } > > if (virNodeDeviceDestroy(dev) == 0) { > - vshPrint(ctl, _("Destroyed node device '%s'\n"), name); > + vshPrint(ctl, _("Destroyed node device '%s'\n"), device_value); > } else { > - vshError(ctl, _("Failed to destroy node device '%s'"), name); > - ret = false; > + vshError(ctl, _("Failed to destroy node device '%s'"), device_value); > + goto cleanup; > } > > + ret = true; > +cleanup: > + if (arr) { > + VIR_FREE(*arr); Hmm, IIUC, that is equiv to VIR_FREE(arr[0]), so what is free'ing arr[1] ?

> + VIR_FREE(arr); > + } ACK if that leak is fixed, or otherwise explained.