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

On 2013年02月11日 21:33, Daniel P. Berrange wrote: > 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] ? vshStringToArray substitutes the delimiter ',' with '\0', and the elements simply point to the pieces. So freeing the first element frees all the memory of the array elements.

> >> + VIR_FREE(arr); >> + } > > > ACK if that leak is fixed, or otherwise explained. > Thanks for the reviewing, I pushed this set. Osier -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list