Re: [Qemu-devel] [PATCH 05/16] pci-assign: propagate errors from get_real_id()

[Laszlo Ersek]

On 04/25/14 18:05, Eric Blake wrote:
> On 04/10/2014 02:24 AM, Laszlo Ersek wrote:
>> get_real_id() has two thin wrappers (and no other callers),
>> get_real_vendor_id() and get_real_device_id(); it's easiest to convert
>> them in one fell swoop.
>>
>> Signed-off-by: Laszlo Ersek <address@hidden>
>> ---
>>  hw/i386/kvm/pci-assign.c | 45 +++++++++++++++++++++++++++------------------
>>  1 file changed, 27 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
> 
>> +static void get_real_id(const char *devpath, const char *idname, uint16_t 
>> *val,
>> +                        Error **errp)
>>  {
>>      FILE *f;
>>      char name[128];
>>      long id;
>>  
>>      snprintf(name, sizeof(name), "%s%s", devpath, idname);
>>      f = fopen(name, "r");
>>      if (f == NULL) {
>> -        error_report("%s: %s: %m", __func__, name);
>> -        return -1;
>> +        error_setg_file_open(errp, errno, name);
>> +        return;
>>      }
>>      if (fscanf(f, "%li\n", &id) == 1) {
>>          *val = id;
> 
> Latent bug (does not affect review of this patch): fscanf(%i) triggers
> undefined behavior on overflow.  Although qemu already has a number of
> violators, I've been advocating that people considering cleanups to use
> strtol() and friends when parsing integers (preferably sane wrappers, as
> strtol() is also a bear to use correctly), to avoid undefined behavior
> of *scanf.

You are right of course. Perhaps this case is a milder offender because
we're parsing sysfs files here, which we could consider a trusted /
stable interface. I think the fopen() check is justified (maybe sysfs is
not mounted), but once the file is available, then maybe we can trust
its contents.

(I'm saying this without having written the original code of course. I'm
not trying to prove the code right -- again, you are right -- just
trying to reinvent the original author's thought process. You could
correctly argue that if we expect sysfs not to be mounted, then
something else mounted in its usual place, with random contents, should
be plausible too.)

> 
>> @@ -759,12 +764,16 @@ static char *assign_failed_examine(const 
>> AssignedDevice *dev)
>>          goto fail;
>>      }
>>  
>>      ns++;
>>  
>> -    if (get_real_vendor_id(dir, &vendor_id) ||
>> -        get_real_device_id(dir, &device_id)) {
>> +    if ((get_real_vendor_id(dir, &vendor_id, &local_err), local_err) ||
>> +        (get_real_device_id(dir, &device_id, &local_err), local_err)) {
>> +        /* We're already analyzing an assignment error, so we suppress this
>> +         * one just like the others above.
>> +         */
>> +        error_free(local_err);
> 
> This feels idiomatically wrong to me to rely on a comma operator only
> ignore a failure.  Why not just:
> 
> get_real_vendor_id(dir, &vendor_id, NULL);
> get_real_device_id(dir, &device_id, NULL);
> 
> and skip the use of local_err?

We're suppressing the error only in the sense that we don't print it or
format it / report it otherwise. (Just like we don't specifically report
any readlink() or strrchr() failures above, which are not visible in the
context here.)

We do handle the error by jumping to the "fail" label; this goto below
is unchanged context:

> 
>>          goto fail;
>>      }

Without using local_err it's not possible to tell if get_real_*_id()
fails or not (they both return void).

>>  
>>      return g_strdup_printf(
>>          "*** The driver '%s' is occupying your device %04x:%02x:%02x.%x.\n"
> 
> Or is the problem that you are trying to prevent this g_strdup_printf
> from using uninitialized vendor_id and device_id if either lookup failed?

Yes. Not so much for avoiding undefined behavior (accessing auto objects
with indeterminate values), although that's a valid direct reason too,
but more because (according to the preexistent function logic) if one
these functions fails, we want to report "couldn't find out why", under
the "fail" label (not visible in the context), rather than saying
"driver foobar is occupying your device -- which we can't identify BTW".

> 
> But as for THIS commit, you have done an accurate transformation with no
> change in semantic behavior.  Therefore, I'm fine leaving it as-is and
> adding:
> 
> Reviewed-by: Eric Blake <address@hidden>

Thank you. That was indeed my purpose, to keep the semantics intact.

Laszlo