Re: Add iommu device when VM configured with > 255 vcpus
On Tuesday, May 28 2024, Jim Fehlig via Devel wrote: > Hi All, > > I vaguely recall a discussion about $subject, but can't find it > now. Perhaps buried in another thread. The topic has been raised > internally again, and I'd like to gauge the community's interest in > automatically adding the necessary devices/config when user has > specified vcpus > 255. > > The comparison for prior art is a bit of a stretch, but we e.g. add > when spice graphics is configured. I know > libvirt has generally tried to avoid policy decisions, but it's not > clear to me where we stand with cases such as this, where every x86 VM > with > 255 vcpus needs a similarly configured iommu. My two cents here: this is something I would certainly appreciate as a downstream maintainer of QEMU/libvirt. In fact, I spent part of last year figuring out and documenting the necessary bits that need to be put together in order to use more than 288 vCPUs. One of the results of this effort (with help from David Woodhouse) was: https://ubuntu.com/server/docs/create-qemu-vms-with-up-to-1024-vcpus I still have to write the equivalent guide for libvirt, FWIW. Cheers, -- Sergio GPG key ID: E92F D0B3 6B14 F1F4 D8E0 EB2F 106D A1C8 C3CB BF14
Re: Add iommu device when VM configured with > 255 vcpus
On 5/29/24 1:41 AM, Peter Krempa wrote: On Tue, May 28, 2024 at 16:26:18 -0600, Jim Fehlig via Devel wrote: Hi All, I vaguely recall a discussion about $subject, but can't find it now. Perhaps buried in another thread. The topic has been raised internally again, and I'd like to gauge the community's interest in automatically adding the necessary devices/config when user has specified vcpus > 255. The comparison for prior art is a bit of a stretch, but we e.g. add when spice graphics is configured. The thing about 'audio' "device" is that it's purely just backend with no impact on the VM ABI. In fact 'audio' and 'graphics' IMO should not have been put under for that reason. I know libvirt has generally tried to avoid policy decisions, but it's not clear to me where we stand with cases such as this, where every x86 VM with > 255 vcpus needs a similarly configured iommu. Adding the IOMMU would change the guest ABI, so libvirt can't auto-add it, unless a VM with > 255 cpus would not start at all. libvirt already prevents defining a VM with > 255 vcpus, but without a properly configured iommu error: unsupported configuration: more than 255 vCPUs require extended interrupt mode enabled on the iommu device It's possible to start such a VM using qemu directly, although the guest (linux at least) does not make much progress [0.095107][T0] [Firmware Bug]: CPU 0: APIC ID mismatch. CPUID: 0x0001 APIC: 0x [0.003921][T0] [Firmware Bug]: CPU 2: APIC ID mismatch. CPUID: 0x0003 APIC: 0x0002 [0.003921][T0] [Firmware Bug]: CPU 4: APIC ID mismatch. CPUID: 0x0005 APIC: 0x0004 [0.003921][T0] [Firmware Bug]: CPU 6: APIC ID mismatch. CPUID: 0x0007 APIC: 0x0006 ... [0.003921][T0] [Firmware Bug]: CPU 250: APIC ID mismatch. CPUID: 0x00fb APIC: 0x00fa [0.003921][T0] [Firmware Bug]: CPU 252: APIC ID mismatch. CPUID: 0x00fd APIC: 0x00fc [0.003921][T0] [Firmware Bug]: CPU 254: APIC ID mismatch. CPUID: 0x00ff APIC: 0x00fe Regards, Jim
Re: [PATCH-for-9.1 v2 2/3] migration: Remove RDMA protocol handling
Lei,
On Wed, May 29, 2024 at 02:43:46AM +, Gonglei (Arei) wrote:
> For rdma programming, the current mainstream implementation is to use
> rdma_cm to establish a connection, and then use verbs to transmit data.
> rdma_cm and ibverbs create two FDs respectively. The two FDs have
> different responsibilities. rdma_cm fd is used to notify connection
> establishment events, and verbs fd is used to notify new CQEs. When
> poll/epoll monitoring is directly performed on the rdma_cm fd, only a
> pollin event can be monitored, which means that an rdma_cm event
> occurs. When the verbs fd is directly polled/epolled, only the pollin
> event can be listened, which indicates that a new CQE is generated.
>
> Rsocket is a sub-module attached to the rdma_cm library and provides
> rdma calls that are completely similar to socket interfaces. However,
> this library returns only the rdma_cm fd for listening to link
> setup-related events and does not expose the verbs fd (readable and
> writable events for listening to data). Only the rpoll interface provided
> by the RSocket can be used to listen to related events. However, QEMU
> uses the ppoll interface to listen to the rdma_cm fd (gotten by raccept
> API). And cannot listen to the verbs fd event. Only some hacking methods
> can be used to address this problem. Do you guys have any ideas? Thanks.
I saw that you mentioned this elsewhere:
> Right. But the question is QEMU do not use rpoll but gilb's ppoll. :(
So what I'm thinking may not make much sense, as I mentioned I don't think
I know rdma at all.. and my idea also has involvement on coroutine stuff
which I also don't know well. But just in case it shed some light in some
form.
IIUC we do iochannel blockings with this no matter for read/write:
if (len == QIO_CHANNEL_ERR_BLOCK) {
if (qemu_in_coroutine()) {
qio_channel_yield(ioc, G_IO_XXX);
} else {
qio_channel_wait(ioc, G_IO_XXX);
}
continue;
}
One thing I'm wondering is whether we can provide a new feature bit for
qiochannel, e.g., QIO_CHANNEL_FEATURE_POLL, so that the iochannel can
define its own poll routine rather than using the default when possible.
I think it may not work if it's in a coroutine, as I guess that'll block
other fds from being waked up. Hence it should look like this:
if (len == QIO_CHANNEL_ERR_BLOCK) {
if (qemu_in_coroutine()) {
qio_channel_yield(ioc, G_IO_XXX);
} else if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_POLL)) {
qio_channel_poll(ioc, G_IO_XXX);
} else {
qio_channel_wait(ioc, G_IO_XXX);
}
continue;
}
Maybe we even want to forbid such channel to be used in coroutine already,
as when QIO_CHANNEL_FEATURE_POLL set it may mean that this iochannel simply
won't work with poll() like in rdma's use case.
Then rdma iochannel can implement qio_channel_poll() using rpoll().
There's one other dependent issue here in that I _think_ the migration recv
side is still in a coroutine.. so we may need to move that into a thread
first. IIRC we don't yet have a major blocker to do that, but I didn't
further check either. I've put that issue aside just to see whether this
may or may not make sense.
Thanks,
--
Peter Xu
Re: [PATCH v3 2/5] qemu: Introduce shared_filesystems configuration option
On Thu, May 09, 2024 at 17:51:59 +0100, Daniel P. Berrangé wrote: > On Thu, May 09, 2024 at 04:47:48PM +, Andrea Bolognani wrote: > > On Thu, May 09, 2024 at 05:10:50PM GMT, Peter Krempa wrote: > > > Now things I see as problem in case when NFS not supporting xattr is > > > used. This means that the remote VM can set XATTRs and must use > > > 'virt_use_nfs' sebool. > > > > I must be confused about the purpose of the virt_use_nfs sebool, and > > I can't seem to find decent documentation about it. Do you have any > > handy? > > Out of the box, there usually is no ability for QEMU to access > files stored on NFS whatsoever, because NFS lacks support for > storing (svirt_image_t:MCS) labels in xattr. > > Setting virt_use_nfs, toggles the policy such that QEMU can now > access *any* nfs_t file. This lets QEMU works on NFS lacking > label support, but at the cost of killing MAC protection against > any other non-VM related files that might be stored on NFS. DAC > protection still applies though, since we're not running QEMU > as root. > > If an NFS deployment *does* support SELinux labels, there is > no reason to use virt_use_nfs, and it should not be used due > to reduced MAC protection. So I've finally got around to test this with NFS-4.2 with "XATTR" support. I used xattr in quotes, as per spec it seems that xattr support over NFS is limited (per spec IIUC, and observably) to only handle the 'user' namespace of xattr labels. Any other namespace is _NOT_ available over NFS at least in default config. (The main point in the docs seem to be that OSes may interpret the security labels in such namespaces differently and NFS doesn't want to deal with attempting to translate them in any shape or form (e.g. on linux we use the 'trusted' namespace while on FreeBSD we use the 'system' namespace to remember lables)) This means that neither 'selinux' labels nor the XATTRs libvirt uses to remember the previous security labels are available on the other side of migration. This works (partially) correctly though, as either side of the migration is handing the full lifecycle of the security label (even despite that the storage doesn't transport them). This means that on outgoing migration the source will remove the reference on the xattr remembering the seclabels (without actually restoring them). Destination would obviously attempt to take a reference but can't as it's not supported. This means that xattrs are not leaked. On incoming migration though there's an issue though. Libvirt tries to apply (the same) seclabel, which is correct. This though involves also an attempt to remember the owner, whcih succeeds, but remembers the *active* seclabel. When the VM is then terminated that seclabel is restored. The above is _not_ a good reason to disable seclabel remembering though, as suggested by docs added by this patch, even when it does in fact work around that issue. We should rather fix the seclabel remembering code: - for now by not trying to create the xattrs to remember the seclabel when we're migrating in and the labels on the image are already correct. - in the future we perhaps could transfer the original seclabels in the migration cookie I'll be posting patches to address this before this series goes in, so that the suggestion to disable 'remember_owner' globally can be dropped. > If an NFS deployment does *not* support SELinux labels, then > virt_use_nfs must be turned on Based on the above, this is in fact needed even with NFS-4.2
Re: [libvirt PATCH] qemu_snapshot: fix memory leak when reverting external snapshot
On Mon, May 27, 2024 at 20:02:39 +0200, Pavel Hrdina wrote:
> The code cleaning up virStorageSource doesn't free data allocated by
> virStorageSourceInit() so we need to call virStorageSourceDeinit()
> explicitly.
>
> Fixes: 8e664737813378d2a1bdeacc2ca8e942327e2cab
> Resolves: https://issues.redhat.com/browse/RHEL-33044
> Signed-off-by: Pavel Hrdina
> ---
> src/qemu/qemu_snapshot.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index 09ec959f10..f5260c4a22 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -2260,6 +2260,8 @@ qemuSnapshotRevertExternalFinish(virDomainObj *vm,
> snapdisk->src->path);
> }
>
> +virStorageSourceDeinit(snapdisk->src);
> +
> virDomainSnapshotDiskDefClear(snapdisk);
> }
>
> @@ -2277,6 +2279,8 @@ qemuSnapshotRevertExternalFinish(virDomainObj *vm,
> VIR_WARN("Failed to remove snapshot image '%s'",
> snapdisk->src->path);
> }
> +
> +virStorageSourceDeinit(snapdisk->src);
> }
> }
>
Reviewed-by: Jiri Denemark
Re: [PATCH v2] run.in: Detect binaries in builddir properly
On Tue, May 28, 2024 at 10:49:34 +0200, Michal Privoznik wrote:
> When attempting to run:
>
> libvirt.git/_build # ./run --selinux ./src/libvirtd
>
> the following error is thrown:
>
> Refusing to change selinux context of file './src/libvirtd' outside build
> directory
>
> which is obviously wrong. The problem is 'being inside of build
> directory' is detected by simple progpath.startswith(builddir).
> While builddir is an absolute path, progpath isn't necessarily.
>
> And while looking into the code, I've noticed chcon() function
> accessing variable outside its scope when printing out the path
> it's working on.
>
> Signed-off-by: Michal Privoznik
> ---
>
> v2 of:
>
> https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/UZ6F7MWEJBUMUUBODXGAXQW4NY2UEEVF/
>
> diff to v1:
> - error out if binary to run can't be identified (i.e. 'which' returns
> None).
>
> run.in | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/run.in b/run.in
> index 5b89b3dcd5..cada74dfcd 100644
> --- a/run.in
> +++ b/run.in
> @@ -138,7 +138,7 @@ def change_unit(name, action):
>
>
> def chcon(path, user, role, type):
> -print("Setting file context of {} to u={}, r={},
> t={}...".format(progpath,
> +print("Setting file context of {} to u={}, r={}, t={}...".format(path,
> user,
> role,
> type))
> @@ -187,6 +187,10 @@ else:
> try:
> dorestorecon = False
> progpath = shutil.which(prog)
> +if not progpath:
> +raise Exception("Can't find executable {}"
> +.format(prog))
> +progpath = os.path.abspath(progpath)
> if len(try_stop_units):
> print("Temporarily stopping systemd units...")
You can drop the second (now unreachable) check a few lines later:
if not progpath:
raise Exception("Can't find executable {} for selinux labeling"
.format(prog))
Reviewed-by: Jiri Denemark
[PATCH v2] virsh: Provide completer for some pool-X-as commands
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
---
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();
+}
+
+
+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();
+}
+
+
+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, , 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();
+
+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, , 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);
+}
+
+
Re: [PATCH] virsh: Provide completer for some pool-X-as commands
On Tue, 28 May 2024 at 17:35, Peter Krempa wrote:
> On Mon, May 27, 2024 at 18:34:38 +, Abhiram Tilak wrote:
> > 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. I left the ones where
> > I was not sure if they need completers.
> >
> > Related Issue: https://gitlab.com/libvirt/libvirt/-/issues/9
> > Signed-off-by: Abhiram Tilak
> > ---
> > src/libvirt_private.syms | 2 ++
> > tools/virsh-completer-pool.c | 48 +++-
> > tools/virsh-completer-pool.h | 10
> > tools/virsh-pool.c | 8 ++
> > 4 files changed, 67 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > index 6b6bcc368a..5a413ca832 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..4a771d894e 100644
> > --- a/tools/virsh-completer-pool.c
> > +++ b/tools/virsh-completer-pool.c
> > @@ -84,7 +84,6 @@ virshPoolEventNameCompleter(vshControl *ctl
> G_GNUC_UNUSED,
> > return g_steal_pointer();
> > }
> >
> > -
>
> Spurious whitespace change.
>
> > char **
> > virshPoolTypeCompleter(vshControl *ctl,
> > const vshCmd *cmd,
> > @@ -106,3 +105,50 @@ virshPoolTypeCompleter(vshControl *ctl,
> >
> > return virshCommaStringListComplete(type_str, (const char **)tmp);
> > }
> > +
>
> Two empty lines between functions in this file.
>
> > +char **
> > +virshPoolFormatCompleter(vshControl *ctl G_GNUC_UNUSED,
> > + const vshCmd *cmd G_GNUC_UNUSED,
> > + unsigned int flags)
> > +{
> > +size_t i = 0, j = 0;
>
> One declaration per line please;
>
> > +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 = 0; i < VIR_STORAGE_POOL_DISK_LAST; i++)
> > +tmp[j++] = g_strdup(virStoragePoolFormatDiskTypeToString(i));
>
> I don't think it makes sense to complete the "unknown" value here ...
>
> Thanks for pointing this out. I totally missed it.
> > +
> > +for (i = 0; i < VIR_STORAGE_POOL_LOGICAL_LAST; i++)
> > +tmp[j++] = g_strdup(virStoragePoolFormatLogicalTypeToString(i));
>
> ... and here.
>
> > +
> > +return g_steal_pointer();
> > +}
> > +
> > +char ** virshPoolAuthTypeCompleter(vshControl *ctl G_GNUC_UNUSED,
> > + const vshCmd *cmd G_GNUC_UNUSED,
> > + unsigned int flags)
>
> Wrong header formatting style, and two lines between functions please.
>
> > +{
> > +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();
> > +}
> > diff --git a/tools/virsh-completer-pool.h b/tools/virsh-completer-pool.h
> > index bff3e5742b..4a53f99577 100644
> > --- a/tools/virsh-completer-pool.h
> > +++ b/tools/virsh-completer-pool.h
> > @@ -40,3 +40,13 @@ 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);
> > diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c
> > index 67cc1b94cf..1a294bb0f6 100644
> > --- a/tools/virsh-pool.c
> > +++ b/tools/virsh-pool.c
> > @@ -81,26 +81,32 @@
> > }, \
> > {.name = "source-path", \
> > .type = VSH_OT_STRING, \
> > + .completer = virshCompletePathLocalExisting, \
> > .help =
Re: Add iommu device when VM configured with > 255 vcpus
On 5/29/24 09:41, Peter Krempa wrote: > On Tue, May 28, 2024 at 16:26:18 -0600, Jim Fehlig via Devel wrote: >> Hi All, >> >> I vaguely recall a discussion about $subject, but can't find it now. Perhaps >> buried in another thread. The topic has been raised internally again, and >> I'd like to gauge the community's interest in automatically adding the >> necessary devices/config when user has specified vcpus > 255. > >> The comparison for prior art is a bit of a stretch, but we e.g. add > type='spice'/> when spice graphics is configured. > > The thing about 'audio' "device" is that it's purely just backend with > no impact on the VM ABI. In fact 'audio' and 'graphics' IMO should not > have been put under for that reason. > >> I know libvirt has >> generally tried to avoid policy decisions, but it's not clear to me where we >> stand with cases such as this, where every x86 VM with > 255 vcpus needs a >> similarly configured iommu. > > Adding the IOMMU would change the guest ABI, so libvirt can't auto-add > it, unless a VM with > 255 cpus would not start at all. > > In case of IOMMU the absence of the element means that the user doesn't > want an IOMMU, rather than that it was not configured, so you'd have no > way to express a configuration where > 255 cpus are declared but no > IOMMU was used to start it. Migrating such a config would then break. > Right, that's what I had on mind but didn't write it. Sorry. We have VIR_DOMAIN_DEF_PARSE_ABI_UPDATE flag that is meant for this purpose, isn't it? Michal
Re: [PATCH] fix virLogCleanerParseFilename logic
On 5/29/24 09:21, Michal Prívozník wrote: > On 5/29/24 09:04, David Negreira wrote: >> Gentle reminder >> > > Can you please send v2? Michal > > Oh, I thought you were going to send v2. But okay, let me do that. Done: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/ZWZ7GFFLZTCMWDUUB3X4FFK4YT5LW4OA/ Michal
[PATCH] log_cleaner: Detect rotated filenames properly
When removing rotated log files, their name is matched against a
regex (@log_regex) and if they contain '.N' suffix the 'N' is
then parsed into an integer. Well, due to a bug in
virLogCleanerParseFilename() this is not how the code works. If
the suffix isn't found then g_match_info_fetch() returns an empty
string instead of NULL which then makes str2int parsing fail.
Just check for this case before parsing the string.
Based on the original patch sent by David.
Reported-by: David Negreira
Signed-off-by: Michal Privoznik
---
The original patch was posted here:
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/ENXJABODLWWXAR6RSKGMG6GAJKZMVKKM/
In the thread you can see me suggesting this alternative approach and
David confirming it works. Therefore, I'd like to get this in before the
release.
src/logging/log_cleaner.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/logging/log_cleaner.c b/src/logging/log_cleaner.c
index 4ee91843aa..4efcbc18e4 100644
--- a/src/logging/log_cleaner.c
+++ b/src/logging/log_cleaner.c
@@ -82,10 +82,8 @@ virLogCleanerParseFilename(const char *path,
*rotated_index = 0;
rotated_index_str = g_match_info_fetch(matchInfo, 3);
-if (!rotated_index_str)
-return chain_prefix;
-
-if (virStrToLong_i(rotated_index_str, NULL, 10, rotated_index) < 0) {
+if (rotated_index_str && STRNEQ(rotated_index_str, "") &&
+virStrToLong_i(rotated_index_str, NULL, 10, rotated_index) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to parse rotated index from '%1$s'"),
rotated_index_str);
--
2.44.1
Re: Add iommu device when VM configured with > 255 vcpus
On Tue, May 28, 2024 at 16:26:18 -0600, Jim Fehlig via Devel wrote: > Hi All, > > I vaguely recall a discussion about $subject, but can't find it now. Perhaps > buried in another thread. The topic has been raised internally again, and > I'd like to gauge the community's interest in automatically adding the > necessary devices/config when user has specified vcpus > 255. > The comparison for prior art is a bit of a stretch, but we e.g. add type='spice'/> when spice graphics is configured. The thing about 'audio' "device" is that it's purely just backend with no impact on the VM ABI. In fact 'audio' and 'graphics' IMO should not have been put under for that reason. >I know libvirt has > generally tried to avoid policy decisions, but it's not clear to me where we > stand with cases such as this, where every x86 VM with > 255 vcpus needs a > similarly configured iommu. Adding the IOMMU would change the guest ABI, so libvirt can't auto-add it, unless a VM with > 255 cpus would not start at all. In case of IOMMU the absence of the element means that the user doesn't want an IOMMU, rather than that it was not configured, so you'd have no way to express a configuration where > 255 cpus are declared but no IOMMU was used to start it. Migrating such a config would then break.
Re: [PATCH] fix virLogCleanerParseFilename logic
On 5/29/24 09:04, David Negreira wrote: > Gentle reminder > >>> >>> Can you please send v2? >>> >>> Michal Oh, I thought you were going to send v2. But okay, let me do that. Michal
