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@xxxxxxxxxx> >> --- >> 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list