Re: [Qemu-devel] [PATCH] Permit zero-sized qemu_malloc() & friends

[Anthony Liguori]

Markus Armbruster wrote:
> Commit a7d27b53 made zero-sized allocations a fatal error, deviating
> from ISO C's malloc() & friends.  Revert that, but take care never to
> return a null pointer, like malloc() & friends may do (it's
> implementation defined), because that's another source of bugs.
>
> Rationale: while zero-sized allocations might occasionally be a sign of
> something going wrong, they can also be perfectly legitimate.  The
> change broke such legitimate uses.  We've found and "fixed" at least one
> of them already (commit eb0b64f7, also reverted by this patch), and
> another one just popped up: the change broke qcow2 images with virtual
> disk size zero, i.e. images that don't hold real data but only VM state
> of snapshots.
>   

I still believe that it is poor practice to pass size==0 to *malloc().  
I think actively discouraging this in qemu is a good thing because it's 
a broken idiom.

That said, we've had this change in for a while and it's painfully 
obviously we haven't eliminated all of these instances in qemu.  Knowing 
that we still have occurrences of this and actively exit()'ing when they 
happen is pretty much suicidal in a production environment.  It's not a 
bug, it's just a poor practice.

Here's what I propose.  I think we should introduce a 
CONFIG_DEBUG_ZERO_MALLOC.  When this is set, we should assert(size!=0).  
Otherwise, we should return malloc(1) as Markus' patch does below.

Using the same rules as we follow for -Werror, we should enable 
CONFIG_DEBUG_ZERO_MALLOC for the development tree and disable it for any 
releases.

This helps us make qemu better during development while not 
unnecessarily causing us harm in a production environment.  What happens 
here long term I think remains to be seen.  But for right now, I think 
this is an important change to make for 0.12.

> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  block/qcow2-snapshot.c |    5 -----
>  qemu-malloc.c          |   14 ++------------
>  2 files changed, 2 insertions(+), 17 deletions(-)
>
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index 94cb838..e3b208c 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -381,11 +381,6 @@ int qcow2_snapshot_list(BlockDriverState *bs, 
> QEMUSnapshotInfo **psn_tab)
>      QCowSnapshot *sn;
>      int i;
>  
> -    if (!s->nb_snapshots) {
> -        *psn_tab = NULL;
> -        return s->nb_snapshots;
> -    }
> -
>   

This does not belong here.

Regards,

Anthony Liguori