Re: [PATCH 3/4] Bootstrappable pci-arbiter

[Samuel Thibault]

Damien Zammit, le mar. 02 mars 2021 22:41:36 +1100, a ecrit:
> +static io_return_t
> +pci_device_open (mach_port_t reply_port, mach_msg_type_name_t 
> reply_port_type,
> +                 dev_mode_t mode, char *name, device_t * devp,
> +                 mach_msg_type_name_t * devicePoly)
> +{
> +  io_return_t err = D_SUCCESS;
> +  mach_port_t dev_master, root;
> +
> +  if (strncmp(name, "pci", 3))
> +    err = D_NO_SUCH_DEVICE;
> +
> +  /* Fall back to opening kernel device master */
> +  if (err)
> +    {
> +      get_privileged_ports(NULL, &dev_master);
> +      err = device_open (dev_master, mode, name, devp);
> +      *devicePoly = MACH_MSG_TYPE_MOVE_SEND;
> +      return D_SUCCESS;

This needs to propagate any error.

> diff --git a/pci-arbiter/netfs_impl.c b/pci-arbiter/netfs_impl.c
> index b987a0bc..45059195 100644
> --- a/pci-arbiter/netfs_impl.c
> +++ b/pci-arbiter/netfs_impl.c
> @@ -59,25 +59,17 @@ get_dirents (struct pcifs_dirent *dir,
>    int i, count;
>    size_t size;
>    char *p;
> +  int nentries = (int)dir->dir->num_entries;
>  
> -  if (first_entry >= dir->dir->num_entries)
> +  if (first_entry >= nentries)
>      {
>        *data_len = 0;
>        *data_entries = 0;
>        return 0;
>      }
>  
> -  if (max_entries < 0)
> -    count = dir->dir->num_entries;
> -  else
> -    {
> -      count = ((first_entry + max_entries) >= dir->dir->num_entries ?
> -            dir->dir->num_entries : max_entries) - first_entry;
> -    }
> -
> -  size =
> -    (count * DIRENTS_CHUNK_SIZE) >
> -    max_data_len ? max_data_len : count * DIRENTS_CHUNK_SIZE;
> +  count = nentries - first_entry;
> +  size = count * DIRENTS_CHUNK_SIZE;

Even if we allocate a new buffer, I believe it's better to still respect
max_entries, otherwise some code could be surprised by the spurious
data.

> @@ -257,6 +251,21 @@ netfs_attempt_lookup (struct iouser * user, struct node 
> * dir,
>  {
>    error_t err = 0;
>    struct pcifs_dirent *entry;
> +  char *last = name;
> +
> +  /* Strip trailing slash */
> +  if (*last)
> +    {
> +      while (*last)
> +        last++;
> +      last--;
> +      if (*last == '/')
> +        *last = '\0';
> +    }

Note that you may have several slashes.
Also, note that a trailing '/' means we are supposed to make sure to be
opening a directory. So we don't actually want to just ignore it, but
really check it, like the O_DIRECTORY open flag.

> +    /* Skip leading dot slash */
> +    if (*name == '.' && *(name+1) == '/')
> +      name += 2;

This should be a while.

> diff --git a/pci-arbiter/startup-ops.c b/pci-arbiter/startup-ops.c
> index eb387fd9..fc285572 100644
> --- a/pci-arbiter/startup-ops.c
> +++ b/pci-arbiter/startup-ops.c
> @@ -25,6 +25,7 @@
>  
>  #include "startup.h"
>  
> +#if 0 // FIXME: this conflicts with trivfs for machdev
>  /* The system is going down. Call netfs_shutdown() */
>  error_t
>  S_startup_dosync (mach_port_t handle)

We could make libmachdev expose a libmachdev_dosync function, that it
would call on S_startup_dosync, and that this S_startup_dosync would
also call.

Samuel