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
>>>
>
_______________________________________________
Libguestfs mailing list
[email protected]
https://listman.redhat.com/mailman/listinfo/libguestfs
