On Thu, Aug 07, 2025 at 01:44:24AM +0000, Pasha Tatashin wrote:
> +struct liveupdate_ioctl_get_fd_state {
> + __u32 size;
> + __u8 incoming;
> + __aligned_u64 token;
> + __u32 state;
> +};
Same remark about explicit padding and checking padding for 0
> + * luo_file_get_state - Get the preservation state of a specific file.
> + * @token: The token of the file to query.
> + * @statep: Output pointer to store the file's current live update state.
> + * @incoming: If true, query the state of a restored file from the incoming
> + * (previous kernel's) set. If false, query a file being prepared
> + * for preservation in the current set.
> + *
> + * Finds the file associated with the given @token in either the incoming
> + * or outgoing tracking arrays and returns its current LUO state
> + * (NORMAL, PREPARED, FROZEN, UPDATED).
> + *
> + * Return: 0 on success, -ENOENT if the token is not found.
> + */
> +int luo_file_get_state(u64 token, enum liveupdate_state *statep, bool incoming)
> +{
> + struct luo_file *luo_file;
> + struct xarray *target_xa;
> + int ret = 0;
> +
> + luo_state_read_enter();
Less globals, at this point everything should be within memory
attached to the file descriptor and not in globals. Doing this will
promote good maintainable structure and not a spaghetti
Also I think a BKL design is not a good idea for new code. We've had
so many bad experiences with this pattern promoting uncontrolled
incomprehensible locking.
The xarray already has a lock, why not have reasonable locking inside
the luo_file? Probably just a refcount?
> + target_xa = incoming ? &luo_files_xa_in : &luo_files_xa_out;
> + luo_file = xa_load(target_xa, token);
> +
> + if (!luo_file) {
> + ret = -ENOENT;
> + goto out_unlock;
> + }
> +
> + scoped_guard(mutex, &luo_file->mutex)
> + *statep = luo_file->state;
> +
> +out_unlock:
> + luo_state_read_exit();
If we are using cleanup.h then use it for this too..
But it seems kind of weird, why not just
xa_lock()
xa_load()
*statep = READ_ONCE(luo_file->state);
xa_unlock()
?
> +static int luo_ioctl_set_fd_event(struct luo_ucmd *ucmd)
> +{
> + struct liveupdate_ioctl_set_fd_event *argp = ucmd->cmd;
> + int ret;
> +
> + switch (argp->event) {
> + case LIVEUPDATE_PREPARE:
> + ret = luo_file_prepare(argp->token);
> + break;
> + case LIVEUPDATE_FREEZE:
> + ret = luo_file_freeze(argp->token);
> + break;
> + case LIVEUPDATE_FINISH:
> + ret = luo_file_finish(argp->token);
> + break;
> + case LIVEUPDATE_CANCEL:
> + ret = luo_file_cancel(argp->token);
> + break;
The token should be converted to a file here instead of duplicated in
each function
> static int luo_open(struct inode *inodep, struct file *filep)
> {
> if (atomic_cmpxchg(&luo_device_in_use, 0, 1))
> @@ -149,6 +191,8 @@ union ucmd_buffer {
> struct liveupdate_ioctl_fd_restore restore;
> struct liveupdate_ioctl_get_state state;
> struct liveupdate_ioctl_set_event event;
> + struct liveupdate_ioctl_get_fd_state fd_state;
> + struct liveupdate_ioctl_set_fd_event fd_event;
> };
>
> struct luo_ioctl_op {
> @@ -179,6 +223,10 @@ static const struct luo_ioctl_op luo_ioctl_ops[] = {
> struct liveupdate_ioctl_get_state, state),
> IOCTL_OP(LIVEUPDATE_IOCTL_SET_EVENT, luo_ioctl_set_event,
> struct liveupdate_ioctl_set_event, event),
> + IOCTL_OP(LIVEUPDATE_IOCTL_GET_FD_STATE, luo_ioctl_get_fd_state,
> + struct liveupdate_ioctl_get_fd_state, token),
> + IOCTL_OP(LIVEUPDATE_IOCTL_SET_FD_EVENT, luo_ioctl_set_fd_event,
> + struct liveupdate_ioctl_set_fd_event, token),
> };
Keep sorted
Jason
