1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH] block/vhdx: Use IEC binary prefixes from "qemu/units.h"

Stefano Garzarella posted 1 patch 6 years, 10 months ago
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test asan passed
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190321094012.36541-1-sgarzare@redhat.com
Maintainers: Max Reitz <mreitz@redhat.com>, Kevin Wolf <kwolf@redhat.com>, Jeff Cody <codyprime@gmail.com>
There is a newer version of this series
block/vhdx-log.c | 1 +
block/vhdx.c     | 4 +++-
block/vhdx.h     | 5 -----
3 files changed, 4 insertions(+), 6 deletions(-)
[Qemu-devel] [PATCH] block/vhdx: Use IEC binary prefixes from "qemu/units.h"
Posted by Stefano Garzarella 6 years, 10 months ago 
IEC binary prefixes are defined in "qemu/units.h", so we
can remove redundant definitions in "block/vhdx.h".

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 block/vhdx-log.c | 1 +
 block/vhdx.c     | 4 +++-
 block/vhdx.h     | 5 -----
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index 3149ff08d8..ad9905eb6d 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -18,6 +18,7 @@
  *
  */
 #include "qemu/osdep.h"
+#include "qemu/units.h"
 #include "qapi/error.h"
 #include "qemu-common.h"
 #include "block/block_int.h"
diff --git a/block/vhdx.c b/block/vhdx.c
index b785aef4b7..e9b16c5ef6 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -16,6 +16,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/units.h"
 #include "qapi/error.h"
 #include "block/block_int.h"
 #include "block/qdict.h"
@@ -1889,7 +1890,8 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,
         return -EINVAL;
     }
     if (block_size > VHDX_BLOCK_SIZE_MAX) {
-        error_setg(errp, "Block size must not exceed %d", VHDX_BLOCK_SIZE_MAX);
+        error_setg(errp, "Block size must not exceed %" PRId64,
+                   VHDX_BLOCK_SIZE_MAX);
         return -EINVAL;
     }
 
diff --git a/block/vhdx.h b/block/vhdx.h
index 1bfb4e4f73..deb6e362ad 100644
--- a/block/vhdx.h
+++ b/block/vhdx.h
@@ -18,11 +18,6 @@
 #ifndef BLOCK_VHDX_H
 #define BLOCK_VHDX_H
 
-#define KiB              (1 * 1024)
-#define MiB            (KiB * 1024)
-#define GiB            (MiB * 1024)
-#define TiB ((uint64_t) GiB * 1024)
-
 #define DEFAULT_LOG_SIZE 1048576 /* 1MiB */
 /* Structures and fields present in the VHDX file */
 
-- 
2.20.1
Re: [Qemu-devel] [Qemu-block] [PATCH] block/vhdx: Use IEC binary prefixes from "qemu/units.h"
Posted by John Snow 6 years, 10 months ago 

On 3/21/19 5:40 AM, Stefano Garzarella wrote:
> IEC binary prefixes are defined in "qemu/units.h", so we
> can remove redundant definitions in "block/vhdx.h".
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  block/vhdx-log.c | 1 +
>  block/vhdx.c     | 4 +++-
>  block/vhdx.h     | 5 -----
>  3 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/block/vhdx-log.c b/block/vhdx-log.c
> index 3149ff08d8..ad9905eb6d 100644
> --- a/block/vhdx-log.c
> +++ b/block/vhdx-log.c
> @@ -18,6 +18,7 @@
>   *
>   */
>  #include "qemu/osdep.h"
> +#include "qemu/units.h"
>  #include "qapi/error.h"
>  #include "qemu-common.h"
>  #include "block/block_int.h"
> diff --git a/block/vhdx.c b/block/vhdx.c
> index b785aef4b7..e9b16c5ef6 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -16,6 +16,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/units.h"
>  #include "qapi/error.h"
>  #include "block/block_int.h"
>  #include "block/qdict.h"
> @@ -1889,7 +1890,8 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,
>          return -EINVAL;
>      }
>      if (block_size > VHDX_BLOCK_SIZE_MAX) {
> -        error_setg(errp, "Block size must not exceed %d", VHDX_BLOCK_SIZE_MAX);
> +        error_setg(errp, "Block size must not exceed %" PRId64,
> +                   VHDX_BLOCK_SIZE_MAX);

Oh, changing the constants changes the inferred type of this constant,
correct?

>          return -EINVAL;
>      }
>  
> diff --git a/block/vhdx.h b/block/vhdx.h
> index 1bfb4e4f73..deb6e362ad 100644
> --- a/block/vhdx.h
> +++ b/block/vhdx.h
> @@ -18,11 +18,6 @@
>  #ifndef BLOCK_VHDX_H
>  #define BLOCK_VHDX_H
>  
> -#define KiB              (1 * 1024)
> -#define MiB            (KiB * 1024)
> -#define GiB            (MiB * 1024)
> -#define TiB ((uint64_t) GiB * 1024)
> -

