Re: [Qemu-block] [PATCH v3 7/6] nbd/client: Ignore inaccessible tail of inconsistent server

[Vladimir Sementsov-Ogievskiy]

29.03.2019 19:34, Eric Blake wrote:
> The NBD spec suggests that a server should never advertise a size
> inconsistent with its minimum block alignment, as that tail is
> effectively inaccessible to a compliant client obeying those block
> constraints. Although the block layer likes to round up, here, we'd
> prefer to truncate down to obey the spec, and note that it is the
> server's fault for advertising bogus size.
> 
> Does not impact either qemu (which always sends properly aligned
> sizes) or nbdkit (which does not send minimum block requirements yet);
> so this is mostly theoretical, to avoid potential asserts elsewhere in
> the code that assume the size is aligned.
> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
>   nbd/client.c     | 8 +++++++-
>   nbd/trace-events | 1 +
>   2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index de7da48246b..be437051429 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -1,5 +1,5 @@
>   /*
> - *  Copyright (C) 2016-2018 Red Hat, Inc.
> + *  Copyright (C) 2016-2019 Red Hat, Inc.
>    *  Copyright (C) 2005  Anthony Liguori <address@hidden>
>    *
>    *  Network Block Device Client Side
> @@ -426,6 +426,12 @@ static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t 
> opt,
>                   nbd_send_opt_abort(ioc);
>                   return -1;
>               }
> +            if (info->min_block &&
> +                !QEMU_IS_ALIGNED(info->size, info->min_block)) {
> +                trace_nbd_opt_info_go_unaligned_size(info->size,
> +                                                     info->min_block);
> +                info->size = QEMU_ALIGN_DOWN(info->size, info->min_block);
> +            }
>               trace_nbd_receive_negotiate_size_flags(info->size, info->flags);
>               break;
> 
> diff --git a/nbd/trace-events b/nbd/trace-events
> index a6cca8fdf83..49dd48c9620 100644
> --- a/nbd/trace-events
> +++ b/nbd/trace-events
> @@ -8,6 +8,7 @@ nbd_reply_err_unsup(uint32_t option, const char *name) 
> "server doesn't understan
>   nbd_receive_list(const char *name, const char *desc) "export list includes 
> '%s', description '%s'"
>   nbd_opt_info_go_start(const char *opt, const char *name) "Attempting %s for 
> export '%s'"
>   nbd_opt_info_go_success(const char *opt) "Export is ready after %s request"
> +nbd_opt_info_go_unaligned_size(uint64_t size, uint32_t align) "Server 
> reported unaligned size 0x%" PRIx64 " for alignment 0x%x; truncating"
>   nbd_opt_info_unknown(int info, const char *name) "Ignoring unknown info %d 
> (%s)"
>   nbd_opt_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'"
> 


And this again leads to silently skip file tail on qemu-img convert from such 
nbd export.
I don't really care, but if not Qemu neither nbdkit are affected, isn't it 
better just
nbd_send_opt_abort and return -1 in this case? So we'll never have hidden 
troubles with
third-party bad servers, and their developers will have to fix their code 
instead (even
not written yet, I suppose).


-- 
Best regards,
Vladimir