Re: [PATCH 2/3] Split off sysfs id retrieval

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Dec 15, 2009 at 07:30:27PM +0100, Alexander Graf wrote:
> To retreive device and vendor ID from a PCI device, we need to read a
> sysfs file. That code is currently hand written at least two times,
> the later patch introducing two more calls.
> 
> So let's move that out to a function.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>

Shouldn't we be using libpci, as we already do
in other places?

> ---
>  hw/device-assignment.c |   66 ++++++++++++++++++++++++++++++++----------------
>  1 files changed, 44 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index 000fa61..566671c 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -562,14 +562,46 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
>      return 0;
>  }
>  
> +static int get_real_id(const char *devpath, const char *idname, uint16_t *val)
> +{
> +    FILE *f;
> +    char name[128];
> +    long id;
> +
> +    snprintf(name, sizeof(name), "%s%s", devpath, idname);
> +    f = fopen(name, "r");
> +    if (f == NULL) {
> +        fprintf(stderr, "%s: %s: %m\n", __func__, name);
> +        return -1;
> +    }
> +    if (fscanf(f, "%li\n", &id) == 1) {
> +        *val = id;
> +    } else {
> +        return -1;
> +    }
> +    fclose(f);
> +
> +    return 0;
> +}
> +
> +static int get_real_vendor_id(const char *devpath, uint16_t *val)
> +{
> +    return get_real_id(devpath, "vendor", val);
> +}
> +
> +static int get_real_device_id(const char *devpath, uint16_t *val)
> +{
> +    return get_real_id(devpath, "device", val);
> +}
> +
>  static int get_real_device(AssignedDevice *pci_dev, uint8_t r_bus,
>                             uint8_t r_dev, uint8_t r_func)
>  {
>      char dir[128], name[128];
> -    int fd, r = 0;
> +    int fd, r = 0, v;
>      FILE *f;
>      unsigned long long start, end, size, flags;
> -    unsigned long id;
> +    uint16_t id;
>      struct stat statbuf;
>      PCIRegion *rp;
>      PCIDevRegions *dev = &pci_dev->real_device;
> @@ -635,31 +667,21 @@ again:
>  
>      fclose(f);
>  
> -    /* read and fill device ID */
> -    snprintf(name, sizeof(name), "%svendor", dir);
> -    f = fopen(name, "r");
> -    if (f == NULL) {
> -        fprintf(stderr, "%s: %s: %m\n", __func__, name);
> +    /* read and fill vendor ID */
> +    v = get_real_vendor_id(dir, &id);
> +    if (v) {
>          return 1;
>      }
> -    if (fscanf(f, "%li\n", &id) == 1) {
> -	pci_dev->dev.config[0] = id & 0xff;
> -	pci_dev->dev.config[1] = (id & 0xff00) >> 8;
> -    }
> -    fclose(f);
> +    pci_dev->dev.config[0] = id & 0xff;
> +    pci_dev->dev.config[1] = (id & 0xff00) >> 8;
>  
> -    /* read and fill vendor ID */
> -    snprintf(name, sizeof(name), "%sdevice", dir);
> -    f = fopen(name, "r");
> -    if (f == NULL) {
> -        fprintf(stderr, "%s: %s: %m\n", __func__, name);
> +    /* read and fill device ID */
> +    v = get_real_device_id(dir, &id);
> +    if (v) {
>          return 1;
>      }
> -    if (fscanf(f, "%li\n", &id) == 1) {
> -	pci_dev->dev.config[2] = id & 0xff;
> -	pci_dev->dev.config[3] = (id & 0xff00) >> 8;
> -    }
> -    fclose(f);
> +    pci_dev->dev.config[2] = id & 0xff;
> +    pci_dev->dev.config[3] = (id & 0xff00) >> 8;
>  
>      /* dealing with virtual function device */
>      snprintf(name, sizeof(name), "%sphysfn/", dir);
> -- 
> 1.6.0.2