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

[PATCH hci-8.0 v2 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

Changes in v2:
- 1/3: the cpr_claim_fds() comment in char.h says an override must call
  the parent's hook first (Andrey)
- 3/3: new case for a source that did not save the log fd: the target
  must not create the logfile path and must warn (Andrey)
- 3/3: _open_files() skips fds closed while /proc/<pid>/fd is read
  (Andrey)
- R-b on 1/3 and 2/3

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/chardev/char.h                        |   5 +-
 include/migration/cpr.h                       |   1 +
 tests/qemu-iotests/tests/migrate-cpr-exec     | 154 ++++++++++++++++++
 tests/qemu-iotests/tests/migrate-cpr-exec.out |   4 +-
 8 files changed, 202 insertions(+), 14 deletions(-)


base-commit: f2f4b3de1a1be3360a77ca6ab79762466d238804
-- 
2.53.0

[PATCH hci-8.0 v2 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>
Reviewed-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 chardev/char-file.c    |  3 +++
 chardev/char-pty.c     |  3 +++
 chardev/char-socket.c  |  3 +++
 chardev/char.c         | 13 +++++++------
 include/chardev/char.h |  5 ++++-
 5 files changed, 20 insertions(+), 7 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;
     }
diff --git a/include/chardev/char.h b/include/chardev/char.h
index 431d07ad3c6..968766fd5d5 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -305,7 +305,10 @@ struct ChardevClass {
     /* notify the backend of frontend open state */
     void (*chr_set_fe_open)(Chardev *chr, int fe_open);
 
-    /* (re-)register the backend's CPR fds, e.g. on a failed chardev-change */
+    /*
+     * (re-)register CPR fds, e.g. on a failed chardev-change; an override
+     * must call the parent's hook first
+     */
     void (*cpr_claim_fds)(Chardev *chr);
 
     /* handle various events */
-- 
2.53.0

[PATCH hci-8.0 v2 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>
Reviewed-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 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 hci-8.0 v2 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     | 154 ++++++++++++++++++
 tests/qemu-iotests/tests/migrate-cpr-exec.out |   4 +-
 2 files changed, 156 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/tests/migrate-cpr-exec b/tests/qemu-iotests/tests/migrate-cpr-exec
index 51e6cbcd659..2b30929561c 100755
--- a/tests/qemu-iotests/tests/migrate-cpr-exec
+++ b/tests/qemu-iotests/tests/migrate-cpr-exec
@@ -288,6 +288,160 @@ 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'
+        files = set()
+        for fd in os.listdir(fd_dir):
+            try:
+                files.add(os.readlink(os.path.join(fd_dir, fd)))
+            except FileNotFoundError:
+                pass
+        return files
+
+    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_chardev_logfile_not_preserved(self) -> None:
+        """A log the source did not save is dropped, not reopened by path"""
+        log = self._log_path('unsaved.log')
+        arg = 'pty,id=char0'
+
+        def add_log():
+            self._replace_arg(arg, f'{arg},logfile={log}')
+
+        def post_cpr():
+            self.assertFalse(os.path.exists(log))
+
+        self.vm.add_args('-chardev', arg,
+                         '-device', 'isa-serial,chardev=char0')
+        self._test_cpr(pre_cpr_hook=add_log, post_cpr_hook=post_cpr)
+        self.assertIn("chardev 'char0': log file was not preserved, "
+                      "continuing without it", self.vm.get_log())
+
     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..cfa5c0d0e66 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 35 tests
 
 OK
-- 
2.53.0

Re: [PATCH hci-8.0 v2 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/20261005092635.3564485-1-den@openvz.org?part=1

Re: [PATCH hci-8.0 v2 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: 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

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

[no-reply]

> 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>

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

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

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

[Andrey Drobyshev]

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

Applied to hci-dev


On 10/5/26 12:26 PM, Denis V. Lunev wrote:
> 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
> 
> Changes in v2:
> - 1/3: the cpr_claim_fds() comment in char.h says an override must call
>   the parent's hook first (Andrey)
> - 3/3: new case for a source that did not save the log fd: the target
>   must not create the logfile path and must warn (Andrey)
> - 3/3: _open_files() skips fds closed while /proc/<pid>/fd is read
>   (Andrey)
> - R-b on 1/3 and 2/3
> 
> 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/chardev/char.h                        |   5 +-
>  include/migration/cpr.h                       |   1 +
>  tests/qemu-iotests/tests/migrate-cpr-exec     | 154 ++++++++++++++++++
>  tests/qemu-iotests/tests/migrate-cpr-exec.out |   4 +-
>  8 files changed, 202 insertions(+), 14 deletions(-)
> 
> 
> base-commit: f2f4b3de1a1be3360a77ca6ab79762466d238804