Re: [PATCH 3/3] iotests/migrate-cpr-exec: cover chardevs with logfile= #VSTOR-148304

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Low] iotests: socket logfile test leaves stale path=sock1 in exec command line
- [Low] iotests: assertFalse on BAD_LOG parent dir cannot detect an attempted logfile open
- [Low] iotests: _open_files() compares /proc readlink results to non-canonical paths
- [Low] iotests: pre_cpr rewrite drops logappend=on from the console chardev argument
--

commit 8a854fc4d41646d884346abb08e5af9be8f46b16
Author: Denis V. Lunev <den@openvz.org>

iotests/migrate-cpr-exec: cover chardevs with logfile= #VSTOR-148304

This adds four tests to migrate-cpr-exec. They cover a pty console, a
guest agent socket and a second QMP monitor, each with a log, through
cpr-exec with logfile paths that cannot be created. They also cover
chardev-change to a new log for pty and socket, and a failed
chardev-change whose rollback must keep the old log.

> diff --git a/tests/qemu-iotests/tests/migrate-cpr-exec b/tests/qemu-iotests/tests/migrate-cpr-exec
> index 51e6cbcd6593..e8a6c502c840 100755
> --- a/tests/qemu-iotests/tests/migrate-cpr-exec
> +++ b/tests/qemu-iotests/tests/migrate-cpr-exec

[ ... ]

> +    def _open_files(self) -> set:
> +        fd_dir = f'/proc/{self.vm.get_pid()}/fd'
> +        return {os.readlink(os.path.join(fd_dir, fd))
> +                for fd in os.listdir(fd_dir)}

[Severity: Low]
Can this comparison fail when TEST_DIR is not canonical?

os.readlink() on /proc/<pid>/fd/N returns the kernel's resolved path. The
expected paths come from _log_path() as os.path.join(iotests.test_dir, name),
and they are not passed through os.path.realpath().

If TEST_DIR is reached through a symlink, this assertion fails even though
the fds were preserved:

    self.assertLessEqual({agent_log, mon_log}, self._open_files())

In the same case, this assertion in _test_chardev_change_logfile() passes
vacuously, so it no longer checks that the old log fd was dropped:

    self.assertNotIn(log_old, self._open_files())

I did not confirm how iotests normalizes test_dir. Would applying
os.path.realpath() to the expected paths avoid the mismatch?

[ ... ]

> +        def pre_cpr():
> +            self._serial_write('before\n')
> +            for c in (console, agent, mon):
> +                self._replace_arg(c, c.replace(c[c.index('logfile='):],
> +                                               f'logfile={self.BAD_LOG}'))

[Severity: Low]
Does this rewrite drop logappend=on from the console chardev argument?

For the console chardev, c[c.index('logfile='):] is
'logfile=/dev/fdset/1,logappend=on'. The whole tail is replaced, so the new
argument becomes:

    pty,id=charserial0,logfile=<BAD_LOG>

The agent and mon arguments have logfile= last, so they are unaffected.

The combination of logappend=on with an fd-backed preserved log is
therefore not exercised after the exec. This is the combination the commit
message says libvirt sends. Today qemu_char_open() does not use the
computed flags when cpr_exec_is_incoming() is true, so the test passes. A
later change that consults logappend on the restore path would not be
caught.

Would it be better to replace only the path value and keep ",logappend=on"?

> +            args = list(self.vm._qemu_full_args)
> +            i = args.index('-add-fd')
> +            self.vm._qemu_full_args = tuple(args[:i] + args[i + 2:])
> +
> +        def post_cpr():
> +            self._serial_write('after\n')
> +            self.assertEqual(self._read(console_log), 'before\nafter\n')
> +            self.assertLessEqual({agent_log, mon_log}, self._open_files())
> +            self.assertFalse(os.path.exists(os.path.dirname(self.BAD_LOG)))

[Severity: Low]
Can this assertion ever fail?

BAD_LOG is <test_dir>/nonexistent/chr.log, and nothing creates the
"nonexistent" directory. qemu_create() called from qemu_char_open() does
not create parent directories. If a regressed QEMU tried to open the path,
it would get ENOENT and the directory would stay absent, so
assertFalse(os.path.exists(os.path.dirname(self.BAD_LOG))) always holds.

The real evidence that the path is not opened is that the new QEMU starts
at all. The docstring ("the new logfile= paths are never opened") and the
commit message suggest this assertion adds coverage that it does not.

Could the assertion be dropped, or replaced with a check that can actually
fail?

[ ... ]

> +    def test_chardev_change_logfile_socket(self) -> None:
> +        sock1 = iotests.file_path('chr-log-sock1')
> +        sock2 = iotests.file_path('chr-log-sock2')
> +        self._test_chardev_change_logfile(
> +            'socket',
> +            {'addr': {'type': 'unix', 'data': {'path': sock2}},
> +             'server': True, 'wait': False},
> +            f'path={sock1},server=on,wait=off,')

[Severity: Low]
This isn't a bug, but the rewritten exec command line for the socket case
looks inconsistent with the live chardev.

In _test_chardev_change_logfile(), change() calls:

    self._replace_arg(arg, f'{backend},id=char0,{opts_old}'
                           f'logfile={self.BAD_LOG}')

For the socket case opts_old is 'path=<sock1>,server=on,wait=off,'. After
chardev-change the live chardev listens on sock2, so the new command line
still names sock1, which is no longer in use.

The existing test_chardev_change_socket_to_socket updates the path to
sock2. The test passes here because chr_socket_cpr_restore_addr_fd() in
chardev/char-socket.c uses the inherited fd and not the path. As written,
the test does not model what a management layer would send. It would also
not catch a regression that used the path instead of the fd.

Should the rewritten argument use sock2 for the path?

[ ... ]

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