On Tue, Dec 02, 2008 at 03:32:28PM +0100, Jim Meyering wrote: > "Daniel P. Berrange" <berrange@xxxxxxxxxx> wrote: > > > On Tue, Dec 02, 2008 at 03:02:17PM +0100, Jim Meyering wrote: > >> "Daniel P. Berrange" <berrange@xxxxxxxxxx> 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 :| -- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list