[PATCH 0/3] chardev: fix qemu-update destroying VMs with a console log #VSTOR-148304

[PATCH 0/3] chardev: fix qemu-update destroying VMs with a console log #VSTOR-148304

[Denis V. Lunev]

qemu-update destroys every running VM whose console log the qemu user
cannot create, which is the case for nova console.log on node-local
storage:

  qemu-kvm: -chardev pty,id=charserial0,logfile=.../console.log,
  logappend=on: Could not create '.../console.log': Permission denied

and the update stops on that node. libvirt passes the log to QEMU as a
virtlogd pipe in an fdset. That fdset cannot be carried over cpr-exec,
so the qemu-update command line names the log path, and the new QEMU
reopens it in qemu_char_open(). On shared storage the open succeeds,
and the VM survives, but QEMU then writes console.log directly instead
of through virtlogd.

The series keeps the log fd, i.e. the virtlogd pipe, in the cpr
registry next to the backend fds. The new QEMU never opens the logfile
path. Patch 1 gives the base chardev class a cpr_claim_fds() hook so
that the log can be re-claimed on a failed chardev-change. Patch 2 is
the fix. Patch 3 extends the cpr-exec iotest.

No libvirt change is needed: the path in the qemu-update command line
is no longer read. A vz-cpr-path placeholder there would make an exec
into an unfixed qemu-kvm fail for every VM with a console log.

The first update from a build without this series still loses the log:
the old QEMU did not save the fd, so the new one warns and runs without
a log until the VM restarts. The VM itself keeps running.

This supersedes qemu.vzs PR #7, which reopens the path and only warns
if that fails.

Tested on hci-8.0:
- migrate-cpr-exec: the new cases fail without patch 2, only the
  rollback case fails without the base cpr_claim_fds() body, all pass
  with the series; test-char and migration-test mode/{transfer,exec}
  pass
- virsh qemu-update of an Alma9 guest with a pty serial log in a
  directory the qemu user cannot write, using the source build as
  emulator: unfixed to unfixed reproduces the failure; unfixed to fixed
  keeps the VM running without a log; fixed to fixed twice keeps the
  same virtlogd pipe and console output keeps reaching console.log

Signed-off-by: Denis V. Lunev <den@openvz.org>
Cc: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Cc: Maxim Nestratov <mnestratov@virtuozzo.com>

Denis V. Lunev (3):
  chardev: implement cpr_claim_fds() in the base class #VSTOR-148304
  chardev: preserve the logfile fd across cpr-exec #VSTOR-148304
  iotests/migrate-cpr-exec: cover chardevs with logfile= #VSTOR-148304

 chardev/char-file.c                           |   3 +
 chardev/char-pty.c                            |   3 +
 chardev/char-socket.c                         |   3 +
 chardev/char.c                                |  43 ++++--
 include/migration/cpr.h                       |   1 +
 tests/qemu-iotests/tests/migrate-cpr-exec     | 132 ++++++++++++++++++
 tests/qemu-iotests/tests/migrate-cpr-exec.out |   4 +-
 7 files changed, 176 insertions(+), 13 deletions(-)


base-commit: f2f4b3de1a1be3360a77ca6ab79762466d238804
-- 
2.53.0

[PATCH 1/3] chardev: implement cpr_claim_fds() in the base class #VSTOR-148304

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

Give TYPE_CHARDEV an empty cpr_claim_fds() and make the file, pty and
socket backends chain to their parent's. Every chardev class then has
the hook, so qmp_chardev_change() calls it without a NULL check, and
state owned by the base class can be re-claimed there as well.

No functional change.

Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 chardev/char-file.c   |  3 +++
 chardev/char-pty.c    |  3 +++
 chardev/char-socket.c |  3 +++
 chardev/char.c        | 13 +++++++------
 4 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/chardev/char-file.c b/chardev/char-file.c
