From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Michael S. Tsirkin" <mst@redhat.com>
Subject: Re: [PATCH 2/3] Split off sysfs id retrieval
Date: Tue, 15 Dec 2009 20:39:40 +0200
Message-ID: <20091215183940.GB25724@redhat.com>
References: <1260901828-5145-1-git-send-email-agraf@suse.de> <1260901828-5145-3-git-send-email-agraf@suse.de>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: kvm list <kvm@vger.kernel.org>, Gleb Natapov <gleb@redhat.com>,
	Muli Ben-Yehuda <muli@il.ibm.com>,
	Chris Wright <chrisw@redhat.com>
To: Alexander Graf <agraf@suse.de>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:11162 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1750746AbZLOSm1 (ORCPT <rfc822;kvm@vger.kernel.org>);
	Tue, 15 Dec 2009 13:42:27 -0500
Content-Disposition: inline
In-Reply-To: <1260901828-5145-3-git-send-email-agraf@suse.de>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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