3:04 a.m.
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(a)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 {
5:30 a.m.
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.
10:55 p.m.
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(a)redhat.com>
To: <libvir-list(a)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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
11:25 p.m.
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 -=|
10:26 a.m.
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 -=|
8:20 p.m.
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(a)redhat.com>
To: "Kazuki Mizushima" <mizushima.kazuk(a)jp.fujitsu.com>
Cc: <libvir-list(a)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 -=|
5:25 a.m.
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
10:41 a.m.
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#0...
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)
6458
days inactive
6472
days old
8 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Kazuki Mizushima -
Richard W.M. Jones