Laszlo Ersek
2022-Apr-17 10:08 UTC
[Libguestfs] [PATCH nbdkit v2] ssh: Allow the remote file to be created
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 >>> >
Richard W.M. Jones
2022-Apr-17 10:20 UTC
[Libguestfs] [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