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 <eblake@redhat.com>
---
 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
  * 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;

-- 
2.20.1

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 <eblake@redhat.com>
> ---
>  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 <sgarzare@redhat.com>

10.05.2019 18:17, 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 <eblake@redhat.com>


With or without Stefano's suggestion:

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir

On 5/13/19 5:03 AM, Vladimir Sementsov-Ogievskiy wrote:
> 10.05.2019 18:17, 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 <eblake@redhat.com>
> 
> 
> With or without Stefano's suggestion:
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

I'm assuming Reviewed-by instead of Signed-off-by?

I'll queue this through my NBD tree, with Stefano's suggestion added in.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

13.05.2019 17:31, Eric Blake wrote:
> On 5/13/19 5:03 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 10.05.2019 18:17, 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 <eblake@redhat.com>
>>
>>
>> With or without Stefano's suggestion:
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> I'm assuming Reviewed-by instead of Signed-off-by?
> 

Aha, of course. Copy-pasted and forget to fix(

-- 
Best regards,
Vladimir