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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list