Re: [PATCH v6 06/20] liveupdate: luo_file: implement file systems callbacks

On Thu, Nov 20, 2025 at 12:20 PM Mike Rapoport [off-list ref] wrote:

On Mon, Nov 17, 2025 at 12:50:56PM -0500, Pasha Tatashin wrote:

quoted

quoted

quoted

+struct liveupdate_file_handler;
+struct liveupdate_session;

Why struct liveupdate_session is a part of public LUO API?

It is an obscure version of private "struct luo_session", in order to
give subsystem access to:
liveupdate_get_file_incoming(s, token, filep)
liveupdate_get_token_outgoing(s, file, tokenp)

For example, if your FD depends on another FD within a session, you
can check if another FD is already preserved via
liveupdate_get_token_outgoing(), and during retrieval time you can
retrieve the "struct file" for your dependency.

And it's essentially unused right now.

I am going to move this API to the end of the series, next to FLB :-)

quoted

quoted

quoted

+     }
+
+     return 0;
+
+exit_err:
+     fput(file);
+     luo_session_free_files_mem(session);

The error handling in this function is a mess. Pasha, please, please, use
goto consistently.

How is this a mess? There is a single exit_err destination, no
exception, no early returns except at the very top of the function
where we do early returns before fget() which makes total sense.

Do you want to add a separate destination for
luo_session_free_files_mem() ? But that is not necessary, in many
places it is considered totally reasonable for free(NULL) to work
correctly...

You have a mix of releasing resources with goto or inside if (err).
And while basic free() primitives like kfree() and vfree() work correctly
with NULL as a parameter, luo_session_free_files_mem() is already not a
basic primitive and it may grow with a time. It already has two conditions
that essentially prevent anything from freeing and this will grow with the
time.

So yes, I want a separate goto destination for freeing each resource and a
goto for

        err = fh->ops->preserve(&args);
        if (err)

Thanks, I made the change.

case.

quoted

quoted

quoted

+             luo_file = kzalloc(sizeof(*luo_file), GFP_KERNEL);
+             if (!luo_file)
+                     return -ENOMEM;

Shouldn't we free files allocated on the previous iterations?

No, for the same reason explained in luo_session.c :-)

A comment here as well please :)

Done

quoted

quoted

quoted

+int liveupdate_get_file_incoming(struct liveupdate_session *s, u64 token,
+                              struct file **filep)
+{

Ditto.

These two functions are part of the public API allowing dependency
tracking for vfio->iommu->memfd during preservation.

So like with FLB, until we get actual users for them they are dead code.
And until it's clear how exactly dependency tracking for vfio->iommu->memfd
will work, we won't know if this API is useful at all or we'll need
something else in the end.

SGTM

--
Sincerely yours,
Mike.