Hi, On Fri, Apr 08, 2011 at 12:31:57PM +0200, Patrik Olsson wrote: > On 07/04/11 21:42, olafbuddenha...@gmx.net wrote:
> > A general remark: not all plattforms have asprintf(). Depending on > > how portable the packages in question are, you might need to add > > configure-time checks and/or compile-time conditionals. > > I am aware of that, but from what I could gather, that package was > very GNU/Linux-specific. OK. Perhaps you should pick one that isn't -- the configure stuff is often more complex than the actual code changes, so it would be good to see how well you can deal with it :-) > I assumed this was a pretty low-level library so I found that > propagating the error should be the most correct. However, for the > snprintf calls this was not done so I decided not to do it for the > asprintf calls either. That's not the same. An snprinf() in the worst case will truncate the output. This could have some odd side effects -- but only in the very very unlikely case that the resulting name actually exceeds PATH_MAX. asprintf() OTOH can actually return a NULL string if memory is scarce, resulting in a segfault. Like all other allocations, the result of asprintf() should always be checked. (Admittedly, that's a bit academic, as the stack allocation using a fairly large PATH_MAX is actually *more* likely to fail than the heap allocation using only the really required size, and will then result in a segfault too... Which however should be easier to track down, as it happens immediately on allocation, rather than possibly much later when the NULL pointer is used, and not clear anymore where it came from.) > >> + if (patches_tmp_filename != NULL) + > >> g_free(patches_tmp_filename); > > > > I don't know how g_free() behaves -- but if it's the same as normal > > free(), you don't need the conditional: free(NULL) is a no-op by > > definition. > > The reason I added the check is because the original code does it too > for other variables in the previous lines, and I preferred to be > consistent. I see. That's OK then. -antrik-
