9:51 p.m.
On 20/06/13 00:35, John Ferlan wrote:
On 06/07/2013 01:03 PM, Osier Yang wrote:
> This takes use of the two utils introduced in previous patches.
>
> Node device HAL backend represents PCI device like PCI_8086_2922,
> (I.E PCI_$vendor_$product), to get the PCI address, we have to
> traverse /sys/devices/ to find it out.
>
> And to get the current scsi host number assigned by the system
> for the scsi host device. We need to traverse /sys/bus/pci/devices/
> with the found PCI address by getScsiHostParentAddress, and the
> specified unique_id.
>
> Later patch will allow to omit the "unique_id", by using the
> scsi host which has the smallest unique_id.
> ---
> src/storage/storage_backend_scsi.c | 81 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 79 insertions(+), 2 deletions(-)
>
> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
> index 13c498d..a77b9ae 100644
> --- a/src/storage/storage_backend_scsi.c
> +++ b/src/storage/storage_backend_scsi.c
> @@ -591,6 +591,66 @@ out:
> return retval;
> }
>
> +/*
> + * Node device udev backend and HAL backend represent PCI
> + * device in different style. Udev backend represents it like
> + * pci_0000_00_1f_2, and HAL backend represents it like
> + * pci_8086_2922.
> + *
> + * Covert the value of "parent" into PCI device address string
> + * (e.g. 0000:00:1f:2). Return the PCI address as string on
> + * success, or -1 on failure.
> + */
> +static char *
> +getScsiHostParentAddress(const char *parent)
> +{
> + char **tokens = NULL;
> + char *vendor = NULL;
> + char *product = NULL;
> + char *ret = NULL;
> + int len;
> +
> + if (!strchr(parent, '_')) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("'parent' of scsi_host adapter must "
> + "be consistent with name of node device"));
> + return NULL;
> + }
> +
> + if (!(tokens = virStringSplit(parent, "_", 0)))
> + return NULL;
Given the following code, the call to Split could use 4 (or 5) for the
max... Not that it matters.
hm, it should be 5.
> +
> + len = virStringListLength(tokens);
> +
> + switch (len) {
> + case 4:
> + if (!(ret = virStringJoin((const char **)(&tokens[1]), ":")))
> + goto cleanup;
> + break;
> +
> + case 2:
> + vendor = tokens[1];
> + product = tokens[2];
> + if (!(ret = virFindPCIDeviceByVPD(NULL, vendor, product))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to find PCI device with vendor
'%s' "
> + "product '%s'"), vendor, product);
> + goto cleanup;
> + }
> +
> + break;
> + default:
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("'parent' of scsi_host adapter must "
> + "be consistent with name of node device"));
> + goto cleanup;
> + }
> +
> +cleanup:
> + virStringFreeList(tokens);
> + return ret;
> +}
> +
> static int
> getHostNumber(const char *adapter_name,
> unsigned int *result)
> @@ -627,10 +687,27 @@ static char *
> getAdapterName(virStoragePoolSourceAdapter adapter)
> {
> char *name = NULL;
> + char *addr = NULL;
>
> if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
> - if (VIR_STRDUP(name, adapter.data.scsi_host.name) < 0)
> - return NULL;
> + if (adapter.data.scsi_host.parent) {
> + if (!adapter.data.scsi_host.has_unique_id) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("'unique_id' is not specified for
"
> + "scsi_host adapter"));
so the optional unique_id isn't so optional after all for a couple of
code paths. Issuing a message here could be confusing. Has this been
tested within an environment where unique_id isn't specified?
It's made optional by later patches. Commit log clarifies it.
> + return NULL;
> + } else {
> + if (!(addr =
getScsiHostParentAddress(adapter.data.scsi_host.parent)))
> + return NULL;
> +
> + name = virParseStableScsiHostAddress(NULL, addr,
> +
adapter.data.scsi_host.unique_id);
You need a VIR_FREE(addr); here
John
> + }
> + } else if (adapter.data.scsi_host.name) {
> + if (VIR_STRDUP(name, adapter.data.scsi_host.name) < 0)
> + return NULL;
> + }
> +
> return name;
> }
>
>