[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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?

Jan

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]