[libvirt] PATCH: 15/28: Reduce return points for storage driver

[Daniel P. Berrange]

On Tue, Dec 02, 2008 at 03:32:28PM +0100, Jim Meyering wrote:
> "Daniel P. Berrange" <berrange at redhat.com> wrote:
> 
> > On Tue, Dec 02, 2008 at 03:02:17PM +0100, Jim Meyering wrote:
> >> "Daniel P. Berrange" <berrange at redhat.com> wrote:
> >> > This patch reduces the number of return points in the storage driver
> >> > methods
> >> ...
> >> > diff --git a/src/storage_driver.c b/src/storage_driver.c
> >> ...
> >> > @@ -893,7 +924,7 @@ storagePoolListVolumes(virStoragePoolPtr
> >> >
> >> >   cleanup:
> >> >      for (n = 0 ; n < maxnames ; n++)
> >> > -        VIR_FREE(names[i]);
> >> > +        VIR_FREE(names[n]);
> >> >
> >> >      memset(names, 0, maxnames);
> >> >      return -1;
> >>
> >> This might be worth putting in a separate bug-fix patch.
> >> At first I thought this was fixing a serious bug,
> >> but you can see that i is always smaller than maxnames,
> >> so the fix is just plugging a leak.
> >>
> >> However, in looking at this I spotted a real problem:
> >> There are numerous statements like this:
> >>
> >>     memset(names, 0, maxnames);
> >>
> >> That zeros out only 1/4 or 1/8 of the memory it should.
> >> It should be doing this:
> >>
> >>     memset(names, 0, maxnames * sizeof (*names));
> >
> > memset() is horribly error prone - also have the classic of getting
> > the 2 & 3rd args the wrong way around. How about adding to memory.h
> > something like
> >
> >   #define VIR_ZERO(buf, nelement) \
> >         memset(buf, 0, sizeof(*(buf)) * nelement)
> 
> Sure, I'll do that after your 28-part change goes in.
> 
> But how about a more generic name like MEM_ZERO?
> 
>    #define MEM_ZERO(buf, nelement) \
>          memset((buf), 0, sizeof(*(buf)) * (nelement))

Sure, works for me.

> > also using your xalloc_oversized magic  ?
> 
> If you want to automatically handle oversized memset, we'd have
> to make it a function and change all users to test the return value.
> That'd be pretty invasive.  With malloc/realloc/calloc, where
> everyone was already testing the return value, it was easy.
> But testing memset's return value isn't useful, so no one does that.

Ok, lets ignore that bit of the idea - chances are the caller will
already have had the check for oversized data when they allocated
the array they're passing.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|