* [Qemu-devel] [PATCH v5 1/9] nbd: Create struct for tracking export info
2017-07-07 20:30 [Qemu-devel] [PATCH v5 0/9] Implement NBD_OPT_GO, block size advertisement Eric Blake
@ 2017-07-07 20:30 ` Eric Blake
2017-07-13 12:09 ` Vladimir Sementsov-Ogievskiy
2017-07-07 20:30 ` [Qemu-devel] [PATCH v5 2/9] nbd: Don't bother tracing an NBD_OPT_ABORT response failure Eric Blake
` (8 subsequent siblings)
9 siblings, 1 reply; 28+ messages in thread
From: Eric Blake @ 2017-07-07 20:30 UTC (permalink / raw)
To: qemu-devel
Cc: pbonzini, vsementsov, den, marcandre.lureau, qemu-block,
Kevin Wolf, Max Reitz
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>
---
v5: rebase to master, enough differences to drop R-b
v4: rebase to master
v3: new patch
---
block/nbd-client.h | 3 +--
include/block/nbd.h | 15 +++++++++++----
block/nbd-client.c | 18 ++++++++----------
block/nbd.c | 2 +-
nbd/client.c | 44 ++++++++++++++++++++++----------------------
qemu-nbd.c | 10 ++++------
6 files changed, 47 insertions(+), 45 deletions(-)
diff --git a/block/nbd-client.h b/block/nbd-client.h
index 49636bc..df80771 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -20,8 +20,7 @@
typedef struct NBDClientSession {
QIOChannelSocket *sioc; /* The master data channel */
QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */
- uint16_t nbdflags;
- off_t size;
+ NBDExportInfo info;
CoMutex send_mutex;
CoQueue free_sema;
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 6d75d5a..9092374 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -123,13 +123,20 @@ enum {
* aren't overflowing some other buffer. */
#define NBD_MAX_NAME_SIZE 256
+/* Details collected by NBD_OPT_EXPORT_NAME and NBD_OPT_GO */
+struct NBDExportInfo {
+ uint64_t size;
+ uint16_t flags;
+};
+typedef struct NBDExportInfo NBDExportInfo;
+
ssize_t nbd_rwv(QIOChannel *ioc, struct iovec *iov, size_t niov, size_t length,
bool do_read, Error **errp);
-int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
+int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
QCryptoTLSCreds *tlscreds, const char *hostname,
- QIOChannel **outioc,
- off_t *size, Error **errp);
-int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size,
+ QIOChannel **outioc, NBDExportInfo *info,
+ Error **errp);
+int nbd_init(int fd, QIOChannelSocket *sioc, NBDExportInfo *info,
Error **errp);
ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest *request);
ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp);
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 208f907..aab1e32 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -242,7 +242,7 @@ int nbd_client_co_pwritev(BlockDriverState *bs, uint64_t offset,
ssize_t ret;
if (flags & BDRV_REQ_FUA) {
- assert(client->nbdflags & NBD_FLAG_SEND_FUA);
+ assert(client->info.flags & NBD_FLAG_SEND_FUA);
request.flags |= NBD_CMD_FLAG_FUA;
}
@@ -270,12 +270,12 @@ int nbd_client_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
};
NBDReply reply;
- if (!(client->nbdflags & NBD_FLAG_SEND_WRITE_ZEROES)) {
+ if (!(client->info.flags & NBD_FLAG_SEND_WRITE_ZEROES)) {
return -ENOTSUP;
}
if (flags & BDRV_REQ_FUA) {
- assert(client->nbdflags & NBD_FLAG_SEND_FUA);
+ assert(client->info.flags & NBD_FLAG_SEND_FUA);
request.flags |= NBD_CMD_FLAG_FUA;
}
if (!(flags & BDRV_REQ_MAY_UNMAP)) {
@@ -299,7 +299,7 @@ int nbd_client_co_flush(BlockDriverState *bs)
NBDReply reply;
ssize_t ret;
- if (!(client->nbdflags & NBD_FLAG_SEND_FLUSH)) {
+ if (!(client->info.flags & NBD_FLAG_SEND_FLUSH)) {
return 0;
}
@@ -327,7 +327,7 @@ int nbd_client_co_pdiscard(BlockDriverState *bs, int64_t offset, int bytes)
NBDReply reply;
ssize_t ret;
- if (!(client->nbdflags & NBD_FLAG_SEND_TRIM)) {
+ if (!(client->info.flags & NBD_FLAG_SEND_TRIM)) {
return 0;
}
@@ -385,19 +385,17 @@ int nbd_client_init(BlockDriverState *bs,
qio_channel_set_blocking(QIO_CHANNEL(sioc), true, NULL);
ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), export,
- &client->nbdflags,
tlscreds, hostname,
- &client->ioc,
- &client->size, errp);
+ &client->ioc, &client->info, errp);
if (ret < 0) {
logout("Failed to negotiate with the NBD server\n");
return ret;
}
- if (client->nbdflags & NBD_FLAG_SEND_FUA) {
+ if (client->info.flags & NBD_FLAG_SEND_FUA) {
bs->supported_write_flags = BDRV_REQ_FUA;
bs->supported_zero_flags |= BDRV_REQ_FUA;
}
- if (client->nbdflags & NBD_FLAG_SEND_WRITE_ZEROES) {
+ if (client->info.flags & NBD_FLAG_SEND_WRITE_ZEROES) {
bs->supported_zero_flags |= BDRV_REQ_MAY_UNMAP;
}
diff --git a/block/nbd.c b/block/nbd.c
index d529305..4a9048c 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -492,7 +492,7 @@ static int64_t nbd_getlength(BlockDriverState *bs)
{
BDRVNBDState *s = bs->opaque;
- return s->client.size;
+ return s->client.info.size;
}
static void nbd_detach_aio_context(BlockDriverState *bs)
diff --git a/nbd/client.c b/nbd/client.c
index 9c52b9b..607ad32 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -425,13 +425,13 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
}
-int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
+int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
QCryptoTLSCreds *tlscreds, const char *hostname,
- QIOChannel **outioc,
- off_t *size, Error **errp)
+ QIOChannel **outioc, NBDExportInfo *info,
+ Error **errp)
{
char buf[256];
- uint64_t magic, s;
+ uint64_t magic;
int rc;
bool zeroes = true;
@@ -532,17 +532,17 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
}
/* Read the response */
- if (nbd_read(ioc, &s, sizeof(s), errp) < 0) {
+ if (nbd_read(ioc, &info->size, sizeof(info->size), errp) < 0) {
error_prepend(errp, "Failed to read export length");
goto fail;
}
- *size = be64_to_cpu(s);
+ be64_to_cpus(&info->size);
- if (nbd_read(ioc, flags, sizeof(*flags), errp) < 0) {
+ if (nbd_read(ioc, &info->flags, sizeof(info->flags), errp) < 0) {
error_prepend(errp, "Failed to read export flags");
goto fail;
}
- be16_to_cpus(flags);
+ be16_to_cpus(&info->flags);
} else if (magic == NBD_CLIENT_MAGIC) {
uint32_t oldflags;
@@ -555,11 +555,11 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
goto fail;
}
- if (nbd_read(ioc, &s, sizeof(s), errp) < 0) {
+ if (nbd_read(ioc, &info->size, sizeof(info->size), errp) < 0) {
error_prepend(errp, "Failed to read export length");
goto fail;
}
- *size = be64_to_cpu(s);
+ be64_to_cpus(&info->size);
if (nbd_read(ioc, &oldflags, sizeof(oldflags), errp) < 0) {
error_prepend(errp, "Failed to read export flags");
@@ -570,13 +570,13 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
error_setg(errp, "Unexpected export flags %0x" PRIx32, oldflags);
goto fail;
}
- *flags = oldflags;
+ info->flags = oldflags;
} else {
error_setg(errp, "Bad magic received");
goto fail;
}
- trace_nbd_receive_negotiate_size_flags(*size, *flags);
+ trace_nbd_receive_negotiate_size_flags(info->size, info->flags);
if (zeroes && nbd_drop(ioc, 124, errp) < 0) {
error_prepend(errp, "Failed to read reserved block");
goto fail;
@@ -588,13 +588,13 @@ fail:
}
#ifdef __linux__
-int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size,
+int nbd_init(int fd, QIOChannelSocket *sioc, NBDExportInfo *info,
Error **errp)
{
- unsigned long sectors = size / BDRV_SECTOR_SIZE;
- if (size / BDRV_SECTOR_SIZE != sectors) {
- error_setg(errp, "Export size %lld too large for 32-bit kernel",
- (long long) size);
+ unsigned long sectors = info->size / BDRV_SECTOR_SIZE;
+ if (info->size / BDRV_SECTOR_SIZE != sectors) {
+ error_setg(errp, "Export size %" PRId64 " too large for 32-bit kernel",
+ info->size);
return -E2BIG;
}
@@ -615,8 +615,8 @@ int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size,
}
trace_nbd_init_set_size(sectors);
- if (size % BDRV_SECTOR_SIZE) {
- trace_nbd_init_trailing_bytes(size % BDRV_SECTOR_SIZE);
+ if (info->size % BDRV_SECTOR_SIZE) {
+ trace_nbd_init_trailing_bytes(info->size % BDRV_SECTOR_SIZE);
}
if (ioctl(fd, NBD_SET_SIZE_BLOCKS, sectors) < 0) {
@@ -625,9 +625,9 @@ int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size,
return -serrno;
}
- if (ioctl(fd, NBD_SET_FLAGS, (unsigned long) flags) < 0) {
+ if (ioctl(fd, NBD_SET_FLAGS, (unsigned long) info->flags) < 0) {
if (errno == ENOTTY) {
- int read_only = (flags & NBD_FLAG_READ_ONLY) != 0;
+ int read_only = (info->flags & NBD_FLAG_READ_ONLY) != 0;
trace_nbd_init_set_readonly();
if (ioctl(fd, BLKROSET, (unsigned long) &read_only) < 0) {
@@ -685,7 +685,7 @@ int nbd_disconnect(int fd)
}
#else
-int nbd_init(int fd, QIOChannelSocket *ioc, uint16_t flags, off_t size,
+int nbd_init(int fd, QIOChannelSocket *ioc, NBDExportInfo *info,
Error **errp)
{
error_setg(errp, "nbd_init is only supported on Linux");
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 4dd3fd4..c8bd47f 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -255,8 +255,7 @@ static void *show_parts(void *arg)
static void *nbd_client_thread(void *arg)
{
char *device = arg;
- off_t size;
- uint16_t nbdflags;
+ NBDExportInfo info;
QIOChannelSocket *sioc;
int fd;
int ret;
@@ -271,9 +270,8 @@ static void *nbd_client_thread(void *arg)
goto out;
}
- ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), NULL, &nbdflags,
- NULL, NULL, NULL,
- &size, &local_error);
+ ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), NULL,
+ NULL, NULL, NULL, &info, &local_error);
if (ret < 0) {
if (local_error) {
error_report_err(local_error);
@@ -288,7 +286,7 @@ static void *nbd_client_thread(void *arg)
goto out_socket;
}
- ret = nbd_init(fd, sioc, nbdflags, size, &local_error);
+ ret = nbd_init(fd, sioc, &info, &local_error);
if (ret < 0) {
error_report_err(local_error);
goto out_fd;
--
2.9.4
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v5 1/9] nbd: Create struct for tracking export info
2017-07-07 20:30 ` [Qemu-devel] [PATCH v5 1/9] nbd: Create struct for tracking export info Eric Blake
@ 2017-07-13 12:09 ` Vladimir Sementsov-Ogievskiy
2017-07-13 12:31 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-07-13 12:09 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: pbonzini, den, marcandre.lureau, qemu-block, Kevin Wolf,
Max Reitz
07.07.2017 23:30, Eric Blake wrote:
> 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.
// it was me who do so)
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v5: rebase to master, enough differences to drop R-b
> v4: rebase to master
> v3: new patch
> ---
> block/nbd-client.h | 3 +--
> include/block/nbd.h | 15 +++++++++++----
> block/nbd-client.c | 18 ++++++++----------
> block/nbd.c | 2 +-
> nbd/client.c | 44 ++++++++++++++++++++++----------------------
> qemu-nbd.c | 10 ++++------
> 6 files changed, 47 insertions(+), 45 deletions(-)
>
> diff --git a/block/nbd-client.h b/block/nbd-client.h
> index 49636bc..df80771 100644
> --- a/block/nbd-client.h
> +++ b/block/nbd-client.h
> @@ -20,8 +20,7 @@
> typedef struct NBDClientSession {
> QIOChannelSocket *sioc; /* The master data channel */
> QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */
> - uint16_t nbdflags;
> - off_t size;
was signed
> + NBDExportInfo info;
>
> CoMutex send_mutex;
> CoQueue free_sema;
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 6d75d5a..9092374 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -123,13 +123,20 @@ enum {
> * aren't overflowing some other buffer. */
> #define NBD_MAX_NAME_SIZE 256
>
> +/* Details collected by NBD_OPT_EXPORT_NAME and NBD_OPT_GO */
> +struct NBDExportInfo {
> + uint64_t size;
becomes unsigned
> + uint16_t flags;
> +};
> +typedef struct NBDExportInfo NBDExportInfo;
> +
> ssize_t nbd_rwv(QIOChannel *ioc, struct iovec *iov, size_t niov, size_t length,
> bool do_read, Error **errp);
[...]
> --- a/nbd/client.c
> +++ b/nbd/client.c
>
[...]
> @@ -570,13 +570,13 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
> error_setg(errp, "Unexpected export flags %0x" PRIx32, oldflags);
> goto fail;
> }
> - *flags = oldflags;
> + info->flags = oldflags;
> } else {
> error_setg(errp, "Bad magic received");
> goto fail;
> }
>
> - trace_nbd_receive_negotiate_size_flags(*size, *flags);
> + trace_nbd_receive_negotiate_size_flags(info->size, info->flags);
trace function declared with uint64_t so this was not very good and
becomes ok with this patch.
> if (zeroes && nbd_drop(ioc, 124, errp) < 0) {
> error_prepend(errp, "Failed to read reserved block");
> goto fail;
> @@ -588,13 +588,13 @@ fail:
> }
>
> #ifdef __linux__
> -int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size,
> +int nbd_init(int fd, QIOChannelSocket *sioc, NBDExportInfo *info,
> Error **errp)
> {
> - unsigned long sectors = size / BDRV_SECTOR_SIZE;
> - if (size / BDRV_SECTOR_SIZE != sectors) {
> - error_setg(errp, "Export size %lld too large for 32-bit kernel",
> - (long long) size);
> + unsigned long sectors = info->size / BDRV_SECTOR_SIZE;
> + if (info->size / BDRV_SECTOR_SIZE != sectors) {
> + error_setg(errp, "Export size %" PRId64 " too large for 32-bit kernel",
should be PRIu64
> + info->size);
> return -E2BIG;
> }
>
[...]
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v5 1/9] nbd: Create struct for tracking export info
2017-07-13 12:09 ` Vladimir Sementsov-Ogievskiy
@ 2017-07-13 12:31 ` Vladimir Sementsov-Ogievskiy
2017-07-13 14:35 ` Eric Blake
0 siblings, 1 reply; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-07-13 12:31 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: pbonzini, den, marcandre.lureau, qemu-block, Kevin Wolf,
Max Reitz
13.07.2017 15:09, Vladimir Sementsov-Ogievskiy wrote:
> 07.07.2017 23:30, Eric Blake wrote:
>> 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.
>
> // it was me who do so)
>
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> ---
>> v5: rebase to master, enough differences to drop R-b
>> v4: rebase to master
>> v3: new patch
>> ---
>> block/nbd-client.h | 3 +--
>> include/block/nbd.h | 15 +++++++++++----
>> block/nbd-client.c | 18 ++++++++----------
>> block/nbd.c | 2 +-
>> nbd/client.c | 44 ++++++++++++++++++++++----------------------
>> qemu-nbd.c | 10 ++++------
>> 6 files changed, 47 insertions(+), 45 deletions(-)
>>
>> diff --git a/block/nbd-client.h b/block/nbd-client.h
>> index 49636bc..df80771 100644
>> --- a/block/nbd-client.h
>> +++ b/block/nbd-client.h
>> @@ -20,8 +20,7 @@
>> typedef struct NBDClientSession {
>> QIOChannelSocket *sioc; /* The master data channel */
>> QIOChannel *ioc; /* The current I/O channel which may differ
>> (eg TLS) */
>> - uint16_t nbdflags;
>> - off_t size;
>
> was signed
>
>> + NBDExportInfo info;
>>
>> CoMutex send_mutex;
>> CoQueue free_sema;
>> diff --git a/include/block/nbd.h b/include/block/nbd.h
>> index 6d75d5a..9092374 100644
>> --- a/include/block/nbd.h
>> +++ b/include/block/nbd.h
>> @@ -123,13 +123,20 @@ enum {
>> * aren't overflowing some other buffer. */
>> #define NBD_MAX_NAME_SIZE 256
>>
>> +/* Details collected by NBD_OPT_EXPORT_NAME and NBD_OPT_GO */
>> +struct NBDExportInfo {
>> + uint64_t size;
>
> becomes unsigned
>
>> + uint16_t flags;
>> +};
>> +typedef struct NBDExportInfo NBDExportInfo;
>> +
>> ssize_t nbd_rwv(QIOChannel *ioc, struct iovec *iov, size_t niov,
>> size_t length,
>> bool do_read, Error **errp);
>
> [...]
>
>> --- a/nbd/client.c
>> +++ b/nbd/client.c
>>
>
> [...]
>
>> @@ -570,13 +570,13 @@ int nbd_receive_negotiate(QIOChannel *ioc,
>> const char *name, uint16_t *flags,
>> error_setg(errp, "Unexpected export flags %0x" PRIx32,
>> oldflags);
>> goto fail;
>> }
>> - *flags = oldflags;
>> + info->flags = oldflags;
>> } else {
>> error_setg(errp, "Bad magic received");
>> goto fail;
>> }
>>
>> - trace_nbd_receive_negotiate_size_flags(*size, *flags);
>> + trace_nbd_receive_negotiate_size_flags(info->size, info->flags);
>
> trace function declared with uint64_t so this was not very good and
> becomes ok with this patch.
>
>> if (zeroes && nbd_drop(ioc, 124, errp) < 0) {
>> error_prepend(errp, "Failed to read reserved block");
>> goto fail;
>> @@ -588,13 +588,13 @@ fail:
>> }
>>
>> #ifdef __linux__
>> -int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t
>> size,
>> +int nbd_init(int fd, QIOChannelSocket *sioc, NBDExportInfo *info,
>> Error **errp)
>> {
>> - unsigned long sectors = size / BDRV_SECTOR_SIZE;
>> - if (size / BDRV_SECTOR_SIZE != sectors) {
>> - error_setg(errp, "Export size %lld too large for 32-bit
>> kernel",
>> - (long long) size);
>> + unsigned long sectors = info->size / BDRV_SECTOR_SIZE;
>> + if (info->size / BDRV_SECTOR_SIZE != sectors) {
>> + error_setg(errp, "Export size %" PRId64 " too large for
>> 32-bit kernel",
>
> should be PRIu64
>
>> + info->size);
>> return -E2BIG;
>> }
>>
> [...]
>
with fixed:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
(if it is not too late ;)
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v5 1/9] nbd: Create struct for tracking export info
2017-07-13 12:31 ` Vladimir Sementsov-Ogievskiy
@ 2017-07-13 14:35 ` Eric Blake
2017-07-13 14:56 ` Paolo Bonzini
0 siblings, 1 reply; 28+ messages in thread
From: Eric Blake @ 2017-07-13 14:35 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel
Cc: pbonzini, den, marcandre.lureau, qemu-block, Kevin Wolf,
Max Reitz
[-- Attachment #1: Type: text/plain, Size: 2377 bytes --]
On 07/13/2017 07:31 AM, Vladimir Sementsov-Ogievskiy wrote:
> 13.07.2017 15:09, Vladimir Sementsov-Ogievskiy wrote:
>> 07.07.2017 23:30, Eric Blake wrote:
>>> 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.
>>
>> // it was me who do so)
>>
>>> +int nbd_init(int fd, QIOChannelSocket *sioc, NBDExportInfo *info,
>>> Error **errp)
>>> {
>>> - unsigned long sectors = size / BDRV_SECTOR_SIZE;
>>> - if (size / BDRV_SECTOR_SIZE != sectors) {
>>> - error_setg(errp, "Export size %lld too large for 32-bit
>>> kernel",
>>> - (long long) size);
>>> + unsigned long sectors = info->size / BDRV_SECTOR_SIZE;
>>> + if (info->size / BDRV_SECTOR_SIZE != sectors) {
>>> + error_setg(errp, "Export size %" PRId64 " too large for
>>> 32-bit kernel",
>>
>> should be PRIu64
Even with an unsigned size, we can't support more than the maximum off_t
(2^63-1) rather than the full uint64_t range (2^64-1). Any positive
size prints the same, and if any caller is passing in a negative size,
things are already weird. But you are correct that matching unsigned to
PRIu64 is nicer, even if we already make blatant assumptions that all
platforms that qemu runs on can interchange signedness in printf without
repercussion.
>>
>>> + info->size);
>>> return -E2BIG;
>>> }
>>>
>> [...]
>>
>
> with fixed:
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> (if it is not too late ;)
Depends on whether Paolo wants to send a v2 pull request omitting the
NBD stuff (in which case I'll submit a separate NBD pull request), or
whether we just let the pull request happen (in which case I'll submit
followup patches to address your reviews). It's slightly cleaner to get
it right from the get-go, but followups are better than nothing, so I'm
not too fussed either way.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v5 1/9] nbd: Create struct for tracking export info
2017-07-13 14:35 ` Eric Blake
@ 2017-07-13 14:56 ` Paolo Bonzini
0 siblings, 0 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-07-13 14:56 UTC (permalink / raw)
To: Eric Blake, Vladimir Sementsov-Ogievskiy, qemu-devel
Cc: den, marcandre.lureau, qemu-block, Kevin Wolf, Max Reitz
[-- Attachment #1: Type: text/plain, Size: 2455 bytes --]
On 13/07/2017 16:35, Eric Blake wrote:
> On 07/13/2017 07:31 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 13.07.2017 15:09, Vladimir Sementsov-Ogievskiy wrote:
>>> 07.07.2017 23:30, Eric Blake wrote:
>>>> 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.
>>>
>>> // it was me who do so)
>>>
>
>>>> +int nbd_init(int fd, QIOChannelSocket *sioc, NBDExportInfo *info,
>>>> Error **errp)
>>>> {
>>>> - unsigned long sectors = size / BDRV_SECTOR_SIZE;
>>>> - if (size / BDRV_SECTOR_SIZE != sectors) {
>>>> - error_setg(errp, "Export size %lld too large for 32-bit
>>>> kernel",
>>>> - (long long) size);
>>>> + unsigned long sectors = info->size / BDRV_SECTOR_SIZE;
>>>> + if (info->size / BDRV_SECTOR_SIZE != sectors) {
>>>> + error_setg(errp, "Export size %" PRId64 " too large for
>>>> 32-bit kernel",
>>>
>>> should be PRIu64
>
> Even with an unsigned size, we can't support more than the maximum off_t
> (2^63-1) rather than the full uint64_t range (2^64-1). Any positive
> size prints the same, and if any caller is passing in a negative size,
> things are already weird. But you are correct that matching unsigned to
> PRIu64 is nicer, even if we already make blatant assumptions that all
> platforms that qemu runs on can interchange signedness in printf without
> repercussion.
>
>>>
>>>> + info->size);
>>>> return -E2BIG;
>>>> }
>>>>
>>> [...]
>>>
>>
>> with fixed:
>>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>> (if it is not too late ;)
>
> Depends on whether Paolo wants to send a v2 pull request omitting the
> NBD stuff (in which case I'll submit a separate NBD pull request), or
> whether we just let the pull request happen (in which case I'll submit
> followup patches to address your reviews). It's slightly cleaner to get
> it right from the get-go, but followups are better than nothing, so I'm
> not too fussed either way.
I've fixed the PRIu64 already when applying (and also the tracepoint in
patch 2).
Paolo
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v5 2/9] nbd: Don't bother tracing an NBD_OPT_ABORT response failure
2017-07-07 20:30 [Qemu-devel] [PATCH v5 0/9] Implement NBD_OPT_GO, block size advertisement Eric Blake
2017-07-07 20:30 ` [Qemu-devel] [PATCH v5 1/9] nbd: Create struct for tracking export info Eric Blake
@ 2017-07-07 20:30 ` Eric Blake
2017-07-13 12:12 ` Vladimir Sementsov-Ogievskiy
2017-07-07 20:30 ` [Qemu-devel] [PATCH v5 3/9] nbd: Expose and debug more NBD constants Eric Blake
` (7 subsequent siblings)
9 siblings, 1 reply; 28+ messages in thread
From: Eric Blake @ 2017-07-07 20:30 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, vsementsov, den, marcandre.lureau, qemu-block
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>
---
v5: new patch
---
nbd/server.c | 15 ++++-----------
1 file changed, 4 insertions(+), 11 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index 9b0c588..e15385b 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -376,7 +376,6 @@ static int nbd_negotiate_options(NBDClient *client, Error **errp)
{
uint32_t flags;
bool fixedNewstyle = false;
- Error *local_err = NULL;
/* Client sends:
[ 0 .. 3] client flags
@@ -479,7 +478,9 @@ static int nbd_negotiate_options(NBDClient *client, Error **errp)
if (ret < 0) {
return ret;
}
- /* Let the client keep trying, unless they asked to quit */
+ /* Let the client keep trying, unless they asked to
+ * quit. In this mode, we've already sent an error, so
+ * we can't ack the abort. */
if (option == NBD_OPT_ABORT) {
return 1;
}
@@ -498,15 +499,7 @@ static int nbd_negotiate_options(NBDClient *client, Error **errp)
/* NBD spec says we must try to reply before
* disconnecting, but that we must also tolerate
* guests that don't wait for our reply. */
- nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, option,
- &local_err);
-
- if (local_err != NULL) {
- const char *error = error_get_pretty(local_err);
- trace_nbd_opt_abort_reply_failed(error);
- error_free(local_err);
- }
-
+ nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, option, NULL);
return 1;
case NBD_OPT_EXPORT_NAME:
--
2.9.4
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v5 2/9] nbd: Don't bother tracing an NBD_OPT_ABORT response failure
2017-07-07 20:30 ` [Qemu-devel] [PATCH v5 2/9] nbd: Don't bother tracing an NBD_OPT_ABORT response failure Eric Blake
@ 2017-07-13 12:12 ` Vladimir Sementsov-Ogievskiy
2017-07-13 12:31 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-07-13 12:12 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: pbonzini, den, marcandre.lureau, qemu-block
07.07.2017 23:30, Eric Blake wrote:
> 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>
>
> ---
> v5: new patch
> ---
> nbd/server.c | 15 ++++-----------
> 1 file changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/nbd/server.c b/nbd/server.c
> index 9b0c588..e15385b 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -376,7 +376,6 @@ static int nbd_negotiate_options(NBDClient *client, Error **errp)
> {
> uint32_t flags;
> bool fixedNewstyle = false;
> - Error *local_err = NULL;
>
> /* Client sends:
> [ 0 .. 3] client flags
> @@ -479,7 +478,9 @@ static int nbd_negotiate_options(NBDClient *client, Error **errp)
> if (ret < 0) {
> return ret;
> }
> - /* Let the client keep trying, unless they asked to quit */
> + /* Let the client keep trying, unless they asked to
> + * quit. In this mode, we've already sent an error, so
> + * we can't ack the abort. */
> if (option == NBD_OPT_ABORT) {
> return 1;
> }
> @@ -498,15 +499,7 @@ static int nbd_negotiate_options(NBDClient *client, Error **errp)
> /* NBD spec says we must try to reply before
> * disconnecting, but that we must also tolerate
> * guests that don't wait for our reply. */
> - nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, option,
> - &local_err);
> -
> - if (local_err != NULL) {
> - const char *error = error_get_pretty(local_err);
> - trace_nbd_opt_abort_reply_failed(error);
Looks like you forgot to drop this trace from nbd/trace-events
> - error_free(local_err);
> - }
> -
> + nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, option, NULL);
> return 1;
>
> case NBD_OPT_EXPORT_NAME:
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v5 2/9] nbd: Don't bother tracing an NBD_OPT_ABORT response failure
2017-07-13 12:12 ` Vladimir Sementsov-Ogievskiy
@ 2017-07-13 12:31 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-07-13 12:31 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: pbonzini, den, marcandre.lureau, qemu-block
13.07.2017 15:12, Vladimir Sementsov-Ogievskiy wrote:
> 07.07.2017 23:30, Eric Blake wrote:
>> 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>
>>
>> ---
>> v5: new patch
>> ---
>> nbd/server.c | 15 ++++-----------
>> 1 file changed, 4 insertions(+), 11 deletions(-)
>>
>> diff --git a/nbd/server.c b/nbd/server.c
>> index 9b0c588..e15385b 100644
>> --- a/nbd/server.c
>> +++ b/nbd/server.c
>> @@ -376,7 +376,6 @@ static int nbd_negotiate_options(NBDClient
>> *client, Error **errp)
>> {
>> uint32_t flags;
>> bool fixedNewstyle = false;
>> - Error *local_err = NULL;
>>
>> /* Client sends:
>> [ 0 .. 3] client flags
>> @@ -479,7 +478,9 @@ static int nbd_negotiate_options(NBDClient
>> *client, Error **errp)
>> if (ret < 0) {
>> return ret;
>> }
>> - /* Let the client keep trying, unless they asked to
>> quit */
>> + /* Let the client keep trying, unless they asked to
>> + * quit. In this mode, we've already sent an error, so
>> + * we can't ack the abort. */
>> if (option == NBD_OPT_ABORT) {
>> return 1;
>> }
>> @@ -498,15 +499,7 @@ static int nbd_negotiate_options(NBDClient
>> *client, Error **errp)
>> /* NBD spec says we must try to reply before
>> * disconnecting, but that we must also tolerate
>> * guests that don't wait for our reply. */
>> - nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
>> option,
>> - &local_err);
>> -
>> - if (local_err != NULL) {
>> - const char *error = error_get_pretty(local_err);
>> - trace_nbd_opt_abort_reply_failed(error);
>
> Looks like you forgot to drop this trace from nbd/trace-events
>
>> - error_free(local_err);
>> - }
>> -
>> + nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
>> option, NULL);
>> return 1;
>>
>> case NBD_OPT_EXPORT_NAME:
>
>
with fixed:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v5 3/9] nbd: Expose and debug more NBD constants
2017-07-07 20:30 [Qemu-devel] [PATCH v5 0/9] Implement NBD_OPT_GO, block size advertisement Eric Blake
2017-07-07 20:30 ` [Qemu-devel] [PATCH v5 1/9] nbd: Create struct for tracking export info Eric Blake
2017-07-07 20:30 ` [Qemu-devel] [PATCH v5 2/9] nbd: Don't bother tracing an NBD_OPT_ABORT response failure Eric Blake
@ 2017-07-07 20:30 ` Eric Blake
2017-07-07 20:51 ` Eric Blake
2017-07-13 12:52 ` Vladimir Sementsov-Ogievskiy
2017-07-07 20:30 ` [Qemu-devel] [PATCH v5 4/9] nbd: Simplify trace of client flags in negotiation Eric Blake
` (6 subsequent siblings)
9 siblings, 2 replies; 28+ messages in thread
From: Eric Blake @ 2017-07-07 20:30 UTC (permalink / raw)
To: qemu-devel
Cc: pbonzini, vsementsov, den, marcandre.lureau, qemu-block,
Kevin Wolf, Max Reitz
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>
---
v5: rebase to master, add command name lookup; enough changes that
R-b is dropped
v4: new patch
---
include/block/nbd.h | 35 +++++++++++++++-----
nbd/nbd-internal.h | 10 +++---
nbd/client.c | 52 +++++++++++++++++++-----------
nbd/common.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++++
nbd/server.c | 18 +++++++----
nbd/trace-events | 12 +++----
6 files changed, 174 insertions(+), 45 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 9092374..4a22eca 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2016 Red Hat, Inc.
+ * Copyright (C) 2016-2017 Red Hat, Inc.
* Copyright (C) 2005 Anthony Liguori <anthony@codemonkey.ws>
*
* Network Block Device
@@ -83,18 +83,37 @@ typedef struct NBDReply NBDReply;
#define NBD_FLAG_C_FIXED_NEWSTYLE (1 << 0) /* Fixed newstyle protocol. */
#define NBD_FLAG_C_NO_ZEROES (1 << 1) /* End handshake without zeroes. */
-/* Reply types. */
+/* Option requests. */
+#define NBD_OPT_EXPORT_NAME (1)
+#define NBD_OPT_ABORT (2)
+#define NBD_OPT_LIST (3)
+/* #define NBD_OPT_PEEK_EXPORT (4) not in use */
+#define NBD_OPT_STARTTLS (5)
+#define NBD_OPT_INFO (6)
+#define NBD_OPT_GO (7)
+#define NBD_OPT_STRUCTURED_REPLY (8)
+
+/* Option reply types. */
#define NBD_REP_ERR(value) ((UINT32_C(1) << 31) | (value))
#define NBD_REP_ACK (1) /* Data sending finished. */
#define NBD_REP_SERVER (2) /* Export description. */
+#define NBD_REP_INFO (3) /* NBD_OPT_INFO/GO. */
-#define NBD_REP_ERR_UNSUP NBD_REP_ERR(1) /* Unknown option */
-#define NBD_REP_ERR_POLICY NBD_REP_ERR(2) /* Server denied */
-#define NBD_REP_ERR_INVALID NBD_REP_ERR(3) /* Invalid length */
-#define NBD_REP_ERR_PLATFORM NBD_REP_ERR(4) /* Not compiled in */
-#define NBD_REP_ERR_TLS_REQD NBD_REP_ERR(5) /* TLS required */
-#define NBD_REP_ERR_SHUTDOWN NBD_REP_ERR(7) /* Server shutting down */
+#define NBD_REP_ERR_UNSUP NBD_REP_ERR(1) /* Unknown option */
+#define NBD_REP_ERR_POLICY NBD_REP_ERR(2) /* Server denied */
+#define NBD_REP_ERR_INVALID NBD_REP_ERR(3) /* Invalid length */
+#define NBD_REP_ERR_PLATFORM NBD_REP_ERR(4) /* Not compiled in */
+#define NBD_REP_ERR_TLS_REQD NBD_REP_ERR(5) /* TLS required */
+#define NBD_REP_ERR_UNKNOWN NBD_REP_ERR(6) /* Export unknown */
+#define NBD_REP_ERR_SHUTDOWN NBD_REP_ERR(7) /* Server shutting down */
+#define NBD_REP_ERR_BLOCK_SIZE_REQD NBD_REP_ERR(8) /* Need INFO_BLOCK_SIZE */
+
+/* Info types, used during NBD_REP_INFO */
+#define NBD_INFO_EXPORT 0
+#define NBD_INFO_NAME 1
+#define NBD_INFO_DESCRIPTION 2
+#define NBD_INFO_BLOCK_SIZE 3
/* Request flags, sent from client to server during transmission phase */
#define NBD_CMD_FLAG_FUA (1 << 0) /* 'force unit access' during write */
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index cf6ecbf..bf95601 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -57,12 +57,6 @@
#define NBD_SET_TIMEOUT _IO(0xab, 9)
#define NBD_SET_FLAGS _IO(0xab, 10)
-#define NBD_OPT_EXPORT_NAME (1)
-#define NBD_OPT_ABORT (2)
-#define NBD_OPT_LIST (3)
-#define NBD_OPT_PEEK_EXPORT (4)
-#define NBD_OPT_STARTTLS (5)
-
/* NBD errors are based on errno numbers, so there is a 1:1 mapping,
* but only a limited set of errno values is specified in the protocol.
* Everything else is squashed to EINVAL.
@@ -133,6 +127,10 @@ struct NBDTLSHandshakeData {
void nbd_tls_handshake(QIOTask *task,
void *opaque);
+const char *nbd_opt_lookup(uint32_t opt);
+const char *nbd_rep_lookup(uint32_t rep);
+const char *nbd_info_lookup(uint16_t info);
+const char *nbd_cmd_lookup(uint16_t info);
int nbd_drop(QIOChannel *ioc, size_t size, Error **errp);
diff --git a/nbd/client.c b/nbd/client.c
index 607ad32..011960b 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2016 Red Hat, Inc.
+ * Copyright (C) 2016-2017 Red Hat, Inc.
* Copyright (C) 2005 Anthony Liguori <anthony@codemonkey.ws>
*
* Network Block Device Client Side
@@ -104,7 +104,7 @@ static int nbd_send_option_request(QIOChannel *ioc, uint32_t opt,
if (len == -1) {
req.length = len = strlen(data);
}
- trace_nbd_send_option_request(opt, len);
+ trace_nbd_send_option_request(opt, nbd_opt_lookup(opt), len);
stq_be_p(&req.magic, NBD_OPTS_MAGIC);
stl_be_p(&req.option, opt);
@@ -154,7 +154,9 @@ static int nbd_receive_option_reply(QIOChannel *ioc, uint32_t opt,
be32_to_cpus(&reply->type);
be32_to_cpus(&reply->length);
- trace_nbd_receive_option_reply(reply->option, reply->type, reply->length);
+ trace_nbd_receive_option_reply(reply->option, nbd_opt_lookup(reply->option),
+ reply->type, nbd_rep_lookup(reply->type),
+ reply->length);
if (reply->magic != NBD_REP_MAGIC) {
error_setg(errp, "Unexpected option reply magic");
@@ -188,12 +190,16 @@ static int nbd_handle_reply_err(QIOChannel *ioc, nbd_opt_reply *reply,
if (reply->length) {
if (reply->length > NBD_MAX_BUFFER_SIZE) {
- error_setg(errp, "server's error message is too long");
+ error_setg(errp, "server error 0x%" PRIx32
+ " (%s) message is too long",
+ reply->type, nbd_rep_lookup(reply->type));
goto cleanup;
}
msg = g_malloc(reply->length + 1);
if (nbd_read(ioc, msg, reply->length, errp) < 0) {
- error_prepend(errp, "failed to read option error message");
+ error_prepend(errp, "failed to read option error 0x%" PRIx32
+ " (%s) message",
+ reply->type, nbd_rep_lookup(reply->type));
goto cleanup;
}
msg[reply->length] = '\0';
@@ -201,38 +207,48 @@ static int nbd_handle_reply_err(QIOChannel *ioc, nbd_opt_reply *reply,
switch (reply->type) {
case NBD_REP_ERR_UNSUP:
- trace_nbd_reply_err_unsup(reply->option);
+ trace_nbd_reply_err_unsup(reply->option, nbd_opt_lookup(reply->option));
result = 0;
goto cleanup;
case NBD_REP_ERR_POLICY:
- error_setg(errp, "Denied by server for option %" PRIx32,
- reply->option);
+ error_setg(errp, "Denied by server for option %" PRIx32 " (%s)",
+ reply->option, nbd_opt_lookup(reply->option));
break;
case NBD_REP_ERR_INVALID:
- error_setg(errp, "Invalid data length for option %" PRIx32,
- reply->option);
+ error_setg(errp, "Invalid data length for option %" PRIx32 " (%s)",
+ reply->option, nbd_opt_lookup(reply->option));
break;
case NBD_REP_ERR_PLATFORM:
- error_setg(errp, "Server lacks support for option %" PRIx32,
- reply->option);
+ error_setg(errp, "Server lacks support for option %" PRIx32 " (%s)",
+ reply->option, nbd_opt_lookup(reply->option));
break;
case NBD_REP_ERR_TLS_REQD:
- error_setg(errp, "TLS negotiation required before option %" PRIx32,
- reply->option);
+ error_setg(errp, "TLS negotiation required before option %" PRIx32
+ " (%s)", reply->option, nbd_opt_lookup(reply->option));
+ break;
+
+ case NBD_REP_ERR_UNKNOWN:
+ error_setg(errp, "Requested export not available for option %" PRIx32
+ " (%s)", reply->option, nbd_opt_lookup(reply->option));
break;
case NBD_REP_ERR_SHUTDOWN:
- error_setg(errp, "Server shutting down before option %" PRIx32,
- reply->option);
+ error_setg(errp, "Server shutting down before option %" PRIx32 " (%s)",
+ reply->option, nbd_opt_lookup(reply->option));
+ break;
+
+ case NBD_REP_ERR_BLOCK_SIZE_REQD:
+ error_setg(errp, "Server requires INFO_BLOCK_SIZE for option %" PRIx32
+ " (%s)", reply->option, nbd_opt_lookup(reply->option));
break;
default:
- error_setg(errp, "Unknown error code when asking for option %" PRIx32,
- reply->option);
+ error_setg(errp, "Unknown error code when asking for option %" PRIx32
+ " (%s)", reply->option, nbd_opt_lookup(reply->option));
break;
}
diff --git a/nbd/common.c b/nbd/common.c
index 4dab41e..a2f28f2 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -101,3 +101,95 @@ void nbd_tls_handshake(QIOTask *task,
data->complete = true;
g_main_loop_quit(data->loop);
}
+
+
+const char *nbd_opt_lookup(uint32_t opt)
+{
+ switch (opt) {
+ case NBD_OPT_EXPORT_NAME:
+ return "export name";
+ case NBD_OPT_ABORT:
+ return "abort";
+ case NBD_OPT_LIST:
+ return "list";
+ case NBD_OPT_STARTTLS:
+ return "starttls";
+ case NBD_OPT_INFO:
+ return "info";
+ case NBD_OPT_GO:
+ return "go";
+ case NBD_OPT_STRUCTURED_REPLY:
+ return "structured reply";
+ default:
+ return "<unknown>";
+ }
+}
+
+
+const char *nbd_rep_lookup(uint32_t rep)
+{
+ switch (rep) {
+ case NBD_REP_ACK:
+ return "ack";
+ case NBD_REP_SERVER:
+ return "server";
+ case NBD_REP_INFO:
+ return "info";
+ case NBD_REP_ERR_UNSUP:
+ return "unsupported";
+ case NBD_REP_ERR_POLICY:
+ return "denied by policy";
+ case NBD_REP_ERR_INVALID:
+ return "invalid";
+ case NBD_REP_ERR_PLATFORM:
+ return "platform lacks support";
+ case NBD_REP_ERR_TLS_REQD:
+ return "TLS required";
+ case NBD_REP_ERR_UNKNOWN:
+ return "export unknown";
+ case NBD_REP_ERR_SHUTDOWN:
+ return "server shutting down";
+ case NBD_REP_ERR_BLOCK_SIZE_REQD:
+ return "block size required";
+ default:
+ return "<unknown>";
+ }
+}
+
+
+const char *nbd_info_lookup(uint16_t info)
+{
+ switch (info) {
+ case NBD_INFO_EXPORT:
+ return "export";
+ case NBD_INFO_NAME:
+ return "name";
+ case NBD_INFO_DESCRIPTION:
+ return "description";
+ case NBD_INFO_BLOCK_SIZE:
+ return "block size";
+ default:
+ return "<unknown>";
+ }
+}
+
+
+const char *nbd_cmd_lookup(uint16_t cmd)
+{
+ switch (cmd) {
+ case NBD_CMD_READ:
+ return "read";
+ case NBD_CMD_WRITE:
+ return "write";
+ case NBD_CMD_DISC:
+ return "discard";
+ case NBD_CMD_FLUSH:
+ return "flush";
+ case NBD_CMD_TRIM:
+ return "trim";
+ case NBD_CMD_WRITE_ZEROES:
+ return "write zeroes";
+ default:
+ return "<unknown>";
+ }
+}
diff --git a/nbd/server.c b/nbd/server.c
index e15385b..27a0aab 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2016 Red Hat, Inc.
+ * Copyright (C) 2016-2017 Red Hat, Inc.
* Copyright (C) 2005 Anthony Liguori <anthony@codemonkey.ws>
*
* Network Block Device Server Side
@@ -139,7 +139,8 @@ static int nbd_negotiate_send_rep_len(QIOChannel *ioc, uint32_t type,
{
uint64_t magic;
- trace_nbd_negotiate_send_rep_len(opt, type, len);
+ trace_nbd_negotiate_send_rep_len(opt, nbd_opt_lookup(opt),
+ type, nbd_rep_lookup(type), len);
magic = cpu_to_be64(NBD_REP_MAGIC);
if (nbd_write(ioc, &magic, sizeof(magic), errp) < 0) {
@@ -441,7 +442,8 @@ static int nbd_negotiate_options(NBDClient *client, Error **errp)
}
length = be32_to_cpu(length);
- trace_nbd_negotiate_options_check_option(option);
+ trace_nbd_negotiate_options_check_option(option,
+ nbd_opt_lookup(option));
if (client->tlscreds &&
client->ioc == (QIOChannel *)client->sioc) {
QIOChannel *tioc;
@@ -532,8 +534,8 @@ static int nbd_negotiate_options(NBDClient *client, Error **errp)
NBD_REP_ERR_UNSUP,
option, errp,
"Unsupported option 0x%"
- PRIx32,
- option);
+ PRIx32 " (%s)", option,
+ nbd_opt_lookup(option));
if (ret < 0) {
return ret;
}
@@ -549,7 +551,8 @@ static int nbd_negotiate_options(NBDClient *client, Error **errp)
return nbd_negotiate_handle_export_name(client, length, errp);
default:
- error_setg(errp, "Unsupported option 0x%" PRIx32, option);
+ error_setg(errp, "Unsupported option 0x%" PRIx32 " (%s)",
+ option, nbd_opt_lookup(option));
return -EINVAL;
}
}
@@ -1033,7 +1036,8 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request,
return -EIO;
}
- trace_nbd_co_receive_request_decode_type(request->handle, request->type);
+ trace_nbd_co_receive_request_decode_type(request->handle, request->type,
+ nbd_cmd_lookup(request->type));
if (request->type != NBD_CMD_WRITE) {
/* No payload, we are ready to read the next request. */
diff --git a/nbd/trace-events b/nbd/trace-events
index 4b233b8..e61feda 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -1,8 +1,8 @@
# nbd/client.c
nbd_unknown_error(int err) "Squashing unexpected error %d to EINVAL"
-nbd_send_option_request(uint32_t opt, uint32_t len) "Sending option request %" PRIu32", len %" PRIu32
-nbd_receive_option_reply(uint32_t option, uint32_t type, uint32_t length) "Received option reply %" PRIx32", type %" PRIx32", len %" PRIu32
-nbd_reply_err_unsup(uint32_t option) "server doesn't understand request %" PRIx32 ", attempting fallback"
+nbd_send_option_request(uint32_t opt, const char *name, uint32_t len) "Sending option request %" PRIu32" (%s), len %" PRIu32
+nbd_receive_option_reply(uint32_t option, const char *optname, uint32_t type, const char *typename, uint32_t length) "Received option reply %" PRIx32" (%s), type %" PRIx32" (%s), len %" PRIu32
+nbd_reply_err_unsup(uint32_t option, const char *name) "server doesn't understand request %" PRIx32 " (%s), attempting fallback"
nbd_receive_query_exports_start(const char *wantname) "Querying export list for '%s'"
nbd_receive_query_exports_success(const char *wantname) "Found desired export name '%s'"
nbd_receive_starttls_request(void) "Requesting TLS from server"
@@ -28,7 +28,7 @@ nbd_send_request(uint64_t from, uint32_t len, uint64_t handle, uint16_t flags, u
nbd_receive_reply(uint32_t magic, int32_t error, uint64_t handle) "Got reply: { magic = 0x%" PRIx32 ", .error = % " PRId32 ", handle = %" PRIu64" }"
# nbd/server.c
-nbd_negotiate_send_rep_len(uint32_t opt, uint32_t type, uint32_t len) "Reply opt=%" PRIx32 " type=%" PRIx32 " len=%" PRIu32
+nbd_negotiate_send_rep_len(uint32_t opt, const char *optname, uint32_t type, const char *typename, uint32_t len) "Reply opt=%" PRIx32 " (%s), type=%" PRIx32 " (%s), len=%" PRIu32
nbd_negotiate_send_rep_err(const char *msg) "sending error message \"%s\""
nbd_negotiate_send_rep_list(const char *name, const char *desc) "Advertising export name '%s' description '%s'"
nbd_negotiate_handle_export_name(void) "Checking length"
@@ -39,7 +39,7 @@ nbd_negotiate_options_flags(void) "Checking client flags"
nbd_negotiate_options_newstyle(void) "Client supports fixed newstyle handshake"
nbd_negotiate_options_no_zeroes(void) "Client supports no zeroes at handshake end"
nbd_negotiate_options_check_magic(uint64_t magic) "Checking opts magic 0x%" PRIx64
-nbd_negotiate_options_check_option(uint32_t option) "Checking option 0x%" PRIx32
+nbd_negotiate_options_check_option(uint32_t option, const char *name) "Checking option 0x%" PRIx32 " (%s)"
nbd_opt_abort_reply_failed(const char *error) "Reply to NBD_OPT_ABORT request failed: %s"
nbd_negotiate_begin(void) "Beginning negotiation"
nbd_negotiate_old_style(uint64_t size, unsigned flags) "advertising size %" PRIu64 " and flags %x"
@@ -50,7 +50,7 @@ nbd_send_reply(int32_t error, uint64_t handle) "Sending response to client: { .e
nbd_blk_aio_attached(const char *name, void *ctx) "Export %s: Attaching clients to AIO context %p\n"
nbd_blk_aio_detach(const char *name, void *ctx) "Export %s: Detaching clients from AIO context %p\n"
nbd_co_send_reply(uint64_t handle, uint32_t error, int len) "Send reply: handle = %" PRIu64 ", error = %" PRIu32 ", len = %d"
-nbd_co_receive_request_decode_type(uint64_t handle, uint16_t type) "Decoding type: handle = %" PRIu64 ", type = %" PRIu16
+nbd_co_receive_request_decode_type(uint64_t handle, uint16_t type, const char *name) "Decoding type: handle = %" PRIu64 ", type = %" PRIu16 " (%s)"
nbd_co_receive_request_payload_received(uint64_t handle, uint32_t len) "Payload received: handle = %" PRIu64 ", len = %" PRIu32
nbd_co_receive_request_cmd_write(uint32_t len) "Reading %" PRIu32 " byte(s)"
nbd_trip(void) "Reading request"
--
2.9.4
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v5 3/9] nbd: Expose and debug more NBD constants
2017-07-07 20:30 ` [Qemu-devel] [PATCH v5 3/9] nbd: Expose and debug more NBD constants Eric Blake
@ 2017-07-07 20:51 ` Eric Blake
2017-07-13 12:52 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 28+ messages in thread
From: Eric Blake @ 2017-07-07 20:51 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, vsementsov, den, qemu-block, Max Reitz,
marcandre.lureau, pbonzini
[-- Attachment #1: Type: text/plain, Size: 2216 bytes --]
On 07/07/2017 03:30 PM, Eric Blake wrote:
> 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>
>
> ---
> v5: rebase to master, add command name lookup; enough changes that
> R-b is dropped
> v4: new patch
Worth squashing this in:
diff --git i/nbd/client.c w/nbd/client.c
index af2b46d..8dd2d69 100644
--- i/nbd/client.c
+++ w/nbd/client.c
@@ -902,7 +902,8 @@ ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest
*request)
uint8_t buf[NBD_REQUEST_SIZE];
trace_nbd_send_request(request->from, request->len, request->handle,
- request->flags, request->type);
+ request->flags, request->type,
+ nbd_cmd_lookup(request->type));
stl_be_p(buf, NBD_REQUEST_MAGIC);
stw_be_p(buf + 4, request->flags);
diff --git i/nbd/trace-events w/nbd/trace-events
index b18d051..74ed3b8 100644
--- i/nbd/trace-events
+++ w/nbd/trace-events
@@ -28,7 +28,7 @@ nbd_client_loop(void) "Doing NBD loop"
nbd_client_loop_ret(int ret, const char *error) "NBD loop returned %d: %s"
nbd_client_clear_queue(void) "Clearing NBD queue"
nbd_client_clear_socket(void) "Clearing NBD socket"
-nbd_send_request(uint64_t from, uint32_t len, uint64_t handle, uint16_t
flags, uint16_t type) "Sending request to server: { .from = %" PRIu64",
.len = %" PRIu32 ", .handle = %" PRIu64 ", .flags = %" PRIx16 ", .type =
%" PRIu16 " }"
+nbd_send_request(uint64_t from, uint32_t len, uint64_t handle, uint16_t
flags, uint16_t type, const char *name) "Sending request to server: {
.from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64 ", .flags =
%" PRIx16 ", .type = %" PRIu16 " (%s) }"
nbd_receive_reply(uint32_t magic, int32_t error, uint64_t handle) "Got
reply: { magic = 0x%" PRIx32 ", .error = % " PRId32 ", handle = %"
PRIu64" }"
# nbd/server.c
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v5 3/9] nbd: Expose and debug more NBD constants
2017-07-07 20:30 ` [Qemu-devel] [PATCH v5 3/9] nbd: Expose and debug more NBD constants Eric Blake
2017-07-07 20:51 ` Eric Blake
@ 2017-07-13 12:52 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-07-13 12:52 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: pbonzini, den, marcandre.lureau, qemu-block, Kevin Wolf,
Max Reitz
07.07.2017 23:30, Eric Blake wrote:
> 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>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
(with or without addition for trace_nbd_send_request)
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v5 4/9] nbd: Simplify trace of client flags in negotiation
2017-07-07 20:30 [Qemu-devel] [PATCH v5 0/9] Implement NBD_OPT_GO, block size advertisement Eric Blake
` (2 preceding siblings ...)
2017-07-07 20:30 ` [Qemu-devel] [PATCH v5 3/9] nbd: Expose and debug more NBD constants Eric Blake
@ 2017-07-07 20:30 ` Eric Blake
2017-07-13 12:53 ` Vladimir Sementsov-Ogievskiy
2017-07-07 20:30 ` [Qemu-devel] [PATCH v5 5/9] nbd: Refactor reply to NBD_OPT_EXPORT_NAME Eric Blake
` (5 subsequent siblings)
9 siblings, 1 reply; 28+ messages in thread
From: Eric Blake @ 2017-07-07 20:30 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, vsementsov, den, marcandre.lureau, qemu-block
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>
---
v5: new patch
---
nbd/server.c | 6 ++----
nbd/trace-events | 4 +---
2 files changed, 3 insertions(+), 7 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index 27a0aab..8e363d3 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -396,21 +396,19 @@ static int nbd_negotiate_options(NBDClient *client, Error **errp)
error_prepend(errp, "read failed: ");
return -EIO;
}
- trace_nbd_negotiate_options_flags();
be32_to_cpus(&flags);
+ trace_nbd_negotiate_options_flags(flags);
if (flags & NBD_FLAG_C_FIXED_NEWSTYLE) {
- trace_nbd_negotiate_options_newstyle();
fixedNewstyle = true;
flags &= ~NBD_FLAG_C_FIXED_NEWSTYLE;
}
if (flags & NBD_FLAG_C_NO_ZEROES) {
- trace_nbd_negotiate_options_no_zeroes();
client->no_zeroes = true;
flags &= ~NBD_FLAG_C_NO_ZEROES;
}
if (flags != 0) {
error_setg(errp, "Unknown client flags 0x%" PRIx32 " received", flags);
- return -EIO;
+ return -EINVAL;
}
while (1) {
diff --git a/nbd/trace-events b/nbd/trace-events
index e61feda..765f5f4 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -35,9 +35,7 @@ nbd_negotiate_handle_export_name(void) "Checking length"
nbd_negotiate_handle_export_name_request(const char *name) "Client requested export '%s'"
nbd_negotiate_handle_starttls(void) "Setting up TLS"
nbd_negotiate_handle_starttls_handshake(void) "Starting TLS handshake"
-nbd_negotiate_options_flags(void) "Checking client flags"
-nbd_negotiate_options_newstyle(void) "Client supports fixed newstyle handshake"
-nbd_negotiate_options_no_zeroes(void) "Client supports no zeroes at handshake end"
+nbd_negotiate_options_flags(uint32_t flags) "Received client flags 0x%" PRIx32
nbd_negotiate_options_check_magic(uint64_t magic) "Checking opts magic 0x%" PRIx64
nbd_negotiate_options_check_option(uint32_t option, const char *name) "Checking option 0x%" PRIx32 " (%s)"
nbd_opt_abort_reply_failed(const char *error) "Reply to NBD_OPT_ABORT request failed: %s"
--
2.9.4
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH v5 5/9] nbd: Refactor reply to NBD_OPT_EXPORT_NAME
2017-07-07 20:30 [Qemu-devel] [PATCH v5 0/9] Implement NBD_OPT_GO, block size advertisement Eric Blake
` (3 preceding siblings ...)
2017-07-07 20:30 ` [Qemu-devel] [PATCH v5 4/9] nbd: Simplify trace of client flags in negotiation Eric Blake
@ 2017-07-07 20:30 ` Eric Blake
2017-07-13 14:52 ` Eric Blake
2017-07-07 20:30 ` [Qemu-devel] [PATCH v5 6/9] nbd: Implement NBD_OPT_GO on server Eric Blake
` (4 subsequent siblings)
9 siblings, 1 reply; 28+ messages in thread
From: Eric Blake @ 2017-07-07 20:30 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, vsementsov, den, marcandre.lureau, qemu-block
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>
---
v5: new patch
---
nbd/server.c | 50 ++++++++++++++++++++++++++++----------------------
1 file changed, 28 insertions(+), 22 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index 8e363d3..841986d 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -84,7 +84,6 @@ struct NBDClient {
int refcount;
void (*close_fn)(NBDClient *client, bool negotiated);
- bool no_zeroes;
NBDExport *exp;
QCryptoTLSCreds *tlscreds;
char *tlsaclname;
@@ -277,9 +276,13 @@ static int nbd_negotiate_handle_list(NBDClient *client, uint32_t length,
}
static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length,
+ uint16_t myflags, bool no_zeroes,
Error **errp)
{
char name[NBD_MAX_NAME_SIZE + 1];
+ char buf[8 + 4 + 124] = "";
+ size_t len;
+ int ret;
/* Client sends:
[20 .. xx] export name (length bytes)
@@ -303,6 +306,17 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length,
return -EINVAL;
}
+ trace_nbd_negotiate_new_style_size_flags(client->exp->size,
+ client->exp->nbdflags | myflags);
+ stq_be_p(buf, client->exp->size);
+ stw_be_p(buf + 8, client->exp->nbdflags | myflags);
+ len = no_zeroes ? 10 : sizeof(buf);
+ ret = nbd_write(client->ioc, buf, len, errp);
+ if (ret < 0) {
+ error_prepend(errp, "write failed: ");
+ return ret;
+ }
+
QTAILQ_INSERT_TAIL(&client->exp->clients, client, next);
nbd_export_get(client->exp);
@@ -373,14 +387,17 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
* 1 if client sent NBD_OPT_ABORT, i.e. on valid disconnect,
* errp is not set
*/
-static int nbd_negotiate_options(NBDClient *client, Error **errp)
+static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
+ Error **errp)
{
uint32_t flags;
bool fixedNewstyle = false;
+ bool no_zeroes = false;
/* Client sends:
[ 0 .. 3] client flags
+ Then we loop until NBD_OPT_EXPORT_NAME:
[ 0 .. 7] NBD_OPTS_MAGIC
[ 8 .. 11] NBD option
[12 .. 15] Data length
@@ -403,7 +420,7 @@ static int nbd_negotiate_options(NBDClient *client, Error **errp)
flags &= ~NBD_FLAG_C_FIXED_NEWSTYLE;
}
if (flags & NBD_FLAG_C_NO_ZEROES) {
- client->no_zeroes = true;
+ no_zeroes = true;
flags &= ~NBD_FLAG_C_NO_ZEROES;
}
if (flags != 0) {
@@ -503,7 +520,9 @@ static int nbd_negotiate_options(NBDClient *client, Error **errp)
return 1;
case NBD_OPT_EXPORT_NAME:
- return nbd_negotiate_handle_export_name(client, length, errp);
+ return nbd_negotiate_handle_export_name(client, length,
+ myflags, no_zeroes,
+ errp);
case NBD_OPT_STARTTLS:
if (nbd_drop(client->ioc, length, errp) < 0) {
@@ -546,7 +565,9 @@ static int nbd_negotiate_options(NBDClient *client, Error **errp)
*/
switch (option) {
case NBD_OPT_EXPORT_NAME:
- return nbd_negotiate_handle_export_name(client, length, errp);
+ return nbd_negotiate_handle_export_name(client, length,
+ myflags, no_zeroes,
+ errp);
default:
error_setg(errp, "Unsupported option 0x%" PRIx32 " (%s)",
@@ -572,7 +593,6 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA |
NBD_FLAG_SEND_WRITE_ZEROES);
bool oldStyle;
- size_t len;
/* Old style negotiation header without options
[ 0 .. 7] passwd ("NBDMAGIC")
@@ -586,10 +606,7 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
[ 0 .. 7] passwd ("NBDMAGIC")
[ 8 .. 15] magic (NBD_OPTS_MAGIC)
[16 .. 17] server flags (0)
- ....options sent....
- [18 .. 25] size
- [26 .. 27] export flags
- [28 .. 151] reserved (0, omit if no_zeroes)
+ ....options sent, ending in NBD_OPT_EXPORT_NAME....
*/
qio_channel_set_blocking(client->ioc, false, NULL);
@@ -618,24 +635,13 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
error_prepend(errp, "write failed: ");
return -EINVAL;
}
- ret = nbd_negotiate_options(client, errp);
+ ret = nbd_negotiate_options(client, myflags, errp);
if (ret != 0) {
if (ret < 0) {
error_prepend(errp, "option negotiation failed: ");
}
return ret;
}
-
- trace_nbd_negotiate_new_style_size_flags(
- client->exp->size, client->exp->nbdflags | myflags);
- stq_be_p(buf + 18, client->exp->size);
- stw_be_p(buf + 26, client->exp->nbdflags | myflags);
- len = client->no_zeroes ? 10 : sizeof(buf) - 18;
- ret = nbd_write(client->ioc, buf + 18, len, errp);
- if (ret < 0) {
- error_prepend(errp, "write failed: ");
- return ret;
- }
}
trace_nbd_negotiate_success();
--
2.9.4
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v5 5/9] nbd: Refactor reply to NBD_OPT_EXPORT_NAME
2017-07-07 20:30 ` [Qemu-devel] [PATCH v5 5/9] nbd: Refactor reply to NBD_OPT_EXPORT_NAME Eric Blake
@ 2017-07-13 14:52 ` Eric Blake
0 siblings, 0 replies; 28+ messages in thread
From: Eric Blake @ 2017-07-13 14:52 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, vsementsov, den, qemu-block, marcandre.lureau
[-- Attachment #1: Type: text/plain, Size: 1518 bytes --]
On 07/07/2017 03:30 PM, Eric Blake wrote:
> 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>
>
> ---
> v5: new patch
> ---
> static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length,
> + uint16_t myflags, bool no_zeroes,
> Error **errp)
> {
> char name[NBD_MAX_NAME_SIZE + 1];
> + char buf[8 + 4 + 124] = "";
Ouch, this is sized 2 bytes too large (it was copying from old-style
negotiation, which sends 4 bytes instead of 2 for the flags after the name).
> + trace_nbd_negotiate_new_style_size_flags(client->exp->size,
> + client->exp->nbdflags | myflags);
> + stq_be_p(buf, client->exp->size);
> + stw_be_p(buf + 8, client->exp->nbdflags | myflags);
> + len = no_zeroes ? 10 : sizeof(buf);
> + ret = nbd_write(client->ioc, buf, len, errp);
which means we are breaking things by sending too much.
I'll submit the followup patch shortly, assuming Paolo's v1 pull request
doesn't get held up for any other reason.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v5 6/9] nbd: Implement NBD_OPT_GO on server
2017-07-07 20:30 [Qemu-devel] [PATCH v5 0/9] Implement NBD_OPT_GO, block size advertisement Eric Blake
` (4 preceding siblings ...)
2017-07-07 20:30 ` [Qemu-devel] [PATCH v5 5/9] nbd: Refactor reply to NBD_OPT_EXPORT_NAME Eric Blake
@ 2017-07-07 20:30 ` Eric Blake
2017-07-07 20:42 ` [Qemu-devel] [Qemu-block] " Eric Blake
2017-07-07 20:30 ` [Qemu-devel] [PATCH v5 7/9] nbd: Implement NBD_OPT_GO on client Eric Blake
` (3 subsequent siblings)
9 siblings, 1 reply; 28+ messages in thread
From: Eric Blake @ 2017-07-07 20:30 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, vsementsov, den, marcandre.lureau, qemu-block
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>
---
v5: update to master, R-b dropped
v4: revamp to another round of NBD protocol changes
v3: revamp to match latest version of NBD protocol
---
nbd/server.c | 179 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
nbd/trace-events | 3 +
2 files changed, 179 insertions(+), 3 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index 841986d..e84d012 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -141,6 +141,7 @@ static int nbd_negotiate_send_rep_len(QIOChannel *ioc, uint32_t type,
trace_nbd_negotiate_send_rep_len(opt, nbd_opt_lookup(opt),
type, nbd_rep_lookup(type), len);
+ assert(len < NBD_MAX_BUFFER_SIZE);
magic = cpu_to_be64(NBD_REP_MAGIC);
if (nbd_write(ioc, &magic, sizeof(magic), errp) < 0) {
error_prepend(errp, "write failed (rep magic): ");
@@ -275,6 +276,8 @@ static int nbd_negotiate_handle_list(NBDClient *client, uint32_t length,
return nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, NBD_OPT_LIST, errp);
}
+/* Send a reply to NBD_OPT_EXPORT_NAME.
+ * Return -errno on error, 0 on success. */
static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length,
uint16_t myflags, bool no_zeroes,
Error **errp)
@@ -323,6 +326,162 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length,
return 0;
}
+/* Send a single NBD_REP_INFO, with a buffer @buf of @length bytes.
+ * The buffer does NOT include the info type prefix.
+ * Return -errno on error, 0 if ready to send more. */
+static int nbd_negotiate_send_info(NBDClient *client, uint32_t opt,
+ uint16_t info, uint32_t length, void *buf,
+ Error **errp)
+{
+ int rc;
+
+ trace_nbd_negotiate_send_info(info, nbd_info_lookup(info), length);
+ rc = nbd_negotiate_send_rep_len(client->ioc, NBD_REP_INFO, opt,
+ sizeof(info) + length, errp);
+ if (rc < 0) {
+ return rc;
+ }
+ cpu_to_be16s(&info);
+ if (nbd_write(client->ioc, &info, sizeof(info), errp) < 0) {
+ return -EIO;
+ }
+ if (nbd_write(client->ioc, buf, length, errp) < 0) {
+ return -EIO;
+ }
+ return 0;
+}
+
+/* Handle NBD_OPT_INFO and NBD_OPT_GO.
+ * Return -errno on error, 0 if ready for next option, and 1 to move
+ * into transmission phase. */
+static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length,
+ uint32_t opt, uint16_t myflags,
+ Error **errp)
+{
+ int rc;
+ char name[NBD_MAX_NAME_SIZE + 1];
+ NBDExport *exp;
+ uint16_t requests;
+ uint16_t request;
+ uint32_t namelen;
+ bool sendname = false;
+ char buf[sizeof(uint64_t) + sizeof(uint16_t)];
+ const char *msg;
+
+ /* Client sends:
+ 4 bytes: L, name length (can be 0)
+ L bytes: export name
+ 2 bytes: N, number of requests (can be 0)
+ N * 2 bytes: N requests
+ */
+ if (length < sizeof(namelen) + sizeof(requests)) {
+ msg = "overall request too short";
+ goto invalid;
+ }
+ if (nbd_read(client->ioc, &namelen, sizeof(namelen), errp) < 0) {
+ return -EIO;
+ }
+ be32_to_cpus(&namelen);
+ length -= sizeof(namelen);
+ if (namelen > length - sizeof(requests) || (length - namelen) % 2) {
+ msg = "name length is incorrect";
+ goto invalid;
+ }
+ if (nbd_read(client->ioc, name, namelen, errp) < 0) {
+ return -EIO;
+ }
+ name[namelen] = '\0';
+ length -= namelen;
+ trace_nbd_negotiate_handle_export_name_request(name);
+
+ if (nbd_read(client->ioc, &requests, sizeof(requests), errp) < 0) {
+ return -EIO;
+ }
+ be16_to_cpus(&requests);
+ length -= sizeof(requests);
+ trace_nbd_negotiate_handle_info_requests(requests);
+ if (requests != length / sizeof(request)) {
+ msg = "incorrect number of requests for overall length";
+ goto invalid;
+ }
+ while (requests--) {
+ if (nbd_read(client->ioc, &request, sizeof(request), errp) < 0) {
+ return -EIO;
+ }
+ be16_to_cpus(&request);
+ length -= sizeof(request);
+ trace_nbd_negotiate_handle_info_request(request,
+ nbd_info_lookup(request));
+ /* For now, we only care about NBD_INFO_NAME; everything else
+ * is either a request we don't know or something we send
+ * regardless of request. */
+ if (request == NBD_INFO_NAME) {
+ sendname = true;
+ }
+ }
+
+ exp = nbd_export_find(name);
+ if (!exp) {
+ return nbd_negotiate_send_rep_err(client->ioc, NBD_REP_ERR_UNKNOWN,
+ opt, errp, "export '%s' not present",
+ name);
+ }
+
+ /* Don't bother sending NBD_INFO_NAME unless client requested it */
+ if (sendname) {
+ rc = nbd_negotiate_send_info(client, opt, NBD_INFO_NAME, length, name,
+ errp);
+ if (rc < 0) {
+ return rc;
+ }
+ }
+
+ /* Send NBD_INFO_DESCRIPTION only if available, regardless of
+ * client request */
+ if (exp->description) {
+ size_t len = strlen(exp->description);
+
+ rc = nbd_negotiate_send_info(client, opt, NBD_INFO_DESCRIPTION,
+ len, exp->description, errp);
+ if (rc < 0) {
+ return rc;
+ }
+ }
+
+ /* Send NBD_INFO_EXPORT always */
+ trace_nbd_negotiate_new_style_size_flags(exp->size,
+ exp->nbdflags | myflags);
+ stq_be_p(buf, exp->size);
+ stw_be_p(buf + 8, exp->nbdflags | myflags);
+ rc = nbd_negotiate_send_info(client, opt, NBD_INFO_EXPORT,
+ sizeof(buf), buf, errp);
+ if (rc < 0) {
+ return rc;
+ }
+
+ /* Final reply */
+ rc = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, opt, errp);
+ if (rc < 0) {
+ return rc;
+ }
+
+ if (opt == NBD_OPT_GO) {
+ client->exp = exp;
+ QTAILQ_INSERT_TAIL(&client->exp->clients, client, next);
+ nbd_export_get(client->exp);
+ rc = 1;
+ }
+ return rc;
+
+ invalid:
+ if (nbd_drop(client->ioc, length, errp) < 0) {
+ return -EIO;
+ }
+ return nbd_negotiate_send_rep_err(client->ioc, NBD_REP_ERR_INVALID, opt,
+ errp, "%s", msg);
+}
+
+
/* Handle NBD_OPT_STARTTLS. Return NULL to drop connection, or else the
* new channel for all further (now-encrypted) communication. */
static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
@@ -380,7 +539,8 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
}
/* nbd_negotiate_options
- * Process all NBD_OPT_* client option commands.
+ * Process all NBD_OPT_* client option commands, during fixed newstyle
+ * negotiation.
* Return:
* -errno on error, errp is set
* 0 on successful negotiation, errp is not set
@@ -397,7 +557,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
/* Client sends:
[ 0 .. 3] client flags
- Then we loop until NBD_OPT_EXPORT_NAME:
+ Then we loop until NBD_OPT_EXPORT_NAME or NBD_OPT_GO:
[ 0 .. 7] NBD_OPTS_MAGIC
[ 8 .. 11] NBD option
[12 .. 15] Data length
@@ -524,6 +684,19 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
myflags, no_zeroes,
errp);
+ case NBD_OPT_INFO:
+ case NBD_OPT_GO:
+ ret = nbd_negotiate_handle_info(client, length, option,
+ myflags, errp);
+ if (ret == 1) {
+ assert(option == NBD_OPT_GO);
+ return 0;
+ }
+ if (ret) {
+ return ret;
+ }
+ break;
+
case NBD_OPT_STARTTLS:
if (nbd_drop(client->ioc, length, errp) < 0) {
return -EIO;
@@ -606,7 +779,7 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
[ 0 .. 7] passwd ("NBDMAGIC")
[ 8 .. 15] magic (NBD_OPTS_MAGIC)
[16 .. 17] server flags (0)
- ....options sent, ending in NBD_OPT_EXPORT_NAME....
+ ....options sent, ending in NBD_OPT_EXPORT_NAME or NBD_OPT_GO....
*/
qio_channel_set_blocking(client->ioc, false, NULL);
diff --git a/nbd/trace-events b/nbd/trace-events
index 765f5f4..80c2447 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -33,6 +33,9 @@ nbd_negotiate_send_rep_err(const char *msg) "sending error message \"%s\""
nbd_negotiate_send_rep_list(const char *name, const char *desc) "Advertising export name '%s' description '%s'"
nbd_negotiate_handle_export_name(void) "Checking length"
nbd_negotiate_handle_export_name_request(const char *name) "Client requested export '%s'"
+nbd_negotiate_send_info(int info, const char *name, uint32_t length) "Sending NBD_REP_INFO type %d (%s) with remaining length %" PRIu32
+nbd_negotiate_handle_info_requests(int requests) "Client requested %d items of info"
+nbd_negotiate_handle_info_request(int request, const char *name) "Client requested info %d (%s)"
nbd_negotiate_handle_starttls(void) "Setting up TLS"
nbd_negotiate_handle_starttls_handshake(void) "Starting TLS handshake"
nbd_negotiate_options_flags(uint32_t flags) "Received client flags 0x%" PRIx32
--
2.9.4
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [Qemu-block] [PATCH v5 6/9] nbd: Implement NBD_OPT_GO on server
2017-07-07 20:30 ` [Qemu-devel] [PATCH v5 6/9] nbd: Implement NBD_OPT_GO on server Eric Blake
@ 2017-07-07 20:42 ` Eric Blake
0 siblings, 0 replies; 28+ messages in thread
From: Eric Blake @ 2017-07-07 20:42 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, vsementsov, den, qemu-block, marcandre.lureau
[-- Attachment #1: Type: text/plain, Size: 832 bytes --]
On 07/07/2017 03:30 PM, Eric Blake wrote:
> 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
Unless review turns up a reason for me to respin, I will update this URL
to the point where the extension was merged into mainline NBD:
https://github.com/NetworkBlockDevice/nbd/commit/e6b56c12f8
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v5 7/9] nbd: Implement NBD_OPT_GO on client
2017-07-07 20:30 [Qemu-devel] [PATCH v5 0/9] Implement NBD_OPT_GO, block size advertisement Eric Blake
` (5 preceding siblings ...)
2017-07-07 20:30 ` [Qemu-devel] [PATCH v5 6/9] nbd: Implement NBD_OPT_GO on server Eric Blake
@ 2017-07-07 20:30 ` Eric Blake
2017-07-17 8:31 ` [Qemu-devel] [Qemu-block] " Kevin Wolf
2017-07-19 16:43 ` [Qemu-devel] " Vladimir Sementsov-Ogievskiy
2017-07-07 20:30 ` [Qemu-devel] [PATCH v5 8/9] nbd: Implement NBD_INFO_BLOCK_SIZE on server Eric Blake
` (2 subsequent siblings)
9 siblings, 2 replies; 28+ messages in thread
From: Eric Blake @ 2017-07-07 20:30 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, vsementsov, den, marcandre.lureau, qemu-block
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>
---
v5: rebase to master; enough changes to drop R-b
v4: NBD protocol changes, again
v3: revamp to match latest version of NBD protocol
---
nbd/nbd-internal.h | 3 ++
nbd/client.c | 126 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
nbd/trace-events | 3 ++
3 files changed, 130 insertions(+), 2 deletions(-)
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index bf95601..4065bc6 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -37,8 +37,11 @@
* https://github.com/yoe/nbd/blob/master/doc/proto.md
*/
+/* Size of all NBD_OPT_*, without payload */
#define NBD_REQUEST_SIZE (4 + 2 + 2 + 8 + 8 + 4)
+/* Size of all NBD_REP_* sent in answer to most NBD_OPT_*, without payload */
#define NBD_REPLY_SIZE (4 + 4 + 8)
+
#define NBD_REQUEST_MAGIC 0x25609513
#define NBD_REPLY_MAGIC 0x67446698
#define NBD_OPTS_MAGIC 0x49484156454F5054LL
diff --git a/nbd/client.c b/nbd/client.c
index 011960b..cb55f3d 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -350,6 +350,114 @@ static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match,
}
+/* Returns -1 if NBD_OPT_GO proves the export @wantname cannot be
+ * used, 0 if NBD_OPT_GO is unsupported (fall back to NBD_OPT_LIST and
+ * NBD_OPT_EXPORT_NAME in that case), and > 0 if the export is good to
+ * go (with @info populated). */
+static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
+ NBDExportInfo *info, Error **errp)
+{
+ nbd_opt_reply reply;
+ uint32_t len = strlen(wantname);
+ uint16_t type;
+ int error;
+ char *buf;
+
+ /* The protocol requires that the server send NBD_INFO_EXPORT with
+ * a non-zero flags (at least NBD_FLAG_HAS_FLAGS must be set); so
+ * flags still 0 is a witness of a broken server. */
+ info->flags = 0;
+
+ trace_nbd_opt_go_start(wantname);
+ buf = g_malloc(4 + len + 2 + 1);
+ stl_be_p(buf, len);
+ memcpy(buf + 4, wantname, len);
+ /* No requests, live with whatever server sends */
+ stw_be_p(buf + 4 + len, 0);
+ if (nbd_send_option_request(ioc, NBD_OPT_GO, len + 6, buf, errp) < 0) {
+ return -1;
+ }
+
+ while (1) {
+ if (nbd_receive_option_reply(ioc, NBD_OPT_GO, &reply, errp) < 0) {
+ return -1;
+ }
+ error = nbd_handle_reply_err(ioc, &reply, errp);
+ if (error <= 0) {
+ return error;
+ }
+ len = reply.length;
+
+ if (reply.type == NBD_REP_ACK) {
+ /* Server is done sending info and moved into transmission
+ phase, but make sure it sent flags */
+ if (len) {
+ error_setg(errp, "server sent invalid NBD_REP_ACK");
+ nbd_send_opt_abort(ioc);
+ return -1;
+ }
+ if (!info->flags) {
+ error_setg(errp, "broken server omitted NBD_INFO_EXPORT");
+ nbd_send_opt_abort(ioc);
+ return -1;
+ }
+ trace_nbd_opt_go_success();
+ return 1;
+ }
+ if (reply.type != NBD_REP_INFO) {
+ error_setg(errp, "unexpected reply type %" PRIx32 ", expected %x",
+ reply.type, NBD_REP_INFO);
+ nbd_send_opt_abort(ioc);
+ return -1;
+ }
+ if (len < sizeof(type)) {
+ error_setg(errp, "NBD_REP_INFO length %" PRIu32 " is too short",
+ len);
+ nbd_send_opt_abort(ioc);
+ return -1;
+ }
+ if (nbd_read(ioc, &type, sizeof(type), errp) < 0) {
+ error_prepend(errp, "failed to read info type");
+ nbd_send_opt_abort(ioc);
+ return -1;
+ }
+ len -= sizeof(type);
+ be16_to_cpus(&type);
+ switch (type) {
+ case NBD_INFO_EXPORT:
+ if (len != sizeof(info->size) + sizeof(info->flags)) {
+ error_setg(errp, "remaining export info len %" PRIu32
+ " is unexpected size", len);
+ nbd_send_opt_abort(ioc);
+ return -1;
+ }
+ if (nbd_read(ioc, &info->size, sizeof(info->size), errp) < 0) {
+ error_prepend(errp, "failed to read info size");
+ nbd_send_opt_abort(ioc);
+ return -1;
+ }
+ be64_to_cpus(&info->size);
+ if (nbd_read(ioc, &info->flags, sizeof(info->flags), errp) < 0) {
+ error_prepend(errp, "failed to read info flags");
+ nbd_send_opt_abort(ioc);
+ return -1;
+ }
+ be16_to_cpus(&info->flags);
+ trace_nbd_receive_negotiate_size_flags(info->size, info->flags);
+ break;
+
+ default:
+ trace_nbd_opt_go_info_unknown(type, nbd_info_lookup(type));
+ if (nbd_drop(ioc, len, errp) < 0) {
+ error_prepend(errp, "Failed to read info payload");
+ nbd_send_opt_abort(ioc);
+ return -1;
+ }
+ break;
+ }
+ }
+}
+
/* Return -1 on failure, 0 if wantname is an available export. */
static int nbd_receive_query_exports(QIOChannel *ioc,
const char *wantname,
@@ -531,11 +639,25 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
name = "";
}
if (fixedNewStyle) {
+ int result;
+
+ /* Try NBD_OPT_GO first - if it works, we are done (it
+ * also gives us a good message if the server requires
+ * TLS). If it is not available, fall back to
+ * NBD_OPT_LIST for nicer error messages about a missing
+ * export, then use NBD_OPT_EXPORT_NAME. */
+ result = nbd_opt_go(ioc, name, info, errp);
+ if (result < 0) {
+ goto fail;
+ }
+ if (result > 0) {
+ return 0;
+ }
/* Check our desired export is present in the
* server export list. Since NBD_OPT_EXPORT_NAME
* cannot return an error message, running this
- * query gives us good error reporting if the
- * server required TLS
+ * query gives us better error reporting if the
+ * export name is not available.
*/
if (nbd_receive_query_exports(ioc, name, errp) < 0) {
goto fail;
diff --git a/nbd/trace-events b/nbd/trace-events
index 80c2447..4969047 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -3,6 +3,9 @@ nbd_unknown_error(int err) "Squashing unexpected error %d to EINVAL"
nbd_send_option_request(uint32_t opt, const char *name, uint32_t len) "Sending option request %" PRIu32" (%s), len %" PRIu32
nbd_receive_option_reply(uint32_t option, const char *optname, uint32_t type, const char *typename, uint32_t length) "Received option reply %" PRIx32" (%s), type %" PRIx32" (%s), len %" PRIu32
nbd_reply_err_unsup(uint32_t option, const char *name) "server doesn't understand request %" PRIx32 " (%s), attempting fallback"
+nbd_opt_go_start(const char *name) "Attempting NBD_OPT_GO for export '%s'"
+nbd_opt_go_success(void) "Export is good to go"
+nbd_opt_go_info_unknown(int info, const char *name) "Ignoring unknown info %d (%s)"
nbd_receive_query_exports_start(const char *wantname) "Querying export list for '%s'"
nbd_receive_query_exports_success(const char *wantname) "Found desired export name '%s'"
nbd_receive_starttls_request(void) "Requesting TLS from server"
--
2.9.4
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [Qemu-block] [PATCH v5 7/9] nbd: Implement NBD_OPT_GO on client
2017-07-07 20:30 ` [Qemu-devel] [PATCH v5 7/9] nbd: Implement NBD_OPT_GO on client Eric Blake
@ 2017-07-17 8:31 ` Kevin Wolf
2017-07-17 11:41 ` Eric Blake
2017-07-19 16:43 ` [Qemu-devel] " Vladimir Sementsov-Ogievskiy
1 sibling, 1 reply; 28+ messages in thread
From: Kevin Wolf @ 2017-07-17 8:31 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, pbonzini, vsementsov, den, qemu-block,
marcandre.lureau
Am 07.07.2017 um 22:30 hat Eric Blake geschrieben:
> 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>
This breaks qemu-iotests 140 and 143:
-can't open device nbd+unix:///drv?socket=TEST_DIR/nbd: No export with name 'drv' available
+can't open device nbd+unix:///drv?socket=TEST_DIR/nbd: Requested export not available for option 7 (go)
+export 'drv' not present
We could just update the reference output, but I actually believe the
old error message was better.
Kevin
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v5 7/9] nbd: Implement NBD_OPT_GO on client
2017-07-17 8:31 ` [Qemu-devel] [Qemu-block] " Kevin Wolf
@ 2017-07-17 11:41 ` Eric Blake
2017-07-17 13:52 ` Eric Blake
0 siblings, 1 reply; 28+ messages in thread
From: Eric Blake @ 2017-07-17 11:41 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-devel, pbonzini, vsementsov, den, qemu-block,
marcandre.lureau
[-- Attachment #1: Type: text/plain, Size: 1494 bytes --]
On 07/17/2017 03:31 AM, Kevin Wolf wrote:
> Am 07.07.2017 um 22:30 hat Eric Blake geschrieben:
>> 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.
>>
>
> This breaks qemu-iotests 140 and 143:
Urrgh, and I even ran into it locally last week after posting my v1, and
had it on my list to fix before posting v2, when Paolo committed my v1
to make softfreeze before going on PTO this week. I will post the fix
later today (I already have a couple other NBD patches pending, so my
plan is to submit a PULL request by the end of my day with all of the
fixes).
>
> -can't open device nbd+unix:///drv?socket=TEST_DIR/nbd: No export with name 'drv' available
> +can't open device nbd+unix:///drv?socket=TEST_DIR/nbd: Requested export not available for option 7 (go)
> +export 'drv' not present
>
> We could just update the reference output, but I actually believe the
> old error message was better.
One line is indeed better than two; I'm still playing with the easiest
way to get the desired output.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v5 7/9] nbd: Implement NBD_OPT_GO on client
2017-07-17 11:41 ` Eric Blake
@ 2017-07-17 13:52 ` Eric Blake
0 siblings, 0 replies; 28+ messages in thread
From: Eric Blake @ 2017-07-17 13:52 UTC (permalink / raw)
To: Kevin Wolf
Cc: vsementsov, den, qemu-block, qemu-devel, marcandre.lureau,
pbonzini
[-- Attachment #1: Type: text/plain, Size: 1137 bytes --]
On 07/17/2017 06:41 AM, Eric Blake wrote:
>> This breaks qemu-iotests 140 and 143:
>
>> -can't open device nbd+unix:///drv?socket=TEST_DIR/nbd: No export with name 'drv' available
>> +can't open device nbd+unix:///drv?socket=TEST_DIR/nbd: Requested export not available for option 7 (go)
>> +export 'drv' not present
>>
>> We could just update the reference output, but I actually believe the
>> old error message was better.
>
> One line is indeed better than two; I'm still playing with the easiest
> way to get the desired output.
The fact that output is two lines is because the client is now replaying
the server's error message (something that was not previously possible,
prior to NBD_OPT_GO). Dropping that information may not hurt in this
case, but it's nice to show the server's message when one is present.
Maybe I can change this to:
can't open device nbd+unix:///drv?socket=TEST_DIR/nbd: Requested export
not available
server reported: export 'drv' not present
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v5 7/9] nbd: Implement NBD_OPT_GO on client
2017-07-07 20:30 ` [Qemu-devel] [PATCH v5 7/9] nbd: Implement NBD_OPT_GO on client Eric Blake
2017-07-17 8:31 ` [Qemu-devel] [Qemu-block] " Kevin Wolf
@ 2017-07-19 16:43 ` Vladimir Sementsov-Ogievskiy
2017-07-19 17:12 ` Eric Blake
1 sibling, 1 reply; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-07-19 16:43 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: pbonzini, den, marcandre.lureau, qemu-block
07.07.2017 23:30, Eric Blake wrote:
[..]
>
> +/* Returns -1 if NBD_OPT_GO proves the export @wantname cannot be
> + * used, 0 if NBD_OPT_GO is unsupported (fall back to NBD_OPT_LIST and
> + * NBD_OPT_EXPORT_NAME in that case), and > 0 if the export is good to
> + * go (with @info populated). */
> +static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
> + NBDExportInfo *info, Error **errp)
> +{
> + nbd_opt_reply reply;
> + uint32_t len = strlen(wantname);
> + uint16_t type;
> + int error;
> + char *buf;
> +
> + /* The protocol requires that the server send NBD_INFO_EXPORT with
> + * a non-zero flags (at least NBD_FLAG_HAS_FLAGS must be set); so
> + * flags still 0 is a witness of a broken server. */
> + info->flags = 0;
> +
> + trace_nbd_opt_go_start(wantname);
> + buf = g_malloc(4 + len + 2 + 1);
> + stl_be_p(buf, len);
> + memcpy(buf + 4, wantname, len);
> + /* No requests, live with whatever server sends */
> + stw_be_p(buf + 4 + len, 0);
> + if (nbd_send_option_request(ioc, NBD_OPT_GO, len + 6, buf, errp) < 0) {
> + return -1;
> + }
> +
> + while (1) {
> + if (nbd_receive_option_reply(ioc, NBD_OPT_GO, &reply, errp) < 0) {
> + return -1;
> + }
> + error = nbd_handle_reply_err(ioc, &reply, errp);
> + if (error <= 0) {
> + return error;
> + }
> + len = reply.length;
> +
> + if (reply.type == NBD_REP_ACK) {
> + /* Server is done sending info and moved into transmission
> + phase, but make sure it sent flags */
> + if (len) {
> + error_setg(errp, "server sent invalid NBD_REP_ACK");
> + nbd_send_opt_abort(ioc);
server is already in transmission phase, so we cant send any options
more, shouldn't CMD_DISC be send here instead?
> + return -1;
> + }
> + if (!info->flags) {
> + error_setg(errp, "broken server omitted NBD_INFO_EXPORT");
> + nbd_send_opt_abort(ioc);
and here
> + return -1;
> + }
> + trace_nbd_opt_go_success();
> + return 1;
> + }
> + if (reply.type != NBD_REP_INFO) {
> + error_setg(errp, "unexpected reply type %" PRIx32 ", expected %x",
> + reply.type, NBD_REP_INFO);
> + nbd_send_opt_abort(ioc);
> + return -1;
> + }
> + if (len < sizeof(type)) {
> + error_setg(errp, "
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v5 7/9] nbd: Implement NBD_OPT_GO on client
2017-07-19 16:43 ` [Qemu-devel] " Vladimir Sementsov-Ogievskiy
@ 2017-07-19 17:12 ` Eric Blake
2017-07-19 17:50 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 28+ messages in thread
From: Eric Blake @ 2017-07-19 17:12 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel
Cc: pbonzini, den, marcandre.lureau, qemu-block
[-- Attachment #1: Type: text/plain, Size: 2271 bytes --]
On 07/19/2017 11:43 AM, Vladimir Sementsov-Ogievskiy wrote:
> 07.07.2017 23:30, Eric Blake wrote:
>
> [..]
>
>>
>> +/* Returns -1 if NBD_OPT_GO proves the export @wantname cannot be
>> + * used, 0 if NBD_OPT_GO is unsupported (fall back to NBD_OPT_LIST and
>> + * NBD_OPT_EXPORT_NAME in that case), and > 0 if the export is good to
>> + * go (with @info populated). */
>> +static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
>> + NBDExportInfo *info, Error **errp)
>> +{
>> + if (reply.type == NBD_REP_ACK) {
>> + /* Server is done sending info and moved into transmission
>> + phase, but make sure it sent flags */
>> + if (len) {
>> + error_setg(errp, "server sent invalid NBD_REP_ACK");
>> + nbd_send_opt_abort(ioc);
>
> server is already in transmission phase, so we cant send any options
> more, shouldn't CMD_DISC be send here instead?
>
>> + return -1;
>> + }
>> + if (!info->flags) {
>> + error_setg(errp, "broken server omitted
>> NBD_INFO_EXPORT");
>> + nbd_send_opt_abort(ioc);
>
> and here
For NBD_OPT_INFO, receipt of NBD_REP_ACK implies the server is still in
negotiation phase; transmission phases is only technically possible on
NBD_OPT_GO. That said, either error condition means the server is
buggy, so it really doesn't matter what we do in response (we don't know
if the server moved on to transmission phase or not, because it has
already broken protocol by sending us garbage) - so trying to be
courteous to tell the server we don't like their garbage vs. just
silently disconnecting makes no real difference; even if the server
doesn't like what we send (because it thinks we are out of phase), the
server already broke protocol first. Either way, we're going to
disconnect, and the server has to mop up after its own bugs.
I don't think a followup patch is essential, but I'd also be okay with a
patch that just eliminates the NBD_OPT_ABORT attempt and does a silent
disconnect instead.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v5 7/9] nbd: Implement NBD_OPT_GO on client
2017-07-19 17:12 ` Eric Blake
@ 2017-07-19 17:50 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-07-19 17:50 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: pbonzini, den, marcandre.lureau, qemu-block
19.07.2017 20:12, Eric Blake wrote:
> On 07/19/2017 11:43 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 07.07.2017 23:30, Eric Blake wrote:
>>
>> [..]
>>
>>> +/* Returns -1 if NBD_OPT_GO proves the export @wantname cannot be
>>> + * used, 0 if NBD_OPT_GO is unsupported (fall back to NBD_OPT_LIST and
>>> + * NBD_OPT_EXPORT_NAME in that case), and > 0 if the export is good to
>>> + * go (with @info populated). */
>>> +static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
>>> + NBDExportInfo *info, Error **errp)
>>> +{
>>> + if (reply.type == NBD_REP_ACK) {
>>> + /* Server is done sending info and moved into transmission
>>> + phase, but make sure it sent flags */
>>> + if (len) {
>>> + error_setg(errp, "server sent invalid NBD_REP_ACK");
>>> + nbd_send_opt_abort(ioc);
>> server is already in transmission phase, so we cant send any options
>> more, shouldn't CMD_DISC be send here instead?
>>
>>> + return -1;
>>> + }
>>> + if (!info->flags) {
>>> + error_setg(errp, "broken server omitted
>>> NBD_INFO_EXPORT");
>>> + nbd_send_opt_abort(ioc);
>> and here
> For NBD_OPT_INFO, receipt of NBD_REP_ACK implies the server is still in
hmm, nothing here about NBD_OPT_INFO.
> negotiation phase; transmission phases is only technically possible on
> NBD_OPT_GO. That said, either error condition means the server is
> buggy, so it really doesn't matter what we do in response (we don't know
> if the server moved on to transmission phase or not, because it has
> already broken protocol by sending us garbage) - so trying to be
> courteous to tell the server we don't like their garbage vs. just
> silently disconnecting makes no real difference; even if the server
> doesn't like what we send (because it thinks we are out of phase), the
> server already broke protocol first. Either way, we're going to
> disconnect, and the server has to mop up after its own bugs.
agree
>
> I don't think a followup patch is essential, but I'd also be okay with a
> patch that just eliminates the NBD_OPT_ABORT attempt and does a silent
> disconnect instead.
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v5 8/9] nbd: Implement NBD_INFO_BLOCK_SIZE on server
2017-07-07 20:30 [Qemu-devel] [PATCH v5 0/9] Implement NBD_OPT_GO, block size advertisement Eric Blake
` (6 preceding siblings ...)
2017-07-07 20:30 ` [Qemu-devel] [PATCH v5 7/9] nbd: Implement NBD_OPT_GO on client Eric Blake
@ 2017-07-07 20:30 ` Eric Blake
2017-07-07 20:30 ` [Qemu-devel] [PATCH v5 9/9] nbd: Implement NBD_INFO_BLOCK_SIZE on client Eric Blake
2017-07-13 11:38 ` [Qemu-devel] [PATCH v5 0/9] Implement NBD_OPT_GO, block size advertisement Paolo Bonzini
9 siblings, 0 replies; 28+ messages in thread
From: Eric Blake @ 2017-07-07 20:30 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, vsementsov, den, marcandre.lureau, qemu-block
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>
---
v5: rebase to master
v4: new patch
---
nbd/server.c | 48 ++++++++++++++++++++++++++++++++++++++++++++----
nbd/trace-events | 1 +
2 files changed, 45 insertions(+), 4 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index e84d012..49ed574 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -365,6 +365,8 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length,
uint16_t request;
uint32_t namelen;
bool sendname = false;
+ bool blocksize = false;
+ uint32_t sizes[3];
char buf[sizeof(uint64_t) + sizeof(uint16_t)];
const char *msg;
@@ -412,11 +414,16 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length,
length -= sizeof(request);
trace_nbd_negotiate_handle_info_request(request,
nbd_info_lookup(request));
- /* For now, we only care about NBD_INFO_NAME; everything else
- * is either a request we don't know or something we send
- * regardless of request. */
- if (request == NBD_INFO_NAME) {
+ /* We care about NBD_INFO_NAME and NBD_INFO_BLOCK_SIZE;
+ * everything else is either a request we don't know or
+ * something we send regardless of request */
+ switch (request) {
+ case NBD_INFO_NAME:
sendname = true;
+ break;
+ case NBD_INFO_BLOCK_SIZE:
+ blocksize = true;
+ break;
}
}
@@ -448,6 +455,27 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length,
}
}
+ /* Send NBD_INFO_BLOCK_SIZE always, but tweak the minimum size
+ * according to whether the client requested it, and according to
+ * whether this is OPT_INFO or OPT_GO. */
+ /* minimum - 1 for back-compat, or 512 if client is new enough.
+ * TODO: consult blk_bs(blk)->bl.request_alignment? */
+ sizes[0] = (opt == NBD_OPT_INFO || blocksize) ? BDRV_SECTOR_SIZE : 1;
+ /* preferred - Hard-code to 4096 for now.
+ * TODO: is blk_bs(blk)->bl.opt_transfer appropriate? */
+ sizes[1] = 4096;
+ /* maximum - At most 32M, but smaller as appropriate. */
+ sizes[2] = MIN(blk_get_max_transfer(exp->blk), NBD_MAX_BUFFER_SIZE);
+ trace_nbd_negotiate_handle_info_block_size(sizes[0], sizes[1], sizes[2]);
+ cpu_to_be32s(&sizes[0]);
+ cpu_to_be32s(&sizes[1]);
+ cpu_to_be32s(&sizes[2]);
+ rc = nbd_negotiate_send_info(client, opt, NBD_INFO_BLOCK_SIZE,
+ sizeof(sizes), sizes, errp);
+ if (rc < 0) {
+ return rc;
+ }
+
/* Send NBD_INFO_EXPORT always */
trace_nbd_negotiate_new_style_size_flags(exp->size,
exp->nbdflags | myflags);
@@ -459,6 +487,18 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length,
return rc;
}
+ /* If the client is just asking for NBD_OPT_INFO, but forgot to
+ * request block sizes, return an error.
+ * TODO: consult blk_bs(blk)->request_align, and only error if it
+ * is not 1? */
+ if (opt == NBD_OPT_INFO && !blocksize) {
+ return nbd_negotiate_send_rep_err(client->ioc,
+ NBD_REP_ERR_BLOCK_SIZE_REQD, opt,
+ errp,
+ "request NBD_INFO_BLOCK_SIZE to "
+ "use this export");
+ }
+
/* Final reply */
rc = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, opt, errp);
if (rc < 0) {
diff --git a/nbd/trace-events b/nbd/trace-events
index 4969047..a3ba4bc 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -39,6 +39,7 @@ nbd_negotiate_handle_export_name_request(const char *name) "Client requested exp
nbd_negotiate_send_info(int info, const char *name, uint32_t length) "Sending NBD_REP_INFO type %d (%s) with remaining length %" PRIu32
nbd_negotiate_handle_info_requests(int requests) "Client requested %d items of info"
nbd_negotiate_handle_info_request(int request, const char *name) "Client requested info %d (%s)"
+nbd_negotiate_handle_info_block_size(uint32_t minimum, uint32_t preferred, uint32_t maximum) "advertising minimum 0x%" PRIx32 ", preferred 0x%" PRIx32 ", maximum 0x%" PRIx32
nbd_negotiate_handle_starttls(void) "Setting up TLS"
nbd_negotiate_handle_starttls_handshake(void) "Starting TLS handshake"
nbd_negotiate_options_flags(uint32_t flags) "Received client flags 0x%" PRIx32
--
2.9.4
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH v5 9/9] nbd: Implement NBD_INFO_BLOCK_SIZE on client
2017-07-07 20:30 [Qemu-devel] [PATCH v5 0/9] Implement NBD_OPT_GO, block size advertisement Eric Blake
` (7 preceding siblings ...)
2017-07-07 20:30 ` [Qemu-devel] [PATCH v5 8/9] nbd: Implement NBD_INFO_BLOCK_SIZE on server Eric Blake
@ 2017-07-07 20:30 ` Eric Blake
2017-07-13 11:38 ` [Qemu-devel] [PATCH v5 0/9] Implement NBD_OPT_GO, block size advertisement Paolo Bonzini
9 siblings, 0 replies; 28+ messages in thread
From: Eric Blake @ 2017-07-07 20:30 UTC (permalink / raw)
To: qemu-devel
Cc: pbonzini, vsementsov, den, marcandre.lureau, qemu-block,
Kevin Wolf, Max Reitz
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>
---
v5: rebase to master
v4: new patch
---
include/block/nbd.h | 6 ++++
block/nbd-client.c | 4 +++
block/nbd.c | 14 +++++++--
nbd/client.c | 81 +++++++++++++++++++++++++++++++++++++++++++++--------
qemu-nbd.c | 2 +-
nbd/trace-events | 1 +
6 files changed, 92 insertions(+), 16 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 4a22eca..9c3d0a5 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -144,8 +144,14 @@ enum {
/* Details collected by NBD_OPT_EXPORT_NAME and NBD_OPT_GO */
struct NBDExportInfo {
+ /* Set by client before nbd_receive_negotiate() */
+ bool request_sizes;
+ /* Set by server results during nbd_receive_negotiate() */
uint64_t size;
uint16_t flags;
+ uint32_t min_block;
+ uint32_t opt_block;
+ uint32_t max_block;
};
typedef struct NBDExportInfo NBDExportInfo;
diff --git a/block/nbd-client.c b/block/nbd-client.c
index aab1e32..25dd284 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -384,6 +384,7 @@ int nbd_client_init(BlockDriverState *bs,
logout("session init %s\n", export);
qio_channel_set_blocking(QIO_CHANNEL(sioc), true, NULL);
+ client->info.request_sizes = true;
ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), export,
tlscreds, hostname,
&client->ioc, &client->info, errp);
@@ -398,6 +399,9 @@ int nbd_client_init(BlockDriverState *bs,
if (client->info.flags & NBD_FLAG_SEND_WRITE_ZEROES) {
bs->supported_zero_flags |= BDRV_REQ_MAY_UNMAP;
}
+ if (client->info.min_block > bs->bl.request_alignment) {
+ bs->bl.request_alignment = client->info.min_block;
+ }
qemu_co_mutex_init(&client->send_mutex);
qemu_co_queue_init(&client->free_sema);
diff --git a/block/nbd.c b/block/nbd.c
index 4a9048c..a50d24b 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -472,9 +472,17 @@ static int nbd_co_flush(BlockDriverState *bs)
static void nbd_refresh_limits(BlockDriverState *bs, Error **errp)
{
- bs->bl.max_pdiscard = NBD_MAX_BUFFER_SIZE;
- bs->bl.max_pwrite_zeroes = NBD_MAX_BUFFER_SIZE;
- bs->bl.max_transfer = NBD_MAX_BUFFER_SIZE;
+ NBDClientSession *s = nbd_get_client_session(bs);
+ uint32_t max = MIN_NON_ZERO(NBD_MAX_BUFFER_SIZE, s->info.max_block);
+
+ bs->bl.max_pdiscard = max;
+ bs->bl.max_pwrite_zeroes = max;
+ bs->bl.max_transfer = max;
+
+ if (s->info.opt_block &&
+ s->info.opt_block > bs->bl.opt_transfer) {
+ bs->bl.opt_transfer = s->info.opt_block;
+ }
}
static void nbd_close(BlockDriverState *bs)
diff --git a/nbd/client.c b/nbd/client.c
index cb55f3d..af2b46d 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -369,12 +369,17 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
info->flags = 0;
trace_nbd_opt_go_start(wantname);
- buf = g_malloc(4 + len + 2 + 1);
+ buf = g_malloc(4 + len + 2 + 2 * info->request_sizes + 1);
stl_be_p(buf, len);
memcpy(buf + 4, wantname, len);
- /* No requests, live with whatever server sends */
- stw_be_p(buf + 4 + len, 0);
- if (nbd_send_option_request(ioc, NBD_OPT_GO, len + 6, buf, errp) < 0) {
+ /* At most one request, everything else up to server */
+ stw_be_p(buf + 4 + len, info->request_sizes);
+ if (info->request_sizes) {
+ stw_be_p(buf + 4 + len + 2, NBD_INFO_BLOCK_SIZE);
+ }
+ if (nbd_send_option_request(ioc, NBD_OPT_GO,
+ 4 + len + 2 + 2 * info->request_sizes, buf,
+ errp) < 0) {
return -1;
}
@@ -405,8 +410,9 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
return 1;
}
if (reply.type != NBD_REP_INFO) {
- error_setg(errp, "unexpected reply type %" PRIx32 ", expected %x",
- reply.type, NBD_REP_INFO);
+ error_setg(errp, "unexpected reply type %" PRIx32
+ " (%s), expected %x",
+ reply.type, nbd_rep_lookup(reply.type), NBD_REP_INFO);
nbd_send_opt_abort(ioc);
return -1;
}
@@ -446,6 +452,51 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
trace_nbd_receive_negotiate_size_flags(info->size, info->flags);
break;
+ case NBD_INFO_BLOCK_SIZE:
+ if (len != sizeof(info->min_block) * 3) {
+ error_setg(errp, "remaining export info len %" PRIu32
+ " is unexpected size", len);
+ nbd_send_opt_abort(ioc);
+ return -1;
+ }
+ if (nbd_read(ioc, &info->min_block, sizeof(info->min_block),
+ errp) < 0) {
+ error_prepend(errp, "failed to read info minimum block size");
+ nbd_send_opt_abort(ioc);
+ return -1;
+ }
+ be32_to_cpus(&info->min_block);
+ if (!is_power_of_2(info->min_block)) {
+ error_setg(errp, "server minimum block size %" PRId32
+ "is not a power of two", info->min_block);
+ nbd_send_opt_abort(ioc);
+ return -1;
+ }
+ if (nbd_read(ioc, &info->opt_block, sizeof(info->opt_block),
+ errp) < 0) {
+ error_prepend(errp, "failed to read info preferred block size");
+ nbd_send_opt_abort(ioc);
+ return -1;
+ }
+ be32_to_cpus(&info->opt_block);
+ if (!is_power_of_2(info->opt_block) ||
+ info->opt_block < info->min_block) {
+ error_setg(errp, "server preferred block size %" PRId32
+ "is not valid", info->opt_block);
+ nbd_send_opt_abort(ioc);
+ return -1;
+ }
+ if (nbd_read(ioc, &info->max_block, sizeof(info->max_block),
+ errp) < 0) {
+ error_prepend(errp, "failed to read info maximum block size");
+ nbd_send_opt_abort(ioc);
+ return -1;
+ }
+ be32_to_cpus(&info->max_block);
+ trace_nbd_opt_go_info_block_size(info->min_block, info->opt_block,
+ info->max_block);
+ break;
+
default:
trace_nbd_opt_go_info_unknown(type, nbd_info_lookup(type));
if (nbd_drop(ioc, len, errp) < 0) {
@@ -729,8 +780,14 @@ fail:
int nbd_init(int fd, QIOChannelSocket *sioc, NBDExportInfo *info,
Error **errp)
{
- unsigned long sectors = info->size / BDRV_SECTOR_SIZE;
- if (info->size / BDRV_SECTOR_SIZE != sectors) {
+ unsigned long sector_size = MAX(BDRV_SECTOR_SIZE, info->min_block);
+ unsigned long sectors = info->size / sector_size;
+
+ /* FIXME: Once the kernel module is patched to honor block sizes,
+ * and to advertise that fact to user space, we should update the
+ * hand-off to the kernel to use any block sizes we learned. */
+ assert(!info->request_sizes);
+ if (info->size / sector_size != sectors) {
error_setg(errp, "Export size %" PRId64 " too large for 32-bit kernel",
info->size);
return -E2BIG;
@@ -744,17 +801,17 @@ int nbd_init(int fd, QIOChannelSocket *sioc, NBDExportInfo *info,
return -serrno;
}
- trace_nbd_init_set_block_size(BDRV_SECTOR_SIZE);
+ trace_nbd_init_set_block_size(sector_size);
- if (ioctl(fd, NBD_SET_BLKSIZE, (unsigned long)BDRV_SECTOR_SIZE) < 0) {
+ if (ioctl(fd, NBD_SET_BLKSIZE, sector_size) < 0) {
int serrno = errno;
error_setg(errp, "Failed setting NBD block size");
return -serrno;
}
trace_nbd_init_set_size(sectors);
- if (info->size % BDRV_SECTOR_SIZE) {
- trace_nbd_init_trailing_bytes(info->size % BDRV_SECTOR_SIZE);
+ if (info->size % sector_size) {
+ trace_nbd_init_trailing_bytes(info->size % sector_size);
}
if (ioctl(fd, NBD_SET_SIZE_BLOCKS, sectors) < 0) {
diff --git a/qemu-nbd.c b/qemu-nbd.c
index c8bd47f..78d05be 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -255,7 +255,7 @@ static void *show_parts(void *arg)
static void *nbd_client_thread(void *arg)
{
char *device = arg;
- NBDExportInfo info;
+ NBDExportInfo info = { .request_sizes = false, };
QIOChannelSocket *sioc;
int fd;
int ret;
diff --git a/nbd/trace-events b/nbd/trace-events
index a3ba4bc..b18d051 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -6,6 +6,7 @@ nbd_reply_err_unsup(uint32_t option, const char *name) "server doesn't understan
nbd_opt_go_start(const char *name) "Attempting NBD_OPT_GO for export '%s'"
nbd_opt_go_success(void) "Export is good to go"
nbd_opt_go_info_unknown(int info, const char *name) "Ignoring unknown info %d (%s)"
+nbd_opt_go_info_block_size(uint32_t minimum, uint32_t preferred, uint32_t maximum) "Block sizes are 0x%" PRIx32 ", 0x%" PRIx32 ", 0x%" PRIx32
nbd_receive_query_exports_start(const char *wantname) "Querying export list for '%s'"
nbd_receive_query_exports_success(const char *wantname) "Found desired export name '%s'"
nbd_receive_starttls_request(void) "Requesting TLS from server"
--
2.9.4
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v5 0/9] Implement NBD_OPT_GO, block size advertisement
2017-07-07 20:30 [Qemu-devel] [PATCH v5 0/9] Implement NBD_OPT_GO, block size advertisement Eric Blake
` (8 preceding siblings ...)
2017-07-07 20:30 ` [Qemu-devel] [PATCH v5 9/9] nbd: Implement NBD_INFO_BLOCK_SIZE on client Eric Blake
@ 2017-07-13 11:38 ` Paolo Bonzini
9 siblings, 0 replies; 28+ messages in thread
From: Paolo Bonzini @ 2017-07-13 11:38 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: vsementsov, den, marcandre.lureau, qemu-block
On 07/07/2017 22:30, Eric Blake wrote:
> The NBD protocol has now finalized the extension that fixes
> several shortcomings with NBD_OPT_EXPORT_NAME (namely, no error
> reporting, no way for the server to advertise block sizes to the
> client):
> https://github.com/NetworkBlockDevice/nbd/blob/extension-info/doc/proto.md
>
> This sort of overlaps with the work Vladimir is proposing for
> implementing NBD_CMD_BLOCK_STATUS, but initial versions of these
> patches have been on the list for over a year now (see patches 37-44):
> https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg03526.html
>
> v4 was here:
> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg04528.html
> Differences since then (if it is even worth comparing) include
> rebasing on top of Vladimir's cleanups and final tweaks to match
> the upstream NBD protocol that was finally promoted from experimental.
>
> Depends on my NBD staging tree (currently Vladimir's v3 nbd refactoring):
> http://repo.or.cz/qemu/ericb.git/shortlog/refs/heads/nbd
> https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg01805.html
>
> 001/9:[0047] [FC] 'nbd: Create struct for tracking export info'
> 002/9:[down] 'nbd: Don't bother tracing an NBD_OPT_ABORT response failure'
> 003/9:[0108] [FC] 'nbd: Expose and debug more NBD constants'
> 004/9:[down] 'nbd: Simplify trace of client flags in negotiation'
> 005/9:[down] 'nbd: Refactor reply to NBD_OPT_EXPORT_NAME'
> 006/9:[0146] [FC] 'nbd: Implement NBD_OPT_GO on server'
> 007/9:[0041] [FC] 'nbd: Implement NBD_OPT_GO on client'
> 008/9:[0014] [FC] 'nbd: Implement NBD_INFO_BLOCK_SIZE on server'
> 009/9:[0050] [FC] 'nbd: Implement NBD_INFO_BLOCK_SIZE on client'
>
> Eric Blake (9):
> nbd: Create struct for tracking export info
> nbd: Don't bother tracing an NBD_OPT_ABORT response failure
> nbd: Expose and debug more NBD constants
> nbd: Simplify trace of client flags in negotiation
> nbd: Refactor reply to NBD_OPT_EXPORT_NAME
> nbd: Implement NBD_OPT_GO on server
> nbd: Implement NBD_OPT_GO on client
> nbd: Implement NBD_INFO_BLOCK_SIZE on server
> nbd: Implement NBD_INFO_BLOCK_SIZE on client
>
> block/nbd-client.h | 3 +-
> include/block/nbd.h | 56 +++++++---
> nbd/nbd-internal.h | 13 +--
> block/nbd-client.c | 22 ++--
> block/nbd.c | 16 ++-
> nbd/client.c | 283 ++++++++++++++++++++++++++++++++++++++++--------
> nbd/common.c | 92 ++++++++++++++++
> nbd/server.c | 304 ++++++++++++++++++++++++++++++++++++++++++++--------
> qemu-nbd.c | 10 +-
> nbd/trace-events | 24 +++--
> 10 files changed, 685 insertions(+), 138 deletions(-)
>
Queued, thanks.
Paolo
^ permalink raw reply [flat|nested] 28+ messages in thread