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

"Daniel P. Berrange" <berrange(a)redhat.com&gt; wrote: > On Tue, Dec 02, 2008 at 03:02:17PM +0100, Jim Meyering wrote: >> "Daniel P. Berrange" <berrange(a)redhat.com&gt; 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))

> 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.