Re: [PATCH libpciaccess] hurd: device_open the pre-existing pci server

[Samuel Thibault]

Hello,

Damien Zammit, le sam. 06 mars 2021 15:00:00 +1100, a ecrit:
> +static int
> +sort_devices(void)
> +{
> +    int bus, dev, func;
> +    struct pci_device_private *sorted;
> +    struct pci_device_private *device;
> +    struct pci_device_private *tmpdev;
> +
> +    sorted = calloc(pci_sys->num_devices, sizeof(struct pci_device_private));
> +    if (!sorted) {
> +        return ENOMEM;
> +    }
> +
> +    tmpdev = sorted;
> +
> +    for (bus = 0; bus < 256; bus++) {
> +        for (dev = 0; dev < 32; dev++) {
> +            for (func = 0; func < 8; func++) {
> +                device = (struct pci_device_private *)
> +                         pci_device_find_by_slot(0, bus, dev, func);
> +                if (device) {
> +                    *tmpdev = *device;
> +                    tmpdev++;
> +                }
> +            }
> +        }
> +    }
> +    if (pci_sys->devices) {
> +        free(pci_sys->devices);
> +        pci_sys->devices = sorted;
> +    }
> +    return 0;
> +}

That probably needs to be discussed with upgrade?

> +simple_readdir(mach_port_t port, uint32_t *first_entry)
> +{
> +    struct dirent *e;
> +    char *data;
> +    int nentries = 0;
> +    vm_size_t size;
> +
> +    dir_readdir (port, &data, &size, *first_entry, 1, 0, &nentries);
> +
> +    if (nentries == 0) {
> +     return NULL;
> +    }
> +
> +    *first_entry = *first_entry + 1;
> +    e = (struct dirent *)(data+4);
> +    return e;
> +}

What is this +4 for?
Is it compiled with -D_FILE_OFFSET_BITS=64? (the __dir_readdir RPC
returns a struct dirent64, not struct dirent). You can build it with
-D_LARGEFILE64_SOURCE to get access to struct dirent64 without changing
all the ABI of off_t, ino_t, etc.

> +    if (pci_port == MACH_PORT_NULL) {
> +        return EGRATUITOUS;
> +    } else {

You don't need an "else" here, the pci_port == MACH_PORT_NULL can be
kept as a two-line quick-exit, and let the rest of the code not inside a
block.

> @@ -370,34 +394,27 @@ enum_devices(const char *parent, int domain,
> -            device_port = file_name_lookup(server, 0, 0);
> +            device_port = file_name_lookup_under(pci_port, server, O_RDWR, 
> 0);
>              if (device_port == MACH_PORT_NULL) {
> -                closedir(dir);
> -                return errno;
> +                return 0;
>              }
>  
>              toread = sizeof(reg);
>              err = pciclient_cfg_read(device_port, PCI_VENDOR_ID, (char*)&reg,
>                                       &toread);
>              if (err) {
> -                if (closedir(dir) < 0)
> -                    return errno;
>                  return err;
>              }

Realizing here: don't we need to deallocate the device_port port?

> @@ -513,12 +521,24 @@ pci_system_hurd_create(void)
>      pci_sys->methods = &hurd_pci_methods;
>  
>      pci_sys->num_devices = 0;
> -    err = enum_devices(_SERVERS_BUS_PCI, -1, -1, -1, -1, LEVEL_DOMAIN);
> +
> +    if (! (err = get_privileged_ports (NULL, &device_master)) && 
> (device_master != MACH_PORT_NULL)) {
> +        err = device_open (device_master, D_READ|D_WRITE, "pci", 
> &pci_server_port);
> +        if (err) {
> +            pci_system_cleanup();
> +            return err;
> +        }

Please also add a failback path that calls
file_name_lookup("/servers/bus/pci"), so that the user can interpose
the PCI world in the FS without having to interposing the device master
port.

> +        root = file_name_lookup_under (pci_server_port, ".", O_DIRECTORY | 
> O_RDWR | O_EXEC, 0);
> +        if (!root) {
> +            pci_system_cleanup();
> +            return errno;
> +        }
> +        err = enum_devices(pci_server_port, root, ".", -1, -1, -1, -1, 
> LEVEL_DOMAIN);

This part also doesn't need to be kept in an "else" part, it is the
normal codepath.

Samuel