[libvirt] PATCH: 15/28: Reduce return points for storage driver
Daniel P. Berrange
berrange at redhat.com
Tue Dec 2 14:36:06 UTC 2008
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 :|
More information about the libvir-list
mailing list