Re: [libvirt] [PATCH 5/9] nodedev: Refactor the helpers

On 14.01.2013 15:34, Osier Yang wrote: > This adds two util functions (virIsCapableFCHost and virIsCapableVport), > and rename helper check_fc_host_linux as detect_scsi_host_caps, > check_capable_vport_linux is removed, as it's abstracted to the util > function virIsCapableVport. detect_scsi_host_caps nows detect both > the fc_host and vport_ops capabilities. "stat(2)" is replaced with > "access(2)" for saving. > > * src/util/virutil.h: > - Declare virIsCapableFCHost and virIsCapableVport > * src/util/virutil.c: > - Implement virIsCapableFCHost and virIsCapableVport > * src/node_device/node_device_linux_sysfs.c: > - Remove check_capable_vport_linux > - Rename check_fc_host_linux as detect_scsi_host_caps, and refactor > it a bit to detect both fc_host and vport_os capabilities > * src/node_device/node_device_driver.h: > - Change/remove the related declarations > * src/node_device/node_device_udev.c: (Use detect_scsi_host_caps) > * src/node_device/node_device_hal.c: (Likewise) > * src/node_device/node_device_driver.c (Likewise) > --- > src/libvirt_private.syms | 2 + > src/node_device/node_device_driver.c | 7 +- > src/node_device/node_device_driver.h | 10 +-- > src/node_device/node_device_hal.c | 4 +- > src/node_device/node_device_linux_sysfs.c | 135 ++++++++--------------------- > src/node_device/node_device_udev.c | 3 +- > src/util/virutil.c | 73 ++++++++++++++++ > src/util/virutil.h | 3 + > 8 files changed, 123 insertions(+), 114 deletions(-) > > diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c > index 742a0bc..054afea 100644 > --- a/src/node_device/node_device_linux_sysfs.c > +++ b/src/node_device/node_device_linux_sysfs.c > @@ -1,8 +1,8 @@ > /* > - * node_device_hal_linuc.c: Linux specific code to gather device data > + * node_device_linux_sysfs.c: Linux specific code to gather device data > * not available through HAL. > * > - * Copyright (C) 2009-2011 Red Hat, Inc. > + * Copyright (C) 2009-2013 Red Hat, Inc. > * > * This library is free software; you can redistribute it and/or > * modify it under the terms of the GNU Lesser General Public > @@ -37,112 +37,53 @@ > > #ifdef __linux__ > > -int check_fc_host_linux(union _virNodeDevCapData *d) > +int > +detect_scsi_host_caps_linux(union _virNodeDevCapData *d) > { > - char *sysfs_path = NULL; > - int retval = 0; > - struct stat st; > + int ret = -1; > > VIR_DEBUG("Checking if host%d is an FC HBA", d->scsi_host.host); > > - if (virAsprintf(&sysfs_path, "%shost%d", > - LINUX_SYSFS_FC_HOST_PREFIX, > - d->scsi_host.host)< 0) { > - virReportOOMError(); > - retval = -1; > - goto out; > + if (virIsCapableFCHost(NULL, d->scsi_host.host)) { > + d->scsi_host.flags |= VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST; > + > + if (virReadFCHost(NULL, > + d->scsi_host.host, > + "port_name", > +&d->scsi_host.wwpn) == -1) { > + VIR_ERROR(_("Failed to read WWPN for host%d"), d->scsi_host.host); > + goto cleanup; > + } > + > + if (virReadFCHost(NULL, > + d->scsi_host.host, > + "node_name", > +&d->scsi_host.wwnn) == -1) { > + VIR_ERROR(_("Failed to read WWNN for host%d"), d->scsi_host.host); > + goto cleanup; > + } > + > + if (virReadFCHost(NULL, > + d->scsi_host.host, > + "fabric_name", > +&d->scsi_host.fabric_wwn) == -1) { > + VIR_ERROR(_("Failed to read fabric WWN for host%d"), > + d->scsi_host.host); > + goto cleanup; > + } > } > > - if (stat(sysfs_path,&st) != 0) { > - /* Not an FC HBA; not an error, either. */ > - goto out; > - } > - > - d->scsi_host.flags |= VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST; > - > - if (virReadFCHost(NULL, > - d->scsi_host.host, > - "port_name", > -&d->scsi_host.wwpn) == -1) { > - VIR_ERROR(_("Failed to read WWPN for host%d"), > - d->scsi_host.host); > - retval = -1; > - goto out; > - } > - > - if (virReadFCHost(NULL, > - d->scsi_host.host, > - "node_name", > -&d->scsi_host.wwnn) == -1) { > - VIR_ERROR(_("Failed to read WWNN for host%d"), > - d->scsi_host.host); > - retval = -1; > - } > - > - if (virReadFCHost(NULL, > - d->scsi_host.host, > - "fabric_name", > -&d->scsi_host.fabric_wwn) == -1) { > - VIR_ERROR(_("Failed to read fabric WWN for host%d"), > - d->scsi_host.host); > - retval = -1; > - goto out; > - } > + if (virIsCapableVport(NULL, d->scsi_host.host) == 0) > + d->scsi_host.flags |= VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS; > > -out: > - if (retval == -1) { > + ret = 0; > +cleanup: > + if (ret == -1) { I'd write this as 'if (ret< 0) {' so we don't have to change this line if we ever invent a new error code for this function. Moreover, it's common practice over libvirt code.

ACK if you fix this. Michal