Re: [Qemu-devel] [PATCH 02/10] block/json: Add JSON protocol driver

[Max Reitz <mreitz@redhat.com>]

On 05.03.2014 17:04, Benoît Canet wrote:
> The Monday 03 Mar 2014 à 16:28:45 (+0100), Max Reitz wrote :
>> Add a JSON protocol driver which allows supplying block driver options
>> through the filename rather than separately. Other than that, it is a
>> pure passthrough driver which identifies itself as a filter.
>>
>> This patch implements the functions bdrv_parse_filename(),
>> bdrv_file_open(), bdrv_close(), bdrv_aio_readv(), bdrv_aio_writev(),
>> bdrv_getlength(), bdrv_refresh_limits() and bdrv_get_info().
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/Makefile.objs |   2 +-
>>   block/json.c        | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 137 insertions(+), 1 deletion(-)
>>   create mode 100644 block/json.c
>>
>> diff --git a/block/Makefile.objs b/block/Makefile.objs
>> index fd88c03..d4b70f4 100644
>> --- a/block/Makefile.objs
>> +++ b/block/Makefile.objs
>> @@ -5,7 +5,7 @@ block-obj-y += qed-check.o
>>   block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o vhdx-log.o
>>   block-obj-$(CONFIG_QUORUM) += quorum.o
>>   block-obj-y += parallels.o blkdebug.o blkverify.o
>> -block-obj-y += snapshot.o qapi.o
>> +block-obj-y += snapshot.o qapi.o json.o
>>   block-obj-$(CONFIG_WIN32) += raw-win32.o win32-aio.o
>>   block-obj-$(CONFIG_POSIX) += raw-posix.o
>>   block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
>> diff --git a/block/json.c b/block/json.c
>> new file mode 100644
>> index 0000000..6d63cf6
>> --- /dev/null
>> +++ b/block/json.c
>> @@ -0,0 +1,136 @@
>> +/*
>> + * JSON filename wrapper protocol driver
>> + *
>> + * Copyright (c) 2014 Red Hat Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include "qemu-common.h"
>> +#include "block/block_int.h"
>> +#include "qapi/qmp/qdict.h"
>> +#include "qapi/qmp/qjson.h"
>> +
>> +static void json_parse_filename(const char *filename, QDict *options,
>> +                                Error **errp)
>> +{
>> +    QObject *file_options_obj;
>> +    QDict *full_options;
>> +
>> +    if (!strstart(filename, "json:", &filename)) {
>> +        error_setg(errp, "Unknown protocol prefix for JSON block driver");
>> +        return;
>> +    }
>> +
>> +    file_options_obj = qobject_from_json(filename);
>> +    if (!file_options_obj) {
>> +        error_setg(errp, "Could not parse the JSON options");
>> +        return;
>> +    }
>> +
>> +    if (qobject_type(file_options_obj) != QTYPE_QDICT) {
>> +        qobject_decref(file_options_obj);
>> +        error_setg(errp, "Invalid JSON object");
>> +        return;
>> +    }
>> +
>> +    full_options = qdict_new();
>> +    qdict_put_obj(full_options, "x-options", file_options_obj);
>> +    qdict_flatten(full_options);
>> +
>> +    qdict_join(options, full_options, true);
>> +    assert(qdict_size(full_options) == 0);
>> +    QDECREF(full_options);
>> +}
>> +
>> +static int json_open(BlockDriverState *bs, QDict *options, int flags,
>> +                     Error **errp)
>> +{
>> +    int ret;
>> +
>> +    assert(bs->file == NULL);
>> +    ret = bdrv_open_image(&bs->file, NULL, options, "x-options", flags, false,
>> +                          errp);
>
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +
>> +    return 0;
> Why not ?
>      return ret;
> }
>
> Do you plan to enrich this code path later ?

You're right, that'd be shorter. It's just that I'm so in the habit of 
writing "ret = foo(); if (ret < 0) { return ret; / goto fail; }" that I 
didn't notice. ;-)

>> +}
>> +
>> +static void json_close(BlockDriverState *bs)
>> +{
>> +}
>> +
>> +static BlockDriverAIOCB *json_aio_readv(BlockDriverState *bs,
>> +                                        int64_t sector_num, QEMUIOVector *qiov,
>> +                                        int nb_sectors,
>> +                                        BlockDriverCompletionFunc *cb,
>> +                                        void *opaque)
>> +{
>> +    return bdrv_aio_readv(bs->file, sector_num, qiov, nb_sectors, cb, opaque);
>> +}
>> +
>> +static BlockDriverAIOCB *json_aio_writev(BlockDriverState *bs,
>> +                                         int64_t sector_num, QEMUIOVector *qiov,
>> +                                         int nb_sectors,
>> +                                         BlockDriverCompletionFunc *cb,
>> +                                         void *opaque)
>> +{
>> +    return bdrv_aio_writev(bs->file, sector_num, qiov, nb_sectors, cb, opaque);
>> +}
>> +
>> +static int64_t json_getlength(BlockDriverState *bs)
>> +{
>> +    return bdrv_getlength(bs->file);
>> +}
>> +
>> +static int json_refresh_limits(BlockDriverState *bs)
>> +{
>> +    bs->bl = bs->file->bl;
>> +    return 0;
>> +}
>> +
>> +static int json_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>> +{
>> +    return bdrv_get_info(bs->file, bdi);
>> +}
>> +
>> +static BlockDriver bdrv_json = {
>> +    .format_name                = "json",
>> +    .protocol_name              = "json",
>> +    .instance_size              = 0,
>> +
>> +    .bdrv_parse_filename        = json_parse_filename,
>> +    .bdrv_file_open             = json_open,
>> +    .bdrv_close                 = json_close,
>> +
>> +    .bdrv_aio_readv             = json_aio_readv,
>> +    .bdrv_aio_writev            = json_aio_writev,
>> +
>> +    .has_variable_length        = true,
>> +    .bdrv_getlength             = json_getlength,
>> +
>> +    .bdrv_refresh_limits        = json_refresh_limits,
>> +    .bdrv_get_info              = json_get_info,
>> +
>> +    .authorizations             = { true, true },
> Paolo make rewrite the snapshot authorization code.
> In the new version you need
>      .is_filter = true;
>      .bdrv_recurse_is_first_children = your_implementation;
>
> I don't know when these patchset will be reviewed and merged so maybe this
> version is correct.

I'll just rebase on top of Paolo's series then.


Thanks,

Max

>> +};
>> +
>> +static void bdrv_json_init(void)
>> +{
>> +    bdrv_register(&bdrv_json);
>> +}
>> +
>> +block_init(bdrv_json_init);
>> -- 
>> 1.9.0
>>
>>