Re: [Qemu-devel] [PATCHv7 1/5] block: add native support for NFS

[Peter Lieven]

Am 30.01.2014 um 15:22 schrieb Stefan Hajnoczi <address@hidden>:

> On Wed, Jan 29, 2014 at 05:19:59PM +0100, Benoît Canet wrote:
>> Le Wednesday 29 Jan 2014 à 09:50:21 (+0100), Peter Lieven a écrit :
> 
> Hi Peter,
> If I read your reply to Benoit correctly, you only addressed the
> questions about nfs_client_close().  Here are Benoits other comments and
> my thoughts on them:
> 
>>> +static void
>>> +nfs_co_generic_cb(int status, struct nfs_context *nfs, void *data,
>>> +                  void *private_data)
>>> +{
>>> +    NFSRPC *task = private_data;
>>> +    task->complete = 1;
>>> +    task->status = status;
>>> +    if (task->status > 0 && task->iov) {
>>> +        if (task->status <= task->iov->size) {
>> It feel very odd to compare something named status with something named size.
> 
> Maybe the argument name isn't ideal.  Something like 'ret' or 'result'
> would be more commonplace.

I took the "status" from libnfs examples. I can change it in the qemu code if
you like.

> 
> Not critical but would be nice to change.
> 
>>> +static int nfs_file_open(BlockDriverState *bs, QDict *options, int flags,
>>> +                         Error **errp) {
>>> +    NFSClient *client = bs->opaque;
>>> +    int64_t ret;
>>> +    QemuOpts *opts;
>>> +    Error *local_err = NULL;
>>> +
>>> +    opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
>>> +    qemu_opts_absorb_qdict(opts, options, &local_err);
>>> +    if (error_is_set(&local_err)) {
>>> +        qerror_report_err(local_err);
>> I have seen more usage of error_propagate(errp, local_err); in QEMU code.
>> Maybe I am missing the point.
> 
> Yes, I think you are right.  The Error should be propagated to the
> caller.  It's not clear to me whether we can ever get an error from
> qemu_opts_absorb_qdict() in this call site though.

Is there any action I should take here? If yes, can you advise what
to do please.

Peter