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

[Stefan Hajnoczi]

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.

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.