Re: [libvirt] [RFC PATCH 5/6] qemu: Switch over command line capabilities to virBitmap
On 02/09/2011 09:02 AM, Jiri Denemark wrote:
> This is done for two reasons:
> - we are getting very close to 64 flags which is the maximum we can use
> with unsigned long long
> - by using LL constants in enum we already violates C99 constraint that
> enum values have to fit into int
Does gcc warn about that (possible via some -W flag that we aren't
currently using)?
It doesn't warn about it by default and I'm not aware of
any flag which would
turn this warning on.
Or are we silently relying on a (common) extension that might bite
us
elsewhere?
Apparently yes.
It already bit me when I wanted to change tests to use qemuCaps* functions
before the patch that starts using virBitmap. Passing enum values as variable
arguments didn't work at all since the values didn't have all the same type.
Those that fit into int were ints and the others were long.
Which reminds me I have a possible bug in the last patch...
> - *flags = qemuCapsComputeCmdFlags(help, *version, *is_kvm,
*kvm_version);
> + qemuCapsComputeCmdFlags(help, *version, *is_kvm, *kvm_version, flags);
> +
> + if (!(strflags = virBitmapString(flags))) {
> + virReportOOMError();
> + return -1;
> + }
> +
> + VIR_DEBUG("Version %u.%u.%u, cooked version %u, flags %s",
> + major, minor, micro, *version, strflags);
> + VIR_FREE(strflags);
Using virReportOOMError() is harsh, when the only thing that we lack is
a VIR_DEBUG(). Would it be any better to do:
if ((strflags = virBitmapString(flags))) {
VIR_DEBUG(...);
VIR_FREE(strflags);
}
and just lose the debug message if we're tight on memory?
Yeah, that's probably better, although it wouldn't make a huge difference in
practise anyway. After all, logging functions also just ignore OOM conditions.
> void
> -qemuCapsClear(unsigned long long *caps,
> +qemuCapsClear(virBitmapPtr caps,
> enum qemuCapsFlags flag)
> {
> - *caps &= ~flag;
> + ignore_value(virBitmapClearBit(caps, flag));
So why does this require caps to exist...
Trying to set caps that are NULL is more a bug in the code than something that
is intentional (as oppose to the Get case) and I prefer to get a SIGSEGV
instead of magically hiding that, esp. since gcc is not very clever (see
below).
> }
>
>
> bool
> -qemuCapsGet(unsigned long long caps,
> +qemuCapsGet(virBitmapPtr caps,
> enum qemuCapsFlags flag)
> {
> - return !!(caps & flag);
> + bool b;
> +
> + if (!caps || virBitmapGetBit(caps, flag, &b) < 0)
while this does not? Shouldn't qemuCapsGet be marked ATTRIBUTE_NONNULL(1)?
This is mostly for two reasons. First, gcc only warns on ATTRIBUTE_NONNULL if
NULL is explicitly passed as a parameter and not if it's done through a
variable. So it doesn't really help with detecting cases where NULL is
indirectly passed to qemuCapsGet(). Second, there are places (actually it's
probably just one case currently) in the code where we don't care what the
qemu binary is capable of when calling a function that expect qemuCaps
argument. So we want to pass NULL there. And it seems more practical to me to
do the test inside qemuCapsGet that in any place where it could possibly be
called with NULL.
>
> -void qemuCapsSet(unsigned long long *caps,
> - enum qemuCapsFlags flag);
> +virBitmapPtr qemuCapsNew(void);
> +
> +# define qemuCapsFree(caps) virBitmapFree(caps)
Add this to cfg.mk's list of free-like functions.
Ah, I forgot to do that because I originally wanted to just get rid of
qemuCapsFree() and call virBitmapFree directly instead. I didn't do that so
I'll add the macro to cfg.mk.
> -bool qemuCapsGet(unsigned long long caps,
> +bool qemuCapsGet(virBitmapPtr caps,
> enum qemuCapsFlags flag);
I guess you really did intend for qemuCapsGet to work even if
qemuCapsFree failed.
I didn't get this last comment. Could you be more specific about what you
wanted to say? :-)
Jirka