index a90f4a77e13..89011f52e3e 100644
--- a/chardev/char-file.c
+++ b/chardev/char-file.c
@@ -124,7 +124,10 @@ out_close:
 static void char_file_cpr_claim_fds(Chardev *chr)
 {
     FDChardev *s = FD_CHARDEV(chr);
+    ChardevClass *pc = CHARDEV_CLASS(
+        object_class_get_parent(object_class_by_name(TYPE_CHARDEV_FILE)));
 
+    pc->cpr_claim_fds(chr);
     if (s->ioc_out) {
         cpr_delete_fd(chr->label, 0);
         cpr_save_fd(chr->label, 0, QIO_CHANNEL_FILE(s->ioc_out)->fd);
diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index a0090e4c3d1..8058da92bb4 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -403,7 +403,10 @@ static void char_pty_open(Chardev *chr,
 static void char_pty_cpr_claim_fds(Chardev *chr)
 {
     PtyChardev *s = PTY_CHARDEV(chr);
+    ChardevClass *pc = CHARDEV_CLASS(
+        object_class_get_parent(object_class_by_name(TYPE_CHARDEV_PTY)));
 
+    pc->cpr_claim_fds(chr);
     if (s->ioc) {
         cpr_delete_fd(chr->label, 0);
         cpr_save_fd(chr->label, 0, QIO_CHANNEL_FILE(s->ioc)->fd);
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index d20aea764f9..c73a3dd0f7a 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1681,7 +1681,10 @@ char_socket_get_connected(Object *obj, Error **errp)
 static void char_socket_cpr_claim_fds(Chardev *chr)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
+    ChardevClass *pc = CHARDEV_CLASS(
+        object_class_get_parent(object_class_by_name(TYPE_CHARDEV_SOCKET)));
 
+    pc->cpr_claim_fds(chr);
     if (s->sioc) {
         cpr_delete_fd(chr->label, CPR_CONNSOCK_ID);
         cpr_save_fd(chr->label, CPR_CONNSOCK_ID, s->sioc->fd);
diff --git a/chardev/char.c b/chardev/char.c
index 87f40d0137e..b64767f574e 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -295,12 +295,17 @@ static int null_chr_write(Chardev *chr, const uint8_t *buf, int len)
     return len;
 }
 
+static void char_cpr_claim_fds(Chardev *chr)
+{
+}
+
 static void char_class_init(ObjectClass *oc, const void *data)
 {
     ChardevClass *cc = CHARDEV_CLASS(oc);
 
     cc->chr_write = null_chr_write;
     cc->chr_be_event = chr_be_event;
+    cc->cpr_claim_fds = char_cpr_claim_fds;
 }
 
 static void char_finalize(Object *obj)
@@ -1164,9 +1169,7 @@ ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
                           backend, chr->gcontext, handover_yank_instance, errp);
     if (!chr_new) {
         /* the failed replacement's open() may have displaced our cpr fds */
-        if (cc->cpr_claim_fds) {
-            cc->cpr_claim_fds(chr);
-        }
+        cc->cpr_claim_fds(chr);
         return NULL;
     }
 
@@ -1186,9 +1189,7 @@ ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
             qemu_chr_be_event(chr, CHR_EVENT_OPENED);
         }
         /* chr_new's open() may have displaced our cpr fds; re-claim them */
-        if (cc->cpr_claim_fds) {
-            cc->cpr_claim_fds(chr);
-        }
+        cc->cpr_claim_fds(chr);
         object_unref(OBJECT(chr_new));
         return NULL;
     }
-- 
2.53.0

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

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

A chardev with logfile= opens the log in qemu_char_open() on every
start, including the start of the new QEMU after cpr-exec. The backend
fds are taken from the cpr fd registry there, the log fd is not, so the
new process reopens the path itself.

libvirt normally opens the log through virtlogd and passes a pipe in an
fdset. The qemu-update command line cannot carry that fdset, so it
names the log file path. The exec'd QEMU, running as the qemu user,
then fails to create a log it has no write access to, e.g. a nova
console.log on node-local storage:

  qemu-kvm: -chardev pty,id=charserial0,logfile=.../console.log,
  logappend=on: Could not create '.../console.log': Permission denied

and the VM is destroyed. Where the open succeeds, the new QEMU writes
the file directly, behind virtlogd's back.

Save the log fd in the cpr registry under the chardev label and take it
from there on cpr-exec incoming, as the backends do. The logfile path
is not opened at all then. If the source did not save it, run without
a log rather than reopen the path. A failed chardev-change re-claims
the log through the base class cpr_claim_fds(), since the replacement's
log has displaced the entry.

Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 chardev/char.c          | 30 +++++++++++++++++++++++++-----
 include/migration/cpr.h |  1 +
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/chardev/char.c b/chardev/char.c
index b64767f574e..c037d2e7730 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -40,6 +40,7 @@
 #include "qemu/id.h"
 #include "qemu/coroutine.h"
 #include "qemu/yank.h"
+#include "migration/cpr.h"
 
 #include "chardev-internal.h"
 
@@ -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);
+            }
         }
     }
 
