Re: [Libguestfs] [PATCH nbdkit v2] ssh: Allow the remote file to be created

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