[libvirt] Some problem with the save function
Hi everyone. This is my first post to this list, so excuse me if it's not the right place to poste info like this. I've found some problems in qemu_driver.c in the latest versions of libvirt: - the first problem was also present in 0.7.0. If we run qemu guests unprivileged, we cannot use the save function (virsh save guestname /path/to/file). The reason seems to be that libvirt first create the destination file to write the header and the XML definition. The file is owned by root with 600 permission. Then, we ask qemu to append the dump of the memory (via the migrate exec: call) to this file. This part doesn't work because the qemu user cannot write to this file. So we endup with a file which only contains the header and the XML, and the guest is stoped :/ - the second problem is present since libvirt 0.7.1. Now that the saved file can be compressed, it seems we cannot save in a raw format any more. This is due to this part in the code (qemu_driver.c): if (STREQ (prog, "raw")) prog = "cat"; internalret = virAsprintf(&command, "migrate \"exec:" "%s -c >> '%s' 2>/dev/null\"", prog, safe_path); which result in "migrate \"exec cat -c >> safe_path 2>/dev/null\"" But cat doesn't support the -c argument, so once again, the save fails, as we end up with a save file which only contains the header and the XML definition. Unfortunately, my C knowledge is near 0, so I cannot correct this myself. - the third problem, is that restore doesn't work any more. I haven't dig it yet, but when I try to restore a guest using a saved file (uncompressed produced under libvirt 0.7.0), I've this error: virsh restore /tmp/guest.state error: Failed to restore domain from /tmp/guest.state error: internal error unable to start guest: Nothing interesting in /var/log/libvirt/qemu/guest.log: LC_ALL=C PATH=/sbin:/usr/sbin:/bin:/usr/bin HOME=/root USER=root LOGNAME=root /usr/bin/qemu-kvm -S -M pc -m 2048 -smp 4 -name guest -uuid 37b5937b-eacf-fda6-f4d0-33537d61b414 -monitor unix:/var/lib/libvirt/qemu/guest.monitor,server,nowait -localtime -no-reboot -boot c -drive file=/dev/data/guest,if=ide,index=0,boot=on -drive file=/var/lib/libvirt/iso/boot.iso,if=ide,media=cdrom,index=2 -net nic,macaddr=52:54:00:48:c4:ca,vlan=0,name=nic.0 -net tap,fd=25,vlan=0,name=tap.0 -serial pty -parallel none -usb -usbdevice tablet -vnc 127.0.0.1:0 -k fr -vga cirrus -incoming exec:cat The qemu process eats 100% of CPU during a few seconds, then stops (if I strace it during this time, I see a lot of error like bad file descriptor). Anyone have save/restore functions working with 0.7.1 ? Cheers, Daniel -- Daniel Berteaud FIREWALL-SERVICES SARL. Société de Services en Logiciels Libres Technopôle Montesquieu 33650 MARTILLAC Tel : 05 56 64 15 32 Fax : 05 56 64 15 32 Mail: daniel@firewall-services.com Web : http://www.firewall-services.com
On Wed, Sep 16, 2009 at 05:05:36PM +0200, Daniel Berteaud wrote:
Hi everyone. This is my first post to this list, so excuse me if it's not the right place to poste info like this.
I've found some problems in qemu_driver.c in the latest versions of libvirt:
- the first problem was also present in 0.7.0. If we run qemu guests unprivileged, we cannot use the save function (virsh save guestname /path/to/file). The reason seems to be that libvirt first create the destination file to write the header and the XML definition. The file is owned by root with 600 permission. Then, we ask qemu to append the dump of the memory (via the migrate exec: call) to this file. This part doesn't work because the qemu user cannot write to this file. So we endup with a file which only contains the header and the XML, and the guest is stoped :/
Doh, that's a nice problem. You can work around it by editing /etc/libvirt/qemu.conf and telling qemu to run as root:root again. For a real fix we'll need to make the save method run 'fchown' on the file descriptor after writing the header. And then fchown it back to root:root once complete, to stop any other guest overwriting it.
- the second problem is present since libvirt 0.7.1. Now that the saved file can be compressed, it seems we cannot save in a raw format any more. This is due to this part in the code (qemu_driver.c):
if (STREQ (prog, "raw")) prog = "cat"; internalret = virAsprintf(&command, "migrate \"exec:" "%s -c >> '%s' 2>/dev/null\"", prog, safe_path);
which result in "migrate \"exec cat -c >> safe_path 2>/dev/null\""
But cat doesn't support the -c argument, so once again, the save fails, as we end up with a save file which only contains the header and the XML definition.
Wierd, I don't know where/when we gained a '-c' arg to cat but it looks rather bogus.
- the third problem, is that restore doesn't work any more. I haven't dig it yet, but when I try to restore a guest using a saved file (uncompressed produced under libvirt 0.7.0), I've this error:
virsh restore /tmp/guest.state error: Failed to restore domain from /tmp/guest.state error: internal error unable to start guest:
Hmm, bad error message :-( We might also need todo a chown() in the restore path to allow QEMU to read it. NB there is no compatability between QEMU version, so if you have upgraded your install of QEMU between the time you saved & restored it is very likely to crash & burn. This is something QEMU devs are working on fixing by allowing for stable machine ABI (available in Fedora 12), and stable pci device addressing (targetted for Fedora 13). Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Le mercredi 16 septembre 2009 à 21:36 +0100, Daniel P. Berrange a écrit :
On Wed, Sep 16, 2009 at 05:05:36PM +0200, Daniel Berteaud wrote:
Hi everyone. This is my first post to this list, so excuse me if it's not the right place to poste info like this.
I've found some problems in qemu_driver.c in the latest versions of libvirt:
- the first problem was also present in 0.7.0. If we run qemu guests unprivileged, we cannot use the save function (virsh save guestname /path/to/file). The reason seems to be that libvirt first create the destination file to write the header and the XML definition. The file is owned by root with 600 permission. Then, we ask qemu to append the dump of the memory (via the migrate exec: call) to this file. This part doesn't work because the qemu user cannot write to this file. So we endup with a file which only contains the header and the XML, and the guest is stoped :/
Doh, that's a nice problem. You can work around it by editing /etc/libvirt/qemu.conf and telling qemu to run as root:root again. For a real fix we'll need to make the save method run 'fchown' on the file descriptor after writing the header. And then fchown it back to root:root once complete, to stop any other guest overwriting it.
As I only use the save function in a script, I work around it by touching the file and chowning it to qemu:qemu before calling save, I sleep better when my guests runs with less privileges ;)
- the second problem is present since libvirt 0.7.1. Now that the saved file can be compressed, it seems we cannot save in a raw format any more. This is due to this part in the code (qemu_driver.c):
if (STREQ (prog, "raw")) prog = "cat"; internalret = virAsprintf(&command, "migrate \"exec:" "%s -c >> '%s' 2>/dev/null\"", prog, safe_path);
which result in "migrate \"exec cat -c >> safe_path 2>/dev/null\""
But cat doesn't support the -c argument, so once again, the save fails, as we end up with a save file which only contains the header and the XML definition.
Wierd, I don't know where/when we gained a '-c' arg to cat but it looks rather bogus.
The change is in this patch : http://libvirt.org/git/?p=libvirt.git;a=commit;h=aec22258ef01d7cf36b031a42d9...
- the third problem, is that restore doesn't work any more. I haven't dig it yet, but when I try to restore a guest using a saved file (uncompressed produced under libvirt 0.7.0), I've this error:
virsh restore /tmp/guest.state error: Failed to restore domain from /tmp/guest.state error: internal error unable to start guest:
Hmm, bad error message :-( We might also need todo a chown() in the restore path to allow QEMU to read it. NB there is no compatability between QEMU version, so if you have upgraded your install of QEMU between the time you saved & restored it is very likely to crash & burn.
I haven't upgraded qemu, just libvirt. And I must admit that I don't really know where and how to debug it, I'm very new to libvirt ;) (and I really don't know anything in C) Would just like to know if someone could get it working in 0.7.1, or if there's a big bug somewhere in this release. Regards, Daniel
This is something QEMU devs are working on fixing by allowing for stable machine ABI (available in Fedora 12), and stable pci device addressing (targetted for Fedora 13).
Regards, Daniel
-- Daniel Berteaud FIREWALL-SERVICES SARL. Société de Services en Logiciels Libres Technopôle Montesquieu 33650 MARTILLAC Tel : 05 56 64 15 32 Fax : 05 56 64 15 32 Mail: daniel@firewall-services.com Web : http://www.firewall-services.com
On Wed, Sep 16, 2009 at 11:09:59PM +0200, Daniel Berteaud wrote:
Le mercredi 16 septembre 2009 à 21:36 +0100, Daniel P. Berrange a écrit :
On Wed, Sep 16, 2009 at 05:05:36PM +0200, Daniel Berteaud wrote:
Hi everyone. This is my first post to this list, so excuse me if it's not the right place to poste info like this.
I've found some problems in qemu_driver.c in the latest versions of libvirt:
- the first problem was also present in 0.7.0. If we run qemu guests unprivileged, we cannot use the save function (virsh save guestname /path/to/file). The reason seems to be that libvirt first create the destination file to write the header and the XML definition. The file is owned by root with 600 permission. Then, we ask qemu to append the dump of the memory (via the migrate exec: call) to this file. This part doesn't work because the qemu user cannot write to this file. So we endup with a file which only contains the header and the XML, and the guest is stoped :/
Doh, that's a nice problem. You can work around it by editing /etc/libvirt/qemu.conf and telling qemu to run as root:root again. For a real fix we'll need to make the save method run 'fchown' on the file descriptor after writing the header. And then fchown it back to root:root once complete, to stop any other guest overwriting it.
As I only use the save function in a script, I work around it by touching the file and chowning it to qemu:qemu before calling save, I sleep better when my guests runs with less privileges ;)
- the second problem is present since libvirt 0.7.1. Now that the saved file can be compressed, it seems we cannot save in a raw format any more. This is due to this part in the code (qemu_driver.c):
if (STREQ (prog, "raw")) prog = "cat"; internalret = virAsprintf(&command, "migrate \"exec:" "%s -c >> '%s' 2>/dev/null\"", prog, safe_path);
which result in "migrate \"exec cat -c >> safe_path 2>/dev/null\""
But cat doesn't support the -c argument, so once again, the save fails, as we end up with a save file which only contains the header and the XML definition.
Wierd, I don't know where/when we gained a '-c' arg to cat but it looks rather bogus.
The change is in this patch : http://libvirt.org/git/?p=libvirt.git;a=commit;h=aec22258ef01d7cf36b031a42d9...
- the third problem, is that restore doesn't work any more. I haven't dig it yet, but when I try to restore a guest using a saved file (uncompressed produced under libvirt 0.7.0), I've this error:
virsh restore /tmp/guest.state error: Failed to restore domain from /tmp/guest.state error: internal error unable to start guest:
Hmm, bad error message :-( We might also need todo a chown() in the restore path to allow QEMU to read it. NB there is no compatability between QEMU version, so if you have upgraded your install of QEMU between the time you saved & restored it is very likely to crash & burn.
I haven't upgraded qemu, just libvirt. And I must admit that I don't really know where and how to debug it, I'm very new to libvirt ;) (and I really don't know anything in C)
Would just like to know if someone could get it working in 0.7.1, or if there's a big bug somewhere in this release.
Ok, I'd recommend filing a bug report against libvirt then with at least the folowing information - The guest XML file - The guest log from /var/log/libvirt/qemu/$GUEST.log after the restore failed - Edit /etc/libvirt/libvirtd.conf and set log_level=1 log_filters="1:libvirt 1:qemu 1:util" log_outputs="1:file:/var/log/libvirt/daemon.log" and then restart libvirtd, and try the save/restore again. Then upload the log you get to the bug - Version of QEMU or KVM used That ought to help us diagnose the problem futher Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Le mercredi 16 septembre 2009 à 22:14 +0100, Daniel P. Berrange a écrit :
Ok, I'd recommend filing a bug report against libvirt then with at least the folowing information
- The guest XML file - The guest log from /var/log/libvirt/qemu/$GUEST.log after the restore failed - Edit /etc/libvirt/libvirtd.conf and set
log_level=1 log_filters="1:libvirt 1:qemu 1:util" log_outputs="1:file:/var/log/libvirt/daemon.log"
and then restart libvirtd, and try the save/restore again. Then upload the log you get to the bug - Version of QEMU or KVM used
That ought to help us diagnose the problem futher
Thanks for helping me reporting that ;) Bug opened: https://bugzilla.redhat.com/show_bug.cgi?id=523916 Should I open separated bug for the other problem reported (save with qemu user and cat -c) ? Regards, Daniel
Regards, Daniel
-- Daniel Berteaud FIREWALL-SERVICES SARL. Société de Services en Logiciels Libres Technopôle Montesquieu 33650 MARTILLAC Tel : 05 56 64 15 32 Fax : 05 56 64 15 32 Mail: daniel@firewall-services.com Web : http://www.firewall-services.com
Daniel P. Berrange wrote:
Hmm, bad error message :-( We might also need todo a chown() in the restore path to allow QEMU to read it. NB there is no compatability between QEMU version, so if you have upgraded your install of QEMU between the time you saved & restored it is very likely to crash & burn.
How difficult would it be to set up named pipes for save and restore operations, and splice from those pipes to the actual files on the libvirtd side? Seems much preferable from a security perspective to chown'ing things around, and (unlike pipes set up ahead of time with pipe() and friends) shouldn't be disrupted by a libvirtd restart.
On 09/18/2009 03:09 AM, Charles Duffy wrote:
Daniel P. Berrange wrote:
Hmm, bad error message :-( We might also need todo a chown() in the restore path to allow QEMU to read it. NB there is no compatability between QEMU version, so if you have upgraded your install of QEMU between the time you saved & restored it is very likely to crash & burn.
How difficult would it be to set up named pipes for save and restore operations, and splice from those pipes to the actual files on the libvirtd side? Seems much preferable from a security perspective to chown'ing things around, and (unlike pipes set up ahead of time with pipe() and friends) shouldn't be disrupted by a libvirtd restart.
For a recent enough QEMU, you could also use the migrate-to-file-descriptor functionality which uses SCM_RIGHTS. It is in QEMU 0.12 only, but since the plumbing is in 0.11 too we could ask for a backport. Paolo
Daniel P. Berrange wrote:
- the second problem is present since libvirt 0.7.1. Now that the saved file can be compressed, it seems we cannot save in a raw format any more. This is due to this part in the code (qemu_driver.c):
if (STREQ (prog, "raw")) prog = "cat"; internalret = virAsprintf(&command, "migrate \"exec:" "%s -c >> '%s' 2>/dev/null\"", prog, safe_path);
which result in "migrate \"exec cat -c >> safe_path 2>/dev/null\""
But cat doesn't support the -c argument, so once again, the save fails, as we end up with a save file which only contains the header and the XML definition.
Wierd, I don't know where/when we gained a '-c' arg to cat but it looks rather bogus.
There was a lot of back and forth around this area. I think Jim ended up committing a patch using cat instead of the old "dd" method, and that's probably what broke it here. I can come up with a patch to fix at least the first 2 things here. -- Chris Lalancette
Charles Duffy wrote:
Daniel Berteaud wrote:
- the second problem is present since libvirt 0.7.1. Now that the saved file can be compressed, it seems we cannot save in a raw format any more.
Yeeowch.
How's this for a fix?
Ah, you beat me to it, but...
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index a65334f..ff30421 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -3912,10 +3912,15 @@ static int qemudDomainSave(virDomainPtr dom, goto cleanup; }
- if (STREQ (prog, "raw")) + const char *args;
I think you'll get a warning about "mixing code and data" here. At least, it's not at the top of a block, so we should move it to the top of a block. Otherwise it looks OK to me.
+ if (STREQ (prog, "raw")) { prog = "cat"; + args = ""; + } else { + args = "-c"; + } internalret = virAsprintf(&command, "migrate \"exec:" - "%s -c >> '%s' 2>/dev/null\"", prog, safe_path); + "%s %s >> '%s' 2>/dev/null\"", prog, args, safe_path);
if (internalret < 0) { virReportOOMError(dom->conn);
-- Chris Lalancette
On Fri, Sep 18, 2009 at 1:53 AM, Chris Lalancette <clalance@redhat.com>wrote:
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index a65334f..ff30421 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -3912,10 +3912,15 @@ static int qemudDomainSave(virDomainPtr dom, goto cleanup; }
- if (STREQ (prog, "raw")) + const char *args;
I think you'll get a warning about "mixing code and data" here. At least, it's not at the top of a block, so we should move it to the top of a block.
I would have done that, but as I recall we're already declaring const char *prog immediately before its first use rather than at the top of a block, so I presumed we're using compile-time options which suppress that warning already. I'll double-check what's going on when I get back to the office.
Charles Duffy wrote:
On Fri, Sep 18, 2009 at 1:53 AM, Chris Lalancette <clalance@redhat.com <mailto:clalance@redhat.com>> wrote:
> diff --git a/src/qemu_driver.c b/src/qemu_driver.c > index a65334f..ff30421 100644 > --- a/src/qemu_driver.c > +++ b/src/qemu_driver.c > @@ -3912,10 +3912,15 @@ static int qemudDomainSave(virDomainPtr dom, > goto cleanup; > } > > - if (STREQ (prog, "raw")) > + const char *args;
I think you'll get a warning about "mixing code and data" here. At least, it's not at the top of a block, so we should move it to the top of a block.
I would have done that, but as I recall we're already declaring const char *prog immediately before its first use rather than at the top of a block, so I presumed we're using compile-time options which suppress that warning already. I'll double-check what's going on when I get back to the office.
No, you are right. This was part of the refactoring, and I just didn't re-read the code. I would prefer to move prog to the top of the block myself, and add args there; it just seems tidier. -- Chris Lalancette
Chris Lalancette wrote:
No, you are right. This was part of the refactoring, and I just didn't re-read the code. I would prefer to move prog to the top of the block myself, and add args there; it just seems tidier.
I agree that it's tidier -- but looking at things in context, I'm not very comfortable putting the declarations at the very top of the function. Part of the reason is that everything else there is initialized, even if only to NULL; I hate to break such a convention, but at the same time, I find it dangerous to suppress any warnings the compiler might otherwise be able to generate should a codepath allow a variable be used uninitialized. Does the below (creating a new code block and declaring both variables there) work for everyone?
Charles Duffy wrote:
Chris Lalancette wrote:
No, you are right. This was part of the refactoring, and I just didn't re-read the code. I would prefer to move prog to the top of the block myself, and add args there; it just seems tidier.
I agree that it's tidier -- but looking at things in context, I'm not very comfortable putting the declarations at the very top of the function. Part of the reason is that everything else there is initialized, even if only to NULL; I hate to break such a convention, but at the same time, I find it dangerous to suppress any warnings the compiler might otherwise be able to generate should a codepath allow a variable be used uninitialized.
Does the below (creating a new code block and declaring both variables there) work for everyone?
ACK -- Chris Lalancette
Charles Duffy wrote:
Chris Lalancette wrote:
No, you are right. This was part of the refactoring, and I just didn't re-read the code. I would prefer to move prog to the top of the block myself, and add args there; it just seems tidier.
I agree that it's tidier -- but looking at things in context, I'm not very comfortable putting the declarations at the very top of the function. Part of the reason is that everything else there is initialized, even if only to NULL; I hate to break such a convention, but at the same time, I find it dangerous to suppress any warnings the compiler might otherwise be able to generate should a codepath allow a variable be used uninitialized.
Does the below (creating a new code block and declaring both variables there) work for everyone?
I've committed this now, thanks. -- Chris Lalancette
participants (5)
-
Charles Duffy -
Chris Lalancette -
Daniel Berteaud -
Daniel P. Berrange -
Paolo Bonzini