@@ -297,6 +310,10 @@ static int null_chr_write(Chardev *chr, const uint8_t *buf, int len)
 
 static void char_cpr_claim_fds(Chardev *chr)
 {
+    if (chr->logfd >= 0) {
+        cpr_delete_fd(chr->label, CPR_CHARDEV_LOG_ID);
+        cpr_save_fd(chr->label, CPR_CHARDEV_LOG_ID, chr->logfd);
+    }
 }
 
 static void char_class_init(ObjectClass *oc, const void *data)
@@ -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);
     }
+    g_free(chr->filename);
+    g_free(chr->label);
     qemu_mutex_destroy(&chr->chr_write_lock);
 }
 
diff --git a/include/migration/cpr.h b/include/migration/cpr.h
index d74ff1ea602..8fb79eb0a28 100644
--- a/include/migration/cpr.h
+++ b/include/migration/cpr.h
@@ -15,6 +15,7 @@
 
 #define CPR_LISTENSOCK_ID       0
 #define CPR_CONNSOCK_ID         1
+#define CPR_CHARDEV_LOG_ID      2
 
 #define QEMU_CPR_FILE_MAGIC     0x51435052
 #define QEMU_CPR_FILE_VERSION   0x00000001
-- 
2.53.0

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

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

Run a console pty with its log in an fdset, a guest agent socket and a
second QMP monitor, each with a log, through cpr-exec with a command
line whose logfile paths cannot be created, as libvirt's is. Cover
chardev-change to a new log for pty and socket, and a failed
chardev-change, whose rollback must keep the old log preserved.

Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 tests/qemu-iotests/tests/migrate-cpr-exec     | 132 ++++++++++++++++++
 tests/qemu-iotests/tests/migrate-cpr-exec.out |   4 +-
 2 files changed, 134 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/tests/migrate-cpr-exec b/tests/qemu-iotests/tests/migrate-cpr-exec
index 51e6cbcd659..e8a6c502c84 100755
--- a/tests/qemu-iotests/tests/migrate-cpr-exec
+++ b/tests/qemu-iotests/tests/migrate-cpr-exec
@@ -288,6 +288,138 @@ class TestCprExec(iotests.QMPTestCase):
             '-device', 'isa-serial,chardev=char0')
         self._test_cpr(pre_cpr_hook=change_socket)
 
