[libvirt] [PATCH 2/3] node device: Break out get_wwns and get_parent_node helpers

[Matthias Bolte]

2009/10/16 Cole Robinson <crobinso at redhat.com>:
> These will be used by the test driver, so move them to a shareable space.
>
> Signed-off-by: Cole Robinson <crobinso at redhat.com>
> ---
>  src/conf/node_device_conf.c          |   89 +++++++++++++++++++++++++++
>  src/conf/node_device_conf.h          |   11 +++
>  src/libvirt_private.syms             |    2 +
>  src/node_device/node_device_driver.c |  112 ++++------------------------------
>  4 files changed, 115 insertions(+), 99 deletions(-)
>
> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
> index f09f814..77f7be3 100644
> --- a/src/conf/node_device_conf.c
> +++ b/src/conf/node_device_conf.c
> @@ -1215,6 +1215,95 @@ virNodeDeviceDefParseFile(virConnectPtr conn,
>     return virNodeDeviceDefParse(conn, NULL, filename, create);
>  }
>
> +/*
> + * Return fc_host dev's WWNN and WWPN
> + */
> +int
> +virNodeDeviceGetWWNs(virConnectPtr conn,
> +                     virNodeDeviceDefPtr def,
> +                     char **wwnn,
> +                     char **wwpn)
> +{
> +    virNodeDevCapsDefPtr cap = NULL;
> +    int ret = 0;
> +
> +    cap = def->caps;
> +    while (cap != NULL) {
> +        if (cap->type == VIR_NODE_DEV_CAP_SCSI_HOST &&
> +            cap->data.scsi_host.flags & VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) {
> +            *wwnn = strdup(cap->data.scsi_host.wwnn);
> +            *wwpn = strdup(cap->data.scsi_host.wwpn);
> +            break;
> +        }
> +
> +        cap = cap->next;
> +    }
> +
> +    if (cap == NULL) {
> +        virNodeDeviceReportError(conn, VIR_ERR_NO_SUPPORT,
> +                                 "%s", _("Device is not a fibre channel HBA"));
> +        ret = -1;
> +    }
> +
> +    if (*wwnn == NULL || *wwpn == NULL) {

I know you have just moved this function to another file, but here
seems to be a logical error that may result in a false-positive OOM
error.

If the while loop is left without finding the wanted device (cap ==
NULL), then *wwnn and *wwpn have not been set inside this function and
point to their initial value (probably NULL). Because they have not
been set inside this function (in the cap == NULL case), they should
not be tested for NULL to detect an OOM condition.

To fix this

    if (*wwnn == NULL || *wwpn == NULL) {

could be changed to

    else if (*wwnn == NULL || *wwpn == NULL) {

so the OOM check is only done if the wanted device has been found (cap != NULL).

> +        /* Free the other one, if allocated... */
> +        VIR_FREE(wwnn);
> +        VIR_FREE(wwpn);
> +        ret = -1;
> +        virReportOOMError(conn);
> +    }
> +
> +    return ret;
> +}
> +

The OOM detection should be fixed by an additional patch, so ACK to this one.

Matthias