dacca04c | 07-Apr-2016 |
Paolo Bonzini <pbonzini@redhat.com> |
nbd: do not hang nbd_wr_syncv if outside a coroutine and no available data
Until commit 1c778ef7 ("nbd: convert to using I/O channels for actual socket I/O", 2016-02-16), nbd_wr_sync returned -EAGAI
nbd: do not hang nbd_wr_syncv if outside a coroutine and no available data
Until commit 1c778ef7 ("nbd: convert to using I/O channels for actual socket I/O", 2016-02-16), nbd_wr_sync returned -EAGAIN this scenario. nbd_reply_ready required these semantics because it has two conflicting requirements:
1) if a reply can be received on the socket, nbd_reply_ready needs to read the header outside coroutine context to identify _which_ coroutine to enter to process the rest of the reply
2) on the other hand, nbd_reply_ready can find a false positive if another thread (e.g. a VCPU thread running aio_poll) sneaks in and calls nbd_reply_ready too. In this case nbd_reply_ready does nothing and expects nbd_wr_syncv to return -EAGAIN.
Currently, the solution to the first requirement is to wait in the very rare case of a read() that doesn't retrieve the reply header in its entirety; this is what nbd_wr_syncv does by calling qio_channel_wait(). However, the unconditional call to qio_channel_wait() breaks the second requirement. To fix this, the patch makes nbd_wr_syncv return -EAGAIN if done is zero, similar to the code before commit 1c778ef7.
This is okay because NBD client-side negotiation is the only other case that calls nbd_wr_syncv outside a coroutine, and it places the socket in blocking mode. On the other hand, it is a bit unpleasant to put this in nbd_wr_syncv(), because the function is used by both client and server.
The full fix would be to add a counter to NbdClientSession for how many bytes have been filled in s->reply. Then a reply can be filled by multiple separate invocations of nbd_reply_ready and the qio_channel_wait() call can be removed completely. Something to consider for 2.7...
Reported-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com> Reviewed-by: Daniel P. Berrange <berrange@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
show more ...
|
156f6a10 | 06-Apr-2016 |
Eric Blake <eblake@redhat.com> |
nbd: Don't kill server when client requests unknown option
nbd-server.c currently fails to handle unsupported options properly. If during option haggling the client sends an unknown request, the ser
nbd: Don't kill server when client requests unknown option
nbd-server.c currently fails to handle unsupported options properly. If during option haggling the client sends an unknown request, the server kills the connection instead of letting the client try to fall back to something older. This is precisely what advertising NBD_FLAG_FIXED_NEWSTYLE was supposed to fix.
Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1459982918-32229-1-git-send-email-eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
show more ...
|
6ff58164 | 06-Apr-2016 |
Alex Bligh <alex@alex.org.uk> |
nbd: Fix NBD unsupported options
nbd-client.c currently fails to handle unsupported options properly. If during option haggling the server finds an option that is unsupported, it returns an NBD_REP_
nbd: Fix NBD unsupported options
nbd-client.c currently fails to handle unsupported options properly. If during option haggling the server finds an option that is unsupported, it returns an NBD_REP_ERR_UNSUP reply.
According to nbd's proto.md, the format for such a reply should be:
S: 64 bits, 0x3e889045565a9 (magic number for replies) S: 32 bits, the option as sent by the client to which this is a reply S: 32 bits, reply type (e.g., NBD_REP_ACK for successful completion, or NBD_REP_ERR_UNSUP to mark use of an option not known by this server S: 32 bits, length of the reply. This may be zero for some replies, in which case the next field is not sent S: any data as required by the reply (e.g., an export name in the case of NBD_REP_SERVER, or optional UTF-8 message for NBD_REP_ERR_*)
However, in nbd-client.c, the reply type was being read, and if it contained an error, it was bailing out and issuing the next option request without first reading the length. This meant that the next option / handshake read had an extra 4 or more bytes of data in it. In practice, this makes Qemu incompatible with servers that do not support NBD_OPT_LIST.
To verify this isn't an error in the specification or my reading of it, replies are sent by the reference implementation here: https://github.com/yoe/nbd/blob/66dfb35/nbd-server.c#L1232 and as is evident it always sends a 'datasize' (aka length) 32 bit word. Unsupported elements are replied to here: https://github.com/yoe/nbd/blob/66dfb35/nbd-server.c#L1371
Signed-off-by: Alex Bligh <alex@alex.org.uk> Message-Id: <1459882500-24316-1-git-send-email-alex@alex.org.uk> [rework to ALWAYS consume an optional UTF-8 message from the server] Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1459961962-18771-1-git-send-email-eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
show more ...
|
7548fe31 | 06-Apr-2016 |
Eric Blake <eblake@redhat.com> |
nbd: Improve debug traces on little-endian
Print debug tracing messages while data is still in native ordering, rather than after we've potentially swapped it into network order for transmission. A
nbd: Improve debug traces on little-endian
Print debug tracing messages while data is still in native ordering, rather than after we've potentially swapped it into network order for transmission. Also, it's nice if the server mentions what it is replying, to correlate it to with what the client says it is receiving.
Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1459913704-19949-4-git-send-email-eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
show more ...
|
8c659712 | 06-Apr-2016 |
Eric Blake <eblake@redhat.com> |
nbd: Avoid bitrot in TRACE() usage
The compiler is smart enough to optimize out 'if (0)', but won't type-check our printfs if they are hidden behind #if.
Signed-off-by: Eric Blake <eblake@redhat.co
nbd: Avoid bitrot in TRACE() usage
The compiler is smart enough to optimize out 'if (0)', but won't type-check our printfs if they are hidden behind #if.
Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1459913704-19949-3-git-send-email-eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
show more ...
|
69b49502 | 10-Feb-2016 |
Daniel P. Berrange <berrange@redhat.com> |
nbd: use "" as a default export name if none provided
If the user does not provide an export name and the server is running the new style protocol, where export names are mandatory, use "" as the de
nbd: use "" as a default export name if none provided
If the user does not provide an export name and the server is running the new style protocol, where export names are mandatory, use "" as the default export name if the user has not specified any. "" is defined in the NBD protocol as the default name to use in such scenarios.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> Message-Id: <1455129674-17255-13-git-send-email-berrange@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
show more ...
|
9344e5f5 | 10-Feb-2016 |
Daniel P. Berrange <berrange@redhat.com> |
nbd: always query export list in fixed new style protocol
With the new style protocol, the NBD client will currenetly send NBD_OPT_EXPORT_NAME as the first (and indeed only) option it wants. The pro
nbd: always query export list in fixed new style protocol
With the new style protocol, the NBD client will currenetly send NBD_OPT_EXPORT_NAME as the first (and indeed only) option it wants. The problem is that the NBD protocol spec does not allow for returning an error message with the NBD_OPT_EXPORT_NAME option. So if the server mandates use of TLS, the client will simply see an immediate connection close after issuing NBD_OPT_EXPORT_NAME which is not user friendly.
To improve this situation, if we have the fixed new style protocol, we can sent NBD_OPT_LIST as the first option to query the list of server exports. We can check for our named export in this list and raise an error if it is not found, instead of going ahead and sending NBD_OPT_EXPORT_NAME with a name that we know will be rejected.
This improves the error reporting both in the case that the server required TLS, and in the case that the client requested export name does not exist on the server.
If the server does not support NBD_OPT_LIST, we just ignore that and carry on with NBD_OPT_EXPORT_NAME as before.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> Message-Id: <1455129674-17255-12-git-send-email-berrange@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
show more ...
|
e2a9d9a3 | 10-Feb-2016 |
Daniel P. Berrange <berrange@redhat.com> |
nbd: make client request fixed new style if advertised
If the server advertises support for the fixed new style negotiation, the client should in turn enable new style. This will allow the client to
nbd: make client request fixed new style if advertised
If the server advertises support for the fixed new style negotiation, the client should in turn enable new style. This will allow the client to negotiate further NBD options besides the export name.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> Message-Id: <1455129674-17255-10-git-send-email-berrange@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
show more ...
|
26afa868 | 10-Feb-2016 |
Daniel P. Berrange <berrange@redhat.com> |
nbd: make server compliant with fixed newstyle spec
If the client does not request the fixed new style protocol, then we should only accept NBD_OPT_EXPORT_NAME. All other options are only valid when
nbd: make server compliant with fixed newstyle spec
If the client does not request the fixed new style protocol, then we should only accept NBD_OPT_EXPORT_NAME. All other options are only valid when fixed new style has been activated.
The qemu-nbd client doesn't currently request fixed new style protocol, but this change won't break qemu-nbd, because it fortunately only ever uses NBD_OPT_EXPORT_NAME, so was never triggering the non-compliant server behaviour.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> Message-Id: <1455129674-17255-9-git-send-email-berrange@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
show more ...
|
f72d705f | 10-Feb-2016 |
Daniel P. Berrange <berrange@redhat.com> |
nbd: invert client logic for negotiating protocol version
The nbd_receive_negotiate() method takes different code paths based on whether 'name == NULL', and then checks the expected protocol version
nbd: invert client logic for negotiating protocol version
The nbd_receive_negotiate() method takes different code paths based on whether 'name == NULL', and then checks the expected protocol version in each branch.
This patch inverts the logic, so that it takes different code paths based on what protocol version it receives and then checks if name is NULL or not as needed.
This facilitates later code which allows the client to be capable of using the new style protocol regardless of whether an export name is listed or not.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> Message-Id: <1455129674-17255-8-git-send-email-berrange@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
show more ...
|
f1c17521 | 07-Jan-2016 |
Paolo Bonzini <pbonzini@redhat.com> |
nbd-server: do not exit on failed memory allocation
The amount of memory allocated in nbd_co_receive_request is driven by the NBD client (possibly a virtual machine). Parallel I/O can cause the ser
nbd-server: do not exit on failed memory allocation
The amount of memory allocated in nbd_co_receive_request is driven by the NBD client (possibly a virtual machine). Parallel I/O can cause the server to allocate a large amount of memory; check for failures and return ENOMEM in that case.
Cc: qemu-block@nongnu.org Reviewed-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
show more ...
|
eb38c3b6 | 07-Jan-2016 |
Paolo Bonzini <pbonzini@redhat.com> |
nbd-server: do not check request length except for reads and writes
Only reads and writes need to allocate memory correspondent to the request length. Other requests can be sent to the storage with
nbd-server: do not check request length except for reads and writes
Only reads and writes need to allocate memory correspondent to the request length. Other requests can be sent to the storage without allocating any memory, and thus any request length is acceptable.
Reported-by: Sitsofe Wheeler <sitsofe@yahoo.com> Cc: qemu-block@nongnu.org Reviewed-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
show more ...
|