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

[Osier Yang]

On 2013年02月11日 22:51, Osier Yang wrote:
> 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.
>

Btw, I think it's time to destroy the use of vshStringToArray
, and use the more general virStringSplit now, (which not only
supports the delimiter ','). It will be later patch.

>>
>>> + 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list