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

[Stefan Hajnoczi]

On Fri, Jan 31, 2014 at 10:11 AM, Peter Lieven <address@hidden> wrote:
> 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;
>     }

Yes.

> The use of error_setg in nfs_client_open is ok?

Yes, it's fine.

The Error API is not 100% obvious when you first see it, but once you
learn the conventions it's pretty usable:

Functions take an Error **errp argument.  This argument is optional,
so a caller that doesn't care about detailed error messages may pass
NULL.  This has implications...

error_setg(errp, fmt, ...) handles errp == NULL internally so you can
call it unconditionally.

The tricky thing is that error_is_set() only works if errp is non-NULL
(otherwise error_setg() skips creating an Error object).  So it means
you cannot rely on your errp argument and often functions will declare
Error *local_err = NULL, so they can pass &local_err to child
functions.  Finally, error_propagate(errp, local_err) is used to pass
out the Error object to our caller.

Hope this summary makes the Error API clear.

Stefan