[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 10/10] Replace sscanf in PCI device address parsing



On 03/30/2010 10:20 AM, Matthias Bolte wrote:
> This also fixes a problem with MinGW's GCC on Windows.
> GCC complained bout the L modifier being unknown.

s/bout/about/

> ---
>  src/util/pci.c |   59 +++++++++++++++++++++++++++++++++++++++++++------------
>  1 files changed, 46 insertions(+), 13 deletions(-)
> 
> diff --git a/src/util/pci.c b/src/util/pci.c
> index 99ec22a..6aa2fe0 100644
> --- a/src/util/pci.c
> +++ b/src/util/pci.c
> @@ -282,15 +282,26 @@ pciIterDevices(pciIterPredicate predicate,
>      }
>  
>      while ((entry = readdir(dir))) {
> -        unsigned domain, bus, slot, function;
> +        unsigned int domain, bus, slot, function;
>          pciDevice *check;
> +        char *tmp;
>  
>          /* Ignore '.' and '..' */
>          if (entry->d_name[0] == '.')
>              continue;
>  
> -        if (sscanf(entry->d_name, "%x:%x:%x.%x",
> -                   &domain, &bus, &slot, &function) < 4) {
> +        /* expected format: <domain>:<bus>:<slot>.<function> */
> +        if (/* domain */
> +            virStrToLong_ui(entry->d_name, &tmp, 16, &domain) < 0 ||
> +            tmp == NULL || *tmp != ':' ||

Useless checks for NULL throughout.

> +            /* bus */
> +            virStrToLong_ui(tmp + 1, &tmp, 16, &bus) < 0 ||
> +            tmp == NULL || *tmp != ':' ||
> +            /* slot */
> +            virStrToLong_ui(tmp + 1, &tmp, 16, &slot) < 0 ||
> +            tmp == NULL || *tmp != '.' ||
> +            /* function */
> +            virStrToLong_ui(tmp + 1, NULL, 16, &function) < 0) {

Another trailing garbage enforcement worth documenting.

>              VIR_WARN("Unusual entry in " PCI_SYSFS "devices: %s", entry->d_name);
>              continue;
>          }
> @@ -914,10 +925,9 @@ pciWaitForDeviceCleanup(pciDevice *dev, const char *matcher)
>      FILE *fp;
>      char line[160];
>      unsigned long long start, end;
> -    int consumed;
>      char *rest;
> -    unsigned long long domain;
> -    int bus, slot, function;
> +    char *tmp;
> +    unsigned int domain, bus, slot, function;

Why the change in the size of domain?

>      int in_matching_device;
>      int ret;
>      size_t match_depth;
> @@ -945,10 +955,18 @@ pciWaitForDeviceCleanup(pciDevice *dev, const char *matcher)
>           * of these situations
>           */
>          if (in_matching_device && (strspn(line, " ") == (match_depth + 2))) {
> -            if (sscanf(line, "%Lx-%Lx : %n", &start, &end, &consumed) != 2)
> +            tmp = line + strspn(line, " ");
> +
> +            /* expected format: <start>-<end> : <suffix> */
> +            if (/* start */
> +                virStrToLong_ull(tmp, &tmp, 16, &start) < 0 ||
> +                tmp == NULL || *tmp != '-' ||
> +                /* end */
> +                virStrToLong_ull(tmp + 1, &tmp, 16, &end) < 0 ||
> +                tmp == NULL || ! STRPREFIX(tmp, " : "))
>                  continue;
>  
> -            rest = line + consumed;
> +            rest = tmp + 3; /* = strlen(" : ") */

Hmm, thinking aloud here: your patch series has introduced several
instances of checking STRPREFIX, then advancing to the end of the
prefix.  Maybe it's time to introduce a helper macro that does both the
check and advances the pointer to the end of the match?  Something like:

#define STPCMP(ptr, cmp) \
  (STRPREFIX (*(ptr), cmp) ? (ptr) += strlen (cmp) : NULL)

STPCMP(&tmp, " : ");
if (tmp == NULL)
    error...
else
    use tmp...

-- 
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]