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

[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.

> 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.

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?

Paolo