From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:53015) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UK8Vg-0000GJ-V1 for qemu-devel@nongnu.org; Mon, 25 Mar 2013 10:36:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UK8Vd-00085t-NE for qemu-devel@nongnu.org; Mon, 25 Mar 2013 10:36:27 -0400 Received: from mx1.redhat.com ([209.132.183.28]:2297) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UK8Vd-00085g-F1 for qemu-devel@nongnu.org; Mon, 25 Mar 2013 10:36:25 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r2PEaOjq026983 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 25 Mar 2013 10:36:24 -0400 Date: Mon, 25 Mar 2013 15:36:22 +0100 From: Kevin Wolf Message-ID: <20130325143622.GA24866@dhcp-200-207.str.redhat.com> References: <1363873138-30568-1-git-send-email-rjones@redhat.com> <1363873138-30568-2-git-send-email-rjones@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1363873138-30568-2-git-send-email-rjones@redhat.com> Subject: Re: [Qemu-devel] [PATCH] block: Add support for Secure Shell (ssh) block device. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Richard W.M. Jones" Cc: qemu-devel@nongnu.org Am 21.03.2013 um 14:38 hat Richard W.M. Jones geschrieben: > From: "Richard W.M. Jones" > > 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://user@host/...) 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 > + * > + * 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