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)? Or are we silently relying on a (common) extension
that might bite us elsewhere?
> - *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? Then again,
if we're that close to running out of memory, it won't be much longer
before something else pushes us over the boundary, so giving up now
might not be too bad.
> 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...
> }
>
>
> 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)?
>
> -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.
> -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 like the idea of this series, and agree that it's too late to make it
into 0.8.8. It's probably worth posting a v2 to address comments I
raised, although an incremental diff might be easier to review at that
point.
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list