I'd include units.h here because there are other references to
KiB/MiB/GiB and so on throughout the rest of this header file.

I think it's bad form to have a header that cannot be included unless
you include other headers manually alongside it.

>  #define DEFAULT_LOG_SIZE 1048576 /* 1MiB */
>  /* Structures and fields present in the VHDX file */
>  
>
Re: [Qemu-devel] [Qemu-block] [PATCH] block/vhdx: Use IEC binary prefixes from "qemu/units.h"
Posted by Philippe Mathieu-Daudé 6 years, 10 months ago 
Le lun. 25 mars 2019 21:53, John Snow <jsnow@redhat.com> a écrit :

>
>
> On 3/21/19 5:40 AM, Stefano Garzarella wrote:
> > IEC binary prefixes are defined in "qemu/units.h", so we
> > can remove redundant definitions in "block/vhdx.h".
> >
> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > ---
> >  block/vhdx-log.c | 1 +
> >  block/vhdx.c     | 4 +++-
> >  block/vhdx.h     | 5 -----
> >  3 files changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/block/vhdx-log.c b/block/vhdx-log.c
> > index 3149ff08d8..ad9905eb6d 100644
> > --- a/block/vhdx-log.c
> > +++ b/block/vhdx-log.c
> > @@ -18,6 +18,7 @@
> >   *
> >   */
> >  #include "qemu/osdep.h"
> > +#include "qemu/units.h"
> >  #include "qapi/error.h"
> >  #include "qemu-common.h"
> >  #include "block/block_int.h"
> > diff --git a/block/vhdx.c b/block/vhdx.c
> > index b785aef4b7..e9b16c5ef6 100644
> > --- a/block/vhdx.c
> > +++ b/block/vhdx.c
> > @@ -16,6 +16,7 @@
> >   */
> >
> >  #include "qemu/osdep.h"
> > +#include "qemu/units.h"
> >  #include "qapi/error.h"
> >  #include "block/block_int.h"
> >  #include "block/qdict.h"
> > @@ -1889,7 +1890,8 @@ static int coroutine_fn
> vhdx_co_create(BlockdevCreateOptions *opts,
> >          return -EINVAL;
> >      }
> >      if (block_size > VHDX_BLOCK_SIZE_MAX) {
> > -        error_setg(errp, "Block size must not exceed %d",
> VHDX_BLOCK_SIZE_MAX);
> > +        error_setg(errp, "Block size must not exceed %" PRId64,
> > +                   VHDX_BLOCK_SIZE_MAX);
>
> Oh, changing the constants changes the inferred type of this constant,
> correct?
>

Yes.


> >          return -EINVAL;
> >      }
> >
> > diff --git a/block/vhdx.h b/block/vhdx.h
> > index 1bfb4e4f73..deb6e362ad 100644
> > --- a/block/vhdx.h
> > +++ b/block/vhdx.h
> > @@ -18,11 +18,6 @@
> >  #ifndef BLOCK_VHDX_H
> >  #define BLOCK_VHDX_H
> >
> > -#define KiB              (1 * 1024)
> > -#define MiB            (KiB * 1024)
> > -#define GiB            (MiB * 1024)
> > -#define TiB ((uint64_t) GiB * 1024)
> > -
>
> I'd include units.h here because there are other references to
> KiB/MiB/GiB and so on throughout the rest of this header file.
>
> I think it's bad form to have a header that cannot be included unless
> you include other headers manually alongside it.
>

Agreed.


> >  #define DEFAULT_LOG_SIZE 1048576 /* 1MiB */
>

Can we also update this definition?

