On Fri, Nov 4, 2011 at 17:49, Konrad Rzeszutek Wilk
<konrad.w...@oracle.com> 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
