qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [Qemu-devel] [PATCH v3 07/44] nbd: Limit nbdflags to 16


From: Alex Bligh
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH v3 07/44] nbd: Limit nbdflags to 16 bits
Date: Mon, 25 Apr 2016 10:24:33 +0100

On 23 Apr 2016, at 00:40, Eric Blake <address@hidden> wrote:

> Rather than asserting that nbdflags is within range, just give
> it the correct type to begin with :)  nbdflags corresponds to
> the per-export portion of NBD Protocol "transmission flags", which
> is 16 bits in response to NBD_OPT_EXPORT_NAME and NBD_OPT_GO.
> 
> Furthermore, upstream NBD has never passed the global flags to
> the kernel via ioctl(NBD_SET_FLAGS) (the ioctl was first
> introduced in NBD 2.9.22; then a latent bug in NBD 3.1 actually
> tried to OR the global flags with the transmission flags, with
> the disaster that the addition of NBD_FLAG_NO_ZEROES in 3.9
> caused all earlier NBD 3.x clients to treat every export as
> read-only; NBD 3.10 and later intentionally clip things to 16
> bits to pass only transmission flags).  Qemu should follow suit,
> since the current two global flags (NBD_FLAG_FIXED_NEWSTYLE
> and NBD_FLAG_NO_ZEROES) have no impact on the kernel's behavior
> during transmission.
> 
> Signed-off-by: Eric Blake <address@hidden>

Looks sensible, but NBD has at least three types of flags. Perhaps
rather than calling them nbdflags you could call them
nbdtransmissionflags, nbdclientflags or whatever which might
help avoid this confusion in future.

Alex


