| Message ID | 20190321094012.36541-1-sgarzare@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block/vhdx: Use IEC binary prefixes from "qemu/units.h" | expand |
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 */ > >
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 */ > > > > > > >
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
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
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 >
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
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 */
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(-)