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

On 04/17/22 11:09, Richard W.M. Jones wrote:

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.]

"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". Laszlo

...

(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