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?
Jan
signature.asc
Description: Digital signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
