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

[Eric Blake]

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.

> @@ -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?

>          goto fail;
>      }
>  
>      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?

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>



-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature