[libvirt] adding tests to qemu_driver
Greetings, I was investigating on an issue in which QEMU's dynamic ownership was not properly working when calling qemuDomainCoreDumpWithFormat(). 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. The function is quite complicated and I was thinking about refactoring it a bit in order to simplify the logic allowing to fix the dynamic ownership handling as well. In order to refactor it, I was planning to write tests in order to cover all its use cases before actually changing the code. 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. 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? Thank you.
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?
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. ...
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.
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. Jirka
2015-11-13 0:30 GMT+02:00 Jiri Denemark <jdenemar@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
On Fri, Nov 13, 2015 at 10:44:01 +0200, NoxDaFox wrote:
2015-11-13 0:30 GMT+02:00 Jiri Denemark <jdenemar@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.
And what is the result? Does dumping fail because of this or does it work anyway? Jirka
2015-11-19 14:25 GMT+02:00 Jiri Denemark <jdenemar@redhat.com>:
On Fri, Nov 13, 2015 at 10:44:01 +0200, NoxDaFox wrote:
2015-11-13 0:30 GMT+02:00 Jiri Denemark <jdenemar@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.
And what is the result? Does dumping fail because of this or does it work anyway?
Jirka
The file gets dumped correctly, nevertheless it's ownership is still root:root and not the one set in the configuration.
From qemu.conf
# Whether libvirt should dynamically change file ownership # to match the configured user/group above. Defaults to 1. # Set to 0 to disable file ownership changes. dynamic_ownership = 1 Now, given a user and a group, when I run qemuDomainCoreDumpWithFormat() I get the file still belonging to root:root. I tested the patch locally and it was working. I could create the file anywhere and the ownership was right. What I'd like to do though, is to refactor the function as its logic is a bit cumbersome. Before that, I'll try to provide a set of tests to help me during the refactoring. It will require me a bit of time as I'm not accustomed to the code. I provided the patch as it is, because it was suggested in the other discussion. Matteo.
On Fri, Nov 20, 2015 at 14:10:49 +0200, NoxDaFox wrote:
2015-11-19 14:25 GMT+02:00 Jiri Denemark <jdenemar@redhat.com>:
On Fri, Nov 13, 2015 at 10:44:01 +0200, NoxDaFox wrote:
2015-11-13 0:30 GMT+02:00 Jiri Denemark <jdenemar@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.
And what is the result? Does dumping fail because of this or does it work anyway?
The file gets dumped correctly, nevertheless it's ownership is still root:root and not the one set in the configuration.
OK, this is actually the correct behavior.
From qemu.conf
# Whether libvirt should dynamically change file ownership # to match the configured user/group above. Defaults to 1. # Set to 0 to disable file ownership changes. dynamic_ownership = 1
Now, given a user and a group, when I run qemuDomainCoreDumpWithFormat() I get the file still belonging to root:root.
Right, but (even though the comment doesn't say so) this applies only to files used by a running QEMU and only when they are used. They are chowned to root:root (they'd ideally be changed back to the original user/group, but we are no there yet) once they are no longer used. E.g., a disk image file will only be owned by the user/group while it is attached to a running domain. When the domain is not running, they will be owned by root:root.
I tested the patch locally and it was working. I could create the file anywhere and the ownership was right.
Hmm, you are right, it looks like I didn't read the code carefully enough. Anyway, the file should be owne by root:root so that other QEMU processes cannot access it.
What I'd like to do though, is to refactor the function as its logic is a bit cumbersome. Before that, I'll try to provide a set of tests to help me during the refactoring.
Oh sure, the code is a mess and if you have any idea for making saner and more readable. Creating a test suite first is definitely a good idea, because we don't want to accidentally change the behavior of this ugly code. Jirka
On 20/11/15 19:18, Jiri Denemark wrote:
On Fri, Nov 20, 2015 at 14:10:49 +0200, NoxDaFox wrote:
2015-11-19 14:25 GMT+02:00 Jiri Denemark <jdenemar@redhat.com>:
On Fri, Nov 13, 2015 at 10:44:01 +0200, NoxDaFox wrote:
2015-11-13 0:30 GMT+02:00 Jiri Denemark <jdenemar@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. And what is the result? Does dumping fail because of this or does it work anyway? The file gets dumped correctly, nevertheless it's ownership is still root:root and not the one set in the configuration. OK, this is actually the correct behavior. If so, I'm definitely lost here :)
From qemu.conf
# Whether libvirt should dynamically change file ownership # to match the configured user/group above. Defaults to 1. # Set to 0 to disable file ownership changes. dynamic_ownership = 1
Now, given a user and a group, when I run qemuDomainCoreDumpWithFormat() I get the file still belonging to root:root. Right, but (even though the comment doesn't say so) this applies only to files used by a running QEMU and only when they are used. They are chowned to root:root (they'd ideally be changed back to the original user/group, but we are no there yet) once they are no longer used. E.g., a disk image file will only be owned by the user/group while it is attached to a running domain. When the domain is not running, they will be owned by root:root.
Now I'm confused. The qemuDomainCoreDumpWithFormat should return a "picture" of the memory state of a running VM. It's a separate file which does not belong to the running VM itself. Why permissions should be transient? Moreover for what I've observed, the file gets root:root and remains so no matter whether the owner process is running or not.
I tested the patch locally and it was working. I could create the file anywhere and the ownership was right. Hmm, you are right, it looks like I didn't read the code carefully enough. Anyway, the file should be owne by root:root so that other QEMU processes cannot access it.
May I ask why such a policy? Wouldn't be better to leave the decision to the API user? In my case is making the logic quite complicated.
What I'd like to do though, is to refactor the function as its logic is a bit cumbersome. Before that, I'll try to provide a set of tests to help me during the refactoring. Oh sure, the code is a mess and if you have any idea for making saner and more readable. Creating a test suite first is definitely a good idea, because we don't want to accidentally change the behavior of this ugly code.
Jirka
The code is not *that* bad but it's pretty core logic. The lack of tests and the fragile logic suggests me this is the right place where to refactor a bit. As I said, my free time is quite limited and I need to get accustomed to libvirt's tests and build process. I'll start exposing the functions to a private header and to add basic tests for them.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (3)
-
Jiri Denemark -
NoxDaFox -
noxdafox