+    BAD_LOG = os.path.join(iotests.test_dir, 'nonexistent', 'chr.log')
+
+    def _serial_write(self, data: str) -> None:
+        for c in data:
+            self.vm.qtest(f'outb 0x3f8 {ord(c)}')
+
+    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)}
+
+    def _replace_arg(self, old: str, new: str) -> None:
+        args = list(self.vm._qemu_full_args)
+        args[args.index(old)] = new
+        self.vm._qemu_full_args = tuple(args)
+
+    def _read(self, path: str) -> str:
+        with open(path, encoding='utf-8') as f:
+            return f.read()
+
+    def _log_path(self, name: str) -> str:
+        path = os.path.join(iotests.test_dir, name)
+        self.addCleanup(lambda: os.path.exists(path) and os.remove(path))
+        return path
+
+    def test_chardev_logfile(self) -> None:
+        """Log fds survive cpr-exec; the new logfile= paths are never opened"""
+        console_log = self._log_path('console.log')
+        agent_log = self._log_path('agent.log')
+        mon_log = self._log_path('mon.log')
+        agent_sock = iotests.file_path('agent.sock')
+        mon_sock = iotests.file_path('mon.sock')
+
+        console = 'pty,id=charserial0,logfile=/dev/fdset/1,logappend=on'
+        agent = (f'socket,id=charchannel0,path={agent_sock},server=on,'
+                 f'wait=off,logfile={agent_log}')
+        mon = (f'socket,id=charmonitor1,path={mon_sock},server=on,'
+               f'wait=off,logfile={mon_log}')
+
+        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}'))
+            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)))
+
+        fd = os.open(console_log, os.O_WRONLY | os.O_CREAT | os.O_APPEND)
+        try:
+            self.vm.add_fd(fd, 1, 'console')
+            self.vm.add_args(
+                '-chardev', console,
+                '-device', 'isa-serial,chardev=charserial0',
+                '-chardev', agent, '-device', 'virtio-serial',
+                '-device', 'virtserialport,chardev=charchannel0,'
+                           'name=org.qemu.guest_agent.0',
+                '-chardev', mon, '-mon', 'chardev=charmonitor1,mode=control')
+            self._test_cpr(pre_cpr_hook=pre_cpr, post_cpr_hook=post_cpr)
+        finally:
+            os.close(fd)
+
+    def _test_chardev_change_logfile(self, backend: str, data_new: dict,
+                                     opts_old: str) -> None:
+        """cpr-exec after chardev-change preserves the new log, not the old"""
+        log_old = self._log_path('chr-old.log')
+        log_new = self._log_path('chr-new.log')
+        arg = f'{backend},id=char0,{opts_old}logfile={log_old}'
+
+        def change():
+            self._serial_write('before\n')
+            res = self.vm.qmp('chardev-change', id='char0',
+                              backend={'type': backend,
+                                       'data': {**data_new,
+                                                'logfile': log_new}})
+            assert 'return' in res, res
+            self._replace_arg(arg, f'{backend},id=char0,{opts_old}'
+                                   f'logfile={self.BAD_LOG}')
+
+        def post_cpr():
+            self._serial_write('after\n')
+            self.assertEqual(self._read(log_old), 'before\n')
+            self.assertEqual(self._read(log_new), 'after\n')
+            self.assertNotIn(log_old, self._open_files())
+
+        self.vm.add_args('-chardev', arg,
+                         '-device', 'isa-serial,chardev=char0')
+        self._test_cpr(pre_cpr_hook=change, post_cpr_hook=post_cpr)
+
+    def test_chardev_change_logfile_pty(self) -> None:
+        self._test_chardev_change_logfile('pty', {}, '')
+
+    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,')
+
+    def test_chardev_change_logfile_rollback(self) -> None:
+        """A rolled back chardev-change keeps the old log preserved"""
+        log = self._log_path('chr.log')
+        log_new = self._log_path('chr-new.log')
+        link = self._log_path('pty-rollback.link')
+        open(link, 'w').close()
+        arg = f'pty,id=char0,logfile={log}'
+
+        def failed_change():
+            self._serial_write('before\n')
+            res = self.vm.qmp('chardev-change', id='char0',
+                              backend={'type': 'pty',
+                                       'data': {'path': link,
+                                                'logfile': log_new}})
+            assert 'error' in res, res
+            self._replace_arg(arg, f'pty,id=char0,logfile={self.BAD_LOG}')
+
+        def post_cpr():
+            self._serial_write('after\n')
+            self.assertEqual(self._read(log), 'before\nafter\n')
+
+        self.vm.add_args('-chardev', arg,
+                         '-device', 'isa-serial,chardev=char0')
+        self._test_cpr(pre_cpr_hook=failed_change, post_cpr_hook=post_cpr)
+
     def test_nbd_attached_hd(self) -> None:
         nbd_img = os.path.join(iotests.test_dir, 'nbd_disk')
         iotests.qemu_img_create('-f', 'qcow2', nbd_img, '64M')
