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

Hello,

Mikhail Karpov, le ven. 03 juil. 2026 20:48:20 +0700, a ecrit:
> On Tue, 23 Jun 2026 02:03:44 Samuel Thibault wrote:
> > Yes, that looks confusing to me. To the user, it would mean that
> > partition 1 itself contains a partition table.
> >
> > Can't we disable the directory facet when there is no actual partition
> > table in the store being driven?
> 
> How can this be verified? The thing is, even after passing a specific
> partition to the translator, no errors occur when reading its partitions.
> Libparted simply thinks it's working with a disk with only one partition.

Uh? So if the disk image has not partition table at all, libparted says
that it has one partition? That's ugly :/ We'd really need to find a
way to ask libparted whether there is a partition table or not. Perhaps
through comparing the PedDiskType name field against "loop"?

> Getting data from struct store_parsed doesn't seem quite right, but I can't
> think of another way.

I don't see the relation with store_parsed?

> > Well, yes, but I meant that netfs_init could be made to call with
> > use_mach_dev=1 first, that would succeed. What I'm surprised is why
> > libdiskfs doesn't have the issue.
> 
> Yes, that helped. Surely this can't negatively impact the work of other
> translators?

I don't think trying mach first would hurt.

> > Having a single-chained open_list is however problematic for efficiency.
> 
> Agreed, I reworked this place and now there is something like std::vector
> from C++.

Good :)

> > But actually, why separating struct open from netfs' struct peropen?
> > netfs' peropen already has the filepointer field for the offset. In
> > po->np->nn->dev you have the dev. And libnetfs usually uses po->np->lock
> > as lock. You need really only nperopens to know whether the store is
> > active.
> 
> Yes, we were able to remove struct open by using only struct peropen.
> Furthermore, this significantly reduced the code in io.c.

Good :)

> > I'm also wondering if we want to plug at the netfs_S_io_read layer, or
> > rather at the netfs_attempt_read layer, since the latter would probably
> > be simpler? And similar for other operations.
> 
> The dev_read and dev_write functions contain code similar to netfs_S_io_read
> and netfs_S_io_write.

? which code?

I was rather thinking about all the code about permission checking and
locking that you have in your netfs_S_io_read/write before calling
dev_read/write.

> If it's worth it, I might give it a try.

I believe it will be worth it, to factorize this permission code that is
risky to get wrong.

> +struct opens
> +{
> +  struct peropen **opens;
> +  size_t size;
> +  size_t capacity;
> +};

You need to document what size and capacity represent exactly.

But why do you maintain that opens array actually? I don't think you
need to iterate on peropens anywhere? You only need to count nperopens
as the existing code does, to clear/mark STORE_INACTIVE.

> +struct netnode
> +{
> +  struct dev *dev;
> +  struct opens *opens;

I don't think this needs to be a pointer? Having a separate structure
is nice for separation of concerns indeed, but you can as well just
allocate it along struct netnode, and use

struct opens *opens = &netnode->opens;

(but again, I don't think you need that struct opens, and just the
existing nperopens).

> +static error_t
> +node_open_create (struct peropen *po)
> +{
> +  struct netnode *netnode = po->np->nn;
> +
> +  pthread_mutex_lock (&netnode->dev->lock);
> +  struct opens *opens = netnode->opens;
> +  if (opens->capacity == opens->size)
> +    {
> +      if (opens->size == 0)

opens->size can be 0 even if opens->capacity is > 0, when
opening/closing, then opening again. I guess you just need to move
that if (opens->size) and its content before if (opens->capacity ==
opens->size) (but again, I believe you don't need that array at all).

With regards,
Samuel