Re: [libvirt] [PATCH] iscsi: do not fail to stop a stopped pool

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?