[Libvir] [PATCH]Reprt error for a existing file
Hi, I make a patch which reports error for a existing file to prevent overwriting before the file. # ./virsh dump 1 a.dump error: file a.dump exists already # echo $? 1 # ./virsh save 1 a.save error: file a.save exists already # echo $? 1 As you know, we need to add this message item to libvirt/po. Signed-off-by: Kazuki Mizushima <mizushima.kazuk@jp.fujitsu.com> Thanks, Kazuki Mizushima Index: src/virsh.c =================================================================== RCS file: /data/cvs/libvirt/src/virsh.c,v retrieving revision 1.58 diff -u -p -r1.58 virsh.c --- src/virsh.c 2 Mar 2007 14:22:33 -0000 1.58 +++ src/virsh.c 5 Mar 2007 06:49:28 -0000 @@ -25,6 +25,7 @@ #include <getopt.h> #include <sys/types.h> #include <sys/time.h> +#include <sys/stat.h> #include <ctype.h> #include <fcntl.h> #include <locale.h> @@ -860,6 +861,7 @@ cmdSave(vshControl * ctl, vshCmd * cmd) virDomainPtr dom; char *name; char *to; + struct stat st; int ret = TRUE; if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) @@ -871,6 +873,11 @@ cmdSave(vshControl * ctl, vshCmd * cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, "domain", &name))) return FALSE; + if (stat(to, &st) == 0){ + vshError(ctl, FALSE, _("file %s exists already"), to); + return FALSE; + } + if (virDomainSave(dom, to) == 0) { vshPrint(ctl, _("Domain %s saved to %s\n"), name, to); } else { @@ -942,6 +949,7 @@ cmdDump(vshControl * ctl, vshCmd * cmd) virDomainPtr dom; char *name; char *to; + struct stat st; int ret = TRUE; if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) @@ -953,6 +961,11 @@ cmdDump(vshControl * ctl, vshCmd * cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, "domain", &name))) return FALSE; + if (stat(to, &st) == 0){ + vshError(ctl, FALSE, _("file %s exists already"), to); + return FALSE; + } + if (virDomainCoreDump(dom, to, 0) == 0) { vshPrint(ctl, _("Domain %s dumpd to %s\n"), name, to); } else {
Kazuki Mizushima wrote:
Hi, I make a patch which reports error for a existing file to prevent overwriting before the file.
# ./virsh dump 1 a.dump error: file a.dump exists already
@@ -871,6 +873,11 @@ cmdSave(vshControl * ctl, vshCmd * cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, "domain", &name))) return FALSE;
+ if (stat(to, &st) == 0){ + vshError(ctl, FALSE, _("file %s exists already"), to); + return FALSE; + } + if (virDomainSave(dom, to) == 0) { vshPrint(ctl, _("Domain %s saved to %s\n"), name, to); } else {
I guess I have a few problems with this patch. Currently only the xend driver actually supports saving domains (remote will support it in the next version too). And it does it by making the path absolute and sending an instruction to xend, so the save is done out of process and is essentially outside the control of libvirt (ie. the behaviour of xend might change in the future). So (IMHO) the questions are: (a) what should the behaviour of virDomainSave be when the file already exists? (b) where should we enforce that behaviour? (c) what behaviours can we support in the underlying drivers? If we accept that virDomainSave should always overwrite the dump file (question (a)), can we support that (question (c) - my reading of the source of libxc is that Xen itself won't overwrite files, but libvirt's xend driver can certainly be modified to delete the file first), and where should we enforce it (question (b) - in libvirt's xend driver). If, on the other hand, we want virDomainSave to never overwrite dump files and core files, then we should probably add a check in the drivers which support the methods (xend and remote) so that they fail under such conditions with a decent error message. My other problem is the use of stat(2) (or access) to determine if a file exists, since we up on slippy ground if this is relied upon in a security-related context. It's better to make the atomic open(2) call fail instead (which is actually what happens in current libxc). Rich.
Hi, Rich Thank you for reply and I understand you. I am lacking of REMOTE point of view, so this patch is not work for a remote hypervisor. And I also understand that this issue is not to be able to deal with libvirt.
My other problem is the use of stat(2) (or access) to determine if a file exists, since we up on slippy ground if this is relied upon in a security-related context. It's better to make the atomic open(2) call fail instead (which is actually what happens in current libxc).
Thank you for your indication. I misunderstand that xenXMDriver uses stat whether file is existing or not. The xenXMDriver uses it which isn't a file(S_ISREG). Thanks, Kazuki Mizushima ----- Original Message ----- From: "Richard W.M. Jones" <rjones@redhat.com> To: <libvir-list@redhat.com> Sent: Monday, March 05, 2007 8:30 PM Subject: Re: [Libvir] [PATCH]Reprt error for a existing file
Kazuki Mizushima wrote:
Hi, I make a patch which reports error for a existing file to prevent overwriting before the file.
# ./virsh dump 1 a.dump error: file a.dump exists already
@@ -871,6 +873,11 @@ cmdSave(vshControl * ctl, vshCmd * cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, "domain", &name))) return FALSE;
+ if (stat(to, &st) == 0){ + vshError(ctl, FALSE, _("file %s exists already"), to); + return FALSE; + } + if (virDomainSave(dom, to) == 0) { vshPrint(ctl, _("Domain %s saved to %s\n"), name, to); } else {
I guess I have a few problems with this patch. Currently only the xend driver actually supports saving domains (remote will support it in the next version too). And it does it by making the path absolute and sending an instruction to xend, so the save is done out of process and is essentially outside the control of libvirt (ie. the behaviour of xend might change in the future).
So (IMHO) the questions are: (a) what should the behaviour of virDomainSave be when the file already exists? (b) where should we enforce that behaviour? (c) what behaviours can we support in the underlying drivers?
If we accept that virDomainSave should always overwrite the dump file (question (a)), can we support that (question (c) - my reading of the source of libxc is that Xen itself won't overwrite files, but libvirt's xend driver can certainly be modified to delete the file first), and where should we enforce it (question (b) - in libvirt's xend driver).
If, on the other hand, we want virDomainSave to never overwrite dump files and core files, then we should probably add a check in the drivers which support the methods (xend and remote) so that they fail under such conditions with a decent error message.
My other problem is the use of stat(2) (or access) to determine if a file exists, since we up on slippy ground if this is relied upon in a security-related context. It's better to make the atomic open(2) call fail instead (which is actually what happens in current libxc).
Rich.
-- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Mar 06, 2007 at 01:55:19PM +0900, Kazuki Mizushima wrote:
My other problem is the use of stat(2) (or access) to determine if a file exists, since we up on slippy ground if this is relied upon in a security-related context. It's better to make the atomic open(2) call fail instead (which is actually what happens in current libxc).
Thank you for your indication. I misunderstand that xenXMDriver uses stat whether file is existing or not. The xenXMDriver uses it which isn't a file(S_ISREG).
BTW, the 'xenXMDriver' is a rather special beast, so I wouldn't neccessarily use it as a guide for good practice in libvirt :-) XenD only got support for managing inactive domains (aka lifecycle support) in version 3.0.4. With older versions of Xen, there is no way to enumerate inactive domains. So we invented the 'xenXMDriver' which reads config files from /etc/xen to determine list of inactive guests. This code is only used on Xen 3.0.3 or older and is fairly limited in what it supports compared to the new XenD lifecycle code. Since xenXMDriver doesn't talk to XenD at all, it operates in more or less the same context as the client app using libvirt, so in this circumstance it is validate to use stat() and other file access calls like that. For any code related to XenD though, file access checks and changes need to be in XenD itself to maintain the correct use context. Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On Mon, Mar 05, 2007 at 06:04:49PM +0900, Kazuki Mizushima wrote:
Hi,
I make a patch which reports error for a existing file to prevent overwriting before the file.
diff -u -p -r1.58 virsh.c --- src/virsh.c 2 Mar 2007 14:22:33 -0000 1.58 +++ src/virsh.c 5 Mar 2007 06:49:28 -0000 @@ -871,6 +873,11 @@ cmdSave(vshControl * ctl, vshCmd * cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, "domain", &name))) return FALSE;
+ if (stat(to, &st) == 0){ + vshError(ctl, FALSE, _("file %s exists already"), to); + return FALSE; + } +
This isn't going to work if virsh is talking to a remote hypervisor. It also assumes that virsh can read the directory where the image is being saved, which isn't neccessarily true. The directory may very well only be readable by XenD itself so putting in an explicit overwrite check is just giving a false sense of security to users. There is really no way virsh/libvirt can reliably check for dump or save file existance - only XenD or QEMU can do that. So if we want these kind of policy checks they should be put into the XenD or QEMU as appropriate. Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
Hi Dan, Thank you for replay. I forgot REMOTE access!!! Certainty, this patch is not be able to work for a remote hypervisor, and I understand that this issue and these kind of policy checks is for under layer, XenD or so. I'II try to put this into XenD. Thanks, Kazuki Mizushima ----- Original Message ----- From: "Daniel P. Berrange" <berrange@redhat.com> To: "Kazuki Mizushima" <mizushima.kazuk@jp.fujitsu.com> Cc: <libvir-list@redhat.com> Sent: Tuesday, March 06, 2007 1:26 AM Subject: Re: [Libvir] [PATCH]Reprt error for a existing file
On Mon, Mar 05, 2007 at 06:04:49PM +0900, Kazuki Mizushima wrote:
Hi,
I make a patch which reports error for a existing file to prevent overwriting before the file.
diff -u -p -r1.58 virsh.c --- src/virsh.c 2 Mar 2007 14:22:33 -0000 1.58 +++ src/virsh.c 5 Mar 2007 06:49:28 -0000 @@ -871,6 +873,11 @@ cmdSave(vshControl * ctl, vshCmd * cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, "domain", &name))) return FALSE;
+ if (stat(to, &st) == 0){ + vshError(ctl, FALSE, _("file %s exists already"), to); + return FALSE; + } +
This isn't going to work if virsh is talking to a remote hypervisor. It also assumes that virsh can read the directory where the image is being saved, which isn't neccessarily true. The directory may very well only be readable by XenD itself so putting in an explicit overwrite check is just giving a false sense of security to users. There is really no way virsh/libvirt can reliably check for dump or save file existance - only XenD or QEMU can do that. So if we want these kind of policy checks they should be put into the XenD or QEMU as appropriate.
Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
Hi,
under layer, XenD or so. I'II try to put this into XenD.
I tried to put xen community, it rejects. Though I am understanding follwing, Isn't there something good way ?
There is really no way virsh/libvirt can reliably check for dump or save file existance - only XenD or QEMU can do that. So if we want
If this goes on, for user managing services by using virsh/libvirt, there is no way to recover for the user. For example, save file(maybe used as backup file) is overwritten without any confirmation, I suspect that user confuses about the one different from expected OS starting. Surely, although it is a user's mistake, I want to solve this by saying a confirmation like a below, so that user can confirm it once. # virsh save <domain> <file> save: save file "<file>"? # # virsh dump <domain> <file> dump: dump file "<file>"? # How do you think about this? Thanks, Kazuki Mizushima
Kazuki Mizushima wrote:
Hi,
under layer, XenD or so. I'II try to put this into XenD.
I tried to put xen community, it rejects.
[For reference, here is the thread on xen-devel] http://lists.xensource.com/archives/html/xen-devel/2007-03/threads.html#0029...
Though I am understanding follwing, Isn't there something good way ?
There is really no way virsh/libvirt can reliably check for dump or save file existance - only XenD or QEMU can do that. So if we want
If this goes on, for user managing services by using virsh/libvirt, there is no way to recover for the user.
For example, save file(maybe used as backup file) is overwritten without any confirmation, I suspect that user confuses about the one different from expected OS starting.
Surely, although it is a user's mistake, I want to solve this by saying a confirmation like a below, so that user can confirm it once.
# virsh save <domain> <file> save: save file "<file>"? #
# virsh dump <domain> <file> dump: dump file "<file>"? #
How do you think about this?
There's not a "good" way to solve this without support in XenD. The question is are there bad ways to solve this in libvirt or virsh. What do people think about having an optional flag in virsh, like: virsh save --no-clobber <domain> <file> Unfortunately if we add support for this flag into libvirt then we either have to break ABI compatibility or else have to add a new call. Neither is pleasant. Or we could just add it to virsh. Don't know ... I've been thinking about this on and off for a while, and I can't see a good way to fix it. Rich. -- Emerging Technologies, Red Hat http://et.redhat.com/~rjones/ 64 Baker Street, London, W1U 7DF Mobile: +44 7866 314 421 "[Negative numbers] darken the very whole doctrines of the equations and make dark of the things which are in their nature excessively obvious and simple" (Francis Maseres FRS, mathematician, 1759)
Hi, Rich
Don't know ... I've been thinking about this on and off for a while, and I can't see a good way to fix it.
Thank you for your reply and showing your policy. I agree that there is no *fundamental* solution without the support of XenD. Thanks, Kazuki Mizushima
participants (3)
-
Daniel P. Berrange -
Kazuki Mizushima -
Richard W.M. Jones