Re: [libvirt] [PATCH 1/6] squash this into domain conf XML "eliminate hardcoded indent" patch
On 03/13/2014 04:06 PM, Eric Blake wrote:
On 03/12/2014 10:04 PM, Laine Stump wrote:
> This should be squashed into the first patch of the previous
> "eliminate hardcoded indent" series (which was ACKed but hasn't yet
> been pushed):
>
> https://www.redhat.com/archives/libvir-list/2014-March/msg00361.html
>
> The initial patch neglected to change the functions that format
> private metadata in the qemu and lxc domain xml.
>
> I have verified that make check still passes when these changes are
> squashed into the original.
> ---
> src/conf/domain_conf.c | 2 +-
> src/lxc/lxc_domain.c | 4 ++--
> src/qemu/qemu_domain.c | 45 +++++++++++++++++++++++++++------------------
> 3 files changed, 30 insertions(+), 21 deletions(-)
>
ACK.
Hmm, your comment about dropping all the virBufferAdjustIndent() calls,
free-form output of the text without regards to indentation, then
running it through util/virxml.c to parse and then pretty-print the xml,
may be more maintainable, even if it eats more CPU cycles. But if we do
that, it could be a followup to this series.
Okay, I made the changes you suggested and pushed this series along with
the previous one. It is now illegal to call a virBuffer function with a
string that starts with spaces followed by <.
I did some experimenting with a pretty-printing virBuffer (as you
mentioned above) tonight. After making a virBufferXMLContentAndReset()
function that calls virXMLParseString() followed by virXMLNodeToString()
(which would magically indent xml that started out unindented), and
modifying virDomainDefFormat() to use it, I found the following:
1) if you modify qemuxml2xmltest to run the parse/format operation 300
times for each document, the time to run the test is ~30 seconds when
using virBufferContentAndReset() (manual indentation), and 35 seconds
when using virBufferXMLContentAndReset() (pretty print). So it adds
about 17% to the time for a parse/format roundtrip.
2) virXMLNodeToString() uses double quotes for strings, while we use
single quotes everywhere. So all of the tests end up failing (this may
have slightly skewed the timing above, since the strcmp would have
exited sooner each time).
(DV pointed out xmlTextWriter*() functions in libxml as a possible way
to overcome the double vs. single quotes problem, but I couldn't
immediately figure out how to use it.)
3) I noticed in the code that many of the places where we call
virBufferContentAndReset(), we are assuming that it will be successful,
just returning the result to the caller, and the caller assumes that if
it gets a NULL, an error will have already been reported. But if there
is an error in the XML, the pretty-printing version could fail, so every
caller will need to be modified to handle an error in this case.
4) libvirt-setuid-rpc-client.la (used by virt-login-shell) needs to have
every util .c file it uses added explicitly because we want to give the
setuid process access to as little as possible. But it already uses
virbuffer.c, and if virbuffer.c calls virXMLParseString(), it will also
need to include virxml.c. *and* if virxml.c is needed, then it will also
need to link with libxml2. That may be beyond what we want in
virt-login-shell.
So for the moment I'm going to just let this idea simmer - I think the
extra overhead is probably acceptable in return for the easier
maintenance, but I'm not sure what to do about the double vs. single
quotes, nor whether or not it is acceptable to add so much new code to
the footprint of virt-login-shell.