Re: [PATCH v2] util/qemu-option: Document the get_opt_value() function

[Philippe Mathieu-Daudé]

On 7/7/20 3:14 AM, Richard Henderson wrote:
> On 6/29/20 12:08 AM, Philippe Mathieu-Daudé wrote:
>> Coverity noticed commit 950c4e6c94 introduced a dereference before
>> null check in get_opt_value (CID1391003):
>>
>>   In get_opt_value: All paths that lead to this null pointer
>>   comparison already dereference the pointer earlier (CWE-476)
>>
>> We fixed this in commit 6e3ad3f0e31, but relaxed the check in commit
>> 0c2f6e7ee99 because "No callers of get_opt_value() pass in a NULL
>> for the 'value' parameter".
>>
>> Since this function is publicly exposed, it risks new users to do
>> the same error again. Avoid that documenting the 'value' argument
>> must not be NULL.
> 
> I think we should also add some use of __attribute__((nonnull(...))) to 
> enforce
> this within the compiler.
> 
> I recently did this without a qemu/compiler.h QEMU_FOO wrapper within
> target/arm.  But the nonnull option has optional arguments, so it might be
> difficult to wrap in macros.

I have this patch after your suggestion from last year:

+#if __has_attribute(nonnull)
+# define QEMU_NONNULL(LIST) __attribute__((nonnull((LIST))))
+#else
+# define QEMU_NONNULL(LIST)
+#endif

Examples:

 SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type,
-                                         uint32_t id);
+                                 uint32_t id) QEMU_NONNULL(1);
 SpaprDrc *spapr_drc_by_index(uint32_t index);
 SpaprDrc *spapr_drc_by_id(const char *type, uint32_t id);
-int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t
drc_type_mask);
+int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t
drc_type_mask)
+                 QEMU_NONNULL(3);

...


 /**
  * memory_region_init_iommu: Initialize a memory region of a custom type
@@ -1066,7 +1073,8 @@ void memory_region_init_iommu(void *_iommu_mr,
                               const char *mrtypename,
                               Object *owner,
                               const char *name,
-                              uint64_t size);
+                              uint64_t size)
+                              QEMU_NONNULL(4);

 /**
  * memory_region_init_ram - Initialize RAM memory region.  Accesses
into the
@@ -1154,7 +1162,8 @@ void memory_region_init_rom_device(MemoryRegion *mr,
                                    void *opaque,
                                    const char *name,
                                    uint64_t size,
-                                   Error **errp);
+                                   Error **errp)
+                                   QEMU_NONNULL(2);

I can send as RFC is that looks OK to you.

Regards,

Phil.