[Qemu-devel] [PATCH] qemu/units: Move out QCow2 specific definitions

[Qemu-devel] [PATCH] qemu/units: Move out QCow2 specific definitions

[Philippe Mathieu-Daudé]

This definitions are QCow2 specific, there is no need to expose them
in the global namespace.

This partially reverts commit 540b8492618eb.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/qcow2.h        | 56 +++++++++++++++++++++++++++++++++++++++++++-
 include/qemu/units.h | 55 -------------------------------------------
 2 files changed, 55 insertions(+), 56 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 29c98d87a0..74d200c8cb 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -27,12 +27,66 @@
 
 #include "crypto/block.h"
 #include "qemu/coroutine.h"
-#include "qemu/units.h"
 
 //#define DEBUG_ALLOC
 //#define DEBUG_ALLOC2
 //#define DEBUG_EXT
 
+#define S_1KiB                  1024
+#define S_2KiB                  2048
+#define S_4KiB                  4096
+#define S_8KiB                  8192
+#define S_16KiB                16384
+#define S_32KiB                32768
+#define S_64KiB                65536
+#define S_128KiB              131072
+#define S_256KiB              262144
+#define S_512KiB              524288
+#define S_1MiB               1048576
+#define S_2MiB               2097152
+#define S_4MiB               4194304
+#define S_8MiB               8388608
+#define S_16MiB             16777216
+#define S_32MiB             33554432
+#define S_64MiB             67108864
+#define S_128MiB           134217728
+#define S_256MiB           268435456
+#define S_512MiB           536870912
+#define S_1GiB            1073741824
+#define S_2GiB            2147483648
+#define S_4GiB            4294967296
+#define S_8GiB            8589934592
+#define S_16GiB          17179869184
+#define S_32GiB          34359738368
+#define S_64GiB          68719476736
+#define S_128GiB        137438953472
+#define S_256GiB        274877906944
+#define S_512GiB        549755813888
+#define S_1TiB         1099511627776
+#define S_2TiB         2199023255552
+#define S_4TiB         4398046511104
+#define S_8TiB         8796093022208
+#define S_16TiB       17592186044416
+#define S_32TiB       35184372088832
+#define S_64TiB       70368744177664
+#define S_128TiB     140737488355328
+#define S_256TiB     281474976710656
+#define S_512TiB     562949953421312
+#define S_1PiB      1125899906842624
+#define S_2PiB      2251799813685248
+#define S_4PiB      4503599627370496
+#define S_8PiB      9007199254740992
+#define S_16PiB    18014398509481984
+#define S_32PiB    36028797018963968
+#define S_64PiB    72057594037927936
+#define S_128PiB  144115188075855872
+#define S_256PiB  288230376151711744
+#define S_512PiB  576460752303423488
+#define S_1EiB   1152921504606846976
+#define S_2EiB   2305843009213693952
+#define S_4EiB   4611686018427387904
+#define S_8EiB   9223372036854775808
+
 #define QCOW_MAGIC (('Q' << 24) | ('F' << 16) | ('I' << 8) | 0xfb)
 
 #define QCOW_CRYPT_NONE 0
diff --git a/include/qemu/units.h b/include/qemu/units.h
index 68a7758650..692db3fbb2 100644
--- a/include/qemu/units.h
+++ b/include/qemu/units.h
@@ -17,59 +17,4 @@
 #define PiB     (INT64_C(1) << 50)
 #define EiB     (INT64_C(1) << 60)
 
