block/vhdx-log.c | 1 + block/vhdx.c | 4 +++- block/vhdx.h | 5 ----- 3 files changed, 4 insertions(+), 6 deletions(-)
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
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
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
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 >
© 2016 - 2024 Red Hat, Inc.