Re: [Qemu-devel] [PATCH] block/iscsi: add support for request timeouts

[Peter Lieven]

Am 25.06.2015 um 23:08 schrieb Paolo Bonzini:
>
> On 16/06/2015 13:45, Peter Lieven wrote:
>> libiscsi starting with 1.15 will properly support timeout of iscsi
>> commands. The default will remain no timeout, but this can
>> be changed via cmdline parameters, e.g.:
>>
>> qemu -iscsi timeout=30 -drive file=iscsi://...
>>
>> If a timeout occurs a reconnect is scheduled and the timed out command
>> will be requeued for processing after a successful reconnect.
>>
>> The required API call iscsi_set_timeout is present since libiscsi
>> 1.10 which was released in October 2013. However, due to some bugs
>> in the libiscsi code the use is not recommended before version 1.15.
> If so, QEMU should not allow it if libiscsi is older than 1.15.

Not accept a timeout parameter or ignore it and print a warning?

>
>> Please note that this patch bumps the libiscsi requirement to 1.10
>> to have all function and macros defined.
> This is not acceptable, unfortunately.  I explained this two months ago
> (https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg01847.html)
> and it is still true.

Sorry, i missed that. Can you verify if 1.15.0 has a soname that
makes it possible to jump to 1.15.0 at some point in the future?

>
> libiscsi keeps breaking ABI compatibility and for a while did not even
> bump the soname when they do.  This makes it completely impossible for
> distros to upgrade to a newer libiscsi, and RHEL7 is thus stuck with 1.9.
>
> Yes, it is 2 years old.  It doesn't matter.  If libiscsi upstream only
> _tried_ to preserve ABI compatibility, they wouldn't be in this
> situation.  And I know that it is not even trying, because it broke
> again sometime between 1.11 and 1.14 for a totally trivial reason:
>
> --- a/iscsi/iscsi.h
> +++ b/iscsi/iscsi.h
> @@ -91,6 +136,8 @@ struct iscsi_url {
>         char target[MAX_STRING_SIZE + 1];
>         char user[MAX_STRING_SIZE + 1];
>         char passwd[MAX_STRING_SIZE + 1];
> +       char target_user[MAX_STRING_SIZE + 1];
> +       char target_passwd[MAX_STRING_SIZE + 1];
>         int lun;
>         struct iscsi_context *iscsi;
>  };
>
>
> This is the only change between these releases that breaks the ABI, but
> it is already one too much. :(
>
> (Also, the parsing of URLs into iscsi_url doesn't even try to obey the
> RFCs...).
>
>> The patch fixes also a
>> off-by-one error in the NOP timeout calculation which was fixed
>> while touching these code parts.
> Can you please separate this part anyway?

Sure.

I will send a v2 that is compatible with 1.9.0 and enables the timeout
stuff only for libiscsi >= 1.15.0

Peter