-#define S_1KiB                  1024
-#define S_2KiB                  2048
-#define S_4KiB                  4096
-#define S_8KiB                  8192
-#define S_16KiB                16384
-#define S_32KiB                32768
-#define S_64KiB                65536
-#define S_128KiB              131072
-#define S_256KiB              262144
-#define S_512KiB              524288
-#define S_1MiB               1048576
-#define S_2MiB               2097152
-#define S_4MiB               4194304
-#define S_8MiB               8388608
-#define S_16MiB             16777216
-#define S_32MiB             33554432
-#define S_64MiB             67108864
-#define S_128MiB           134217728
-#define S_256MiB           268435456
-#define S_512MiB           536870912
-#define S_1GiB            1073741824
-#define S_2GiB            2147483648
-#define S_4GiB            4294967296
-#define S_8GiB            8589934592
-#define S_16GiB          17179869184
-#define S_32GiB          34359738368
-#define S_64GiB          68719476736
-#define S_128GiB        137438953472
-#define S_256GiB        274877906944
-#define S_512GiB        549755813888
-#define S_1TiB         1099511627776
-#define S_2TiB         2199023255552
-#define S_4TiB         4398046511104
-#define S_8TiB         8796093022208
-#define S_16TiB       17592186044416
-#define S_32TiB       35184372088832
-#define S_64TiB       70368744177664
-#define S_128TiB     140737488355328
-#define S_256TiB     281474976710656
-#define S_512TiB     562949953421312
-#define S_1PiB      1125899906842624
-#define S_2PiB      2251799813685248
-#define S_4PiB      4503599627370496
-#define S_8PiB      9007199254740992
-#define S_16PiB    18014398509481984
-#define S_32PiB    36028797018963968
-#define S_64PiB    72057594037927936
-#define S_128PiB  144115188075855872
-#define S_256PiB  288230376151711744
-#define S_512PiB  576460752303423488
-#define S_1EiB   1152921504606846976
-#define S_2EiB   2305843009213693952
-#define S_4EiB   4611686018427387904
-#define S_8EiB   9223372036854775808
-
 #endif
-- 
2.17.2

Re: [Qemu-devel] [PATCH] qemu/units: Move out QCow2 specific definitions

[Kevin Wolf]

Am 02.11.2018 um 09:58 hat Philippe Mathieu-Daudé geschrieben:
> This definitions are QCow2 specific, there is no need to expose them
> in the global namespace.
> 
> This partially reverts commit 540b8492618eb.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

If we don't want this globally, I think we also don't want it in qcow2.
Or at least reduce it to only those constants that qcow2 actually uses.

Kevin

Re: [Qemu-devel] [PATCH] qemu/units: Move out QCow2 specific definitions

[Philippe Mathieu-Daudé]

Hi Kevin,

On 2/11/18 12:07, Kevin Wolf wrote:
> Am 02.11.2018 um 09:58 hat Philippe Mathieu-Daudé geschrieben:
>> This definitions are QCow2 specific, there is no need to expose them
>> in the global namespace.
>>
>> This partially reverts commit 540b8492618eb.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> If we don't want this globally, I think we also don't want it in qcow2.

I only see this definitions used by block/qcow2.h (b6a95c6d1007).

Per 540b8492618eb description "This is needed when a size has to be 
stringified" but I can't find other code requiring these definitions in 
the codebase.

> Or at least reduce it to only those constants that qcow2 actually uses.

Fine by me, I'll let Leonid opine first.

Regards,

Phil.

Re: [Qemu-devel] [PATCH] qemu/units: Move out QCow2 specific definitions

[Kevin Wolf]

Am 02.11.2018 um 13:37 hat Philippe Mathieu-Daudé geschrieben:
> Hi Kevin,
> 
> On 2/11/18 12:07, Kevin Wolf wrote:
> > Am 02.11.2018 um 09:58 hat Philippe Mathieu-Daudé geschrieben:
> > > This definitions are QCow2 specific, there is no need to expose them
> > > in the global namespace.
> > > 
> > > This partially reverts commit 540b8492618eb.
> > > 
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > 
> > If we don't want this globally, I think we also don't want it in qcow2.
> 
> I only see this definitions used by block/qcow2.h (b6a95c6d1007).
> 
> Per 540b8492618eb description "This is needed when a size has to be
> stringified" but I can't find other code requiring these definitions in the
> codebase.

