Re: [Qemu-devel] [Xen-devel] [PATCH V3 02/10] Introduce HostPCIDevice to access a pci device on the host.

[Anthony PERARD]

On Fri, Nov 4, 2011 at 17:49, Konrad Rzeszutek Wilk
<address@hidden> wrote:
>> +static unsigned long get_value(HostPCIDevice *d, const char *name)
>> +{
>> +    char path[PATH_MAX];
>> +    FILE *f;
>> +    unsigned long value;
>> +
>> +    path_to(d, name, path, sizeof (path));
>> +    f = fopen(path, "r");
>> +    if (!f) {
>> +        fprintf(stderr, "Error: Can't open %s: %s\n", path, 
>> strerror(errno));
>> +        return -1;
>
> So the decleration is 'unsigned long' but you return -1 here.
>
> Should the decleration be 'signed long' ?
>
> Or perhaps return the value as parameter and return zero for success
> and <= 0 for failure?

I will use an extra parameter for the value, and return the
success/failure. And check for error.

>> +    }
>> +    if (fscanf(f, "%lx\n", &value) != 1) {
>> +        fprintf(stderr, "Error: Syntax error in %s\n", path);
>> +        value = -1;
>> +    }
>> +    fclose(f);
>> +    return value;
>> +}
>> +
>> +static int pci_dev_is_virtfn(HostPCIDevice *d)
>> +{
>> +    int rc;
>> +    char path[PATH_MAX];
>> +    struct stat buf;
>> +
>> +    path_to(d, "physfn", path, sizeof (path));
>> +    rc = !stat(path, &buf);
>> +
>> +    return rc;
>
> Seems like this could be a 'bool'?

Yes, and the result is store in a bool :-(, so I will just change the
return type of this function.

>> +}
>> +
>> +static int host_pci_config_fd(HostPCIDevice *d)
>> +{
>> +    char path[PATH_MAX];
>> +
>> +    if (d->config_fd < 0) {
>> +        path_to(d, "config", path, sizeof (path));
>> +        d->config_fd = open(path, O_RDWR);
>> +        if (d->config_fd < 0) {
>> +            fprintf(stderr, "HostPCIDevice: Can not open '%s': %s\n",
>> +                    path, strerror(errno));
>> +        }
>> +    }
>> +    return d->config_fd;
>> +}
>> +static int host_pci_config_read(HostPCIDevice *d, int pos, void *buf, int 
>> len)
>> +{
>> +    int fd = host_pci_config_fd(d);
>> +    int res = 0;
>> +
>> +    res = pread(fd, buf, len, pos);
>> +    if (res < 0) {
>> +        fprintf(stderr, "host_pci_config: read failed: %s (fd: %i)\n",
>> +                strerror(errno), fd);
>> +        return -1;
>> +    }
>> +    return res;
>> +}
>> +static int host_pci_config_write(HostPCIDevice *d,
>> +                                 int pos, const void *buf, int len)
>> +{
>> +    int fd = host_pci_config_fd(d);
>> +    int res = 0;
>> +
>> +    res = pwrite(fd, buf, len, pos);
>> +    if (res < 0) {
>> +        fprintf(stderr, "host_pci_config: write failed: %s\n",
>> +                strerror(errno));
>> +        return -1;
>> +    }
>> +    return res;
>> +}
>> +
>> +uint8_t host_pci_get_byte(HostPCIDevice *d, int pos)
>> +{
>> +  uint8_t buf;
>> +  host_pci_config_read(d, pos, &buf, 1);
>
> Not checking the return value?
>> +  return buf;
>> +}
>> +uint16_t host_pci_get_word(HostPCIDevice *d, int pos)
>> +{
>> +  uint16_t buf;
>> +  host_pci_config_read(d, pos, &buf, 2);
>
> Here as well?
>> +  return le16_to_cpu(buf);
>
> So if we can't read those buffers, won't that mean we end up with
> garbage in buf? As we haven't actually written anything to it?
>
> Perhaps we should do:
>
>  if (host_pci..() < 0)
>        return 0;
>  ... normal case?

Yes, I should probably check for error. and check if pread has
actually read the size we expect.

>> +}
>> +uint32_t host_pci_get_long(HostPCIDevice *d, int pos)
>> +{
>> +  uint32_t buf;
>> +  host_pci_config_read(d, pos, &buf, 4);
>> +  return le32_to_cpu(buf);
>> +}
>> +int host_pci_get_block(HostPCIDevice *d, int pos, uint8_t *buf, int len)
>> +{
>> +  return host_pci_config_read(d, pos, buf, len);
>
> Oh, so that is called.. Hm, not much chance of returning an error there is.

Well, errors are already check by _config_read, so this function is
just an alias.

> Can we propage the errors in case there is some fundamental failure
> when reading/writting the data stream? Say the PCI device gets
> unplugged by the user.. won't pread return -EXIO?

I could introduce another parameter, a pointer to a buffer were to
right the value, and return only success/faillure. It's should also be
a faillure if pread read less bytes then the ask size, I will fix that
as well.

And with this extra parameter, it's should be better than return 0 as
a read value.


Thanks,

-- 
Anthony PERARD