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

[Peter Lieven]

On 31.01.2014 09:57, Stefan Hajnoczi wrote:
> On Thu, Jan 30, 2014 at 10:35 PM, Peter Lieven <address@hidden> wrote:
> > 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 :
> > > > > +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.
> The issue is that nfs_file_open() takes an Error **errp argument.
> This means the function should report detailed errors using the Error
> object.
>
> The patch prints and then discards the local_error instead of
> propagating it to the caller's errp.
>
> We should just propagate the error instead of printing it:
> if (error_is_set(&local_err)) {
>      error_propagate(errp, local_err);
>      goto ...;

Ok, you are just referring to this part in nfs_file_open:

    if (error_is_set(&local_err)) {
        qerror_report_err(local_err);
        error_free(local_err);
        return -EINVAL;
    }

which I would change to:

    if (error_is_set(&local_err)) {
        error_propagate(errp, local_err);
        return -EINVAL;
    }

The use of error_setg in nfs_client_open is ok?

Peter