| Message ID | 20181102085800.21860-1-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu/units: Move out QCow2 specific definitions | expand |
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
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.
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
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.
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
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.
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?
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
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(-)