diff --git a/tests/qemu-iotests/tests/migrate-cpr-exec.out b/tests/qemu-iotests/tests/migrate-cpr-exec.out
index 5ce2f9a2ed9..88bf7fa73a5 100644
--- a/tests/qemu-iotests/tests/migrate-cpr-exec.out
+++ b/tests/qemu-iotests/tests/migrate-cpr-exec.out
@@ -1,5 +1,5 @@
-..............................
+..................................
 ----------------------------------------------------------------------
-Ran 30 tests
+Ran 34 tests
 
 OK
-- 
2.53.0

Re: [PATCH 1/3] chardev: implement cpr_claim_fds() in the base class #VSTOR-148304

[no-reply]

> From: Denis V. Lunev <den@openvz.org>
> 
> Give TYPE_CHARDEV an empty cpr_claim_fds() and make the file, pty and
> socket backends chain to their parent's. Every chardev class then has
> the hook, so qmp_chardev_change() calls it without a NULL check, and
> state owned by the base class can be re-claimed there as well.
> 
> No functional change.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>

Sashiko has reviewed this patch and found no issues. It looks great!

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

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

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] chardev: logfd saved in all CPR modes but claimed only on cpr-exec incoming
--

commit 05df918f015920ae0431d59bdf9bb6ff8741e4fc
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, it takes the fd from the registry instead of reopening the logfile path. If the source did not save the fd, the chardev runs without a log.

> 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]
Is the log fd handled consistently across cpr modes here? The fd is saved
to the registry for every start where cpr_is_incoming() is false, but it
is only claimed when cpr_exec_is_incoming() is true.

In cpr-transfer, the target takes the else branch in qemu_char_open():

    chr->logfd = qemu_create(common->logfile, flags, 0666, errp);

so it reopens the logfile path itself. The save is then skipped because
cpr_is_incoming() is true. Can this leave the transferred fd unclaimed in
the registry, and also hit the same permission and virtlogd-bypass problem
the commit message describes for cpr-exec?

This follows the exec-only pattern in char-file.c and char-pty.c. I did not
verify whether cpr-transfer sends every registry fd. If it does not, the
leak does not occur, but the path reopen remains.

[ ... ]

> @@ -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);
>      }
> +    g_free(chr->filename);
> +    g_free(chr->label);
>      qemu_mutex_destroy(&chr->chr_write_lock);
>  }

[Severity: Low]
In the cpr-transfer target, can the registry entry outlive the chardev
here? cpr_find_fd() in char_finalize() returns the transferred fd, which
differs from chr->logfd, so cpr_delete_fd() is not called.

Would the stale registry entry and its open fd then stay until process
exit?

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

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

[no-reply]

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

Re: [PATCH 1/3] chardev: implement cpr_claim_fds() in the base class #VSTOR-148304

[Andrey Drobyshev]

On 10/2/26 7:48 PM, Denis V. Lunev wrote:
> From: Denis V. Lunev <den@openvz.org>
> 
> Give TYPE_CHARDEV an empty cpr_claim_fds() and make the file, pty and
> socket backends chain to their parent's. Every chardev class then has
> the hook, so qmp_chardev_change() calls it without a NULL check, and
> state owned by the base class can be re-claimed there as well.
> 
> No functional change.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> ---
>  [...]

