[Libvir] [PATCH]Reprt error for a existing file

[Richard W.M. Jones]

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.