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

[Peter Lieven <pl@kamp.de>]

On 31.01.2014 11:46, Stefan Hajnoczi wrote:
> On Fri, Jan 31, 2014 at 10:11 AM, Peter Lieven <pl@kamp.de> wrote:
>> On 31.01.2014 09:57, Stefan Hajnoczi wrote:
>>> On Thu, Jan 30, 2014 at 10:35 PM, Peter Lieven <pl@kamp.de> wrote:
>>>> Am 30.01.2014 um 15:22 schrieb Stefan Hajnoczi <stefanha@gmail.com>:
>>>>
>>>>> 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
Thanks for the explanation. I will sent you an updated series shortly.

Peter


-- 

Mit freundlichen Grüßen

Peter Lieven

...........................................................

   KAMP Netzwerkdienste GmbH
   Vestische Str. 89-91 | 46117 Oberhausen
   Tel: +49 (0) 208.89 402-50 | Fax: +49 (0) 208.89 402-40
   pl@kamp.de | http://www.kamp.de

   Geschäftsführer: Heiner Lante | Michael Lante
   Amtsgericht Duisburg | HRB Nr. 12154
   USt-Id-Nr.: DE 120607556

...........................................................