I guess the real question is: Is qcow2 the only place that needs
stringification of sizes?

The only value where this actually seems to be used in qcow2 is for
DEFAULT_CLUSTER_SIZE, as the default value for QemuOpts. Other drivers
still use plain numbers, but this is less readable.

Then there is VDI which uses (1 * MiB), but that is compiled out and if
you enable it, it breaks. So it needs the same fix.

Are block drivers the only places where we stringify a size? I imagine
some device models might use something like it, too?

I don't mind too much which solution we end up using, but I'd prefer it
to be universal.

Kevin

Re: [Qemu-devel] [PATCH] qemu/units: Move out QCow2 specific definitions

[Eric Blake]

On 11/2/18 9:10 AM, Kevin Wolf wrote:
> Am 02.11.2018 um 13:37 hat Philippe Mathieu-Daudé geschrieben:
>> Hi Kevin,
>>
>> On 2/11/18 12:07, Kevin Wolf wrote:
>>> Am 02.11.2018 um 09:58 hat Philippe Mathieu-Daudé geschrieben:
>>>> This definitions are QCow2 specific, there is no need to expose them
>>>> in the global namespace.
>>>>
>>>> This partially reverts commit 540b8492618eb.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>
>>> If we don't want this globally, I think we also don't want it in qcow2.

Agreed. I didn't want it in the first place, arguing that if we want 
stringification of defaults, it would be better to have a runtime 
function do that, rather than adding a set of near-duplicate macro names.

>>
>> I only see this definitions used by block/qcow2.h (b6a95c6d1007).
>>
>> Per 540b8492618eb description "This is needed when a size has to be
>> stringified" but I can't find other code requiring these definitions in the
>> codebase.
> 
> I guess the real question is: Is qcow2 the only place that needs
> stringification of sizes?

Probably not. It seems like stringifying a default value is a common desire.

> 
> The only value where this actually seems to be used in qcow2 is for
> DEFAULT_CLUSTER_SIZE, as the default value for QemuOpts. Other drivers
> still use plain numbers, but this is less readable.
> 
> Then there is VDI which uses (1 * MiB), but that is compiled out and if
> you enable it, it breaks. So it needs the same fix.
> 
> Are block drivers the only places where we stringify a size? I imagine
> some device models might use something like it, too?

Indeed, I would prefer a patch that makes it possible for QemuOpts to 
pretty-print a default value using a generic runtime stringifier, rather 
than keeping these S_ macros around.

> 
> I don't mind too much which solution we end up using, but I'd prefer it
> to be universal.

Agree.

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

Re: [Qemu-devel] [PATCH] qemu/units: Move out QCow2 specific definitions

[Kevin Wolf]

Am 02.11.2018 um 15:52 hat Eric Blake geschrieben:
> On 11/2/18 9:10 AM, Kevin Wolf wrote:
> > Am 02.11.2018 um 13:37 hat Philippe Mathieu-Daudé geschrieben:
> > > Hi Kevin,
> > > 
> > > On 2/11/18 12:07, Kevin Wolf wrote:
> > > > Am 02.11.2018 um 09:58 hat Philippe Mathieu-Daudé geschrieben:
> > > > > This definitions are QCow2 specific, there is no need to expose them
> > > > > in the global namespace.
> > > > > 
> > > > > This partially reverts commit 540b8492618eb.
> > > > > 
> > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > > > 
> > > > If we don't want this globally, I think we also don't want it in qcow2.
> 
> Agreed. I didn't want it in the first place, arguing that if we want
> stringification of defaults, it would be better to have a runtime function
> do that, rather than adding a set of near-duplicate macro names.
> 
> > > 
> > > I only see this definitions used by block/qcow2.h (b6a95c6d1007).
> > > 
> > > Per 540b8492618eb description "This is needed when a size has to be
> > > stringified" but I can't find other code requiring these definitions in the
> > > codebase.
> > 
> > I guess the real question is: Is qcow2 the only place that needs
> > stringification of sizes?
> 
> Probably not. It seems like stringifying a default value is a common desire.
> 
> > 
> > The only value where this actually seems to be used in qcow2 is for
> > DEFAULT_CLUSTER_SIZE, as the default value for QemuOpts. Other drivers
> > still use plain numbers, but this is less readable.
> > 
> > Then there is VDI which uses (1 * MiB), but that is compiled out and if
> > you enable it, it breaks. So it needs the same fix.
> > 
> > Are block drivers the only places where we stringify a size? I imagine
> > some device models might use something like it, too?
> 
> Indeed, I would prefer a patch that makes it possible for QemuOpts to
> pretty-print a default value using a generic runtime stringifier, rather
> than keeping these S_ macros around.

