Libguestfs - Apr 2022 - [PATCH nbdkit v2] ssh: Allow the remote file to be created

On Sun, Apr 17, 2022 at 12:08:36PM +0200, Laszlo Ersek
wrote:
> On 04/17/22 11:09, Richard W.M. Jones wrote:
> > On Sun, Apr 17, 2022 at 09:32:03AM +0200, Laszlo Ersek wrote:
> >> Two comments:
> >>
> >> (1) If the file already exists, then O_CREAT will have no effect;
> >> however, we'll still resize the file. I think that's not
ideal.
> >>
> >> I think the usual approach for this is:
> >>
> >> (a) attempt O_CREAT | O_EXCL; if it succeeds, perform the truncate
as well
> >>
> >> (b) if O_CREAT | O_EXCL fails, then just attempt the open without
> >> *either* flag. If that succeeds, do not perform the truncate.
> > 
> > I wonder here if what we feel about:
> > 
> >   nbdkit ssh ... create=true create-size=100M
> > 
> > that ends up working but the size isn't 100M.
> > 
> > Should we fail if the remote exists already?  Maybe this is something
> > we should let the user choose.
> > 
> > [Also I wonder what qemu blockdev-create does - need to investigate.]
> 
> "create-mode" is ignored if the file exists; ignoring
"create-size" in
> that case would be consistent. "Use this image if it exists; if it
does
> not, create it with these permissons and this size".
This is one way it could work, but from a usability point of view, how
does the nbdkit user know if the remote file exists or not?

With the current implementation we leave an zero-sized file lying
around if setstat fails.  We should fix that bug, but I doubt an
implementation can be 100% reliable (eg: if sftp_setstat fails, is the
connection dead?  will sftp_unlink work?)  Ignoring create-size if the
file exists could result in a zero-sized file that's impossible to
recover from except by manual intervention.

I don't think there's a good answer here.  Possibly we want to let the
user decide how they want it to work ...

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

Thinking about this even more, maybe if create=true we want to always
create the file, and never ignore create-mode or create-size?

(One issue with the current code is if the file exists already, we
truncate it to the correct size but leave the existing content.  But
callers might reasonably expect that create=true creates an
all-zeroes sparse file).

I think we could do this using O_TRUNC.  The permissions can be set
using sftp_setstat (which I think ignores umask).

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On 04/17/22 12:20, Richard W.M. Jones wrote:
> On Sun, Apr 17, 2022 at 12:08:36PM +0200, Laszlo Ersek wrote:
>> On 04/17/22 11:09, Richard W.M. Jones wrote:
>>> On Sun, Apr 17, 2022 at 09:32:03AM +0200, Laszlo Ersek wrote:
>>>> Two comments:
>>>>
>>>> (1) If the file already exists, then O_CREAT will have no
effect;
>>>> however, we'll still resize the file. I think that's
not ideal.
>>>>
>>>> I think the usual approach for this is:
>>>>
>>>> (a) attempt O_CREAT | O_EXCL; if it succeeds, perform the
truncate as well
>>>>
>>>> (b) if O_CREAT | O_EXCL fails, then just attempt the open
without
>>>> *either* flag. If that succeeds, do not perform the truncate.
>>>
>>> I wonder here if what we feel about:
>>>
>>>   nbdkit ssh ... create=true create-size=100M
>>>
>>> that ends up working but the size isn't 100M.
>>>
>>> Should we fail if the remote exists already?  Maybe this is
something
>>> we should let the user choose.
>>>
>>> [Also I wonder what qemu blockdev-create does - need to
investigate.]
>>
>> "create-mode" is ignored if the file exists; ignoring
"create-size" in
>> that case would be consistent. "Use this image if it exists; if it
does
>> not, create it with these permissons and this size".
> 
> This is one way it could work, but from a usability point of view, how
> does the nbdkit user know if the remote file exists or not?
> 
> With the current implementation we leave an zero-sized file lying
> around if setstat fails.  We should fix that bug, but I doubt an
> implementation can be 100% reliable (eg: if sftp_setstat fails, is the
> connection dead?  will sftp_unlink work?)  Ignoring create-size if the
> file exists could result in a zero-sized file that's impossible to
> recover from except by manual intervention.
> 
> I don't think there's a good answer here.  Possibly we want to let
the
> user decide how they want it to work ...
Right; you can always introduce more plugin params for controlling this!
Laszlo