On Sun, Apr 17, 2022 at 09:32:03AM +0200, Laszlo Ersek wrote:
> On 04/16/22 13:21, Richard W.M. Jones wrote:
> > This adds new parameters, create=(true|false), create-size=SIZE and
> > create-mode=MODE to create and truncate the remote file.
> > ---
> > plugins/ssh/nbdkit-ssh-plugin.pod | 37 +++++++++++++++++--
> > plugins/ssh/ssh.c | 60 +++++++++++++++++++++++++++++--
> > tests/test-ssh.sh | 13 ++++++-
> > 3 files changed, 105 insertions(+), 5 deletions(-)
> >
> > diff --git a/plugins/ssh/nbdkit-ssh-plugin.pod
> > b/plugins/ssh/nbdkit-ssh-plugin.pod
> > index 3f401c15..648fa94d 100644
> > --- a/plugins/ssh/nbdkit-ssh-plugin.pod
> > +++ b/plugins/ssh/nbdkit-ssh-plugin.pod
> > @@ -5,8 +5,10 @@ nbdkit-ssh-plugin - access disk images over the SSH
> > protocol
> > =head1 SYNOPSIS
> >
> > nbdkit ssh host=HOST [path=]PATH
> > - [compression=true] [config=CONFIG_FILE] [identity=FILENAME]
> > - [known-hosts=FILENAME] [password=PASSWORD|-|+FILENAME]
> > + [compression=true] [config=CONFIG_FILE]
> > + [create=true] [create-mode=MODE] [create-size=SIZE]
> > + [identity=FILENAME] [known-hosts=FILENAME]
> > + [password=PASSWORD|-|+FILENAME]
> > [port=PORT] [timeout=SECS] [user=USER]
> > [verify-remote-host=false]
> >
> > @@ -62,6 +64,37 @@ The C<config> parameter is optional. If it is I<not>
> > specified at all
> > then F<~/.ssh/config> and F</etc/ssh/ssh_config> are both read.
> > Missing or unreadable files are ignored.
> >
> > +=item B<create=true>
> > +
> > +(nbdkit E<ge> 1.32)
> > +
> > +If set, the remote file will be created if it does not exist already.
> > +Note the remote file is created on first NBD connection to nbdkit.
> > +
> > +If using this option, you must use C<create-size>. C<create-mode> can
> > +be used to control the permissions of the new file.
> > +
> > +=item B<create-mode=>MODE
> > +
> > +(nbdkit E<ge> 1.32)
> > +
> > +If using C<create=true> specify the default permissions of the new
> > +remote file. You can use octal modes like C<create-mode=0777>. The
> > +default is C<0600>, ie. only readable and writable by the remote user.
> > +
> > +The SFTP server may apply a L<umask(2)> which is specific to the
> > +remote system and cannot be queried from the client, so the final
> > +permissions may be more restrictive than requested. For OpenSSH I
> > +found that files are always created with mode & 0700 whatever mode is
> > +specified here, but other servers might be different.
> > +
> > +=item B<create-size=>SIZE
> > +
> > +(nbdkit E<ge> 1.32)
> > +
> > +If using C<create=true>, specify the virtual size of the new disk.
> > +C<SIZE> can use modifiers like C<100M> etc.
> > +
> > =item B<host=>HOST
> >
> > Specify the name or IP address of the remote host.
> > diff --git a/plugins/ssh/ssh.c b/plugins/ssh/ssh.c
> > index 3b2b21c8..66998813 100644
> > --- a/plugins/ssh/ssh.c
> > +++ b/plugins/ssh/ssh.c
> > @@ -63,6 +63,9 @@ static const char *known_hosts = NULL;
> > static const_string_vector identities = empty_vector;
> > static uint32_t timeout = 0;
> > static bool compression = false;
> > +static bool create = false;
> > +static int64_t create_size = -1;
> > +static unsigned create_mode = S_IRUSR | S_IWUSR /* 0600 */;
> >
> > /* config can be:
> > * NULL => parse options from default file
> > @@ -167,6 +170,27 @@ ssh_config (const char *key, const char *value)
> > return -1;
> > compression = r;
> > }
> > + else if (strcmp (key, "create") == 0) {
> > + r = nbdkit_parse_bool (value);
> > + if (r == -1)
> > + return -1;
> > + create = r;
> > + }
> > + else if (strcmp (key, "create-size") == 0) {
> > + create_size = nbdkit_parse_size (value);
> > + if (create_size == -1)
> > + return -1;
> > + }
> > + else if (strcmp (key, "create-mode") == 0) {
> > + r = nbdkit_parse_unsigned (key, value, &create_mode);
> > + if (r == -1)
> > + return -1;
> > + /* OpenSSH checks this too. */
> > + if (create_mode > 0777) {
> > + nbdkit_error ("create-mode must be <= 0777");
> > + return -1;
> > + }
> > + }
> >
> > else {
> > nbdkit_error ("unknown parameter '%s'", key);
> > @@ -186,6 +210,13 @@ ssh_config_complete (void)
> > return -1;
> > }
> >
> > + /* If create=true, create-size must be supplied. */
> > + if (create && create_size == -1) {
> > + nbdkit_error ("if using create=true, you must specify the size "
> > + "of the new remote file using create-size=SIZE");
> > + return -1;
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -200,7 +231,10 @@ ssh_config_complete (void)
> > "identity=<FILENAME> Prepend private key (identity) file.\n" \
> > "timeout=SECS Set SSH connection timeout.\n" \
> > "verify-remote-host=false Ignore known_hosts.\n" \
> > - "compression=true Enable compression."
> > + "compression=true Enable compression.\n" \
> > + "create=true Create the remote file.\n" \
> > + "create-mode=MODE Set the permissions of the remote file.\n" \
> > + "create-size=SIZE Set the size of the remote file."
> >
> > /* The per-connection handle. */
> > struct ssh_handle {
> > @@ -480,7 +514,8 @@ ssh_open (int readonly)
> > goto err;
> > }
> > access_type = readonly ? O_RDONLY : O_RDWR;
> > - h->file = sftp_open (h->sftp, path, access_type, S_IRWXU);
> > + if (create) access_type |= O_CREAT;
> > + h->file = sftp_open (h->sftp, path, access_type, create_mode);
> > if (!h->file) {
> > nbdkit_error ("cannot open file for %s: %s",
> > readonly ? "reading" : "writing",
> > @@ -488,6 +523,27 @@ ssh_open (int readonly)
> > goto err;
> > }
> >
> > + if (create) {
> > + /* There's no sftp_truncate call. However OpenSSH lets you call
> > + * SSH_FXP_SETSTAT + SSH_FILEXFER_ATTR_SIZE which invokes
> > + * truncate(2) on the server. Libssh doesn't provide a binding
> > + * for SSH_FXP_FSETSTAT so we have to pass the session + path.
> > + */
> > + struct sftp_attributes_struct attrs = {
> > + .flags = SSH_FILEXFER_ATTR_SIZE,
> > + .size = create_size,
> > + };
> > +
> > + r = sftp_setstat (h->sftp, path, &attrs);
> > + if (r != SSH_OK) {
> > + nbdkit_error ("truncate failed: %s", ssh_get_error (h->session));
> > + goto err;
> > + }
> > + }
>
> 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.]
> (c) if the normal open failed too, then we witnessed some race
> condition, where the file existed at point (a), but disappeared until
> point (b). In that case (i.e., if both opens fail), simply report the
> error for the second open (b), and bail out.
>
> (2) Additionally, there's an obscure error mode here (I think) that's
> due to the creation and the size setting not being atomic: if we succeed
> to create in step (1a), but fail to set the size (for whatever reason),
> on next startup, the file will exist (most likely) and so there won't
> even be an attempt to set the size. I think we can improve upon this a
> little bit by deleting the remote file if the O_CREAT|O_EXCL open
> succeeded (1a), but sftp_setstat() fails.
>
> ... I think my observation (1) is more important than (2); AIUI, there's
> no way to know "I created this file as a new file" except trying
> O_CREAT|O_EXCL, and seeing it succeed.
>
> > +
> > + /* On the next open, don't create or truncate the file. */
> > + create = false;
>
> (3) What is the thread model of this plugin? Is it possible for two
> opens to run in parallel?
It's SERIALIZE_REQUESTS so we're OK.
$ nbdkit ssh --dump-plugin | grep thread
max_thread_model=serialize_requests
thread_model=serialize_requests
Rich.
> Thanks,
> Laszlo
>
> > +
> > nbdkit_debug ("opened libssh handle");
> >
> > return h;
> > diff --git a/tests/test-ssh.sh b/tests/test-ssh.sh
> > index 6c0ce410..f04b4488 100755
> > --- a/tests/test-ssh.sh
> > +++ b/tests/test-ssh.sh
> > @@ -36,6 +36,7 @@ set -x
> >
> > requires test -f disk
> > requires nbdcopy --version
> > +requires stat --version
> >
> > # Check that ssh to localhost will work without any passwords or phrases.
> > #
> > @@ -48,7 +49,7 @@ then
> > exit 77
> > fi
> >
> > -files="ssh.img"
> > +files="ssh.img ssh2.img"
> > rm -f $files
> > cleanup_fn rm -f $files
> >
> > @@ -59,3 +60,13 @@ nbdkit -v -D ssh.log=2 -U - \
> >
> > # The output should be identical.
> > cmp disk ssh.img
> > +
> > +# Copy local file 'ssh.img' to newly created "remote" 'ssh2.img'
> > +size="$(stat -c %s disk)"
> > +nbdkit -v -D ssh.log=2 -U - \
> > + ssh host=localhost $PWD/ssh2.img \
> > + create=true create-size=$size \
> > + --run 'nbdcopy ssh.img "$uri"'
> > +
> > +# The output should be identical.
> > +cmp disk ssh2.img
> >
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
_______________________________________________
Libguestfs mailing list
[email protected]
https://listman.redhat.com/mailman/listinfo/libguestfs
