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

Martin Kletzander mkletzan at redhat.com
Wed Apr 29 15:38:56 UTC 2015


On Wed, Apr 29, 2015 at 04:20:14PM +0200, Ján Tomko wrote:
>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?
>

One (the number of issues this fixes) is WAY greater than zero (the
number of issues I, personally have with this patch).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150429/22a1dcc9/attachment-0001.sig>


More information about the libvir-list mailing list