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

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

 



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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]