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.