Just as we allow stopping filesystem pools when they were unmounted externally, do not fail to stop an iscsi pool when someone else closed the session externally. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1171984 --- src/storage/storage_backend_iscsi.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 197d333..bea6758 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -449,8 +449,13 @@ virStorageBackendISCSIStopPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool) { char *portal; + char *session; int ret = -1; + if ((session = virStorageBackendISCSISession(pool, false)) == NULL)

+ return 0; + VIR_FREE(session); +

if ((portal = virStorageBackendISCSIPortal(&pool->def->source)) == NULL) return -1; -- 2.0.5 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, Apr 29, 2015 at 03:08:14PM +0200, Ján Tomko wrote: >Just as we allow stopping filesystem pools when they were unmounted >externally, do not fail to stop an iscsi pool when someone else >closed the session externally. > >Resolves: >https://bugzilla.redhat.com/show_bug.cgi?id=1171984 >--- > src/storage/storage_backend_iscsi.c | 5 +++++ > 1 file changed, 5 insertions(+) > >diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c >index 197d333..bea6758 100644 >--- a/src/storage/storage_backend_iscsi.c >+++ b/src/storage/storage_backend_iscsi.c >@@ -449,8 +449,13 @@ virStorageBackendISCSIStopPool(virConnectPtr conn ATTRIBUTE_UNUSED, > virStoragePoolObjPtr pool) > { > char *portal; >+ char *session; > int ret = -1; > >+ if ((session = virStorageBackendISCSISession(pool, false)) == NULL) Shouldn't this be called with true and not false, so it doesn't report an error?

>+ return 0; >+ VIR_FREE(session); >+ Will this help that much, since it can be stopped right after the previous command found it?

Wouldn't it be better to handle this in virISCSIConnection() or virISCSIConnectionLogout() somehow?

I'm just asking because I'm not the proper one to review iscsi stuff, otherwise I'd probably ACK'd it (if there was that "true"), since it will fix more than it will break (1 >> 0).

On Wed, Apr 29, 2015 at 03:38:49PM +0200, Martin Kletzander wrote: > On Wed, Apr 29, 2015 at 03:08:14PM +0200, Ján Tomko wrote: > >Just as we allow stopping filesystem pools when they were unmounted > >externally, do not fail to stop an iscsi pool when someone else > >closed the session externally. > > > >Resolves: > >https://bugzilla.redhat.com/show_bug.cgi?id=1171984 > >--- > > src/storage/storage_backend_iscsi.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > >diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c > >index 197d333..bea6758 100644 > >--- a/src/storage/storage_backend_iscsi.c > >+++ b/src/storage/storage_backend_iscsi.c > >@@ -449,8 +449,13 @@ virStorageBackendISCSIStopPool(virConnectPtr conn ATTRIBUTE_UNUSED, > > virStoragePoolObjPtr pool) > > { > > char *portal; > >+ char *session; > > int ret = -1; > > > >+ if ((session = virStorageBackendISCSISession(pool, false)) == NULL) > > Shouldn't this be called with true and not false, so it doesn't report > an error? > Yes, not reporting an error when we return 0 makes sense. > >+ return 0; > >+ VIR_FREE(session); > >+ > > Will this help that much, since it can be stopped right after the > previous command found it? There still is a race, but the window is much shorter. Also, if the above command found it, but it was stopped by the time we got to Logout, after this patch a subsequent StopPool call should succeed. Before, a libvirtd restart was needed. > Wouldn't it be better to handle this in > virISCSIConnection() or virISCSIConnectionLogout() somehow? I think this error should be fatal for everything except ConnectionLogout. We could check the error code returned by isciadm (from open-iscsi's iscsi_err.h): ISCSI_ERR_NO_OBJS_FOUND = 21, But this error code is also returned on other errors, like the unability to resolve the hostname. Returning 0 in that case would feel wrong, if we leave the stale session on the host. > > I'm just asking because I'm not the proper one to review iscsi stuff, > otherwise I'd probably ACK'd it (if there was that "true"), since it > will fix more than it will break (1 >> 0). 1 >> 0?