summaryrefslogtreecommitdiffstats
path: root/nbd
Commit message (Collapse)AuthorAgeFilesLines
* nbd/client: fix nbd_send_request to return intVladimir Sementsov-Ogievskiy2017-08-301-1/+1
| | | | | | | | | | Fix nbd_send_request to return int, as it returns a return value of nbd_write (which is int), and the only user of nbd_send_request's return value (nbd_co_send_request) consider it as int too. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20170804151440.320927-5-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com>
* nbd/client: refactor nbd_receive_replyVladimir Sementsov-Ogievskiy2017-08-301-3/+9
| | | | | | | | | | | Refactor nbd_receive_reply to return 1 on success, 0 on eof, when no data was read and <0 for other cases, because returned size of read data is not actually used. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20170804151440.320927-4-vsementsov@virtuozzo.com> [eblake: tweak function comments] Signed-off-by: Eric Blake <eblake@redhat.com>
* nbd/client: refactor nbd_read_eofVladimir Sementsov-Ogievskiy2017-08-302-14/+24
| | | | | | | | | | | Refactor nbd_read_eof to return 1 on success, 0 on eof, when no data was read and <0 for other cases, because returned size of read data is not actually used. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20170804151440.320927-3-vsementsov@virtuozzo.com> [eblake: tweak function comments, rebase to test 083 enhancements] Signed-off-by: Eric Blake <eblake@redhat.com>
* nbd/client: fix nbd_opt_goVladimir Sementsov-Ogievskiy2017-08-301-2/+0Star
| | | | | | | | | | Do not send NBD_OPT_ABORT to the broken server. After sending NBD_REP_ACK on NBD_OPT_GO server is most probably in transmission phase, when option sending is finished. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20170804151440.320927-2-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com>
* nbd: Fix order of bdrv_set_perm and bdrv_invalidate_cacheKevin Wolf2017-08-151-9/+11
| | | | | | | | | | | The "inactive" state of BDS affects whether the permissions can be granted, we must call bdrv_invalidate_cache before bdrv_set_perm to support "-incoming defer" case. Reported-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Fam Zheng <famz@redhat.com> Message-Id: <20170815130740.31229-3-famz@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com>
* nbd: Fix trace message for disconnectEric Blake2017-08-151-1/+1
| | | | | | | | NBD_CMD_DISC is a disconnect request, not a data discard request. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20170811015749.20365-1-eblake@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
* trace-events: fix code style: print 0x before hex numbersVladimir Sementsov-Ogievskiy2017-08-011-9/+9
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | The only exception are groups of numers separated by symbols '.', ' ', ':', '/', like 'ab.09.7d'. This patch is made by the following: > find . -name trace-events | xargs python script.py where script.py is the following python script: ========================= #!/usr/bin/env python import sys import re import fileinput rhex = '%[-+ *.0-9]*(?:[hljztL]|ll|hh)?(?:x|X|"\s*PRI[xX][^"]*"?)' rgroup = re.compile('((?:' + rhex + '[.:/ ])+' + rhex + ')') rbad = re.compile('(?<!0x)' + rhex) files = sys.argv[1:] for fname in files: for line in fileinput.input(fname, inplace=True): arr = re.split(rgroup, line) for i in range(0, len(arr), 2): arr[i] = re.sub(rbad, '0x\g<0>', arr[i]) sys.stdout.write(''.join(arr)) ========================= Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Acked-by: Cornelia Huck <cohuck@redhat.com> Message-id: 20170731160135.12101-5-vsementsov@virtuozzo.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
* nbd: fix memory leak in nbd_opt_go()Philippe Mathieu-Daudé2017-07-281-3/+5
| | | | | | | | | | | nbd/client.c:385:12: warning: Potential leak of memory pointed to by 'buf' Reported-by: Clang Static Analyzer Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Reviewed-by: Eric Blake <eblake@redhat.com> Message-Id: <20170727024224.22900-5-f4bug@amsat.org> [introduced in commit 8ecaeae8] Signed-off-by: Eric Blake <eblake@redhat.com>
* nbd: Fix server reply to NBD_OPT_EXPORT_NAME of older clientsEric Blake2017-07-182-10/+16
| | | | | | | | | | | | | | | | | | | | | | | | | A typo in commit 23e099c set the size of buf[] used in response to NBD_OPT_EXPORT_NAME according to the length needed for old-style negotiation (4 bytes of flag information) instead of the intended 2 bytes used in new style. If the client doesn't enable NBD_FLAG_C_NO_ZEROES, then the server sends two bytes too many, and is then out of sync in response to the client's next command (the bug is masked when modern qemu is the client, since we enable the no zeroes flag). While touching this code, add some more defines to nbd_internal.h rather than having quite so many magic numbers in the .c; also, use "" initialization rather than memset(), and tweak the oldstyle negotiation to better match the spec description of the layout (since the spec is big-endian, skipping two bytes as 0 followed by writing a 2-byte flag is the same as writing a zero-extended 4-byte flag), to make it a bit easier to follow compared to the spec. [checkpatch.pl has some false positives in the comments] Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20170717192635.17880-3-eblake@redhat.com> Reviewed-by: John Snow <jsnow@redhat.com>
* nbd: Trace client command being sentEric Blake2017-07-182-2/+3
| | | | | | | | | Make the client trace slightly more legible by including the name of the command being sent. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: John Snow <jsnow@redhat.com> Message-Id: <20170717192635.17880-2-eblake@redhat.com>
* nbd: Fix iotests failure due to changed client error messageEric Blake2017-07-171-3/+2Star
| | | | | | | | | | | | | | | | | | | | | | | | | | | | Commit 8ecaeae8 changed the way the client requests an NBD export, and in the process also changed the resulting error message when the export is not present, breaking a couple of iotests. The error message is now directly given by the server (a failed NBD_OPT_GO) instead of implied by the client (after exhausting NBD_OPT_LIST), but looking at the testsuite changes, it proves worthwhile to reword the error message to be slightly less verbose (as this is one particular error message likely to be hit by a user). Note that the error message is now sensitive to which binary is running the server as well as the client (since the expected output is replaying a message received from the server - for that matter, it depends on a server new enough to understand NBD_OPT_GO); in general iotests are run on client and server from the same source code base so the default setup will pass; but if it proves problematic for people overriding QEMU_PROG, QEMU_IMG_PROG, QEMU_IO_PROG, and QEMU_NBD_PROG to point across multiple builds for cross-version integration testing, we may have to later tweak or sanitize the output somehow. Reported-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20170717142310.17048-1-eblake@redhat.com> Tested-by: John Snow <jsnow@redhat.com> Reviewed-by: John Snow <jsnow@redhat.com>
* nbd: Implement NBD_INFO_BLOCK_SIZE on clientEric Blake2017-07-142-12/+70
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | The upstream NBD Protocol has defined a new extension to allow the server to advertise block sizes to the client, as well as a way for the client to inform the server whether it intends to obey block sizes. When using the block layer as the client, we will obey block sizes; but when used as 'qemu-nbd -c' to hand off to the kernel nbd module as the client, we are still waiting for the kernel to implement a way for us to learn if it will honor block sizes (perhaps by an addition to sysfs, rather than an ioctl), as well as any way to tell the kernel what additional block sizes to obey (NBD_SET_BLKSIZE appears to be accurate for the minimum size, but preferred and maximum sizes would probably be new ioctl()s), so until then, we need to make our request for block sizes conditional. When using ioctl(NBD_SET_BLKSIZE) to hand off to the kernel, use the minimum block size as the sector size if it is larger than 512, which also has the nice effect of cooperating with (non-qemu) servers that don't do read-modify-write when exposing a block device with 4k sectors; it might also allow us to visit a file larger than 2T on a 32-bit kernel. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20170707203049.534-10-eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
* nbd: Implement NBD_INFO_BLOCK_SIZE on serverEric Blake2017-07-142-4/+45
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | The upstream NBD Protocol has defined a new extension to allow the server to advertise block sizes to the client, as well as a way for the client to inform the server that it intends to obey block sizes. Thanks to a recent fix (commit df7b97ff), our real minimum transfer size is always 1 (the block layer takes care of read-modify-write on our behalf), but we're still more efficient if we advertise 512 when the client supports it, as follows: - OPT_INFO, but no NBD_INFO_BLOCK_SIZE: advertise 512, then fail with NBD_REP_ERR_BLOCK_SIZE_REQD; client is free to try something else since we don't disconnect - OPT_INFO with NBD_INFO_BLOCK_SIZE: advertise 512 - OPT_GO, but no NBD_INFO_BLOCK_SIZE: advertise 1 - OPT_GO with NBD_INFO_BLOCK_SIZE: advertise 512 We can also advertise the optimum block size (presumably the cluster size, when exporting a qcow2 file), and our absolute maximum transfer size of 32M, to help newer clients avoid EINVAL failures or abrupt disconnects on oversize requests. We do not reject clients for using the older NBD_OPT_EXPORT_NAME; we are no worse off for those clients than we used to be. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20170707203049.534-9-eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
* nbd: Implement NBD_OPT_GO on clientEric Blake2017-07-143-2/+130
| | | | | | | | | | | | | | | | | | | | | | | | | | NBD_OPT_EXPORT_NAME is lousy: per the NBD protocol, any failure requires the server to close the connection rather than report an error to us. Therefore, upstream NBD recently added NBD_OPT_GO as the improved version of the option that does what we want [1]: it reports sane errors on failures, and on success provides at least as much info as NBD_OPT_EXPORT_NAME. [1] https://github.com/NetworkBlockDevice/nbd/blob/extension-info/doc/proto.md This is a first cut at use of the information types. Note that we do not need to use NBD_OPT_INFO, and that use of NBD_OPT_GO means we no longer have to use NBD_OPT_LIST to learn whether a server requires TLS (this requires servers that gracefully handle unknown NBD_OPT, many servers prior to qemu 2.5 were buggy, but I have patched qemu, upstream nbd, and nbdkit in the meantime, in part because of interoperability testing with this patch). We still fall back to NBD_OPT_LIST when NBD_OPT_GO is not supported on the server, as it is still one last chance for a nicer error message. Later patches will use further info, like NBD_INFO_BLOCK_SIZE. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20170707203049.534-8-eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
* nbd: Implement NBD_OPT_GO on serverEric Blake2017-07-142-3/+179
| | | | | | | | | | | | | | | | | | | | | NBD_OPT_EXPORT_NAME is lousy: per the NBD protocol, any failure requires us to close the connection rather than report an error. Therefore, upstream NBD recently added NBD_OPT_GO as the improved version of the option that does what we want [1], along with NBD_OPT_INFO that returns the same information but does not transition to transmission phase. [1] https://github.com/NetworkBlockDevice/nbd/blob/extension-info/doc/proto.md This is a first cut at the information types, and only passes the same information already available through NBD_OPT_LIST and NBD_OPT_EXPORT_NAME; items like NBD_INFO_BLOCK_SIZE (and thus any use of NBD_REP_ERR_BLOCK_SIZE_REQD) are intentionally left for later patches. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20170707203049.534-7-eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
* nbd: Refactor reply to NBD_OPT_EXPORT_NAMEEric Blake2017-07-141-22/+28
| | | | | | | | | | | Reply directly in nbd_negotiate_handle_export_name(), rather than waiting until nbd_negotiate_options() completes. This will make it easier to implement NBD_OPT_GO. Pass additional parameters around, rather than stashing things inside NBDClient. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20170707203049.534-6-eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
* nbd: Simplify trace of client flags in negotiationEric Blake2017-07-142-7/+3Star
| | | | | | | | | Simplify the tracing of client flags in the server, and return -EINVAL instead of -EIO if we successfully read but don't like those flags. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20170707203049.534-5-eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
* nbd: Expose and debug more NBD constantsEric Blake2017-07-145-37/+147
| | | | | | | | | | | The NBD protocol has several constants defined in various extensions that we are about to implement. Expose them to the code, along with an easy way to map various constants to strings during diagnostic messages. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20170707203049.534-4-eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
* nbd: Don't bother tracing an NBD_OPT_ABORT response failureEric Blake2017-07-142-12/+4Star
| | | | | | | | | We really don't care if our spec-compliant reply to NBD_OPT_ABORT was received, so shave off some lines of code by not even tracing it. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20170707203049.534-3-eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
* nbd: Create struct for tracking export infoEric Blake2017-07-141-22/+22
| | | | | | | | | | | | | The NBD Protocol is introducing some additional information about exports, such as minimum request size and alignment, as well as an advertised maximum request size. It will be easier to feed this information back to the block layer if we gather all the information into a struct, rather than adding yet more pointer parameters during negotiation. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20170707203049.534-2-eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
* nbd: use generic trace subsystem instead of TRACE macroVladimir Sementsov-Ogievskiy2017-07-104-95/+119
| | | | | | | | | | | | | | | | | | Let NBD use the trace mechanisms already present in qemu. Now you can use the -trace optino of qemu, or the -T/--trace option of qemu-img, qemu-io, and qemu-nbd, to select nbd traces. For qemu, the QMP commands trace-event-{get,set}-state can also toggle tracing on the fly. Example: qemu-nbd --trace 'nbd_*' <image file> # enables all nbd traces Recompilation with CFLAGS=-DDEBUG_NBD is no more needed, furthermore, DEBUG_NBD macro is removed from the code. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20170707152918.23086-11-vsementsov@virtuozzo.com> [eblake: minor tweaks to a couple of traces] Signed-off-by: Eric Blake <eblake@redhat.com>
* nbd: refactor tracingVladimir Sementsov-Ogievskiy2017-07-102-24/+9Star
| | | | | | | | Reorganize traces: move, reword, add information, drop extra ones. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20170707152918.23086-10-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com>
* nbd/server: rename clientflags var in nbd_negotiate_optionsVladimir Sementsov-Ogievskiy2017-07-101-19/+19
| | | | | | | | | | Rename 'clientflags' to just 'option'. This variable has nothing to do with flags, but is a single integer representing the option requested by the client. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20170707152918.23086-9-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com>
* nbd/server: fix TRACE in nbd_negotiate_send_rep_lenVladimir Sementsov-Ogievskiy2017-07-101-1/+1
| | | | | | | | Fix wrong order of TRACE arguments. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20170707152918.23086-8-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com>
* nbd/client: refactor TRACE of NBD_MAGICVladimir Sementsov-Ogievskiy2017-07-101-9/+2Star
| | | | | | | | | We are going to switch from TRACE macro to trace points, this TRACE complicates things, this patch simplifies it. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20170707152918.23086-7-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com>
* nbd/common: nbd_tls_handshake: remove extra TRACEVladimir Sementsov-Ogievskiy2017-07-101-3/+1Star
| | | | | | | | | Error is propagated to the caller, TRACE is not needed. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-Id: <20170707152918.23086-6-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com>
* nbd/server: add errp to nbd_send_reply()Vladimir Sementsov-Ogievskiy2017-07-101-8/+9
| | | | | | | Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-Id: <20170707152918.23086-5-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com>
* nbd/server: use errp instead of LOGVladimir Sementsov-Ogievskiy2017-07-101-106/+160
| | | | | | | | Move to modern errp scheme from just LOGging errors. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20170707152918.23086-4-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com>
* nbd/server: refactor nbd_negotiateVladimir Sementsov-Ogievskiy2017-07-101-9/+3Star
| | | | | | | | | | | | Combine two successive "if (oldStyle) {...} else {...}" into one. Block "if (client->tlscreds)" under "if (oldStyle)" is unreachable, as we have "oldStyle = client->exp != NULL && !client->tlscreds;". So, delete this block. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20170707152918.23086-3-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com>
* nbd/server: nbd_negotiate: return 1 on NBD_OPT_ABORTVladimir Sementsov-Ogievskiy2017-07-101-5/+15
| | | | | | | | | | | | | Separate the case when a client sends NBD_OPT_ABORT from all other errors. It will be needed for the following patch, where errors will be reported. This particular case is not actually an error - it honestly follows the NBD protocol. Therefore it should not be reported like an error. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-Id: <20170707152918.23086-2-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com>
* nbd/server: refactor nbd_tripVladimir Sementsov-Ogievskiy2017-06-151-33/+20Star
| | | | | | | | | | | - do not use 'goto error_reply' outside a switch to jump into the middle of the switch's default case label - reduce code duplication Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-Id: <20170602150150.258222-13-vsementsov@virtuozzo.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
* nbd/server: rename rc to retVladimir Sementsov-Ogievskiy2017-06-151-19/+19
| | | | | | | | | | For consistency use 'ret' name for saving return code everywhere in the file. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-Id: <20170602150150.258222-12-vsementsov@virtuozzo.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
* nbd/server: get rid of fail: return rcVladimir Sementsov-Ogievskiy2017-06-151-16/+12Star
| | | | | | | | | | "goto fail" error handling scheme is not needed for just returning error code. Better is return it immediately. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-Id: <20170602150150.258222-11-vsementsov@virtuozzo.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
* nbd/server: nbd_negotiate: fix error pathVladimir Sementsov-Ogievskiy2017-06-151-1/+2
| | | | | | | | | | Current code will return 0 on this nbd_write fail, as rc is 0 after successful nbd_negotiate_options. Fix this. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-Id: <20170602150150.258222-10-vsementsov@virtuozzo.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
* nbd/server: remove NBDClientNewDataVladimir Sementsov-Ogievskiy2017-06-151-18/+7Star
| | | | | | | | | | | "co" field of NBDClientNewData has never been used, all the way back to its declaration in commit 1a6245a5. So let's just use client pointer instead of extra structure. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-Id: <20170602150150.258222-9-vsementsov@virtuozzo.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
* nbd/server: refactor nbd_co_receive_requestVladimir Sementsov-Ogievskiy2017-06-151-28/+13Star
| | | | | | | | | | | Move function tail, about receiving next request out of the function. Error path is simplified and nbd_co_receive_request becomes more corresponding to its name. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-Id: <20170602150150.258222-8-vsementsov@virtuozzo.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
* nbd/server: get rid of EAGAIN dead codeVladimir Sementsov-Ogievskiy2017-06-151-11/+7Star
| | | | | | | | | For now nbd_read never returns EAGAIN. So, don't handle it. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-Id: <20170602150150.258222-7-vsementsov@virtuozzo.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
* nbd/server: refactor nbd_co_send_replyVladimir Sementsov-Ogievskiy2017-06-151-4/+4
| | | | | | | | | As nbd_write never returns value > 0, we can get rid of extra ret. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-Id: <20170602150150.258222-6-vsementsov@virtuozzo.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
* nbd/server: get rid of ssize_tVladimir Sementsov-Ogievskiy2017-06-151-10/+8Star
| | | | | | | | | Now nbd_read and friends return int, so get rid of ssize_t. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-Id: <20170602150150.258222-5-vsementsov@virtuozzo.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
* nbd/server: get rid of nbd_negotiate_read and friendsVladimir Sementsov-Ogievskiy2017-06-151-85/+22Star
| | | | | | | | | | | | | | | | | | | Functions nbd_negotiate_{read,write,drop_sync} were introduced in 1a6245a5b, when nbd_rwv (was nbd_wr_sync) was working through qemu_co_sendv_recvv (the path is nbd_wr_sync -> qemu_co_{recv/send} -> qemu_co_send_recv -> qemu_co_sendv_recvv), which just yields, without setting any handlers. But starting from ff82911cd nbd_rwv (was nbd_wr_syncv) works through qio_channel_yield() which sets handlers, so watchers are redundant in nbd_negotiate_{read,write,drop_sync}, then, let's just use nbd_{read,write,drop} functions. Functions nbd_{read,write,drop} has errp parameter, which is unused in this patch. This will be fixed later. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-Id: <20170602150150.258222-4-vsementsov@virtuozzo.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
* nbd: make nbd_drop publicVladimir Sementsov-Ogievskiy2017-06-153-26/+28
| | | | | | | | | Following commit will reuse it for nbd server too. Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20170602150150.258222-3-vsementsov@virtuozzo.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
* nbd: rename read_sync and friendsVladimir Sementsov-Ogievskiy2017-06-154-46/+42Star
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Rename nbd_wr_syncv -> nbd_rwv read_sync -> nbd_read read_sync_eof -> nbd_read_eof write_sync -> nbd_write drop_sync -> nbd_drop 1. nbd_ prefix read_sync and write_sync are already shared, so it is good to have a namespace prefix. drop_sync will be shared, and read_sync_eof is related to read_sync, so let's rename them all. 2. _sync suffix _sync is related to the fact that nbd_wr_syncv doesn't return if a write to socket returns EAGAIN. The first implementation of nbd_wr_syncv (was wr_sync in 7a5ca8648b) just loops while getting EAGAIN, the current implementation yields in this case. Why we want to get rid of it: - it is normal for r/w functions to be synchronous, so having an additional suffix for it looks redundant (contrariwise, we have _aio suffix for async functions) - _sync suffix in block layer is used when function does flush (so using it for other thing is confusing a bit) - keep function names short after adding nbd_ prefix 3. for nbd_wr_syncv let's use more common notation 'rw' Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20170602150150.258222-2-vsementsov@virtuozzo.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
* nbd: Fix regression on resiliency to port scanEric Blake2017-06-151-9/+15
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Back in qemu 2.5, qemu-nbd was immune to port probes (a transient server would not quit, regardless of how many probe connections came and went, until a connection actually negotiated). But we broke that in commit ee7d7aa when removing the return value to nbd_client_new(), although that patch also introduced a bug causing an assertion failure on a client that fails negotiation. We then made it worse during refactoring in commit 1a6245a (a segfault before we could even assert); the (masked) assertion was cleaned up in d3780c2 (still in 2.6), and just recently we finally fixed the segfault ("nbd: Fully intialize client in case of failed negotiation"). But that still means that ever since we added TLS support to qemu-nbd, we have been vulnerable to an ill-timed port-scan being able to cause a denial of service by taking down qemu-nbd before a real client has a chance to connect. Since negotiation is now handled asynchronously via coroutines, we no longer have a synchronous point of return by re-adding a return value to nbd_client_new(). So this patch instead wires things up to pass the negotiation status through the close_fn callback function. Simple test across two terminals: $ qemu-nbd -f raw -p 30001 file $ nmap 127.0.0.1 -p 30001 && \ qemu-io -c 'r 0 512' -f raw nbd://localhost:30001 Note that this patch does not change what constitutes successful negotiation (thus, a client must enter transmission phase before that client can be considered as a reason to terminate the server when the connection ends). Perhaps we may want to tweak things in a later patch to also treat a client that uses NBD_OPT_ABORT as being a 'successful' negotiation (the client correctly talked the NBD protocol, and informed us it was not going to use our export after all), but that's a discussion for another day. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1451614 Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20170608222617.20376-1-eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
* nbd: Fully initialize client in case of failed negotiationEric Blake2017-06-071-5/+3Star
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | If a non-NBD client connects to qemu-nbd, we would end up with a SIGSEGV in nbd_client_put() because we were trying to unregister the client's association to the export, even though we skipped inserting the client into that list. Easy trigger in two terminals: $ qemu-nbd -p 30001 --format=raw file $ nmap 127.0.0.1 -p 30001 nmap claims that it thinks it connected to a pago-services1 server (which probably means nmap could be updated to learn the NBD protocol and give a more accurate diagnosis of the open port - but that's not our problem), then terminates immediately, so our call to nbd_negotiate() fails. The fix is to reorder nbd_co_client_start() to ensure that all initialization occurs before we ever try talking to a client in nbd_negotiate(), so that the teardown sequence on negotiation failure doesn't fault while dereferencing a half-initialized object. While debugging this, I also noticed that nbd_update_server_watch() called by nbd_client_closed() was still adding a channel to accept the next client, even when the state was no longer RUNNING. That is fixed by making nbd_can_accept() pay attention to the current state. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1451614 Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20170527030421.28366-1-eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
* nbd/client.c: use errp instead of LOGVladimir Sementsov-Ogievskiy2017-06-061-13/+17
| | | | | | | | Move to modern errp scheme from just LOGging errors. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20170526110913.89098-1-vsementsov@virtuozzo.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
* nbd: add errp to read_sync, write_sync and drop_syncVladimir Sementsov-Ogievskiy2017-06-063-50/+54
| | | | | | | | | | There a lot of calls of these functions, which already have errp, which they are filling themselves. On the other hand, nbd_wr_syncv has errp parameter too, so it would be great to connect them. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20170516094533.6160-5-vsementsov@virtuozzo.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
* nbd: add errp parameter to nbd_wr_syncv()Vladimir Sementsov-Ogievskiy2017-06-062-9/+7Star
| | | | | | | | | Will be used in following patch to provide actual error message in some cases. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20170516094533.6160-4-vsementsov@virtuozzo.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
* nbd: read_sync and friends: return 0 on successVladimir Sementsov-Ogievskiy2017-06-063-93/+88Star
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | functions read_sync, drop_sync, write_sync, and also nbd_negotiate_write, nbd_negotiate_read, nbd_negotiate_drop_sync returns number of processed bytes. But what this number can be, except requested number of bytes? Actually, underlying nbd_wr_syncv function returns a value >= 0 and != requested_bytes only on eof on read operation. So, firstly, it is impossible on write (let's add an assert) and on read it actually means, that communication is broken (except nbd_receive_reply, see below). Most of callers operate like this: if (func(..., size) != size) { /* error path */ } , i.e.: 1. They are not interested in partial success 2. Extra duplications in code (especially bad are duplications of magic numbers) 3. User doesn't see actual error message, as return code is lost. (this patch doesn't fix this point, but it makes fixing easier) Several callers handles ret >= 0 and != requested-size separately, by just returning EINVAL in this case. This patch makes read_sync and friends return EINVAL in this case, so final behavior is the same. And only one caller - nbd_receive_reply() does something not so obvious. It returns EINVAL for ret > 0 and != requested-size, like previous group, but for ret == 0 it returns 0. The only caller of nbd_receive_reply() - nbd_read_reply_entry() handles ret == 0 in the same way as ret < 0, so for now it doesn't matter. However, in following commits error path handling will be improved and we'll need to distinguish success from fail in this case too. So, this patch adds separate helper for this case - read_sync_eof. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20170516094533.6160-3-vsementsov@virtuozzo.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
* nbd: strict nbd_wr_syncvVladimir Sementsov-Ogievskiy2017-06-061-5/+6
| | | | | | | | | | | | | | nbd_wr_syncv is called either from coroutine or from client negotiation code, when socket is in blocking mode. So, -EAGAIN is impossible. Furthermore, EAGAIN is confusing, as, what to read/write again? With EAGAIN as a return code we don't know how much data is already read or written by the function, so in case of EAGAIN the whole communication is broken. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20170516094533.6160-2-vsementsov@virtuozzo.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
* nbd-client: fix handling of hungup connectionsPaolo Bonzini2017-03-271-1/+1
| | | | | | | | | | | | | | | | | | | | | After the switch to reading replies in a coroutine, nothing is reentering pending receive coroutines if the connection hangs. Move nbd_recv_coroutines_enter_all to the reply read coroutine, which is the place where hangups are detected. nbd_teardown_connection can simply wait for the reply read coroutine to detect the hangup and clean up after itself. This wouldn't be enough though because nbd_receive_reply returns 0 (rather than -EPIPE or similar) when reading from a hung connection. Fix the return value check in nbd_read_reply_entry. This fixes qemu-iotests 083. Reported-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-id: 20170314111157.14464-1-pbonzini@redhat.com Reviewed-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Max Reitz <mreitz@redhat.com>
generated by cgit v1.2.3-55-g7522 (git 2.39.0) at 2024-12-22 17:46:44 +0100