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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel