[libvirt] [PATCH v2 5/6] util: Add helpers for safe domain console operations

[Peter Krempa]

On 01/18/2012 12:38 AM, Eric Blake wrote:
> On 12/07/2011 11:08 AM, Peter Krempa wrote:
>> This patch adds a set of functions used in creating console streams for
>> domains using PTYs and ensures mutualy exculsive access to the PTYs.
>
> Picking up on my (stalled) review...
>
>> +++ b/src/Makefile.am
>> @@ -101,6 +101,9 @@ UTIL_SOURCES =							\
>>   		util/virsocketaddr.h util/virsocketaddr.c \
>>   		util/virtime.h util/virtime.c
>>
>> +SAFE_CONSOLE_SOURCES = \
>> +		util/domain_safe_console.c util/domain_safe_console.h
>
> Unless you are planning on including these sources into an independent
> library, I see no need to make a new variable.  Just add these files
> into the right place within UTIL_SOURCES.

This is a little bit tricky. My first approach was to add it to 
UTIL_SOURCES, but then I got errors about missing references (This code 
needs references to streams and other stuff from libvirt.[c|h].) while 
building the LXC helper tool. This works it around, as the lxc app does 
not get built with these sources, and the util lib does.

Do you have any suggestions, how to work around getting errors from the 
lxc app? I'd rather use the UTIL_SOURCES var. if possible.


Thanks,

Peter

>
> Those are some rather long names.  Also, our convention for new files
> has lately been to go with vir<name>.[ch], with APIs starting with
> vir<Name>...().  I'm thinking that this patch may be better as:
>
> virconsole.[ch]
> virConsoleAlloc()
> virConsoleFree()
> virConsoleOpen()
>
> That is, the name Domain doesn't add anything (we might want a console
> for other reasons) and the name Safe is redundant (the whole point of
> adding a virName API is to make the Name action safer and easier to use).
>
>> +
>>   EXTRA_DIST += $(srcdir)/util/virkeymaps.h $(srcdir)/util/keymaps.csv \
>>   		$(srcdir)/util/virkeycode-mapgen.py
>>
>> @@ -555,7 +558,7 @@ noinst_LTLIBRARIES = libvirt_util.la
>>   libvirt_la_LIBADD = $(libvirt_la_BUILT_LIBADD)
>>   libvirt_la_BUILT_LIBADD = libvirt_util.la
>>   libvirt_util_la_SOURCES =					\
>> -		$(UTIL_SOURCES)
>> +		$(UTIL_SOURCES) $(SAFE_CONSOLE_SOURCES)
>
> If you add the new files directly to UTIL_SOURCES, then you don't need
> to edit this line.
>