- Devel - libvirt List Archives

[PATCH] vmx: Ensure unique disk targets when parsing
by Adam Julis 24 Jul '24

24 Jul '24

Disk targets were generated in virVMXParseConfig() with virVMXGenerateDiskTarget(). It works on combination of controller, fix offset, unit and prefix. While SCSI and SATA have same prefix "sd", function virVMXGenerateDiskTarget() could returned in some cases same targets. In this patch, after loaded SCSI and SATA disks to the def, it checks if in array exists any SATA disks after SCSI. If not, nothing happened. If yes, targets of all SATA disks are changed. Disk target is calculated as: last SCSI target value + n, where n is position of disk in array after last SCSI (1,2,..). Because assigned addresses of disks are generated from their indexes, for every changed SATA disk is called virDomainDiskDefAssignAddress() with the updated value. The corresponding tests have been modified to match the index changes. Signed-off-by: Adam Julis <ajulis(a)redhat.com> --- src/vmx/vmx.c | 32 ++++++++++++++++++++++++ tests/vmx2xmldata/esx-in-the-wild-12.xml | 4 +-- tests/vmx2xmldata/esx-in-the-wild-8.xml | 4 +-- 3 files changed, 36 insertions(+), 4 deletions(-) diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 227744d062..9fdf0f1cd4 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -1389,6 +1389,9 @@ virVMXParseConfig(virVMXContext *ctx, bool smbios_reflecthost = false; int controller; int bus; + int ndisk; + int last_scsi; + int offset = -1; int port; bool present; int scsi_virtualDev[4] = { -1, -1, -1, -1 }; @@ -1805,6 +1808,35 @@ virVMXParseConfig(virVMXContext *ctx, } } + /* now disks contain only SCSI and SATA, SATA could have same index (dst) as SCSI + * find last SCSI index in array and use it as offset for all SATA indexes + * (overwrite old values) + * finally, regenerate correct addresses, while it depends on the index */ + for (ndisk = 0; ndisk < def->ndisks; ndisk++) { + virDomainDiskDef *dsc = def->disks[ndisk]; + + if (dsc->bus == VIR_DOMAIN_DISK_BUS_SCSI) { + offset = virDiskNameToIndex(dsc->dst); + last_scsi = ndisk; + continue; + } + + if (offset > -1) { + VIR_FREE(def->disks[ndisk]->dst); + def->disks[ndisk]->dst = virIndexToDiskName(offset + ndisk - last_scsi, "sd"); + + if (virDomainDiskDefAssignAddress(NULL, def->disks[ndisk], def) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not assign address to disk '%1$s'"), + virDomainDiskGetSource(dsc)); + goto cleanup; + } + } + + else + break; + } + /* def:disks (ide) */ for (bus = 0; bus < 2; ++bus) { for (unit = 0; unit < 2; ++unit) { diff --git a/tests/vmx2xmldata/esx-in-the-wild-12.xml b/tests/vmx2xmldata/esx-in-the-wild-12.xml index 42184501d0..a7730845ee 100644 --- a/tests/vmx2xmldata/esx-in-the-wild-12.xml +++ b/tests/vmx2xmldata/esx-in-the-wild-12.xml @@ -21,9 +21,9 @@ <address type='drive' controller='0' bus='0' target='0' unit='0'/> </disk> <disk type='file' device='cdrom'> - <target dev='sda' bus='sata'/> + <target dev='sdb' bus='sata'/> <readonly/> - <address type='drive' controller='0' bus='0' target='0' unit='0'/> + <address type='drive' controller='0' bus='0' target='0' unit='1'/> </disk> <controller type='scsi' index='0' model='vmpvscsi'/> <controller type='sata' index='0'/> diff --git a/tests/vmx2xmldata/esx-in-the-wild-8.xml b/tests/vmx2xmldata/esx-in-the-wild-8.xml index 47d22ced2a..d5356bda34 100644 --- a/tests/vmx2xmldata/esx-in-the-wild-8.xml +++ b/tests/vmx2xmldata/esx-in-the-wild-8.xml @@ -36,9 +36,9 @@ </disk> <disk type='file' device='cdrom'> <source file='[692eb778-2d4937fe] CentOS-4.7.ServerCD-x86_64.iso'/> - <target dev='sda' bus='sata'/> + <target dev='sdd' bus='sata'/> <readonly/> - <address type='drive' controller='0' bus='0' target='0' unit='0'/> + <address type='drive' controller='0' bus='0' target='0' unit='3'/> </disk> <controller type='scsi' index='0' model='vmpvscsi'/> <controller type='sata' index='0'/> -- 2.45.2

2 1

[libvirt PATCH] qemu: virtiofs: cache: use 'never' instead of 'none'
by Ján Tomko 24 Jul '24

24 Jul '24

The new option style renamed one of the cache modes. Signed-off-by: Ján Tomko <jtomko(a)redhat.com> --- src/qemu/qemu_virtiofs.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c index 703f1226a2..db987ddfe1 100644 --- a/src/qemu/qemu_virtiofs.c +++ b/src/qemu/qemu_virtiofs.c @@ -147,10 +147,19 @@ qemuVirtioFSBuildCommandLine(virQEMUDriverConfig *cfg, virCommandAddArg(cmd, "--shared-dir"); virCommandAddArg(cmd, fs->src->path); - if (fs->cache) { + switch (fs->cache) { + case VIR_DOMAIN_FS_CACHE_MODE_DEFAULT: + break; + case VIR_DOMAIN_FS_CACHE_MODE_NONE: + virCommandAddArg(cmd, "--cache"); + virCommandAddArg(cmd, "never"); + break; + case VIR_DOMAIN_FS_CACHE_MODE_ALWAYS: virCommandAddArg(cmd, "--cache"); virCommandAddArg(cmd, virDomainFSCacheModeTypeToString(fs->cache)); + break; } + if (fs->sandbox) { virCommandAddArg(cmd, "--sandbox"); virCommandAddArg(cmd, virDomainFSSandboxModeTypeToString(fs->sandbox)); -- 2.45.2

2 1

[PATCH] meson: correct git detection
by John Levon 24 Jul '24

24 Jul '24

The current "building from git" test uses "test -d .git"; however, that doesn't work when libvirt is used as a submodule, as in that case .git is a normal file. Use "test -e .git" instead. Signed-off-by: John Levon <john.levon(a)nutanix.com> --- meson.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/meson.build b/meson.build index 3ab8e57e4a..231470e2ee 100644 --- a/meson.build +++ b/meson.build @@ -17,7 +17,7 @@ endif # figure out if we are building from git -git = run_command('test', '-d', '.git', check: false).returncode() == 0 +git = run_command('test', '-e', '.git', check: false).returncode() == 0 if git and not get_option('no_git') run_command('git', 'submodule', 'update', '--init', check: true) -- 2.34.1

2 1

[PATCH] virtiofs: rename member to 'openfiles' for clarity
by Adam Julis 24 Jul '24

24 Jul '24

New element 'openfiles' had confusing name. Since the patch with this new element wasn't propagate yet, old name ('rlimit_nofile') was changed. ... <binary> <openfiles max='122333'/> </binary> ... Signed-off-by: Adam Julis <ajulis(a)redhat.com> --- docs/formatdomain.rst | 6 +++--- src/conf/domain_conf.c | 14 +++++++------- src/conf/domain_conf.h | 2 +- src/conf/schemas/domaincommon.rng | 4 ++-- src/qemu/qemu_virtiofs.c | 4 ++-- ... vhost-user-fs-fd-openfiles.x86_64-latest.args} | 0 .../vhost-user-fs-fd-openfiles.x86_64-latest.xml | 1 + ...d-rlimit.xml => vhost-user-fs-fd-openfiles.xml} | 2 +- .../vhost-user-fs-fd-rlimit.x86_64-latest.xml | 1 - tests/qemuxmlconftest.c | 2 +- 10 files changed, 18 insertions(+), 18 deletions(-) rename tests/qemuxmlconfdata/{vhost-user-fs-fd-rlimit.x86_64-latest.args => vhost-user-fs-fd-openfiles.x86_64-latest.args} (100%) create mode 120000 tests/qemuxmlconfdata/vhost-user-fs-fd-openfiles.x86_64-latest.xml rename tests/qemuxmlconfdata/{vhost-user-fs-fd-rlimit.xml => vhost-user-fs-fd-openfiles.xml} (97%) delete mode 120000 tests/qemuxmlconfdata/vhost-user-fs-fd-rlimit.x86_64-latest.xml diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 10584dfe83..c2a6d0b910 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -3734,10 +3734,10 @@ A directory on the host that can be accessed directly from the guest. The thread pool helps increase the number of requests in flight when used with storage that has a higher latency. However, it has an overhead, and so for fast, low latency filesystems, it may be best to turn it off. ( :since:`Since 8.5.0` ) - Element ``rlimit_profile`` accepts one attribute ``size`` which defines the + Element ``openfiles`` accepts one attribute ``max`` which defines the maximum number of file descriptors. Non-positive values are forbidden. - Although numbers greater than 1M are allowed, the virtiofsd documentation - states that in this case its set by virtiofsd to the 1M. ( :since:`Since 10.6.0` ) + The upper bound on the number of open files is implementation defined. + ( :since:`Since 10.6.0` ) ``source`` The resource on the host that is being accessed in the guest. The ``name`` attribute must be used with ``type='template'``, and the ``dir`` attribute diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6733857a3a..d1c59f7a91 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8868,7 +8868,7 @@ virDomainFSDefParseXML(virDomainXMLOption *xmlopt, g_autofree char *queue_size = virXPathString("string(./driver/@queue)", ctxt); g_autofree char *binary = virXPathString("string(./binary/@path)", ctxt); g_autofree char *thread_pool_size = virXPathString("string(./binary/thread_pool/@size)", ctxt); - g_autofree char *rlimit_nofile = virXPathString("string(./binary/rlimit_nofile/@size)", ctxt); + g_autofree char *openfiles = virXPathString("string(./binary/openfiles/@max)", ctxt); xmlNodePtr binary_node = virXPathNode("./binary", ctxt); xmlNodePtr binary_lock_node = virXPathNode("./binary/lock", ctxt); xmlNodePtr binary_cache_node = virXPathNode("./binary/cache", ctxt); @@ -8892,11 +8892,11 @@ virDomainFSDefParseXML(virDomainXMLOption *xmlopt, goto error; } - if (rlimit_nofile && - virStrToLong_ull(rlimit_nofile, NULL, 10, &def->rlimit_nofile) < 0) { + if (openfiles && + virStrToLong_ull(openfiles, NULL, 10, &def->openfiles) < 0) { virReportError(VIR_ERR_XML_ERROR, - _("cannot parse rlimit_nofile '%1$s' for virtiofs"), - rlimit_nofile); + _("cannot parse openfiles '%1$s' for virtiofs"), + openfiles); goto error; } @@ -23424,8 +23424,8 @@ virDomainFSDefFormat(virBuffer *buf, if (def->thread_pool_size >= 0) virBufferAsprintf(&binaryBuf, "<thread_pool size='%d'/>\n", def->thread_pool_size); - if (def->rlimit_nofile > 0) - virBufferAsprintf(&binaryBuf, "<rlimit_nofile size='%lld'/>\n", def->rlimit_nofile); + if (def->openfiles > 0) + virBufferAsprintf(&binaryBuf, "<openfiles max='%lld'/>\n", def->openfiles); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8283493dfc..6972f6ae9b 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -890,7 +890,7 @@ struct _virDomainFSDef { bool symlinksResolved; char *binary; unsigned long long queue_size; - unsigned long long rlimit_nofile; + unsigned long long openfiles; virTristateSwitch xattr; virDomainFSCacheMode cache; virTristateSwitch posix_lock; diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index ab5374d5f0..8dae6416e9 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng @@ -3381,9 +3381,9 @@ </element> </optional> <optional> - <element name="rlimit_nofile"> + <element name="openfiles"> <optional> - <attribute name="size"> + <attribute name="max"> <data type="positiveInteger"/> </attribute> </optional> diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c index 703f1226a2..a8f2416273 100644 --- a/src/qemu/qemu_virtiofs.c +++ b/src/qemu/qemu_virtiofs.c @@ -194,8 +194,8 @@ qemuVirtioFSBuildCommandLine(virQEMUDriverConfig *cfg, if (fs->thread_pool_size >= 0) virCommandAddArgFormat(cmd, "--thread-pool-size=%i", fs->thread_pool_size); - if (fs->rlimit_nofile > 0) - virCommandAddArgFormat(cmd, "--rlimit-nofile=%llu", fs->rlimit_nofile); + if (fs->openfiles > 0) + virCommandAddArgFormat(cmd, "--rlimit-nofile=%llu", fs->openfiles); if (cfg->virtiofsdDebug) { if (virBitmapIsBitSet(fs->caps, QEMU_VHOST_USER_FS_FEATURE_SEPARATE_OPTIONS)) diff --git a/tests/qemuxmlconfdata/vhost-user-fs-fd-rlimit.x86_64-latest.args b/tests/qemuxmlconfdata/vhost-user-fs-fd-openfiles.x86_64-latest.args similarity index 100% rename from tests/qemuxmlconfdata/vhost-user-fs-fd-rlimit.x86_64-latest.args rename to tests/qemuxmlconfdata/vhost-user-fs-fd-openfiles.x86_64-latest.args diff --git a/tests/qemuxmlconfdata/vhost-user-fs-fd-openfiles.x86_64-latest.xml b/tests/qemuxmlconfdata/vhost-user-fs-fd-openfiles.x86_64-latest.xml new file mode 120000 index 0000000000..74761b0d0f --- /dev/null +++ b/tests/qemuxmlconfdata/vhost-user-fs-fd-openfiles.x86_64-latest.xml @@ -0,0 +1 @@ +vhost-user-fs-fd-openfiles.xml \ No newline at end of file diff --git a/tests/qemuxmlconfdata/vhost-user-fs-fd-rlimit.xml b/tests/qemuxmlconfdata/vhost-user-fs-fd-openfiles.xml similarity index 97% rename from tests/qemuxmlconfdata/vhost-user-fs-fd-rlimit.xml rename to tests/qemuxmlconfdata/vhost-user-fs-fd-openfiles.xml index 2983d3f275..3e925a8c8b 100644 --- a/tests/qemuxmlconfdata/vhost-user-fs-fd-rlimit.xml +++ b/tests/qemuxmlconfdata/vhost-user-fs-fd-openfiles.xml @@ -32,7 +32,7 @@ <cache mode='always'/> <sandbox mode='chroot'/> <thread_pool size='16'/> - <rlimit_nofile size='122333'/> + <openfiles max='122333'/> </binary> <idmap> <uid start='0' target='100000' count='65535'/> diff --git a/tests/qemuxmlconfdata/vhost-user-fs-fd-rlimit.x86_64-latest.xml b/tests/qemuxmlconfdata/vhost-user-fs-fd-rlimit.x86_64-latest.xml deleted file mode 120000 index 68c4e8a482..0000000000 --- a/tests/qemuxmlconfdata/vhost-user-fs-fd-rlimit.x86_64-latest.xml +++ /dev/null @@ -1 +0,0 @@ -vhost-user-fs-fd-rlimit.xml \ No newline at end of file diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c index cc984440ea..bf88bd2f8d 100644 --- a/tests/qemuxmlconftest.c +++ b/tests/qemuxmlconftest.c @@ -2816,7 +2816,7 @@ mymain(void) DO_TEST_CAPS_ARCH_LATEST("launch-security-s390-pv", "s390x"); DO_TEST_CAPS_LATEST("vhost-user-fs-fd-memory"); - DO_TEST_CAPS_LATEST("vhost-user-fs-fd-rlimit"); + DO_TEST_CAPS_LATEST("vhost-user-fs-fd-openfiles"); DO_TEST_CAPS_LATEST("vhost-user-fs-hugepages"); DO_TEST_CAPS_LATEST_PARSE_ERROR("vhost-user-fs-readonly"); -- 2.45.2

2 1

[PATCH] qemu_hotplug: Do not allow absent values in rom settings
by Kristina Hanicova 24 Jul '24

24 Jul '24

If there are absent values in an already existing element specifying rom settings, we simply use the old ones. This behaviour is not desired, as users might think that deleting the element from XML would delete the setting (because the hotplug succeeds) - which does not happen. Because of that, we should not accept an interface without elements that cannot be changed. Therefore, we should not allow absent values for already existing rom setting during hotplug. Resolves: https://issues.redhat.com/browse/RHEL-7109 Signed-off-by: Kristina Hanicova <khanicov(a)redhat.com> --- src/qemu/qemu_hotplug.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1f4620d833..7f158ddcd5 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3838,16 +3838,12 @@ qemuDomainChangeNet(virQEMUDriver *driver, /* device alias is checked already in virDomainDefCompatibleDevice */ - if (newdev->info.rombar == VIR_TRISTATE_SWITCH_ABSENT) - newdev->info.rombar = olddev->info.rombar; if (olddev->info.rombar != newdev->info.rombar) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("cannot modify network device rom bar setting")); goto cleanup; } - if (!newdev->info.romfile) - newdev->info.romfile = g_strdup(olddev->info.romfile); if (STRNEQ_NULLABLE(olddev->info.romfile, newdev->info.romfile)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("cannot modify network rom file")); @@ -3860,8 +3856,6 @@ qemuDomainChangeNet(virQEMUDriver *driver, goto cleanup; } - if (newdev->info.romenabled == VIR_TRISTATE_BOOL_ABSENT) - newdev->info.romenabled = olddev->info.romenabled; if (olddev->info.romenabled != newdev->info.romenabled) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("cannot modify network device rom enabled setting")); -- 2.45.1

2 1

[PATCH v3] qemu: add a monitor to /proc/$pid when killing times out
by Boris Fiuczynski 24 Jul '24

24 Jul '24

In cases when a QEMU process takes longer than the time sigterm and sigkill are issued to kill the process do not simply fail and leave the VM in state VIR_DOMAIN_SHUTDOWN until the daemon stops. Instead set up an fd on /proc/$pid and get notified when the QEMU process finally has terminated to cleanup the VM state. Resolves: https://issues.redhat.com/browse/RHEL-28819 Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com> --- src/qemu/qemu_domain.c | 8 +++ src/qemu/qemu_domain.h | 2 + src/qemu/qemu_driver.c | 18 ++++++ src/qemu/qemu_process.c | 124 ++++++++++++++++++++++++++++++++++++++-- src/qemu/qemu_process.h | 1 + 5 files changed, 148 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2134b11038..8147ff02fd 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1889,6 +1889,11 @@ qemuDomainObjPrivateFree(void *data) virChrdevFree(priv->devs); + if (priv->pidMonitored >= 0) { + virEventRemoveHandle(priv->pidMonitored); + priv->pidMonitored = -1; + } + /* This should never be non-NULL if we get here, but just in case... */ if (priv->mon) { VIR_ERROR(_("Unexpected QEMU monitor still active during domain deletion")); @@ -1934,6 +1939,8 @@ qemuDomainObjPrivateAlloc(void *opaque) priv->blockjobs = virHashNew(virObjectUnref); priv->fds = virHashNew(g_object_unref); + priv->pidMonitored = -1; + /* agent commands block by default, user can choose different behavior */ priv->agentTimeout = VIR_DOMAIN_AGENT_RESPONSE_TIMEOUT_BLOCK; priv->migMaxBandwidth = QEMU_DOMAIN_MIG_BANDWIDTH_MAX; @@ -11680,6 +11687,7 @@ qemuProcessEventFree(struct qemuProcessEvent *event) case QEMU_PROCESS_EVENT_RESET: case QEMU_PROCESS_EVENT_NBDKIT_EXITED: case QEMU_PROCESS_EVENT_MONITOR_EOF: + case QEMU_PROCESS_EVENT_SHUTDOWN_COMPLETED: case QEMU_PROCESS_EVENT_LAST: break; } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index d777559119..a5092dd7f0 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -119,6 +119,7 @@ struct _qemuDomainObjPrivate { bool beingDestroyed; char *pidfile; + int pidMonitored; virDomainPCIAddressSet *pciaddrs; virDomainUSBAddressSet *usbaddrs; @@ -469,6 +470,7 @@ typedef enum { QEMU_PROCESS_EVENT_UNATTENDED_MIGRATION, QEMU_PROCESS_EVENT_RESET, QEMU_PROCESS_EVENT_NBDKIT_EXITED, + QEMU_PROCESS_EVENT_SHUTDOWN_COMPLETED, QEMU_PROCESS_EVENT_LAST } qemuProcessEventType; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9f3013e231..6b1e4084f6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4041,6 +4041,21 @@ processNbdkitExitedEvent(virDomainObj *vm, } +static void +processShutdownCompletedEvent(virQEMUDriver *driver, + virDomainObj *vm) +{ + if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0) + return; + + if (virDomainObjIsActive(vm)) + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_UNKNOWN, + VIR_ASYNC_JOB_NONE, 0); + + virDomainObjEndJob(vm); +} + + static void qemuProcessEventHandler(void *data, void *opaque) { struct qemuProcessEvent *processEvent = data; @@ -4101,6 +4116,9 @@ static void qemuProcessEventHandler(void *data, void *opaque) case QEMU_PROCESS_EVENT_NBDKIT_EXITED: processNbdkitExitedEvent(vm, processEvent->data); break; + case QEMU_PROCESS_EVENT_SHUTDOWN_COMPLETED: + processShutdownCompletedEvent(driver, vm); + break; case QEMU_PROCESS_EVENT_LAST: break; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 25dfd04272..c6f7d34168 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -25,6 +25,9 @@ #include <unistd.h> #include <signal.h> #include <sys/stat.h> +#if WITH_SYS_SYSCALL_H +# include <sys/syscall.h> +#endif #if defined(__linux__) # include <linux/capability.h> #elif defined(__FreeBSD__) @@ -8387,9 +8390,114 @@ qemuProcessCreatePretendCmdBuild(virDomainObj *vm, } +#if WITH_SYS_SYSCALL_H && defined(SYS_pidfd_open) +typedef struct { + virDomainObj *vm; + int pidfd; +} qemuProcessInShutdownEventData; + + +static qemuProcessInShutdownEventData* +qemuProcessInShutdownEventDataNew(virDomainObj *vm, int pidfd) +{ + qemuProcessInShutdownEventData *d = g_new(qemuProcessInShutdownEventData, 1); + d->vm = virObjectRef(vm); + d->pidfd = pidfd; + return d; +} + + +static void +qemuProcessInShutdownEventDataFree(qemuProcessInShutdownEventData *d) +{ + virObjectUnref(d->vm); + VIR_FORCE_CLOSE(d->pidfd); + g_free(d); +} + + +static void +qemuProcessInShutdownPidfdCb(int watch, + int fd, + int events G_GNUC_UNUSED, + void *opaque) +{ + qemuProcessInShutdownEventData *data = opaque; + virDomainObj *vm = data->vm; + + VIR_DEBUG("vm=%p name=%s pid=%lld fd=%d", + vm, vm->def->name, (long long)vm->pid, fd); + + virEventRemoveHandle(watch); + + virObjectLock(vm); + + VIR_INFO("QEMU process %lld finally completed termination", + (long long)vm->pid); + + QEMU_DOMAIN_PRIVATE(vm)->pidMonitored = -1; + qemuProcessEventSubmit(vm, QEMU_PROCESS_EVENT_SHUTDOWN_COMPLETED, + 0, 0, NULL); + + virObjectUnlock(vm); +} +#endif /* WITH_SYS_SYSCALL_H && defined(SYS_pidfd_open) */ + + +static int +qemuProcessInShutdownStartMonitor(virDomainObj *vm) +{ +#if WITH_SYS_SYSCALL_H && defined(SYS_pidfd_open) + qemuDomainObjPrivate *priv = vm->privateData; + qemuProcessInShutdownEventData *data; + int pidfd; + int ret = -1; + + VIR_DEBUG("vm=%p name=%s pid=%lld pidMonitored=%d", + vm, vm->def->name, (long long)vm->pid, + priv->pidMonitored); + + if (priv->pidMonitored >= 0) { + VIR_DEBUG("Monitoring qemu in-shutdown process %i already set up", vm->pid); + goto cleanup; + } + + pidfd = syscall(SYS_pidfd_open, vm->pid, 0); + if (pidfd < 0) { + if (errno == ESRCH) /* process has already terminated */ + ret = 1; + goto cleanup; + } + + data = qemuProcessInShutdownEventDataNew(vm, pidfd); + if ((priv->pidMonitored = virEventAddHandle(pidfd, + VIR_EVENT_HANDLE_READABLE, + qemuProcessInShutdownPidfdCb, + data, + (virFreeCallback)qemuProcessInShutdownEventDataFree)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to monitor qemu in-shutdown process %1$i"), + vm->pid); + qemuProcessInShutdownEventDataFree(data); + goto cleanup; + } + VIR_DEBUG("Monitoring qemu in-shutdown process %i for termination", vm->pid); + ret = 0; + + cleanup: + return ret; +#else /* !WITH_SYS_SYSCALL_H || !defined(SYS_pidfd_open) */ + VIR_DEBUG("Monitoring qemu process %i not implemented", vm->pid); + return -1; +#endif /* !WITH_SYS_SYSCALL_H || !defined(SYS_pidfd_open) */ +} + + int qemuProcessKill(virDomainObj *vm, unsigned int flags) { + int ret = -1; + VIR_DEBUG("vm=%p name=%s pid=%lld flags=0x%x", vm, vm->def->name, (long long)vm->pid, flags); @@ -8410,10 +8518,16 @@ qemuProcessKill(virDomainObj *vm, unsigned int flags) /* Request an extra delay of two seconds per current nhostdevs * to be safe against stalls by the kernel freeing up the resources */ - return virProcessKillPainfullyDelay(vm->pid, - !!(flags & VIR_QEMU_PROCESS_KILL_FORCE), - vm->def->nhostdevs * 2, - false); + ret = virProcessKillPainfullyDelay(vm->pid, + !!(flags & VIR_QEMU_PROCESS_KILL_FORCE), + vm->def->nhostdevs * 2, + false); + + if (ret < 0 && (flags & VIR_QEMU_PROCESS_KILL_MONITOR_ON_ERROR)) + if (qemuProcessInShutdownStartMonitor(vm) == 1) + ret = 0; /* process termination detected */ + + return ret; } @@ -8438,7 +8552,7 @@ qemuProcessBeginStopJob(virDomainObj *vm, * cleared inside qemuProcessStop */ priv->beingDestroyed = true; - if (qemuProcessKill(vm, killFlags) < 0) + if (qemuProcessKill(vm, killFlags|VIR_QEMU_PROCESS_KILL_MONITOR_ON_ERROR) < 0) goto error; /* Wake up anything waiting on domain condition */ diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index cb67bfcd2d..2324aeb7bd 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -180,6 +180,7 @@ typedef enum { VIR_QEMU_PROCESS_KILL_FORCE = 1 << 0, VIR_QEMU_PROCESS_KILL_NOWAIT = 1 << 1, VIR_QEMU_PROCESS_KILL_NOCHECK = 1 << 2, /* bypass the running vm check */ + VIR_QEMU_PROCESS_KILL_MONITOR_ON_ERROR = 1 << 3, /* on error enable process monitor */ } virQemuProcessKillMode; int qemuProcessKill(virDomainObj *vm, unsigned int flags); -- 2.45.0

3 5

[PATCH v3] virsh: Provide completer for some pool-X-as commands
by Abhiram Tilak 24 Jul '24

24 Jul '24

From: notpua <atp(a)tutamail.com> Provides completers for auth-type and source-format commands for virsh pool-create-as and pool-define-as commands. Use Empty completers for options where completions are not required. Related Issue: https://gitlab.com/libvirt/libvirt/-/issues/9 Signed-off-by: Abhiram Tilak <atp.exp(a)gmail.com> --- Changes in v2: - Fix all formatting errors - Change some options using Empty completers, to use LocalPath completers. - Add completers for AdapterName and AdapterParent using information from node devices. Changes in v3: - Call virshNodeDeviceNameComplete with modified flags instead of duplicating the whole implementation. src/libvirt_private.syms | 2 ++ tools/virsh-completer-pool.c | 70 ++++++++++++++++++++++++++++++++++++ tools/virsh-completer-pool.h | 20 +++++++++++ tools/virsh-pool.c | 9 +++++ 4 files changed, 101 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c35366c9e1..6ba1e8e2c5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1123,6 +1123,8 @@ virStorageAuthDefCopy; virStorageAuthDefFormat; virStorageAuthDefFree; virStorageAuthDefParse; +virStorageAuthTypeFromString; +virStorageAuthTypeToString; virStorageFileFeatureTypeFromString; virStorageFileFeatureTypeToString; virStorageFileFormatTypeFromString; diff --git a/tools/virsh-completer-pool.c b/tools/virsh-completer-pool.c index 3568bb985b..ba7855fdba 100644 --- a/tools/virsh-completer-pool.c +++ b/tools/virsh-completer-pool.c @@ -23,6 +23,7 @@ #include "virsh-completer-pool.h" #include "virsh-util.h" #include "conf/storage_conf.h" +#include "conf/node_device_conf.h" #include "virsh-pool.h" #include "virsh.h" @@ -106,3 +107,72 @@ virshPoolTypeCompleter(vshControl *ctl, return virshCommaStringListComplete(type_str, (const char **)tmp); } + + +char ** +virshPoolFormatCompleter(vshControl *ctl G_GNUC_UNUSED, + const vshCmd *cmd G_GNUC_UNUSED, + unsigned int flags) +{ + size_t i = 0; + size_t j = 0; + g_auto(GStrv) tmp = NULL; + size_t nformats = VIR_STORAGE_POOL_FS_LAST + VIR_STORAGE_POOL_NETFS_LAST + + VIR_STORAGE_POOL_DISK_LAST + VIR_STORAGE_POOL_LOGICAL_LAST; + + virCheckFlags(0, NULL); + + tmp = g_new0(char *, nformats + 1); + + /* Club all PoolFormats for completion */ + for (i = 0; i < VIR_STORAGE_POOL_FS_LAST; i++) + tmp[j++] = g_strdup(virStoragePoolFormatFileSystemTypeToString(i)); + + for (i = 0; i < VIR_STORAGE_POOL_NETFS_LAST; i++) + tmp[j++] = g_strdup(virStoragePoolFormatFileSystemNetTypeToString(i)); + + for (i = 1; i < VIR_STORAGE_POOL_DISK_LAST; i++) + tmp[j++] = g_strdup(virStoragePoolFormatDiskTypeToString(i)); + + for (i = 1; i < VIR_STORAGE_POOL_LOGICAL_LAST; i++) + tmp[j++] = g_strdup(virStoragePoolFormatLogicalTypeToString(i)); + + return g_steal_pointer(&tmp); +} + + +char ** +virshPoolAuthTypeCompleter(vshControl *ctl G_GNUC_UNUSED, + const vshCmd *cmd G_GNUC_UNUSED, + unsigned int flags) +{ + size_t i = 0; + g_auto(GStrv) tmp = NULL; + + virCheckFlags(0, NULL); + + tmp = g_new0(char *, VIR_STORAGE_AUTH_TYPE_LAST + 1); + + for (i = 0; i < VIR_STORAGE_AUTH_TYPE_LAST; i++) + tmp[i] = g_strdup(virStorageAuthTypeToString(i)); + + return g_steal_pointer(&tmp); +} + + +char ** +virshAdapterNameCompleter(vshControl *ctl, + const vshCmd *cmd, unsigned int flags) +{ + flags |= VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI_HOST; + return virshNodeDeviceNameCompleter(ctl, cmd, flags); +} + + +char ** +virshAdapterParentCompleter(vshControl *ctl, + const vshCmd *cmd, unsigned int flags) +{ + flags |= VIR_CONNECT_LIST_NODE_DEVICES_CAP_VPORTS; + return virshNodeDeviceNameCompleter(ctl, cmd, flags); +} diff --git a/tools/virsh-completer-pool.h b/tools/virsh-completer-pool.h index bff3e5742b..eccc08a73f 100644 --- a/tools/virsh-completer-pool.h +++ b/tools/virsh-completer-pool.h @@ -40,3 +40,23 @@ char ** virshPoolTypeCompleter(vshControl *ctl, const vshCmd *cmd, unsigned int flags); + +char ** +virshPoolFormatCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags); + +char ** +virshPoolAuthTypeCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags); + +char ** +virshAdapterNameCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags); + +char ** +virshAdapterParentCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags); diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index f9aad8ded0..0cbd1417e6 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -80,31 +80,37 @@ {.name = "source-path", \ .type = VSH_OT_STRING, \ .unwanted_positional = true, \ + .completer = virshCompletePathLocalExisting, \ .help = N_("source path for underlying storage") \ }, \ {.name = "source-dev", \ .type = VSH_OT_STRING, \ .unwanted_positional = true, \ + .completer = virshCompletePathLocalExisting, \ .help = N_("source device for underlying storage") \ }, \ {.name = "source-name", \ .type = VSH_OT_STRING, \ .unwanted_positional = true, \ + .completer = virshCompleteEmpty, \ .help = N_("source name for underlying storage") \ }, \ {.name = "target", \ .type = VSH_OT_STRING, \ .unwanted_positional = true, \ + .completer = virshCompletePathLocalExisting, \ .help = N_("target for underlying storage") \ }, \ {.name = "source-format", \ .type = VSH_OT_STRING, \ .unwanted_positional = true, \ + .completer = virshPoolFormatCompleter, \ .help = N_("format for underlying storage") \ }, \ {.name = "auth-type", \ .type = VSH_OT_STRING, \ .unwanted_positional = true, \ + .completer = virshPoolAuthTypeCompleter, \ .help = N_("auth type to be used for underlying storage") \ }, \ {.name = "auth-username", \ @@ -126,6 +132,7 @@ {.name = "adapter-name", \ .type = VSH_OT_STRING, \ .unwanted_positional = true, \ + .completer = virshAdapterNameCompleter, \ .help = N_("adapter name to be used for underlying storage") \ }, \ {.name = "adapter-wwnn", \ @@ -141,6 +148,7 @@ {.name = "adapter-parent", \ .type = VSH_OT_STRING, \ .unwanted_positional = true, \ + .completer = virshAdapterParentCompleter, \ .help = N_("adapter parent scsi_hostN to be used for underlying vHBA storage") \ }, \ {.name = "adapter-parent-wwnn", \ @@ -161,6 +169,7 @@ {.name = "source-protocol-ver", \ .type = VSH_OT_STRING, \ .unwanted_positional = true, \ + .completer = virshCompleteEmpty, \ .help = N_("nfsvers value for NFS pool mount option") \ }, \ {.name = "source-initiator", \ -- 2.39.2

2 1

[PATCH v2] virsh: Provide completer for some pool-X-as commands
by Abhiram Tilak 23 Jul '24

23 Jul '24

Provides completers for auth-type and source-format commands for virsh pool-create-as and pool-define-as commands. Use Empty completers for options where completions are not required. Related Issue: https://gitlab.com/libvirt/libvirt/-/issues/9 Signed-off-by: Abhiram Tilak <atp.exp(a)gmail.com> --- Changes in v2: - Fix all formatting errors - Change some options using Empty completers, to use LocalPath completers. - Add completers for AdapterName and AdapterParent using information from node devices. src/libvirt_private.syms | 2 + tools/virsh-completer-pool.c | 128 +++++++++++++++++++++++++++++++++++ tools/virsh-completer-pool.h | 20 ++++++ tools/virsh-pool.c | 9 +++ 4 files changed, 159 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f0f7aa8654..fcb0ef7afe 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1117,6 +1117,8 @@ virStorageAuthDefCopy; virStorageAuthDefFormat; virStorageAuthDefFree; virStorageAuthDefParse; +virStorageAuthTypeFromString; +virStorageAuthTypeToString; virStorageFileFeatureTypeFromString; virStorageFileFeatureTypeToString; virStorageFileFormatTypeFromString; diff --git a/tools/virsh-completer-pool.c b/tools/virsh-completer-pool.c index 3568bb985b..7db2a20347 100644 --- a/tools/virsh-completer-pool.c +++ b/tools/virsh-completer-pool.c @@ -23,6 +23,7 @@ #include "virsh-completer-pool.h" #include "virsh-util.h" #include "conf/storage_conf.h" +#include "conf/node_device_conf.h" #include "virsh-pool.h" #include "virsh.h" @@ -106,3 +107,130 @@ virshPoolTypeCompleter(vshControl *ctl, return virshCommaStringListComplete(type_str, (const char **)tmp); } + + +char ** +virshPoolFormatCompleter(vshControl *ctl G_GNUC_UNUSED, + const vshCmd *cmd G_GNUC_UNUSED, + unsigned int flags) +{ + size_t i = 0; + size_t j = 0; + g_auto(GStrv) tmp = NULL; + size_t nformats = VIR_STORAGE_POOL_FS_LAST + VIR_STORAGE_POOL_NETFS_LAST + + VIR_STORAGE_POOL_DISK_LAST + VIR_STORAGE_POOL_LOGICAL_LAST; + + virCheckFlags(0, NULL); + + tmp = g_new0(char *, nformats + 1); + + /* Club all PoolFormats for completion */ + for (i = 0; i < VIR_STORAGE_POOL_FS_LAST; i++) + tmp[j++] = g_strdup(virStoragePoolFormatFileSystemTypeToString(i)); + + for (i = 0; i < VIR_STORAGE_POOL_NETFS_LAST; i++) + tmp[j++] = g_strdup(virStoragePoolFormatFileSystemNetTypeToString(i)); + + for (i = 1; i < VIR_STORAGE_POOL_DISK_LAST; i++) + tmp[j++] = g_strdup(virStoragePoolFormatDiskTypeToString(i)); + + for (i = 1; i < VIR_STORAGE_POOL_LOGICAL_LAST; i++) + tmp[j++] = g_strdup(virStoragePoolFormatLogicalTypeToString(i)); + + return g_steal_pointer(&tmp); +} + + +char ** +virshPoolAuthTypeCompleter(vshControl *ctl G_GNUC_UNUSED, + const vshCmd *cmd G_GNUC_UNUSED, + unsigned int flags) +{ + size_t i = 0; + g_auto(GStrv) tmp = NULL; + + virCheckFlags(0, NULL); + + tmp = g_new0(char *, VIR_STORAGE_AUTH_TYPE_LAST + 1); + + for (i = 0; i < VIR_STORAGE_AUTH_TYPE_LAST; i++) + tmp[i] = g_strdup(virStorageAuthTypeToString(i)); + + return g_steal_pointer(&tmp); +} + + +char ** +virshAdapterNameCompleter(vshControl *ctl, + const vshCmd *cmd G_GNUC_UNUSED, + unsigned int flags) +{ + virshControl *priv = ctl->privData; + virNodeDevicePtr *devs = NULL; + int ndevs = 0; + size_t i = 0; + char **ret = NULL; + g_auto(GStrv) tmp = NULL; + + virCheckFlags(0, NULL); + + if (!priv->conn || virConnectIsAlive(priv->conn) <= 0) + return NULL; + + flags |= VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI_HOST; + if ((ndevs = virConnectListAllNodeDevices(priv->conn, &devs, flags)) < 0) + return NULL; + + tmp = g_new0(char *, ndevs + 1); + + for (i = 0; i < ndevs; i++) { + const char *name = virNodeDeviceGetName(devs[i]); + + tmp[i] = g_strdup(name); + } + + ret = g_steal_pointer(&tmp); + + for (i = 0; i < ndevs; i++) + virshNodeDeviceFree(devs[i]); + g_free(devs); + return ret; +} + + +char ** +virshAdapterParentCompleter(vshControl *ctl, + const vshCmd *cmd G_GNUC_UNUSED, + unsigned int flags) +{ + virshControl *priv = ctl->privData; + virNodeDevicePtr *devs = NULL; + int ndevs = 0; + size_t i = 0; + char **ret = NULL; + g_auto(GStrv) tmp = NULL; + + virCheckFlags(0, NULL); + + if (!priv->conn || virConnectIsAlive(priv->conn) <= 0) + return NULL; + + flags |= VIR_CONNECT_LIST_NODE_DEVICES_CAP_VPORTS; + if ((ndevs = virConnectListAllNodeDevices(priv->conn, &devs, flags)) < 0) + return NULL; + + tmp = g_new0(char *, ndevs + 1); + + for (i = 0; i < ndevs; i++) { + const char *name = virNodeDeviceGetName(devs[i]); + + tmp[i] = g_strdup(name); + } + + ret = g_steal_pointer(&tmp); + + for (i = 0; i < ndevs; i++) + virshNodeDeviceFree(devs[i]); + g_free(devs); + return ret; +} diff --git a/tools/virsh-completer-pool.h b/tools/virsh-completer-pool.h index bff3e5742b..eccc08a73f 100644 --- a/tools/virsh-completer-pool.h +++ b/tools/virsh-completer-pool.h @@ -40,3 +40,23 @@ char ** virshPoolTypeCompleter(vshControl *ctl, const vshCmd *cmd, unsigned int flags); + +char ** +virshPoolFormatCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags); + +char ** +virshPoolAuthTypeCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags); + +char ** +virshAdapterNameCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags); + +char ** +virshAdapterParentCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags); diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index f9aad8ded0..0cbd1417e6 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -80,31 +80,37 @@ {.name = "source-path", \ .type = VSH_OT_STRING, \ .unwanted_positional = true, \ + .completer = virshCompletePathLocalExisting, \ .help = N_("source path for underlying storage") \ }, \ {.name = "source-dev", \ .type = VSH_OT_STRING, \ .unwanted_positional = true, \ + .completer = virshCompletePathLocalExisting, \ .help = N_("source device for underlying storage") \ }, \ {.name = "source-name", \ .type = VSH_OT_STRING, \ .unwanted_positional = true, \ + .completer = virshCompleteEmpty, \ .help = N_("source name for underlying storage") \ }, \ {.name = "target", \ .type = VSH_OT_STRING, \ .unwanted_positional = true, \ + .completer = virshCompletePathLocalExisting, \ .help = N_("target for underlying storage") \ }, \ {.name = "source-format", \ .type = VSH_OT_STRING, \ .unwanted_positional = true, \ + .completer = virshPoolFormatCompleter, \ .help = N_("format for underlying storage") \ }, \ {.name = "auth-type", \ .type = VSH_OT_STRING, \ .unwanted_positional = true, \ + .completer = virshPoolAuthTypeCompleter, \ .help = N_("auth type to be used for underlying storage") \ }, \ {.name = "auth-username", \ @@ -126,6 +132,7 @@ {.name = "adapter-name", \ .type = VSH_OT_STRING, \ .unwanted_positional = true, \ + .completer = virshAdapterNameCompleter, \ .help = N_("adapter name to be used for underlying storage") \ }, \ {.name = "adapter-wwnn", \ @@ -141,6 +148,7 @@ {.name = "adapter-parent", \ .type = VSH_OT_STRING, \ .unwanted_positional = true, \ + .completer = virshAdapterParentCompleter, \ .help = N_("adapter parent scsi_hostN to be used for underlying vHBA storage") \ }, \ {.name = "adapter-parent-wwnn", \ @@ -161,6 +169,7 @@ {.name = "source-protocol-ver", \ .type = VSH_OT_STRING, \ .unwanted_positional = true, \ + .completer = virshCompleteEmpty, \ .help = N_("nfsvers value for NFS pool mount option") \ }, \ {.name = "source-initiator", \ -- 2.39.2

3 2

[PATCH] qemuProcessStop: Don't unlock domain until most of the cleanup is done
by Michal Privoznik 23 Jul '24

23 Jul '24

Imagine two threads. Thread A is executing qemuProcessStop() and thread B is executing qemuDomainCreateXML(). To make things more interesting, the domain XML passed to qemuDomainCreateXML matches name + UUID of that the virDomainObj that qemuProcessStop() is currently working with. Here's what happens. 1) Thread A locks @vm, enters qemuProcessStop(). 2) Thread B parses given XML, calls virDomainObjListAdd() -> virDomainObjListAdd() -> virDomainObjListAddLocked() -> virDomainObjListFindByUUIDLocked(). Since there's a match on UUID, an virDomainObj object is returned and the thread proceeds to calling virObjectLock(). NB, it's the same object as in thread A. 3) Thread A sets vm->def->id = -1; this means that from this point on, virDomainObjIsActive() will return false. 4) Thread A calls qemuDomainObjStopWorker() which unlocks the @vm. 5) Thread B acquires the @vm lock and since virDomainObjIsActive() returns false, it proceeds to calling virDomainObjAssignDef() where vm->def is replaced. 6) Thread B then calls qemuProcessBeginJob() which unlocks the @vm temporarily. 7) Thread A, still in qemuDomainObjStopWorker() acquires @vm lock and proceeds with cleanup. 8) Thread A finds different definition than the one needing cleanup. In my testing I've seen stale pointers, e.g. vm->def->nets[0]->priv was NULL, which lead to a SIGSEGV as there's 'QEMU_DOMAIN_NETWORK_PRIVATE(net)->created' line when cleaning up nets. Your mileage may vary. Even if we did not crash, the plain fact that vm->def is changed in the middle of qemuProcessStop() means we might be cleaning up something else than intended. As a fix, I'm moving all lines that obviously touch vm->def before the domain object is unlocked. That left virHookCall(VIR_HOOK_QEMU_OP_STOPPED, VIR_HOOK_SUBOP_END) nearly next to virHookCall(VIR_HOOK_QEMU_OP_RELEASE, VIR_HOOK_SUBOP_END) which I figured is not something we want. So I've shuffled things a bit more. Fixes: 3865410e7f67ca4ec66e9a905e75f452762a97f0 Resolves: https://issues.redhat.com/browse/RHEL-49607 Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- src/qemu/qemu_process.c | 122 ++++++++++++++++++++-------------------- 1 file changed, 62 insertions(+), 60 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 25dfd04272..9ea6c678b8 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8530,6 +8530,18 @@ void qemuProcessStop(virQEMUDriver *driver, VIR_QEMU_PROCESS_KILL_FORCE| VIR_QEMU_PROCESS_KILL_NOCHECK)); + vm->pid = 0; + + /* now that we know it's stopped call the hook if present */ + if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { + g_autofree char *xml = qemuDomainDefFormatXML(driver, NULL, vm->def, 0); + + /* we can't stop the operation even if the script raised an error */ + ignore_value(virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, + VIR_HOOK_QEMU_OP_STOPPED, VIR_HOOK_SUBOP_END, + NULL, xml, NULL)); + } + if (priv->agent) { g_clear_pointer(&priv->agent, qemuAgentClose); } @@ -8553,25 +8565,6 @@ void qemuProcessStop(virQEMUDriver *driver, qemuDBusStop(driver, vm); - /* Only after this point we can reset 'priv->beingDestroyed' so that - * there's no point at which the VM could be considered as alive between - * entering the destroy job and this point where the active "flag" is - * cleared. - */ - vm->def->id = -1; - priv->beingDestroyed = false; - - /* Wake up anything waiting on domain condition */ - virDomainObjBroadcast(vm); - - /* IMPORTANT: qemuDomainObjStopWorker() unlocks @vm in order to prevent - * deadlocks with the per-VM event loop thread. This MUST be done after - * marking the VM as dead */ - qemuDomainObjStopWorker(vm); - - if (!!g_atomic_int_dec_and_test(&driver->nactive) && driver->inhibitCallback) - driver->inhibitCallback(false, driver->inhibitOpaque); - /* Clear network bandwidth */ virDomainClearNetBandwidth(vm->def); @@ -8588,18 +8581,6 @@ void qemuProcessStop(virQEMUDriver *driver, } } - virPortAllocatorRelease(priv->nbdPort); - priv->nbdPort = 0; - - if (priv->monConfig) { - if (priv->monConfig->type == VIR_DOMAIN_CHR_TYPE_UNIX) - unlink(priv->monConfig->data.nix.path); - g_clear_pointer(&priv->monConfig, virObjectUnref); - } - - /* Remove the master key */ - qemuDomainMasterKeyRemove(priv); - ignore_value(virDomainChrDefForeach(vm->def, false, qemuProcessCleanupChardevDevice, @@ -8609,22 +8590,6 @@ void qemuProcessStop(virQEMUDriver *driver, /* Its namespace is also gone then. */ qemuDomainDestroyNamespace(driver, vm); - virFileDeleteTree(priv->libDir); - virFileDeleteTree(priv->channelTargetDir); - - /* Stop autodestroy in case guest is restarted */ - virCloseCallbacksDomainRemove(vm, NULL, qemuProcessAutoDestroy); - - /* now that we know it's stopped call the hook if present */ - if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { - g_autofree char *xml = qemuDomainDefFormatXML(driver, NULL, vm->def, 0); - - /* we can't stop the operation even if the script raised an error */ - ignore_value(virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, - VIR_HOOK_QEMU_OP_STOPPED, VIR_HOOK_SUBOP_END, - NULL, xml, NULL)); - } - /* Reset Security Labels unless caller don't want us to */ if (!(flags & VIR_QEMU_PROCESS_STOP_NO_RELABEL)) qemuSecurityRestoreAllLabel(driver, vm, @@ -8672,8 +8637,6 @@ void qemuProcessStop(virQEMUDriver *driver, virResctrlAllocRemove(vm->def->resctrls[i]->alloc); } - qemuProcessRemoveDomainStatus(driver, vm); - /* Remove VNC and Spice ports from port reservation bitmap, but only if they were reserved by the driver (autoport=yes) */ @@ -8706,20 +8669,9 @@ void qemuProcessStop(virQEMUDriver *driver, } } - for (i = 0; i < vm->ndeprecations; i++) - g_free(vm->deprecations[i]); - g_clear_pointer(&vm->deprecations, g_free); - vm->ndeprecations = 0; - vm->taint = 0; - vm->pid = 0; - virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, reason); for (i = 0; i < vm->def->niothreadids; i++) vm->def->iothreadids[i]->thread_id = 0; - /* clean up a possible backup job */ - if (priv->backup) - qemuBackupJobTerminate(vm, VIR_DOMAIN_JOB_STATUS_CANCELED); - /* Do this explicitly after vm->pid is reset so that security drivers don't * try to enter the domain's namespace which is non-existent by now as qemu * is no longer running. */ @@ -8753,6 +8705,56 @@ void qemuProcessStop(virQEMUDriver *driver, qemuSecurityReleaseLabel(driver->securityManager, vm->def); + /* Only after this point we can reset 'priv->beingDestroyed' so that + * there's no point at which the VM could be considered as alive between + * entering the destroy job and this point where the active "flag" is + * cleared. + */ + vm->def->id = -1; + priv->beingDestroyed = false; + + /* Wake up anything waiting on domain condition */ + virDomainObjBroadcast(vm); + + /* IMPORTANT: qemuDomainObjStopWorker() unlocks @vm in order to prevent + * deadlocks with the per-VM event loop thread. This MUST be done after + * marking the VM as dead */ + qemuDomainObjStopWorker(vm); + + if (!!g_atomic_int_dec_and_test(&driver->nactive) && driver->inhibitCallback) + driver->inhibitCallback(false, driver->inhibitOpaque); + + virPortAllocatorRelease(priv->nbdPort); + priv->nbdPort = 0; + + if (priv->monConfig) { + if (priv->monConfig->type == VIR_DOMAIN_CHR_TYPE_UNIX) + unlink(priv->monConfig->data.nix.path); + g_clear_pointer(&priv->monConfig, virObjectUnref); + } + + /* Remove the master key */ + qemuDomainMasterKeyRemove(priv); + + virFileDeleteTree(priv->libDir); + virFileDeleteTree(priv->channelTargetDir); + + /* Stop autodestroy in case guest is restarted */ + virCloseCallbacksDomainRemove(vm, NULL, qemuProcessAutoDestroy); + + qemuProcessRemoveDomainStatus(driver, vm); + + for (i = 0; i < vm->ndeprecations; i++) + g_free(vm->deprecations[i]); + g_clear_pointer(&vm->deprecations, g_free); + vm->ndeprecations = 0; + vm->taint = 0; + virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, reason); + + /* clean up a possible backup job */ + if (priv->backup) + qemuBackupJobTerminate(vm, VIR_DOMAIN_JOB_STATUS_CANCELED); + /* clear all private data entries which are no longer needed */ qemuDomainObjPrivateDataClear(priv); -- 2.44.2

3 4

[PATCH v2] virt-host-validate: Allow longer list of CPU flags
by Michal Privoznik 23 Jul '24

23 Jul '24

On various occasions, virt-host-validate parses /proc/cpuinfo to learn about CPU flags (see virHostValidateGetCPUFlags()). It does so, by reading the file line by line until the line with CPU flags is reached. Then the line is split into individual flags (using space as a delimiter) and the list of flags is then iterated over. This works, except for cases when the line with CPU flags is too long. Problem is - the line is capped at 1024 bytes and on newer CPUs (and newer kernels), the line can be significantly longer. I've seen a line that's ~1200 characters long (with 164 flags reported). Switch to unbounded read from the file (getline()). Resolves: https://issues.redhat.com/browse/RHEL-39969 Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- v2 of: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/LK74… diff to v2: - Keep trimming of the optional newline tools/virt-host-validate-common.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index 591143c24d..63cc3dbe7b 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -106,21 +106,19 @@ virBitmap *virHostValidateGetCPUFlags(void) { FILE *fp; virBitmap *flags = NULL; + g_autofree char *line = NULL; + size_t linelen = 0; if (!(fp = fopen("/proc/cpuinfo", "r"))) return NULL; flags = virBitmapNew(VIR_HOST_VALIDATE_CPU_FLAG_LAST); - do { - char line[1024]; + while (getline(&line, &linelen, fp) > 0) { char *start; g_auto(GStrv) tokens = NULL; GStrv next; - if (!fgets(line, sizeof(line), fp)) - break; - /* The line we're interested in is marked differently depending * on the architecture, so check possible prefixes */ if (!STRPREFIX(line, "flags") && @@ -129,11 +127,9 @@ virBitmap *virHostValidateGetCPUFlags(void) !STRPREFIX(line, "facilities")) continue; - /* fgets() includes the trailing newline in the output buffer, - * so we need to clean that up ourselves. We can safely access - * line[strlen(line) - 1] because the checks above would cause - * us to skip empty strings */ - line[strlen(line) - 1] = '\0'; + /* getline() may include the trailing newline in the output + * buffer, so we need to clean that up ourselves. */ + virStringTrimOptionalNewline(line); /* Skip to the separator */ if (!(start = strchr(line, ':'))) @@ -153,7 +149,7 @@ virBitmap *virHostValidateGetCPUFlags(void) if ((value = virHostValidateCPUFlagTypeFromString(*next)) >= 0) ignore_value(virBitmapSetBit(flags, value)); } - } while (1); + } VIR_FORCE_FCLOSE(fp); -- 2.44.2

2 1