Le mar. 26 mars 2019 10:14, Kevin Wolf <kw...@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 <js...@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 <sgarz...@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
>
