Re: [PATCH hci-8.0 v2 2/3] chardev: preserve the logfile fd across cpr-exec #VSTOR-148304

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] chardev: log fd left stale in CPR registry on cpr-transfer target
--

commit 9181fad86181ebb4fb1b78687516f5494301eab0
Author: Denis V. Lunev <den@openvz.org>

chardev: preserve the logfile fd across cpr-exec #VSTOR-148304

This patch saves the chardev log fd in the CPR fd registry under the
chardev label. On cpr-exec incoming, qemu_char_open() takes the fd from
the registry instead of reopening the logfile path. char_finalize() and
char_cpr_claim_fds() are updated to keep the registry entry consistent.

> diff --git a/chardev/char.c b/chardev/char.c
> --- a/chardev/char.c
> +++ b/chardev/char.c

[ ... ]

> @@ -261,9 +262,21 @@ static void qemu_char_open(Chardev *chr, ChardevBackend *backend,
>          } else {
>              flags |= O_TRUNC;
>          }
> -        chr->logfd = qemu_create(common->logfile, flags, 0666, errp);
> -        if (chr->logfd < 0) {
> -            return;
> +        if (cpr_exec_is_incoming()) {
> +            chr->logfd = cpr_find_fd(chr->label, CPR_CHARDEV_LOG_ID);
> +            if (chr->logfd < 0) {
> +                warn_report("chardev '%s': log file was not preserved, "
> +                            "continuing without it", chr->label);
> +            }
> +        } else {
> +            chr->logfd = qemu_create(common->logfile, flags, 0666, errp);
> +            if (chr->logfd < 0) {
> +                return;
> +            }
> +            if (!cpr_is_incoming()) {
> +                cpr_delete_fd(chr->label, CPR_CHARDEV_LOG_ID);
> +                cpr_save_fd(chr->label, CPR_CHARDEV_LOG_ID, chr->logfd);
> +            }
>          }
>      }

[Severity: Low]
On a cpr-transfer target, cpr_is_incoming() is true but
cpr_exec_is_incoming() is false. Does qemu_char_open() then take the
else branch and reopen the path with qemu_create()?

The log fd that the source saved with cpr_save_fd() is part of the
registry that cpr_state_save() serializes for MIG_MODE_CPR_TRANSFER as
well. On the target, that fd is neither used nor replaced, because the
cpr_save_fd() call is skipped by the !cpr_is_incoming() check.

Could this leave an unused log fd open and registered under
(label, CPR_CHARDEV_LOG_ID) until process exit? If the target later does
a CPR save, the stale entry would be serialized too.

Would the Permission denied failure described in the commit message also
still occur in cpr-transfer mode, since the path is reopened there?

[ ... ]

> @@ -315,11 +332,14 @@ static void char_finalize(Object *obj)
>      if (chr->be) {
>          chr->be->chr = NULL;
>      }
> -    g_free(chr->filename);
> -    g_free(chr->label);
>      if (chr->logfd != -1) {
> +        if (cpr_find_fd(chr->label, CPR_CHARDEV_LOG_ID) == chr->logfd) {
> +            cpr_delete_fd(chr->label, CPR_CHARDEV_LOG_ID);
> +        }
>          close(chr->logfd);
>      }

Related to the above: in char_finalize(), the registry entry is removed
only when cpr_find_fd() equals chr->logfd. On a cpr-transfer target, the
stale entry holds the fd received from the source, which differs from the
freshly opened chr->logfd. Does that entry then never get removed, so its
fd stays open?

I did not find any code that clears it, and the follow-up patch only adds
an iotest.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261005092635.3564485-1-den@openvz.org?part=2