Re: [libvirt] [PATCH v3 3/4] scsi: Adjust return value for virStorageBackendSCSINewLun

On 04/16/2015 09:14 AM, Ján Tomko wrote: > On Tue, Apr 07, 2015 at 04:21:02PM -0400, John Ferlan wrote: >> Use virStorageBackendPoolUseDevPath API to determine whether creation of >> stable target path is possible for the volume. If not, then return failure. >> >> This will differentiate a failed virStorageBackendStablePath which won't >> need to be fatal. Thus, we'll add a -2 return value to differentiate that >> the failure was a result of either the inability to find the symlink for >> the device or failure to open the target path directory >> >> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >> --- >> src/storage/storage_backend_scsi.c | 27 ++++++++++++++++++++++++--- >> 1 file changed, 24 insertions(+), 3 deletions(-) >> >> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c >> index b96caec..d3c6470 100644 >> --- a/src/storage/storage_backend_scsi.c >> +++ b/src/storage/storage_backend_scsi.c >> @@ -146,6 +146,16 @@ virStorageBackendSCSISerial(const char *dev) >> } >> >> >> +/* >> + * Attempt to create a new LUN >> + * >> + * Returns: >> + * >> + * 0 => Success >> + * -1 => Failure due to some sort of OOM or other fatal issue found when >> + * attempting to get/update information about a found volume >> + * -2 => Failure to find a stable path, not fatal, caller can try another >> + */ >> static int >> virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, >> uint32_t host ATTRIBUTE_UNUSED, >> @@ -158,6 +168,18 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, >> char *devpath = NULL; >> int retval = -1; >> >> + /* Before we get too far - let's see if the pool is using target path >> + * starting with /dev. Attempts to find a stable path not based on a >> + * pool target starting with /dev will fail and do lots of unnecessary >> + * work - so we'll short circuit here. > > This also rejects the non-stable path '/dev' that was accepted before. > Not quite sure I see the issue - can you be more specific? Am I missing something obvious? Previously if "pool->def->target.path" is "/dev" (or NULL or "/dev/" or didn't start with "/dev"), then we'd return a strdup'd 'devpath' into 'vol->target.path'. I see no way 'devpath' could be '/dev' or '/dev/' since it is formatted as : if (virAsprintf(&devpath, "/dev/%s", dev) < 0) goto cleanup;

where 'dev' is an argument to this function as found by processLU and cannot be NULL from a call: if (getBlockDevice(host, bus, target, lun, &block_device) < 0) { VIR_DEBUG("Failed to find block device for this LUN"); goto out; which fills 'block_device' from either getNewStyleBlockDevice or getOldStyleBlockDevice. John -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list