>  /* Structures and fields present in the VHDX file */
> >
> >
>
>
>
Re: [Qemu-devel] [Qemu-block] [PATCH] block/vhdx: Use IEC binary prefixes from "qemu/units.h"
Posted by Stefano Garzarella 6 years, 10 months ago 
On Mon, Mar 25, 2019 at 10:56:57PM +0100, Philippe Mathieu-Daudé wrote:
> Le lun. 25 mars 2019 21:53, John Snow <jsnow@redhat.com> a écrit :
> > On 3/21/19 5:40 AM, Stefano Garzarella wrote:
> > > IEC binary prefixes are defined in "qemu/units.h", so we
> > > can remove redundant definitions in "block/vhdx.h".
> > >
> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > ---
> > >  block/vhdx-log.c | 1 +
> > >  block/vhdx.c     | 4 +++-
> > >  block/vhdx.h     | 5 -----
> > >  3 files changed, 4 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/block/vhdx-log.c b/block/vhdx-log.c
> > > index 3149ff08d8..ad9905eb6d 100644
> > > --- a/block/vhdx-log.c
> > > +++ b/block/vhdx-log.c
> > > @@ -18,6 +18,7 @@
> > >   *
> > >   */
> > >  #include "qemu/osdep.h"
> > > +#include "qemu/units.h"
> > >  #include "qapi/error.h"
> > >  #include "qemu-common.h"
> > >  #include "block/block_int.h"
> > > diff --git a/block/vhdx.c b/block/vhdx.c
> > > index b785aef4b7..e9b16c5ef6 100644
> > > --- a/block/vhdx.c
> > > +++ b/block/vhdx.c
> > > @@ -16,6 +16,7 @@
> > >   */
> > >
> > >  #include "qemu/osdep.h"
> > > +#include "qemu/units.h"
> > >  #include "qapi/error.h"
> > >  #include "block/block_int.h"
> > >  #include "block/qdict.h"
> > > @@ -1889,7 +1890,8 @@ static int coroutine_fn
> > vhdx_co_create(BlockdevCreateOptions *opts,
> > >          return -EINVAL;
> > >      }
> > >      if (block_size > VHDX_BLOCK_SIZE_MAX) {
> > > -        error_setg(errp, "Block size must not exceed %d",
> > VHDX_BLOCK_SIZE_MAX);
> > > +        error_setg(errp, "Block size must not exceed %" PRId64,
> > > +                   VHDX_BLOCK_SIZE_MAX);
> >
> > Oh, changing the constants changes the inferred type of this constant,
> > correct?
> >
> 

Yes, in "units.h" all the constants are int64_t.

> Yes.
> 
> 
> > >          return -EINVAL;
> > >      }
> > >
> > > diff --git a/block/vhdx.h b/block/vhdx.h
> > > index 1bfb4e4f73..deb6e362ad 100644
> > > --- a/block/vhdx.h
> > > +++ b/block/vhdx.h
> > > @@ -18,11 +18,6 @@
> > >  #ifndef BLOCK_VHDX_H
> > >  #define BLOCK_VHDX_H
> > >
> > > -#define KiB              (1 * 1024)
> > > -#define MiB            (KiB * 1024)
> > > -#define GiB            (MiB * 1024)
> > > -#define TiB ((uint64_t) GiB * 1024)
> > > -
> >
> > I'd include units.h here because there are other references to
> > KiB/MiB/GiB and so on throughout the rest of this header file.
> >
> > I think it's bad form to have a header that cannot be included unless
> > you include other headers manually alongside it.
> >
> 
> Agreed.

Okay, I'll include units.h here.

> 
> 
> > >  #define DEFAULT_LOG_SIZE 1048576 /* 1MiB */
> >
> 
> Can we also update this definition?
> 

Sure, I'll update it in v2.


