[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [RFC PATCH v1 3/7] include virterror_internal.h in threads.h



On Wed, Jan 16, 2013 at 05:34:52PM -0700, Eric Blake wrote:
> On 01/16/2013 03:13 AM, Daniel P. Berrange wrote:
> > On Wed, Jan 16, 2013 at 10:53:05AM +0800, Hu Tao wrote:
> >> required by virSetError.
> >> ---
> >>  src/util/virthread.h | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/src/util/virthread.h b/src/util/virthread.h
> >> index b11a251..c209440 100644
> >> --- a/src/util/virthread.h
> >> +++ b/src/util/virthread.h
> >> @@ -23,6 +23,7 @@
> >>  # define __THREADS_H_
> >>  
> >>  # include "internal.h"
> >> +# include "virerror.h"
> >>  
> >>  typedef struct virMutex virMutex;
> >>  typedef virMutex *virMutexPtr;
> > 
> > AFAICT, the header file doesn't need this addition - the .c file
> > needs it.
> 
> I disagree.  The definition of the VIR_ONCE_GLOBAL_INIT macro uses
> virErrorPtr, which means if we don't include virerror.h here, _every_ .c
> file that uses one-shot initialization would then have to pull in
> virerror.h.  It turns out that all existing clients already do, or we
> would have hit a compilation error sooner; thus this patch is really
> only mandatory for the new client later in this series - but that commit
> has already been nack'd on design grounds.  Still, requiring a client .c
> file to do extra includes just to use the macro makes it harder than
> necessary, so I'm still in favor of making  virthread.h self-contained.
> 
> ACK, after updating the commit message to explain why.  I've pushed 1-3
> in this series.  Like Dan, I agree that the remainder of the series
> needs a step back to figure out what the real design goal is, before
> doing any more refactoring.

Ah, I forgot about the macro expanding to an error call. Makes sense to
include this change

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]