[libvirt] [PATCH 1/2] storage: Move and rename getVhbaSCSIHostParent

Michal Privoznik mprivozn at redhat.com
Fri Nov 28 14:55:26 UTC 2014


On 18.11.2014 22:26, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1159180
>
> Move the API from the backend to storage_conf and rename it to
> virStoragePoolGetVhbaSCSIHostParent.  A future patch will need to
> use this functionality from storage_conf
>
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>   src/conf/storage_conf.c            | 66 ++++++++++++++++++++++++++++++++++++++
>   src/conf/storage_conf.h            |  5 +++
>   src/libvirt_private.syms           |  1 +
>   src/storage/storage_backend_scsi.c | 66 ++------------------------------------
>   4 files changed, 74 insertions(+), 64 deletions(-)
>
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 6ad37f8..751c0c0 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -2078,6 +2078,72 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,
>       return ret;
>   }
>
> +/*
> + * virStoragePoolGetVhbaSCSIHostParent:
> + *
> + * Using the Node Device Driver, find the host# name found via wwnn/wwpn
> + * lookup in the fc_host sysfs tree (e.g. virGetFCHostNameByWWN) to get
> + * the parent 'scsi_host#'.
> + *
> + * @conn: Connection pointer (must be non-NULL on entry)
> + * @name: Pointer a string from a virGetFCHostNameByWWN (e.g., "host#")
> + *
> + * Returns a "scsi_host#" string of the parent of the vHBA
> + */
> +char *
> +virStoragePoolGetVhbaSCSIHostParent(virConnectPtr conn,
> +                                    const char *name)
> +{
> +    char *nodedev_name = NULL;
> +    virNodeDevicePtr device = NULL;
> +    char *xml = NULL;
> +    virNodeDeviceDefPtr def = NULL;
> +    char *vhba_parent = NULL;
> +    virErrorPtr savedError = NULL;
> +
> +    VIR_DEBUG("conn=%p, name=%s", conn, name);
> +
> +    /* We get passed "host#" from the return from virGetFCHostNameByWWN,
> +     * so we need to adjust that to what the nodedev lookup expects
> +     */
> +    if (virAsprintf(&nodedev_name, "scsi_%s", name) < 0)
> +        goto cleanup;
> +
> +    /* Compare the scsi_host for the name with the provided parent
> +     * if not the same, then fail
> +     */
> +    if (!(device = virNodeDeviceLookupByName(conn, nodedev_name))) {
> +        virReportError(VIR_ERR_XML_ERROR,
> +                       _("Cannot find '%s' in node device database"),
> +                       nodedev_name);
> +        goto cleanup;
> +    }
> +
> +    if (!(xml = virNodeDeviceGetXMLDesc(device, 0)))
> +        goto cleanup;
> +
> +    if (!(def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL)))
> +        goto cleanup;
> +
> +    /* The caller checks whether the returned value is NULL or not
> +     * before continuing
> +     */
> +    ignore_value(VIR_STRDUP(vhba_parent, def->parent));
> +
> + cleanup:
> +    if (!vhba_parent)
> +        savedError = virSaveLastError();
> +    VIR_FREE(nodedev_name);
> +    virNodeDeviceDefFree(def);
> +    VIR_FREE(xml);
> +    virNodeDeviceFree(device);
> +    if (savedError) {
> +        virSetError(savedError);
> +        virFreeError(savedError);
> +    }
> +    return vhba_parent;


While you're at this, would it make sense to:

1) s/virNodeDeviceFree/virObjectUnref/
2) drop savedError?

Firstly, virNodeDeviceFree() must be guarded with check for device != 
NULL (yep, our public API madness where free(NULL) is not accepted). 
Moreover, since it is a public API it resets the error object, which you 
don't want to and hence you save the error. But using the very same 
function that virNodeDeviceFree() is using, you will achieve the same 
goal and without any temporary variables and cleaner code.

Michal




More information about the libvir-list mailing list