On 04/02/2013 08:22 AM, Michal Privoznik wrote: > --- > HACKING | 4 ++ > docs/hacking.html.in | 4 ++ > po/POTFILES.in | 1 + > python/libvirt-override.c | 102 > ++++++++++++++++++++++---------------------- > src/esx/esx_vi.c | 5 +-- > src/util/viralloc.c | 91 +++++++++++++++++++++++++++++++++------ > src/util/viralloc.h | 52 +++++++++++++++------- > src/util/virbuffer.c | 8 ++-- > src/util/virerror.c | 4 +- > src/util/virthreadpthread.c | 2 +- > tests/commandhelper.c | 2 +- > tests/networkxml2conftest.c | 2 +- > tests/openvzutilstest.c | 2 +- > tests/test_conf.c | 2 +- > tests/xmconfigtest.c | 2 +- > 15 files changed, 188 insertions(+), 95 deletions(-) > > diff --git a/HACKING b/HACKING > index 6230ffd..c511cab 100644 > --- a/HACKING > +++ b/HACKING > @@ -595,6 +595,10 @@ size: > > > > +These memory allocation macros report OOM error automatically. In case, you > +want to suppress such behavior, use their NOOOM variant, e.g. VIR_ALLOCNOOM, > +VIR_REALLOC_NNOOM etc.
Three different spellings in one sentence! NOOOM, NOOM, NNOOM. All the more reason to consider my 3/5 suggestion of a simpler name, such as VIR_ALLOC_QUIET. > - if (VIR_ALLOC_N(ret, size) < 0) { > + if (VIR_ALLOC_NNOOM(ret, size) < 0) { Oh, I misread your spellings. If anything, have an underscore as part of the suffix: VIR_ALLOC_N_NOOM (or VIR_ALLOC_N_QUIET) so that it is obvious that N (a count to alloc) and NOOM (or QUIET) is an attribute of what to do on allocation failure, instead of wondering if NNOOM is a typo. > +++ b/src/esx/esx_vi.c > @@ -1769,10 +1769,9 @@ esxVI_Alloc(void **ptrptr, size_t size) > return -1; > } > > - if (virAllocN(ptrptr, size, 1) < 0) { > - virReportOOMError(); > + if (virAllocN(ptrptr, size, 1, true, VIR_FROM_THIS, > + __FILE__, __FUNCTION__, __LINE__) < 0) > return -1; > - } Can we rework this to use VIR_ALLOC_N_NOOM(*ptrptr, size 1)? instead of the raw call to virAllocN, along with the cleanup to cfg.mk? > @@ -270,7 +316,8 @@ int virResizeN(void *ptrptr, size_t size, size_t > *allocptr, size_t count, > void virShrinkN(void *ptrptr, size_t size, size_t *countptr, size_t toremove) > { > if (toremove < *countptr) > - ignore_value(virReallocN(ptrptr, size, *countptr -= toremove)); > + ignore_value(virReallocN(ptrptr, size, *countptr -= toremove, false, > + 0, NULL, NULL, 0)); How come you aren't adding location arguments to virResizeN and touching up VIR_RESIZE_N to pass in location? In fact, it may be worth splitting this into two patches - one that adds location information to existing functions, and the second that introduces new functions and adds the 'report' option. > @@ -318,7 +365,8 @@ virInsertElementsN(void *ptrptr, size_t size, size_t at, > > if (inPlace) { > *countptr += add; > - } else if (virExpandN(ptrptr, size, countptr, add) < 0) { > + } else if (virExpandN(ptrptr, size, countptr, add, > + false, 0, NULL, NULL, 0) < 0) { > return -1; Another function where adding location may make sense. > @@ -82,7 +90,9 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); > * > * Returns -1 on failure, 0 on success > */ > -# define VIR_ALLOC(ptr) virAlloc(&(ptr), sizeof(*(ptr))) > +# define VIR_ALLOC(ptr) virAlloc(&(ptr), sizeof(*(ptr)), true, > VIR_FROM_THIS, \ > + __FILE__, __FUNCTION__, __LINE__) > +# define VIR_ALLOCNOOM(ptr) virAlloc(&(ptr), sizeof(*(ptr)), false, 0, NULL, > NULL, 0) Hmm. At first, I wondered if still passing VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__ on through to virAlloc, even if they aren't used, would aid in debugging. But then I realized that if you can stop the debugger on virAlloc failure, then you can also use the debugger to determine the callstack. So this seems fine, after all (for the trailing arguments - the choice of naming is still an open issue...). > +++ b/tests/openvzutilstest.c > @@ -101,7 +101,7 @@ testReadNetworkConf(const void *data ATTRIBUTE_UNUSED) > " </devices>\n" > "</domain>\n"; > > - if (VIR_ALLOC(def) < 0 || > + if (VIR_ALLOCNOOM(def) < 0 || > !(def->os.type = strdup("exe")) || > !(def->os.init = strdup("/sbin/init"))) I thought 2/5 got rid of all raw strdup()? Or maybe the syntax check rule exempted a bit too much? Again an argument for making things do a per-use exemption, instead of a per-file exemption, the way we do for strtol. The places where you used the new function look sane. Looking forward to v2 with saner names. Again, splitting into two patches (one to make viralloc.h preserve locations but with no new functionality, and another to add the new functionality) might make review easier. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list