[libvirt] adding tests to qemu_driver

[NoxDaFox]

2015-11-13 0:30 GMT+02:00 Jiri Denemark <jdenemar at redhat.com>:

> On Thu, Nov 12, 2015 at 23:47:54 +0200, noxdafox wrote:
> > Greetings,
> >
> > I was investigating on an issue in which QEMU's dynamic ownership was
> > not properly working when calling qemuDomainCoreDumpWithFormat().
>
> Could describe this issue you are investigating?
>

When calling qemuDomainCoreDumpWithFormat() the file is create as root:root
even when the dynamic ownership is specified in the qemu.conf configuration
file.


>
> > The core of the issue seems to be the qemuOpenFileAs() function which
> > does not handle the dynamic ownership. This might affect other libvirt's
> > features within as well.
>
> Because you are most likely looking at a wrong place; qemuOpenFileAs is
> a quite low level function which is just supposed to open/create a file
> accessible to a given user. It's up to the caller to decide what the
> user should be.
>

When debugging the call, the parameters are correctly forwarded:
dynamicOwnership is true and fallback_uid and fallback_gid are correct. The
function itself seems to ignore them when a new file is created.

The following patch I provided on libvirt-users list seems to fix the issue
but I'm not aware of possible side effects. This is why I'd like to provide
some tests for these core functions.

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a2cc002..1b47dc6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2932,6 +2932,11 @@ qemuOpenFileAs(uid_t fallback_uid, gid_t
fallback_gid,
         if (path_shared <= 0 || dynamicOwnership)
             vfoflags |= VIR_FILE_OPEN_FORCE_OWNER;

+        if (dynamicOwnership) {
+            uid = fallback_uid;
+            gid = fallback_gid;
+        }
+
         if (stat(path, &sb) == 0) {
             /* It already exists, we don't want to delete it on error */
             need_unlink = false;


> ...
> > The issue is that all the functions within the qemu_driver.c module are
> > static. I could indeed include the module itself in my tests but I'm not
> > sure whether this is acceptable.
>
> We solve this kind of issues by removing "static" from the functions and
> adding a new header file (if it doesn't exist yet) called *priv.h
> (qemu_driverpriv.h in this case) with the prototypes for such functions.
> Only tests are allowed to include such header files.
>

This is a way more elegant solution but I didn't know if it was acceptable
to modify the headers.


> > Furthermore I'd like to have some clarification about the NFS related
> > code. It seems that some effort has been put in order to tackle
> > something I'm not aware of. Could someone briefly explain how to
> > reproduce NFS failing scenarios?
>
> The main problem with NFS which this ugly function is trying to handle
> is called "root-squash". This feature maps all access from UID 0 to an
> unprivileged UID. That is, libvirtd (even though it is running as root)
> will not be able to access the desired file.
>

This is very helpful thanks!

I'll try to set some tests and provide patches later on. I guess it would
be beneficial for libvirt anyway. About the refactoring we can discuss
further later.


>
> Jirka
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20151113/7033030f/attachment-0001.htm>