Re: [Qemu-devel] [Qemu-block] [PATCH] nbd/server: Nicer spelling of max BLOCK_STATUS reply length

[Stefano Garzarella]

On Fri, May 10, 2019 at 10:17:35AM -0500, Eric Blake wrote:
> Commit 3d068aff (3.0) introduced NBD_MAX_BITMAP_EXTENTS as a limit on
> how large we would allow a reply to NBD_CMD_BLOCK_STATUS to grow when
> it is visiting a qemu:dirty-bitmap: context.  Later, commit fb7afc79
> (3.1) reused the constant to limit base:allocation context replies,
> although the name is now less appropriate in that situation.
> 
> Rename things, and improve the macro to use units.h for better
> legibility. Then reformat the comment to comply with checkpatch rules
> added in the meantime. No semantic change.
> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  nbd/server.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index e21bd501dc6..2c49744fc43 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -21,15 +21,18 @@
>  #include "qapi/error.h"
>  #include "trace.h"
>  #include "nbd-internal.h"
> +#include "qemu/units.h"
> 
>  #define NBD_META_ID_BASE_ALLOCATION 0
>  #define NBD_META_ID_DIRTY_BITMAP 1
> 
> -/* NBD_MAX_BITMAP_EXTENTS: 1 mb of extents data. An empirical
> +/*
> + * NBD_MAX_BLOCK_STATUS_EXTENTS: 1 mb of extents data. An empirical

Since we are here, what about using MiB also in this comment block?

>   * constant. If an increase is needed, note that the NBD protocol
>   * recommends no larger than 32 mb, so that the client won't consider
> - * the reply as a denial of service attack. */
> -#define NBD_MAX_BITMAP_EXTENTS (0x100000 / 8)
> + * the reply as a denial of service attack.
> + */
> +#define NBD_MAX_BLOCK_STATUS_EXTENTS (1 * MiB / 8)
> 
>  static int system_errno_to_nbd_errno(int err)
>  {
> @@ -1958,7 +1961,7 @@ static int nbd_co_send_block_status(NBDClient *client, 
> uint64_t handle,
>                                      Error **errp)
>  {
>      int ret;
> -    unsigned int nb_extents = dont_fragment ? 1 : NBD_MAX_BITMAP_EXTENTS;
> +    unsigned int nb_extents = dont_fragment ? 1 : 
> NBD_MAX_BLOCK_STATUS_EXTENTS;
>      NBDExtent *extents = g_new(NBDExtent, nb_extents);
>      uint64_t final_length = length;
> 
> @@ -2043,7 +2046,7 @@ static int nbd_co_send_bitmap(NBDClient *client, 
> uint64_t handle,
>                                uint32_t context_id, Error **errp)
>  {
>      int ret;
> -    unsigned int nb_extents = dont_fragment ? 1 : NBD_MAX_BITMAP_EXTENTS;
> +    unsigned int nb_extents = dont_fragment ? 1 : 
> NBD_MAX_BLOCK_STATUS_EXTENTS;
>      NBDExtent *extents = g_new(NBDExtent, nb_extents);
>      uint64_t final_length = length;

With or without changing the comment:

Reviewed-by: Stefano Garzarella <address@hidden>