Reviewed-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

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

[Andrey Drobyshev]

On 10/2/26 7:48 PM, Denis V. Lunev wrote:
> From: Denis V. Lunev <den@openvz.org>
> 
> A chardev with logfile= opens the log in qemu_char_open() on every
> start, including the start of the new QEMU after cpr-exec. The backend
> fds are taken from the cpr fd registry there, the log fd is not, so the
> new process reopens the path itself.
> 
> libvirt normally opens the log through virtlogd and passes a pipe in an
> fdset. The qemu-update command line cannot carry that fdset, so it
> names the log file path. The exec'd QEMU, running as the qemu user,
> then fails to create a log it has no write access to, e.g. a nova
> console.log on node-local storage:
> 
>   qemu-kvm: -chardev pty,id=charserial0,logfile=.../console.log,
>   logappend=on: Could not create '.../console.log': Permission denied
> 
> and the VM is destroyed. Where the open succeeds, the new QEMU writes
> the file directly, behind virtlogd's back.
> 
> Save the log fd in the cpr registry under the chardev label and take it
> from there on cpr-exec incoming, as the backends do. The logfile path
> is not opened at all then. If the source did not save it, run without
> a log rather than reopen the path. A failed chardev-change re-claims
> the log through the base class cpr_claim_fds(), since the replacement's
> log has displaced the entry.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> ---
>  [...]

LGTM.

Nit: include/chardev/char.h:308 - comment probably needs updating (this
patch or previous one), stating that overrides must call the parent's
hook first.

Reviewed-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

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

[Andrey Drobyshev]

One important case isn't covered. Namely, when source does NOT save the
log FD and target has to proceed without it - what we saw in the
VSTOR-148304.

1. Start with -chardev pty,id=char0 (no log) and isa-serial.
2. In the pre-CPR hook, replace that arg with
pty,id=char0,logfile=<BAD_LOG>.
3. After CPR, check that the VM is still running and that BAD_LOG was
not created (put it in an existing directory, so a reopen by path would
show up).
4. After shutdown, check that self.vm.get_log() contains log file was
not preserved, continuing without it. The exec'd QEMU inherits the old
process's stderr, so the warning ends up in the same log.

Please add this case.


On 10/2/26 7:48 PM, Denis V. Lunev wrote:
> From: Denis V. Lunev <den@openvz.org>
> 
> Run a console pty with its log in an fdset, a guest agent socket and a
> second QMP monitor, each with a log, through cpr-exec with a command
> line whose logfile paths cannot be created, as libvirt's is. Cover
> chardev-change to a new log for pty and socket, and a failed
> chardev-change, whose rollback must keep the old log preserved.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> ---
>  tests/qemu-iotests/tests/migrate-cpr-exec     | 132 ++++++++++++++++++
>  tests/qemu-iotests/tests/migrate-cpr-exec.out |   4 +-
>  2 files changed, 134 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/tests/migrate-cpr-exec b/tests/qemu-iotests/tests/migrate-cpr-exec
> index 51e6cbcd659..e8a6c502c84 100755
> --- a/tests/qemu-iotests/tests/migrate-cpr-exec
> +++ b/tests/qemu-iotests/tests/migrate-cpr-exec
> @@ -288,6 +288,138 @@ class TestCprExec(iotests.QMPTestCase):
>              '-device', 'isa-serial,chardev=char0')
>          self._test_cpr(pre_cpr_hook=change_socket)
>  
> +    BAD_LOG = os.path.join(iotests.test_dir, 'nonexistent', 'chr.log')
> +
> +    def _serial_write(self, data: str) -> None:
> +        for c in data:
> +            self.vm.qtest(f'outb 0x3f8 {ord(c)}')
> +
> +    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)}

listdir() and readlink() are racy here.  Incoming migration cleanup may
still be closing fds at that point, so readlink can raise
FileNotFoundError. Skip fds that disappear with a try/except.

Andrey