[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 2/8] nbd: Create struct for tracking export i
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH v4 2/8] nbd: Create struct for tracking export info |
Date: |
Tue, 21 Feb 2017 08:11:36 +0000 |
On Tue, Feb 21, 2017 at 6:49 AM Eric Blake <address@hidden> 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.
>
> Signed-off-by: Eric Blake <address@hidden>
>
Reviewed-by: Marc-André Lureau <address@hidden>
>
> ---
> 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 | 47 +++++++++++++++++++++++++----------------------
> qemu-nbd.c | 10 ++++------
> 6 files changed, 50 insertions(+), 45 deletions(-)
>
> diff --git a/block/nbd-client.h b/block/nbd-client.h
> index f8d6006..098b65c 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 3e373f0..8cc9cbe 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -123,16 +123,23 @@ 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_wr_syncv(QIOChannel *ioc,
> struct iovec *iov,
> size_t niov,
> size_t length,
> bool do_read);
> -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);
> ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest *request);
> ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply);
> int nbd_client(int fd);
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index 06f1532..32d7c90 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -258,7 +258,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;
> }
>
> @@ -287,12 +287,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)) {
> @@ -317,7 +317,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;
> }
>
> @@ -346,7 +346,7 @@ int nbd_client_co_pdiscard(BlockDriverState *bs,
> int64_t offset, int count)
> NBDReply reply;
> ssize_t ret;
>
> - if (!(client->nbdflags & NBD_FLAG_SEND_TRIM)) {
> + if (!(client->info.flags & NBD_FLAG_SEND_TRIM)) {
> return 0;
> }
>
> @@ -405,19 +405,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 35f24be..c43fa35 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -485,7 +485,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 0d16cd1..69f0e09 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -454,13 +454,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;
>
> @@ -573,17 +573,19 @@ int nbd_receive_negotiate(QIOChannel *ioc, const
> char *name, uint16_t *flags,
> }
>
> /* Read the response */
> - if (read_sync(ioc, &s, sizeof(s)) != sizeof(s)) {
> + if (read_sync(ioc, &info->size, sizeof(info->size)) !=
> + sizeof(info->size)) {
> error_setg(errp, "Failed to read export length");
> goto fail;
> }
> - *size = be64_to_cpu(s);
> + be64_to_cpus(&info->size);
>
> - if (read_sync(ioc, flags, sizeof(*flags)) != sizeof(*flags)) {
> + if (read_sync(ioc, &info->flags, sizeof(info->flags)) !=
> + sizeof(info->flags)) {
> error_setg(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;
>
> @@ -596,12 +598,12 @@ int nbd_receive_negotiate(QIOChannel *ioc, const
> char *name, uint16_t *flags,
> goto fail;
> }
>
> - if (read_sync(ioc, &s, sizeof(s)) != sizeof(s)) {
> + if (read_sync(ioc, &info->size, sizeof(info->size)) !=
> + sizeof(info->size)) {
> error_setg(errp, "Failed to read export length");
> goto fail;
> }
> - *size = be64_to_cpu(s);
> - TRACE("Size is %" PRIu64, *size);
> + be64_to_cpus(&info->size);
>
> if (read_sync(ioc, &oldflags, sizeof(oldflags)) !=
> sizeof(oldflags)) {
> error_setg(errp, "Failed to read export flags");
> @@ -612,13 +614,14 @@ 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("Size is %" PRIu64 ", export flags %" PRIx16, *size, *flags);
> + TRACE("Size is %" PRIu64 ", export flags %" PRIx16,
> + info->size, info->flags);
> if (zeroes && drop_sync(ioc, 124) != 124) {
> error_setg(errp, "Failed to read reserved block");
> goto fail;
> @@ -630,11 +633,11 @@ 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)
> {
> - unsigned long sectors = size / BDRV_SECTOR_SIZE;
> - if (size / BDRV_SECTOR_SIZE != sectors) {
> - LOG("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) {
> + LOG("Export size %" PRId64 " too large for 32-bit kernel",
> info->size);
> return -E2BIG;
> }
>
> @@ -655,9 +658,9 @@ int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t
> flags, off_t size)
> }
>
> TRACE("Setting size to %lu block(s)", sectors);
> - if (size % BDRV_SECTOR_SIZE) {
> + if (info->size % BDRV_SECTOR_SIZE) {
> TRACE("Ignoring trailing %d bytes of export",
> - (int) (size % BDRV_SECTOR_SIZE));
> + (int) (info->size % BDRV_SECTOR_SIZE));
> }
>
> if (ioctl(fd, NBD_SET_SIZE_BLOCKS, sectors) < 0) {
> @@ -666,9 +669,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("Setting readonly attribute");
>
> if (ioctl(fd, BLKROSET, (unsigned long) &read_only) < 0) {
> @@ -726,7 +729,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)
> {
> return -ENOTSUP;
> }
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index e4fede6..c598cbe 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -254,8 +254,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;
> @@ -270,9 +269,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);
> @@ -287,7 +285,7 @@ static void *nbd_client_thread(void *arg)
> goto out_socket;
> }
>
> - ret = nbd_init(fd, sioc, nbdflags, size);
> + ret = nbd_init(fd, sioc, &info);
> if (ret < 0) {
> goto out_fd;
> }
> --
> 2.9.3
>
>
> --
Marc-André Lureau
- [Qemu-devel] [PATCH v4 0/8] Implement NBD_OPT_GO, block size advertisement, Eric Blake, 2017/02/20
- [Qemu-devel] [PATCH v4 1/8] nbd/client: fix drop_sync [CVE-2017-2630], Eric Blake, 2017/02/20
- [Qemu-devel] [PATCH v4 3/8] block: Add blk_get_opt_transfer(), Eric Blake, 2017/02/20
- [Qemu-devel] [PATCH v4 4/8] nbd: Expose and debug more NBD constants, Eric Blake, 2017/02/20
- [Qemu-devel] [PATCH v4 2/8] nbd: Create struct for tracking export info, Eric Blake, 2017/02/20
- Re: [Qemu-devel] [PATCH v4 2/8] nbd: Create struct for tracking export info,
Marc-André Lureau <=
- [Qemu-devel] [PATCH v4 6/8] nbd: Implement NBD_OPT_GO on client, Eric Blake, 2017/02/20
- [Qemu-devel] [PATCH v4 7/8] nbd: Implement NBD_INFO_BLOCK_SIZE on server, Eric Blake, 2017/02/20
- Re: [Qemu-devel] [PATCH v4 7/8] nbd: Implement NBD_INFO_BLOCK_SIZE on server, Paolo Bonzini, 2017/02/22
- Re: [Qemu-devel] [PATCH v4 7/8] nbd: Implement NBD_INFO_BLOCK_SIZE on server, Eric Blake, 2017/02/22
- Re: [Qemu-devel] [PATCH v4 7/8] nbd: Implement NBD_INFO_BLOCK_SIZE on server, Paolo Bonzini, 2017/02/22
- Re: [Qemu-devel] [PATCH v4 7/8] nbd: Implement NBD_INFO_BLOCK_SIZE on server, Eric Blake, 2017/02/22
- Re: [Qemu-devel] [PATCH v4 7/8] nbd: Implement NBD_INFO_BLOCK_SIZE on server, Paolo Bonzini, 2017/02/23
- Re: [Qemu-devel] [PATCH v4 7/8] nbd: Implement NBD_INFO_BLOCK_SIZE on server, Eric Blake, 2017/02/23
[Qemu-devel] [PATCH v4 5/8] nbd: Implement NBD_OPT_GO on server, Eric Blake, 2017/02/20
[Qemu-devel] [PATCH v4 8/8] nbd: Implement NBD_INFO_BLOCK_SIZE on client, Eric Blake, 2017/02/20