On Mon, Mar 27, 2017 at 10:27 AM, Stefan Hajnoczi <stefa...@gmail.com> wrote:
> On Sun, Mar 26, 2017 at 07:50:35PM -0700, Ashish Mittal wrote:
>
> Have you tested live migration?
>
> If live migration is not supported then a migration blocker should be
> added using migrate_add_blocker().
>
We do support live migration. We have been testing a fork of this code
(slightly different version) with live migration.
>> v10 changelog:
>> (1) Implemented accepting TLS creds per block device via the CLI
>> (see 3rd e.g in commit log). Corresponding changes made to the
>> libqnio library.
>> (2) iio_open() changed to accept TLS creds and use these internally
>> to set up SSL connections.
>> (3) Got rid of hard-coded VXHS_UUID_DEF. qemu_uuid is no longer used
>> for authentication in any way.
>
> Why does the code still access qemu_uuid and pass the UUID string to
> iio_init()?
>
I was of the opinion that knowing what instance (for qemu-kvm case)
was opening a block device could be a useful piece of information for
the block device to have in the future.
> In libqnio.git (66698ca47bc594a9f623c240d63ea535f5a42b47) the 'instance'
> field is unused and not sent over the wire. Please drop it.
>
It is not used at present. I will drop it.
>> diff --git a/block/vxhs.c b/block/vxhs.c
>> new file mode 100644
>> index 0000000..b98b535
>> --- /dev/null
>> +++ b/block/vxhs.c
>> @@ -0,0 +1,595 @@
>> +/*
>> + * QEMU Block driver for Veritas HyperScale (VxHS)
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include <qnio/qnio_api.h>
>> +#include <sys/param.h>
>> +#include "block/block_int.h"
>> +#include "qapi/qmp/qerror.h"
>> +#include "qapi/qmp/qdict.h"
>> +#include "qapi/qmp/qstring.h"
>> +#include "trace.h"
>> +#include "qemu/uri.h"
>> +#include "qapi/error.h"
>> +#include "qemu/uuid.h"
>> +#include "crypto/tlscredsx509.h"
>> +
>> +#define VXHS_OPT_FILENAME "filename"
>> +#define VXHS_OPT_VDISK_ID "vdisk-id"
>> +#define VXHS_OPT_SERVER "server"
>> +#define VXHS_OPT_HOST "host"
>> +#define VXHS_OPT_PORT "port"
>> +
>> +QemuUUID qemu_uuid __attribute__ ((weak));
>> +
>> +static uint32_t vxhs_ref;
>
> It would be nice to add:
> /* Only accessed under QEMU global mutex */
>
Will do.
>> +/*
>> + * Parse the incoming URI and populate *options with the host information.
>> + * URI syntax has the limitation of supporting only one host info.
>> + * To pass multiple host information, use the JSON syntax.
>
> References to multiple hosts are out of date. The driver only supports
> a single host now.
>
Will change.
>> + */
>> +static int vxhs_parse_uri(const char *filename, QDict *options)
>> +{
>> + URI *uri = NULL;
>> + char *hoststr, *portstr;
>> + char *port;
>> + int ret = 0;
>> +
>> + trace_vxhs_parse_uri_filename(filename);
>> + uri = uri_parse(filename);
>> + if (!uri || !uri->server || !uri->path) {
>> + uri_free(uri);
>> + return -EINVAL;
>> + }
>> +
>> + hoststr = g_strdup(VXHS_OPT_SERVER".host");
>> + qdict_put(options, hoststr, qstring_from_str(uri->server));
>> + g_free(hoststr);
>> +
>> + portstr = g_strdup(VXHS_OPT_SERVER".port");
>> + if (uri->port) {
>> + port = g_strdup_printf("%d", uri->port);
>> + qdict_put(options, portstr, qstring_from_str(port));
>> + g_free(port);
>> + }
>> + g_free(portstr);
>
> The g_strdup()/g_free() isn't necessary for the qdict_put() key
> argument. The key belongs to the caller so we can pass a string
> literal:
Will Change.
>
> qdict_put(options, VXHS_OPT_SERVER ".host", qstring_from_str(uri->server));
> if (uri->port) {
> port = g_strdup_printf("%d", uri->port);
> qdict_put(options, VXHS_OPT_SERVER ".port", qstring_from_str(port));
> g_free(port);
> }
>
>> +
>> + if (strstr(uri->path, "vxhs") == NULL) {
>
> What does this check do?
>
Not sure about the history, but it's been there since first code
draft. Will check if it serves any purpose, or remove it.
>> +static int vxhs_open(BlockDriverState *bs, QDict *options,
>> + int bdrv_flags, Error **errp)
>> +{
>> + BDRVVXHSState *s = bs->opaque;
>> + void *dev_handlep = NULL;
>> + QDict *backing_options = NULL;
>> + QemuOpts *opts, *tcp_opts;
>> + char *of_vsa_addr = NULL;
>> + Error *local_err = NULL;
>> + const char *vdisk_id_opt;
>> + const char *server_host_opt;
>> + char *str = NULL;
>> + int ret = 0;
>> + char *cacert = NULL;
>> + char *client_key = NULL;
>> + char *client_cert = NULL;
>> +
>> + ret = vxhs_init_and_ref();
>> + if (ret < 0) {
>> + return ret;
>> + }
>> +
>> + /* Create opts info from runtime_opts and runtime_tcp_opts list */
>> + opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
>> + tcp_opts = qemu_opts_create(&runtime_tcp_opts, NULL, 0, &error_abort);
>> +
>> + qemu_opts_absorb_qdict(opts, options, &local_err);
>> + if (local_err) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + /* vdisk-id is the disk UUID */
>> + vdisk_id_opt = qemu_opt_get(opts, VXHS_OPT_VDISK_ID);
>> + if (!vdisk_id_opt) {
>> + error_setg(&local_err, QERR_MISSING_PARAMETER, VXHS_OPT_VDISK_ID);
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + /* vdisk-id may contain a leading '/' */
>> + if (strlen(vdisk_id_opt) > UUID_FMT_LEN + 1) {
>> + error_setg(&local_err, "vdisk-id cannot be more than %d characters",
>> + UUID_FMT_LEN);
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + s->vdisk_guid = g_strdup(vdisk_id_opt);
>> + trace_vxhs_open_vdiskid(vdisk_id_opt);
>> +
>> + /* get the 'server.' arguments */
>> + str = g_strdup_printf(VXHS_OPT_SERVER".");
>> + qdict_extract_subqdict(options, &backing_options, str);
>
> g_strdup_printf() is unnecessary. You can eliminate the 'str' local
> variable and just do:
>
> qdict_extract_subqdict(options, &backing_options, VXHS_OPT_SERVER ".");
>
Will do. Thanks!
>> +
>> + qemu_opts_absorb_qdict(tcp_opts, backing_options, &local_err);
>> + if (local_err != NULL) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + server_host_opt = qemu_opt_get(tcp_opts, VXHS_OPT_HOST);
>> + if (!server_host_opt) {
>> + error_setg(&local_err, QERR_MISSING_PARAMETER,
>> + VXHS_OPT_SERVER"."VXHS_OPT_HOST);
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + if (strlen(server_host_opt) > MAXHOSTNAMELEN) {
>> + error_setg(&local_err, "server.host cannot be more than %d
>> characters",
>> + MAXHOSTNAMELEN);
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + /* check if we got tls-creds via the --object argument */
>> + s->tlscredsid = g_strdup(qemu_opt_get(opts, "tls-creds"));
>> + if (s->tlscredsid) {
>> + vxhs_get_tls_creds(s->tlscredsid, &cacert, &client_key,
>> + &client_cert, &local_err);
>> + if (local_err != NULL) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> + trace_vxhs_get_creds(cacert, client_key, client_cert);
>> + }
>> +
>> + s->vdisk_hostinfo.host = g_strdup(server_host_opt);
>> + s->vdisk_hostinfo.port = g_ascii_strtoll(qemu_opt_get(tcp_opts,
>> + VXHS_OPT_PORT),
>> + NULL, 0);
>> +
>> + trace_vxhs_open_hostinfo(s->vdisk_hostinfo.host,
>> + s->vdisk_hostinfo.port);
>> +
>> + of_vsa_addr = g_strdup_printf("of://%s:%d",
>> + s->vdisk_hostinfo.host,
>> + s->vdisk_hostinfo.port);
>> +
>> + /*
>> + * Open qnio channel to storage agent if not opened before
>> + */
>> + dev_handlep = iio_open(of_vsa_addr, s->vdisk_guid, 0,
>> + cacert, client_key, client_cert);
>> + if (dev_handlep == NULL) {
>> + trace_vxhs_open_iio_open(of_vsa_addr);
>> + ret = -ENODEV;
>> + goto out;
>> + }
>> + s->vdisk_hostinfo.dev_handle = dev_handlep;
>> +
>> +out:
>> + g_free(str);
>> + g_free(of_vsa_addr);
>> + QDECREF(backing_options);
>> + qemu_opts_del(tcp_opts);
>> + qemu_opts_del(opts);
>> + g_free(cacert);
>> + g_free(client_key);
>> + g_free(client_cert);
>> +
>> + if (ret < 0) {
>> + vxhs_unref();
>> + error_propagate(errp, local_err);
>> + g_free(s->vdisk_hostinfo.host);
>> + g_free(s->vdisk_guid);
>> + g_free(s->tlscredsid);
>> + s->vdisk_guid = NULL;
>> + errno = -ret;
>
> .bdrv_open() does not promise anything about errno. This line can be
> dropped.
>
Will do.
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static const AIOCBInfo vxhs_aiocb_info = {
>> + .aiocb_size = sizeof(VXHSAIOCB)
>> +};
>> +
>> +/*
>> + * This allocates QEMU-VXHS callback for each IO
>> + * and is passed to QNIO. When QNIO completes the work,
>> + * it will be passed back through the callback.
>> + */
>> +static BlockAIOCB *vxhs_aio_rw(BlockDriverState *bs, int64_t sector_num,
>> + QEMUIOVector *qiov, int nb_sectors,
>> + BlockCompletionFunc *cb, void *opaque,
>> + VDISKAIOCmd iodir)
>> +{
>> + VXHSAIOCB *acb = NULL;
>> + BDRVVXHSState *s = bs->opaque;
>> + size_t size;
>> + uint64_t offset;
>> + int iio_flags = 0;
>> + int ret = 0;
>> + void *dev_handle = s->vdisk_hostinfo.dev_handle;
>> +
>> + offset = sector_num * BDRV_SECTOR_SIZE;
>> + size = nb_sectors * BDRV_SECTOR_SIZE;
>> + acb = qemu_aio_get(&vxhs_aiocb_info, bs, cb, opaque);
>> +
>> + /*
>> + * Initialize VXHSAIOCB.
>> + */
>> + acb->err = 0;
>> + acb->qiov = qiov;
>
> This field is unused, please remove it.
>
Yes! Thanks!
>> +static BlockDriver bdrv_vxhs = {
>> + .format_name = "vxhs",
>> + .protocol_name = "vxhs",
>> + .instance_size = sizeof(BDRVVXHSState),
>> + .bdrv_file_open = vxhs_open,
>> + .bdrv_parse_filename = vxhs_parse_filename,
>> + .bdrv_close = vxhs_close,
>> + .bdrv_getlength = vxhs_getlength,
>> + .bdrv_aio_readv = vxhs_aio_readv,
>> + .bdrv_aio_writev = vxhs_aio_writev,
>
> Missing .bdrv_aio_flush(). Does VxHS promise that every completed write
> request is persistent?
>
Yes, every acknowledged write request is persistent.
> In that case it may be better to disable the emulated disk write cache
> so the guest operating system and application know not to send flush
> commands.
We do pass "cache=none" on the qemu command line for every block
device. Are there any other code changes necessary? Any pointers will
help.
Thanks,
Ashish
