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

[Osier Yang <jyang@xxxxxxxxxx>]

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