Re: [Qemu-devel] [PATCH] block: Add support for Secure Shell (ssh) block device.

[Kevin Wolf]

Am 21.03.2013 um 14:38 hat Richard W.M. Jones geschrieben:
> From: "Richard W.M. Jones" <address@hidden>
> 
>   qemu-system-x86_64 -drive file=ssh://hostname/some/image
> 
> QEMU will ssh into 'hostname' and open '/some/image' which is made
> available as a standard block device.
> 
> You can specify a username (ssh://address@hidden/...) and/or a port number
> (ssh://host:port/...).
> 
> Current limitations:
> 
> - Authentication must be done without passwords or passphrases, using
>   ssh-agent.  Other authentication methods are not supported. (*)

Maybe you can just expose it as an encrypted image in order to allow
password authentication?

> - Does not check host key. (*)
> 
> - New remote files cannot be created. (*)
> 
> - Uses coroutine read/write, instead of true AIO.  (libssh2 supports
>   non-blocking access, so this could be fixed with some effort).
> 
> - Blocks during connection and authentication.
> 
> (*) = potentially easy fix
> 
> This is implemented using libssh2 on the client side.  The server just
> requires a regular ssh daemon with sftp-server support.  Most ssh
> daemons on Unix/Linux systems will work out of the box.

> --- /dev/null
> +++ b/block/ssh.c
> @@ -0,0 +1,514 @@
> +/*
> + * Secure Shell (ssh) backend for QEMU.
> + *
> + * Copyright (C) 2013 Red Hat Inc., Richard W.M. Jones <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */

Please consider the MIT license or LGPL for block layer code; otherwise
we may have to compile the driver out for a future libqblock.

> +
> +#define SECTOR_SIZE 512

BDRV_SECTOR_SIZE exists.

> +
> +static void __attribute__((constructor)) ssh_init(void)
> +{
> +    int r;
> +
> +    r = libssh2_init(0);
> +    if (r != 0) {
> +        fprintf(stderr, "libssh2 initialization failed, %d\n", r);
> +        exit(EXIT_FAILURE);
> +    }
> +}

Why don't you include this in bdrv_ssh_init()?

> +static int parse_uri(BDRVSSHState *s, const char *filename)
> +{
> +    URI *uri;
> +
> +    uri = uri_parse(filename);
> +    if (!uri)
> +        return -EINVAL;
> +
> +    if (strcmp(uri->scheme, "ssh") != 0) {
> +        error_report("URI scheme must be 'ssh'");
> +        goto err;
> +    }
> +
> +    if (!uri->server || strcmp(uri->server, "") == 0) {
> +        error_report("missing hostname in URI");
> +        goto err;
> +    }
> +
> +    if (!uri->path || strcmp(uri->path, "") == 0) {
> +        error_report("missing remote path in URI");
> +        goto err;
> +    }
> +
> +    /* If user is defined in the URI, use it.  Else use local username. */
> +    if(uri->user && strcmp(uri->user, "") != 0)
> +        s->user = g_strdup(uri->user);
> +    else {
> +        s->user = get_username();
> +        if (!s->user)
> +            goto err;
> +    }
> +
> +    /* Construct the hostname:port string for inet_connect. */
> +    if(uri->port == 0)
> +        uri->port = 22;
> +    s->host = g_strdup_printf("%s:%d", uri->server, uri->port);
> +
> +    s->path = g_strdup(uri->path);
> +
> +    return 0;
> +
> + err:
> +    uri_free(uri);
> +    return -EINVAL;
> +}

You could implement this similar to what I just changed in NBD, using
the new .bdrv_parse_filename callback and driver-specific options, so
that instead of specifying a URL you can use something like:

    -drive file.driver=ssh,file.host=localhost,file.user=test

This should turn out much more elegant than management tools encoding
everything in a single filename string and qemu parsing it again,
especially once QMP support passing this as JSON objects.

> +static int ssh_file_open(BlockDriverState *bs, const char *filename,
> +                         int bdrv_flags)

After rebasing, this will have a new parameter for driver-specific
options.

> +    ret = parse_uri(s, filename);
> +    if (ret < 0)
> +        goto err;

I only see it now, but it seems to be repeated all over the place: The
qemu coding style requires braces here.

> +
> +    /* Open the socket and connect. */
> +    s->sock = inet_connect(s->host, &err);
> +    if (err != NULL) {
> +        ret = -errno;
> +        qerror_report_err(err);
> +        error_free(err);
> +        goto err;
> +    }
> +
> +    /* Start up SSH. */
> +    ret = connect_to_ssh(s);
> +    if (ret < 0)
> +        goto err;
> +
> +#if 0 /* Enable this when AIO callbacks are implemented. */
> +    /* Go non-blocking. */
> +    libssh2_session_set_blocking(s->session, 0);
> +#endif
> +
> +    qemu_co_mutex_init(&s->lock);
> +
> +    return 0;
> +
> + err:
> +    if (s->sock >= 0)
> +        close(s->sock);
> +    s->sock = -1;
> +
> +    g_free(s->path);
> +    s->path = NULL;
> +    g_free(s->host);
> +    s->host = NULL;
> +    g_free(s->user);
> +    s->user = NULL;

Resetting the fields isn't really necessary. s will be freed anyway.

> +static BlockDriver bdrv_ssh = {
> +    .format_name                  = "ssh",
> +    .protocol_name                = "ssh",
> +    .instance_size                = sizeof(BDRVSSHState),
> +    .bdrv_file_open               = ssh_file_open,
> +    .bdrv_close                   = ssh_close,
> +    .bdrv_read                    = ssh_co_read,
> +    .bdrv_write                   = ssh_co_write,
> +    .bdrv_getlength               = ssh_getlength,
> +#if 0
> +    .bdrv_aio_readv               = ssh_aio_readv,
> +    .bdrv_aio_writev              = ssh_aio_writev,
> +    .bdrv_aio_flush               = ssh_aio_flush,
> +#endif
> +};

You don't have a bdrv_co_flush_to_disk, what does this mean? Is every
write immediately flushed to the disk, is the driver unsafe by design,
or is this just a missing implementation detail?

Kevin