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

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.