[libvirt] [PATCH 3/3] scsi: Check for invalid target.path after processLU failure

[John Ferlan]

On 03/31/2015 07:57 AM, Ján Tomko wrote:
> On Mon, Mar 30, 2015 at 07:16:34PM -0400, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1171933
>>
>> After processing all the LU's and find no "real" LU's - check if perhaps
>> the cause of that was a poorly formed 'target.path'. The expection is
>> generally that the path is /dev/disk/by-path or at least something in /dev.
>> Although it's not impossible for the user to provide something. If they
>> do provide something it should be valid or we should just cause a failure
>> to start the pool with an appropriate message.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/storage/storage_backend_scsi.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
>> index 2f1f5ed..c3a1892 100644
>> --- a/src/storage/storage_backend_scsi.c
>> +++ b/src/storage/storage_backend_scsi.c
>> @@ -467,6 +467,15 @@ virStorageBackendSCSIFindLUsInternal(virStoragePoolObjPtr pool,
>>      if (!*found)
>>          VIR_DEBUG("No LU found for pool %s", pool->def->name);
>>  
>> +    if (!*found && !virFileExists(pool->def->target.path) &&
>> +        !STRPREFIX(pool->def->target.path, "/dev")) {
> 
> Checking for *found here seems pointless. After the logic change in the
> previous patch, it is implied by either of the following conditions:
> 
> a) If the target path does not start with "/dev", *found will be false:
> virStorageBackendStablePath will short-circuit, just strduping
> the volume path, and virStorageBackendSCSINewLun will return -1
> in that case.
> 
> b) If the target path does not exist, it will either be rejected by the
> above code path, or by the failed opendir in
> virStorageBackendStablePath.
> 
> 
> If all you want to do is forbid to start an {,i}SCSI pool that does not
> start with /dev, we can do that much earlier in {,I}SCSIStartPool.
> 

The goal was to not start one that has a non-existent pool target.path,
but where/how that's determined is a little bit more involved than other
pools which could use virFileExists() on the pool's target.path in a
Check or Refresh and decide that it's not possible to start the pool.
For iscsi, that path creation is not "managed" by libvirt, hence the
wait loop in virStorageBackendStablePath.

I suppose I could check in start/check that if the target.path doesn't
start with /dev[/], then do a virFileExists on the provided path. If
that path doesn't exist, then fail the startup.  That would "solve" the
bug without messing with processLU's return values.

> To catch wrong paths in /dev, I think the proper way is to stop ignoring
> the return value of processLU and make it return -1 on fatal errors
> (OOM, pool target path does not exist, etc.) and -2 on errors that won't
> necessarily stop us from finding other volumes.
> 

Having processLU return 1 had more to do with distinguishing the
difference between a non disk/lun and a finding a disk/lun. What I found
was for iSCSI "found = true" was being set because of the
/sys/bus/scsi/devices/<id>/type file had 0xC (or 12 or a controller)
(<id> is the host:bus:target:lun).

The concern over wrong paths or something that doesn't start with
/dev[/] and having it be a failure is there has to be a reason a non
/dev[/] path was allowed and if we return -1 just because of that I'm
unclear what effect that has since the code is shared between scsi and
iscsi... I do agree that other failures in virStorageBackendSCSINewLun
should be differentiated.

I've made some adjustments and will repost shortly

John