Re: [Qemu-devel] [PATCH v5 1/2] block/nfs: Introduce runtime_opts in NFS

[Kevin Wolf <kwolf@redhat.com>]

Am 28.10.2016 um 19:09 hat Ashijeet Acharya geschrieben:
> Make NFS block driver use various fine grained runtime_opts.
> Set .bdrv_parse_filename() to nfs_parse_filename() and introduce two
> new functions nfs_parse_filename() and nfs_parse_uri() to help parsing
> the URI.
> Add a new option "server" which then accepts a new struct NFSServer.
> "host" is supported as a legacy option and is mapped to its NFSServer
> representation.

This sentence isn't up to date any more and should be removed.

> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> ---
>  block/nfs.c | 430 +++++++++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 337 insertions(+), 93 deletions(-)

> +static void nfs_refresh_filename(BlockDriverState *bs, QDict *options)
> +{
> +    NFSClient *client = bs->opaque;
> +    QDict *opts = qdict_new();
> +    QObject *server_qdict;
> +    Visitor *ov;
> +
> +    qdict_put(opts, "driver", qstring_from_str("nfs"));
> +
> +    snprintf(bs->exact_filename, sizeof(bs->exact_filename),
> +             "nfs://%s/%s?uid=%" PRId64 "&gid=%" PRId64 "&tcp-syncnt=%" PRId64
> +             "&readahead=%" PRId64 "&pagecache=%" PRId64 "&debug=%" PRId64,
> +             client->server->host, client->path, client->uid, client->gid,
> +             client->tcp_syncnt, client->readahead, client->pagecache,
> +             client->debug);

bs->exact_filename should contain only the parameters that are actually
necessary to identify the image, but not general runtime options. This
is what the comment for bdrv_refresh_filename() says:

 *  - exact_filename: A filename which may be used for opening a block device
 *                    which (mostly) equals the given BDS (even without any
 *                    other options; so reading and writing must return the same
 *                    results, but caching etc. may be different)

So I believe you can remove most of the query parameters here.

'uid' and 'gid' aren't completely clear, they could possibly be argued
to be part of identifying the image, but we should still omit them if
the defaults were used.

More importantly, the path you're returning doesn't include the
filename! Also, the double slash between host and path doesn't look
perfect:

    $ ./qemu-img info nfs://localhost/home/kwolf/images/hd.img
    image: nfs://localhost//home/kwolf/images?uid=0&gid=0&tcp-syncnt=0&readahead=0&pagecache=0&debug=0
    file format: raw
    virtual size: 64M (67108864 bytes)
    disk size: 64M

> +    ov = qobject_output_visitor_new(&server_qdict);

The visitor is leaked, you're missing a visit_free().

> +    visit_type_NFSServer(ov, NULL, &client->server, &error_abort);
> +    visit_complete(ov, &client->server);
> +    assert(qobject_type(server_qdict) == QTYPE_QDICT);

This even fails an assertion for me, I got the above result only after
fixing this:

    $ ./qemu-img info nfs://localhost/home/kwolf/images/hd.img
    qemu-img: qapi/qobject-output-visitor.c:206: qobject_output_complete: Assertion `opaque == qov->result' failed.
    Abgebrochen (Speicherabzug geschrieben)

The reason is that visit_complete() should get &server_qdict rather than
&client->server.

Kevin