Re: [PATCH] Uncouple netfs_make_protid (netfs_make_peropen()) function calls in Hurd.

[Samuel Thibault]

Hello,

Svante Signell, le jeu. 27 janv. 2022 23:07:34 +0100, a ecrit:
> Attached are two patches, hopefully useful in decoupling the nested function
> calls in libnetfs: netfs_make_protid (netfs_make_peropen()). Hopefully, the
> patches is useful to enhance error tracking.

AIUI they also fix some memory leaks on errors, don't they? That should
be mentioned in the git log too, that's a very useful improvement! :)

Note that the first line of the git log should be really small,
typically 60-80 chars max.

> Due to indentation differences they are larger than they would be if
> not.

That's not necessarily a problem since one can use -w options to ignore
the indentation changes. That being said, you'll probably want to avoid
them for another reason: it's better to avoid piling up indentation.

> The attached patches could be broken down to a series of patches, one for each
> C-file. However, the function interface for netfs_make_peropen() and
> netfs_make_protid() is preferably the same as the corresponding functions in
> libdiskfs. If/when making such changes all functions and function calls would
> have to be modified simultaneously.

It's fine to do all of this at the same time yes. When a given small
refactoring such as this one is repeated in various places, it's simpler
to commit it all at the same time.

Concerning the function interface, it's not a problem at all to change
it as you did, i.e. make the netfs function set errno on error: errno is
already set by various libc functions called by the netfs function. So
it's fine to start saying "now netfs really always set errno on error",
since it was already clobbering it anyway.


I was about to ask about diskfs_make_peropen, but it seems that that one
is already properly decoupled :)


> @@ -427,36 +433,42 @@ netfs_S_dir_lookup (struct protid *dircred,
>    if (err)
>      goto out;
>  
> -  newpi = netfs_make_protid (netfs_make_peropen (np, flags, dircred->po),
> -                          user);
> -  if (! newpi)
> +  newpo = netfs_make_peropen (np, flags, dircred->po);
> +  if (! newpo)
> +    err = errno;

You could goto out here already, can't you? That'd avoid this
indentation:

> +  if (! err)
>      {
> -      iohelp_free_iouser (user);
> -      err = errno;
> -      goto out;
> -    }
> +      newpi = netfs_make_protid (newpo, user);
> +      if (! newpi)
> +     {
> +       err = errno;
> +       netfs_release_peropen (newpo);
> +       iohelp_free_iouser (user);
> +       goto out;
> +     }

as well as the rest:

> -  mach_port_t rendezvous = MACH_PORT_NULL;
> -  struct flock64 lock =
> -    {
> -    l_start: 0,
> -    l_len: 0,
> -    l_whence: SEEK_SET
> -    };
> +      mach_port_t rendezvous = MACH_PORT_NULL;
> +      struct flock64 lock =
> +     {
> +     l_start: 0,
> +     l_len: 0,
> +     l_whence: SEEK_SET
> +     };

etc.


Here you did it well for instance:

> @@ -41,10 +43,16 @@ netfs_S_file_reparent (struct protid *cred, mach_port_t 
> parent,
>    node = cred->po->np;
>  
>    pthread_mutex_lock (&node->lock);
> -  
> -  new_cred =
> -    netfs_make_protid (netfs_make_peropen (node, cred->po->openstat, 
> cred->po),
> -                    user);
> +
> +  new_peropen = netfs_make_peropen (node, cred->po->openstat, cred->po);
> +  if (! new_peropen)
> +    {
> +      err = errno;
> +      pthread_mutex_unlock (&node->lock);
> +      return err;
> +    }
> +
> +  new_cred = netfs_make_protid (new_peropen, user);
>    pthread_mutex_unlock (&node->lock);
>  
>    if (new_cred)

as well as in various other places. Please do the same for the other
places, so we avoid too much indentation.


> diff --git a/libnetfs/make-peropen.c b/libnetfs/make-peropen.c
> index 4bd74740..7bcab695 100644
> --- a/libnetfs/make-peropen.c
> +++ b/libnetfs/make-peropen.c
> @@ -29,12 +29,18 @@ netfs_make_peropen (struct node *np, int flags, struct 
> peropen *context)
>    struct peropen *po = malloc (sizeof (struct peropen));
>  
>    if (!po)
> -    return NULL;
> +    {
> +      errno = ENOMEM;
> +      return NULL;
> +    }

That is useless, malloc already sets errno.


Thanks for your work on this!
Samuel