Re: [PATCH] Migrating storeio from trivfs to netfs.

Hello,

Thanks for the patch revision, sorry I was on vacation for two weeks.
We're getting closer!

Mikhail Karpov, le dim. 09 août 2026 18:35:34 +0700, a ecrit:
> @@ -37,7 +37,7 @@ lib-subdirs = libshouldbeinlibc libihash libiohelp libports \
>  # Hurd programs
>  prog-subdirs = auth proc exec term \
>  	       ext2fs isofs tmpfs fatfs \
> -	       storeio pflocal pfinet defpager mach-defpager \
> +	       pflocal pfinet defpager mach-defpager \
>  	       login daemons boot console \
>  	       hostmux usermux ftpfs trans \
>  	       console-client utils sutils libfshelp-tests \
> @@ -70,6 +70,10 @@ ifeq ($(HAVE_LIBACPICA),yes)
>  prog-subdirs += acpi
>  endif
>  
> +ifneq ($(PARTED_LIBS),)
> +prog-subdirs += storeio
> +endif
> +

Ah, no, we do not want to have to depend on parted for such a low-level
thing as storeio. Better automatically disable the part of the code that
adds partitions, it shouldn't be very hard, it'd essentially mean making
create_partitions just return ENOTDIR, and #ifdef-out the parted-using
functions.

> diff --git a/libnetfs/init-init.c b/libnetfs/init-init.c
> index 19ed0d3..f64c7bb 100644
> --- a/libnetfs/init-init.c
> +++ b/libnetfs/init-init.c
> @@ -38,9 +38,9 @@ void
>  netfs_init (void)
>  {
>    error_t err;
> -  err = maptime_map (0, 0, &netfs_mtime);
> +  err = maptime_map (1, 0, &netfs_mtime);
>    if (err)
> -    err = maptime_map (1, 0, &netfs_mtime);
> +    err = maptime_map (0, 0, &netfs_mtime);
>    if (err)
>      error (2, err, "mapping time");
>  

Better make this a separate patch.

> diff --git a/storeio/dev.c b/storeio/dev.c
> index c87400c..97b36cd 100644
> --- a/storeio/dev.c
> +++ b/storeio/dev.c
> @@ -22,7 +22,6 @@
>  #include <assert-backtrace.h>
>  #include <string.h>
>  #include <hurd/pager.h>
> -#include <hurd/store.h>

dev.c is still calling store_create etc. so should include store.h

> @@ -140,48 +139,31 @@ dev_buf_rw (struct dev *dev, size_t buf_offs, size_t *io_offs, size_t *len,
>        return 0;
>      }
>  }
> -
> -/* Called with DEV->lock held.  Try to open the store underlying DEV.  */
> +
>  error_t
> -dev_open (struct dev *dev)
> +dev_open_from_store (struct dev *dev, struct store *store)

Keep the commend about DEV->lock being held, and add a comment so the
reader understands the difference with dev_open.

> diff --git a/storeio/dev.h b/storeio/dev.h
> index eda7a93..2252f60 100644
> --- a/storeio/dev.h
> +++ b/storeio/dev.h
> @@ -84,14 +63,48 @@ struct dev
>    pthread_mutex_t pager_lock;
>  };
>  
> +struct netnode
> +{
> +  struct dev *dev;
> +  struct opens *opens;

"opens" does not seem to be used any more?

> +  char *name;
> +  struct node **entries;
> +  size_t entries_size;
> +};

Please comment what entries_size is exactly.

> diff --git a/storeio/pager.c b/storeio/pager.c
> index 11bf469..7df1a79 100644
> --- a/storeio/pager.c
> +++ b/storeio/pager.c
> @@ -27,6 +27,7 @@
>  #include <error.h>
>  #include <sys/mman.h>
>  #include <stdio.h>
> +#include <string.h>
>  
>  #include "dev.h"
>  
> @@ -51,6 +52,11 @@ pager_read_page (struct user_pager_info *upi,
>      /* Read a partial page if necessary to avoid reading off the end.  */
>      want = store->size - page;
>  
> +  void *new_buf = mmap (0, want, PROT_READ|PROT_WRITE, MAP_ANON, 0, 0);
> +  if (new_buf == MAP_FAILED)
> +    return EIO;
> +
> +  *((void **) buf) = new_buf;
>    err = dev_read (dev, page, want, (void **)buf, &read);

Mmm, why always allocating? It's better to allocate lazily in case the
read can be directly be fed by raw_read i.e. store_read, and avoid
a copy. That means keeping ensure_buf() etc., did you have a reason
to remove it? You can make pager_read_page set buf to NULL, and let
dev_read cope with that case, and otherwise try to use the provided
buffer (netfs_attempt_read case).

> diff --git a/storeio/storeio.c b/storeio/storeio.c
> index 4e8a962..3e79e12 100644
> --- a/storeio/storeio.c
> +++ b/storeio/storeio.c
[...]
> +static inline char *
> +create_node_name (const size_t num)
>  {
> -  struct dev *const device = fsys->hook;
> +  char buffer[20];
> +  snprintf (buffer, sizeof (buffer), "%zu", num);
> +
> +  return strdup (buffer);
> +}

Better use asprintf.

Samuel