Re: [libvirt] [PATCH v2 5/6] util: Add helpers for safe domain console operations
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.