Thanks for the review,
Stefano
Re: [Qemu-devel] [Qemu-block] [PATCH] block/vhdx: Use IEC binary prefixes from "qemu/units.h"
Posted by Kevin Wolf 6 years, 10 months ago 
Am 26.03.2019 um 08:55 hat Stefano Garzarella geschrieben:
> On Mon, Mar 25, 2019 at 10:56:57PM +0100, Philippe Mathieu-Daudé wrote:
> > Le lun. 25 mars 2019 21:53, John Snow <jsnow@redhat.com> a écrit :
> > > On 3/21/19 5:40 AM, Stefano Garzarella wrote:
> > > > IEC binary prefixes are defined in "qemu/units.h", so we
> > > > can remove redundant definitions in "block/vhdx.h".
> > > >
> > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > ---
> > > >  block/vhdx-log.c | 1 +
> > > >  block/vhdx.c     | 4 +++-
> > > >  block/vhdx.h     | 5 -----
> > > >  3 files changed, 4 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/block/vhdx-log.c b/block/vhdx-log.c
> > > > index 3149ff08d8..ad9905eb6d 100644
> > > > --- a/block/vhdx-log.c
> > > > +++ b/block/vhdx-log.c
> > > > @@ -18,6 +18,7 @@
> > > >   *
> > > >   */
> > > >  #include "qemu/osdep.h"
> > > > +#include "qemu/units.h"
> > > >  #include "qapi/error.h"
> > > >  #include "qemu-common.h"
> > > >  #include "block/block_int.h"
> > > > diff --git a/block/vhdx.c b/block/vhdx.c
> > > > index b785aef4b7..e9b16c5ef6 100644
> > > > --- a/block/vhdx.c
> > > > +++ b/block/vhdx.c
> > > > @@ -16,6 +16,7 @@
> > > >   */
> > > >
> > > >  #include "qemu/osdep.h"
> > > > +#include "qemu/units.h"
> > > >  #include "qapi/error.h"
> > > >  #include "block/block_int.h"
> > > >  #include "block/qdict.h"
> > > > @@ -1889,7 +1890,8 @@ static int coroutine_fn
> > > vhdx_co_create(BlockdevCreateOptions *opts,
> > > >          return -EINVAL;
> > > >      }
> > > >      if (block_size > VHDX_BLOCK_SIZE_MAX) {
> > > > -        error_setg(errp, "Block size must not exceed %d",
> > > VHDX_BLOCK_SIZE_MAX);
> > > > +        error_setg(errp, "Block size must not exceed %" PRId64,
> > > > +                   VHDX_BLOCK_SIZE_MAX);
> > >
> > > Oh, changing the constants changes the inferred type of this constant,
> > > correct?
> > >
> > 
> 
> Yes, in "units.h" all the constants are int64_t.
> 
> > Yes.
> > 
> > 
> > > >          return -EINVAL;
> > > >      }
> > > >
> > > > diff --git a/block/vhdx.h b/block/vhdx.h
> > > > index 1bfb4e4f73..deb6e362ad 100644
> > > > --- a/block/vhdx.h
> > > > +++ b/block/vhdx.h
> > > > @@ -18,11 +18,6 @@
> > > >  #ifndef BLOCK_VHDX_H
> > > >  #define BLOCK_VHDX_H
> > > >
> > > > -#define KiB              (1 * 1024)
> > > > -#define MiB            (KiB * 1024)
> > > > -#define GiB            (MiB * 1024)
> > > > -#define TiB ((uint64_t) GiB * 1024)
> > > > -
> > >
> > > I'd include units.h here because there are other references to
> > > KiB/MiB/GiB and so on throughout the rest of this header file.
> > >
> > > I think it's bad form to have a header that cannot be included unless
> > > you include other headers manually alongside it.
> > >
> > 
> > Agreed.
> 
> Okay, I'll include units.h here.
> 
> > 
> > 
> > > >  #define DEFAULT_LOG_SIZE 1048576 /* 1MiB */
> > >
> > 
> > Can we also update this definition?
> > 
> 
> Sure, I'll update it in v2.

This is used in vhdx_create_opts:

    .def_value_str = stringify(DEFAULT_LOG_SIZE),

You want the default size to be a number, not an expression, so you'd
have to change the default string to retain the number. Probably better
to just leave the #define alone.

We could, however, put a comment next to the define lik vdi.c has:

    /* Note: can't use 1 * MiB, because it's passed to stringify() */

Kevin
Re: [Qemu-devel] [Qemu-block] [PATCH] block/vhdx: Use IEC binary prefixes from "qemu/units.h"
Posted by Stefano Garzarella 6 years, 10 months ago 
On Tue, Mar 26, 2019 at 10:13:54AM +0100, Kevin Wolf wrote:
> Am 26.03.2019 um 08:55 hat Stefano Garzarella geschrieben:
> > On Mon, Mar 25, 2019 at 10:56:57PM +0100, Philippe Mathieu-Daudé wrote:
> > > Le lun. 25 mars 2019 21:53, John Snow <jsnow@redhat.com> a écrit :
> > > > On 3/21/19 5:40 AM, Stefano Garzarella wrote:
> > > > >  #define DEFAULT_LOG_SIZE 1048576 /* 1MiB */
> > > >
> > > 
> > > Can we also update this definition?
> > > 
> > 
> > Sure, I'll update it in v2.
> 
> This is used in vhdx_create_opts:
> 
>     .def_value_str = stringify(DEFAULT_LOG_SIZE),
> 
> You want the default size to be a number, not an expression, so you'd
> have to change the default string to retain the number. Probably better
> to just leave the #define alone.
> 
> We could, however, put a comment next to the define lik vdi.c has:
> 
>     /* Note: can't use 1 * MiB, because it's passed to stringify() */

Many thanks for this suggestion!
I'll put the comment next to the define.

Thanks,
Stefano
Re: [Qemu-devel] [Qemu-block] [PATCH] block/vhdx: Use IEC binary prefixes from "qemu/units.h"
Posted by Philippe Mathieu-Daudé 6 years, 10 months ago 
Le mar. 26 mars 2019 10:14, Kevin Wolf <kwolf@redhat.com> a écrit :