The thing is just, QemuOpts is completetly string based. The default
value field is const char*. Either we get rid of QemuOpts and switch
everything to QAPI (nice thought, but a little unrealistic in the short
term), or we add ways to add non-string values to QemuOpts (would
require significant development on a piece of code we want to get rid of
in the long term), or you keep doing stringification at build time
(which I believe is the only reasonable choice at the moment).

Kevin

Re: [Qemu-devel] [PATCH] qemu/units: Move out QCow2 specific definitions

[Leonid Bloch]

Hi,

On 11/2/18 5:28 PM, Kevin Wolf wrote:
> Am 02.11.2018 um 15:52 hat Eric Blake geschrieben:
>> On 11/2/18 9:10 AM, Kevin Wolf wrote:
>>> Am 02.11.2018 um 13:37 hat Philippe Mathieu-Daudé geschrieben:
>>>> Hi Kevin,
>>>>
>>>> On 2/11/18 12:07, Kevin Wolf wrote:
>>>>> Am 02.11.2018 um 09:58 hat Philippe Mathieu-Daudé geschrieben:
>>>>>> This definitions are QCow2 specific, there is no need to expose them
>>>>>> in the global namespace.

These are not QCOW2 specific. I wrote these for convenience in QCOW2, 
but there are many other places where these can be used (many 
pre-defined sizes are powers of two), and there are few places where 
they must replace the current notation, like in block/vdi.c with 
DEFAULT_CLUSTER_SIZE (unless an explicit value in bytes will be defined 
instead).

>>
>> Agreed. I didn't want it in the first place, arguing that if we want
>> stringification of defaults, it would be better to have a runtime function
>> do that, rather than adding a set of near-duplicate macro names.

A runtime function will not help here, as these are used in compile 
time. These result in strings that are actually compiled into the binaries.

>>>
>>> Then there is VDI which uses (1 * MiB), but that is compiled out and if
>>> you enable it, it breaks. So it needs the same fix.

Yeah, I need to fix that as promised. Will do shortly. :)

Leonid.

Re: [Qemu-devel] [PATCH] qemu/units: Move out QCow2 specific definitions

[Eric Blake]

On 11/2/18 7:29 PM, Leonid Bloch wrote:
>>> Agreed. I didn't want it in the first place, arguing that if we want
>>> stringification of defaults, it would be better to have a runtime function
>>> do that, rather than adding a set of near-duplicate macro names.
> 
> A runtime function will not help here, as these are used in compile
> time. These result in strings that are actually compiled into the binaries.

Well, my point is that right now, QemuOpts outputs a hard-coded string 
(with no alternative), which does mean that things are compiled in.  Is 
it worth exploring an enhancement to QemuOpts that lets it decide 
between either a const char * hardcoded string, or a runtime formatter 
callback function, and convert all existing hardcoded strings with 
awkward contents to instead use a new runtime formatter?

Or is that just putting lipstick on a pig, since we are already trying 
to move away from QemuOpts into a more structured command line 
introspection?

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