[libvirt] [PATCH] Fix qemuMigrationToFile nonull annotation
Eric Blake
eblake at redhat.com
Thu May 5 11:49:27 UTC 2011
On 05/05/2011 04:38 AM, Daniel P. Berrange wrote:
> The qemuMigrationToFile method was accidentally annotated for
> the 'compressor' parameter to be non-null, instead of the
> 'path' parameter. Thus GCC with -O2, unhelpfully deleted the
> entire 'if (compressor == NULL)' block of code during
> optimization. Thus NULL was passed to virCommandNew() with
> predictably bad results.
Regression introduced in commit 7c31e1e, so fortunately 0.9.1 is immune.
> This shows that the 'ATTRIBUTE_NONNULL' annotation is actually
> really very dangerous to use. GCC is incapable of issuing any
> warnings about callers passing NULL, unless they pass a literal
> "NULL". If the caller does 'void *p = NULL; foo(p)' it will not
> warn. GCC is also not warning about the fact that there is a
> huge block of code for 'if (compressor == NULL)' that is "dead"
> code and being deleted.
>
> While it is perhaps nice to have ATTRIBUTE_NONNULL for static
> analysis tools like clang, IMHO, it is too dangerous for us
> to continue to have it enabled in builds. I think we should
> define it to a no-op macro, unless explicitly enabled with
> configure.
Hmm, maybe you're right, although it should still be defined for cases
when compiling with a static analyzer (such as how sa_assert is a no-op
for gcc but does something important for clang).
Maybe we should also file an enhancement request against gcc, to alter
the syntax for attribute-nonnull. That is,
void f(void *a, void *b) ATTRIBUTE_NONNULL(2,b)
would be nicer, where gcc complains if parameter 2 is not named b, so
that the mere deletion of parameter a catches this sort of mistake faster.
> +++ b/src/qemu/qemu_migration.h
> @@ -64,7 +64,7 @@ int qemuMigrationToFile(struct qemud_driver *driver, virDomainObjPtr vm,
> int fd, off_t offset, const char *path,
> const char *compressor,
> bool is_reg, bool bypassSecurityDriver)
> - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(6)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5)
> ATTRIBUTE_RETURN_CHECK;
ACK.
--
Eric Blake eblake at redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110505/2c7313b1/attachment-0001.sig>
More information about the libvir-list
mailing list