Re: [libvirt PATCH v2 0/5] Add support for vDPA block devices
On 9/12/23 7:00 AM, Peter Krempa wrote: On Mon, Sep 11, 2023 at 16:53:42 -0500, Jonathon Jongsma wrote: see https://bugzilla.redhat.com/show_bug.cgi?id=1900770. Changes in v2: - Don't use virStorageSource->path for vdpa device path to avoid clashing with existing path functionality - Move vdpa device opening to the qemuProcessPrepareHostStorageSource() function rather than the qemuDomainPrepareStorageSource() function. This also required some additional support in the tests for setting up the objects properly for testing. - rebased to latest master branch Reviewed-by: Peter Krempa I pushed this series, but for some reason only the ubuntu images are failing CI with no useful output: https://gitlab.com/libvirt/libvirt/-/pipelines/1001459836 I suspect it may be related to address sanitizer stuff, does anybody have tips on getting more information about this failure? Jonathon
[PATCH] fix uint64 underflow
According to previous statement, 'free_mem' is less than 'needed_mem'. So, the subtraction 'free_mem - needed_mem' is negative, and will raise uint64 underflow. Signed-off-by: Dmitry Frolov --- src/libxl/libxl_domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 6c167df63e..36be042971 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -940,7 +940,7 @@ libxlDomainFreeMem(libxl_ctx *ctx, libxl_domain_config *d_config) if (free_mem >= needed_mem) return 0; -target_mem = free_mem - needed_mem; +target_mem = needed_mem - free_mem; if (libxlSetMemoryTargetWrapper(ctx, 0, target_mem, /* relative */ 1, 0) < 0) goto error; -- 2.34.1
Re: [libvirt PATCHv1 8/8] docs: virtiofs: add section about ID remapping
On Tue, Sep 12, 2023 at 04:05:04PM +0200, Ján Tomko wrote: > On a Monday in 2023, Daniel P. Berrangé wrote: > > On Mon, Sep 11, 2023 at 03:51:28PM +0200, Ján Tomko wrote: > > > Signed-off-by: Ján Tomko > > > --- > > > docs/kbase/virtiofs.rst | 29 + > > > 1 file changed, 29 insertions(+) > > > > > > diff --git a/docs/kbase/virtiofs.rst b/docs/kbase/virtiofs.rst > > > index 5940092db5..ecfb8e4236 100644 > > > --- a/docs/kbase/virtiofs.rst > > > +++ b/docs/kbase/virtiofs.rst > > > @@ -59,6 +59,35 @@ Sharing a host directory with a guest > > > > > > Note: this requires virtiofs support in the guest kernel (Linux v5.4 > > > or later) > > > > > > +ID mapping > > > +== > > > + > > > +In unprivileged mode (``qemu:///session``), mapping user/group IDs is > > > available > > > +(since libvirt version TBD). After reserving an ID range from the host > > > for your > > > +regular user > > > > Is the GUID/GID mapping available as in optional, or available as > > in mandatory ? > > > > In this series, optional. > > My reasoning was that someone might want to not do it and would prefer > to run virtiofsd as their own user. > > On second thought, that is not what accessmode='passthrough' means, > so for non-mapping non-privileged use, a different/new accessmode > attribute will be needed. > > > I would expect libvirt to "do the right thing" and automatically load > > the /etc/subuid data for the current user and NOT require any extra > > XML mapping to be set for unprivileged usage. > > > > So, by default libvirt would assume that unprivileged > accessmode='passthrough' means "use the whole range for my user > from /etc/subuid"? > > Podman treats /etc/subuid as a pool and chooses a 64K range that is > (to its knowledge) unused. I'm undecided whether that would also be > a reasonable option for a default. I thought podman simply used the entry that is in /etc/subuid as is: $ grep $LOGNAME /etc/subuid berrange:165536:65536 $ podman run -it centos:stream9 cat /proc/self/uid_map 0 1001 1 1 165536 65536 Maps "root" to my original unpriv login UID, and maps everything else to the 64k IDs reserved in /etc/subuid With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCHv1 8/8] docs: virtiofs: add section about ID remapping
On a Monday in 2023, Daniel P. Berrangé wrote: On Mon, Sep 11, 2023 at 03:51:28PM +0200, Ján Tomko wrote: Signed-off-by: Ján Tomko --- docs/kbase/virtiofs.rst | 29 + 1 file changed, 29 insertions(+) diff --git a/docs/kbase/virtiofs.rst b/docs/kbase/virtiofs.rst index 5940092db5..ecfb8e4236 100644 --- a/docs/kbase/virtiofs.rst +++ b/docs/kbase/virtiofs.rst @@ -59,6 +59,35 @@ Sharing a host directory with a guest Note: this requires virtiofs support in the guest kernel (Linux v5.4 or later) +ID mapping +== + +In unprivileged mode (``qemu:///session``), mapping user/group IDs is available +(since libvirt version TBD). After reserving an ID range from the host for your +regular user Is the GUID/GID mapping available as in optional, or available as in mandatory ? In this series, optional. My reasoning was that someone might want to not do it and would prefer to run virtiofsd as their own user. On second thought, that is not what accessmode='passthrough' means, so for non-mapping non-privileged use, a different/new accessmode attribute will be needed. I would expect libvirt to "do the right thing" and automatically load the /etc/subuid data for the current user and NOT require any extra XML mapping to be set for unprivileged usage. So, by default libvirt would assume that unprivileged accessmode='passthrough' means "use the whole range for my user from /etc/subuid"? Podman treats /etc/subuid as a pool and chooses a 64K range that is (to its knowledge) unused. I'm undecided whether that would also be a reasonable option for a default. Jano By all means have an XML config for it, but it should be optional and something that is essentially never used except for niche scenarios signature.asc Description: PGP signature
Re: [libvirt PATCHv1 0/8] support running virtiofsd in session mode
On a Monday in 2023, Michal Prívozník wrote: On 9/11/23 15:51, Ján Tomko wrote: https://bugzilla.redhat.com/show_bug.cgi?id=2034630 https://gitlab.com/libvirt/libvirt/-/issues/535 https://gitlab.gnome.org/GNOME/gnome-boxes/-/issues/292 Ján Tomko (8): qemu: fix indentation conf: move idmap definition earlier conf: move idmap parsing earlier conf: add idmap element to filesystem qemu: format uid/gid map for virtiofs qemu: virtiofs: do not force UID 0 qemu: allow running virtiofsd in session mode docs: virtiofs: add section about ID remapping docs/formatdomain.rst | 7 + docs/kbase/virtiofs.rst | 29 src/conf/domain_conf.c| 149 -- src/conf/domain_conf.h| 29 ++-- src/conf/schemas/domaincommon.rng | 3 + src/qemu/qemu_validate.c | 25 ++- src/qemu/qemu_virtiofs.c | 17 +- .../vhost-user-fs-fd-memory.xml | 4 + 8 files changed, 181 insertions(+), 82 deletions(-) So how does this idmap end up on virtiofsd cmd line? Maybe you forgot to send some additional patches? It's in PATCH 5/8: qemu: format uid/gid map for virtiofs Perhaps you asking this question is a good indicator that I should add some qemuvirtiofsxml2argvtest. However, in the linked bug, German suggested that the user namespace be created by libvirt, because they intend to remove that --uid-map option from virtiofsd, so the virtiofsd command line change will be missing from v2 of this series. Jano Michal signature.asc Description: PGP signature
[PATCH v3] interface: fix udev_device_get_sysattr_value return value check
Reviewing the code I found that return value of function
udev_device_get_sysattr_value() is dereferenced without a check.
udev_device_get_sysattr_value() may return NULL by number of reasons.
v2: VIR_DEBUG added, replaced STREQ(NULLSTR()) with STREQ_NULLABLE()
v3: More checks added, to skip earlier. More verbose VIR_DEBUG.
Signed-off-by: Dmitry Frolov
---
src/interface/interface_backend_udev.c | 26 +++---
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/src/interface/interface_backend_udev.c
b/src/interface/interface_backend_udev.c
index a0485ddd21..fb6799ed94 100644
--- a/src/interface/interface_backend_udev.c
+++ b/src/interface/interface_backend_udev.c
@@ -23,6 +23,7 @@
#include
#include
+#include "virlog.h"
#include "virerror.h"
#include "virfile.h"
#include "datatypes.h"
@@ -40,6 +41,8 @@
#define VIR_FROM_THIS VIR_FROM_INTERFACE
+VIR_LOG_INIT("interface.interface_backend_udev");
+
struct udev_iface_driver {
struct udev *udev;
/* pid file FD, ensures two copies of the driver can't use the same root */
@@ -354,11 +357,20 @@ udevConnectListAllInterfaces(virConnectPtr conn,
const char *macaddr;
g_autoptr(virInterfaceDef) def = NULL;
-path = udev_list_entry_get_name(dev_entry);
-dev = udev_device_new_from_syspath(udev, path);
-name = udev_device_get_sysname(dev);
+if (!(path = udev_list_entry_get_name(dev_entry))) {
+VIR_DEBUG("Skipping interface, path == NULL");
+continue;
+}
+if (!(dev = udev_device_new_from_syspath(udev, path))) {
+VIR_DEBUG("Skipping interface '%s', dev == NULL", path);
+continue;
+}
+if (!(name = udev_device_get_sysname(dev))) {
+VIR_DEBUG("Skipping interface '%s', name == NULL", path);
+continue;
+}
macaddr = udev_device_get_sysattr_value(dev, "address");
-status = STREQ(udev_device_get_sysattr_value(dev, "operstate"), "up");
+status = STREQ_NULLABLE(udev_device_get_sysattr_value(dev,
"operstate"), "up");
def = udevGetMinimalDefForDevice(dev);
if (!virConnectListAllInterfacesCheckACL(conn, def)) {
@@ -964,9 +976,9 @@ udevGetIfaceDef(struct udev *udev, const char *name)
/* MTU */
mtu_str = udev_device_get_sysattr_value(dev, "mtu");
-if (virStrToLong_ui(mtu_str, NULL, 10, ) < 0) {
+if (!mtu_str || virStrToLong_ui(mtu_str, NULL, 10, ) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
-_("Could not parse MTU value '%1$s'"), mtu_str);
+_("Could not parse MTU value '%1$s'"), NULLSTR(mtu_str));
goto error;
}
ifacedef->mtu = mtu;
@@ -1089,7 +1101,7 @@ udevInterfaceIsActive(virInterfacePtr ifinfo)
goto cleanup;
/* Check if it's active or not */
-status = STREQ(udev_device_get_sysattr_value(dev, "operstate"), "up");
+status = STREQ_NULLABLE(udev_device_get_sysattr_value(dev, "operstate"),
"up");
udev_device_unref(dev);
--
2.34.1
Re: [libvirt PATCH v2 0/5] Add support for vDPA block devices
On Mon, Sep 11, 2023 at 16:53:42 -0500, Jonathon Jongsma wrote: > see https://bugzilla.redhat.com/show_bug.cgi?id=1900770. > > Changes in v2: > - Don't use virStorageSource->path for vdpa device path to avoid clashing > with >existing path functionality > - Move vdpa device opening to the qemuProcessPrepareHostStorageSource() >function rather than the qemuDomainPrepareStorageSource() function. This >also required some additional support in the tests for setting up the >objects properly for testing. > - rebased to latest master branch Reviewed-by: Peter Krempa
Re: [PATCH v1 0/9] query & cache host-recommended CPU model
On Mon, Sep 11, 2023 at 17:07:07 -0400, Collin Walling wrote: > Notes: > https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg02518.html > - For information regarding the QEMU "static-recommended" (aka > host-recommended) >CPU model, please see the analogous QEMU patches: > > https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg02518.html > > - Patches include caps added to QEMU 8.1 test files. This is a stand-in for > the >interim and the appropriate caps files will be updated in the future > pending >QEMU acceptance. I went looking for the feature in qemu and couldn't find it, but you explain it here. Note that it's acceptable only as a RFC to gather feedback, but must not be comitted in this form. The capability dumps must represent real qemu capabilities, and thus hacks where you "backport" something are not acceptable. Once the qemu patches get commited upstream, it's okay if you create a capability dump from the in-progress development cycle. We do that also for x86_64.
Re: [PATCH v1 2/9] qemu: capabilities: add static-recommended capability
On Mon, Sep 11, 2023 at 17:07:09 -0400, Collin Walling wrote:
> Check for the QEMU capability to query for a static-recommended CPU
> model via CPU model expansion. Cache this capability for later.
>
> Signed-off-by: Collin Walling
> Reviewed-by: Boris Fiuczynski
> ---
> src/qemu/qemu_capabilities.c| 2 ++
> src/qemu/qemu_capabilities.h| 1 +
> tests/qemucapabilitiesdata/caps_8.1.0_s390x.replies | 6 +-
> tests/qemucapabilitiesdata/caps_8.1.0_s390x.xml | 1 +
> 4 files changed, 9 insertions(+), 1 deletion(-)
[...]
> diff --git a/tests/qemucapabilitiesdata/caps_8.1.0_s390x.replies
> b/tests/qemucapabilitiesdata/caps_8.1.0_s390x.replies
> index 57ce64e88e..8cd7312bea 100644
> --- a/tests/qemucapabilitiesdata/caps_8.1.0_s390x.replies
> +++ b/tests/qemucapabilitiesdata/caps_8.1.0_s390x.replies
> @@ -15398,12 +15398,16 @@
> },
> {
>"name": "full"
> +},
> +{
> + "name": "static-recommended"
> }
>],
>"meta-type": "enum",
>"values": [
> "static",
> -"full"
> +"full",
> +"static-recommended"
This is suspicious. We usually don't allow manual modification of the
file because it can be overwritten by a subsequent update of the file.
Could you please properly update the capability dump, by running
tests/qemucapsprobe /path/to/qemu-system-s390x >
/path/to/libvirt.git/tests/qemucapabilitiesdata/caps_8.1.0_s390x.replies
and commiting that to a separate commit. Ideally do this with the
released qemu-8.1, so that we have the most recent dump.
