Re: Libvirt Rust bindings could use some work
On Mon, Feb 21, 2022 at 11:53 AM Wim de With wrote: > > > Since the intent of libvirt using LGPL was explicitly to allow non-GPL > > compatible applications to use libvirt, it is a mistake to be using > > a license that breaks this ability for Rust. > > > > In Golang we've used MIT license > > > > For Rust we should aim for whatever is most appropriate - MIT or BSD > > or Apache - I'm not familiar with which is dominant in the Rust world. > > Apache and MIT or even dual licensing of both are the most common. > The official API guidelines recommend dual licensing under Apache and > MIT. > I would prefer we didn't repeat that dumb advice in libvirt-rs. Just go with Apache-2.0 if we want a permissively licensed crate. I would suggest MPL-2.0 for libvirt-rs, though. That allows proprietary linking but maintains that each file that makes up the crate *must* remain FOSS, and is compatible with GNU licenses. It's static-link compatible copyleft, basically. -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCH v3 00/16] Add QEMU "-display dbus" support
On Fri, Dec 31, 2021 at 1:27 AM Marc-André Lureau wrote: > > Hi Neal > > On Fri, Dec 31, 2021 at 6:09 AM Neal Gompa wrote: > > > > On Wed, Dec 22, 2021 at 2:44 PM wrote: > > > > > > From: Marc-André Lureau > > > > > > Hi, > > > > > > This series implements supports for the uQEMU "-display dbus" support, > > > that > > > landed earlier this week for 7.0. > > > > > > By default, libvirt will start a private VM bus (sharing and reusing the > > > existing "vmstate" VM bus & code). > > > > > > The feature set should cover the needs to replace Spice as local client > > > of choice, > > > including 3daccel/dmabuf, audio, clipboard sharing, usb redirection, and > > > arbitrary > > > chardev/channels (for serial etc). > > > > > > The test Gtk4 client is also in progress, currently in development at > > > https://gitlab.com/marcandre.lureau/qemu-display/. A few dependencies, > > > such as > > > zbus, require an upcoming release. virt-viewer & boxes will need a port > > > to Gtk4 > > > to make use of the shared widget. > > > > > > Comments welcome, as we can still adjust the QEMU side etc. > > > > > > thanks > > > > > > v3: after QEMU 7.0 dev cycle opening and merge > > > - rebased > > > - add 7.0 x86-64 capabilities (instead of tweaking 6.2) > > > - fix version annotations > > > > > > Marc-André Lureau (16): > > > qemu: add chardev-vdagent capability check > > > qemu: add -display dbus capability check > > > qemucapabilitiestest: Add x64 test data for the qemu-7.0 development > > > cycle > > > conf: add > > > qemu: start the D-Bus daemon for the display > > > qemu: add -display dbus support > > > virsh: refactor/split cmdDomDisplay() > > > virsh: report the D-Bus bus URI for domdisplay > > > conf: add support > > > qemu: add audio type 'dbus' > > > conf: add dbus > > > qemu: add dbus clipboard sharing > > > conf: add > > > qemu: add -chardev dbus support > > > qemu: add usbredir type 'dbus' > > > docs: document type dbus > > > > > > NEWS.rst | 7 +- > > > docs/formatdomain.rst |43 +- > > > docs/schemas/basictypes.rng | 7 + > > > docs/schemas/domaincommon.rng |71 + > > > src/bhyve/bhyve_command.c | 1 + > > > src/conf/domain_conf.c| 141 +- > > > src/conf/domain_conf.h|15 + > > > src/conf/domain_validate.c|41 +- > > > src/libxl/libxl_conf.c| 1 + > > > src/qemu/qemu_capabilities.c | 8 + > > > src/qemu/qemu_capabilities.h | 4 + > > > src/qemu/qemu_command.c |77 +- > > > src/qemu/qemu_domain.c| 1 + > > > src/qemu/qemu_driver.c|10 +- > > > src/qemu/qemu_extdevice.c |13 + > > > src/qemu/qemu_hotplug.c | 1 + > > > src/qemu/qemu_monitor_json.c |10 + > > > src/qemu/qemu_process.c |41 +- > > > src/qemu/qemu_validate.c |33 + > > > src/security/security_dac.c | 2 + > > > src/vmx/vmx.c | 1 + > > > .../domaincapsdata/qemu_7.0.0-q35.x86_64.xml | 231 + > > > .../domaincapsdata/qemu_7.0.0-tcg.x86_64.xml | 237 + > > > tests/domaincapsdata/qemu_7.0.0.x86_64.xml| 231 + > > > .../caps_6.1.0.x86_64.xml | 1 + > > > .../caps_6.2.0.aarch64.xml| 1 + > > > .../caps_6.2.0.x86_64.xml | 1 + > > > .../caps_7.0.0.x86_64.replies | 37335 > > > .../caps_7.0.0.x86_64.xml | 3720 ++ > > > .../graphics-dbus-address.args|30 + > > > .../graphics-dbus-address.xml |35 + > > > .../qemuxml2argvdata/graphics-dbus-audio.args |33 + > > > .../qemuxml2argvdata/graphics-dbus-audio.xml |45 + > > > .../graphics-dbus-chardev.args
Re: [libvirt PATCH v3 00/16] Add QEMU "-display dbus" support
On Wed, Dec 22, 2021 at 2:44 PM wrote: > > From: Marc-André Lureau > > Hi, > > This series implements supports for the uQEMU "-display dbus" support, that > landed earlier this week for 7.0. > > By default, libvirt will start a private VM bus (sharing and reusing the > existing "vmstate" VM bus & code). > > The feature set should cover the needs to replace Spice as local client of > choice, > including 3daccel/dmabuf, audio, clipboard sharing, usb redirection, and > arbitrary > chardev/channels (for serial etc). > > The test Gtk4 client is also in progress, currently in development at > https://gitlab.com/marcandre.lureau/qemu-display/. A few dependencies, such as > zbus, require an upcoming release. virt-viewer & boxes will need a port to > Gtk4 > to make use of the shared widget. > > Comments welcome, as we can still adjust the QEMU side etc. > > thanks > > v3: after QEMU 7.0 dev cycle opening and merge > - rebased > - add 7.0 x86-64 capabilities (instead of tweaking 6.2) > - fix version annotations > > Marc-André Lureau (16): > qemu: add chardev-vdagent capability check > qemu: add -display dbus capability check > qemucapabilitiestest: Add x64 test data for the qemu-7.0 development > cycle > conf: add > qemu: start the D-Bus daemon for the display > qemu: add -display dbus support > virsh: refactor/split cmdDomDisplay() > virsh: report the D-Bus bus URI for domdisplay > conf: add support > qemu: add audio type 'dbus' > conf: add dbus > qemu: add dbus clipboard sharing > conf: add > qemu: add -chardev dbus support > qemu: add usbredir type 'dbus' > docs: document type dbus > > NEWS.rst | 7 +- > docs/formatdomain.rst |43 +- > docs/schemas/basictypes.rng | 7 + > docs/schemas/domaincommon.rng |71 + > src/bhyve/bhyve_command.c | 1 + > src/conf/domain_conf.c| 141 +- > src/conf/domain_conf.h|15 + > src/conf/domain_validate.c|41 +- > src/libxl/libxl_conf.c| 1 + > src/qemu/qemu_capabilities.c | 8 + > src/qemu/qemu_capabilities.h | 4 + > src/qemu/qemu_command.c |77 +- > src/qemu/qemu_domain.c| 1 + > src/qemu/qemu_driver.c|10 +- > src/qemu/qemu_extdevice.c |13 + > src/qemu/qemu_hotplug.c | 1 + > src/qemu/qemu_monitor_json.c |10 + > src/qemu/qemu_process.c |41 +- > src/qemu/qemu_validate.c |33 + > src/security/security_dac.c | 2 + > src/vmx/vmx.c | 1 + > .../domaincapsdata/qemu_7.0.0-q35.x86_64.xml | 231 + > .../domaincapsdata/qemu_7.0.0-tcg.x86_64.xml | 237 + > tests/domaincapsdata/qemu_7.0.0.x86_64.xml| 231 + > .../caps_6.1.0.x86_64.xml | 1 + > .../caps_6.2.0.aarch64.xml| 1 + > .../caps_6.2.0.x86_64.xml | 1 + > .../caps_7.0.0.x86_64.replies | 37335 > .../caps_7.0.0.x86_64.xml | 3720 ++ > .../graphics-dbus-address.args|30 + > .../graphics-dbus-address.xml |35 + > .../qemuxml2argvdata/graphics-dbus-audio.args |33 + > .../qemuxml2argvdata/graphics-dbus-audio.xml |45 + > .../graphics-dbus-chardev.args|32 + > .../graphics-dbus-chardev.xml |43 + > .../graphics-dbus-clipboard.args |31 + > .../graphics-dbus-clipboard.xml |35 + > tests/qemuxml2argvdata/graphics-dbus-p2p.args |30 + > tests/qemuxml2argvdata/graphics-dbus-p2p.xml |33 + > .../graphics-dbus-usbredir.args |34 + > .../graphics-dbus-usbredir.xml|30 + > tests/qemuxml2argvdata/graphics-dbus.args |30 + > tests/qemuxml2argvdata/graphics-dbus.xml |33 + > tests/qemuxml2argvtest.c |22 + > .../graphics-dbus-address.xml | 1 + > .../graphics-dbus-audio.xml | 1 + > .../graphics-dbus-chardev.xml | 1 + > .../graphics-dbus-clipboard.xml | 1 + > .../qemuxml2xmloutdata/graphics-dbus-p2p.xml | 1 + > tests/qemuxml2xmloutdata/graphics-dbus.xml| 1 + > tests/qemuxml2xmltest.c |20 + > tools/virsh-domain.c | 366 +- > 52 files changed, 42981 insertions(+), 192 deletions(-) > create mode 100644 tests/domaincapsdata/qemu_7.0.0-q35.x86_64.xml > create mode 100644 tests/domaincapsdata/qemu_7.0.0-tcg.x86_64.xml > create mode 100644
Re: [libvirt PATCH 0/3] storage: Use the FICLONE ioctl unconditionally on Linux
On Tue, Dec 28, 2021 at 1:40 PM Andrea Bolognani wrote: > > Test pipeline: https://gitlab.com/abologna/libvirt/-/pipelines/438075320 > > Andrea Bolognani (3): > storage: Use the FICLONE ioctl unconditionally on Linux > meson: Don't look for btrfs and xfs headers > spec: Drop BuildRequires on xfsprogs-devel > > libvirt.spec.in| 2 -- > meson.build| 4 > src/storage/storage_util.c | 14 ++ > 3 files changed, 2 insertions(+), 18 deletions(-) > > -- > 2.31.1 > > LGTM. Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH V3 0/2] Apparmor: Add profiles for hypervisor daemons
On Thu, Jun 24, 2021 at 4:49 PM Jim Fehlig wrote: > > V2: https://listman.redhat.com/archives/libvir-list/2021-June/msg00676.html > V1: https://listman.redhat.com/archives/libvir-list/2021-June/msg00456.html > > Changes since V2: > Patches 3 and 4 ACKed and pushed since they are bug fixes independent of > modular vs monolithic daemons. > > The qemu_bridge_helper subprofile in patch 1 was adjusted for > communication with virtqemud instead of libvirtd. > > After snooping through git history, I found a few capabilities explicitly > added for xen that have been added back to the virtxend profile. > > Note: The profile for virtlxcd will have to wait until the following > issue is fixed > > https://gitlab.com/libvirt/libvirt/-/issues/181 > > Jim Fehlig (2): > Apparmor: Add profile for virtqemud > Apparmor: Add profile for virtxend > > src/security/apparmor/libvirt-qemu | 3 + > src/security/apparmor/meson.build | 2 + > src/security/apparmor/usr.sbin.virtqemud.in | 134 > src/security/apparmor/usr.sbin.virtxend.in | 55 > 4 files changed, 194 insertions(+) > create mode 100644 src/security/apparmor/usr.sbin.virtqemud.in > create mode 100644 src/security/apparmor/usr.sbin.virtxend.in > > -- > 2.31.1 > > Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH] virt-aa-helper: Allow swtpm to fsync on dir
On Tue, Jul 13, 2021 at 2:42 PM Stefan Berger wrote: > > Allow swtpm (0.7.0 or later) to fsync on the directory where it writes > its state files into so that "the entry in the directory containing the > file has also reached disk" (fsync(2)). > > Signed-off-by: Stefan Berger > --- > src/security/virt-aa-helper.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c > index 52cfebf6e0..e21557c810 100644 > --- a/src/security/virt-aa-helper.c > +++ b/src/security/virt-aa-helper.c > @@ -1250,8 +1250,11 @@ get_files(vahControl * ctl) > " \"%s/libvirt/qemu/swtpm/%s-swtpm.sock\" rw,\n", > RUNSTATEDIR, shortName); > /* Paths for swtpm to use: give it access to its state > - * directory, log, and PID files. > + * directory (state files and fsync on dir), log, and PID files. > */ > +virBufferAsprintf(, > +" \"%s/lib/libvirt/swtpm/%s/%s/\" r,\n", > +LOCALSTATEDIR, uuidstr, tpmpath); > virBufferAsprintf(, > " \"%s/lib/libvirt/swtpm/%s/%s/**\" rwk,\n", > LOCALSTATEDIR, uuidstr, tpmpath); > -- > 2.31.1 > Patch looks fine to me. Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCH 1/2] docs: point go imports to gitlab.com repos
On Fri, Jul 2, 2021 at 8:35 AM Andrea Bolognani wrote: > > On Fri, Jul 02, 2021 at 11:54:54AM +0100, Daniel P. Berrangé wrote: > > > > - https://libvirt.org/git/libvirt-go.git"/> > > + https://gitlab.com/libvirt/libvirt-go"/> > > > > > > - https://libvirt.org/git/libvirt-go-xml.git"/> > > + https://gitlab.com/libvirt/libvirt-go-xml"/> > > > > I believe in terms of git commands it's more appropriate to use > > https://gitlab.com/libvirt/libvirt-go.git > https://gitlab.com/libvirt/libvirt-go-xml.git > > but both will work due to redirects and such. So either way > > Reviewed-by: Andrea Bolognani > The redirects have not consistently worked in the past, so I would ask that you use the .git URLs Andrea mentioned. -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH] apparmor: Add denied capabilities
On Tue, Jun 8, 2021 at 1:35 PM Jim Fehlig wrote:
>
> On 6/7/21 5:43 PM, Neal Gompa wrote:
> > On Mon, Jun 7, 2021 at 6:34 PM Jim Fehlig wrote:
> >>
> >> The audit log contains the following denials from libvirtd
> >>
> >> apparmor="DENIED" operation="capable" profile="libvirtd" pid=6012
> >> comm="daemon-init" capability=17 capname="sys_rawio"
> >> apparmor="DENIED" operation="capable" profile="libvirtd" pid=6012
> >> comm="rpc-worker" capability=39 capname="bpf"
> >> apparmor="DENIED" operation="capable" profile="libvirtd" pid=6012
> >> comm="rpc-worker" capability=38 capname="perfmon"
> >>
> >> Squelch the denials and allow the capabilities in the libvirtd
> >> apparmor profile.
> >>
> >> Signed-off-by: Jim Fehlig
> >> ---
> >>
> >> I'm not really sure when these denials first started appearing, nor
> >> have I noticed any problems they are causing. Likely I have not exercised
> >> the affected functionality.
> >>
> >> src/security/apparmor/usr.sbin.libvirtd.in | 3 +++
> >> 1 file changed, 3 insertions(+)
> >>
> >> diff --git a/src/security/apparmor/usr.sbin.libvirtd.in
> >> b/src/security/apparmor/usr.sbin.libvirtd.in
> >> index bf4563e1e8..928782b709 100644
> >> --- a/src/security/apparmor/usr.sbin.libvirtd.in
> >> +++ b/src/security/apparmor/usr.sbin.libvirtd.in
> >> @@ -25,6 +25,9 @@ profile libvirtd @sbindir@/libvirtd
> >> flags=(attach_disconnected) {
> >> capability fsetid,
> >> capability audit_write,
> >> capability ipc_lock,
> >> + capability sys_rawio,
> >> + capability bpf,
> >> + capability perfmon,
> >>
> >> # Needed for vfio
> >> capability sys_resource,
> >> --
> >> 2.31.1
> >>
> >>
> >
> > The patch LGTM, but the title is confusing. Maybe the following?
> >
> > "apparmor: Permit new capabilities required by libvirt"
>
> Reading again, I agree it is poorly worded. I used your suggestion, but
> s/libvirt/libvirtd/.
>
Works for me.
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH] apparmor: Add denied capabilities
On Mon, Jun 7, 2021 at 6:34 PM Jim Fehlig wrote:
>
> The audit log contains the following denials from libvirtd
>
> apparmor="DENIED" operation="capable" profile="libvirtd" pid=6012
> comm="daemon-init" capability=17 capname="sys_rawio"
> apparmor="DENIED" operation="capable" profile="libvirtd" pid=6012
> comm="rpc-worker" capability=39 capname="bpf"
> apparmor="DENIED" operation="capable" profile="libvirtd" pid=6012
> comm="rpc-worker" capability=38 capname="perfmon"
>
> Squelch the denials and allow the capabilities in the libvirtd
> apparmor profile.
>
> Signed-off-by: Jim Fehlig
> ---
>
> I'm not really sure when these denials first started appearing, nor
> have I noticed any problems they are causing. Likely I have not exercised
> the affected functionality.
>
> src/security/apparmor/usr.sbin.libvirtd.in | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/src/security/apparmor/usr.sbin.libvirtd.in
> b/src/security/apparmor/usr.sbin.libvirtd.in
> index bf4563e1e8..928782b709 100644
> --- a/src/security/apparmor/usr.sbin.libvirtd.in
> +++ b/src/security/apparmor/usr.sbin.libvirtd.in
> @@ -25,6 +25,9 @@ profile libvirtd @sbindir@/libvirtd
> flags=(attach_disconnected) {
>capability fsetid,
>capability audit_write,
>capability ipc_lock,
> + capability sys_rawio,
> + capability bpf,
> + capability perfmon,
>
># Needed for vfio
>capability sys_resource,
> --
> 2.31.1
>
>
The patch LGTM, but the title is confusing. Maybe the following?
"apparmor: Permit new capabilities required by libvirt"
Otherwise...
Reviewed-by: Neal Gompa
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH RESEND v3 8/8] libvirt.spec: drop no longer required build dependencies
On Tue, May 25, 2021 at 11:34 AM Andrea Bolognani wrote: > > On Tue, May 25, 2021 at 05:06:41PM +0200, Michal Prívozník wrote: > > On 5/25/21 12:00 PM, Andrea Bolognani wrote: > > > On Tue, May 25, 2021 at 10:00:47AM +0200, Pavel Hrdina wrote: > > >> -# For storage wiping with different algorithms > > >> -BuildRequires: scrub > > > > > > Turns out we're actually missing the runtime dependency on scrub! Can > > > you please take care of addressing that issue in a separate patch? > > > > Are we? scrub is needed if and only if you want to pass a special > > algorithm to virStorageVolWipePattern(). Does that justify a runtime > > dependency? We fail gracefully if scrub isn't installed. We aren't even > > requiring qemu binary for daemon-driver-qemu package. > > > > Wasn't the whole idea to drop dependencies? > > The point was to drop **build** dependencies, specifically > problematic ones such as ZFS. When it comes to runtime dependencies, > our RPMs generally try to be as featureful as possible, so adding a > dependency on scrub is the right thing to do IMO. > Why is ZFS problematic? The zfs-fuse package is totally fine for us to depend on... -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH] rpm: re-enable ppc64 on RHEL-8
On Fri, May 14, 2021 at 8:24 AM Andrea Bolognani wrote: > > On Fri, May 14, 2021 at 12:57:24PM +0100, Daniel P. Berrangé wrote: > > Historically PowerPC 64 was always supported with qemu-kvm in RHEL. > > > > In future RHEL-9 it is being discontinued and this was addressed > > in > > > > commit 03cc3c9064322ac4028a2213105cd230fe28c013 > > Author: Jiri Denemark > > Date: Wed Apr 21 14:55:03 2021 +0200 > > > > spec: Do not build qemu driver for Power on RHEL-9 > > > > when the specfile was cleaned up to remove RHEL-7 support: > > > > commit 0f601d2f868f2017cdd16e0a7ca90a59e7d5e120 > > Author: Andrea Bolognani > > Date: Wed May 5 19:30:46 2021 +0200 > > > > spec: Bump min_fedora and min_rhel > > > > it also removed the logic that applied to RHEL-8 wrt arch list > > and lost PowerPC 64 support on 8. This reverts that part of the > > change but with the condition reversed to prioritize the future > > state. > > > > Signed-off-by: Daniel P. Berrangé > > --- > > libvirt.spec.in | 6 +- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > My bad! Thanks for catching and fixing this :) > > Reviewed-by: Andrea Bolognani > Mechanically, this change is fine, so... Reviewed-by: Neal Gompa But, I'm kind of surprised that this is going away, since IBM has put special attention on improving PowerKVM support specifically for RHEL, so this seems really weird... -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH] rpm: disable glusterfs on RHEL-9
On Fri, May 14, 2021 at 8:27 AM Andrea Bolognani wrote:
>
> On Fri, May 14, 2021 at 12:58:09PM +0100, Daniel P. Berrangé wrote:
> > Support for glusterfs with KVM is being dropped in RHEL-9 in the
> > virtualization stack.
> >
> > Signed-off-by: Daniel P. Berrangé
> > ---
> > libvirt.spec.in | 11 ---
> > 1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/libvirt.spec.in b/libvirt.spec.in
> > index 8ac324be0a..c52f607bd1 100644
> > --- a/libvirt.spec.in
> > +++ b/libvirt.spec.in
> > @@ -65,10 +65,15 @@
> > %endif
> >
> > %define with_storage_gluster 0%{!?_without_storage_gluster:1}
> > -%ifnarch %{arches_qemu_kvm}
> > -# gluster is only built where qemu driver is enabled on RHEL
> > -%if 0%{?rhel}
> > +%if 0%{?rhel}
> > +# Glusterfs dropped in RHEL-9, and before that only
>
> *has been dropped
>
> Reviewed-by: Andrea Bolognani
>
Press "F" to pay respects to GlusterFS
Reviewed-by: Neal Gompa
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH 2/2] rpm: Drop unnecessary libiscsi runtime dependency
On Mon, Jan 11, 2021 at 10:52 AM Andrea Bolognani wrote:
>
> On Thu, 2021-01-07 at 13:48 -0500, Neal Gompa wrote:
> > On Thu, Jan 7, 2021 at 12:38 PM Jiri Denemark wrote:
> > > On Thu, Jan 07, 2021 at 09:58:09 -0500, Neal Gompa wrote:
> > > > +++ b/libvirt.spec.in
> > > > @@ -614,7 +614,6 @@ volumes using the host iscsi stack.
> > > > Summary: Storage driver plugin for iscsi-direct
> > > > Requires: libvirt-daemon-driver-storage-core = %{version}-%{release}
> > > > Requires: libvirt-libs = %{version}-%{release}
> > > > -Requires: libiscsi
> > >
> > > The explicit dependency was added by Andrea 2.5 years ago, perhaps he
> > > had reasons to do so. Any comments Andrea?
> >
> > It most likely dates back to when Fedora had two providers of libiscsi
> > sharing the same soname. That situation no longer exists today. Other
> > distributions also don't have that issue.
>
> I didn't offer much in the way of explanation for the change in
>
> commit fe5b35c6b29dc952babf4436ccba83c4a0ffa82e
> Author: Andrea Bolognani
> Date: Tue Aug 14 14:31:35 2018 +0200
>
> spec: Enable the iscsi-direct storage driver conditionally
>
> Most distributions we build RPMs on don't ship a
> recent enough version of libiscsi, so we can't enable
> the driver unconditionally. Add an explicit dependency
> on the runtime package while at it.
>
> Signed-off-by: Andrea Bolognani
> Reviewed-by: Pavel Hrdina
>
> so I can only guess that I was mistakenly thinking the runtime
> dependency would not be added automatically.
>
> Under the assumption that Neal has already verified the set of
> runtime dependencies is exactly the same, on all RPM-based targets,
> before and after this patch has been applied,
>
> Reviewed-by: Andrea Bolognani
>
> --
> Andrea Bolognani / Red Hat / Virtualization
>
So apparently this wasn't actually pushed yet... Can we get this pushed now?
--
真実はいつも一つ！/ Always, there's only one truth!
[PATCH] rpm: Set version information for libvirt-admin virtual name
The libvirt-daemon package now provides the 'libvirt-admin' virtual
name, but the Provides stanza doesn't declare version information,
which breaks things depending on that package using a versioned
dependency. Fix this by setting the version-release of libvirt to
that name to mimic the previous state.
Fixes: 2244ac168d42c3fa424bae6d33ecdbb8726da7c2
Signed-off-by: Neal Gompa
---
libvirt.spec.in | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 9dea6c6787..d7a90c42f5 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -415,7 +415,7 @@ Requires: gettext
# Ensure smooth upgrades
Obsoletes: libvirt-admin < 7.3.0
-Provides: libvirt-admin
+Provides: libvirt-admin = %{version}-%{release}
Obsoletes: libvirt-bash-completion < 7.3.0
%description daemon
--
2.31.1
Re: [libvirt PATCH 0/3] spec: Fix platform check
On Tue, May 11, 2021 at 11:21 AM Andrea Bolognani wrote: > > > > Andrea Bolognani (3): > spec: Reintroduce supported_platform variable > spec: Move definition of supported_platform variable > spec: Simplify platform check > > libvirt.spec.in | 12 +--- > mingw-libvirt.spec.in | 12 +--- > 2 files changed, 18 insertions(+), 6 deletions(-) > > -- > 2.26.3 > > This resolves my issues. Series LGTM. Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCH 3/3] spec: Drop supported_platform variable
On Wed, May 5, 2021 at 4:12 PM Andrea Bolognani wrote:
>
> It's only used in one place, and it's nicer to keep the error
> message close to the check that causes it to be emitted.
>
> Signed-off-by: Andrea Bolognani
> ---
> libvirt.spec.in | 12 +++-
> mingw-libvirt.spec.in | 12 +++-
> 2 files changed, 6 insertions(+), 18 deletions(-)
>
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index 013c7742a2..9dea6c6787 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -6,12 +6,6 @@
> %define min_rhel 8
> %define min_fedora 33
>
> -%if 0%{?fedora} >= %{min_fedora} || 0%{?rhel} >= %{min_rhel}
> -%define supported_platform 1
> -%else
> -%define supported_platform 0
> -%endif
> -
> %define arches_qemu_kvm %{ix86} x86_64 %{power64} %{arm} aarch64
> s390x
> %if 0%{?rhel}
> %define arches_qemu_kvm x86_64 aarch64 s390x
> @@ -929,9 +923,9 @@ Libvirt plugin for NSS for translating domain names into
> IP addresses.
> %autosetup -S git_am
>
> %build
> -%if ! %{supported_platform}
> -echo "This RPM requires either Fedora >= %{min_fedora} or RHEL >=
> %{min_rhel}"
> -exit 1
> +%if (0%{?fedora} && 0%{?fedora} < %{min_fedora}) || (0%{?rhel} && 0%{?rhel}
> < %{min_rhel})
> +echo "This RPM requires either Fedora >= %{min_fedora} or RHEL >=
> %{min_rhel}"
> +exit 1
> %endif
>
This broke my builds locally, because now the libvirt spec throws an
error on Fedora and RHEL. Your conditional logic will always trigger
an error because you're now checking to see if *either* Fedora < 33 or
RHEL < 7, and that will always be true and fail the build. :(
I'm going to send a fixup for this.
> %if %{with_qemu}
> diff --git a/mingw-libvirt.spec.in b/mingw-libvirt.spec.in
> index 84b8998f74..61a4843fb3 100644
> --- a/mingw-libvirt.spec.in
> +++ b/mingw-libvirt.spec.in
> @@ -5,12 +5,6 @@
> # or versions, but no effort will be made to ensure that going forward.
> %define min_fedora 33
>
> -%if 0%{?fedora} && 0%{?fedora} >= %{min_fedora}
> -%define supported_platform 1
> -%else
> -%define supported_platform 0
> -%endif
> -
> Name: mingw-libvirt
> Version:@VERSION@
> Release:1%{?dist}
> @@ -95,9 +89,9 @@ MinGW Windows libvirt virtualization library.
> %setup -q -n libvirt-%{version}
>
> %build
> -%if ! %{supported_platform}
> -echo "This RPM requires Fedora >= %{min_fedora}"
> -exit 1
> +%if 0%{?fedora} < %{min_fedora}
> +echo "This RPM requires Fedora >= %{min_fedora}"
> +exit 1
> %endif
>
> %mingw_meson \
> --
> 2.26.3
>
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCH v4 0/7] spec: Reorganize some packages
On Tue, Apr 20, 2021 at 1:13 PM Andrea Bolognani wrote:
>
> Changes from [v3]:
>
> * generate per-command bash completion scripts using Meson instead
> of tweaking the contents after the fact for RPM build;
>
> * update documentation as files are moved around and packages are
> dropped.
>
> Changes from [v2]:
>
> * move virt-admin to -daemon rather than -client;
>
> * move other host-only tools from -client to -daemon;
>
> * move systemtap probes from -client to -libs.
>
> Changes from [v1]:
>
> * add Obsoletes/Provides for a smooth transition.
>
> [v3] https://listman.redhat.com/archives/libvir-list/2021-April/msg00900.html
> [v2] https://listman.redhat.com/archives/libvir-list/2021-April/msg00613.html
> [v1] https://listman.redhat.com/archives/libvir-list/2021-April/msg00604.html
>
> Andrea Bolognani (7):
> docs: Use consistent vertical spacing
> docs: Expand upon the contents of the -daemon package
> spec: Merge -admin package into -daemon
> spec: Move some files from -client to -daemon
> spec: Move systemtap probes from -client to -libs
> tools: Generate per-command bash completion script
> spec: Drop -bash-completion package
>
> docs/kbase/rpm-deployment.rst | 55 ---
> libvirt.spec.in | 76 ++-
> tools/bash-completion/meson.build | 24 +
> tools/bash-completion/{vsh => vsh.in} | 7 ++-
> 4 files changed, 54 insertions(+), 108 deletions(-)
> rename tools/bash-completion/{vsh => vsh.in} (91%)
>
> --
> 2.26.3
>
>
Series LGTM.
Reviewed-by: Neal Gompa
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCH v2 00/21] cleanup meson checks for runtime binaries
On Mon, Apr 19, 2021 at 1:14 PM Pavel Hrdina wrote: > > Recent attempt to add a lot of meson options to specify different > runtime paths motivated me enough to cleanup this from meson. > > Changes in v2: > - split and rework patch 16/17 to address review comments > - added a new patch to cleanup libvirt.spec.in file > > Pavel Hrdina (21): > bridge_driver: fix comment about dnsmasqCaps > virdnsmasq: drop unused dnsmasqCapsNewFromFile function > virdnsmasq: drop unused dnsmasqCapsRefresh function > virdnsmasq: remove binaryPath argument from dnsmasqCapsNewFromBuffer > virdnsmasq: remove binaryPath argument from dnsmasqCapsNewFromBinary > virfirewall: use virFindFileInPath instead of virFileIsExecutable > tests: introduce virfirewallmock > tests: use virfirewallmock instead of hasNetfilterTools > virnetdev: move virNetDevSetRootQDisc to virnetdevbandwidth > tests: testutilsqemu: move virFindFileInPath into domaincapsmock > meson: don't check collie as program for sheepdog > bhyvexml2argvtest: use virCommandToStringFull to strip command path > storage: use virFindFileInPath to validate presence of mkfs > virfile: introduce virFindFileInPathFull() > qemu_conf: use virFindFileInPathFull for runtime binaries > meson: drop check for runtime binary dependencies > meson: move iscsiadm check into storage_iscsi condition > meson: stop setting runtime binaries defines during compilation > meson: use runtime binaries to only resolve features with "auto" value > meson: optional_programs should be used only for building libvirt > libvirt.spec: drop no longer required build dependencies > > libvirt.spec.in | 31 --- > meson.build | 214 ++ > src/bhyve/bhyve_command.c | 4 + > src/libvirt_private.syms | 6 +- > src/locking/lock_driver_lockd.c | 12 +- > src/network/bridge_driver.c | 8 +- > src/node_device/node_device_driver.c | 2 + > src/qemu/qemu_conf.c | 23 +- > src/qemu/qemu_domain.c| 3 +- > src/storage/storage_backend_fs.c | 24 +- > src/storage/storage_backend_logical.c | 13 ++ > src/storage/storage_backend_sheepdog.c| 2 + > src/storage/storage_backend_zfs.c | 3 + > src/storage/storage_util.c| 2 + > src/storage/storage_util.h| 6 + > src/util/virdnsmasq.c | 56 + > src/util/virdnsmasq.h | 8 +- > src/util/virfile.c| 16 +- > src/util/virfile.h| 6 +- > src/util/virfirewall.c| 4 +- > src/util/virfirewall.h| 4 + > src/util/viriscsi.h | 2 + > src/util/virkmod.h| 3 + > src/util/virnetdev.c | 46 > src/util/virnetdev.h | 4 - > src/util/virnetdevbandwidth.c | 50 > src/util/virnetdevbandwidth.h | 6 + > src/util/virnetdevip.c| 2 + > src/util/virnetdevmidonet.c | 2 + > src/util/virnetdevopenvswitch.c | 2 + > src/util/virnuma.c| 1 + > src/util/virsysinfo.c | 1 + > src/util/virutil.c| 2 + > .../bhyvexml2argv-acpiapic.args | 2 +- > .../bhyvexml2argv-acpiapic.ldargs | 2 +- > ...ml2argv-addr-isa-controller-on-slot-1.args | 2 +- > ...2argv-addr-isa-controller-on-slot-1.ldargs | 2 +- > ...l2argv-addr-isa-controller-on-slot-31.args | 2 +- > ...argv-addr-isa-controller-on-slot-31.ldargs | 2 +- > ...xml2argv-addr-more-than-32-sata-disks.args | 2 +- > ...l2argv-addr-more-than-32-sata-disks.ldargs | 2 +- > ...hyvexml2argv-addr-multiple-sata-disks.args | 2 +- > ...vexml2argv-addr-multiple-sata-disks.ldargs | 2 +- > ...vexml2argv-addr-multiple-virtio-disks.args | 2 +- > ...xml2argv-addr-multiple-virtio-disks.ldargs | 2 +- > ...rgv-addr-no32devs-multiple-sata-disks.args | 2 +- > ...v-addr-no32devs-multiple-sata-disks.ldargs | 2 +- > ...l2argv-addr-no32devs-single-sata-disk.args | 2 +- > ...argv-addr-no32devs-single-sata-disk.ldargs | 2 +- > ...rgv-addr-non-isa-controller-on-slot-1.args | 2 +- > .../bhyvexml2argv-addr-single-sata-disk.args | 2 +- > ...bhyvexml2argv-addr-single-sata-disk.ldargs | 2 +- > ...bhyvexml2argv-addr-single-virtio-disk.args | 2 +- > ...yvexml2argv-addr-single-virtio-disk.ldargs | 2 +- > .../bhyvexml2argvdata/bhyvexml2argv-base.args | 2 +- > .../bhyvexml2argv-base.ldargs | 2 +- > .../bhyvexml2argv-bhyveload-bootorder.args| 2 +- >
Re: [libvirt PATCH 00/17] drop meson checks for runtime binaries
On Mon, Apr 19, 2021 at 11:10 AM Pavel Hrdina wrote: > > On Mon, Apr 19, 2021 at 01:59:52PM +0100, Daniel P. Berrangé wrote: > > On Mon, Apr 19, 2021 at 02:50:13PM +0200, Michal Privoznik wrote: > > > On 4/19/21 2:24 PM, Neal Gompa wrote: > > > > On Fri, Apr 16, 2021 at 3:13 PM Pavel Hrdina wrote: > > > > > > > > > > > > > > > > > > I don't think this is a good "cleanup" to do. Having these checks is > > > > useful since without them, we'd blindly build modules that possibly > > > > wouldn't work because we haven't verified that those dependencies > > > > exist. People do install from source into runtime (I don't, but people > > > > do), and it's useful for making sure all the necessary dependencies > > > > are captured for runtime use at build-time for package builds (I've > > > > caught mistakes because of these). > > > > > > To be fair though, some cleanups Pavel did are worth merging (e.g. couple > > > of > > > first patches that fix comments or remove unused functions) regardless of > > > ... > > > > > > > > > > > So I NACK the whole series. > > > > > > > > > > > > > > this NACK. What I am worried about is that usually, when a distro builds > > > libvirt package a path to a runtime binary will be recorded (e.g. DNSMASQ > > > will be expanded to /usr/sbin/dnsmasq and compiled in). This way, we will > > > try to find "dnsmasq" in PATH, which may work for qemu:///system, but may > > > lead to unexpected results for qemu:///session because for instance I > > > override PATH for my regular user so that a directory with my helper > > > scripts > > > comes first. Let's hope that I won't pick wrong name for my scripts. > > > > I think that's actually the desirable situation for libvirtd running > > as non-root. If the user overrides a system binary with an alternate > > impl, it is right that we honour that. > > Agreed. > > > I think we ought to consider two parts to this series - honouring $PATH, > > and probing at meson time. We can have the former, without changing > > the latter, so we still get feature auto-detection at build time to > > avoid uncessarily building stuff that's not appropriate for the OS > > in question. > > I was about to propose that as well. We can have check for some binaries > in meson to figure out if something should be enabled or not for > features that have value set to "auto" so by default meson would do the > "right" thing. > > But if a user/developer explicitly enables some feature, for example > iscsi storage we should not error out if the binary is missing because > the code can be compiled without the binary. This would also allow > package maintainers to reduce a list of build dependencies that are > required. > > I'll post a v2 with this approach. So, I think you *shouldn't* do it that way. The problem with doing this is that we can wind up with mismatched capabilities and non-functional libvirt build based on the assumption that things will just "be there" with no check that they will actually be there. In other words, runtime executable dependencies should be treated *exactly* as they are now, because we have no other avenue to guarantee that things work for a given installation. -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCH 00/17] drop meson checks for runtime binaries
On Fri, Apr 16, 2021 at 3:13 PM Pavel Hrdina wrote: > > Recent attempt to add a lot of meson options to specify different > runtime paths motivated me enough to cleanup this from meson. > > Pavel Hrdina (17): > bridge_driver: fix comment about dnsmasqCaps > virdnsmasq: drop unused dnsmasqCapsNewFromFile function > virdnsmasq: drop unused dnsmasqCapsRefresh function > virdnsmasq: remove binaryPath argument from dnsmasqCapsNewFromBuffer > virdnsmasq: remove binaryPath argument from dnsmasqCapsNewFromBinary > virfirewall: use virFindFileInPath instead of virFileIsExecutable > tests: introduce virfirewallmock > tests: use virfirewallmock instead of hasNetfilterTools > virnetdev: move virNetDevSetRootQDisc to virnetdevbandwidth > tests: testutilsqemu: move virFindFileInPath into domaincapsmock > meson: don't check collie as program for sheepdog > bhyvexml2argvtest: use virCommandToStringFull to strip command path > storage: use virFindFileInPath to validate presence of mkfs > virfile: introduce virFindFileInPathFull() > qemu_conf: use virFindFileInPathFull for runtime binaries > meson: drop check for runtime binary dependencies > meson: optional_programs should be used only for building libvirt > > meson.build | 180 +- > src/bhyve/bhyve_command.c | 4 + > src/libvirt_private.syms | 6 +- > src/locking/lock_driver_lockd.c | 12 +- > src/network/bridge_driver.c | 8 +- > src/node_device/node_device_driver.c | 2 + > src/qemu/qemu_conf.c | 23 ++- > src/qemu/qemu_domain.c| 3 +- > src/storage/storage_backend_fs.c | 24 +-- > src/storage/storage_backend_logical.c | 13 ++ > src/storage/storage_backend_sheepdog.c| 2 + > src/storage/storage_backend_zfs.c | 3 + > src/storage/storage_util.c| 2 + > src/storage/storage_util.h| 6 + > src/util/virdnsmasq.c | 56 +- > src/util/virdnsmasq.h | 8 +- > src/util/virfile.c| 16 +- > src/util/virfile.h| 6 +- > src/util/virfirewall.c| 4 +- > src/util/virfirewall.h| 4 + > src/util/viriscsi.h | 2 + > src/util/virkmod.h| 3 + > src/util/virnetdev.c | 46 - > src/util/virnetdev.h | 4 - > src/util/virnetdevbandwidth.c | 50 + > src/util/virnetdevbandwidth.h | 6 + > src/util/virnetdevip.c| 2 + > src/util/virnetdevmidonet.c | 2 + > src/util/virnetdevopenvswitch.c | 2 + > src/util/virnuma.c| 1 + > src/util/virsysinfo.c | 1 + > src/util/virutil.c| 2 + > .../bhyvexml2argv-acpiapic.args | 2 +- > .../bhyvexml2argv-acpiapic.ldargs | 2 +- > ...ml2argv-addr-isa-controller-on-slot-1.args | 2 +- > ...2argv-addr-isa-controller-on-slot-1.ldargs | 2 +- > ...l2argv-addr-isa-controller-on-slot-31.args | 2 +- > ...argv-addr-isa-controller-on-slot-31.ldargs | 2 +- > ...xml2argv-addr-more-than-32-sata-disks.args | 2 +- > ...l2argv-addr-more-than-32-sata-disks.ldargs | 2 +- > ...hyvexml2argv-addr-multiple-sata-disks.args | 2 +- > ...vexml2argv-addr-multiple-sata-disks.ldargs | 2 +- > ...vexml2argv-addr-multiple-virtio-disks.args | 2 +- > ...xml2argv-addr-multiple-virtio-disks.ldargs | 2 +- > ...rgv-addr-no32devs-multiple-sata-disks.args | 2 +- > ...v-addr-no32devs-multiple-sata-disks.ldargs | 2 +- > ...l2argv-addr-no32devs-single-sata-disk.args | 2 +- > ...argv-addr-no32devs-single-sata-disk.ldargs | 2 +- > ...rgv-addr-non-isa-controller-on-slot-1.args | 2 +- > .../bhyvexml2argv-addr-single-sata-disk.args | 2 +- > ...bhyvexml2argv-addr-single-sata-disk.ldargs | 2 +- > ...bhyvexml2argv-addr-single-virtio-disk.args | 2 +- > ...yvexml2argv-addr-single-virtio-disk.ldargs | 2 +- > .../bhyvexml2argvdata/bhyvexml2argv-base.args | 2 +- > .../bhyvexml2argv-base.ldargs | 2 +- > .../bhyvexml2argv-bhyveload-bootorder.args| 2 +- > .../bhyvexml2argv-bhyveload-bootorder.ldargs | 2 +- > .../bhyvexml2argv-bhyveload-bootorder1.args | 2 +- > .../bhyvexml2argv-bhyveload-bootorder1.ldargs | 2 +- > .../bhyvexml2argv-bhyveload-bootorder3.args | 2 +- > .../bhyvexml2argv-bhyveload-bootorder3.ldargs | 2 +- > .../bhyvexml2argv-bhyveload-explicitargs.args | 2 +- > ...hyvexml2argv-bhyveload-explicitargs.ldargs | 2 +- > .../bhyvexml2argv-commandline.args| 2 +- >
Re: [RFC PATCH 00/43] qemu: Remove support for qemu-1.5 - qemu-2.10 and clean up capabilities
On Fri, Apr 9, 2021 at 11:00 AM Peter Krempa wrote: > > May 7, 2021 is the 2nd anniversary of release of rhel-8, which means we > no longer have to support qemu-1.5. > > Remove the capabilities and test data for the versions we no longer care > about and clean up some capabilities related stuff: > > - remove code for QEMU_CAPS_DEVICE_VIDEO_PRIMARY > - remove checking for QEMU_CAPS_QUERY_QMP_SCHEMA since all versions now > support it > > This is also a good opportunity to clean up few things regarding the > capabilties test data, namely call 'query-qmp-schema' before > 'query-commands' to prepare to retire query-commands. > > This series is heavily truncated since there were massive (27MiB) > patches resulting from some of the mechanical (and generated) changes > and also depends on my previous command line wrapping cleanup. > Fetch the full version at: > > git fetch https://gitlab.com/pipo.sk/libvirt.git qemu-deprec-2 > > The cleanups in patches 41, 42, and the massive move of commands in > patch 43 was done using a tool based on pieces of our code which I've > uploaded to: > > https://gitlab.com/pipo.sk/libvirt/-/commits/qemu-capabilities-tool > > The tool may come in handy for other things too in the future as it > allows programatic modification of the capability replies files. > > Peter Krempa (43): > docs/platforms: Drop separate link to qemu-kvm on repology.org > qemuxml2xmltest: testInfoSetPaths: Remove return value > qemuxml2(argv|xml)test: Don't exit early when testQemuInfoSetArgs > fails > qemu: Formally deprecate support for qemu < 2.11 > NEWS: Mention that minimum supported qemu version was bumped to 2.11 > qemuxml2argvtest: Remove versioned tests for qemu < 2.11 > [fixup] qemuxml2argvdata: Remove unused output files > qemuxml2xmltest: Remove versioned tests for qemu < 2.11 > [fixup] qemuxml2xmldata: Remove unused output files > qemucapabilitiesdata: Drop capability test data for qemu < 2.11 > virQEMUCapsHasPCIMultiBus: Remove logic for PPC multibus support check > qemuAssignDeviceControllerAlias: Remove unused 'qemuCaps' argument > qemuBuildDeviceAddressStr: Remove unused 'qemuCaps' > virQEMUCapsInitProcessCaps: Remove obsolete version checks > virQEMUCapsInitQMPVersionCaps: Remove unneeded version checks > virQEMUCapsInitQMPBasicArch: Use switch for arch-based decisions > qemuxml2argvtest: Rewrite parsing of XMLs to provide earlier parsing > qemuxml2argvtest: Parse 'arch' from XML early > qemuxml2xmltest: Always include basic set of capabilities > qemu: capabilities: Move setting of QEMU_CAPS_CPU_AARCH64_OFF to > virQEMUCapsInitQMPBasicArch > qemuxml2argvtest: Remove negative test case > 'pseries-features-htp-resize' > qemu: capabilities: Move setting of PPC specific flags to > virQEMUCapsInitQMPBasicArch > qemuxml2argvtest: Remove negative test for gic v3/host > qemu: capabilities: Move setting of QEMU_CAPS_MACH_VIRT_GIC_VERSION to > virQEMUCapsInitQMPBasicArch > qemuxml2argvtest: Remove negative test case for 'net-vhostuser-multiq' > qemuxml2argvtest: Remove tests for absence of > QEMU_CAPS_DEVICE_VIDEO_PRIMARY > qemu: capabilities: Move setting QEMU_CAPS_DEVICE_VIDEO_PRIMARY to > virQEMUCapsInitQMPBasicArch > [fixup] Fix test fallout in tests/qemuxml2argvdata > [fixup] Fix test fallout in tests/qemuxml2xmloutdata > qemuxml2(argv|xml)test: Retire QEMU_CAPS_DEVICE_VIDEO_PRIMARY from > tests > qemu: capabilities: Move rest of always present caps to > virQEMUCapsInitQMPArch > qemuhotplugtest: Add also always-present capabilities > qemu: command: Remove legacy '-vga' commandline formatter > qemu_domain_address: Drop compatibility with pre-device vga > specification for i440fx > qemu_domain_address: Drop compatibility with pre-device vga > specification for q35 > qemuDomainValidateDevicePCISlotsChipsets: Remove unused @qemuCaps > qemu: capabilities: Retire QEMU_CAPS_DEVICE_VIDEO_PRIMARY > qemu: capabilities: Always assume QEMU_CAPS_QUERY_QMP_SCHEMA > qemu: monitor: Remove qemuMonitorGetEvents > qemu: monitor: Remove qemuMonitorSupportsActiveCommit > tests: qemucapabilitiesdata: Fix formatting of manually added hunk > tests: qemucapabilitiesdata: Fix wrong command identifier in > caps_4.0.0.riscv64.replies > qemu: capabilities: Probe QMP schema before probing commands > > NEWS.rst | 7 + > docs/drvqemu.rst | 2 +- > docs/platforms.rst| 2 - > src/qemu/qemu_alias.c | 5 +- > src/qemu/qemu_alias.h | 1 - > src/qemu/qemu_capabilities.c | 237 +- > src/qemu/qemu_capabilities.h | 7 +- > src/qemu/qemu_command.c | 190 +- > src/qemu/qemu_domain_address.c|82 +- > src/qemu/qemu_hotplug.c | 2 +- >
Re: [PATCH] hyperv: Handle long CPU models better.
On Thu, Apr 8, 2021 at 11:52 AM Dawid Zamirski wrote:
>
> Apparenlly exising code was dealing with stripping down Intel CPU models
> as reported by Hyper-V host but was still having issues with my AMD CPU
> for which Hyper-V returns "AMD FX(tm)-8350 Eight-Core Processor".
> Therefore, this patch deals with it by stripping out the "
> Processor" part, and if there's another CPU that we don't know of
> yet that causes trouble, trim the resulting string to 32 characters
> rather than failing as the node info has other information that are
> more useful than the model.
> ---
> src/hyperv/hyperv_driver.c | 17 -
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
> index 17f5be1f0d..6e03aa4f18 100644
> --- a/src/hyperv/hyperv_driver.c
> +++ b/src/hyperv/hyperv_driver.c
> @@ -1948,14 +1948,14 @@ hypervNodeGetInfo(virConnectPtr conn, virNodeInfoPtr
> info)
> if (STRPREFIX(tmp, " ")) {
> memmove(tmp, tmp + 1, strlen(tmp + 1) + 1);
> continue;
> -} else if (STRPREFIX(tmp, "(R)") || STRPREFIX(tmp, "(C)")) {
> +} else if (STRCASEPREFIX(tmp, "(R)") || STRCASEPREFIX(tmp, "(C)")) {
> memmove(tmp, tmp + 3, strlen(tmp + 3) + 1);
> continue;
> -} else if (STRPREFIX(tmp, "(TM)")) {
> +} else if (STRCASEPREFIX(tmp, "(TM)")) {
> memmove(tmp, tmp + 4, strlen(tmp + 4) + 1);
> continue;
> -} else if (STRPREFIX(tmp, " @ ")) {
> -/* Remove " @ X.YZGHz" from the end. */
> +} else if (STRPREFIX(tmp, " @ ") || STRPREFIX(tmp, " Processor")) {
> +/* Remove " @ X.YZGHz" or " Processor" from the end. */
> *tmp = '\0';
> break;
> }
> @@ -1963,13 +1963,12 @@ hypervNodeGetInfo(virConnectPtr conn, virNodeInfoPtr
> info)
> ++tmp;
> }
>
> +/* trim whatever is left to 32 characters - better this than nothing */
> +processorList->data->Name[31] = '\0';
> +
> /* Fill struct */
> -if (virStrcpyStatic(info->model, processorList->data->Name) < 0) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("CPU model %s too long for destination"),
> - processorList->data->Name);
> +if (virStrcpyStatic(info->model, processorList->data->Name) < 0)
> return -1;
> -}
>
> info->memory = computerSystem->data->TotalPhysicalMemory / 1024; /* byte
> to kilobyte */
> info->mhz = processorList->data->MaxClockSpeed;
> --
> 2.31.1
>
Looks reasonable to me.
When you resend with the Signed-off-by statement added, you can also add:
Reviewed-by: Neal Gompa
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH] rpc: libssh2: Enable EC host keys
On Sun, Mar 28, 2021 at 5:10 PM Bastian Germann wrote: > > libssh2 has ECDSA and ED25519 support beginning with v1.9.0. libvirt cannot > make use of those because it will handle them as unknown key types. > > Add support for those host key types. > > Signed-off-by: Bastian Germann > --- > src/rpc/virnetsshsession.c | 16 +++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/src/rpc/virnetsshsession.c b/src/rpc/virnetsshsession.c > index fe77594..c311e90 100644 > --- a/src/rpc/virnetsshsession.c > +++ b/src/rpc/virnetsshsession.c > @@ -389,7 +389,21 @@ virNetSSHCheckHostKey(virNetSSHSessionPtr sess) > case LIBSSH2_HOSTKEY_TYPE_DSS: > keyType = LIBSSH2_KNOWNHOST_KEY_SSHDSS; > break; > - > +#ifdef LIBSSH2_HOSTKEY_TYPE_ED25519 > +/* defs from libssh2 v1.9.0 or later */ > +case LIBSSH2_HOSTKEY_TYPE_ECDSA_256: > +keyType = LIBSSH2_KNOWNHOST_KEY_ECDSA_256; > +break; > +case LIBSSH2_HOSTKEY_TYPE_ECDSA_384: > +keyType = LIBSSH2_KNOWNHOST_KEY_ECDSA_384; > +break; > +case LIBSSH2_HOSTKEY_TYPE_ECDSA_521: > +keyType = LIBSSH2_KNOWNHOST_KEY_ECDSA_521; > +break; > +case LIBSSH2_HOSTKEY_TYPE_ED25519: > +keyType = LIBSSH2_KNOWNHOST_KEY_ED25519; > +break; > +#endif > case LIBSSH2_HOSTKEY_TYPE_UNKNOWN: > default: > virReportError(VIR_ERR_SSH, "%s", > -- > 2.31.0 > LGTM. Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH] rpc: libssh2: Enable EC host keys
On Sun, Mar 28, 2021 at 9:17 AM Bastian Germann
wrote:
>
> libssh2 has ECDSA and ED25519 support beginning with v1.9.0. libvirt cannot
> make use of those because it will handle them as unknown key types.
>
> Add support for those host key types.
>
> Signed-off-by: Bastian Germann
> ---
> libvirt.spec.in| 2 +-
> meson.build| 2 +-
> src/rpc/virnetsshsession.c | 12
> 3 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index f9af330186..8f5b3f126c 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -359,7 +359,7 @@ BuildRequires: libcap-ng-devel >= 0.5.0
> BuildRequires: fuse-devel >= 2.8.6
> %endif
> %if %{with_libssh2}
> -BuildRequires: libssh2-devel >= 1.3.0
> +BuildRequires: libssh2-devel >= 1.9.0
> %endif
> %if %{with_netcf}
> BuildRequires: netcf-devel >= 0.2.2
> diff --git a/meson.build b/meson.build
> index ea93a2a8ec..5e5b22107c 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1142,7 +1142,7 @@ else
>libssh_dep = dependency('', required: false)
> endif
>
> -libssh2_version = '1.3'
> +libssh2_version = '1.9'
> if get_option('driver_remote').enabled()
>libssh2_dep = dependency('libssh2', version: '>=' + libssh2_version,
> required: get_option('libssh2'))
>if libssh2_dep.found()
> diff --git a/src/rpc/virnetsshsession.c b/src/rpc/virnetsshsession.c
> index fe77594f65..cb081bcf4f 100644
> --- a/src/rpc/virnetsshsession.c
> +++ b/src/rpc/virnetsshsession.c
> @@ -389,6 +389,18 @@ virNetSSHCheckHostKey(virNetSSHSessionPtr sess)
> case LIBSSH2_HOSTKEY_TYPE_DSS:
> keyType = LIBSSH2_KNOWNHOST_KEY_SSHDSS;
> break;
> +case LIBSSH2_HOSTKEY_TYPE_ECDSA_256:
> +keyType = LIBSSH2_KNOWNHOST_KEY_ECDSA_256;
> +break;
> +case LIBSSH2_HOSTKEY_TYPE_ECDSA_384:
> +keyType = LIBSSH2_KNOWNHOST_KEY_ECDSA_384;
> +break;
> +case LIBSSH2_HOSTKEY_TYPE_ECDSA_521:
> +keyType = LIBSSH2_KNOWNHOST_KEY_ECDSA_521;
> +break;
> +case LIBSSH2_HOSTKEY_TYPE_ED25519:
> +keyType = LIBSSH2_KNOWNHOST_KEY_ED25519;
> +break;
>
> case LIBSSH2_HOSTKEY_TYPE_UNKNOWN:
> default:
> --
> 2.30.2
>
While this looks good to me, could we have this adjusted so that this
would be supported only if libssh2 >= 1.9.0 is detected and just not
add these cases when an older version is present?
libssh2 is only at 1.8.0 on Ubuntu 20.04, so this would cause it to fail there.
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH 3/3] selinux: Remove 'make' dependency
On Fri, Mar 12, 2021 at 9:01 AM Vit Mojzis wrote:
>
>
> On 3/11/21 1:29 PM, Neal Gompa wrote:
> > On Thu, Mar 11, 2021 at 6:35 AM Daniel P. Berrangé
> > wrote:
> >> On Wed, Mar 10, 2021 at 01:50:43PM -0500, Neal Gompa wrote:
> >>> On Wed, Mar 10, 2021 at 7:43 AM Nikola Knazekova
> >>> wrote:
> >>>> From: Vit Mojzis
> >>>>
> >>>> Compile the policy using a shell script executed by meson.
> >>>>
> >>>> Signed-off-by: Vit Mojzis
> >>>> ---
> >>>> libvirt.spec.in | 12
> >>>> meson.build | 12
> >>>> selinux/compile_policy.sh | 39 +++
> >>>> selinux/meson.build | 23 +++
> >>>> 4 files changed, 74 insertions(+), 12 deletions(-)
> >>>> create mode 100755 selinux/compile_policy.sh
> >>>> create mode 100644 selinux/meson.build
> >>>>
> >>>> diff --git a/libvirt.spec.in b/libvirt.spec.in
> >>>> index db08d91043..de664084fa 100644
> >>>> --- a/libvirt.spec.in
> >>>> +++ b/libvirt.spec.in
> >>>> @@ -1240,14 +1240,6 @@ export SOURCE_DATE_EPOCH=$(stat --printf='%Y'
> >>>> %{_specdir}/%{name}.spec)
> >>>> %{?arg_login_shell}
> >>>>
> >>>> %meson_build
> >>>> -%if 0%{?with_selinux}
> >>>> -# SELinux policy (originally from selinux-policy-contrib)
> >>>> -# this policy module will override the production module
> >>>> -cd selinux
> >>>> -
> >>>> -make -f %{_datadir}/selinux/devel/Makefile %{modulename}.pp
> >>>> -bzip2 -9 %{modulename}.pp
> >>>> -%endif
> >>>>
> >>>> %install
> >>>> rm -fr %{buildroot}
> >>>> @@ -1332,10 +1324,6 @@ mv
> >>>> $RPM_BUILD_ROOT%{_datadir}/systemtap/tapset/libvirt_qemu_probes.stp \
> >>>> %endif
> >>>> %endif
> >>>>
> >>>> -%if 0%{?with_selinux}
> >>>> -install -D -m 0644 selinux/%{modulename}.pp.bz2
> >>>> %{buildroot}%{_datadir}/selinux/packages/%{selinuxtype}/%{modulename}.pp.bz2
> >>>> -%endif
> >>>> -
> >>>> %check
> >>>> # Building on slow archs, like emulated s390x in Fedora copr, requires
> >>>> # raising the test timeout
> >>>> diff --git a/meson.build b/meson.build
> >>>> index c81c6ab205..d060e441b5 100644
> >>>> --- a/meson.build
> >>>> +++ b/meson.build
> >>>> @@ -2183,6 +2183,18 @@ endif
> >>>>
> >>>> subdir('build-aux')
> >>>>
> >>>> +os_release = run_command('grep', '^ID=', '/etc/os-release').stdout()
> >>>> +os_version = run_command('grep', '^VERSION_ID=',
> >>>> '/etc/os-release').stdout().split('=')
> >>>> +if (os_version.length() == 2)
> >>>> + os_version = os_version[1]
> >>>> +else
> >>>> + os_version = 0
> >>>> +endif
> >>>> +
> >>>> +if ((os_release.contains('fedora') and
> >>>> os_version.version_compare('>32')) or
> >>>> +(os_release.contains('rhel') and os_version.version_compare('>7')))
> >>>> + subdir('selinux')
> >>>> +endif
> >>>>
> >>>> # install pkgconfig files
> >>>> pkgconfig_files = [
> >>>> diff --git a/selinux/compile_policy.sh b/selinux/compile_policy.sh
> >>>> new file mode 100755
> >>>> index 00..02780e4aed
> >>>> --- /dev/null
> >>>> +++ b/selinux/compile_policy.sh
> >>>> @@ -0,0 +1,39 @@
> >>>> +#!/bin/sh
> >>>> +set -x
> >>>> +
> >>>> +if [[ $# -ne 5 ]] ; then
> >>>> +echo "Usage: compile_policy.sh .te .if .fc
> >>>> .pp "
> >>>> +exit 1
> >>>> +fi
> >>>> +
> >>>> +# checkmodule requires consistent file names
> >>>> +MODULE_NAME=$(basename -- "$1")
> >>>> +MODULE_NAME=${MODULE_NAME%.*}
> >>>> +
> >>>> +M4PARAM="-D enable_mcs -D distro_redhat -D hide_broken_symptoms -D
> >>>> mls_num_sens=16 -D mls_n
Re: [libvirt PATCH 00/18] qemu: add support for audio backend configuration
On Wed, Mar 3, 2021 at 1:19 PM Daniel P. Berrangé wrote: > > Historically we've done almost nothing with audio backend > configuration. In QEMU we merely set QEMU_AUDIO_DRV to one > of sdl, spice, none depending on . We also have > the somewhat crazy ability to let QEMU inherit the > QEMU_AUDIO_DRV env variable from libvirtd. > > Fairly recently BHyve wanted audio backend config for OSS > so introduced the element. We designed that to allow > QEMU to later extend it, and that's what this series does. > We add types for all the QEMU backends, except the > Windows only DSound which isn't relevant for libvirt. > > The QEMU driver is updated to use this element to configure > things. QEMU has many many many more env variables for > configuring audio settings, which we can now support. These > are all deprecated since 4.0.0 though, so we also add support > for the new -audiodev argument. > > Unfortunately -audiodev isn't introspectable due to limits > in QEMU fixed by: > >https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg00653.html > > The lack of introspection isn't critical though. We can > detect existance of -audiodev by querying for '-vnc audiodev=3DNNN' > argument support. We simply lack ability to determine what QEMU > audio backends are compiled in. This means we have to delegate > error reporting to QEMU itself, which is OK. > > We'll make use of the query-audiodev command at a later date > to track future improvements to QEMU audiodev backends. > > Daniel P. Berrang=C3=A9 (18): > config: cleanup some typos / baggage wrt compiler checks > conf: stronger error reporting when parsing audio related params > conf: don't force existance of audio child elements > conf: add helper to test for sound device codec support > conf: add missing iteration over audio backends > conf: refactor OSS audio backend specific options > conf: add coverage for all QEMU audio backend types > conf: add support for audio backend for the VNC server > conf: add validation of audio backend IDs > conf: rename and improve virDomainDefFindAudioForSound > qemu: support use of elements > qemu: populate element with default config > qemu: probe for -vnc audiodev property > qemu: add support for generating -audiodev arguments > conf: introduce support for common audio settings > qemu: wire up support for common audio backend settings > conf: add support for audio backend specific settings > qemu: wire up support for backend specific audio settings > > config.h | 10 +- > docs/formatdomain.rst | 322 +++- > docs/schemas/domaincommon.rng | 384 +- > src/bhyve/bhyve_command.c | 30 +- > src/conf/domain_conf.c| 693 +- > src/conf/domain_conf.h| 125 +++- > src/conf/domain_validate.c| 67 +- > src/libvirt_private.syms | 8 +- > src/qemu/qemu_capabilities.c | 4 + > src/qemu/qemu_capabilities.h | 3 + > src/qemu/qemu_command.c | 484 +++- > src/qemu/qemu_domain.c| 110 ++- > src/qemu/qemu_validate.c | 136 +++- > .../caps_4.2.0.aarch64.xml| 1 + > .../qemucapabilitiesdata/caps_4.2.0.ppc64.xml | 1 + > .../qemucapabilitiesdata/caps_4.2.0.s390x.xml | 1 + > .../caps_4.2.0.x86_64.xml | 1 + > .../caps_5.0.0.aarch64.xml| 1 + > .../qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 1 + > .../caps_5.0.0.riscv64.xml| 1 + > .../caps_5.0.0.x86_64.xml | 1 + > .../qemucapabilitiesdata/caps_5.1.0.sparc.xml | 1 + > .../caps_5.1.0.x86_64.xml | 1 + > .../caps_5.2.0.aarch64.xml| 1 + > .../qemucapabilitiesdata/caps_5.2.0.ppc64.xml | 1 + > .../caps_5.2.0.riscv64.xml| 1 + > .../qemucapabilitiesdata/caps_5.2.0.s390x.xml | 1 + > .../caps_5.2.0.x86_64.xml | 1 + > .../caps_6.0.0.x86_64.xml | 1 + > .../redefine.xml | 1 + > .../disk_snapshot_redefine.xml| 1 + > .../external_vm_redefine.xml | 1 + > .../full_domain.xml | 1 + > .../qemudomainsnapshotxml2xmlout/metadata.xml | 1 + > .../ppc64-modern-bulk-result-conf.xml | 1 + > .../ppc64-modern-bulk-result-live.xml | 1 + > .../ppc64-modern-individual-result-conf.xml | 1 + > .../ppc64-modern-individual-result-live.xml | 1 + > .../x86-modern-bulk-result-conf.xml | 1 + > .../x86-modern-bulk-result-live.xml | 1 + > .../x86-modern-individual-add-result-conf.xml | 1 + > .../x86-modern-individual-add-result-live.xml | 1 + > .../x86-old-bulk-result-conf.xml
Re: [PATCH 3/3] selinux: Remove 'make' dependency
On Thu, Mar 11, 2021 at 6:35 AM Daniel P. Berrangé wrote:
>
> On Wed, Mar 10, 2021 at 01:50:43PM -0500, Neal Gompa wrote:
> > On Wed, Mar 10, 2021 at 7:43 AM Nikola Knazekova
> > wrote:
> > >
> > > From: Vit Mojzis
> > >
> > > Compile the policy using a shell script executed by meson.
> > >
> > > Signed-off-by: Vit Mojzis
> > > ---
> > > libvirt.spec.in | 12
> > > meson.build | 12
> > > selinux/compile_policy.sh | 39 +++
> > > selinux/meson.build | 23 +++
> > > 4 files changed, 74 insertions(+), 12 deletions(-)
> > > create mode 100755 selinux/compile_policy.sh
> > > create mode 100644 selinux/meson.build
> > >
> > > diff --git a/libvirt.spec.in b/libvirt.spec.in
> > > index db08d91043..de664084fa 100644
> > > --- a/libvirt.spec.in
> > > +++ b/libvirt.spec.in
> > > @@ -1240,14 +1240,6 @@ export SOURCE_DATE_EPOCH=$(stat --printf='%Y'
> > > %{_specdir}/%{name}.spec)
> > > %{?arg_login_shell}
> > >
> > > %meson_build
> > > -%if 0%{?with_selinux}
> > > -# SELinux policy (originally from selinux-policy-contrib)
> > > -# this policy module will override the production module
> > > -cd selinux
> > > -
> > > -make -f %{_datadir}/selinux/devel/Makefile %{modulename}.pp
> > > -bzip2 -9 %{modulename}.pp
> > > -%endif
> > >
> > > %install
> > > rm -fr %{buildroot}
> > > @@ -1332,10 +1324,6 @@ mv
> > > $RPM_BUILD_ROOT%{_datadir}/systemtap/tapset/libvirt_qemu_probes.stp \
> > > %endif
> > > %endif
> > >
> > > -%if 0%{?with_selinux}
> > > -install -D -m 0644 selinux/%{modulename}.pp.bz2
> > > %{buildroot}%{_datadir}/selinux/packages/%{selinuxtype}/%{modulename}.pp.bz2
> > > -%endif
> > > -
> > > %check
> > > # Building on slow archs, like emulated s390x in Fedora copr, requires
> > > # raising the test timeout
> > > diff --git a/meson.build b/meson.build
> > > index c81c6ab205..d060e441b5 100644
> > > --- a/meson.build
> > > +++ b/meson.build
> > > @@ -2183,6 +2183,18 @@ endif
> > >
> > > subdir('build-aux')
> > >
> > > +os_release = run_command('grep', '^ID=', '/etc/os-release').stdout()
> > > +os_version = run_command('grep', '^VERSION_ID=',
> > > '/etc/os-release').stdout().split('=')
> > > +if (os_version.length() == 2)
> > > + os_version = os_version[1]
> > > +else
> > > + os_version = 0
> > > +endif
> > > +
> > > +if ((os_release.contains('fedora') and
> > > os_version.version_compare('>32')) or
> > > +(os_release.contains('rhel') and os_version.version_compare('>7')))
> > > + subdir('selinux')
> > > +endif
> > >
> > > # install pkgconfig files
> > > pkgconfig_files = [
> > > diff --git a/selinux/compile_policy.sh b/selinux/compile_policy.sh
> > > new file mode 100755
> > > index 00..02780e4aed
> > > --- /dev/null
> > > +++ b/selinux/compile_policy.sh
> > > @@ -0,0 +1,39 @@
> > > +#!/bin/sh
> > > +set -x
> > > +
> > > +if [[ $# -ne 5 ]] ; then
> > > +echo "Usage: compile_policy.sh .te .if .fc
> > > .pp "
> > > +exit 1
> > > +fi
> > > +
> > > +# checkmodule requires consistent file names
> > > +MODULE_NAME=$(basename -- "$1")
> > > +MODULE_NAME=${MODULE_NAME%.*}
> > > +
> > > +M4PARAM="-D enable_mcs -D distro_redhat -D hide_broken_symptoms -D
> > > mls_num_sens=16 -D mls_num_cats=1024 -D mcs_num_cats=1024"
> > > +SHAREDIR="/usr/share/selinux"
> > > +HEADERDIR="$SHAREDIR/devel/include"
> > > +M4SUPPORT=$(echo $HEADERDIR/support/*.spt)
> > > +HEADER_LAYERS=$(find "/usr/share/selinux/devel/include"/* -maxdepth 0
> > > -type d | grep -v "/usr/share/selinux/devel/include/support")
> > > +HEADER_INTERFACES=""
> > > +for LAYER in $HEADER_LAYERS
> > > +do
> > > +HEADER_INTERFACES="$HEADER_INTERFACES $(echo $LAYER/*.if)"
> > > +done
> > > +
> > > +# prepare temp folder
> > > +mkdir -p $5
> > > +# remove old trash from the temp folder
> > > +rm -rf "$5/i
Re: [PATCH 3/3] selinux: Remove 'make' dependency
On Wed, Mar 10, 2021 at 7:43 AM Nikola Knazekova wrote:
>
> From: Vit Mojzis
>
> Compile the policy using a shell script executed by meson.
>
> Signed-off-by: Vit Mojzis
> ---
> libvirt.spec.in | 12
> meson.build | 12
> selinux/compile_policy.sh | 39 +++
> selinux/meson.build | 23 +++
> 4 files changed, 74 insertions(+), 12 deletions(-)
> create mode 100755 selinux/compile_policy.sh
> create mode 100644 selinux/meson.build
>
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index db08d91043..de664084fa 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -1240,14 +1240,6 @@ export SOURCE_DATE_EPOCH=$(stat --printf='%Y'
> %{_specdir}/%{name}.spec)
> %{?arg_login_shell}
>
> %meson_build
> -%if 0%{?with_selinux}
> -# SELinux policy (originally from selinux-policy-contrib)
> -# this policy module will override the production module
> -cd selinux
> -
> -make -f %{_datadir}/selinux/devel/Makefile %{modulename}.pp
> -bzip2 -9 %{modulename}.pp
> -%endif
>
> %install
> rm -fr %{buildroot}
> @@ -1332,10 +1324,6 @@ mv
> $RPM_BUILD_ROOT%{_datadir}/systemtap/tapset/libvirt_qemu_probes.stp \
> %endif
> %endif
>
> -%if 0%{?with_selinux}
> -install -D -m 0644 selinux/%{modulename}.pp.bz2
> %{buildroot}%{_datadir}/selinux/packages/%{selinuxtype}/%{modulename}.pp.bz2
> -%endif
> -
> %check
> # Building on slow archs, like emulated s390x in Fedora copr, requires
> # raising the test timeout
> diff --git a/meson.build b/meson.build
> index c81c6ab205..d060e441b5 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -2183,6 +2183,18 @@ endif
>
> subdir('build-aux')
>
> +os_release = run_command('grep', '^ID=', '/etc/os-release').stdout()
> +os_version = run_command('grep', '^VERSION_ID=',
> '/etc/os-release').stdout().split('=')
> +if (os_version.length() == 2)
> + os_version = os_version[1]
> +else
> + os_version = 0
> +endif
> +
> +if ((os_release.contains('fedora') and os_version.version_compare('>32')) or
> +(os_release.contains('rhel') and os_version.version_compare('>7')))
> + subdir('selinux')
> +endif
>
> # install pkgconfig files
> pkgconfig_files = [
> diff --git a/selinux/compile_policy.sh b/selinux/compile_policy.sh
> new file mode 100755
> index 00..02780e4aed
> --- /dev/null
> +++ b/selinux/compile_policy.sh
> @@ -0,0 +1,39 @@
> +#!/bin/sh
> +set -x
> +
> +if [[ $# -ne 5 ]] ; then
> +echo "Usage: compile_policy.sh .te .if .fc
> .pp "
> +exit 1
> +fi
> +
> +# checkmodule requires consistent file names
> +MODULE_NAME=$(basename -- "$1")
> +MODULE_NAME=${MODULE_NAME%.*}
> +
> +M4PARAM="-D enable_mcs -D distro_redhat -D hide_broken_symptoms -D
> mls_num_sens=16 -D mls_num_cats=1024 -D mcs_num_cats=1024"
> +SHAREDIR="/usr/share/selinux"
> +HEADERDIR="$SHAREDIR/devel/include"
> +M4SUPPORT=$(echo $HEADERDIR/support/*.spt)
> +HEADER_LAYERS=$(find "/usr/share/selinux/devel/include"/* -maxdepth 0 -type
> d | grep -v "/usr/share/selinux/devel/include/support")
> +HEADER_INTERFACES=""
> +for LAYER in $HEADER_LAYERS
> +do
> +HEADER_INTERFACES="$HEADER_INTERFACES $(echo $LAYER/*.if)"
> +done
> +
> +# prepare temp folder
> +mkdir -p $5
> +# remove old trash from the temp folder
> +rm -rf "$5/iferror.m4 $5/all_interfaces.conf $5/$MODULE_NAME.*"
> +# tmp/all_interfaces.conf
> +echo "ifdef(\`__if_error',\`m4exit(1)')" > $5/iferror.m4
> +echo "divert(-1)" > $5/all_interfaces.conf
> +m4 $M4SUPPORT $HEADER_INTERFACES $2 $5/iferror.m4 | sed -e
> s/dollarsstar/\$\$\*/g >> $5/all_interfaces.conf
> +echo "divert" >> $5/all_interfaces.conf
> +# tmp/%.mod
> +m4 $M4PARAM -s $M4SUPPORT $5/all_interfaces.conf $1 > $5/$MODULE_NAME.tmp
> +/usr/bin/checkmodule -M -m $5/$MODULE_NAME.tmp -o $5/$MODULE_NAME.mod
> +# tmp/%.mod.fc
> +m4 $M4PARAM $M4SUPPORT $3 > $5/$MODULE_NAME.mod.fc
> +# %.pp
> +/usr/bin/semodule_package -o $4 -m $5/$MODULE_NAME.mod -f
> $5/$MODULE_NAME.mod.fc
> diff --git a/selinux/meson.build b/selinux/meson.build
> new file mode 100644
> index 00..1c76fd40aa
> --- /dev/null
> +++ b/selinux/meson.build
> @@ -0,0 +1,23 @@
> +selinux_sources = [
> + 'virt.te',
> + 'virt.if',
> + 'virt.fc',
> +]
> +
> +compile_policy_prog = find_program('compile_policy.sh')
> +
> +virt_pp = custom_target('virt.pp',
> + output : 'virt.pp',
> + input : selinux_sources,
> + command : [compile_policy_prog, '@INPUT@', '@OUTPUT@', 'selinux/tmp'],
> + install : false)
> +
> +bzip2_prog = find_program('bzip2')
> +
> +bzip = custom_target('virt.pp.bz2',
> + output : 'virt.pp.bz2',
> + input : virt_pp,
> + command : [bzip2_prog, '-c', '-9', '@INPUT@'],
> + capture : true,
> + install : true,
> + install_dir : 'share/selinux/packages/targeted')
> --
> 2.29.2
>
This smells like a bad idea, because we're not relying on the
framework that SELinux policies are supposed to be built with. I don't
think we should do this.
--
真実はいつも一つ！/ Always, there's only
Re: [libvirt PATCH v2 0/2] stop using netcf for the interface driver backend
On Sun, Jan 24, 2021 at 1:45 AM Laine Stump wrote:
>
> V1 here:
> https://www.redhat.com/archives/libvir-list/2021-January/msg00922.html
>
> A short version of the cover letter from V1: this is a followup to my
> proposal to stop using netcf for the interface driver backend in
> Fedora/RHEL/CentOS builds and use the udev backend instead (it turns
> out Ubuntu already disabled netcf in 2018).
>
> Changes in V2:
>
> * removed the patch that made the default netcf=disabled even when
> netcf-devel was found on the host. If someone has netcf-devel
> installed and still wants to build without netcf, then can add
> "-Dnetcf=disabled" on the meson commandline.
>
> * Made the specfile changes more intelligent:
>
> * instead of hardcoding -Dnetcf=disabled, we now have a variable
> %{with_netcf} that is set to 1 for current Fedora (< 34) and current
> RHEL (< 9) but will be set to 0 for future Fedora/RHEL. This way the
> behavior on current OS releases will remain the same even for future
> libvirt.
>
> * it is possible to for netcf support off even in current/older OS
> releases by adding "--without netcf" to the rpmbuild commandline.
>
> I think at this point I would be comfortable pushing these patches, unless
> someone has misgivings about it...
>
> Laine Stump (2):
> build: support explicitly disabling netcf
> rpm: disable netcf for the interface driver in rpm build on new
> targets
>
> libvirt.spec.in | 22 +-
> meson.build | 10 ++
> 2 files changed, 23 insertions(+), 9 deletions(-)
>
> --
> 2.29.2
>
This looks fine to me, but I'm wondering why libvirt doesn't
communicate with NetworkManager for this information? That's a cross
distribution method of handling complex network configuration that we
basically know will exist and can handle parsing and configuring
networks effectively.
Otherwise...
Reviewed-by: Neal Gompa
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH 00/12] Hyper-V serial ports, NICs, and screeshots
On Fri, Jan 22, 2021 at 3:19 PM Matt Coleman wrote: > > This patchset makes the following changes to the Hyper-V driver: > * enable XML parsing and creation of serial ports and NICs > * implement several networking APIs > * implement screenshots > > Matt Coleman (12): > hyperv: XML parsing of serial ports > hyperv: add support for creating serial devices > hyperv: XML parsing of Ethernet adapters > hyperv: add support for creating network adapters > hyperv: implement connectListAllNetworks and connectNumOfNetworks > hyperv: implement networkLookupByName and networkLookupByUUID > hyperv: implement connectNumOfDefinedNetworks and > connectListDefinedNetworks > hyperv: implement networkGetAutostart, networkIsActive, and > networkIsPersistent > hyperv: implement networkGetXMLDesc > hyperv: implement domainScreenshot > hyperv: provide a more detailed error message for WSMan faults > news: implement new Hyper-V APIs > > NEWS.rst | 12 + > po/POTFILES.in| 1 + > src/hyperv/hyperv_driver.c| 620 ++ > src/hyperv/hyperv_network_driver.c| 242 ++ > src/hyperv/hyperv_network_driver.h| 26 ++ > src/hyperv/hyperv_wmi.c | 45 +- > src/hyperv/hyperv_wmi.h | 12 + > src/hyperv/hyperv_wmi_classes.h | 36 ++ > src/hyperv/hyperv_wmi_generator.input | 309 + > src/hyperv/meson.build| 1 + > 10 files changed, 1301 insertions(+), 3 deletions(-) > create mode 100644 src/hyperv/hyperv_network_driver.c > create mode 100644 src/hyperv/hyperv_network_driver.h > > -- > 2.30.0 > > The patch set builds and works for me locally, and I don't spot any obvious issues in the code... Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH] spec: Increase meson test timeout 10x
On Thu, Jan 21, 2021 at 4:59 PM Cole Robinson wrote:
>
> Tests time out when building in slow environments, like emulated
> s390x in Fedora copr. Bump up the test timeout
>
> Signed-off-by: Cole Robinson
> ---
> libvirt.spec.in | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index 0a8b0ebad4..1731aa8bd9 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -1288,7 +1288,9 @@ mv
> $RPM_BUILD_ROOT%{_datadir}/systemtap/tapset/libvirt_qemu_probes.stp \
> %endif
>
> %check
> -VIR_TEST_DEBUG=1 %meson_test --no-suite syntax-check
> +# Building on slow archs, like emulated s390x in Fedora copr, requires
> +# raising the test timeout
> +VIR_TEST_DEBUG=1 %meson_test --no-suite syntax-check --timeout-multiplier 10
>
> %post libs
> %if 0%{?rhel} == 7
> --
> 2.29.2
>
I have to do this for slow hardware too, so I'm fine with it.
Reviewed-by: Neal Gompa
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH 00/55] Hyper-V: code cleanup & prep for future changes
eanup in hypervEnumAndPull > hyperv: use GLib auto-cleanup in hypervSerializeEmbeddedParam > hyperv: use GLib auto-cleanup in hypervCreateInvokeXmlDoc > hyperv: use g_auto for WsXmlDocH in hypervDomainAttachVirtualDisk > hyperv: use g_auto for WsXmlDocH in hypervDomainAttachCDROM > > scripts/hyperv_wmi_generator.py | 16 +- > src/hyperv/hyperv_driver.c | 755 +++- > src/hyperv/hyperv_private.h | 4 +- > src/hyperv/hyperv_wmi.c | 408 +++-- > src/hyperv/hyperv_wmi.h | 4 +- > src/hyperv/hyperv_wsman.h | 28 ++ > 6 files changed, 457 insertions(+), 758 deletions(-) > create mode 100644 src/hyperv/hyperv_wsman.h > > -- > 2.30.0 Quite the doozy, but looks mostly repetitious and simple... Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH v2] rpm: adjust xenlight requirements
On Fri, Jan 8, 2021 at 5:26 AM Olaf Hering wrote:
>
> According to meson.build, and the actual code, Xen 4.6+ is required.
> Drop the version, they is mentioned in meson.build.
>
> Signed-off-by: Olaf Hering
> ---
> libvirt.spec.in | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index 0a8b0ebad4..470631f7bd 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -126,7 +126,7 @@
> %endif
>
> # RHEL doesn't ship OpenVZ, VBox, PowerHypervisor,
> -# VMware, libxenlight (Xen 4.1 and newer),
> +# VMware, libxenlight
> # or HyperV.
> %if 0%{?rhel}
> %define with_openvz 0
> @@ -272,7 +272,7 @@ BuildRequires: perl
> BuildRequires: python3
> BuildRequires: systemd-units
> %if %{with_libxl}
> -BuildRequires: xen-devel
> +BuildRequires: pkgconfig(xenlight)
> %endif
> BuildRequires: glib2-devel >= 2.48
> BuildRequires: libxml2-devel
>
Why are we changing to `pkgconfig(xenlight)` from xen-devel? It looks
like xen-devel provides that name...
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH 2/2] rpm: Drop unnecessary libiscsi runtime dependency
On Thu, Jan 7, 2021 at 12:38 PM Jiri Denemark wrote:
>
> On Thu, Jan 07, 2021 at 09:58:09 -0500, Neal Gompa wrote:
> > This is automatically picked up by the dependency generator, so
> > there's no reason to have this here.
> >
> > Signed-off-by: Neal Gompa
> > ---
> > libvirt.spec.in | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/libvirt.spec.in b/libvirt.spec.in
> > index 0a8b0ebad4..41e532102a 100644
> > --- a/libvirt.spec.in
> > +++ b/libvirt.spec.in
> > @@ -614,7 +614,6 @@ volumes using the host iscsi stack.
> > Summary: Storage driver plugin for iscsi-direct
> > Requires: libvirt-daemon-driver-storage-core = %{version}-%{release}
> > Requires: libvirt-libs = %{version}-%{release}
> > -Requires: libiscsi
>
> The explicit dependency was added by Andrea 2.5 years ago, perhaps he
> had reasons to do so. Any comments Andrea?
It most likely dates back to when Fedora had two providers of libiscsi
sharing the same soname. That situation no longer exists today. Other
distributions also don't have that issue.
--
真実はいつも一つ！/ Always, there's only one truth!
[PATCH 1/2] rpm: Simplify expression of supported platforms
Stanzas like "0%{?fedora} && 0%{?fedora} >= %{min_fedora}" contain
redundant definitions, as "0%{?fedora} >= %{min_fedora}" implies that
"%fedora" is defined and has a value. Thus, we can simplify this.
Signed-off-by: Neal Gompa
---
libvirt.spec.in | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index ce1a8d7078..0a8b0ebad4 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -6,7 +6,7 @@
%define min_rhel 7
%define min_fedora 31
-%if (0%{?fedora} && 0%{?fedora} >= %{min_fedora}) || (0%{?rhel} && 0%{?rhel}
>= %{min_rhel})
+%if 0%{?fedora} >= %{min_fedora} || 0%{?rhel} >= %{min_rhel}
%define supported_platform 1
%else
%define supported_platform 0
--
2.29.2
[PATCH 2/2] rpm: Drop unnecessary libiscsi runtime dependency
This is automatically picked up by the dependency generator, so
there's no reason to have this here.
Signed-off-by: Neal Gompa
---
libvirt.spec.in | 1 -
1 file changed, 1 deletion(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 0a8b0ebad4..41e532102a 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -614,7 +614,6 @@ volumes using the host iscsi stack.
Summary: Storage driver plugin for iscsi-direct
Requires: libvirt-daemon-driver-storage-core = %{version}-%{release}
Requires: libvirt-libs = %{version}-%{release}
-Requires: libiscsi
%description daemon-driver-storage-iscsi-direct
The storage driver backend adding implementation of the storage APIs for iscsi
--
2.29.2
[PATCH 0/2] Minor fixes to the RPM spec
As part of the work to add openSUSE support to the spec file, I discovered a couple of small fixes worth submitting separately. Neal Gompa (2): rpm: Simplify expression of supported platforms rpm: Drop unnecessary libiscsi runtime dependency libvirt.spec.in | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) -- 2.29.2
Re: [PATCH] polkit: Allow libvirt group access to libvirtd ro socket
On Tue, Dec 1, 2020 at 4:23 PM Jim Fehlig wrote: > > On 12/1/20 2:17 AM, Daniel P. Berrangé wrote: > > On Mon, Nov 30, 2020 at 05:28:16PM -0700, Jim Fehlig wrote: > >> As a normal user, 'virsh connect qemu:///system' and > >> 'virsh connect --readonly qemu:///system' will prompt for root password. > >> If the user is added to the libvirt group, only > >> 'virsh connect --readonly qemu:///system' will prompt for root password. > > > > This doesn't make sense - the readonly case should never prompt for > > a password, since libvirtd.polkit.in grants that permission out of > > the box. > > I thought something smelled a bit fishy. I meant to annotate the patch with > "It > is possible I have a broader polkit config issue", but forgot before sending > it > last night. > > And indeed after looking again today with fresh eyes I see the problem is in > our > polkit-default-privs package -> downstream bug. Ignore this patch. > Hah, and I didn't catch this because I rip out the default openSUSE stuff that ruins usability by restricting polkit too much. :) Shame on me for not double checking my environment. :) -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH] polkit: Allow libvirt group access to libvirtd ro socket
On Mon, Nov 30, 2020 at 7:29 PM Jim Fehlig wrote:
>
> As a normal user, 'virsh connect qemu:///system' and
> 'virsh connect --readonly qemu:///system' will prompt for root password.
> If the user is added to the libvirt group, only
> 'virsh connect --readonly qemu:///system' will prompt for root password.
>
> The libvirt polkit rules already allow libvirt group members access to
> the rw socket. Add a rule allowing to access the ro socket.
>
> Signed-off-by: Jim Fehlig
> ---
> src/remote/libvirtd.rules | 11 +--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/src/remote/libvirtd.rules b/src/remote/libvirtd.rules
> index 01a15fac2e..d9be94fcc4 100644
> --- a/src/remote/libvirtd.rules
> +++ b/src/remote/libvirtd.rules
> @@ -1,5 +1,12 @@
> -// Allow any user in the 'libvirt' group to connect to system libvirtd
> -// without entering a password.
> +// Allow any user in the 'libvirt' group to connect to the system libvirtd
> +// ro and rw sockets without entering a password.
> +
> +polkit.addRule(function(action, subject) {
> +if (action.id == "org.libvirt.unix.monitor" &&
> +subject.isInGroup("libvirt")) {
> +return polkit.Result.YES;
> +}
> +});
>
> polkit.addRule(function(action, subject) {
> if (action.id == "org.libvirt.unix.manage" &&
> --
> 2.29.2
>
>
LGTM.
Reviewed-by: Neal Gompa
--
真実はいつも一つ！/ Always, there's only one truth!
Re: regression in meson build, AC_PATH_PROG lost
On Mon, Nov 23, 2020 at 11:24 AM Andrea Bolognani wrote: > > On Fri, 2020-11-20 at 08:51 -0500, Neal Gompa wrote: > > On Fri, Nov 20, 2020 at 6:25 AM Andrea Bolognani > > wrote: > > > On Thu, 2020-11-19 at 16:56 -0500, Neal Gompa wrote: > > > > The way I solved *that* specific problem was to use zfs-fuse at > > > > build-time and have runtime work with either implementation. That > > > > should work for you too, since Debian has zfs-fuse in main. > > > > > > This sounds intriguing. Can you please point me to the code? > > > > I build libvirt for Debian/Ubuntu using a modified version of the > > upstream spec file (if y'all are interested, I can upstream those > > modifications so we could build and test that way in CI too), but > > here's what I did: > > > > * > > https://pagure.io/libvirt-deb/blob/90d9373670564341503f768880e996f01da596a6/f/libvirt.spec#_42-47 > > * > > https://pagure.io/libvirt-deb/blob/90d9373670564341503f768880e996f01da596a6/f/libvirt.spec#_365-369 > > * > > https://pagure.io/libvirt-deb/blob/90d9373670564341503f768880e996f01da596a6/f/libvirt.spec#_967-974 > > > > For work, we *only* want to use ZoL at runtime, but this can be done > > where zfs-fuse could be a valid candidate too. > > > > It's not terribly difficult to translate this into debian/control goop. :) > > I just looked at your spec file and that's certainly unique! Out of > curiosity, are you shipping the resulting Debian packages anywhere? > I had been previously building internally at work, but I've set up a public build here now: https://build.opensuse.org/package/show/home:Pharaoh_Atem:libvirt-dev/libvirt We had to backport Meson from Fedora as well to build it, but that wasn't a big deal. :) -- 真実はいつも一つ！/ Always, there's only one truth!
Re: regression in meson build, AC_PATH_PROG lost
On Fri, Nov 20, 2020 at 6:25 AM Andrea Bolognani wrote: > > On Thu, 2020-11-19 at 16:56 -0500, Neal Gompa wrote: > > On Fri, Nov 13, 2020 at 8:14 AM Andrea Bolognani > > wrote: > > > As another data point, Debian currently carries a patch[1] which > > > allows us to enable the ZFS driver without installing the ZFS > > > packages in the build environment: this is necessary because, due to > > > its license, ZFS is kept outside of the main Debian repository. > > > > > > Being able to use something like > > > > > > -Dprog_zfs=/sbin/zfs -Dprog_zfspool=/sbin/zfspool > > > > > > to achieve the same result would allow us to drop that patch, which I > > > would be *extremely* happy about :) > > > > The way I solved *that* specific problem was to use zfs-fuse at > > build-time and have runtime work with either implementation. That > > should work for you too, since Debian has zfs-fuse in main. > > This sounds intriguing. Can you please point me to the code? > I build libvirt for Debian/Ubuntu using a modified version of the upstream spec file (if y'all are interested, I can upstream those modifications so we could build and test that way in CI too), but here's what I did: * https://pagure.io/libvirt-deb/blob/90d9373670564341503f768880e996f01da596a6/f/libvirt.spec#_42-47 * https://pagure.io/libvirt-deb/blob/90d9373670564341503f768880e996f01da596a6/f/libvirt.spec#_365-369 * https://pagure.io/libvirt-deb/blob/90d9373670564341503f768880e996f01da596a6/f/libvirt.spec#_967-974 For work, we *only* want to use ZoL at runtime, but this can be done where zfs-fuse could be a valid candidate too. It's not terribly difficult to translate this into debian/control goop. :) -- 真実はいつも一つ！/ Always, there's only one truth!
Re: regression in meson build, AC_PATH_PROG lost
On Fri, Nov 13, 2020 at 8:14 AM Andrea Bolognani wrote: > > On Fri, 2020-11-13 at 12:37 +0100, Pavel Hrdina wrote: > > On Thu, Nov 12, 2020 at 10:40:02PM +0100, Olaf Hering wrote: > > > Since meson does not support environment variables it seems the > > > only way to address this is to introduce an option in > > > meson_options.txt for each runtime executable. > > > > > > Will such change be accepted? > > > > This would be one way how to address runtime executables, currently we > > have a lot of them. These executables are checked in order to > > enable/disable some libvirt feature but is used only in runtime: > > > > parted, bhyve, bhyvectl, bhyveload, mount, umount, mkfs, pvcreate, > > vgcreate, lvcreate, pvremove, vgremove, lvremove, lvchange, vgchange, > > vgscan, pvs, vgs, lvs, (collie|dog), vstorage, vstorage-mount, zfs, > > zpool, numad > > > > This one is even more broken as if we don't find it during build time it > > is set to empty string and never checked in our code: > > > > showmount > > > > These are checked during build time but if not present they are set to > > known absolute path: > > > > qemu-bridge-helper, qemu-pr-helper, slirp-helper, dbus-daemon > > > > And we have a list of optional_programs that are checked during build > > time and if not present they are set to the name of the executable and > > resolved during runtime from $PATH. > > > > The last executable, pkcheck, is not used during build and in runtime. > > We only check its presence to enable/disable polkit support. We should > > be able to simply drop this check and figure out the presence of polkit > > using DBus only as we do for machined or logind and other DBus services > > that we use. > > > > All of this was copied from autoconf except for the fact that it is no > > longer possible to use ENV variables. > > > > I think we need to unify the process how we handle runtime executable > > dependencies and possibly add meson options for them. > > As another data point, Debian currently carries a patch[1] which > allows us to enable the ZFS driver without installing the ZFS > packages in the build environment: this is necessary because, due to > its license, ZFS is kept outside of the main Debian repository. > > Being able to use something like > > -Dprog_zfs=/sbin/zfs -Dprog_zfspool=/sbin/zfspool > > to achieve the same result would allow us to drop that patch, which I > would be *extremely* happy about :) > > > [1] > https://salsa.debian.org/libvirt-team/libvirt/-/blob/debian/master/debian/patches/debian/Set-defaults-for-zfs-tools.patch The way I solved *that* specific problem was to use zfs-fuse at build-time and have runtime work with either implementation. That should work for you too, since Debian has zfs-fuse in main. -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt] improve security by adjusting the privileges of libvirtd processes
On Mon, Nov 16, 2020 at 7:12 AM yebiaoxiang wrote: > > Hi Team > > The daemon libvirtd runs as root user, which against the least privilege > security model. > > root 567642 1.2 0.0 2856020 47576 ? Ssl 15:49 0:02 /usr/sbin/libvirtd --listen > > In addition, the "--listen" parameter exposes TCP or TLS ports on the network, > it increasing the attack surface. > > tcp 0 0 0.0.0.0:16509 0.0.0.0:* LISTEN 647824/libvirtd > tcp 0 0 0.0.0.0:16514 0.0.0.0:* LISTEN 647824/libvirtd > > I have the following puzzles: > 1. Whether root is the least privilege required for libvirtd to manage > virtualization platforms, it's possible to run libvirtd as a non-root > user? > > 2. Is there any plan to resolve this security weaknesses? > (like move the function of "--listen" to an independent non-root process, > or other better schemes) While generally this is a good idea (and libvirt has been splitting out functionality into separate daemons for improving security around the service in general), I'm wondering if you looked at what libvirt is supposed to do and how it works today. Note that at least on reasonable distribution configurations, "--listen" is no longer used by default (at least not for a couple of years now), and even in socket-activated mode, listening on IP sockets (TCP/TLS) requires some configuration before it works. At least out of the box, it crashes with a not-configured error. So some interaction is required to configure and activate that mode. While it is possible to run libvirtd as a non-root user, it's quite annoying to do so and requires sufficient amount of hoop-jumping (granting access to KVM socket, ensuring it has ability to bind to ports, configuring network, etc.) that it's easier to run it as root and then impose rules to effectively deprivilege it by other means (SELinux, daemon separation, etc.). Unless you're running a version of libvirt from before 2018, I think that your concerns are fairly well resolved. P.S.: Your Cc for your colleagues was malformed. I fixed it in my reply. -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH] apparmor: allow kvm-spice compat wrapper
On Tue, Nov 17, 2020 at 11:49 AM Christian Ehrhardt wrote: > > On Mon, Nov 16, 2020 at 3:28 PM Michal Privoznik wrote: > > > > On 11/16/20 1:26 PM, Christian Ehrhardt wrote: > > > 'kvm-spice' is a binary name used to call 'kvm' which actually is a > > > wrapper > > > around qemu-system-x86_64 enabling kvm acceleration. This isn't in use > > > for quite a while anymore, but required to work for compatibility e.g. > > > when migrating in old guests. > > > > > > For years this was a symlink kvm-spice->kvm and therefore covered > > > apparmor-wise by the existing entry: > > > /usr/bin/kvm rmix, > > > But due to a recent change [1] in qemu packaging this now is no symlink, > > > but a wrapper on its own and therefore needs an own entry that allows it > > > to be executed. > > > > > > [1]: https://salsa.debian.org/qemu-team/qemu/-/commit/9944836d3 > > > > > > Signed-off-by: Christian Ehrhardt > > > --- > > > src/security/apparmor/libvirt-qemu | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > > Reviewed-by: Michal Privoznik > > Thank you Michal, > it also passed fine through my tests (as backport to 6.8 and 6.9). > We are not in any freeze, review has happened, tests LGTM - pushed to git. > Hold up, why was this merged? Did anyone validate whether this would break the other AppArmor user (SUSE)? Unlike SELinux, AppArmor functionality is quite fragmented between Ubuntu and SUSE distributions (the two major users of AppArmor), and there did not seem to be any indication that this AppArmor patch was validated with openSUSE before merging. My personal experience with AppArmor across the two distribution families is that it's really easy to make profiles that work for Ubuntu but fail on SUSE because of the disparity of functionality. I also don't see Jim Fehlig stepping in to indicate that this worked for him. I haven't had a chance to test this myself, but I am immediately suspicious of a change that references a commit based on Debian packaging of QEMU. -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCH 2/2] spec: Drop UUID handling for default network
On Tue, Nov 17, 2020 at 2:29 PM Andrea Bolognani wrote:
>
> On Tue, 2020-11-17 at 13:01 -0500, Neal Gompa wrote:
> > On Sun, Nov 15, 2020 at 3:43 PM Andrea Bolognani
> > wrote:
> > > -UUID=`/usr/bin/uuidgen`
> > > sed -e "s/${orig_sub}/${sub}/g" \
> > > --e "s,,\n $UUID," \
> > > < %{_datadir}/libvirt/networks/default.xml \
> > > > %{_sysconfdir}/libvirt/qemu/networks/default.xml
> > > ln -s ../default.xml
> > > %{_sysconfdir}/libvirt/qemu/networks/autostart/default.xml
> >
> > Looks good to me, though when did we make this change?
>
> In the first patch of the series ;)
>
D'oh! This is why email patch review is hit-or-miss... :/
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCH 2/2] spec: Drop UUID handling for default network
On Sun, Nov 15, 2020 at 3:43 PM Andrea Bolognani wrote:
>
> We're no longer generating a UUID during installation, so we
> clearly don't need to strip it afterwards; and since the network
> driver is perfectly capable of generating a UUID if necessary, we
> don't need to do that at %post time either.
>
> Signed-off-by: Andrea Bolognani
> ---
> libvirt.spec.in | 4
> 1 file changed, 4 deletions(-)
>
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index 034982809d..f4cee8940d 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -1240,8 +1240,6 @@ cp -a
> $RPM_BUILD_ROOT%{_sysconfdir}/libvirt/nwfilter/*.xml \
> # libvirt saves these files with mode 600
> chmod 600 $RPM_BUILD_ROOT%{_sysconfdir}/libvirt/nwfilter/*.xml
>
> -# Strip auto-generated UUID - we need it generated per-install
> -sed -i -e "//d" $RPM_BUILD_ROOT%{_datadir}/libvirt/networks/default.xml
> %if ! %{with_qemu}
> rm -f $RPM_BUILD_ROOT%{_datadir}/augeas/lenses/libvirtd_qemu.aug
> rm -f $RPM_BUILD_ROOT%{_datadir}/augeas/lenses/tests/test_libvirtd_qemu.aug
> @@ -1422,9 +1420,7 @@ if test $1 -eq 1 && test ! -f
> %{_sysconfdir}/libvirt/qemu/networks/default.xml ;
> ;;
> esac
>
> -UUID=`/usr/bin/uuidgen`
> sed -e "s/${orig_sub}/${sub}/g" \
> --e "s,,\n $UUID," \
> < %{_datadir}/libvirt/networks/default.xml \
> > %{_sysconfdir}/libvirt/qemu/networks/default.xml
> ln -s ../default.xml
> %{_sysconfdir}/libvirt/qemu/networks/autostart/default.xml
> --
> 2.26.2
>
Looks good to me, though when did we make this change?
Regardless...
Reviewed-by: Neal Gompa
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCH 00/16] docs: add manpages for all the modular daemons
On Tue, Nov 17, 2020 at 12:03 PM Daniel P. Berrangé wrote: > > Most of the modular daemon stuff has been done from a single template, > but for the man pages that is more trouble than it is worth, so we > create a separate man page source for each daemon, which makes it easy > to extend with driver specific information. > > Daniel P. Berrangé (16): > docs: consistently mark libvirtd as preformatted text > docs: don't hardcode an ancient version in manpage examples > docs: tweak heading for daemon manual pages > docs: add manpage for virtproxyd > docs: add manpage for virtbhyved > docs: add manpage for virtinterfaced > docs: add manpage for virtlxcd > docs: add manpage for virtnetworkd > docs: add manpage for virtnodedevd > docs: add manpage for virtnwfilterd > docs: add manpage for virtqemud > docs: add manpage for virtsecretd > docs: add manpage for virtstoraged > docs: add manpage for virtvboxd > docs: add manpage for virtvzd > docs: add manpage for virtxend > > docs/manpages/index.rst | 25 ++- > docs/manpages/libvirtd.rst | 50 +++--- > docs/manpages/meson.build| 14 ++ > docs/manpages/virtbhyved.rst | 215 ++ > docs/manpages/virtinterfaced.rst | 215 ++ > docs/manpages/virtlockd.rst | 2 +- > docs/manpages/virtlogd.rst | 2 +- > docs/manpages/virtlxcd.rst | 215 ++ > docs/manpages/virtnetworkd.rst | 215 ++ > docs/manpages/virtnodedevd.rst | 214 ++ > docs/manpages/virtnwfilterd.rst | 215 ++ > docs/manpages/virtproxyd.rst | 256 +++ > docs/manpages/virtqemud.rst | 215 ++ > docs/manpages/virtsecretd.rst| 214 ++ > docs/manpages/virtstoraged.rst | 215 ++ > docs/manpages/virtvboxd.rst | 213 + > docs/manpages/virtvzd.rst| 215 ++ > docs/manpages/virtxend.rst | 215 ++ > 18 files changed, 2896 insertions(+), 29 deletions(-) > create mode 100644 docs/manpages/virtbhyved.rst > create mode 100644 docs/manpages/virtinterfaced.rst > create mode 100644 docs/manpages/virtlxcd.rst > create mode 100644 docs/manpages/virtnetworkd.rst > create mode 100644 docs/manpages/virtnodedevd.rst > create mode 100644 docs/manpages/virtnwfilterd.rst > create mode 100644 docs/manpages/virtproxyd.rst > create mode 100644 docs/manpages/virtqemud.rst > create mode 100644 docs/manpages/virtsecretd.rst > create mode 100644 docs/manpages/virtstoraged.rst > create mode 100644 docs/manpages/virtvboxd.rst > create mode 100644 docs/manpages/virtvzd.rst > create mode 100644 docs/manpages/virtxend.rst > > -- > 2.28.0 > > Series LGTM. Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH v2 00/12] Replace virHashTable by GHashTable
macmap.c | 4 +- > src/util/virstoragefile.c | 8 +- > src/util/virsystemd.c | 2 +- > tests/meson.build | 2 - > tests/nwfilterxml2firewalltest.c | 24 +- > tests/qemublocktest.c | 26 +- > tests/qemuhotplugtest.c | 6 +- > tests/qemumigparamstest.c | 4 +- > tests/qemumonitorjsontest.c | 30 +- > tests/qemumonitortestutils.c | 6 +- > tests/qemumonitortestutils.h | 4 +- > tests/qemusecuritymock.c | 4 +- > .../blockjob-blockdev-in.xml | 116 ++-- > tests/qemuxml2argvtest.c | 4 +- > tests/qemuxml2xmltest.c | 5 +- > tests/testutilsqemu.c | 6 +- > tests/testutilsqemu.h | 4 +- > tests/testutilsqemuschema.c | 10 +- > tests/testutilsqemuschema.h | 8 +- > tests/virdeterministichashmock.c | 36 -- > tests/virhashdata.h | 284 - > tests/virhashtest.c | 564 - > tests/virmacmaptest.c | 2 +- > 87 files changed, 690 insertions(+), 1682 deletions(-) > delete mode 100644 tests/virdeterministichashmock.c > delete mode 100644 tests/virhashdata.h > delete mode 100644 tests/virhashtest.c > > -- > 2.26.2 > Series LGTM. Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH 00/28] a bunch of domain_conf cleanup
On Thu, Nov 5, 2020 at 10:33 PM Matt Coleman wrote: > > Most of this is making functions void that unnecessarily return an int. > It also includes some conversion to GLib. > > Feel free to squash related commits, if you'd like. I left them separate > to make it easier to review. > > Matt Coleman (28): > domain_conf: make virDomainDiskSetDriver() void > domain_conf: make virDomainPostParseCheckISCSIPath() void > domain_conf: use g_free() in virDomainPostParseCheckISCSIPath() > domain_conf: make virDomainHostdevAssignAddress() void > domain_conf: make virDomainChr/RNG/Video/VsockDefPostParse() and > virDomainNVRAMDefFormat() void > domain_conf: make virDomainDeviceInfoFormat() void > domain_conf: make virDomainGraphicsDefParseXMLEGLHeadless() void > domain_conf: make virDomainLeaseDefFormat() void > domain_conf: make virDomainDiskSourceFormatNetwork() void > domain_conf: make virDomainDiskDefFormatIotune() void > domain_conf: make virDomainDiskDefFormatDriver() void > domain_conf: make virDomainControllerDriverFormat() void > domain_conf: make virDomainVirtioNetGuestOpts/HostOpts/DriverFormat() > void > domain_conf: make virDomainRedirFilterDefFormat() void > domain_conf: make virDomainIOMMUDefFormat() void > domain_conf: make virDomainDefFormatBlkiotune() void > domain_conf: make virDomainChrSourceDefFormat() void > domain_conf: make virDomainDiskSetBlockIOTune() void > domain_conf: use g_free in virDomainDiskSetBlockIOTune() > domain_conf: use g_renew in virDomainDiskInsert() and > virDomainControllerInsert() > domain_conf: make virDomainDiskInsert() void > domain_conf: make virDomainControllerInsert() void > domain_conf: use g_renew in virDomainLeaseInsertPreAlloc() > domain_conf: make virDomainLeaseInsertPreAlloc() void > domain_conf: make virDomainLeaseInsert() void > domain_conf: make virDomainPanicDefFormat() void > domain_conf: make virDomainShmemDefFormat() void > domain_conf: make virDomainVsockDefFormat() void > > src/conf/domain_conf.c | 349 ++- > src/conf/domain_conf.h | 21 +-- > src/libxl/libxl_conf.c | 5 +- > src/libxl/libxl_domain.c | 5 +- > src/libxl/libxl_driver.c | 9 +- > src/libxl/xen_xl.c | 12 +- > src/libxl/xen_xm.c | 10 +- > src/lxc/lxc_driver.c | 3 +- > src/qemu/qemu_domain.c | 5 +- > src/qemu/qemu_driver.c | 15 +- > src/qemu/qemu_hotplug.c | 3 +- > src/test/test_driver.c | 3 +- > src/vz/vz_sdk.c | 9 +- > tests/qemublocktest.c| 5 +- > 14 files changed, 158 insertions(+), 296 deletions(-) > > -- > 2.27.0 > > Series LGTM. Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCH 00/25] cgroup related fixes and cleanup
On Tue, Nov 3, 2020 at 7:42 AM Pavel Hrdina wrote: > > Pavel Hrdina (25): > qemu_cgroup: remove unused @empty variable > qemu: remove dead code that setup cgroups for helper processes > qemu_dbus: use emulator cgroup for dbus-daemon > vircgroupv2: properly detect empty tasks > vircgroupv2: properly detect placement of running VM > vircgroupv2: detect controllers enabled in parent cgroup > vircgroup: remove useless cgroup->path variable > vircgroup: introduce virCgroupSetBackends helper > vircgroup: introduce virCgroupCopyMounts helper > vircgroup: introduce virCgroupCopyPlacement helper > vircgroup: introduce virCgroupValidatePlacement helper > vircgroup: introduce virCgroupDetectControllers helper > vircgroup: extract virCgroupNewDetect from virCgroupNew > vircgroup: introduce virCgroupNewParent > vircgroup: drop @parent from virCgroupNew > vircgroup: virCgroupNew is now always called with absolute path > vircgroup: expand virCgroupDetect into virCgroupNew > vircgroup: no need to use PID in virCgroupEnableMissingControllers > vircgroup: drop @pid argument from virCgroupNew > vircgroup: introduce virCgroupSetPlacement > vircgroup: drop @create from virCgroupNewDomainPartition > vircgroup: refactor virCgroupEnableMissingControllers > vircgroup: move parentPath declaration > vircgroup: refactor virCgroupNewPartition > vircgroup: drop condition for absolute path from copyPlacement > callbacks > > src/qemu/qemu_cgroup.c | 5 +- > src/qemu/qemu_dbus.c| 9 +- > src/qemu/qemu_dbus.h| 3 +- > src/qemu/qemu_extdevice.c | 2 +- > src/qemu/qemu_slirp.c | 4 - > src/util/vircgroup.c| 337 ++-- > src/util/vircgroupbackend.h | 5 + > src/util/vircgrouppriv.h| 7 +- > src/util/vircgroupv1.c | 57 +++--- > src/util/vircgroupv2.c | 61 --- > tests/vircgrouptest.c | 33 ++-- > 11 files changed, 309 insertions(+), 214 deletions(-) > > -- > 2.26.2 > Series LGTM. Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH] remote: Add libvirtd dependency to virt-guest-shutdown.target
shutdown. > Oct 28 15:40:00 systemd[1]: Stopping Libvirt guests shutdown. > Oct 28 15:40:00 systemd[1]: Stopping Virtualization daemon... > Oct 28 15:40:00 systemd[1]: Stopped Virtualization daemon. > Oct 28 15:40:00 systemd[1]: Closed Virtual machine log manager socket. > Oct 28 15:40:00 systemd[1]: Stopping Virtual machine log manager socket. > Oct 28 15:40:00 systemd[1]: Listening on Virtual machine log manager socket. > Oct 28 15:40:00 systemd[1]: Closed Libvirt admin socket. > Oct 28 15:40:00 systemd[1]: Stopping Libvirt admin socket. > Oct 28 15:40:00 systemd[1]: Closed Libvirt local read-only socket. > Oct 28 15:40:00 systemd[1]: Stopping Libvirt local read-only socket. > Oct 28 15:40:00 systemd[1]: Closed Libvirt local socket. > Oct 28 15:40:00 systemd[1]: Stopping Libvirt local socket. > Oct 28 15:40:00 systemd[1]: Listening on Libvirt local socket. > Oct 28 15:40:00 systemd[1]: Listening on Libvirt admin socket. > Oct 28 15:40:00 systemd[1]: Listening on Libvirt local read-only socket. > Oct 28 15:40:00 systemd[1]: Closed Virtual machine lock manager socket. > Oct 28 15:40:00 systemd[1]: Stopping Virtual machine lock manager socket. > Oct 28 15:40:00 systemd[1]: Listening on Virtual machine lock manager socket. > Oct 28 15:40:00 systemd[1]: Starting Virtualization daemon... > Oct 28 15:40:00 systemd[1]: Started Virtualization daemon. > Oct 28 15:40:00 systemd[1]: Reached target Libvirt guests shutdown. > Oct 28 15:40:00 systemd[1]: Starting Suspend/Resume Running libvirt Guests... > Oct 28 15:40:00 systemd[1]: Started Suspend/Resume Running libvirt Guests. > > Signed-off-by: Jim Fehlig > --- > > I asked about this on the dev list but it is probably best to send a > patch for discussion instead. > > https://www.redhat.com/archives/libvir-list/2020-October/msg01492.html > > src/remote/virt-guest-shutdown.target | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/remote/virt-guest-shutdown.target > b/src/remote/virt-guest-shutdown.target > index 25d4aaa267..e2efa3e63a 100644 > --- a/src/remote/virt-guest-shutdown.target > +++ b/src/remote/virt-guest-shutdown.target > @@ -1,3 +1,4 @@ > [Unit] > Description=Libvirt guests shutdown > +Requires=libvirtd.service > Documentation=https://libvirt.org > -- > 2.28.0 > > LGTM. Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH 0/8] more Hyper-V code cleanup
On Mon, Nov 2, 2020 at 7:22 PM Matt Coleman wrote: > > Here's a draft GitLab MR if you'd prefer to review the changes there: > https://gitlab.com/iammattcoleman/libvirt/-/merge_requests/5 > > Matt Coleman (8): > hyperv: g_autofree username and password in hypervConnectOpen() > hyperv: remove spaces after hypervObject* casts > hyperv: WMI class list function general cleanup > hyperv: move hypervGetWmiClass to hyperv_wmi.h > hyperv: move hypervGetProcSDByVSSDInstanceId to hyperv_wmi.c > hyperv: consistent names for SettingData functions > hyperv: minor formatting fix in hyperv_wmi.h > hyperv: call openwsman's ws_serializer_free_mem > > src/hyperv/hyperv_driver.c | 87 ++ > src/hyperv/hyperv_wmi.c| 63 +++ > src/hyperv/hyperv_wmi.h| 45 ++-- > 3 files changed, 89 insertions(+), 106 deletions(-) > > -- > 2.27.0 > > Series LGTM. Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH] rpm: Fix handling of SOURCE_DATE_EPOCH
On Thu, Oct 29, 2020 at 9:03 AM Daniel P. Berrangé wrote:
>
> On Thu, Oct 29, 2020 at 09:01:05AM -0400, Neal Gompa wrote:
> > On Thu, Oct 29, 2020 at 7:39 AM Daniel P. Berrangé
> > wrote:
> > >
> > > On Thu, Oct 29, 2020 at 12:27:16PM +0100, Michal Privoznik wrote:
> > > > On 10/28/20 9:47 PM, Neal Gompa wrote:
> > > > > On Wed, Oct 28, 2020 at 7:49 AM Michal Privoznik
> > > > > wrote:
> > > > > >
> > > > > > On 10/27/20 1:06 PM, Neal Gompa wrote:
> > > > > > > On Tue, Oct 27, 2020 at 6:24 AM Michal Privoznik
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > On 10/26/20 11:08 PM, Neal Gompa wrote:
> > > > > > > > > Contemporary versions of Fedora automatically set
> > > > > > > > > SOURCE_DATE_EPOCH
> > > > > > > > > based on the changelog entry date stamp. In scenarios where
> > > > > > > > > it already
> > > > > > > > > is defined, we do not want to redefine it.
> > > > > > > > >
> > > > > > > >
> > > > > > > > This part is okay.
> > > > > > > >
> > > > > > > > > Additionally, when building the libvirt package in an Open
> > > > > > > > > Build Service
> > > > > > > > > instance, the spec file is not present in %_specdir, but
> > > > > > > > > instead in %_sourcedir.
> > > > > > > > >
> > > > > > > >
> > > > > > > > But this looks fishy. Is the %_specdir defined in that case?
> > > > > > > >
> > > > > > >
> > > > > > > It is (that comes from RPM itself), however the directory is
> > > > > > > empty.
> > > > > >
> > > > > > That feels like a bug in OBS then. IIUC this macro can be specified
> > > > > > on
> > > > > > the rpmbuild's cmd line. Can't it set the %_specdir to be the same
> > > > > > as
> > > > > > %_sourcedir?
> > > > > >
> > > > >
> > > > > Nothing about RPM mandates that %_specdir is actually *used* for
> > > > > anything.
> > > > >
> > > >
> > > > But this is not the case, is it? %_specdir is defined and points to an
> > > > actual directory. Having said that, I am not against the change, but
> > > > maybe
> > > > we can document this weirdness somewhere? Also, with the latest specfile
> > > > discussion I'll let Andrea take a look.
> > >
> >
> > RPM internally populates a specfile variable that is the true pointer
> > to the spec file, regardless of path. This is not exposed as a macro
> > despite attempts to do so[1]. There is no rule that the spec file
> > *must* come from %_specdir, and it's really only used for the location
> > to store the specfile when extracting an SRPM.
> >
> > [1]: https://github.com/rpm-software-management/rpm/pull/202
> >
> > > I'm inclined to just delete all the source epoch stuff from the spec and
> > > rely on the build environment set it if they want reproducable builds.
> > >
> >
> > Considering that RHEL 8 doesn't have the change to turn on
> > SOURCE_DATE_EPOCH[2], I am inclined to keep it.
>
> We can just wrap the current setting in "%if 0%{?rhel}" then, so we
> honour the setting from Fedora.
>
> >
> > [2]:
> > https://src.fedoraproject.org/rpms/redhat-rpm-config/c/86aae600e62fadc18760d95d1fddd323cf9e9a86
>
That doesn't really change the need for this. I'm building this in an
OBS system, and I want that property to take effect properly.
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCH 1/2] rpm: remove with_bash_completion condition
On Thu, Oct 29, 2020 at 5:40 AM Daniel P. Berrangé wrote:
>
> On Wed, Oct 28, 2020 at 04:49:43PM -0400, Neal Gompa wrote:
> > On Wed, Oct 28, 2020 at 8:36 AM Daniel P. Berrangé
> > wrote:
> > >
> > > The %meson macro sets "--auto-features=enabled", thus any feature in the
> > > RPM which has a "with_XXX" condition, needs to explicitly pass a
> > > "-DXXX=state" arg to %meson to override the auto features setting.
> > >
> > > The with_bash_completion condition is always set to 1, so rather than
> > > adding an arg to %meson, just remove the condition.
> > >
> > > Signed-off-by: Daniel P. Berrangé
> > > ---
> > > libvirt.spec.in | 15 ---
> > > 1 file changed, 15 deletions(-)
> > >
> > > diff --git a/libvirt.spec.in b/libvirt.spec.in
> > > index 84515cc7de..47fb53c681 100644
> > > --- a/libvirt.spec.in
> > > +++ b/libvirt.spec.in
> > > @@ -94,7 +94,6 @@
> > > %endif
> > >
> > > # Other optional features
> > > -%define with_bash_completion 0%{!?_without_bash_completion:1}
> > > %define with_numactl 0%{!?_without_numactl:1}
> > >
> > > # A few optional bits off by default, we enable later
> > > @@ -279,9 +278,7 @@ BuildRequires: glib2-devel >= 2.48
> > > BuildRequires: libxml2-devel
> > > BuildRequires: libxslt
> > > BuildRequires: readline-devel
> > > -%if %{with_bash_completion}
> > > BuildRequires: bash-completion >= 2.0
> > > -%endif
> > > BuildRequires: gettext
> > > BuildRequires: libtasn1-devel
> > > BuildRequires: gnutls-devel
> > > @@ -897,9 +894,7 @@ Requires: %{name}-libs = %{version}-%{release}
> > > Requires: gettext
> > > # Needed by virt-pki-validate script.
> > > Requires: gnutls-utils
> > > -%if %{with_bash_completion}
> > > Requires: %{name}-bash-completion = %{version}-%{release}
> > > -%endif
> > >
> > > %description client
> > > The client binaries needed to access the virtualization
> > > @@ -919,20 +914,16 @@ Shared libraries for accessing the libvirt daemon.
> > > %package admin
> > > Summary: Set of tools to control libvirt daemon
> > > Requires: %{name}-libs = %{version}-%{release}
> > > -%if %{with_bash_completion}
> > > Requires: %{name}-bash-completion = %{version}-%{release}
> > > -%endif
> > >
> > > %description admin
> > > The client side utilities to control the libvirt daemon.
> > >
> > > -%if %{with_bash_completion}
> > > %package bash-completion
> > > Summary: Bash completion script
> > >
> > > %description bash-completion
> > > Bash completion script stub.
> > > -%endif
> > >
> > > %if %{with_wireshark}
> > > %package wireshark
> > > @@ -1855,9 +1846,7 @@ exit 0
> > > %{_datadir}/systemtap/tapset/libvirt_qemu_probes*.stp
> > > %endif
> > >
> > > -%if %{with_bash_completion}
> > > %{_datadir}/bash-completion/completions/virsh
> > > -%endif
> > >
> > >
> > > %{_unitdir}/libvirt-guests.service
> > > @@ -1885,14 +1874,10 @@ exit 0
> > > %files admin
> > > %{_mandir}/man1/virt-admin.1*
> > > %{_bindir}/virt-admin
> > > -%if %{with_bash_completion}
> > > %{_datadir}/bash-completion/completions/virt-admin
> > > -%endif
> > >
> > > -%if %{with_bash_completion}
> > > %files bash-completion
> > > %{_datadir}/bash-completion/completions/vsh
> > > -%endif
> > >
> > > %if %{with_wireshark}
> > > %files wireshark
> > > --
> > > 2.26.2
> > >
> >
> > This doesn't make sense unless you're ripping out the conditional
> > logic from Meson. The bug here would be that flipping the conditional
> > does not flip the behavior in Meson.
>
> The RPM spec should only have conditionals that are actually needed to tune
> the build options for Fedora / RHEL distros. Since bash completion is always
> on for Fedora / RHEL, there's no need for a conditional in the RPM spec.
> Meson still wants the conditionals, because we keep all our features with
> external deps as conditional in the build system.
>
Fine, I guess...
Reviewed-by: Neal Gompa
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH] rpm: Fix handling of SOURCE_DATE_EPOCH
On Thu, Oct 29, 2020 at 7:39 AM Daniel P. Berrangé wrote: > > On Thu, Oct 29, 2020 at 12:27:16PM +0100, Michal Privoznik wrote: > > On 10/28/20 9:47 PM, Neal Gompa wrote: > > > On Wed, Oct 28, 2020 at 7:49 AM Michal Privoznik > > > wrote: > > > > > > > > On 10/27/20 1:06 PM, Neal Gompa wrote: > > > > > On Tue, Oct 27, 2020 at 6:24 AM Michal Privoznik > > > > > wrote: > > > > > > > > > > > > On 10/26/20 11:08 PM, Neal Gompa wrote: > > > > > > > Contemporary versions of Fedora automatically set > > > > > > > SOURCE_DATE_EPOCH > > > > > > > based on the changelog entry date stamp. In scenarios where it > > > > > > > already > > > > > > > is defined, we do not want to redefine it. > > > > > > > > > > > > > > > > > > > This part is okay. > > > > > > > > > > > > > Additionally, when building the libvirt package in an Open Build > > > > > > > Service > > > > > > > instance, the spec file is not present in %_specdir, but instead > > > > > > > in %_sourcedir. > > > > > > > > > > > > > > > > > > > But this looks fishy. Is the %_specdir defined in that case? > > > > > > > > > > > > > > > > It is (that comes from RPM itself), however the directory is empty. > > > > > > > > That feels like a bug in OBS then. IIUC this macro can be specified on > > > > the rpmbuild's cmd line. Can't it set the %_specdir to be the same as > > > > %_sourcedir? > > > > > > > > > > Nothing about RPM mandates that %_specdir is actually *used* for anything. > > > > > > > But this is not the case, is it? %_specdir is defined and points to an > > actual directory. Having said that, I am not against the change, but maybe > > we can document this weirdness somewhere? Also, with the latest specfile > > discussion I'll let Andrea take a look. > RPM internally populates a specfile variable that is the true pointer to the spec file, regardless of path. This is not exposed as a macro despite attempts to do so[1]. There is no rule that the spec file *must* come from %_specdir, and it's really only used for the location to store the specfile when extracting an SRPM. [1]: https://github.com/rpm-software-management/rpm/pull/202 > I'm inclined to just delete all the source epoch stuff from the spec and > rely on the build environment set it if they want reproducable builds. > Considering that RHEL 8 doesn't have the change to turn on SOURCE_DATE_EPOCH[2], I am inclined to keep it. [2]: https://src.fedoraproject.org/rpms/redhat-rpm-config/c/86aae600e62fadc18760d95d1fddd323cf9e9a86 -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH] Revert "spec: Simplify setting features off by default"
On Tue, Oct 27, 2020 at 9:30 AM Andrea Bolognani wrote:
>
> On Tue, 2020-10-27 at 14:05 +0100, Andrea Bolognani wrote:
> > I'm not convinced reverting this was the right call.
> >
> > The way RPM conditional macros work is that, if you have
> >
> > %{!?macro:value}
> >
> > that will expand to 'value' if 'macro' is *not* defined, and to
> > nothing otherwise. So if you have for example
> >
> > %define with_fuse 0%{!?_without_fuse:0}
> >
> > that means that, if you pass
> >
> > --define '_without_fuse 1'
> >
> > to rpmbuild the line will expand to
> >
> > %define with_fuse 0
> >
> > and if you don't pass the extra option to rpmbuild it will instead
> > expand to
> >
> > %define with_fuse 00
> >
> > The two are clearly equivalent, so there is no point in keeping the
> > conditional macro - it merely obfuscates the logic.
> >
> > Of course things would be different if you had
> >
> > %define with_fuse 0%{!?_without_fuse:1}
> >
> > instead: in this case, the line would expand to
> >
> > %define with_fuse 0
> >
> > and
> >
> > %define with_fuse 01
> >
> > respectively, which means the feature would be enabled by default but
> > could optionally be disabled by passing the correct argument to
> > rpmbuild, as expected. We have plenty similar macros in libvirt.spec,
> > and since they work just as intended 31d687a3218c didn't touch them.
> >
> > Note that in this case I've removed
> >
> > # fuse is used to provide virtualized /proc for LXC
> > %if %{with_lxc}
> > %define with_fuse 0%{!?_without_fuse:1}
> > %endif
> >
> > from the spec to make sure that the value for the 'fuse' option
> > passed to Meson depended solely on the value of the _without_fuse
> > macro, and then checked the rpmbuild output to compare.
>
Ugh, you're right, and those values need to be changed to 1.
> Also note that I'm aware you want to eventually push for adoption of
> the standard bcond macros, and I fully stand behind that desire! If
> this patch had been the first in a series that introduced bcond
> support and was clearing the path for that, I would have zero
> problems with it. As it is, however, you're simply reintroducing some
> of the obfuscation we had recently managed to get rid of, without
> getting anything in return.
>
Fixing this so that I can switch to bconds is going to be a massive
rewrite of how feature enablement works. That is not something I can
push for a 6.9.0 freeze break patch.
My in-progress rewrite is going to be a massive break in how this is managed...
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCH 1/2] rpm: remove with_bash_completion condition
On Wed, Oct 28, 2020 at 8:36 AM Daniel P. Berrangé wrote:
>
> The %meson macro sets "--auto-features=enabled", thus any feature in the
> RPM which has a "with_XXX" condition, needs to explicitly pass a
> "-DXXX=state" arg to %meson to override the auto features setting.
>
> The with_bash_completion condition is always set to 1, so rather than
> adding an arg to %meson, just remove the condition.
>
> Signed-off-by: Daniel P. Berrangé
> ---
> libvirt.spec.in | 15 ---
> 1 file changed, 15 deletions(-)
>
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index 84515cc7de..47fb53c681 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -94,7 +94,6 @@
> %endif
>
> # Other optional features
> -%define with_bash_completion 0%{!?_without_bash_completion:1}
> %define with_numactl 0%{!?_without_numactl:1}
>
> # A few optional bits off by default, we enable later
> @@ -279,9 +278,7 @@ BuildRequires: glib2-devel >= 2.48
> BuildRequires: libxml2-devel
> BuildRequires: libxslt
> BuildRequires: readline-devel
> -%if %{with_bash_completion}
> BuildRequires: bash-completion >= 2.0
> -%endif
> BuildRequires: gettext
> BuildRequires: libtasn1-devel
> BuildRequires: gnutls-devel
> @@ -897,9 +894,7 @@ Requires: %{name}-libs = %{version}-%{release}
> Requires: gettext
> # Needed by virt-pki-validate script.
> Requires: gnutls-utils
> -%if %{with_bash_completion}
> Requires: %{name}-bash-completion = %{version}-%{release}
> -%endif
>
> %description client
> The client binaries needed to access the virtualization
> @@ -919,20 +914,16 @@ Shared libraries for accessing the libvirt daemon.
> %package admin
> Summary: Set of tools to control libvirt daemon
> Requires: %{name}-libs = %{version}-%{release}
> -%if %{with_bash_completion}
> Requires: %{name}-bash-completion = %{version}-%{release}
> -%endif
>
> %description admin
> The client side utilities to control the libvirt daemon.
>
> -%if %{with_bash_completion}
> %package bash-completion
> Summary: Bash completion script
>
> %description bash-completion
> Bash completion script stub.
> -%endif
>
> %if %{with_wireshark}
> %package wireshark
> @@ -1855,9 +1846,7 @@ exit 0
> %{_datadir}/systemtap/tapset/libvirt_qemu_probes*.stp
> %endif
>
> -%if %{with_bash_completion}
> %{_datadir}/bash-completion/completions/virsh
> -%endif
>
>
> %{_unitdir}/libvirt-guests.service
> @@ -1885,14 +1874,10 @@ exit 0
> %files admin
> %{_mandir}/man1/virt-admin.1*
> %{_bindir}/virt-admin
> -%if %{with_bash_completion}
> %{_datadir}/bash-completion/completions/virt-admin
> -%endif
>
> -%if %{with_bash_completion}
> %files bash-completion
> %{_datadir}/bash-completion/completions/vsh
> -%endif
>
> %if %{with_wireshark}
> %files wireshark
> --
> 2.26.2
>
This doesn't make sense unless you're ripping out the conditional
logic from Meson. The bug here would be that flipping the conditional
does not flip the behavior in Meson.
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH] rpm: Fix handling of SOURCE_DATE_EPOCH
On Wed, Oct 28, 2020 at 7:49 AM Michal Privoznik wrote: > > On 10/27/20 1:06 PM, Neal Gompa wrote: > > On Tue, Oct 27, 2020 at 6:24 AM Michal Privoznik > > wrote: > >> > >> On 10/26/20 11:08 PM, Neal Gompa wrote: > >>> Contemporary versions of Fedora automatically set SOURCE_DATE_EPOCH > >>> based on the changelog entry date stamp. In scenarios where it already > >>> is defined, we do not want to redefine it. > >>> > >> > >> This part is okay. > >> > >>> Additionally, when building the libvirt package in an Open Build Service > >>> instance, the spec file is not present in %_specdir, but instead in > >>> %_sourcedir. > >>> > >> > >> But this looks fishy. Is the %_specdir defined in that case? > >> > > > > It is (that comes from RPM itself), however the directory is empty. > > That feels like a bug in OBS then. IIUC this macro can be specified on > the rpmbuild's cmd line. Can't it set the %_specdir to be the same as > %_sourcedir? > Nothing about RPM mandates that %_specdir is actually *used* for anything. -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH] rpm: Fix handling of SOURCE_DATE_EPOCH
On Tue, Oct 27, 2020 at 6:24 AM Michal Privoznik wrote: > > On 10/26/20 11:08 PM, Neal Gompa wrote: > > Contemporary versions of Fedora automatically set SOURCE_DATE_EPOCH > > based on the changelog entry date stamp. In scenarios where it already > > is defined, we do not want to redefine it. > > > > This part is okay. > > > Additionally, when building the libvirt package in an Open Build Service > > instance, the spec file is not present in %_specdir, but instead in > > %_sourcedir. > > > > But this looks fishy. Is the %_specdir defined in that case? > It is (that comes from RPM itself), however the directory is empty. -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCH 1/2] rpc: Fix virt-ssh-helper detection
On Mon, Oct 26, 2020 at 7:30 PM Andrea Bolognani wrote:
>
> When trying to figure out whether virt-ssh-helper is available
> on the remote host, we mistakenly look for the helper by the
> name it had while the feature was being worked on instead of
> the one that was ultimately picked, and thus end up using the
> netcat fallback every single time.
>
> Fixes: f8ec7c842df9e40c6607eae9b0223766cb226336
> Signed-off-by: Andrea Bolognani
> ---
> src/rpc/virnetclient.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
> index c6591ecdfc..2eabacd6e8 100644
> --- a/src/rpc/virnetclient.c
> +++ b/src/rpc/virnetclient.c
> @@ -453,7 +453,7 @@ virNetClientSSHHelperCommand(virNetClientProxy proxy,
>
> switch (proxy) {
> case VIR_NET_CLIENT_PROXY_AUTO:
> -return g_strdup_printf("sh -c 'which virt-nc 1>/dev/null 2>&1; "
> +return g_strdup_printf("sh -c 'which virt-ssh-helper 1>/dev/null
> 2>&1; "
> "if test $? = 0; then "
> "%s; "
> "else"
> --
> 2.26.2
>
Reviewed-by: Neal Gompa
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCH 2/2] news: Mention virt-ssh-helper detection fix
On Mon, Oct 26, 2020 at 7:31 PM Andrea Bolognani wrote: > > Signed-off-by: Andrea Bolognani > --- > NEWS.rst | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/NEWS.rst b/NEWS.rst > index 3587bc2c13..5bd8ed1c91 100644 > --- a/NEWS.rst > +++ b/NEWS.rst > @@ -46,6 +46,12 @@ v6.9.0 (unreleased) > Relying on the "Description" field caused queries to fail on non-"en-US" > systems. The queries have been updated to avoid using localized strings. > > + * rpc: Fix ``virt-ssh-helper`` detection > + > +libvirt 6.8.0 failed to correctly detect the availability of the new > +``virt-ssh-helper`` command on the remote host, and thus always used the > +fallback instead; this has now been fixed. > + > > v6.8.0 (2020-10-01) > === > -- > 2.26.2 > Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
[PATCH] rpm: Fix handling of SOURCE_DATE_EPOCH
Contemporary versions of Fedora automatically set SOURCE_DATE_EPOCH
based on the changelog entry date stamp. In scenarios where it already
is defined, we do not want to redefine it.
Additionally, when building the libvirt package in an Open Build Service
instance, the spec file is not present in %_specdir, but instead in %_sourcedir.
Signed-off-by: Neal Gompa
---
libvirt.spec.in | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 2a4324b974..eb0fba4b5a 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1128,7 +1128,16 @@ exit 1
# place macros above and build commands below this comment
-export SOURCE_DATE_EPOCH=$(stat --printf='%Y' %{_specdir}/%{name}.spec)
+if [ -z "${SOURCE_DATE_EPOCH}" ]; then
+# If SOURCE_DATE_EPOCH is not defined, set it based on spec file
+if [ ! -f "%{_specdir}/%{name}.spec" ]; then
+# OBS does not install the spec file into the specdir
+SPECFILE_PATH="%{_sourcedir}/%{name}.spec"
+else
+SPECFILE_PATH="%{_specdir}/%{name}.spec"
+fi
+export SOURCE_DATE_EPOCH=$(stat --printf='%Y' ${SPECFILE_PATH})
+fi
%meson \
-Drunstatedir=%{_rundir} \
--
2.28.0
Re: Entering freeze for libvirt-6.9.0
On Mon, Oct 26, 2020 at 2:03 PM Jiri Denemark wrote: > > I have just tagged v6.9.0-rc1 in the repository and pushed signed > tarballs and source RPMs to https://libvirt.org/sources/ > > Please give the release candidate some testing and in case you find a > serious issue which should have a fix in the upcoming release, feel > free to reply to this thread to make sure the issue is more visible. > > If you have not done so yet, please update NEWS.rst to document any > significant change you made since the last release. > I encountered two basic issues when testing package builds in my pipeline, and have sent patches to the list: * https://www.redhat.com/archives/libvir-list/2020-October/msg01407.html * https://www.redhat.com/archives/libvir-list/2020-October/msg01408.html Those two were the most urgent I discovered with a quick run for my pipeline. -- 真実はいつも一つ！/ Always, there's only one truth!
[PATCH] Revert "spec: Simplify setting features off by default"
As it turns out, the rather complicated structure that is
currently used for enabling or disabling features in the libvirt
build does not cleanly map well to RPM's bcond feature.
Consequently, we need these back in order to support trivially
activating these features through extra macros as build inputs.
This reverts commit 31d687a3218c9072d7050dd608e013e063ca180f.
Signed-off-by: Neal Gompa
---
libvirt.spec.in | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 2a4324b974..84515cc7de 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -98,14 +98,14 @@
%define with_numactl 0%{!?_without_numactl:1}
# A few optional bits off by default, we enable later
-%define with_fuse 0
-%define with_sanlock 0
-%define with_numad0
-%define with_firewalld_zone 0
-%define with_libssh2 0
-%define with_wireshark0
-%define with_libssh 0
-%define with_dmidecode0
+%define with_fuse 0%{!?_without_fuse:0}
+%define with_sanlock 0%{!?_without_sanlock:0}
+%define with_numad0%{!?_without_numad:0}
+%define with_firewalld_zone 0%{!?_without_firewalld:0}
+%define with_libssh2 0%{!?_without_libssh2:0}
+%define with_wireshark0%{!?_without_wireshark:0}
+%define with_libssh 0%{!?_without_libssh:0}
+%define with_dmidecode0%{!?_without_dmidecode:0}
# Finally set the OS / architecture specific special cases
--
2.28.0
Re: [PATCH 0/4] hyperv: Deduplicate and reformat
On Fri, Oct 23, 2020 at 10:53 AM Neal Gompa wrote: > > On Wed, Oct 21, 2020 at 11:45 AM Michal Privoznik wrote: > > > > The more I look into the code the more things to fix I find. Well, here > > are some fixes. > > > > Michal Prívozník (4): > > hyperv: Don't overwrite errors from hypervCreateInvokeParamsList() > > hyperv: Use hypervRequestStateChange() in hypervDomainSuspend() > > hyperv: Use two empty lines between functions > > hyperv: Reformat > > > > src/hyperv/hyperv_driver.c | 186 +--- > > src/hyperv/hyperv_util.c | 1 - > > src/hyperv/hyperv_wmi.c| 288 +++-- > > src/hyperv/hyperv_wmi.h| 15 +- > > 4 files changed, 229 insertions(+), 261 deletions(-) > > > > -- > > 2.26.2 > > > > Series LGTM. > > Reviewed-by: Neal Gompa > > But... uhh... there's also this series from Matt Coleman, which I just > reviewed: > https://www.redhat.com/archives/libvir-list/2020-October/msg01282.html > > Can we put the two together somehow? > Ignore me, apparently this was seen by him already (his review was in spam...) -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH 0/4] hyperv: Deduplicate and reformat
On Wed, Oct 21, 2020 at 11:45 AM Michal Privoznik wrote: > > The more I look into the code the more things to fix I find. Well, here > are some fixes. > > Michal Prívozník (4): > hyperv: Don't overwrite errors from hypervCreateInvokeParamsList() > hyperv: Use hypervRequestStateChange() in hypervDomainSuspend() > hyperv: Use two empty lines between functions > hyperv: Reformat > > src/hyperv/hyperv_driver.c | 186 +--- > src/hyperv/hyperv_util.c | 1 - > src/hyperv/hyperv_wmi.c| 288 +++-- > src/hyperv/hyperv_wmi.h| 15 +- > 4 files changed, 229 insertions(+), 261 deletions(-) > > -- > 2.26.2 > Series LGTM. Reviewed-by: Neal Gompa But... uhh... there's also this series from Matt Coleman, which I just reviewed: https://www.redhat.com/archives/libvir-list/2020-October/msg01282.html Can we put the two together somehow? -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH 0/6] Hyper-V code cleanup
On Thu, Oct 22, 2020 at 12:38 PM Matt Coleman wrote: > > Here's a draft GitLab MR if you'd prefer to review the changes there: > https://gitlab.com/iammattcoleman/libvirt/-/merge_requests/3/diffs > > Matt Coleman (6): > hyperv: reformat WQL query strings > hyperv: remove duplicate function hypervGetVSSDFromUUID() > hyperv: remove duplicate function hypervGetMemSDByVSSDInstanceId() > hyperv: remove unneeded braces in hypervDomainGetInfo() and > hypervDomainGetXMLDesc() > hyperv: reduce duplicate code for Msvm_ComputerSystem lookups > hyperv: do not overwrite errors from hypervInvokeMethod() > > src/hyperv/hyperv_driver.c | 169 + > src/hyperv/hyperv_wmi.c| 57 +++-- > src/hyperv/hyperv_wmi.h| 4 + > 3 files changed, 75 insertions(+), 155 deletions(-) > > -- > 2.27.0 > > Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
Re: Hyper-V CPU details
On Thu, Oct 22, 2020 at 11:01 PM Matt Coleman wrote: > > Hello, > > I'm implementing domainGetVcpus and could use some guidance on what > value to use for virVcpuInfo->cpu. > > Hyper-V does not allow the user to pin vCPUs to host CPUs and doesn't > allow the user to see which host CPU a vCPU is currently running on. > Since it's a type 1 hypervisor, none of its scheduling data is > available to the Windows userspace: there aren't any processes or > threads that correspond to vCPUs that I could query the OS scheduler > about. > > My code currently sets it to -1, which produces the following `virsh > vcpuinfo` output for a running VM with two cores: > > VCPU: 0 > CPU:-1 > State: running > CPU time: 1684.5s > CPU Affinity: > > VCPU: 1 > CPU:-1 > State: running > CPU time: 1346.0s > CPU Affinity: > > However, this doesn't match the comment in _virVcpuInfo's declaration, > which says that -1 signifies an offline CPU: > > https://gitlab.com/libvirt/libvirt/-/blob/v6.8.0/include/libvirt/libvirt-domain.h#L1918 > > Should I stick with -1? Or, should I introduce -2 as a value that > indicates that the hypervisor doesn't provide that information? Or, is > there some better way to handle this that I'm not aware of? > I would suggest introducing a new value. The problem with overloading values is that if there's other code expecting that to mean something, it gets confusing really quickly. -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH 0/2] hyperv: bump minimum openwsman version to 2.6.3
On Wed, Oct 14, 2020 at 9:28 PM Neal Gompa wrote: > > On Fri, Oct 9, 2020 at 3:47 AM Matt Coleman wrote: > > > > As Daniel mentioned in the thread for my commit "hyperv: make > > Msvm_ComputerSystem WQL queries locale agnostic", we can increase the > > minimum openwsman version since all the supported distributions have at > > least version 2.6.3. > > > > Comments about older versions have been removed throughout the codebase. > > > > Also, openwsman.h has been removed entirely, since its original purpose > > was to work around bugs in earlier (now unsupported) openwsman versions. > > > > Matt Coleman (2): > > hyperv: bump minimum openwsman version to 2.6.3 > > hyperv: remove openwsman.h > > > > libvirt.spec.in | 2 +- > > meson.build | 2 +- > > src/hyperv/hyperv_driver.c | 5 +--- > > src/hyperv/hyperv_private.h | 3 ++- > > src/hyperv/hyperv_wmi.c | 12 +++-- > > src/hyperv/hyperv_wmi.h | 1 - > > src/hyperv/hyperv_wmi_classes.h | 3 ++- > > src/hyperv/openwsman.h | 47 - > > 8 files changed, 10 insertions(+), 65 deletions(-) > > delete mode 100644 src/hyperv/openwsman.h > > > > -- > > 2.27.0 > > > > > > Series LGTM. > > Reviewed-by: Neal Gompa > Blech, ignore this, the threading was broken due to the auto-marking as spam and I didn't know this was already pushed... -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH 0/2] hyperv: bump minimum openwsman version to 2.6.3
On Fri, Oct 9, 2020 at 3:47 AM Matt Coleman wrote: > > As Daniel mentioned in the thread for my commit "hyperv: make > Msvm_ComputerSystem WQL queries locale agnostic", we can increase the > minimum openwsman version since all the supported distributions have at > least version 2.6.3. > > Comments about older versions have been removed throughout the codebase. > > Also, openwsman.h has been removed entirely, since its original purpose > was to work around bugs in earlier (now unsupported) openwsman versions. > > Matt Coleman (2): > hyperv: bump minimum openwsman version to 2.6.3 > hyperv: remove openwsman.h > > libvirt.spec.in | 2 +- > meson.build | 2 +- > src/hyperv/hyperv_driver.c | 5 +--- > src/hyperv/hyperv_private.h | 3 ++- > src/hyperv/hyperv_wmi.c | 12 +++-- > src/hyperv/hyperv_wmi.h | 1 - > src/hyperv/hyperv_wmi_classes.h | 3 ++- > src/hyperv/openwsman.h | 47 - > 8 files changed, 10 insertions(+), 65 deletions(-) > delete mode 100644 src/hyperv/openwsman.h > > -- > 2.27.0 > > Series LGTM. Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCH 3/3] ci: Start building RPMs
On Fri, Oct 9, 2020 at 4:49 AM Andrea Bolognani wrote: > > On Thu, 2020-10-08 at 22:17 -0400, Neal Gompa wrote: > > On Thu, Oct 8, 2020 at 1:43 PM Andrea Bolognani wrote: > > > We lost this coverage during the move from CentOS CI to GitLab CI, > > > and it's high time we brought it back. > > > > > > Building RPMs is currently skipped for: > > > > > > * openSUSE, which is not supported by our spec file; > > > > I've got a patch set locally that adds the few knobs needed to make > > openSUSE build with the upstream spec file. If you want, I can clean > > that up and submit it for upstream inclusion? > > Sure thing! > I'll do it after your spec cleanup patch set lands, because otherwise it's going to be a bit hellish to rebase. -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCH v2 0/9] spec: Improve feature and architecture handling
On Fri, Oct 9, 2020 at 5:27 AM Andrea Bolognani wrote: > > Changes from [v1]: > > * address review feedback. > > [v1] https://www.redhat.com/archives/libvir-list/2020-October/msg00336.html > > Andrea Bolognani (9): > spec: Simplify setting features off by default > spec: firewalld is always enabled > spec: bash completion actually defaults to on > spec: Move with_numactl definition > spec: Introduce with_dmidecode > spec: Move _vpath_builddir definition > spec: Drop s390 architecture from conditionals > spec: Refactor qemu_kvm_arches definition > spec: Introduce arches_* > > libvirt.spec.in | 116 > 1 file changed, 57 insertions(+), 59 deletions(-) > > -- > 2.26.2 > > Series LGTM. Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCH 3/3] ci: Start building RPMs
On Thu, Oct 8, 2020 at 1:43 PM Andrea Bolognani wrote: > > We lost this coverage during the move from CentOS CI to GitLab CI, > and it's high time we brought it back. > > Building RPMs is currently skipped for: > > * openSUSE, which is not supported by our spec file; > I've got a patch set locally that adds the few knobs needed to make openSUSE build with the upstream spec file. If you want, I can clean that up and submit it for upstream inclusion? -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCH] spec: Explain the BuildRequires on firewalld-filesystem
On Thu, Oct 8, 2020 at 7:06 AM Andrea Bolognani wrote:
>
> It's not immediately obvious why it is needed.
>
> Suggested-by: Pavel Hrdina
> Signed-off-by: Andrea Bolognani
> ---
> libvirt.spec.in | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index 6d4eef86ad..c0d84c0e75 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -399,6 +399,7 @@ BuildRequires: rpcgen
>
> BuildRequires: libtirpc-devel
>
> +# Needed for the %firewalld_reload macro
> %if %{with_firewalld_zone}
> BuildRequires: firewalld-filesystem
> %endif
> --
> 2.26.2
>
I'd suggest putting the comment inside the conditional, but otherwise it's fine.
Reviewed-by: Neal Gompa
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCHv2 00/16] refactor qemuAgentGetInterfaces
On Wed, Oct 7, 2020 at 8:35 AM Ján Tomko wrote: > > v2: > * add new patch separating error checking in qemuAgentGetInterfaceOneAddress > * split out naddrs++ on a separate line > * added documentation for qemuAgentGetInterfaceAddresses > * added qemuAgentGetAllInterfaceAddresses > > Ján Tomko (16): > qemu: agent: remove redundant checks > qemu: agent: split out qemuAgentGetInterfaceOneAddress > qemu: agent: remove impossible errors > qemu: agent: reduce scope of addrs_count > qemu: agent: expand addrs upfront > qemu: agent: use g_auto for ifname > qemu: agent: use virHashNew > qemu: agent: simplify access to ifaces_ret > qemu: agent: split out qemuAgentGetInterfaceAddresses > qemu: agent: use GetArray to remove a check > qemu: agent: use g_auto in qemuAgentGetInterfaces > qemu: agent: remove cleanup in qemuAgentGetInterfaces > qemuAgentGetInterfaceAddresses: turn ifname into char* > qemu: agent: rename tmp_iface to iface_obj > qemuAgentGetInterfaceOneAddress: check for errors early > qemu: agent: split out qemuAgentGetAllInterfaceAddresses > > src/qemu/qemu_agent.c | 355 +- > 1 file changed, 180 insertions(+), 175 deletions(-) > > -- > 2.26.2 > Series LGTM. Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCH 00/13] conf: use g_new0 (glib chronicles)
On Wed, Oct 7, 2020 at 3:22 PM Ján Tomko wrote: > > *** BLURB HERE *** > *** BLURB THERE *** > *** BLURB EVEYWHERE *** > > Ján Tomko (13): > conf: domain: use g_new0 > conf: capabilities: use g_new0 > conf: network: use g_new0 > conf: node_device: use g_new0 > conf: domain_addr: use g_new0 > conf: interface: use g_new0 > conf: numa: use g_new0 > conf: storage: use g_new0 > conf: virsecretobj: use g_new0 > conf: nwfilter_params: use g_new0 > conf: cpu: use g_new0 > conf: use g_new0 > conf: virDomainUSBAddressHubNew: refactor > > src/conf/capabilities.c | 39 +-- > src/conf/checkpoint_conf.c| 4 +- > src/conf/cpu_conf.c | 15 +- > src/conf/domain_addr.c| 37 +-- > src/conf/domain_capabilities.c| 7 +- > src/conf/domain_conf.c| 347 ++ > src/conf/domain_event.c | 3 +- > src/conf/domain_nwfilter.c| 3 +- > src/conf/interface_conf.c | 29 +-- > src/conf/netdev_bandwidth_conf.c | 9 +- > src/conf/netdev_vlan_conf.c | 4 +- > src/conf/netdev_vport_profile_conf.c | 3 +- > src/conf/network_conf.c | 37 +-- > src/conf/networkcommon_conf.c | 3 +- > src/conf/node_device_conf.c | 37 +-- > src/conf/numa_conf.c | 23 +- > src/conf/nwfilter_conf.c | 12 +- > src/conf/nwfilter_params.c| 21 +- > src/conf/object_event.c | 11 +- > src/conf/secret_conf.c| 3 +- > src/conf/snapshot_conf.c | 4 +- > src/conf/storage_conf.c | 21 +- > src/conf/virchrdev.c | 10 +- > src/conf/virdomaincheckpointobjlist.c | 10 +- > src/conf/virdomainmomentobjlist.c | 6 +- > src/conf/virdomainobjlist.c | 8 +- > src/conf/virdomainsnapshotobjlist.c | 9 +- > src/conf/virinterfaceobj.c| 5 +- > src/conf/virnetworkobj.c | 7 +- > src/conf/virnetworkportdef.c | 4 +- > src/conf/virnodedeviceobj.c | 7 +- > src/conf/virnwfilterbindingdef.c | 6 +- > src/conf/virnwfilterbindingobjlist.c | 8 +- > src/conf/virnwfilterobj.c | 9 +- > src/conf/virsecretobj.c | 21 +- > 35 files changed, 271 insertions(+), 511 deletions(-) > > -- > 2.26.2 > Series LGTM. Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH] spec: Add cpu.rng to %files
On Wed, Oct 7, 2020 at 7:22 PM Cole Robinson wrote:
>
> Fixes: 51v5d325240c645ea6c1a0902c695cf299410b1f90c
>
> Signed-off-by: Cole Robinson
> ---
> Pushed as trivial
>
> libvirt.spec.in | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index 933e12f4d2..52f30be096 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -1868,6 +1868,7 @@ exit 0
>
> %{_datadir}/libvirt/schemas/basictypes.rng
> %{_datadir}/libvirt/schemas/capability.rng
> +%{_datadir}/libvirt/schemas/cpu.rng
> %{_datadir}/libvirt/schemas/cputypes.rng
> %{_datadir}/libvirt/schemas/domain.rng
> %{_datadir}/libvirt/schemas/domainbackup.rng
> --
> 2.28.0
>
Reviewed-by: Neal Gompa
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCH 9/9] spec: Introduce arches_*
On Tue, Oct 6, 2020 at 12:35 PM Andrea Bolognani wrote:
>
> On Tue, 2020-10-06 at 08:15 -0400, Neal Gompa wrote:
> > Then can we flip this conditional to %if 0%{?rhel} for the
> > architecture list? As it is, it's unclear that the reason that *RHEL*
> > is the less-capable variant.
>
> Are you thinking of something like
>
> %define arches_qemu_kvm %{arches_x86} %{power64} s390x %{arm} aarch64
> %if 0%{?rhel}
> %define arches_qemu_kvm x86_64 %{power64} aarch64 s390x
> %endif
>
> ? I can definitely go that route.
>
That's a way to do it. Another way to do it:
%if 0%{?rhel}
%define arches_qemu_kvm x86_64 %{power64} aarch64 s390x
%else
%define arches_qemu_kvm %{arches_x86} %{power64} s390x %{arm} aarch64
%endif
But either way works. :)
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH v2 00/10] hyperv: implement new APIs & more
On Mon, Oct 5, 2020 at 9:07 PM Neal Gompa wrote: > > On Mon, Oct 5, 2020 at 12:20 PM Matt Coleman wrote: > > > > These patches fix a couple bugs, consolidate duplicate code, and > > implement several APIs. > > > > Currently, some interactions with Hyper-V systems fail when the system > > is not configured for the "en-US" locale. Additionally, some CPU names > > also contain the clock frequency, making it too long for > > _virNodeInfo.model. The first two patches fix these bugs. > > > > The second two patches clean up the code a little: one moves repeated > > operations into new helper functions; the other replaces the generic > > "get WMI class list" functions with a macro. > > > > The next four patches implement the following APIs in the Hyper-V > > driver: > > * virConnectGetCapabilities() > > * virConnectGetMaxVcpus() > > * virConnectGetVersion() > > * virDomainGetAutostart() > > > > Changes since v1: > > * all NEWS updates are now in a single commit at the end > > * long function calls have been split up into multiple lines > > * improved error handling in hypervDomainLookupBy*(): they now throw > > VIR_ERR_NO_DOMAIN when appropriate > > * unnecessary calls to virReportError() have been removed from > > hypervDomainGetInfo() and hypervDomainGetXMLDesc() > > * HTML documentation for the unusual Hyper-V versions > > * the WMI schema fix has been split into a separate commit > > * hypervConnectGetVersion() uses a new hypervParseVersionString() and > > directly produces the unsigned long instead of calling > > virParseVersionString() > > > > Matt Coleman (10): > > hyperv: make Msvm_ComputerSystem WQL queries locale agnostic > > hyperv: fix nodeGetInfo failures caused by long CPU names > > hyperv: break out common lookups into separate functions > > hyperv: replace generic WMI class list helpers with a macro > > hyperv: implement connectGetCapabilities > > hyperv: implement connectGetMaxVcpus > > hyperv: fix Win32_OperatingSystem WMI queries > > hyperv: implement connectGetVersion > > hyperv: implement domainGetAutostart > > news: document new Hyper-V features and bug fixes > > > > NEWS.rst | 10 + > > docs/drvhyperv.html.in| 50 ++ > > src/hyperv/hyperv_driver.c| 751 ++ > > src/hyperv/hyperv_private.h | 2 + > > src/hyperv/hyperv_wmi.c | 83 +-- > > src/hyperv/hyperv_wmi.h | 28 +- > > src/hyperv/hyperv_wmi_classes.h | 4 +- > > src/hyperv/hyperv_wmi_generator.input | 2 +- > > 8 files changed, 596 insertions(+), 334 deletions(-) > > > > -- > > 2.27.0 > > > > > > In general, the series looks good to me, though please take a look at > my feedback for Patch 1. > With my question resolved, I'm happy to ack the whole series. Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCH 9/9] spec: Introduce arches_*
On Tue, Oct 6, 2020 at 6:41 AM Andrea Bolognani wrote:
>
> On Mon, 2020-10-05 at 20:40 -0400, Neal Gompa wrote:
> > On Mon, Oct 5, 2020 at 2:41 PM Andrea Bolognani wrote:
> > > %if 0%{?fedora}
> > > +%define arches_qemu_kvm %{arches_x86} %{power64} s390x %{arm}
> > > aarch64
> > > %else
> > > +%define arches_qemu_kvm x86_64 %{power64} aarch64 s390x
> > > %endif
> >
> > This conditional is functionally irrelevant. The superset defined for
> > Fedora does not change how things work for RHEL, and it'd be easier to
> > just use the one architecture set.
>
> The difference I can see is that %{ix86} is not currently included in
> %{arches_qemu_kvm} on RHEL, but with your change it would and, unlike
> what happens for 32-bit ARM, RHEL packages are actually being built
> on i686.
>
> Later on we have
>
> > > %define with_storage_gluster 0%{!?_without_storage_gluster:1}
> > > +%ifnarch %{arches_qemu_kvm}
> > > # gluster is only built where qemu driver is enabled on RHEL 8
> > > %if 0%{?rhel} >= 8
> > > %define with_storage_gluster 0
>
> and AFAICT that would break with your proposed change, because we
> would try to build with gluster support on i686 RHEL where gluster is
> not actually available.
>
Then can we flip this conditional to %if 0%{?rhel} for the
architecture list? As it is, it's unclear that the reason that *RHEL*
is the less-capable variant.
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCH 6/9] spec: Move _vpath_builddir definition
On Tue, Oct 6, 2020 at 6:28 AM Andrea Bolognani wrote:
>
> On Mon, 2020-10-05 at 20:46 -0400, Neal Gompa wrote:
> > On Mon, Oct 5, 2020 at 2:41 PM Andrea Bolognani wrote:
> > > It belongs before package-specific feature flags are defined.
> > >
> > > Signed-off-by: Andrea Bolognani
> > > ---
> > > libvirt.spec.in | 10 +-
> > > 1 file changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/libvirt.spec.in b/libvirt.spec.in
> > > index 2401404008..d8f689e651 100644
> > > --- a/libvirt.spec.in
> > > +++ b/libvirt.spec.in
> > > @@ -12,6 +12,11 @@
> > > %define supported_platform 0
> > > %endif
> > >
> > > +# On RHEL 7 and older macro _vpath_builddir is not defined.
> > > +%if 0%{?rhel} && 0%{?rhel} <= 7
> > > +%define _vpath_builddir %{_target_platform}
> > > +%endif
> >
> > Do we still need this anymore? Meson in EPEL 7 works without this
> > definition...
>
> We're installing Meson from PyPi instead of EPEL on CentOS as part of
> our CI setup, but indeed it looks like that's not necessary and we
> could simply use the EPEL package. I'll look into that.
>
> Anyway, at least on my Fedora machine _vpath_builddir is defined in
> /usr/lib/rpm/macros.d/macros.vpath, which is part of the
> redhat-rpm-config package, and the various %meson macros defined in
> /usr/lib/rpm/macros.d/macros.meson use it, so it doesn't look like
> even installing Meson from EPEL would remove the need to define that
> value? Note I haven't actually tried O:-)
>
In EPEL 7, the %_vpath_* macros are defined by epel-rpm-macros, which
are installed automatically into the build environment when you do a
package build. We could just have this installed in our CentOS 7 CI
environment.
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCH 1/9] spec: Simplify setting features off by default
On Tue, Oct 6, 2020 at 6:09 AM Andrea Bolognani wrote:
>
> On Mon, 2020-10-05 at 20:42 -0400, Neal Gompa wrote:
> > On Mon, Oct 5, 2020 at 2:40 PM Andrea Bolognani wrote:
> > > -%define with_fuse 0%{!?_without_fuse:0}
> > > -%define with_sanlock 0%{!?_without_sanlock:0}
> > > -%define with_numad 0%{!?_without_numad:0}
> > > -%define with_firewalld 0%{!?_without_firewalld:0}
> > > -%define with_firewalld_zone 0%{!?_without_firewalld_zone:0}
> > > -%define with_libssh2 0%{!?_without_libssh2:0}
> > > -%define with_wireshark 0%{!?_without_wireshark:0}
> > > -%define with_libssh0%{!?_without_libssh:0}
> > > -%define with_bash_completion 0%{!?_without_bash_completion:0}
> > > +%define with_fuse 0
> > > +%define with_sanlock 0
> > > +%define with_numad0
> > > +%define with_firewalld0
> > > +%define with_firewalld_zone 0
> > > +%define with_libssh2 0
> > > +%define with_wireshark0
> > > +%define with_libssh 0
> > > +%define with_bash_completion 0
> >
> > This crazy setup is an attempt to implement rpm %bcond behavior
> > without actually using %bcond statements. Instead of removing the
> > right-hand-side conditionals, perhaps it'd be better to just finally
> > make the switch to %bcond statements.
>
> Do you have any idea what the availability of %bcond is?
> Specifically, can we use it on RHEL 7? If so, I can take a stab at
> converting our spec file, but I don't think that invalidates the
> current series and I would prefer it to be merged first. It will be a
> better starting point for the conversion anyway.
>
%bcond support has been around since 2000. :)
If this is merged first, I can then convert them to %bconds...
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH] mailmap: consolidate my email addresses
On Mon, Oct 5, 2020 at 3:15 PM Matt Coleman wrote: > > Signed-off-by: Matt Coleman > --- > .mailmap | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/.mailmap b/.mailmap > index 2b080568bc..9dfb7a832b 100644 > --- a/.mailmap > +++ b/.mailmap > @@ -47,6 +47,7 @@ > > > > + > > # Name consolidation: > # Preferred author spelling > -- > 2.27.0 > > Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH v2 01/10] hyperv: make Msvm_ComputerSystem WQL queries locale agnostic
On Mon, Oct 5, 2020 at 9:13 PM Matt Coleman wrote: > > > On Oct 5, 2020, at 8:54 PM, Neal Gompa wrote: > > > > Should we require a bump to openwsman 2.6 for this? > > That won’t make any difference: openwsman 2.6+ supports specifying the > locale, but Windows ignores it. So, this patch changes the queries to > not use localized strings. > > -- > Matt > Ahh, okay then. Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH v2 00/10] hyperv: implement new APIs & more
On Mon, Oct 5, 2020 at 12:20 PM Matt Coleman wrote: > > These patches fix a couple bugs, consolidate duplicate code, and > implement several APIs. > > Currently, some interactions with Hyper-V systems fail when the system > is not configured for the "en-US" locale. Additionally, some CPU names > also contain the clock frequency, making it too long for > _virNodeInfo.model. The first two patches fix these bugs. > > The second two patches clean up the code a little: one moves repeated > operations into new helper functions; the other replaces the generic > "get WMI class list" functions with a macro. > > The next four patches implement the following APIs in the Hyper-V > driver: > * virConnectGetCapabilities() > * virConnectGetMaxVcpus() > * virConnectGetVersion() > * virDomainGetAutostart() > > Changes since v1: > * all NEWS updates are now in a single commit at the end > * long function calls have been split up into multiple lines > * improved error handling in hypervDomainLookupBy*(): they now throw > VIR_ERR_NO_DOMAIN when appropriate > * unnecessary calls to virReportError() have been removed from > hypervDomainGetInfo() and hypervDomainGetXMLDesc() > * HTML documentation for the unusual Hyper-V versions > * the WMI schema fix has been split into a separate commit > * hypervConnectGetVersion() uses a new hypervParseVersionString() and > directly produces the unsigned long instead of calling > virParseVersionString() > > Matt Coleman (10): > hyperv: make Msvm_ComputerSystem WQL queries locale agnostic > hyperv: fix nodeGetInfo failures caused by long CPU names > hyperv: break out common lookups into separate functions > hyperv: replace generic WMI class list helpers with a macro > hyperv: implement connectGetCapabilities > hyperv: implement connectGetMaxVcpus > hyperv: fix Win32_OperatingSystem WMI queries > hyperv: implement connectGetVersion > hyperv: implement domainGetAutostart > news: document new Hyper-V features and bug fixes > > NEWS.rst | 10 + > docs/drvhyperv.html.in| 50 ++ > src/hyperv/hyperv_driver.c| 751 ++ > src/hyperv/hyperv_private.h | 2 + > src/hyperv/hyperv_wmi.c | 83 +-- > src/hyperv/hyperv_wmi.h | 28 +- > src/hyperv/hyperv_wmi_classes.h | 4 +- > src/hyperv/hyperv_wmi_generator.input | 2 +- > 8 files changed, 596 insertions(+), 334 deletions(-) > > -- > 2.27.0 > > In general, the series looks good to me, though please take a look at my feedback for Patch 1. -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH v2 09/10] hyperv: implement domainGetAutostart
On Mon, Oct 5, 2020 at 12:21 PM Matt Coleman wrote:
>
> Co-authored-by: Sri Ramanujam
> Signed-off-by: Matt Coleman
> ---
> src/hyperv/hyperv_driver.c | 40 ++
> 1 file changed, 40 insertions(+)
>
> diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
> index 528c826e16..dcde469442 100644
> --- a/src/hyperv/hyperv_driver.c
> +++ b/src/hyperv/hyperv_driver.c
> @@ -1274,6 +1274,45 @@ hypervDomainCreate(virDomainPtr domain)
>
>
>
> +static int
> +hypervDomainGetAutostart(virDomainPtr domain, int *autostart)
> +{
> +int result = -1;
> +char uuid_string[VIR_UUID_STRING_BUFLEN];
> +hypervPrivate *priv = domain->conn->privateData;
> +g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER;
> +Msvm_VirtualSystemGlobalSettingData *vsgsd = NULL;
> +Msvm_VirtualSystemSettingData *vssd = NULL;
> +
> +virUUIDFormat(domain->uuid, uuid_string);
> +
> +if (priv->wmiVersion == HYPERV_WMI_VERSION_V1) {
> +virBufferEscapeSQL(,
> + MSVM_VIRTUALSYSTEMGLOBALSETTINGDATA_WQL_SELECT
> + "WHERE SystemName = \"%s\"", uuid_string);
> +
> +if (hypervGetWmiClass(Msvm_VirtualSystemGlobalSettingData, ) <
> 0)
> +goto cleanup;
> +
> +*autostart = vsgsd->data.common->AutomaticStartupAction == 2;
> +result = 0;
> +} else {
> +if (hypervGetVSSDFromUUID(priv, uuid_string, ) < 0)
> +goto cleanup;
> +
> +*autostart = vssd->data.v2->AutomaticStartupAction == 4;
> +result = 0;
> +}
> +
> +cleanup:
> +hypervFreeObject(priv, (hypervObject *) vsgsd);
> +hypervFreeObject(priv, (hypervObject *) vssd);
> +
> +return result;
> +}
> +
> +
> +
> static int
> hypervConnectIsEncrypted(virConnectPtr conn)
> {
> @@ -1824,6 +1863,7 @@ static virHypervisorDriver hypervHypervisorDriver = {
> .connectNumOfDefinedDomains = hypervConnectNumOfDefinedDomains, /* 0.9.5
> */
> .domainCreate = hypervDomainCreate, /* 0.9.5 */
> .domainCreateWithFlags = hypervDomainCreateWithFlags, /* 0.9.5 */
> +.domainGetAutostart = hypervDomainGetAutostart, /* 6.9.0 */
> .connectIsEncrypted = hypervConnectIsEncrypted, /* 0.9.5 */
> .connectIsSecure = hypervConnectIsSecure, /* 0.9.5 */
> .domainIsActive = hypervDomainIsActive, /* 0.9.5 */
> --
> 2.27.0
>
>
Reviewed-by: Neal Gompa
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH v2 10/10] news: document new Hyper-V features and bug fixes
On Mon, Oct 5, 2020 at 12:22 PM Matt Coleman wrote: > > Signed-off-by: Matt Coleman > --- > NEWS.rst | 10 ++ > 1 file changed, 10 insertions(+) > > diff --git a/NEWS.rst b/NEWS.rst > index b2ed661c8e..e708f06e9e 100644 > --- a/NEWS.rst > +++ b/NEWS.rst > @@ -19,10 +19,20 @@ v6.9.0 (unreleased) > local file-backed disks to configure a disk which discards changes made > to > it while the VM was active. > > + * hyperv: implement new APIs > + > +The ``virConnectGetCapabilities()``, ``virConnectGetMaxVcpus()``, > +``virConnectGetVersion()``, and ``virDomainGetAutostart()`` APIs have > been > +implemented in the Hyper-V driver. > + > * **Improvements** > > * **Bug fixes** > > + * hyperv: ensure WQL queries work in all locales > + > +Relying on the "Description" field caused queries to fail on non-"en-US" > +systems. The queries have been updated to avoid using localized strings. > > v6.8.0 (2020-10-01) > === > -- > 2.27.0 > > Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH v2 07/10] hyperv: fix Win32_OperatingSystem WMI queries
On Mon, Oct 5, 2020 at 12:21 PM Matt Coleman wrote: > > CurrentTimeZone's type is a signed integer, not unsigned. > > Signed-off-by: Matt Coleman > --- > src/hyperv/hyperv_wmi_generator.input | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/hyperv/hyperv_wmi_generator.input > b/src/hyperv/hyperv_wmi_generator.input > index 77543cf6ef..bbca550790 100644 > --- a/src/hyperv/hyperv_wmi_generator.input > +++ b/src/hyperv/hyperv_wmi_generator.input > @@ -673,7 +673,7 @@ class Win32_OperatingSystem > string CSCreationClassName > string CSDVersion > string CSName > -uint16 CurrentTimeZone > +int16CurrentTimeZone > boolean DataExecutionPrevention_Available > boolean DataExecutionPrevention_32BitApplications > boolean DataExecutionPrevention_Drivers > -- > 2.27.0 > > Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH v2 08/10] hyperv: implement connectGetVersion
> +
> +if (hypervParseVersionString(os->data.common->Version,
> + , , ) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Could not parse version from '%s'"),
> + os->data.common->Version);
> +goto cleanup;
> +}
> +
> +/*
> + * Pack the version into an unsigned long while retaining all the digits.
> + *
> + * Since Microsoft's build numbers are almost always over 1000, this
> driver
> + * needs to pack the value differently compared to the format defined by
> + * virConnectGetVersion().
> + *
> + * This results in `virsh version` producing unexpected output.
> + *
> + * For example...
> + * 2008: 6.0.6001 => 600.6.1
> + * 2008 R2: 6.1.7600 => 601.7.600
> + * 2012: 6.2.9200 => 602.9.200
> + * 2012 R2: 6.3.9600 => 603.9.600
> + * 2016: 10.0.14393 => 1000.14.393
> + * 2019: 10.0.17763 => 1000.17.763
> + */
> +if (major > 99 || minor > 99 || micro > 99) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Could not produce packed version number from
> '%s'"),
> + os->data.common->Version);
> +goto cleanup;
> +}
> +
> +*version = major * 1 + minor * 100 + micro;
> +
> +result = 0;
> +
> + cleanup:
> +hypervFreeObject(priv, (hypervObject *) os);
> +
> +return result;
> +}
> +
> +
> +
> static char *
> hypervConnectGetHostname(virConnectPtr conn)
> {
> @@ -1721,6 +1801,7 @@ static virHypervisorDriver hypervHypervisorDriver = {
> .connectOpen = hypervConnectOpen, /* 0.9.5 */
> .connectClose = hypervConnectClose, /* 0.9.5 */
> .connectGetType = hypervConnectGetType, /* 0.9.5 */
> +.connectGetVersion = hypervConnectGetVersion, /* 6.9.0 */
> .connectGetHostname = hypervConnectGetHostname, /* 0.9.5 */
> .connectGetMaxVcpus = hypervConnectGetMaxVcpus, /* 6.9.0 */
> .nodeGetInfo = hypervNodeGetInfo, /* 0.9.5 */
> --
> 2.27.0
>
>
Ugh, this is kind of gross, but it makes sense...
Reviewed-by: Neal Gompa
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH v2 06/10] hyperv: implement connectGetMaxVcpus
On Mon, Oct 5, 2020 at 12:21 PM Matt Coleman wrote:
>
> Co-authored-by: Sri Ramanujam
> Signed-off-by: Matt Coleman
> ---
> src/hyperv/hyperv_driver.c | 34 ++
> 1 file changed, 34 insertions(+)
>
> diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
> index 93e08c54c0..bbe892fd62 100644
> --- a/src/hyperv/hyperv_driver.c
> +++ b/src/hyperv/hyperv_driver.c
> @@ -553,6 +553,39 @@ hypervConnectGetCapabilities(virConnectPtr conn)
>
>
>
> +static int
> +hypervConnectGetMaxVcpus(virConnectPtr conn, const char *type G_GNUC_UNUSED)
> +{
> +int result = -1;
> +hypervPrivate *priv = conn->privateData;
> +g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER;
> +Msvm_ProcessorSettingData *processorSettingData = NULL;
> +
> +/* Get max processors definition */
> +virBufferAddLit(,
> +MSVM_PROCESSORSETTINGDATA_WQL_SELECT
> +"WHERE InstanceID LIKE 'Microsoft:Definition%Maximum'");
> +
> +if (hypervGetWmiClass(Msvm_ProcessorSettingData, )
> < 0)
> +goto cleanup;
> +
> +if (!processorSettingData) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Could not get maximum definition of
> Msvm_ProcessorSettingData for host %s"),
> + conn->uri->server);
> +goto cleanup;
> +}
> +
> +result = processorSettingData->data.common->VirtualQuantity;
> +
> + cleanup:
> +hypervFreeObject(priv, (hypervObject *) processorSettingData);
> +
> +return result;
> +}
> +
> +
> +
> static int
> hypervNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info)
> {
> @@ -1689,6 +1722,7 @@ static virHypervisorDriver hypervHypervisorDriver = {
> .connectClose = hypervConnectClose, /* 0.9.5 */
> .connectGetType = hypervConnectGetType, /* 0.9.5 */
> .connectGetHostname = hypervConnectGetHostname, /* 0.9.5 */
> +.connectGetMaxVcpus = hypervConnectGetMaxVcpus, /* 6.9.0 */
> .nodeGetInfo = hypervNodeGetInfo, /* 0.9.5 */
> .connectGetCapabilities = hypervConnectGetCapabilities, /* 6.9.0 */
> .connectListDomains = hypervConnectListDomains, /* 0.9.5 */
> --
> 2.27.0
>
>
Reviewed-by: Neal Gompa
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH v2 05/10] hyperv: implement connectGetCapabilities
On Mon, Oct 5, 2020 at 12:21 PM Matt Coleman wrote:
>
> Co-authored-by: Sri Ramanujam
> Signed-off-by: Matt Coleman
> ---
> src/hyperv/hyperv_driver.c | 90 +
> src/hyperv/hyperv_private.h | 2 +
> 2 files changed, 92 insertions(+)
>
> diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
> index 3e4563252e..93e08c54c0 100644
> --- a/src/hyperv/hyperv_driver.c
> +++ b/src/hyperv/hyperv_driver.c
> @@ -36,6 +36,7 @@
> #include "openwsman.h"
> #include "virstring.h"
> #include "virkeycode.h"
> +#include "domain_conf.h"
>
> #define VIR_FROM_THIS VIR_FROM_HYPERV
>
> @@ -283,6 +284,76 @@ hypervGetMemSDByVSSDInstanceId(hypervPrivate *priv,
> const char *id,
>
>
>
> +/*
> + * API-specific utility functions
> + */
> +
> +static int
> +hypervLookupHostSystemBiosUuid(hypervPrivate *priv, unsigned char *uuid)
> +{
> +Win32_ComputerSystemProduct *computerSystem = NULL;
> +g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER;
> +int result = -1;
> +
> +virBufferAddLit(, WIN32_COMPUTERSYSTEMPRODUCT_WQL_SELECT);
> +if (hypervGetWmiClass(Win32_ComputerSystemProduct, ) < 0)
> +goto cleanup;
> +
> +if (virUUIDParse(computerSystem->data.common->UUID, uuid) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Could not parse UUID from string '%s'"),
> + computerSystem->data.common->UUID);
> +goto cleanup;
> +}
> +result = 0;
> +
> + cleanup:
> +hypervFreeObject(priv, (hypervObject *) computerSystem);
> +
> +return result;
> +}
> +
> +static virCapsPtr
> +hypervCapsInit(hypervPrivate *priv)
> +{
> +virCapsPtr caps = NULL;
> +virCapsGuestPtr guest = NULL;
> +
> +caps = virCapabilitiesNew(VIR_ARCH_X86_64, 1, 1);
> +
> +if (!caps)
> +return NULL;
> +
> +if (hypervLookupHostSystemBiosUuid(priv, caps->host.host_uuid) < 0)
> +goto error;
> +
> +/* i686 caps */
> +guest = virCapabilitiesAddGuest(caps, VIR_DOMAIN_OSTYPE_HVM,
> VIR_ARCH_I686,
> +NULL, NULL, 0, NULL);
> +if (!guest)
> +goto error;
> +
> +if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_HYPERV, NULL,
> NULL, 0, NULL))
> +goto error;
> +
> +/* x86_64 caps */
> +guest = virCapabilitiesAddGuest(caps, VIR_DOMAIN_OSTYPE_HVM,
> VIR_ARCH_X86_64,
> +NULL, NULL, 0, NULL);
> +if (!guest)
> +goto error;
> +
> +if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_HYPERV, NULL,
> NULL, 0, NULL))
> +goto error;
> +
> +return caps;
> +
> + error:
> +virObjectUnref(caps);
> +return NULL;
> +}
> +
> +
> +
> /*
> * Driver functions
> */
> @@ -298,6 +369,9 @@ hypervFreePrivate(hypervPrivate **priv)
> wsmc_release((*priv)->client);
> }
>
> +if ((*priv)->caps)
> +virObjectUnref((*priv)->caps);
> +
> hypervFreeParsedUri(&(*priv)->parsedUri);
> VIR_FREE(*priv);
> }
> @@ -408,6 +482,11 @@ hypervConnectOpen(virConnectPtr conn, virConnectAuthPtr
> auth,
> if (hypervInitConnection(conn, priv, username, password) < 0)
> goto cleanup;
>
> +/* set up capabilities */
> +priv->caps = hypervCapsInit(priv);
> +if (!priv->caps)
> +goto cleanup;
> +
> conn->privateData = priv;
> priv = NULL;
> result = VIR_DRV_OPEN_SUCCESS;
> @@ -464,6 +543,16 @@ hypervConnectGetHostname(virConnectPtr conn)
>
>
>
> +static char*
> +hypervConnectGetCapabilities(virConnectPtr conn)
> +{
> +hypervPrivate *priv = conn->privateData;
> +
> +return virCapabilitiesFormatXML(priv->caps);
> +}
> +
> +
> +
> static int
> hypervNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info)
> {
> @@ -1601,6 +1690,7 @@ static virHypervisorDriver hypervHypervisorDriver = {
> .connectGetType = hypervConnectGetType, /* 0.9.5 */
> .connectGetHostname = hypervConnectGetHostname, /* 0.9.5 */
> .nodeGetInfo = hypervNodeGetInfo, /* 0.9.5 */
> +.connectGetCapabilities = hypervConnectGetCapabilities, /* 6.9.0 */
> .connectListDomains = hypervConnectListDomains, /* 0.9.5 */
> .connectNumOfDomains = hypervConnectNumOfDomains, /* 0.9.5 */
> .connectListAllDomains = hypervConnectListAllDomains, /* 0.10.2 */
> diff --git a/src/hyperv/hyperv_private.h b/src/hyperv/hyperv_private.h
> index b502e71d83..cf08bf542b 100644
> --- a/src/hyperv/hyperv_private.h
> +++ b/src/hyperv/hyperv_private.h
> @@ -26,6 +26,7 @@
> #include "virerror.h"
> #include "hyperv_util.h"
> #include "openwsman.h"
> +#include "capabilities.h"
>
> typedef enum _hypervWmiVersion hypervWmiVersion;
> enum _hypervWmiVersion {
> @@ -38,4 +39,5 @@ struct _hypervPrivate {
> hypervParsedUri *parsedUri;
> WsManClient *client;
> hypervWmiVersion wmiVersion;
> +virCapsPtr caps;
> };
> --
> 2.27.0
>
>
Reviewed-by: Neal Gompa
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH v2 04/10] hyperv: replace generic WMI class list helpers with a macro
rivate *priv, virBufferPtr query,
> - Win32_ComputerSystem **list)
> -{
> -return hypervGetWmiClassList(priv, Win32_ComputerSystem_WmiInfo, query,
> - (hypervObject **)list);
> -}
> -
> -int
> -hypervGetWin32ProcessorList(hypervPrivate *priv, virBufferPtr query,
> -Win32_Processor **list)
> -{
> -return hypervGetWmiClassList(priv, Win32_Processor_WmiInfo, query,
> - (hypervObject **)list);
> -}
> -
> -int
> -hypervGetMsvmVirtualSystemSettingDataList(hypervPrivate *priv,
> - virBufferPtr query,
> - Msvm_VirtualSystemSettingData
> **list)
> -{
> -return hypervGetWmiClassList(priv,
> Msvm_VirtualSystemSettingData_WmiInfo, query,
> - (hypervObject **)list);
> -}
> -
> -int
> -hypervGetMsvmProcessorSettingDataList(hypervPrivate *priv,
> - virBufferPtr query,
> - Msvm_ProcessorSettingData **list)
> -{
> -return hypervGetWmiClassList(priv, Msvm_ProcessorSettingData_WmiInfo,
> query,
> - (hypervObject **)list);
> -}
> -
> -int
> -hypervGetMsvmMemorySettingDataList(hypervPrivate *priv, virBufferPtr query,
> - Msvm_MemorySettingData **list)
> -{
> -return hypervGetWmiClassList(priv, Msvm_MemorySettingData_WmiInfo, query,
> - (hypervObject **)list);
> -}
> -
> -int hypervGetMsvmKeyboardList(hypervPrivate *priv, virBufferPtr query,
> - Msvm_Keyboard **list)
> -{
> -return hypervGetWmiClassList(priv, Msvm_Keyboard_WmiInfo, query,
> - (hypervObject **)list);
> -}
> -
> -
> -
> /* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
> *
> * Msvm_ComputerSystem
> */
> @@ -1371,7 +1300,8 @@
> hypervInvokeMsvmComputerSystemRequestStateChange(virDomainPtr domain,
> virBufferAddLit(, MSVM_CONCRETEJOB_WQL_SELECT);
> virBufferAsprintf(, "where InstanceID = \"%s\"",
> instanceID);
>
> -if (hypervGetMsvmConcreteJobList(priv, , ) < 0)
> +if (hypervGetWmiClassList(priv, Msvm_ConcreteJob_WmiInfo, ,
> + (hypervObject **)) < 0)
> goto cleanup;
>
> if (concreteJob == NULL) {
> @@ -1560,7 +1490,8 @@ hypervMsvmComputerSystemFromDomain(virDomainPtr domain,
> virBufferAddLit(, MSVM_COMPUTERSYSTEM_WQL_VIRTUAL);
> virBufferAsprintf(, "and Name = \"%s\"", uuid_string);
>
> -if (hypervGetMsvmComputerSystemList(priv, , computerSystem) < 0)
> +if (hypervGetWmiClassList(priv, Msvm_ComputerSystem_WmiInfo, ,
> + (hypervObject **)computerSystem) < 0)
> return -1;
>
> if (*computerSystem == NULL) {
> diff --git a/src/hyperv/hyperv_wmi.h b/src/hyperv/hyperv_wmi.h
> index 74a74e0e09..8c9c5ed9c1 100644
> --- a/src/hyperv/hyperv_wmi.h
> +++ b/src/hyperv/hyperv_wmi.h
> @@ -198,40 +198,18 @@ const char *hypervReturnCodeToString(int returnCode);
> * Generic "Get WMI class list" helpers
> */
>
> -int hypervGetMsvmComputerSystemList(hypervPrivate *priv, virBufferPtr query,
> -Msvm_ComputerSystem **list);
> -
> -int hypervGetMsvmConcreteJobList(hypervPrivate *priv, virBufferPtr query,
> - Msvm_ConcreteJob **list);
> -
> -int hypervGetWin32ComputerSystemList(hypervPrivate *priv, virBufferPtr query,
> - Win32_ComputerSystem **list);
> -
> -int hypervGetWin32ProcessorList(hypervPrivate *priv, virBufferPtr query,
> -Win32_Processor **list);
> -
> -int hypervGetMsvmVirtualSystemSettingDataList(hypervPrivate *priv,
> - virBufferPtr query,
> - Msvm_VirtualSystemSettingData
> **list);
> +int hypervGetWmiClassList(hypervPrivate *priv,
> + hypervWmiClassInfoListPtr wmiInfo, virBufferPtr
> query,
> + hypervObject **wmiClass);
>
> int hypervGetMsvmVirtualSystemSettingDataFromUUID(hypervPrivate *priv,
>const char *uuid_string,
>
> Msvm_VirtualSystemSettingData **list);
>
> -int hypervGetMsvmProcessorSettingDataList(hypervPrivate *priv,
> - virBufferPtr query,
> - Msvm_ProcessorSettingData **list);
> -
> -int hypervGetMsvmMemorySettingDataList(hypervPrivate *priv, virBufferPtr
> query,
> - Msvm_MemorySettingData **list);
> -
> int hypervGetMsvmMemorySettingDataFromVSSD(hypervPrivate *priv,
> const char *vssd_instanceid,
> Msvm_MemorySettingData **list);
>
> -int hypervGetMsvmKeyboardList(hypervPrivate *priv, virBufferPtr query,
> - Msvm_Keyboard **list);
> -
> /* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
> *
> * Msvm_ComputerSystem
> */
> --
> 2.27.0
>
>
Reviewed-by: Neal Gompa
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH v2 03/10] hyperv: break out common lookups into separate functions
emSettingData.InstanceID=\"%s\"} "
> - "where AssocClass =
> Msvm_VirtualSystemSettingDataComponent "
> - "ResultClass = Msvm_ProcessorSettingData",
> - virtualSystemSettingData->data.common->InstanceID);
> -
> -if (hypervGetMsvmProcessorSettingDataList(priv, ,
> - ) < 0) {
> +if (hypervGetVSSDFromUUID(priv, uuid_string,
> + ) < 0) {
> goto cleanup;
> }
>
> -if (processorSettingData == NULL) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Could not lookup %s for domain %s"),
> - "Msvm_ProcessorSettingData",
> - computerSystem->data.common->ElementName);
> +if (hypervGetProcSDByVSSDInstanceId(priv,
> + virtualSystemSettingData->data.common->InstanceID,
> + ) < 0) {
> goto cleanup;
> }
>
> -/* Get Msvm_MemorySettingData */
> -virBufferEscapeSQL(,
> - "associators of "
> - "{Msvm_VirtualSystemSettingData.InstanceID=\"%s\"} "
> - "where AssocClass =
> Msvm_VirtualSystemSettingDataComponent "
> - "ResultClass = Msvm_MemorySettingData",
> - virtualSystemSettingData->data.common->InstanceID);
> -
> -if (hypervGetMsvmMemorySettingDataList(priv, ,
> - ) < 0) {
> -goto cleanup;
> -}
> -
> -
> -if (memorySettingData == NULL) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Could not lookup %s for domain %s"),
> - "Msvm_MemorySettingData",
> - computerSystem->data.common->ElementName);
> +if (hypervGetMemSDByVSSDInstanceId(priv,
> + virtualSystemSettingData->data.common->InstanceID,
> + ) < 0) {
> goto cleanup;
> }
>
> @@ -909,7 +956,6 @@ hypervConnectListDefinedDomains(virConnectPtr conn, char
> **const names, int maxn
> {
> bool success = false;
> hypervPrivate *priv = conn->privateData;
> -g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER;
> Msvm_ComputerSystem *computerSystemList = NULL;
> Msvm_ComputerSystem *computerSystem = NULL;
> int count = 0;
> @@ -918,16 +964,8 @@ hypervConnectListDefinedDomains(virConnectPtr conn, char
> **const names, int maxn
> if (maxnames == 0)
> return 0;
>
> -virBufferAddLit(, MSVM_COMPUTERSYSTEM_WQL_SELECT);
> -virBufferAddLit(, "where ");
> -virBufferAddLit(, MSVM_COMPUTERSYSTEM_WQL_VIRTUAL);
> -virBufferAddLit(, "and ");
> -virBufferAddLit(, MSVM_COMPUTERSYSTEM_WQL_INACTIVE);
> -
> -if (hypervGetMsvmComputerSystemList(priv, ,
> -) < 0) {
> +if (hypervGetInactiveVirtualSystemList(priv, ) < 0)
> goto cleanup;
> -}
>
> for (computerSystem = computerSystemList; computerSystem != NULL;
> computerSystem = computerSystem->next) {
> @@ -961,21 +999,12 @@ hypervConnectNumOfDefinedDomains(virConnectPtr conn)
> {
> bool success = false;
> hypervPrivate *priv = conn->privateData;
> -g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER;
> Msvm_ComputerSystem *computerSystemList = NULL;
> Msvm_ComputerSystem *computerSystem = NULL;
> int count = 0;
>
> -virBufferAddLit(, MSVM_COMPUTERSYSTEM_WQL_SELECT);
> -virBufferAddLit(, "where ");
> -virBufferAddLit(, MSVM_COMPUTERSYSTEM_WQL_VIRTUAL);
> -virBufferAddLit(, "and ");
> -virBufferAddLit(, MSVM_COMPUTERSYSTEM_WQL_INACTIVE);
> -
> -if (hypervGetMsvmComputerSystemList(priv, ,
> -) < 0) {
> +if (hypervGetInactiveVirtualSystemList(priv, ) < 0)
> goto cleanup;
> -}
>
> for (computerSystem = computerSystemList; computerSystem != NULL;
> computerSystem = computerSystem->next) {
> --
> 2.27.0
>
>
Reviewed-by: Neal Gompa
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH v2 02/10] hyperv: fix nodeGetInfo failures caused by long CPU names
On Mon, Oct 5, 2020 at 12:21 PM Matt Coleman wrote:
>
> Some CPU model names were too long for _virNodeInfo.model.
> For example: Intel Xeon CPU E5-2620 v2 @ 2.10GHz
> This commit removes the clock frequency suffix.
>
> Signed-off-by: Matt Coleman
> ---
> src/hyperv/hyperv_driver.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
> index b57325f2a5..9bbc2f67fb 100644
> --- a/src/hyperv/hyperv_driver.c
> +++ b/src/hyperv/hyperv_driver.c
> @@ -285,6 +285,10 @@ hypervNodeGetInfo(virConnectPtr conn, virNodeInfoPtr
> info)
> } else if (STRPREFIX(tmp, "(TM)")) {
> memmove(tmp, tmp + 4, strlen(tmp + 4) + 1);
> continue;
> +} else if (STRPREFIX(tmp, " @ ")) {
> +/* Remove " @ X.YZGHz" from the end. */
> +*tmp = '\0';
> +break;
> }
>
> ++tmp;
> --
> 2.27.0
>
>
Reviewed-by: Neal Gompa
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH v2 01/10] hyperv: make Msvm_ComputerSystem WQL queries locale agnostic
On Mon, Oct 5, 2020 at 12:21 PM Matt Coleman wrote: > > There are two specific WQL queries we're using to get either a list of > virtual machines or the hypervisor host itself from Msvm_ComputerSystem. > Those queries rely on filtering results based on the "Description" > field. Since the "Description" field is locale sensitive, the queries > will fail if the Windows host is using a language pack. While the WSMAN > spec allows the client to set the requested locale (and it is supported > since openwsman 2.6.x), the Windows WinRM service does not respect this > setting: it returns non-English strings despite the WSMAN request > properly setting the locale to 'en-US'. Therefore, this patch changes > the WQL query to make use of the "__SERVER" field to stop relying on > English strings in queries and side step the issue. > > Co-authored-by: Dawid Zamirski > Signed-off-by: Matt Coleman > --- > src/hyperv/hyperv_wmi_classes.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/hyperv/hyperv_wmi_classes.h b/src/hyperv/hyperv_wmi_classes.h > index a19b6a656d..7465684d6e 100644 > --- a/src/hyperv/hyperv_wmi_classes.h > +++ b/src/hyperv/hyperv_wmi_classes.h > @@ -44,10 +44,10 @@ > */ > > #define MSVM_COMPUTERSYSTEM_WQL_VIRTUAL \ > -"Description = \"Microsoft Virtual Machine\" " > +"Name != __SERVER " > > #define MSVM_COMPUTERSYSTEM_WQL_PHYSICAL \ > -"Description = \"Microsoft Hosting Computer System\" " > +"Name = __SERVER " > > #define MSVM_COMPUTERSYSTEM_WQL_ACTIVE \ > "(EnabledState != 0 and EnabledState != 3 and EnabledState != 32769) " > -- > 2.27.0 > > Should we require a bump to openwsman 2.6 for this? -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCH 7/9] spec: Drop s390 architecture from conditionals
On Mon, Oct 5, 2020 at 2:41 PM Andrea Bolognani wrote:
>
> Neither Fedora nor RHEL build packages on this architecture.
>
> Signed-off-by: Andrea Bolognani
> ---
> libvirt.spec.in | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index d8f689e651..e036307d30 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -109,12 +109,12 @@
> %endif
>
> # Numactl is not available on many non-x86 archs
> -%ifarch s390 s390x %{arm} riscv64
> +%ifarch s390x %{arm} riscv64
> %define with_numactl 0
> %endif
>
> # zfs-fuse is not available on some architectures
> -%ifarch s390 s390x aarch64 riscv64
> +%ifarch s390x aarch64 riscv64
> %define with_storage_zfs 0
> %endif
>
> @@ -179,7 +179,7 @@
> %if %{with_qemu} || %{with_lxc}
> # numad is used to manage the CPU and memory placement dynamically,
> # it's not available on many non-x86 architectures.
> -%ifnarch s390 s390x %{arm} riscv64
> +%ifnarch s390x %{arm} riscv64
> %define with_numad0%{!?_without_numad:1}
> %endif
> %endif
> --
> 2.26.2
>
Good riddance to bad architectures...
Reviewed-by: Neal Gompa
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCH 8/9] spec: Refactor qemu_kvm_arches definition
On Mon, Oct 5, 2020 at 2:41 PM Andrea Bolognani wrote:
>
> There's no need to set a default for it if we're going to override
> it immediately afterwards anyway, and setting with_qemu_tcg at the
> same time only makes things more confusing.
>
> Signed-off-by: Andrea Bolognani
> ---
> libvirt.spec.in | 26 +-
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index e036307d30..b62b17ee80 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -17,31 +17,31 @@
> %define _vpath_builddir %{_target_platform}
> %endif
>
> +%if 0%{?fedora}
> +%define qemu_kvm_arches %{ix86} x86_64 %{power64} s390x %{arm}
> aarch64
> +%else
> +%define qemu_kvm_arches x86_64 %{power64} aarch64 s390x
> +%endif
> +
> # The hypervisor drivers that run in libvirtd
> %define with_qemu 0%{!?_without_qemu:1}
> %define with_lxc 0%{!?_without_lxc:1}
> %define with_libxl 0%{!?_without_libxl:1}
> %define with_vbox 0%{!?_without_vbox:1}
>
> -%define with_qemu_tcg %{with_qemu}
> -
> -%define qemu_kvm_arches %{ix86} x86_64
> -
> -%if 0%{?fedora}
> -%define qemu_kvm_arches %{ix86} x86_64 %{power64} s390x %{arm} aarch64
> -%endif
> -
> -%if 0%{?rhel}
> -%define with_qemu_tcg 0
> -%define qemu_kvm_arches x86_64 %{power64} aarch64 s390x
> -%endif
> -
> %ifarch %{qemu_kvm_arches}
> %define with_qemu_kvm %{with_qemu}
> %else
> %define with_qemu_kvm 0
> %endif
>
> +%define with_qemu_tcg %{with_qemu}
> +
> +# RHEL disables TCG on all architectures
> +%if 0%{?rhel}
> + %define with_qemu_tcg 0
> +%endif
> +
> %if ! %{with_qemu_tcg} && ! %{with_qemu_kvm}
> %define with_qemu 0
> %endif
> --
> 2.26.2
>
Reviewed-by: Neal Gompa
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCH 6/9] spec: Move _vpath_builddir definition
On Mon, Oct 5, 2020 at 2:41 PM Andrea Bolognani wrote:
>
> It belongs before package-specific feature flags are defined.
>
> Signed-off-by: Andrea Bolognani
> ---
> libvirt.spec.in | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index 2401404008..d8f689e651 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -12,6 +12,11 @@
> %define supported_platform 0
> %endif
>
> +# On RHEL 7 and older macro _vpath_builddir is not defined.
> +%if 0%{?rhel} && 0%{?rhel} <= 7
> +%define _vpath_builddir %{_target_platform}
> +%endif
> +
> # The hypervisor drivers that run in libvirtd
> %define with_qemu 0%{!?_without_qemu:1}
> %define with_lxc 0%{!?_without_lxc:1}
> @@ -31,11 +36,6 @@
> %define qemu_kvm_arches x86_64 %{power64} aarch64 s390x
> %endif
>
> -# On RHEL 7 and older macro _vpath_builddir is not defined.
> -%if 0%{?rhel} && 0%{?rhel} <= 7
> -%define _vpath_builddir %{_target_platform}
> -%endif
> -
> %ifarch %{qemu_kvm_arches}
> %define with_qemu_kvm %{with_qemu}
> %else
> --
> 2.26.2
>
Do we still need this anymore? Meson in EPEL 7 works without this definition...
But otherwise...
Reviewed-by: Neal Gompa
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCH 5/9] spec: Introduce with_dmidecode
On Mon, Oct 5, 2020 at 2:41 PM Andrea Bolognani wrote:
>
> To keep things maintainable, we want to have architecture handling
> all in one spot instead of sprinkling %ifarch conditionals all over
> the place.
>
> Signed-off-by: Andrea Bolognani
> ---
> libvirt.spec.in | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index 65df7dc79f..2401404008 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -94,6 +94,7 @@
> %define with_libssh2 0
> %define with_wireshark0
> %define with_libssh 0
> +%define with_dmidecode0
>
> # Finally set the OS / architecture specific special cases
>
> @@ -183,6 +184,10 @@
> %endif
> %endif
>
> +%ifarch %{ix86} x86_64
> +%define with_dmidecode 0%{!?_without_dmidecode:1}
> +%endif
> +
> # Force QEMU to run as non-root
> %define qemu_user qemu
> %define qemu_group qemu
> @@ -434,7 +439,7 @@ Requires: iproute-tc
> %endif
>
> Requires: polkit >= 0.112
> -%ifarch %{ix86} x86_64
> +%if %{with_dmidecode}
> # For virConnectGetSysinfo
> Requires: dmidecode
> %endif
> --
> 2.26.2
>
Reviewed-by: Neal Gompa
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [libvirt PATCH 4/9] spec: Move with_numactl definition
On Mon, Oct 5, 2020 at 2:40 PM Andrea Bolognani wrote:
>
> Keep it close to similar ones.
>
> Signed-off-by: Andrea Bolognani
> ---
> libvirt.spec.in | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index 9e4c5d2b81..65df7dc79f 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -68,8 +68,6 @@
> %endif
> %endif
>
> -%define with_numactl 0%{!?_without_numactl:1}
> -
> # F25+ has zfs-fuse
> %if 0%{?fedora}
> %define with_storage_zfs 0%{!?_without_storage_zfs:1}
> @@ -86,6 +84,7 @@
>
> # Other optional features
> %define with_bash_completion 0%{!?_without_bash_completion:1}
> +%define with_numactl 0%{!?_without_numactl:1}
>
> # A few optional bits off by default, we enable later
> %define with_fuse 0
> --
> 2.26.2
>
Reviewed-by: Neal Gompa
--
真実はいつも一つ！/ Always, there's only one truth!
