23.08.2019 17:37, Eric Blake wrote:
> When creating a read-only image, we are still advertising support for
> TRIM and WRITE_ZEROES to the client, even though the client should not
> be issuing those commands. But seeing this requires looking across
> multiple functions:
>
> All callers to nbd_export_new() passed a single flag based solely on
> whether the export allows writes. Later, we then pass a constant set
> of flags to nbd_negotiate_options() (namely, the set of flags which we
> always support, at least for writable images), which is then further
> dynamically modified based on client requests for structured options.
> Finally, when processing NBD_OPT_EXPORT_NAME or NBD_OPT_EXPORT_GO we
> bitwise-or the original caller's flag with the runtime set of flags
> we've built up over several functions.
>
> Let's refactor things to instead compute a baseline of flags as soon
> as possible, in nbd_export_new(), and changing the signature for the
> callers to pass in a simpler bool rather than having to figure out
> flags. We can then get rid of the 'myflags' parameter to various
> functions, and instead refer to client for everything we need (we
> still have to perform a bitwise-OR during NBD_OPT_EXPORT_NAME and
> NBD_OPT_EXPORT_GO, but it's easier to see what is being computed).
> This lets us quit advertising senseless flags for read-only images, as
> well as making the next patch for exposing FAST_ZERO support easier to
> write.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> ---
> include/block/nbd.h | 2 +-
> blockdev-nbd.c | 3 +--
> nbd/server.c | 62 +++++++++++++++++++++++++--------------------
> qemu-nbd.c | 6 ++---
> 4 files changed, 39 insertions(+), 34 deletions(-)
>
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 991fd52a5134..2c87b42dfd48 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -326,7 +326,7 @@ typedef struct NBDClient NBDClient;
>
> NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
> uint64_t size, const char *name, const char *desc,
> - const char *bitmap, uint16_t nbdflags, bool shared,
> + const char *bitmap, bool readonly, bool shared,
> void (*close)(NBDExport *), bool writethrough,
> BlockBackend *on_eject_blk, Error **errp);
> void nbd_export_close(NBDExport *exp);
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index ecfa2ef0adb5..1cdffab4fce1 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -187,8 +187,7 @@ void qmp_nbd_server_add(const char *device, bool
> has_name, const char *name,
> writable = false;
> }
>
> - exp = nbd_export_new(bs, 0, len, name, NULL, bitmap,
> - writable ? 0 : NBD_FLAG_READ_ONLY, !writable,
> + exp = nbd_export_new(bs, 0, len, name, NULL, bitmap, !writable,
> !writable,
> NULL, false, on_eject_blk, errp);
> if (!exp) {
> return;
> diff --git a/nbd/server.c b/nbd/server.c
> index 0fb41c6c50ea..b5577828aa44 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -423,14 +423,14 @@ static void nbd_check_meta_export(NBDClient *client)
>
> /* Send a reply to NBD_OPT_EXPORT_NAME.
> * Return -errno on error, 0 on success. */
> -static int nbd_negotiate_handle_export_name(NBDClient *client,
> - uint16_t myflags, bool no_zeroes,
> +static int nbd_negotiate_handle_export_name(NBDClient *client, bool
> no_zeroes,
> Error **errp)
> {
> char name[NBD_MAX_NAME_SIZE + 1];
> char buf[NBD_REPLY_EXPORT_NAME_SIZE] = "";
> size_t len;
> int ret;
> + uint16_t myflags;
>
> /* Client sends:
> [20 .. xx] export name (length bytes)
> @@ -458,10 +458,13 @@ static int nbd_negotiate_handle_export_name(NBDClient
> *client,
> return -EINVAL;
> }
>
> - trace_nbd_negotiate_new_style_size_flags(client->exp->size,
> - client->exp->nbdflags |
> myflags);
> + myflags = client->exp->nbdflags;
> + if (client->structured_reply) {
> + myflags |= NBD_FLAG_SEND_DF;
> + }
why we cant do just
client->exp->nbdflags |= NBD_FLAG_SEND_DF ?
Or even do it earlier, when client->structured_reply becomes true?
> + trace_nbd_negotiate_new_style_size_flags(client->exp->size, myflags);
> stq_be_p(buf, client->exp->size);
> - stw_be_p(buf + 8, client->exp->nbdflags | myflags);
> + stw_be_p(buf + 8, myflags);
> len = no_zeroes ? 10 : sizeof(buf);
> ret = nbd_write(client->ioc, buf, len, errp);
> if (ret < 0) {
> @@ -526,8 +529,7 @@ static int nbd_reject_length(NBDClient *client, bool
> fatal, Error **errp)
> /* 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, uint16_t myflags,
> - Error **errp)
> +static int nbd_negotiate_handle_info(NBDClient *client, Error **errp)
> {
> int rc;
> char name[NBD_MAX_NAME_SIZE + 1];
[..]
--
Best regards,
Vladimir
