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 <ler...@redhat.com> > --- > 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 <ebl...@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