> 
> ---
> v3: expand scope of patch
> ---
> block/nbd-client.h  |  2 +-
> include/block/nbd.h |  6 +++---
> nbd/client.c        | 28 +++++++++++++++-------------
> nbd/server.c        | 10 ++++------
> qemu-nbd.c          |  4 ++--
> 5 files changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/block/nbd-client.h b/block/nbd-client.h
> index bc7aec0..1243612 100644
> --- a/block/nbd-client.h
> +++ b/block/nbd-client.h
> @@ -20,7 +20,7 @@
> typedef struct NbdClientSession {
>     QIOChannelSocket *sioc; /* The master data channel */
>     QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */
> -    uint32_t nbdflags;
> +    uint16_t nbdflags;
>     off_t size;
> 
>     CoMutex send_mutex;
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index b86a976..134f117 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -83,11 +83,11 @@ ssize_t nbd_wr_syncv(QIOChannel *ioc,
>                      size_t offset,
>                      size_t length,
>                      bool do_read);
> -int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
> +int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
>                           QCryptoTLSCreds *tlscreds, const char *hostname,
>                           QIOChannel **outioc,
>                           off_t *size, Error **errp);
> -int nbd_init(int fd, QIOChannelSocket *sioc, uint32_t flags, off_t size);
> +int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size);
> ssize_t nbd_send_request(QIOChannel *ioc, struct nbd_request *request);
> ssize_t nbd_receive_reply(QIOChannel *ioc, struct nbd_reply *reply);
> int nbd_client(int fd);
> @@ -97,7 +97,7 @@ typedef struct NBDExport NBDExport;
> typedef struct NBDClient NBDClient;
> 
> NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size,
> -                          uint32_t nbdflags, void (*close)(NBDExport *),
> +                          uint16_t nbdflags, void (*close)(NBDExport *),
>                           Error **errp);
> void nbd_export_close(NBDExport *exp);
> void nbd_export_get(NBDExport *exp);
> diff --git a/nbd/client.c b/nbd/client.c
> index f1afa49..937344c 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -406,7 +406,7 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
> }
> 
> 
> -int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
> +int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
>                           QCryptoTLSCreds *tlscreds, const char *hostname,
>                           QIOChannel **outioc,
>                           off_t *size, Error **errp)
> @@ -466,7 +466,6 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char 
> *name, uint32_t *flags,
>         uint32_t opt;
>         uint32_t namesize;
>         uint16_t globalflags;
> -        uint16_t exportflags;
>         bool fixedNewStyle = false;
> 
>         if (read_sync(ioc, &globalflags, sizeof(globalflags)) !=
> @@ -475,7 +474,6 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char 
> *name, uint32_t *flags,
>             goto fail;
>         }
>         globalflags = be16_to_cpu(globalflags);
> -        *flags = globalflags << 16;
>         TRACE("Global flags are %" PRIx32, globalflags);
>         if (globalflags & NBD_FLAG_FIXED_NEWSTYLE) {
>             fixedNewStyle = true;
> @@ -543,17 +541,15 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char 
> *name, uint32_t *flags,
>             goto fail;
>         }
>         *size = be64_to_cpu(s);
> -        TRACE("Size is %" PRIu64, *size);
> 
> -        if (read_sync(ioc, &exportflags, sizeof(exportflags)) !=
> -            sizeof(exportflags)) {
> +        if (read_sync(ioc, flags, sizeof(*flags)) != sizeof(*flags)) {
>             error_setg(errp, "Failed to read export flags");
>             goto fail;
>         }
> -        exportflags = be16_to_cpu(exportflags);
> -        *flags |= exportflags;
> -        TRACE("Export flags are %" PRIx16, exportflags);
> +        be16_to_cpus(flags);
>     } else if (magic == NBD_CLIENT_MAGIC) {
> +        uint32_t oldflags;
> +
>         if (name) {
>             error_setg(errp, "Server does not support export names");
>             goto fail;
> @@ -570,16 +566,22 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char 
> *name, uint32_t *flags,
>         *size = be64_to_cpu(s);
>         TRACE("Size is %" PRIu64, *size);
> 
> -        if (read_sync(ioc, flags, sizeof(*flags)) != sizeof(*flags)) {
> +        if (read_sync(ioc, &oldflags, sizeof(oldflags)) != sizeof(oldflags)) 
> {
>             error_setg(errp, "Failed to read export flags");
>             goto fail;
>         }
> -        *flags = be32_to_cpup(flags);
> +        be32_to_cpus(&oldflags);
> +        if (oldflags & ~0xffff) {
> +            error_setg(errp, "Unexpected export flags %0x" PRIx32, oldflags);
> +            goto fail;
> +        }
> +        *flags = oldflags;
>     } else {
>         error_setg(errp, "Bad magic received");
>         goto fail;
>     }
> 
> +    TRACE("Size is %" PRIu64 ", export flags %" PRIx16, *size, *flags);
>     if (read_sync(ioc, &buf, 124) != 124) {
>         error_setg(errp, "Failed to read reserved block");
>         goto fail;
> @@ -591,7 +593,7 @@ fail:
> }
> 
> #ifdef __linux__
> -int nbd_init(int fd, QIOChannelSocket *sioc, uint32_t flags, off_t size)
> +int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size)
> {
>     unsigned long sectors = size / BDRV_SECTOR_SIZE;
>     if (size / BDRV_SECTOR_SIZE != sectors) {
> @@ -687,7 +689,7 @@ int nbd_disconnect(int fd)
> }
> 
> #else
> -int nbd_init(int fd, QIOChannelSocket *ioc, uint32_t flags, off_t size)
> +int nbd_init(int fd, QIOChannelSocket *ioc, uint16_t flags, off_t size)
> {
>     return -ENOTSUP;
> }
> diff --git a/nbd/server.c b/nbd/server.c
> index 789189d..31fc9cf 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -63,7 +63,7 @@ struct NBDExport {
>     char *name;
>     off_t dev_offset;
>     off_t size;
> -    uint32_t nbdflags;
> +    uint16_t nbdflags;
>     QTAILQ_HEAD(, NBDClient) clients;
>     QTAILQ_ENTRY(NBDExport) next;
> 
> @@ -544,8 +544,8 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData 
> *data)
>     NBDClient *client = data->client;
>     char buf[8 + 8 + 8 + 128];
>     int rc;
> -    const int myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
> -                         NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA);
> +    const uint16_t myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
> +                              NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA);
>     bool oldStyle;
> 
>     /* Old style negotiation header without options
> @@ -575,7 +575,6 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData 
> *data)
> 
>     oldStyle = client->exp != NULL && !client->tlscreds;
>     if (oldStyle) {
> -        assert ((client->exp->nbdflags & ~65535) == 0);
>         stq_be_p(buf + 8, NBD_CLIENT_MAGIC);
>         stq_be_p(buf + 16, client->exp->size);
>         stw_be_p(buf + 26, client->exp->nbdflags | myflags);
> @@ -604,7 +603,6 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData 
> *data)
>             goto fail;
>         }
> 
> -        assert ((client->exp->nbdflags & ~65535) == 0);
>         stq_be_p(buf + 18, client->exp->size);
>         stw_be_p(buf + 26, client->exp->nbdflags | myflags);
>         if (nbd_negotiate_write(client->ioc, buf + 18, sizeof(buf) - 18) !=
> @@ -806,7 +804,7 @@ static void nbd_eject_notifier(Notifier *n, void *data)
> }
> 
> NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size,
> -                          uint32_t nbdflags, void (*close)(NBDExport *),
> +                          uint16_t nbdflags, void (*close)(NBDExport *),
>                           Error **errp)
> {
>     NBDExport *exp = g_malloc0(sizeof(NBDExport));
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 2c9754e..71bfdeb 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -241,7 +241,7 @@ static void *nbd_client_thread(void *arg)
> {
>     char *device = arg;
>     off_t size;
> -    uint32_t nbdflags;
> +    uint16_t nbdflags;
>     QIOChannelSocket *sioc;
>     int fd;
>     int ret;
> @@ -455,7 +455,7 @@ int main(int argc, char **argv)
>     BlockBackend *blk;
>     BlockDriverState *bs;
>     off_t dev_offset = 0;
> -    uint32_t nbdflags = 0;
> +    uint16_t nbdflags = 0;
>     bool disconnect = false;
>     const char *bindto = "0.0.0.0";
>     const char *port = NULL;
> -- 
> 2.5.5
> 
> 

-- 
Alex Bligh







reply via email to

[Prev in Thread] Current Thread [Next in Thread]