Re: [libvirt] [PATCH V2 2/3] add error report for virMutexInit virRWLockInit and virCondInit

On Mon, Jul 21, 2014 at 06:13:36PM +0800, Jincheng Miao wrote: > Implement vir{Mutex,RWLock,Cond}InitInternal functions which have > related error report when failure, and vir{Mutex,RWLock,Cond}Init > as macros. So that the caller no longer to print error message > explicitly. > > Signed-off-by: Jincheng Miao <jmiao(a)redhat.com&gt; > --- > src/libvirt_private.syms | 7 +++--- > src/util/virthread.c | 56 > +++++++++++++++++++++--------------------------- > src/util/virthread.h | 25 +++++++++++++++++---- > 3 files changed, 49 insertions(+), 39 deletions(-) > > > +#define VIR_THREAD_ERR_EXIT(str) do { \ > + errno = ret; \ > + virReportSystemErrorFull(VIR_FROM_NONE, errno, \ > + filename, funcname, linenr, \ > + "%s", _(str)); \ > + return -1; \ > + } while (0) [snip] > -int virCondInit(virCondPtr cond) > +int virCondInitInternal(virCondPtr cond, const char *filename, > + const char *funcname, size_t linenr) > { > - int ret; > - if ((ret = pthread_cond_init(&cond->cond, NULL)) != 0) { > - errno = ret; > - return -1; > - } > + int ret = pthread_cond_init(&cond->cond, NULL); > + if (ret != 0) > + VIR_THREAD_ERR_EXIT("unable to init Condition"); > + > return 0; > } [snip] > -int virCondInit(virCondPtr cond) ATTRIBUTE_RETURN_CHECK; > +int virCondInitInternal(virCondPtr cond, const char *filename, > + const char *funcname, size_t linenr) > + ATTRIBUTE_RETURN_CHECK; > +# define virCondInit(cond) \ > + virCondInitInternal(cond, __FILE__, __FUNCTION__, __LINE__) > + I can see what you're trying todo here by passing in __FILE__, __FUNCTION__, __LINE__ through from the calling site, but I don't really think this is beneficial and is not the way we deal with error reporting in any other similar helper APIs. IMHO just remove all this passing around of source location and let it do the default thing. In fact the default behaviour is probably better since if we want to search for any thread related error messages, we can query the journald for the virthread.c file directly.

Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|