> Am 26.03.2019 um 08:55 hat Stefano Garzarella geschrieben:
> > On Mon, Mar 25, 2019 at 10:56:57PM +0100, Philippe Mathieu-Daudé wrote:
> > > Le lun. 25 mars 2019 21:53, John Snow <jsnow@redhat.com> a écrit :
> > > > On 3/21/19 5:40 AM, Stefano Garzarella wrote:
> > > > > IEC binary prefixes are defined in "qemu/units.h", so we
> > > > > can remove redundant definitions in "block/vhdx.h".
> > > > >
> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > > ---
> > > > >  block/vhdx-log.c | 1 +
> > > > >  block/vhdx.c     | 4 +++-
> > > > >  block/vhdx.h     | 5 -----
> > > > >  3 files changed, 4 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/block/vhdx-log.c b/block/vhdx-log.c
> > > > > index 3149ff08d8..ad9905eb6d 100644
> > > > > --- a/block/vhdx-log.c
> > > > > +++ b/block/vhdx-log.c
> > > > > @@ -18,6 +18,7 @@
> > > > >   *
> > > > >   */
> > > > >  #include "qemu/osdep.h"
> > > > > +#include "qemu/units.h"
> > > > >  #include "qapi/error.h"
> > > > >  #include "qemu-common.h"
> > > > >  #include "block/block_int.h"
> > > > > diff --git a/block/vhdx.c b/block/vhdx.c
> > > > > index b785aef4b7..e9b16c5ef6 100644
> > > > > --- a/block/vhdx.c
> > > > > +++ b/block/vhdx.c
> > > > > @@ -16,6 +16,7 @@
> > > > >   */
> > > > >
> > > > >  #include "qemu/osdep.h"
> > > > > +#include "qemu/units.h"
> > > > >  #include "qapi/error.h"
> > > > >  #include "block/block_int.h"
> > > > >  #include "block/qdict.h"
> > > > > @@ -1889,7 +1890,8 @@ static int coroutine_fn
> > > > vhdx_co_create(BlockdevCreateOptions *opts,
> > > > >          return -EINVAL;
> > > > >      }
> > > > >      if (block_size > VHDX_BLOCK_SIZE_MAX) {
> > > > > -        error_setg(errp, "Block size must not exceed %d",
> > > > VHDX_BLOCK_SIZE_MAX);
> > > > > +        error_setg(errp, "Block size must not exceed %" PRId64,
> > > > > +                   VHDX_BLOCK_SIZE_MAX);
> > > >
> > > > Oh, changing the constants changes the inferred type of this
> constant,
> > > > correct?
> > > >
> > >
> >
> > Yes, in "units.h" all the constants are int64_t.
> >
> > > Yes.
> > >
> > >
> > > > >          return -EINVAL;
> > > > >      }
> > > > >
> > > > > diff --git a/block/vhdx.h b/block/vhdx.h
> > > > > index 1bfb4e4f73..deb6e362ad 100644
> > > > > --- a/block/vhdx.h
> > > > > +++ b/block/vhdx.h
> > > > > @@ -18,11 +18,6 @@
> > > > >  #ifndef BLOCK_VHDX_H
> > > > >  #define BLOCK_VHDX_H
> > > > >
> > > > > -#define KiB              (1 * 1024)
> > > > > -#define MiB            (KiB * 1024)
> > > > > -#define GiB            (MiB * 1024)
> > > > > -#define TiB ((uint64_t) GiB * 1024)
> > > > > -
> > > >
> > > > I'd include units.h here because there are other references to
> > > > KiB/MiB/GiB and so on throughout the rest of this header file.
> > > >
> > > > I think it's bad form to have a header that cannot be included unless
> > > > you include other headers manually alongside it.
> > > >
> > >
> > > Agreed.
> >
> > Okay, I'll include units.h here.
> >
> > >
> > >
> > > > >  #define DEFAULT_LOG_SIZE 1048576 /* 1MiB */
> > > >
> > >
> > > Can we also update this definition?
> > >
> >
> > Sure, I'll update it in v2.
>
> This is used in vhdx_create_opts:
>
>     .def_value_str = stringify(DEFAULT_LOG_SIZE),
>
> You want the default size to be a number, not an expression, so you'd
> have to change the default string to retain the number. Probably better
> to just leave the #define alone.
>

Oops I guess remember we had the same discussion 1 year ago...


> We could, however, put a comment next to the define lik vdi.c has:
>
>     /* Note: can't use 1 * MiB, because it's passed to stringify() */
>
> Kevin
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.