Re: TCK regression from blockdev change
On Mon, Mar 09, 2020 at 15:11:22 -0600, Jim Fehlig wrote: > Hi all, > > I noticed libvirt-tck test 207-disk-media-change.t is failing with 6.1.0, > although I _think_ the failure has actually been around since the change to > using blockdev in the qemu driver. The test essentially creates a minimal > domain with a cdrom disk device, then calls attach_device a few times, > changing the of the disk. The last attach_device uses the same > as when the domain was created and expects the initial and final > XML to be the same. For reference, here's a link to the test > > https://libvirt.org/git/?p=libvirt-tck.git;a=blob;f=scripts/domain/207-disk-media-change.t;h=dfef7c95c9d062e0f20dd2ed53d4dfb12cca0ab3;hb=HEAD > > However the test fails since the 'index' attribute of the element > is not the same between initial XML (index='1') and final XML (index='5'). > Indeed with each attach operation the index is incremented. Should libvirt > be fixed to always provide an index of 1 for this disk config? Or should we > adjust the test case? I'm not so familiar with this functionality but I lean With -blockdev the 'index' will be always unique. The idea is that it always points to the same image so if you unplug it and plug it back even at that point it will be incremented. This was specifically meant as a feature because during block jobs it prevents accidentally refering to the wrong part of the backing chain. In the above case it doesn't seem that the test is that useful, and it certainly should ignore the index changing.
Re: RFC: qemu: use uuid instead of name for misc filenames
On 06.03.2020 21:20, Daniel P. Berrangé wrote: > On Fri, Feb 28, 2020 at 10:09:41AM +0300, Nikolay Shirokovskiy wrote: >> On 27.02.2020 16:48, Daniel P. Berrangé wrote: >>> On Thu, Feb 27, 2020 at 03:57:04PM +0300, Nikolay Shirokovskiy wrote: Hi, everyone. I'm working on supporting domain renaming when it has snapshots which is not supported now. And it strikes me things will be much simplier to manage on renaming if we use uuid in filenames instead of domain names. 1. Renaming will only involve saving updated config. The saving is atomic thanx to tmp file and rename(2) approach. In constast current renaming on error paths can leave config with old or new name. Thus on libvirt restart extra VM will appear. And we don't need to rename autostart links, snapshot directories etc. >>> >>> Yes, renaming is hard due to the non-atomic nature of the problem. >>> It ought to be possible to recover/rollback from all but the most >>> serious failure scenarios though. Problems in the most serious >>> scenarios, are likely to cause failures in other parts of libvirt >>> already, such as existing VM shutdown/startup. >>> >>> 2. Renaming will be possible for running domains with no efforts. We only need to pass uuid instead of name in '-name guest=...' command line. >>> >>> Passing a uuid for the -name arg would be incorrect. This is a user >>> visible string, for example displayed in the SDL window title or >>> the VNC display name. UUIDs are not desirable for exposure to users, >>> they are for machine usage. >> >> Yes, this is not convinient. The broader idea was to stop passing name on >> command >> line as we can't change it after process is started. So instead we can omit >> name >> parameter altogether and set/update name thru QMP. > > I see no problem in continuing to use -name at startup, but certainly if we > wish to propagate renames, then we'll need a QMP command for that. I use quite regularly 'ps ax | grep $NAME' to check if process is running when debugging so having domain name in command line can be be confusing. > >> Also with domain name in path renaming of an active domain looks really messy >> to implement. > > Yep, this goes to the disk image too, as its normal practice to name the > root disk image, based on the VM name. Admins are not going to like > looking in /var/lib/libvirt/images/ and seeing 100's of UUIDs. > > QEMU can be told to re-open disk images, via the blockjob QMP commands, > which could allow us to rename disk images on running VMs. I wonder > if a simple "rename()" works on UNIX sockets - I can't say I've ever > tried it, but I think it probably would work. Yeah, renaming should work I guess. But together with renaming disk images it is lot of operations thus in case of errors we can end up with some paths renamed and some not. > 3. Mgmt can stop using autogenerated names for domains. I guess openstack for example uses names like instance-02ff because we have many limitations on domain renaming. And if these limitations are removed then openstack can just use user supplied names for domains. >>> >>> In openstack the instance names are unique within the scope of a single >>> project. >>> >>> In Libvirt the VM names are unique within the scope of a single libvirt >>> driver connection (effectively a single host). >>> >>> Even if libvirt didn't use the name for on disk files, it will still have >>> the requirement for unique names per-host in libvirt. >>> >>> This is the key reason why openstack uses "instance-$HEX" as the libvirt >>> guest name. >>> >>> It could perhaps use "projectname-guestname" as the name, but i'm not >>> sure there's much appetite for change in this respect, as it is a long >>> standing convention now. >>> 4. No issues with long domain names and filename length limit If the above conversion makes sense I guess the good time to apply it is on domain start (and rename to support renaming with snapshots). >>> >>> The above has not considered the benefit that using the VM name >>> has. Essentially the UUID is good for machines, the VM name is >>> good for humans. Seeing the guest XML files, or VM log files >>> using a filename based on UUID instead of name is a *really* >>> unappealing idea to me. >> >> I agree. But we can also keep symlinks with domain names for configs/logs etc >> This can be don
Re: [PATCH] cpu_map: Add more -noTSX x86 CPU models
> ... > > Running make check would reveal all these issues because every single > test which involves parsing the cpu_map was failing due to multiple > definitions of the same CPU model. > > I'd not have expected that the tests will exercise the new XMLs in any way. Good to know and thanks for your feedback. > And since this patch is adding several CPU models which are already > supported by QEMU since 4.2.0, you need to update several existing test > files for domaincapstest too. You can use > > VIR_TEST_REGENERATE_OUTPUT=1 tests/domaincapstest > > to regenerate the files. Just make sure you review the changes before > adding them to this commit. > Without regenerating these I see as expected FAIL: domaincapstest FAIL: cputest Adding these 5 types to the qemu 4.2 and qemu 5.0 tests/domaincapsdata worked. I've squashed that into the same patch - let me know if you'd prefer the domaincapsdata change as an individual patch instead. Regenerating the test files will also be needed for cputest because I > just pushed the "cputest: Add data for Intel(R) Core(TM) i7-8550U CPU > without TSX". Doing so will nicely show that the computed host CPU model > (in x86_64-cpuid-Core-i7-8550U-host.xml file) is > Skylake-Client-noTSX-IBRS rather than Broadwell-noTSX-IBRS. > Indeed: 1007 In '/home/paelzer/work/libvirt/libvirt-ubuntu-git/build/../tests/cputestdata/x86_64-cpuid-Core-i7-8550U-host.xml': ... 1009 Expect [Broadwell-noTSX-IBRS ... 1043 Actual [Skylake-Client-noTSX-IBRS > However, the CPU used for host-model (and reported in domain > capabilities) as shown in x86_64-cpuid-Core-i7-8550U-guest.xml and > x86_64-cpuid-Core-i7-8550U-json.xml will change from Skylake-Client-IBRS > to Skylake-Client-noTSX-IBRS. As I said in my previous reply to this > patch, I think these two CPU definitions should keep using the old > Skylake-Client-IBRS model to make sure any domain with host-model CPU > will always use the CPU models without noTSX for better compatibility > between current and future version of libvirt. I see three changes: x86_64-cpuid-Core-i7-8550U-host.xml: Broadwell-noTSX-IBRS -> Skylake-Client-noTSX-IBRS x86_64-cpuid-Core-i7-8550U-guest.xml: Skylake-Client-IBRS -> Skylake-Client-noTSX-IBRS This shows up twice in: 238) cpuTestGuestCPUID(x86_64): Core-i7-8550U 240) cpuTestGuestCPUID(x86_64): Core-i7-8550U x86_64-cpuid-Core-i7-8550U-json.xml: Skylake-Client-IBRS -> Skylake-Client-noTSX-IBRS So far so good and as expected. But I have to beg your pardon and need to ask where such an override to continue to use "Skylake-Client-IBRS + policy='disable' name='hle' policy='disable' name='rtm'" instead of just "Skylake-Client-noTSX-IBRS" would have to go? Holding back v2 submission until this is solved ... This change should be in > a separate patch, but in single series with the current patch. > > Jirka > > -- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
Re: [PATCH v3 4/4] qemu-img: Deprecate use of -b without -F
On Mon, Mar 09, 2020 at 10:42:25AM -0500, Eric Blake wrote: > On 3/9/20 10:31 AM, Kashyap Chamarthy wrote: > > > After (with the patch series applied to QEMU Git): > > > > $> git describe > > v4.2.0-2204-gd6c7830114 > > > > # Create; *without* specifying "-F raw" > > $> ~/build/qemu/qemu-img create -f qcow2 -b ./base.raw ./overlay2.qcow2 > > qemu-img: warning: Deprecated use of backing file without explicit > > backing format (detected format of raw) > > Formatting './overlay2.qcow2', fmt=qcow2 size=4294967296 > > backing_file=./base.raw backing_fmt=raw cluster_size=65536 > > lazy_refcounts=off refcount_bits=16 > > If you'll note, this case _did_ write an implied backing_fmt=raw into the > image. Ah, I missed to notice that. Interesting. > Constrast that with creating an image on a qcow2 backing file, which > tells you it detected a format of qcow2, but does NOT write > backing_fmt=qcow2 into the image (this was a change from v2, at Peter's > request). Indeed, here we go, confirming the overlay of a QCOW2 backing file _not_ having the 'backing_fmt' written into the image: $> ~/build/qemu/qemu-img create -f qcow2 -b ./another_base.qcow2 ./overlay1_of_ab.qcow2 qemu-img: warning: Deprecated use of backing file without explicit backing format (detected format of qcow2) Formatting './overlay1_of_ab.qcow2', fmt=qcow2 size=4294967296 backing_file=./another_base.qcow2 cluster_size=65536 lazy_refcounts=off refcount_bits=16 That's neat. (I wonder if this bit should also be documented.) > Thus, when the backing is raw, we warn but future use of the > image is now safe where it previously was not; when the backing file is > non-raw, we warn but do not change our behavior, but because the backing > file is non-raw any future probes will not be any less safe than before. Understood. Easy to miss when not paying attention; thanks for pointing it out. [...] > > However, for the "Convert" case, is it correct that no warning is thrown > > for the below? > > > > $> ~/build/qemu/qemu-img info overlay1.qcow2 > > image: overlay1.qcow2 > > file format: qcow2 > > virtual size: 4 GiB (4294967296 bytes) > > disk size: 196 KiB > > cluster_size: 65536 > > backing file: base.raw > > Format specific information: > > compat: 1.1 > > lazy refcounts: false > > refcount bits: 16 > > corrupt: false > > We have an image with no backing format, so we had to probe. This patch > series did not change the behavior of opening an existing image, only for > creating a new image (or amending an image in-place). So the lack of a > warning on opening the unsafe image may be desirable, but it would be via > even more patches. Fair enough; it's an understandable compromise. And your series is a strict improvement as-is; it should not be held up. > > > $> ~/build/qemu/qemu-img convert -f qcow2 -O qcow2 overlay1.qcow2 > > flattened.raw > > Ouch - you are creating a qcow2 destination file named 'flattened.raw', > which is rather confusing on your part. Oops, yes it is; bad me. Sorry for the mix-up. I meant to amend the format to 'raw'. > However, as your destination file is being created without a backing image, > it is to be expected that there is no warning (when there is no backing > file, -F makes no sense). Yeah, fair enough. > To provoke the warning during convert, you'll > have to also pass -B (or -o backing_file), without -o backing_fmt (since > convert lacks the -F shorthand). Hmm, I tried the following way, but it doesn't provoke the warning: $> ~/build/qemu/qemu-img convert -B ./base.raw -O qcow2 overlay1.qcow2 flattened.qcow2 $> ~/build/qemu/qemu-img info flattened.qcow2 image: flattened.qcow2 file format: qcow2 virtual size: 4 GiB (4294967296 bytes) disk size: 196 KiB cluster_size: 65536 backing file: ./base.raw Format specific information: compat: 1.1 lazy refcounts: false refcount bits: 16 corrupt: false What am I missing? - - - Ah, didn't realize the inconsistency of 'convert' lacking the '-F' shorthand ... which reminds me, there are at least _three_ ways that I know of, to specify backing file format with 'create': $ qemu-img create -f qcow2 -o 'backing_file=./base.raw,backing_fmt=raw' ./overlay1.qcow2 $ qemu-img create -f qcow2 -b ./base.raw -o backing_fmt=raw overlay1.qcow2 $ qemu-img create -f qcow2 -b ./base.raw -F raw ./overlay1.qcow2 I'm wondering about the consistency of having all the above three supported for other operations too. Now I at least know 'convert' lacks the "-F". Not sure how much people care about this :-) [...] -- /kashyap
[libvirt PATCH 2/5] gitlab: reduce number of cross-build CI jobs
We're going to add more build jobs to CI, and users have limited time granted on the shared CI runners. The number of cross-build jobs currently present is not sustainable, so cut it down to two interesting jobs to cover big endian and 32-bit platform variants. Signed-off-by: Daniel P. Berrangé --- .gitlab-ci.yml | 37 ++--- 1 file changed, 6 insertions(+), 31 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 6f7e0ce135..b6a8db7881 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -5,29 +5,12 @@ - ../autogen.sh $CONFIGURE_OPTS || (cat config.log && exit 1) - make -j $(getconf _NPROCESSORS_ONLN) -# We could run every arch on every versions, but it is a little -# overkill. Instead we split jobs evenly across 9, 10 and sid -# to achieve reasonable cross-coverage. - -debian-9-cross-armv6l: - <<: *job_definition - image: quay.io/libvirt/buildenv-libvirt-debian-9-cross-armv6l:latest - -debian-9-cross-mips64el: - <<: *job_definition - image: quay.io/libvirt/buildenv-libvirt-debian-9-cross-mips64el:latest - -debian-9-cross-mips: - <<: *job_definition - image: quay.io/libvirt/buildenv-libvirt-debian-9-cross-mips:latest - -debian-10-cross-aarch64: - <<: *job_definition - image: quay.io/libvirt/buildenv-libvirt-debian-10-cross-aarch64:latest - -debian-10-cross-ppc64le: - <<: *job_definition - image: quay.io/libvirt/buildenv-libvirt-debian-10-cross-ppc64le:latest +# There are many possible cross-arch jobs we could do, but to preserve +# limited CI resource time allocated to users, we cut it down to two +# interesting variants. The default jobs are x86_64, which means 64-bit +# and little endian. We thus pick armv7l as an interesting 32-bit +# platform, and s390x as an interesting big endian platform. We split +# between Debian 10 and sid to help detect problems on the horizon. debian-10-cross-s390x: <<: *job_definition @@ -37,14 +20,6 @@ debian-sid-cross-armv7l: <<: *job_definition image: quay.io/libvirt/buildenv-libvirt-debian-sid-cross-armv7l:latest -debian-sid-cross-i686: - <<: *job_definition - image: quay.io/libvirt/buildenv-libvirt-debian-sid-cross-i686:latest - -debian-sid-cross-mipsel: - <<: *job_definition - image: quay.io/libvirt/buildenv-libvirt-debian-sid-cross-mipsel:latest - # This artifact published by this job is downloaded by libvirt.org to # be deployed to the web root: # https://gitlab.com/libvirt/libvirt/-/jobs/artifacts/master/download?job=website -- 2.24.1
[libvirt PATCH 1/5] gitlab: use CI for building website contents
Run the bare minimum build that is possible to create the docs. Ideally the '--without-remote' arg would be passed, but there are several bugs preventing a build from succeeding without the remote driver built. The generated website is published as an artifact and thus is browsable on build completion and can be downloaded as a zip file. Signed-off-by: Daniel P. Berrangé --- .gitlab-ci.yml | 23 +++ 1 file changed, 23 insertions(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index ea49c6178b..6f7e0ce135 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -44,3 +44,26 @@ debian-sid-cross-i686: debian-sid-cross-mipsel: <<: *job_definition image: quay.io/libvirt/buildenv-libvirt-debian-sid-cross-mipsel:latest + +# This artifact published by this job is downloaded by libvirt.org to +# be deployed to the web root: +# https://gitlab.com/libvirt/libvirt/-/jobs/artifacts/master/download?job=website +website: + script: +- mkdir build +- cd build +- ../autogen.sh $CONFIGURE_OPTS --prefix=$(pwd)/../vroot || (cat config.log && exit 1) +- make -j $(getconf _NPROCESSORS_ONLN) +- make -j $(getconf _NPROCESSORS_ONLN) install +- cd .. +- mv vroot/share/doc/libvirt/html/ website + image: quay.io/libvirt/buildenv-libvirt-fedora-31:latest + variables: +CONFIGURE_OPTS: --without-libvirtd --without-esx --without-hyperv --without-test --without-dtrace --without-openvz --without-vmware --without-attr --without-audit --without-blkid --without-bash-completion --without-capng --without-curl --without-dbus --without-firewalld --without-fuse --without-glusterfs --without-libiscsi --without-libssh --without-numactl --without-openwsman --without-pciaccess --without-readline --without-sanlock --without-sasl --without-selinux --without-ssh2 --without-udev + artifacts: +expose_as: 'Website' +name: 'website' +when: on_success +expire_in: 30 days +paths: + - website -- 2.24.1
[libvirt PATCH 3/5] gitlab: group jobs into stages
Within a stage all jobs run in parallel. Stages are ordered so later stages are only executed if previous stages succeeded. By using separate stages for the cross builds, we can avoid wasting CI resources if the relatively simple website build fails. Later we can avoid running cross builds, if the native build fails too. Signed-off-by: Daniel P. Berrangé --- .gitlab-ci.yml | 12 +--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index b6a8db7881..e28ec584ea 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -1,4 +1,9 @@ -.job_template: &job_definition +stages: + - website + - cross_build + +.cross_build_job_template: &cross_build_job_definition + stage: cross_build script: - mkdir build - cd build @@ -13,17 +18,18 @@ # between Debian 10 and sid to help detect problems on the horizon. debian-10-cross-s390x: - <<: *job_definition + <<: *cross_build_job_definition image: quay.io/libvirt/buildenv-libvirt-debian-10-cross-s390x:latest debian-sid-cross-armv7l: - <<: *job_definition + <<: *cross_build_job_definition image: quay.io/libvirt/buildenv-libvirt-debian-sid-cross-armv7l:latest # This artifact published by this job is downloaded by libvirt.org to # be deployed to the web root: # https://gitlab.com/libvirt/libvirt/-/jobs/artifacts/master/download?job=website website: + stage: website script: - mkdir build - cd build -- 2.24.1
[libvirt PATCH 5/5] gitlab: rename the cross build jobs
The pipeline UI will truncate the names of jobs after about 15 characters. As a result with the cross-builds, we truncate the most important part of the job name. Putting the most important part first is robust against truncation. Signed-off-by: Daniel P. Berrangé --- .gitlab-ci.yml | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 3e15d08d17..3254ec4d4f 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -50,19 +50,19 @@ ubuntu-1804: # platform, and s390x as an interesting big endian platform. We split # between Debian 10 and sid to help detect problems on the horizon. -debian-10-cross-s390x: +s390x-cross-debian-10: <<: *cross_build_job_definition image: quay.io/libvirt/buildenv-libvirt-debian-10-cross-s390x:latest -debian-sid-cross-armv7l: +armv7l-cross-debian-sid: <<: *cross_build_job_definition image: quay.io/libvirt/buildenv-libvirt-debian-sid-cross-armv7l:latest -fedora-30-cross-mingw32: +mingw32-cross-fedora-30: <<: *cross_build_job_definition image: quay.io/libvirt/buildenv-libvirt-fedora-30-cross-mingw32:latest -fedora-30-cross-mingw64: +mingw64-cross-fedora-30: <<: *cross_build_job_definition image: quay.io/libvirt/buildenv-libvirt-fedora-30-cross-mingw64:latest -- 2.24.1
[libvirt PATCH 4/5] gitlab: add several native CI jobs
With GitLab CI aiming to replace Jenkins and Travis for CI purposes, we need to expand the coverage to include native builds. This patch adds all the jobs currently run in Travis. Compared to Jenkins we obviously miss the FreeBSD jobs, but also Debian 10 and Fedora 30, but we gain the Ubuntu 1804 job as a substitute for Debian. Signed-off-by: Daniel P. Berrangé --- .gitlab-ci.yml | 41 + 1 file changed, 41 insertions(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index e28ec584ea..3e15d08d17 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -1,7 +1,39 @@ stages: - website + - native_build - cross_build + +.native_build_job_template: &native_build_job_definition + stage: native_build + script: +- mkdir build +- cd build +- ../autogen.sh $CONFIGURE_OPTS || (cat config.log && exit 1) +- make -j $(getconf _NPROCESSORS_ONLN) syntax-check +- make -j $(getconf _NPROCESSORS_ONLN) distcheck + +debian-9: + <<: *native_build_job_definition + image: quay.io/libvirt/buildenv-libvirt-debian-9:latest + +centos-7: + <<: *native_build_job_definition + image: quay.io/libvirt/buildenv-libvirt-centos-7:latest + +fedora-31: + <<: *native_build_job_definition + image: quay.io/libvirt/buildenv-libvirt-fedora-31:latest + +fedora-rawhide: + <<: *native_build_job_definition + image: quay.io/libvirt/buildenv-libvirt-fedora-rawhide:latest + +ubuntu-1804: + <<: *native_build_job_definition + image: quay.io/libvirt/buildenv-libvirt-ubuntu-1804:latest + + .cross_build_job_template: &cross_build_job_definition stage: cross_build script: @@ -10,6 +42,7 @@ stages: - ../autogen.sh $CONFIGURE_OPTS || (cat config.log && exit 1) - make -j $(getconf _NPROCESSORS_ONLN) + # There are many possible cross-arch jobs we could do, but to preserve # limited CI resource time allocated to users, we cut it down to two # interesting variants. The default jobs are x86_64, which means 64-bit @@ -25,6 +58,14 @@ debian-sid-cross-armv7l: <<: *cross_build_job_definition image: quay.io/libvirt/buildenv-libvirt-debian-sid-cross-armv7l:latest +fedora-30-cross-mingw32: + <<: *cross_build_job_definition + image: quay.io/libvirt/buildenv-libvirt-fedora-30-cross-mingw32:latest + +fedora-30-cross-mingw64: + <<: *cross_build_job_definition + image: quay.io/libvirt/buildenv-libvirt-fedora-30-cross-mingw64:latest + # This artifact published by this job is downloaded by libvirt.org to # be deployed to the web root: # https://gitlab.com/libvirt/libvirt/-/jobs/artifacts/master/download?job=website -- 2.24.1
[libvirt PATCH 0/5] gitlab: expand the CI job coverage (RESEND)
There are two main goals with this series - Introduce a minimal job building the website and publishing an artifact which can be deployed onto libvirt.org - Expanding CI jobs to get coverage closer to Travis/Jenkins Previous posting lost last two mails due to transient SMTP problem Daniel P. Berrangé (5): gitlab: use CI for building website contents gitlab: reduce number of cross-build CI jobs gitlab: group jobs into stages gitlab: add several native CI jobs gitlab: rename the cross build jobs .gitlab-ci.yml | 105 +++-- 1 file changed, 75 insertions(+), 30 deletions(-) -- 2.24.1
Re: [PATCH] cpu_map: Add more -noTSX x86 CPU models
On Tue, Mar 10, 2020 at 10:18 AM Christian Ehrhardt < christian.ehrha...@canonical.com> wrote: > > ... >> >> Running make check would reveal all these issues because every single >> test which involves parsing the cpu_map was failing due to multiple >> definitions of the same CPU model. >> >> I'd not have expected that the tests will exercise the new XMLs in any > way. > Good to know and thanks for your feedback. > > >> And since this patch is adding several CPU models which are already >> supported by QEMU since 4.2.0, you need to update several existing test >> files for domaincapstest too. You can use >> >> VIR_TEST_REGENERATE_OUTPUT=1 tests/domaincapstest >> >> to regenerate the files. Just make sure you review the changes before >> adding them to this commit. >> > > Without regenerating these I see as expected > FAIL: domaincapstest > FAIL: cputest > > Adding these 5 types to the qemu 4.2 and qemu 5.0 > tests/domaincapsdata worked. > I've squashed that into the same patch - let me know if you'd prefer the > domaincapsdata change as an individual patch instead. > > Regenerating the test files will also be needed for cputest because I >> just pushed the "cputest: Add data for Intel(R) Core(TM) i7-8550U CPU >> without TSX". Doing so will nicely show that the computed host CPU model >> (in x86_64-cpuid-Core-i7-8550U-host.xml file) is >> Skylake-Client-noTSX-IBRS rather than Broadwell-noTSX-IBRS. >> > > Indeed: > 1007 In > '/home/paelzer/work/libvirt/libvirt-ubuntu-git/build/../tests/cputestdata/x86_64-cpuid-Core-i7-8550U-host.xml': > ... > 1009 Expect [Broadwell-noTSX-IBRS > ... > 1043 Actual [Skylake-Client-noTSX-IBRS > > >> However, the CPU used for host-model (and reported in domain >> capabilities) as shown in x86_64-cpuid-Core-i7-8550U-guest.xml and >> x86_64-cpuid-Core-i7-8550U-json.xml will change from Skylake-Client-IBRS >> to Skylake-Client-noTSX-IBRS. As I said in my previous reply to this >> patch, I think these two CPU definitions should keep using the old >> Skylake-Client-IBRS model to make sure any domain with host-model CPU >> will always use the CPU models without noTSX for better compatibility >> between current and future version of libvirt. > > > I see three changes: > x86_64-cpuid-Core-i7-8550U-host.xml: Broadwell-noTSX-IBRS -> > Skylake-Client-noTSX-IBRS > x86_64-cpuid-Core-i7-8550U-guest.xml: Skylake-Client-IBRS -> > Skylake-Client-noTSX-IBRS > This shows up twice in: > 238) cpuTestGuestCPUID(x86_64): Core-i7-8550U > 240) cpuTestGuestCPUID(x86_64): Core-i7-8550U > x86_64-cpuid-Core-i7-8550U-json.xml: Skylake-Client-IBRS -> > Skylake-Client-noTSX-IBRS > > So far so good and as expected. > But I have to beg your pardon and need to ask where such an override to > continue to use > "Skylake-Client-IBRS + policy='disable' name='hle' policy='disable' > name='rtm'" instead of just "Skylake-Client-noTSX-IBRS" would have to go? > Do I get it right that this request would break the current definition of cpuDecode: ... 185 * when decoding the data. In general, this function will select the model 186 * closest to the CPU specified by @data. 187 * 188 * For VIR_ARCH_I686 and VIR_ARCH_X86_64 architectures this means the computed 189 * CPU definition will have the shortest possible list of additional features. The actual decode is arch specific via x86DecodeCPUData, here it evaluates the length of the feature list and thereby prefers the -noTSX type. Skylake-Client-noTSX-IBRS: (gdb) p cpuCandidate->nfeatures $30 = 25 And since it needs to disable hle/rtm the type Skylake-Client-IBRS will have (gdb) p cpuCandidate->nfeatures $33 = 27 I don't see an obvious non-hacky way yet to get these detected as non -noTSX types. And in general I think we'd not want to map just -noTSX-IBRS to the -IBRS types. With the versioned CPUs it is more like "Skylake-Client-* => Skylake-Client" which is like selecting the moving base version. I really beg your pardon, but I don't see this as part of this patch set just trying to add these new -noTSX types, but some later work to add versioned CPU support. Please tell if I'm missing something here and guide me to where such an override of the preferred type should be done. Until then I'll prep a v2 that also adapts the cputests to match ... Holding back v2 submission until this is solved ... > > This change should be in >> a separate patch, but in single series with the current patch. >> >> Jirka >> >> > > -- > Christian Ehrhardt > Staff Engineer, Ubuntu Server > Canonical Ltd > -- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
[PATCH v2 1/1] cpu_map: Add more -noTSX x86 CPU models
One of the mitigation methods for TAA[1] is to disable TSX support on the host system. Linux added a mechanism to disable TSX globally through the kernel command line, and many Linux distributions now default to tsx=off. This makes existing CPU models that have HLE and RTM enabled not usable anymore. Add new versions of all CPU models that have the HLE and RTM features enabled, that can be used when TSX is disabled in the host system. On systems disabling the features without those types defined in cpu-maps users end up without modern CPU types in the list of usable CPUs to use in the likes of virsh domcapabilities or tools higher in the stack like virt-manager. This adds: -Cascadelake-Server-noTSX -Icelake-Client-noTSX -Icelake-Server-noTSX -Skylake-Server-noTSX-IBRS -Skylake-Client-noTSX-IBRS Introduced in QEMU by commit v4.2.0-rc2-3-g9ab2237f19 (function) and commit v4.2.0-rc2-4-g02fa60d101 (names) References: [1] TAA, TSX asynchronous Abort: https://software.intel.com/security-software-guidance/insights/deep-dive-intel-transactional-synchronization-extensions-intel-tsx-asynchronous-abort https://www.kernel.org/doc/html/latest/admin-guide/hw-vuln/tsx_async_abort.html Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1853200 Signed-off-by: Christian Ehrhardt --- src/cpu_map/Makefile.inc.am | 5 ++ src/cpu_map/index.xml | 5 ++ src/cpu_map/x86_Cascadelake-Server-noTSX.xml | 78 src/cpu_map/x86_Icelake-Client-noTSX.xml | 81 + src/cpu_map/x86_Icelake-Server-noTSX.xml | 90 +++ src/cpu_map/x86_Skylake-Client-noTSX-IBRS.xml | 73 +++ src/cpu_map/x86_Skylake-Server-noTSX-IBRS.xml | 75 .../x86_64-cpuid-Core-i7-8550U-guest.xml | 4 +- .../x86_64-cpuid-Core-i7-8550U-host.xml | 11 +-- .../x86_64-cpuid-Core-i7-8550U-json.xml | 4 +- .../domaincapsdata/qemu_4.2.0-q35.x86_64.xml | 5 ++ .../domaincapsdata/qemu_4.2.0-tcg.x86_64.xml | 5 ++ tests/domaincapsdata/qemu_4.2.0.x86_64.xml| 5 ++ .../domaincapsdata/qemu_5.0.0-q35.x86_64.xml | 5 ++ .../domaincapsdata/qemu_5.0.0-tcg.x86_64.xml | 5 ++ tests/domaincapsdata/qemu_5.0.0.x86_64.xml| 5 ++ 16 files changed, 440 insertions(+), 16 deletions(-) create mode 100644 src/cpu_map/x86_Cascadelake-Server-noTSX.xml create mode 100644 src/cpu_map/x86_Icelake-Client-noTSX.xml create mode 100644 src/cpu_map/x86_Icelake-Server-noTSX.xml create mode 100644 src/cpu_map/x86_Skylake-Client-noTSX-IBRS.xml create mode 100644 src/cpu_map/x86_Skylake-Server-noTSX-IBRS.xml diff --git a/src/cpu_map/Makefile.inc.am b/src/cpu_map/Makefile.inc.am index e935178304..be64c9a0d4 100644 --- a/src/cpu_map/Makefile.inc.am +++ b/src/cpu_map/Makefile.inc.am @@ -20,6 +20,7 @@ cpumap_DATA = \ cpu_map/x86_Broadwell-noTSX.xml \ cpu_map/x86_Broadwell-noTSX-IBRS.xml \ cpu_map/x86_Cascadelake-Server.xml \ + cpu_map/x86_Cascadelake-Server-noTSX.xml \ cpu_map/x86_Conroe.xml \ cpu_map/x86_core2duo.xml \ cpu_map/x86_coreduo.xml \ @@ -33,7 +34,9 @@ cpumap_DATA = \ cpu_map/x86_Haswell-noTSX.xml \ cpu_map/x86_Haswell-noTSX-IBRS.xml \ cpu_map/x86_Icelake-Client.xml \ + cpu_map/x86_Icelake-Client-noTSX.xml \ cpu_map/x86_Icelake-Server.xml \ + cpu_map/x86_Icelake-Server-noTSX.xml \ cpu_map/x86_IvyBridge.xml \ cpu_map/x86_IvyBridge-IBRS.xml \ cpu_map/x86_kvm32.xml \ @@ -58,8 +61,10 @@ cpumap_DATA = \ cpu_map/x86_SandyBridge-IBRS.xml \ cpu_map/x86_Skylake-Client.xml \ cpu_map/x86_Skylake-Client-IBRS.xml \ + cpu_map/x86_Skylake-Client-noTSX-IBRS.xml \ cpu_map/x86_Skylake-Server.xml \ cpu_map/x86_Skylake-Server-IBRS.xml \ + cpu_map/x86_Skylake-Server-noTSX-IBRS.xml \ cpu_map/x86_Westmere.xml \ cpu_map/x86_Westmere-IBRS.xml \ $(NULL) diff --git a/src/cpu_map/index.xml b/src/cpu_map/index.xml index ffb2f6fe1b..50b030de29 100644 --- a/src/cpu_map/index.xml +++ b/src/cpu_map/index.xml @@ -44,11 +44,16 @@ + + + + + diff --git a/src/cpu_map/x86_Cascadelake-Server-noTSX.xml b/src/cpu_map/x86_Cascadelake-Server-noTSX.xml new file mode 100644 index 00..d24415ebce --- /dev/null +++ b/src/cpu_map/x86_Cascadelake-Server-noTSX.xml @@ -0,0 +1,78 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/src/c
[PATCH v2 0/1] fix systems having hle/rtm disabled by the kernel
Thanks for the discussion on v1. I was rerunning the tests to make the tests pass. I have not made it prefer -IBRS types before -noTS-IBRS types as outlined in my former reply to this thread. But I wanted to submit the v2 to have this topic make some progress without waiting for the potentially long "should we prefer -IBRS discussion". Updates in v2: - fix model names in new cpu_map files - update domaincapsdata to match the new models - update cputestdata to match the new models Christian Ehrhardt (1): cpu_map: Add more -noTSX x86 CPU models src/cpu_map/Makefile.inc.am | 5 ++ src/cpu_map/index.xml | 5 ++ src/cpu_map/x86_Cascadelake-Server-noTSX.xml| 78 src/cpu_map/x86_Icelake-Client-noTSX.xml| 81 + src/cpu_map/x86_Icelake-Server-noTSX.xml| 90 +++ src/cpu_map/x86_Skylake-Client-noTSX-IBRS.xml | 73 +++ src/cpu_map/x86_Skylake-Server-noTSX-IBRS.xml | 75 tests/x86_64-cpuid-Core-i7-8550U-guest.xml | 4 +- tests/x86_64-cpuid-Core-i7-8550U-host.xml | 11 +-- tests/x86_64-cpuid-Core-i7-8550U-json.xml | 4 +- tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml | 5 ++ tests/domaincapsdata/qemu_4.2.0-tcg.x86_64.xml | 5 ++ tests/domaincapsdata/qemu_4.2.0.x86_64.xml | 5 ++ tests/domaincapsdata/qemu_5.0.0-q35.x86_64.xml | 5 ++ tests/domaincapsdata/qemu_5.0.0-tcg.x86_64.xml | 5 ++ tests/domaincapsdata/qemu_5.0.0.x86_64.xml | 5 ++ 16 files changed, 440 insertions(+), 16 deletions(-) create mode 100644 src/cpu_map/x86_Cascadelake-Server-noTSX.xml create mode 100644 src/cpu_map/x86_Icelake-Client-noTSX.xml create mode 100644 src/cpu_map/x86_Icelake-Server-noTSX.xml create mode 100644 src/cpu_map/x86_Skylake-Client-noTSX-IBRS.xml create mode 100644 src/cpu_map/x86_Skylake-Server-noTSX-IBRS.xml -- 2.25.1
Re: [PATCH v3 4/4] qemu-img: Deprecate use of -b without -F
On Mon, Mar 09, 2020 at 10:42:25AM -0500, Eric Blake wrote: [...] > > > +qemu-img backing file without format (since 5.0.0) > > > +'' > > > + > > > +The use of ``qemu-img create``, ``qemu-img rebase``, ``qemu-img > > > +convert``, or ``qemu-img amend`` to create or modify an image that > > > +depends on a backing file now recommends that an explicit backing > > > +format be provided. Also for the `qemu-img amend` case, I'm not clear if the following scenario ought to throw the warning: $> ~/build/qemu/qemu-img info --backing-chain overlay1.qcow2 image: overlay1.qcow2 file format: qcow2 virtual size: 4 GiB (4294967296 bytes) disk size: 196 KiB cluster_size: 65536 backing file: base.raw Format specific information: compat: 1.1 lazy refcounts: false refcount bits: 16 corrupt: false image: base.raw file format: raw virtual size: 4 GiB (4294967296 bytes) disk size: 778 MiB $> ~/build/qemu/qemu-img amend -o compat=v3 overlay1.qcow2 $> echo $? 0 I'm just trying to work out the scenarios where the warnings ought to show up (for all the four cases: create, rebase, convert, amend). [...] -- /kashyap
Re: [PATCH 0/2] virbpf: Set errno instead of reporting errors
On 3/9/20 9:31 AM, Michal Privoznik wrote: Noticed this while helping to debug a CGroupV2 problem. Michal Prívozník (2): virCgroupV2DevicesAvailable: Print stringified errno in the debug log virbpf: Set errno instead of reporting errors po/POTFILES.in| 1 - src/util/virbpf.c | 39 --- src/util/vircgroupv2devices.c | 2 +- 3 files changed, 14 insertions(+), 28 deletions(-) Reviewed-by: Daniel Henrique Barboza
[RFC] qemu: convert DomainLogContext class to use GObject
---
src/qemu/qemu_domain.c | 36
src/qemu/qemu_domain.h | 6 --
src/qemu/qemu_process.c | 4 ++--
3 files changed, 26 insertions(+), 20 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 3d3f796d85..0d2edf4dbe 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -150,7 +150,7 @@ qemuDomainObjFromDomain(virDomainPtr domain)
struct _qemuDomainLogContext {
-virObject parent;
+GObject parent;
int writefd;
int readfd; /* Only used if manager == NULL */
@@ -160,37 +160,46 @@ struct _qemuDomainLogContext {
virLogManagerPtr manager;
};
-static virClassPtr qemuDomainLogContextClass;
+G_DEFINE_TYPE(qemuDomainLogContext, qemu_domain_log_context, G_TYPE_OBJECT);
static virClassPtr qemuDomainSaveCookieClass;
-static void qemuDomainLogContextDispose(void *obj);
+static void qemuDomainLogContextFinalize(GObject *obj);
static void qemuDomainSaveCookieDispose(void *obj);
static int
qemuDomainOnceInit(void)
{
-if (!VIR_CLASS_NEW(qemuDomainLogContext, virClassForObject()))
-return -1;
-
if (!VIR_CLASS_NEW(qemuDomainSaveCookie, virClassForObject()))
return -1;
return 0;
}
+static void qemu_domain_log_context_init(qemuDomainLogContext *logctxt
G_GNUC_UNUSED)
+{
+}
+
+static void qemu_domain_log_context_class_init(qemuDomainLogContextClass
*klass)
+{
+GObjectClass *obj = G_OBJECT_CLASS(klass);
+
+obj->finalize = qemuDomainLogContextFinalize;
+}
+
VIR_ONCE_GLOBAL_INIT(qemuDomain);
static void
-qemuDomainLogContextDispose(void *obj)
+qemuDomainLogContextFinalize(GObject *object)
{
-qemuDomainLogContextPtr ctxt = obj;
+qemuDomainLogContextPtr ctxt = QEMU_DOMAIN_LOG_CONTEXT(object);
VIR_DEBUG("ctxt=%p", ctxt);
virLogManagerFree(ctxt->manager);
VIR_FREE(ctxt->path);
VIR_FORCE_CLOSE(ctxt->writefd);
VIR_FORCE_CLOSE(ctxt->readfd);
+G_OBJECT_CLASS(qemu_domain_log_context_parent_class)->finalize(object);
}
const char *
@@ -10557,13 +10566,7 @@ qemuDomainLogContextPtr
qemuDomainLogContextNew(virQEMUDriverPtr driver,
qemuDomainLogContextMode mode)
{
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
-qemuDomainLogContextPtr ctxt = NULL;
-
-if (qemuDomainInitialize() < 0)
-return NULL;
-
-if (!(ctxt = virObjectNew(qemuDomainLogContextClass)))
-return NULL;
+qemuDomainLogContextPtr ctxt =
QEMU_DOMAIN_LOG_CONTEXT(g_object_new(QEMU_TYPE_DOMAIN_LOG_CONTEXT, NULL));
VIR_DEBUG("Context new %p stdioLogD=%d", ctxt, cfg->stdioLogD);
ctxt->writefd = -1;
@@ -10632,7 +10635,8 @@ qemuDomainLogContextPtr
qemuDomainLogContextNew(virQEMUDriverPtr driver,
return ctxt;
error:
-virObjectUnref(ctxt);
+if (ctxt)
+g_object_unref(ctxt);
return NULL;
}
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 3929ee9ca1..3c270b87a2 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -37,6 +37,8 @@
#include "virmdev.h"
#include "virchrdev.h"
#include "virobject.h"
+#include "internal.h"
+#include
#include "logging/log_manager.h"
#include "virdomainmomentobjlist.h"
#include "virenum.h"
@@ -598,9 +600,9 @@ struct qemuProcessEvent {
void qemuProcessEventFree(struct qemuProcessEvent *event);
-typedef struct _qemuDomainLogContext qemuDomainLogContext;
+#define QEMU_TYPE_DOMAIN_LOG_CONTEXT qemu_domain_log_context_get_type()
+G_DECLARE_FINAL_TYPE(qemuDomainLogContext, qemu_domain_log_context, QEMU,
DOMAIN_LOG_CONTEXT, GObject);
typedef qemuDomainLogContext *qemuDomainLogContextPtr;
-G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuDomainLogContext, virObjectUnref);
typedef struct _qemuDomainSaveCookie qemuDomainSaveCookie;
typedef qemuDomainSaveCookie *qemuDomainSaveCookiePtr;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 499d39a920..9f7c245c7e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1925,7 +1925,7 @@ static void
qemuProcessMonitorLogFree(void *opaque)
{
qemuDomainLogContextPtr logCtxt = opaque;
-virObjectUnref(logCtxt);
+g_clear_object(&logCtxt);
}
@@ -1978,7 +1978,7 @@ qemuConnectMonitor(virQEMUDriverPtr driver,
virDomainObjPtr vm, int asyncJob,
driver);
if (mon && logCtxt) {
-virObjectRef(logCtxt);
+g_object_ref(logCtxt);
qemuMonitorSetDomainLog(mon,
qemuProcessMonitorReportLogError,
logCtxt,
--
2.24.1
Re: [PATCH v3 4/4] qemu-img: Deprecate use of -b without -F
On 3/10/20 4:47 AM, Kashyap Chamarthy wrote: To provoke the warning during convert, you'll have to also pass -B (or -o backing_file), without -o backing_fmt (since convert lacks the -F shorthand). Hmm, I tried the following way, but it doesn't provoke the warning: $> ~/build/qemu/qemu-img convert -B ./base.raw -O qcow2 overlay1.qcow2 flattened.qcow2 $> ~/build/qemu/qemu-img info flattened.qcow2 image: flattened.qcow2 file format: qcow2 virtual size: 4 GiB (4294967296 bytes) disk size: 196 KiB cluster_size: 65536 backing file: ./base.raw Format specific information: compat: 1.1 lazy refcounts: false refcount bits: 16 corrupt: false What am I missing? Rather, it looks like my patch is missing a warning on that path. I'll have to investigate, for v4. - - - Ah, didn't realize the inconsistency of 'convert' lacking the '-F' shorthand ... which reminds me, there are at least _three_ ways that I know of, to specify backing file format with 'create': $ qemu-img create -f qcow2 -o 'backing_file=./base.raw,backing_fmt=raw' ./overlay1.qcow2 $ qemu-img create -f qcow2 -b ./base.raw -o backing_fmt=raw overlay1.qcow2 $ qemu-img create -f qcow2 -b ./base.raw -F raw ./overlay1.qcow2 I'm wondering about the consistency of having all the above three supported for other operations too. Now I at least know 'convert' lacks the "-F". The -o forms (backing_file= and backing_fmt=) always work. Various commands then have additional shorthand: -b/-F for create, -B for convert. You're right that we aren't very consistent, but I'm reluctant to change the inconsistencies in this patch (at one point in the past, we tried to get rid of the shorthand and force all users to go through -o, but that broke too many clients that were depending on the undocumented shorthand, so we documented the existing shorthand instead). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 4/4] qemu-img: Deprecate use of -b without -F
On 3/10/20 5:57 AM, Kashyap Chamarthy wrote: On Mon, Mar 09, 2020 at 10:42:25AM -0500, Eric Blake wrote: [...] +qemu-img backing file without format (since 5.0.0) +'' + +The use of ``qemu-img create``, ``qemu-img rebase``, ``qemu-img +convert``, or ``qemu-img amend`` to create or modify an image that +depends on a backing file now recommends that an explicit backing +format be provided. Also for the `qemu-img amend` case, I'm not clear if the following scenario ought to throw the warning: $> ~/build/qemu/qemu-img info --backing-chain overlay1.qcow2 image: overlay1.qcow2 file format: qcow2 virtual size: 4 GiB (4294967296 bytes) disk size: 196 KiB cluster_size: 65536 backing file: base.raw Format specific information: compat: 1.1 lazy refcounts: false refcount bits: 16 corrupt: false image: base.raw file format: raw virtual size: 4 GiB (4294967296 bytes) disk size: 778 MiB $> ~/build/qemu/qemu-img amend -o compat=v3 overlay1.qcow2 This particular amend is not changing the backing file, and since I did not add warnings on the opening of an existing unsafe image, it is silent. Instead, you need to test a case where amend provokes a path that would change the backing file (perhaps as simple as '-o backing_file=./base.raw'), while omitting a format for the new backing file string. $> echo $? 0 I'm just trying to work out the scenarios where the warnings ought to show up (for all the four cases: create, rebase, convert, amend). [...] -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 4/4] qemu-img: Deprecate use of -b without -F
On 3/10/20 5:57 AM, Kashyap Chamarthy wrote: On Mon, Mar 09, 2020 at 10:42:25AM -0500, Eric Blake wrote: [...] +qemu-img backing file without format (since 5.0.0) +'' + +The use of ``qemu-img create``, ``qemu-img rebase``, ``qemu-img +convert``, or ``qemu-img amend`` to create or modify an image that +depends on a backing file now recommends that an explicit backing +format be provided. Also for the `qemu-img amend` case, I'm not clear if the following scenario ought to throw the warning: $> ~/build/qemu/qemu-img info --backing-chain overlay1.qcow2 image: overlay1.qcow2 file format: qcow2 virtual size: 4 GiB (4294967296 bytes) disk size: 196 KiB cluster_size: 65536 backing file: base.raw Format specific information: compat: 1.1 lazy refcounts: false refcount bits: 16 corrupt: false image: base.raw file format: raw virtual size: 4 GiB (4294967296 bytes) disk size: 778 MiB $> ~/build/qemu/qemu-img amend -o compat=v3 overlay1.qcow2 This particular amend is not changing the backing file, and since I did not add warnings on the opening of an existing unsafe image, it is silent. Instead, you need to test a case where amend provokes a path that would change the backing file (perhaps as simple as '-o backing_file=./base.raw'), while omitting a format for the new backing file string. $> echo $? 0 I'm just trying to work out the scenarios where the warnings ought to show up (for all the four cases: create, rebase, convert, amend). Look at patch 2/4 - that patch was written AFTER this patch in order to silence every warning that was introduced because of this patch, then rebased to occur first. My experience in writing 2/4 was that I indeed hit warnings through all four sub-commands. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [RFC] qemu: convert DomainLogContext class to use GObject
On Tue, Mar 10, 2020 at 17:30:01 +0530, Gaurav Agrawal wrote:
> ---
> src/qemu/qemu_domain.c | 36
> src/qemu/qemu_domain.h | 6 --
> src/qemu/qemu_process.c | 4 ++--
> 3 files changed, 26 insertions(+), 20 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 3d3f796d85..0d2edf4dbe 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -150,7 +150,7 @@ qemuDomainObjFromDomain(virDomainPtr domain)
[...]
> static int
> qemuDomainOnceInit(void)
> {
> -if (!VIR_CLASS_NEW(qemuDomainLogContext, virClassForObject()))
> -return -1;
> -
> if (!VIR_CLASS_NEW(qemuDomainSaveCookie, virClassForObject()))
> return -1;
>
> return 0;
> }
>
> +static void qemu_domain_log_context_init(qemuDomainLogContext *logctxt
> G_GNUC_UNUSED)
> +{
> +}
There's no reason to break coding style rules in this kind of refactor
nor to make it inconsistent in span of 20 lines.
> +
> +static void qemu_domain_log_context_class_init(qemuDomainLogContextClass
> *klass)
> +{
> +GObjectClass *obj = G_OBJECT_CLASS(klass);
> +
> +obj->finalize = qemuDomainLogContextFinalize;
> +}
> +
Re: [RFC] qemu: convert DomainLogContext class to use GObject
On Tue, Mar 10, 2020 at 01:33:42PM +0100, Peter Krempa wrote:
> On Tue, Mar 10, 2020 at 17:30:01 +0530, Gaurav Agrawal wrote:
> > ---
> > src/qemu/qemu_domain.c | 36
> > src/qemu/qemu_domain.h | 6 --
> > src/qemu/qemu_process.c | 4 ++--
> > 3 files changed, 26 insertions(+), 20 deletions(-)
> >
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index 3d3f796d85..0d2edf4dbe 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -150,7 +150,7 @@ qemuDomainObjFromDomain(virDomainPtr domain)
>
> [...]
>
> > static int
> > qemuDomainOnceInit(void)
> > {
> > -if (!VIR_CLASS_NEW(qemuDomainLogContext, virClassForObject()))
> > -return -1;
> > -
> > if (!VIR_CLASS_NEW(qemuDomainSaveCookie, virClassForObject()))
> > return -1;
> >
> > return 0;
> > }
> >
> > +static void qemu_domain_log_context_init(qemuDomainLogContext *logctxt
> > G_GNUC_UNUSED)
> > +{
> > +}
>
> There's no reason to break coding style rules in this kind of refactor
> nor to make it inconsistent in span of 20 lines.
If you're refering to the use of underscores, then this is expected
and indeed required because these method names are auto-generated
by the GObject macros. This should only be the case for three
specific methods (_init, _class_init and _get_type), with the rest
all following normal style. See the virIdentity conversion for
the prior example of this.
>
> > +
> > +static void qemu_domain_log_context_class_init(qemuDomainLogContextClass
> > *klass)
> > +{
> > +GObjectClass *obj = G_OBJECT_CLASS(klass);
> > +
> > +obj->finalize = qemuDomainLogContextFinalize;
> > +}
> > +
>
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [RFC] qemu: convert DomainLogContext class to use GObject
On Tue, Mar 10, 2020 at 12:42:47 +, Daniel Berrange wrote:
> On Tue, Mar 10, 2020 at 01:33:42PM +0100, Peter Krempa wrote:
> > On Tue, Mar 10, 2020 at 17:30:01 +0530, Gaurav Agrawal wrote:
> > > ---
> > > src/qemu/qemu_domain.c | 36
> > > src/qemu/qemu_domain.h | 6 --
> > > src/qemu/qemu_process.c | 4 ++--
> > > 3 files changed, 26 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > > index 3d3f796d85..0d2edf4dbe 100644
> > > --- a/src/qemu/qemu_domain.c
> > > +++ b/src/qemu/qemu_domain.c
> > > @@ -150,7 +150,7 @@ qemuDomainObjFromDomain(virDomainPtr domain)
> >
> > [...]
> >
> > > static int
> > > qemuDomainOnceInit(void)
> > > {
> > > -if (!VIR_CLASS_NEW(qemuDomainLogContext, virClassForObject()))
> > > -return -1;
> > > -
> > > if (!VIR_CLASS_NEW(qemuDomainSaveCookie, virClassForObject()))
> > > return -1;
> > >
> > > return 0;
> > > }
> > >
> > > +static void qemu_domain_log_context_init(qemuDomainLogContext *logctxt
> > > G_GNUC_UNUSED)
> > > +{
> > > +}
> >
> > There's no reason to break coding style rules in this kind of refactor
> > nor to make it inconsistent in span of 20 lines.
>
> If you're refering to the use of underscores, then this is expected
> and indeed required because these method names are auto-generated
> by the GObject macros. This should only be the case for three
> specific methods (_init, _class_init and _get_type), with the rest
> all following normal style. See the virIdentity conversion for
> the prior example of this.
Yeah, I meant mainly that (also the newline after the type).
Can't we at least keep camel-case for the type itself?
'qemuDomainLogContext_init'
That way at least looking for the symbol name will be less painful.
Re: [RFC] qemu: convert DomainLogContext class to use GObject
On Tue, Mar 10, 2020 at 01:48:39PM +0100, Peter Krempa wrote:
> On Tue, Mar 10, 2020 at 12:42:47 +, Daniel Berrange wrote:
> > On Tue, Mar 10, 2020 at 01:33:42PM +0100, Peter Krempa wrote:
> > > On Tue, Mar 10, 2020 at 17:30:01 +0530, Gaurav Agrawal wrote:
> > > > ---
> > > > src/qemu/qemu_domain.c | 36
> > > > src/qemu/qemu_domain.h | 6 --
> > > > src/qemu/qemu_process.c | 4 ++--
> > > > 3 files changed, 26 insertions(+), 20 deletions(-)
> > > >
> > > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > > > index 3d3f796d85..0d2edf4dbe 100644
> > > > --- a/src/qemu/qemu_domain.c
> > > > +++ b/src/qemu/qemu_domain.c
> > > > @@ -150,7 +150,7 @@ qemuDomainObjFromDomain(virDomainPtr domain)
> > >
> > > [...]
> > >
> > > > static int
> > > > qemuDomainOnceInit(void)
> > > > {
> > > > -if (!VIR_CLASS_NEW(qemuDomainLogContext, virClassForObject()))
> > > > -return -1;
> > > > -
> > > > if (!VIR_CLASS_NEW(qemuDomainSaveCookie, virClassForObject()))
> > > > return -1;
> > > >
> > > > return 0;
> > > > }
> > > >
> > > > +static void qemu_domain_log_context_init(qemuDomainLogContext *logctxt
> > > > G_GNUC_UNUSED)
> > > > +{
> > > > +}
> > >
> > > There's no reason to break coding style rules in this kind of refactor
> > > nor to make it inconsistent in span of 20 lines.
> >
> > If you're refering to the use of underscores, then this is expected
> > and indeed required because these method names are auto-generated
> > by the GObject macros. This should only be the case for three
> > specific methods (_init, _class_init and _get_type), with the rest
> > all following normal style. See the virIdentity conversion for
> > the prior example of this.
>
> Yeah, I meant mainly that (also the newline after the type).
>
> Can't we at least keep camel-case for the type itself?
>
> 'qemuDomainLogContext_init'
>
> That way at least looking for the symbol name will be less painful.
We had considered that in the original virIdentity conversion, but
decided mixing two different naming styles in one identifier was
even more ugly.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [RFC] qemu: convert DomainLogContext class to use GObject
On Tue, Mar 10, 2020 at 12:53:59 +, Daniel Berrange wrote: > On Tue, Mar 10, 2020 at 01:48:39PM +0100, Peter Krempa wrote: > > On Tue, Mar 10, 2020 at 12:42:47 +, Daniel Berrange wrote: > > > On Tue, Mar 10, 2020 at 01:33:42PM +0100, Peter Krempa wrote: > > > > On Tue, Mar 10, 2020 at 17:30:01 +0530, Gaurav Agrawal wrote: > > > > > --- > > > > > src/qemu/qemu_domain.c | 36 > > > > > src/qemu/qemu_domain.h | 6 -- > > > > > src/qemu/qemu_process.c | 4 ++-- > > > > > 3 files changed, 26 insertions(+), 20 deletions(-) [...] > > > > > > > > There's no reason to break coding style rules in this kind of refactor > > > > nor to make it inconsistent in span of 20 lines. > > > > > > If you're refering to the use of underscores, then this is expected > > > and indeed required because these method names are auto-generated > > > by the GObject macros. This should only be the case for three > > > specific methods (_init, _class_init and _get_type), with the rest > > > all following normal style. See the virIdentity conversion for > > > the prior example of this. > > > > Yeah, I meant mainly that (also the newline after the type). > > > > Can't we at least keep camel-case for the type itself? > > > > 'qemuDomainLogContext_init' > > > > That way at least looking for the symbol name will be less painful. > > We had considered that in the original virIdentity conversion, but > decided mixing two different naming styles in one identifier was > even more ugly. While ugly I still think that for greppability/searchability it's way better to keep the object name using camel case. In the end as you've pointed out it's just for the specific methods named above. Making it look ugly is IMO less worse than missing something when trying to understand how the code works.
[libvirt PATCH 2/4] util: remove virStrerror
Now that we use g_strerror exclusively, remove this unused
function.
Signed-off-by: Ján Tomko
---
src/libvirt_private.syms | 1 -
src/util/virerror.c | 25 -
src/util/virerror.h | 2 --
3 files changed, 28 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 511fb88872..22f6d14ced 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1981,7 +1981,6 @@ virReportOOMErrorFull;
virReportSystemErrorFull;
virSetError;
virSetErrorLogPriorityFunc;
-virStrerror;
# util/vireventglib.h
diff --git a/src/util/virerror.c b/src/util/virerror.c
index 16c384d2f9..774c36bca3 100644
--- a/src/util/virerror.c
+++ b/src/util/virerror.c
@@ -1307,31 +1307,6 @@ void virReportErrorHelper(int domcode,
errno = save_errno;
}
-/**
- * virStrerror:
- * @theerrno: the errno value
- * @errBuf: the buffer to save the error to
- * @errBufLen: the buffer length
- *
- * Generate an error string for the given errno
- *
- * Returns a pointer to the error string, possibly indicating that the
- * error is unknown
- */
-const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen)
-{
-int save_errno = errno;
-const char *ret;
-const char *str = g_strerror(theerrno);
-size_t len = strlen(str);
-
-memcpy(errBuf, str, MIN(len, errBufLen));
-errBuf[errBufLen-1] = '\0';
-ret = errBuf;
-errno = save_errno;
-return ret;
-}
-
/**
* virReportSystemErrorFull:
* @domcode: the virErrorDomain indicating where it's coming from
diff --git a/src/util/virerror.h b/src/util/virerror.h
index 2e27655b72..9d3e40d65a 100644
--- a/src/util/virerror.h
+++ b/src/util/virerror.h
@@ -192,8 +192,6 @@ void virReportOOMErrorFull(int domcode,
int virSetError(virErrorPtr newerr);
virErrorPtr virErrorCopyNew(virErrorPtr err);
void virDispatchError(virConnectPtr conn);
-/* DEPRECATED: use g_strerror() directly */
-const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen);
typedef int (*virErrorLogPriorityFunc)(virErrorPtr, int);
void virSetErrorLogPriorityFunc(virErrorLogPriorityFunc func);
--
2.24.1
[libvirt PATCH 1/4] Use g_strerror instead of virStrerror
Remove lots of stack-allocated buffers.
Signed-off-by: Ján Tomko
---
src/libxl/libxl_conf.c | 3 +--
src/libxl/libxl_driver.c| 11 +--
src/libxl/libxl_logger.c| 6 ++
src/locking/lock_daemon.c | 9 +++--
src/logging/log_daemon.c| 9 +++--
src/lxc/lxc_container.c | 3 +--
src/lxc/lxc_process.c | 7 +++
src/network/bridge_driver.c | 3 +--
src/qemu/qemu_capabilities.c| 3 +--
src/qemu/qemu_interface.c | 3 +--
src/qemu/qemu_process.c | 9 +++--
src/remote/remote_daemon.c | 13 -
src/security/security_manager.c | 5 ++---
src/security/security_selinux.c | 5 ++---
src/storage/storage_driver.c| 3 +--
src/util/viraudit.c | 3 +--
src/util/vircommand.c | 3 +--
src/util/virfile.c | 3 +--
src/util/virhostcpu.c | 8 +++-
src/util/virlockspace.c | 9 +++--
src/util/virpci.c | 12
src/util/virpidfile.c | 3 +--
src/util/virprocess.c | 3 +--
src/util/virutil.c | 6 ++
tools/vsh.c | 28 ++--
25 files changed, 60 insertions(+), 110 deletions(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 907df525c5..be5fc505fe 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1740,14 +1740,13 @@ libxlDriverConfigNew(void)
int
libxlDriverConfigInit(libxlDriverConfigPtr cfg)
{
-char ebuf[1024];
unsigned int free_mem;
if (virFileMakePath(cfg->logDir) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("failed to create log dir '%s': %s"),
cfg->logDir,
- virStrerror(errno, ebuf, sizeof(ebuf)));
+ g_strerror(errno));
return -1;
}
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index f2387e2a20..cc93f1802d 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -654,7 +654,6 @@ libxlStateInitialize(bool privileged,
{
libxlDriverConfigPtr cfg;
g_autofree char *driverConf = NULL;
-char ebuf[1024];
bool autostart = true;
if (root != NULL) {
@@ -725,35 +724,35 @@ libxlStateInitialize(bool privileged,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("failed to create state dir '%s': %s"),
cfg->stateDir,
- virStrerror(errno, ebuf, sizeof(ebuf)));
+ g_strerror(errno));
goto error;
}
if (virFileMakePath(cfg->libDir) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("failed to create lib dir '%s': %s"),
cfg->libDir,
- virStrerror(errno, ebuf, sizeof(ebuf)));
+ g_strerror(errno));
goto error;
}
if (virFileMakePath(cfg->saveDir) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("failed to create save dir '%s': %s"),
cfg->saveDir,
- virStrerror(errno, ebuf, sizeof(ebuf)));
+ g_strerror(errno));
goto error;
}
if (virFileMakePath(cfg->autoDumpDir) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("failed to create dump dir '%s': %s"),
cfg->autoDumpDir,
- virStrerror(errno, ebuf, sizeof(ebuf)));
+ g_strerror(errno));
goto error;
}
if (virFileMakePath(cfg->channelDir) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("failed to create channel dir '%s': %s"),
cfg->channelDir,
- virStrerror(errno, ebuf, sizeof(ebuf)));
+ g_strerror(errno));
goto error;
}
diff --git a/src/libxl/libxl_logger.c b/src/libxl/libxl_logger.c
index 1581a3070c..379d7a8d00 100644
--- a/src/libxl/libxl_logger.c
+++ b/src/libxl/libxl_logger.c
@@ -67,7 +67,6 @@ libvirt_vmessage(xentoollog_logger *logger_in,
char timestamp[VIR_TIME_STRING_BUFLEN];
char *message = NULL;
char *start, *end;
-char ebuf[1024];
VIR_DEBUG("libvirt_vmessage: context='%s' format='%s'", context, format);
@@ -104,7 +103,7 @@ libvirt_vmessage(xentoollog_logger *logger_in,
fprintf(logFile, "%s", message);
if (errnoval >= 0)
-fprintf(logFile, ": %s", virStrerror(errnoval, ebuf, sizeof(ebuf)));
+fprintf(logFile, ": %s", g_strerror(errnoval));
fputc('\n', logFile);
fflush(logFile);
@@ -192,14 +191,13 @@ libxlLoggerOpenFile(libxlLoggerPtr logger,
char *path = NULL;
FILE *logFile = NULL;
char *domidstr = NULL;
-char ebuf[1024];
path = g_strdup_printf("%s/%s.log", logger->logDir, name);
domidstr = g_strdup_pri
[libvirt PATCH 0/4] Use g_strerror instead of virStrerror (glib chronicles)
Ján Tomko (4): Use g_strerror instead of virStrerror util: remove virStrerror tools: vsh.c: remove virstrerror.h include docs: hacking: move virStrerror to removed functions docs/hacking.html.in| 3 +-- src/libvirt_private.syms| 1 - src/libxl/libxl_conf.c | 3 +-- src/libxl/libxl_driver.c| 11 +-- src/libxl/libxl_logger.c| 6 ++ src/locking/lock_daemon.c | 9 +++-- src/logging/log_daemon.c| 9 +++-- src/lxc/lxc_container.c | 3 +-- src/lxc/lxc_process.c | 7 +++ src/network/bridge_driver.c | 3 +-- src/qemu/qemu_capabilities.c| 3 +-- src/qemu/qemu_interface.c | 3 +-- src/qemu/qemu_process.c | 9 +++-- src/remote/remote_daemon.c | 13 - src/security/security_manager.c | 5 ++--- src/security/security_selinux.c | 5 ++--- src/storage/storage_driver.c| 3 +-- src/util/viraudit.c | 3 +-- src/util/vircommand.c | 3 +-- src/util/virerror.c | 25 - src/util/virerror.h | 2 -- src/util/virfile.c | 3 +-- src/util/virhostcpu.c | 8 +++- src/util/virlockspace.c | 9 +++-- src/util/virpci.c | 12 src/util/virpidfile.c | 3 +-- src/util/virprocess.c | 3 +-- src/util/virutil.c | 6 ++ tools/vsh.c | 29 ++--- 29 files changed, 61 insertions(+), 141 deletions(-) -- 2.24.1
[libvirt PATCH 3/4] tools: vsh.c: remove virstrerror.h include
This was only used to pull in virStrerror. Signed-off-by: Ján Tomko --- tools/vsh.c | 1 - 1 file changed, 1 deletion(-) diff --git a/tools/vsh.c b/tools/vsh.c index 0488295c10..58bb1e6a3c 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -37,7 +37,6 @@ #endif #include "internal.h" -#include "virerror.h" #include "virbuffer.h" #include "viralloc.h" #include "virfile.h" -- 2.24.1
[libvirt PATCH 4/4] docs: hacking: move virStrerror to removed functions
Signed-off-by: Ján Tomko --- docs/hacking.html.in | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 94c74863b9..1756e84fc4 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -1070,8 +1070,6 @@ BAD: virAsprintfg_strdup_printf virVasprintfg_strdup_vprint use g_vasprintf if you really need to know the returned length -virStrerrorg_strerror -the error strings are cached globally so no need to free it @@ -1127,6 +1125,7 @@ BAD: ATTRIBUTE_UNUSEDG_GNUC_UNUSED VIR_STRDUPg_strdup VIR_STRNDUPg_strndup + virStrerrorg_strerror -- 2.24.1
Re: [PATCH v3 4/4] qemu-img: Deprecate use of -b without -F
On Tue, Mar 10, 2020 at 07:19:25AM -0500, Eric Blake wrote: > On 3/10/20 5:57 AM, Kashyap Chamarthy wrote: (Slightly long e-mail, as it contains a bunch of tests and their results; please bear with me.) [...] > > $> ~/build/qemu/qemu-img amend -o compat=v3 overlay1.qcow2 > > This particular amend is not changing the backing file, and since I did not > add warnings on the opening of an existing unsafe image, it is silent. I see; okay, that's expected. > Instead, you need to test a case where amend provokes a path that would > change the backing file (perhaps as simple as '-o backing_file=./base.raw'), > while omitting a format for the new backing file string. I couldn't work out the black magic to change the backing file via 'qemu-img amend'. It is surely not this: $> ~/build/qemu/qemu-img amend -o 'backing_file=./bar.qcow2' -o another_base.qcow2 qemu-img: Expecting one image file name Let's try something else: give a *non-existent* "bar.qcow2" to '-o': $> ~/build/qemu/qemu-img amend -o 'backing_file=./bar.qcow2' another_base.qcow2 qemu-img: Could not open 'another_base.qcow2': Could not open backing file: Failed to get shared "write" lock Is another process using the image [./another_base.qcow2]? That's strange; there is no live QEMU process on this host (let alone one that is using another_base.qcow2): $> pgrep qemu-system-x86 $> echo $? 1 Probably it is just complaning about the non-existent bar.qcow2 file? Then I'd expect it to say as much. On IRC you pointed out iotest 082 to look for help. There I don't see a way to change the backing file. But only a combination of 'amend' + 'rebase': run_qemu_img amend -f $IMGFMT \ -o backing_fmt=$IMGFMT,backing_file="$TEST_IMG",,\? "$TEST_IMG" run_qemu_img rebase -u -b "" -f $IMGFMT "$TEST_IMG" (I know you can use 'rebase' alone to change the backing file format.) Note to self: we really need to document 'amend' much better, in which scenarios it is useful, and contrast it with 'rebase'. - - - Meanwhile, I've done a bunch of tests with 'amend'. Here are the results. Scenario: base.raw <-- overlay1.qcow2 - Without "-f raw", the warning is provoked when trying to amend the backing file (let's ignore for a moment that you can't seem to amend a raw file): $> ~/build/qemu/qemu-img amend -o compat=v3 base.raw WARNING: Image format was not specified for 'base.raw' and probing guessed raw. Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted. Specify the 'raw' format explicitly to remove the restrictions. qemu-img: Format driver 'raw' does not support option amendment With "-f raw", the warning is not triggerred (correctly so?): $> ~/build/qemu/qemu-img amend -o compat=v3 -f raw base.raw qemu-img: Format driver 'raw' does not support option amendment And these two tests (one with relative path; the other with absolute path) don't trigger the warning either (on IRC you said the following is _supposed_ to trigger a warning): $> ~/build/qemu/qemu-img amend \ -o 'backing_file=base.raw' -f qcow2 overlay1.qcow2 $> ~/build/qemu/qemu-img amend \ -o 'backing_file=./base.raw' -f qcow2 overlay1.qcow2 'qemu-img info' of the above disk image chain: $> ~/build/qemu/qemu-img info --backing-chain overlay1.qcow2 image: overlay1.qcow2 file format: qcow2 virtual size: 4 GiB (4294967296 bytes) disk size: 196 KiB cluster_size: 65536 backing file: ./base.raw backing file format: raw Format specific information: compat: 1.1 lazy refcounts: false refcount bits: 16 corrupt: false image: ./base.raw file format: raw virtual size: 4 GiB (4294967296 bytes) disk size: 778 MiB Scenario: another_base.qcow2 <-- overlay1_of_ab.qcow2 - With and w/o specifying the aAmend the backing file (none of these provoke the warning -- expected?): $> ~/build/qemu/qemu-img amend another_base.qcow2 $> ~/build/qemu/qemu-img amend -f qcow2 another_base.qcow2 Tests to amend the overlay file (none of these provoke the warning -- expected, per your previous reply): $> ~/build/qemu/qemu-img amend overlay1_of_ab.qcow2 $> ~/build/qemu/qemu-img amend -f qcow2 overlay1_of_ab.qcow2 'qemu-img info' of the above disk image chain: $> ~/build/qemu/qemu-img info --backing-chain overlay1_of_ab.qcow2 image: overlay1_of_ab.qcow2 file format: qcow2 virtual size: 4 GiB (4294967296 bytes) disk size: 196 KiB cluster_size: 65536 backing file: ./another_base.qcow2 Format specific information: compat: 1.1 lazy refcounts: false refcount bits: 16 corrupt: false image: ./another_base.qcow2 file format: qcow2 virtual size:
Re: [PATCH v3 4/4] qemu-img: Deprecate use of -b without -F
On Tue, Mar 10, 2020 at 07:15:29AM -0500, Eric Blake wrote: > On 3/10/20 4:47 AM, Kashyap Chamarthy wrote: [...] > > > > > > Ah, didn't realize the inconsistency of 'convert' lacking the '-F' > > shorthand ... which reminds me, there are at least _three_ ways that I > > know of, to specify backing file format with 'create': > > > > $ qemu-img create -f qcow2 -o > > 'backing_file=./base.raw,backing_fmt=raw' ./overlay1.qcow2 > > $ qemu-img create -f qcow2 -b ./base.raw -o backing_fmt=raw > > overlay1.qcow2 > > $ qemu-img create -f qcow2 -b ./base.raw -F raw ./overlay1.qcow2 > > > > I'm wondering about the consistency of having all the above three > > supported for other operations too. Now I at least know 'convert' lacks > > the "-F". > > The -o forms (backing_file= and backing_fmt=) always work. Various commands > then have additional shorthand: -b/-F for create, -B for convert. You're > right that we aren't very consistent, but I'm reluctant to change the > inconsistencies in this patch Oh, I wasn't implying to tackle the inconsistency as part of this patch, or series. Hence the 'digression' :-) Was just wondering out loud. > (at one point in the past, we tried to get rid > of the shorthand and force all users to go through -o, but that broke too > many clients that were depending on the undocumented shorthand, so we > documented the existing shorthand instead). Fair enough; let's not touch these things for now. -- /kashyap
Re: qemu:///embed and isolation from global components
On Mon, 2020-03-09 at 18:04 +, Daniel P. Berrangé wrote: > On Mon, Mar 09, 2020 at 06:09:13PM +0100, Andrea Bolognani wrote: > > On Fri, 2020-03-06 at 17:49 +, Daniel P. Berrangé wrote: > > > On Fri, Mar 06, 2020 at 06:24:15PM +0100, Andrea Bolognani wrote: [...] > > Aside: instead of a per-VM setting, would it make sense for this to > > be a connection-level setting? That way, even on traditional libvirt > > deployments, the hypervisor admin could eg. opt out of machinectl > > registration if they so desire; at the same time, you'd avoid the > > situation where most VMs are confined using CGroups but a few are > > not, which is probably not a desirable scenario. > > Yes, functionally it would work fine as a connection level setting > too, though this hides the behaviour from the anyone looking at the > guest config. We've previously punted quite a few things to the > qemu.conf because we didn't want to go through process of representing > them in the domain XML. This was OK when the qemu.conf settings were > something done once at host deployment time. > > With the embedded driver, I think this is not so desirable, as means > to get the configuration they want from a VM, they need to deal with > two distinct config files. The ideal would be that everything that > is commonly needed can be achieved solely in the domain XML, and > I think resource control backend is one such common tunable. I don't have a strong opinion either way, and as far as my current use is concerned it doesn't bother me to have to deal with a second configuration file. The reason why I thought a per-VM setting might not be desirable is that applications would then be able to override it, and so maybe VMs created with virt-manager would be registered against machinectl but VMs created using GNOME Boxes would not, and if the sysadmin likes to use machinectl to get a comprehensive view of the system they'd no longer be guaranteed that. But if that's not the kind of scenario we think we should prevent, then a per-VM setting is fine by me :) [...] > > Right now we're already doing > > > > qemu-$domid-$domname > > > > where I'm not entirely sure how much $domid actually buys us. > > IIRC $domid was a hack because at one time we had problems with > systemd not cleaning up the transient scope when QEMU was killed. > This would prevent libvirt starting the guest again thereafter. > I can't remember now if this was a bug we fixed in systemd or > libvirt or both or neither. I see! It would be neat if we could get rid of it, assuming of course it's no longer needed on the platforms we target. [...] > > > Of course you can turn off virtlogd via qemu.conf > > > > That's what I'm doing right now and it works well enough, but I'm > > afraid that requiring a bunch of setup will discourage developers > > from using the embedded driver. We should aim for a reasonable out > > of the box experience instead. > > Why do you need to turn it off though ? It should already > "do the right thing" as the log files should appear under a > different location and not have any clash. Turning it off > immediately creates a denial of service CVE in your application. I was getting the same SELinux denial that Michal reported a few days back: virtlogd wants to verify it's being connected to by a process running as root, but it's only allowed by the policy to look into libvirtd's /proc/$pid for this information. So, for the same reason virtqemud can't currently connect to virtlogd when SELinux is in enforcing mode, neither can my qemu:///embed-using application. Besides that, there is the fact that a lot of people, mainly those coming from a containers background, are not happy with having extra daemons running. I'm not saying they would prefer being hit by a DoS than having virtlogd running :) but they really, really don't like daemons :) > None the less, as above I think we need common things to be > controllable via the domain XML. So either we need to make a > tunable there for use of logd or not, or we need to implement > the FIFO idea to avoid need for logd at all. It seems like the FIFO idea (though I'll admit I don't fully understand it) would be the best of both worlds. [...] > > > If you don't want to use virtnetworkd, then you won't be > > > creating such an config in the first place. > > > The app will have the option to open an embedded seconary > > > driver if desired. Some of the drivers won't really make > > > sense as embedded things though, at least not without > > > extra work. ie a embedded network or nwfilter driver has > > > no value unless your app has moved into a new network > > > namespace, as otherwise it will just fight with the > > > global network driver. > > > > Again, I think our defaults for qemu:///embed should be consistent > > and conservative: instead of having developers opt out of using > > network:///system, they should have to opt in before global > > resources are involved. > > They already opt-in t
[PATCH 3/5] qemuDomainBlockCopyCommon: Record updated flags to block job
For a long time we've masked out VIR_DOMAIN_BLOCK_COPY_SHALLOW if
there's no backing chain for the copied disk to simplify the code.
One of the refacors of caused that we no longer update the 'flags'
variable just the local copies. This was okay until in ccd4228afff I
started storing the job flags in the block job data.
Given that we modify how we call qemu we also should modify @flags so
that the correct value is recorded in the block job data.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_driver.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c0bf3c4eff..609b0b136d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17917,8 +17917,10 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
}
/* clear the _SHALLOW flag if there is only one layer */
-if (!virStorageSourceHasBacking(disk->src))
+if (!virStorageSourceHasBacking(disk->src)) {
+flags &= ~VIR_DOMAIN_BLOCK_COPY_SHALLOW;
mirror_shallow = false;
+}
if (qemuDomainBlockCopyCommonValidateUserMirrorBackingStore(mirror,
mirror_shallow,
--
2.24.1
[PATCH 1/5] qemu: capabilities: Update qemu-5.0.0 capabilities for x86_64
Currently upstream commit 7f368aed67211798 + patches from this series:
https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg02585.html
I'll update once more to pick up the proper upstream commit for the
patches mentioned above.
---
.../caps_5.0.0.x86_64.replies | 2668 +
.../caps_5.0.0.x86_64.xml |2 +-
2 files changed, 1368 insertions(+), 1302 deletions(-)
diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies
b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies
index 5ffa795108..7e6d711f96 100644
--- a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies
+++ b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.replies
@@ -21,7 +21,7 @@
"minor": 2,
"major": 4
},
-"package": "v4.2.0-1858-gdb736e0437"
+"package": "v4.2.0-2265-g67923a7ea6"
},
"id": "libvirt-2"
}
@@ -45,6 +45,9 @@
{
"return": [
+{
+ "name": "object-add"
+},
{
"name": "netdev_add"
},
@@ -201,9 +204,6 @@
{
"name": "object-del"
},
-{
- "name": "object-add"
-},
{
"name": "qom-list-properties"
},
@@ -433,28 +433,31 @@
"name": "job-pause"
},
{
- "name": "x-blockdev-set-iothread"
+ "name": "blockdev-snapshot-delete-internal-sync"
},
{
- "name": "x-blockdev-change"
+ "name": "blockdev-snapshot-internal-sync"
},
{
- "name": "block-set-write-threshold"
+ "name": "nbd-server-stop"
},
{
- "name": "blockdev-change-medium"
+ "name": "nbd-server-remove"
},
{
- "name": "blockdev-insert-medium"
+ "name": "nbd-server-add"
},
{
- "name": "blockdev-remove-medium"
+ "name": "nbd-server-start"
},
{
- "name": "blockdev-close-tray"
+ "name": "x-blockdev-set-iothread"
},
{
- "name": "blockdev-open-tray"
+ "name": "x-blockdev-change"
+},
+{
+ "name": "block-set-write-threshold"
},
{
"name": "blockdev-create"
@@ -492,9 +495,6 @@
{
"name": "block-stream"
},
-{
- "name": "block_set_io_throttle"
-},
{
"name": "blockdev-mirror"
},
@@ -565,25 +565,25 @@
"name": "block-latency-histogram-set"
},
{
- "name": "nbd-server-stop"
+ "name": "block_set_io_throttle"
},
{
- "name": "nbd-server-remove"
+ "name": "blockdev-change-medium"
},
{
- "name": "nbd-server-add"
+ "name": "blockdev-insert-medium"
},
{
- "name": "nbd-server-start"
+ "name": "blockdev-remove-medium"
},
{
- "name": "eject"
+ "name": "blockdev-close-tray"
},
{
- "name": "blockdev-snapshot-delete-internal-sync"
+ "name": "blockdev-open-tray"
},
{
- "name": "blockdev-snapshot-internal-sync"
+ "name": "eject"
},
{
"name": "query-pr-managers"
@@ -782,14 +782,14 @@
"name": "Icelake-Client-x86_64-cpu",
"parent": "x86_64-cpu"
},
-{
- "name": "Westmere-v1-x86_64-cpu",
- "parent": "x86_64-cpu"
-},
{
"name": "chardev-wctablet",
"parent": "chardev"
},
+{
+ "name": "Westmere-v1-x86_64-cpu",
+ "parent": "x86_64-cpu"
+},
{
"name": "Opteron_G5-v1-x86_64-cpu",
"parent": "x86_64-cpu"
@@ -982,14 +982,14 @@
"name": "ipmi-bmc-extern",
"parent": "ipmi-bmc"
},
-{
- "name": "pc-q35-4.0-machine",
- "parent": "generic-pc-machine"
-},
{
"name": "authz-list-file",
"parent": "authz"
},
+{
+ "name": "pc-q35-4.0-machine",
+ "parent": "generic-pc-machine"
+},
{
"name": "usb-audio",
"parent": "usb-device"
@@ -1386,6 +1386,10 @@
"name": "System",
"parent": "bus"
},
+{
+ "name": "virtio-iommu-pci-transitional",
+ "parent": "virtio-iommu-device-base"
+},
{
"name": "pc-1.0-machine",
"parent": "generic-pc-machine"
@@ -1714,14 +1718,14 @@
"name": "Snowridge-v2-x86_64-cpu",
"parent": "x86_64-cpu"
},
-{
- "name": "vhost-user-scsi-pci",
- "parent": "vhost-user-scsi-pci-base"
-},
{
"name": "kvaser_pci",
"parent": "pci-device"
},
+{
+ "name": "vhost-user-scsi-pci",
+ "parent": "vhost-user-scsi-pci-base"
+},
{
"name": "i82559a",
"parent": "pci-device"
@@ -1786,6 +1790,10 @@
"name": "Skylake-Server-v2-x86_64-cpu",
"parent": "x86_64-cpu"
},
+{
+ "name": "qio-net-listener",
+ "parent": "object"
+},
{
"name": "pc-q35-4.1-machine",
"parent": "generic-pc-machine"
@@ -1807,8 +1815,8 @@
"parent": "x86_64-cpu"
},
{
- "name": "qio-net-listener",
- "parent": "object"
+ "name": "qio-channel-websock",
+ "parent": "qio-channel"
},
{
"name
[PATCH 0/5] qemu: Allow late opening of backing chain on a shallow block-copy
See patch 5/5 for explanation. As described in patch 1/5 this is based on patches which are not yet pushed in qemu. Peter Krempa (5): qemu: capabilities: Update qemu-5.0.0 capabilities for x86_64 qemuDomainBlockPivot: Move check prior to executing the pivot steps qemuDomainBlockCopyCommon: Record updated flags to block job qemu: capabilities: Introduce QEMU_CAPS_BLOCKDEV_SNAPSHOT_ALLOW_WRITE_ONLY qemu: blockcopy: Allow late opening of the backing chain of a shallow copy src/qemu/qemu_capabilities.c |2 + src/qemu/qemu_capabilities.h |1 + src/qemu/qemu_driver.c| 75 +- .../caps_5.0.0.x86_64.replies | 2668 + .../caps_5.0.0.x86_64.xml |3 +- 5 files changed, 1435 insertions(+), 1314 deletions(-) -- 2.24.1
[PATCH 2/5] qemuDomainBlockPivot: Move check prior to executing the pivot steps
Move the check whether the job is already synchronised to the beginning
of the function so that we don't try to do some of the steps necessary
for pivoting prior to actually wanting to pivot.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_driver.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index cd761f87b5..c0bf3c4eff 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17236,6 +17236,13 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver,
bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV);
g_autoptr(virJSONValue) actions = NULL;
+if (job->state != QEMU_BLOCKJOB_STATE_READY) {
+virReportError(VIR_ERR_BLOCK_COPY_ACTIVE,
+ _("block job '%s' not ready for pivot yet"),
+ job->name);
+return -1;
+}
+
switch ((qemuBlockJobType) job->type) {
case QEMU_BLOCKJOB_TYPE_NONE:
case QEMU_BLOCKJOB_TYPE_LAST:
@@ -17273,13 +17280,6 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver,
break;
}
-if (job->state != QEMU_BLOCKJOB_STATE_READY) {
-virReportError(VIR_ERR_BLOCK_COPY_ACTIVE,
- _("block job '%s' not ready for pivot yet"),
- job->name);
-return -1;
-}
-
qemuDomainObjEnterMonitor(driver, vm);
if (blockdev) {
int rc = 0;
--
2.24.1
[PATCH 5/5] qemu: blockcopy: Allow late opening of the backing chain of a shallow copy
oVirt used a quirk in the pre-blockdev semantics of drive-mirror which
opened the backing chain of the mirror destination only once
'block-job-complete' was called.
Our introduction of blockdev made qemu open the backing chain images
right at the start of the job. This broke oVirt's usage of this API
because they copy the data into the backing chain during the time the
block copy job is running.
Re-introduce late open of the backing chain if qemu allows to use
blockdev-snapshot on write-only nodes as it can be used to install the
backing chain even for an existing image now.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_driver.c | 57 +++---
1 file changed, 53 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 609b0b136d..d565054436 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17231,10 +17231,12 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver,
qemuBlockJobDataPtr job,
virDomainDiskDefPtr disk)
{
+g_autoptr(qemuBlockStorageSourceChainData) chainattachdata = NULL;
int ret = -1;
qemuDomainObjPrivatePtr priv = vm->privateData;
bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV);
g_autoptr(virJSONValue) actions = NULL;
+g_autoptr(virJSONValue) reopenactions = NULL;
if (job->state != QEMU_BLOCKJOB_STATE_READY) {
virReportError(VIR_ERR_BLOCK_COPY_ACTIVE,
@@ -17265,6 +17267,7 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver,
if (blockdev && !job->jobflagsmissing) {
g_autoptr(virHashTable) blockNamedNodeData = NULL;
bool shallow = job->jobflags & VIR_DOMAIN_BLOCK_COPY_SHALLOW;
+bool reuse = job->jobflags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT;
if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm,
QEMU_ASYNC_JOB_NONE)))
return -1;
@@ -17273,6 +17276,27 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver,
blockNamedNodeData,
shallow, &actions) < 0)
return -1;
+
+/* Open and install the backing chain of 'mirror' late if we can
use
+ * blockdev-snapshot to do it. This is to appease oVirt that wants
+ * to copy data into the backing chain while the top image is being
+ * copied shallow */
+if (reuse && shallow &&
+virQEMUCapsGet(priv->qemuCaps,
QEMU_CAPS_BLOCKDEV_SNAPSHOT_ALLOW_WRITE_ONLY) &&
+virStorageSourceHasBacking(disk->mirror)) {
+
+if (!(chainattachdata =
qemuBuildStorageSourceChainAttachPrepareBlockdev(disk->mirror->backingStore,
+
priv->qemuCaps)))
+return -1;
+
+reopenactions = virJSONValueNewArray();
+
+if (qemuMonitorTransactionSnapshotBlockdev(reopenactions,
+
disk->mirror->backingStore->nodeformat,
+
disk->mirror->nodeformat))
+return -1;
+}
+
}
break;
@@ -17284,7 +17308,15 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver,
if (blockdev) {
int rc = 0;
-if (actions)
+if (chainattachdata) {
+if ((rc = qemuBlockStorageSourceChainAttach(priv->mon,
chainattachdata)) == 0) {
+/* install backing images on success, or unplug them on
failure */
+if ((rc = qemuMonitorTransaction(priv->mon, &reopenactions))
!= 0)
+qemuBlockStorageSourceChainDetach(priv->mon,
chainattachdata);
+}
+}
+
+if (actions && rc == 0)
rc = qemuMonitorTransaction(priv->mon, &actions);
if (rc == 0)
@@ -18029,9 +18061,26 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
if (blockdev) {
if (mirror_reuse) {
-if (!(data =
qemuBuildStorageSourceChainAttachPrepareBlockdev(mirror,
-
priv->qemuCaps)))
-goto endjob;
+/* oVirt depended on late-backing-chain-opening semantics the old
+ * qemu command had to copy the backing chain data while the top
+ * level is being copied. To restore this semantics if
+ * blockdev-reopen is supported defer opening of the backing chain
+ * of 'mirror' to the pivot step */
+if (virQEMUCapsGet(priv->qemuCaps,
QEMU_CAPS_BLOCKDEV_SNAPSHOT_ALLOW_WRITE_ONLY)) {
+g_autoptr(virStorageSource) terminator = virStorageSourceNew();
+
+if (!terminator)
+goto endjob;
+
+if (!(data =
qemuBuildStorageSourceChainAttachPrepareBlockde
[PATCH 4/5] qemu: capabilities: Introduce QEMU_CAPS_BLOCKDEV_SNAPSHOT_ALLOW_WRITE_ONLY
The capability is based on qemu's support of using blockdev-snapshot to
install backing chain also for images which are in use by a block-copy
job.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_capabilities.c | 2 ++
src/qemu/qemu_capabilities.h | 1 +
tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml | 1 +
3 files changed, 4 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index a75ca0574d..25a77c24af 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -565,6 +565,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
/* 355 */
"vhost-user-fs",
"query-named-block-nodes.flat",
+ "blockdev-snapshot.allow-write-only-overlay",
);
@@ -1442,6 +1443,7 @@ static struct virQEMUCapsStringFlags
virQEMUCapsQMPSchemaQueries[] = {
{ "human-monitor-command/$savevm-monitor-nodes",
QEMU_CAPS_SAVEVM_MONITOR_NODES },
{ "blockdev-add/arg-type/+nvme", QEMU_CAPS_DRIVE_NVME },
{ "query-named-block-nodes/arg-type/flat",
QEMU_CAPS_QMP_QUERY_NAMED_BLOCK_NODES_FLAT },
+{ "blockdev-snapshot/$allow-write-only-overlay",
QEMU_CAPS_BLOCKDEV_SNAPSHOT_ALLOW_WRITE_ONLY },
};
typedef struct _virQEMUCapsObjectTypeProps virQEMUCapsObjectTypeProps;
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 8b6145c327..e952fcb6b8 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -546,6 +546,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
syntax-check */
/* 355 */
QEMU_CAPS_DEVICE_VHOST_USER_FS, /* -device vhost-user-fs */
QEMU_CAPS_QMP_QUERY_NAMED_BLOCK_NODES_FLAT, /* query-named-block-nodes
supports the 'flat' option */
+QEMU_CAPS_BLOCKDEV_SNAPSHOT_ALLOW_WRITE_ONLY, /* blockdev-snapshot has the
'allow-write-only-overlay' feature */
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml
index 5b8005dd16..3a3c9b1363 100644
--- a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml
@@ -225,6 +225,7 @@
+
4002050
0
43100241
--
2.24.1
Re: qemu:///embed and isolation from global components
On Tue, Mar 10, 2020 at 04:42:57PM +0100, Andrea Bolognani wrote: > On Mon, 2020-03-09 at 18:04 +, Daniel P. Berrangé wrote: > > Of course when we do connect to virnetworkd, we MUST ensure that > > anything we do preserves isolation from other QEMU driver instances. > > > > I would also note that the virtnetworkd daemon connected to, may > > or may not actually be the same as the host OS one. It is entirely > > possible that the application has opened the embedded QEMU driver > > from within a namesace, that results in a connection to the > > /var/run/libvirt/virnetworkd being serviced by a completely isolated > > virnetworkd running inside a different network namespace from the > > host OS virtnetworkd. This is of no concern to the QEMU driver > > though - it just needs to honour what is in the domain XML. > > This kind of setup sounds extremely confusing. > > Would you have multiple instances of virtnetworkd, one per network > namespace, all running at the same time? How would the application > pick the correct one to connect to? I forgot to mention that this is actually a scenario we'd like to support even ignoring namespaces. The big pain point for desktop virt apps like Boxes or Virt-manager is that the QEMU session driver lacked any sane networking connectivity. For this reason we invented the QEMU setuid network helper which runs as root and opens a TAP device connected to a bridge, passing it back to unprivileged libvirtd. This is really crude and not a general solution to the problem. The key design flaw in session libvirtd, was tieing together the privileges of the virt driver and the secondary drivers. So we had a 1-1 relation between them. What we really need to have is a N-N relation between them. The virtual network driver as it exists today, requires elevated privileges, but if we had a NetworkManager backed impl it could work unprivileged. The nwfilter driver requires privileges. The storage driver is a little different because some backends (directory backend) work when unprivileged, but other backends (SCSI, LVM, disk) only ever work when privileged. The split daemon model is intended to allow us to address this long standing design flaw, by allowing the QEMU session driver to optionally talk to a secondary driver running with different privileges, instead of the instance running with the same privs. So currently we have This refers to a secondary driver running at the same privilege level. I expected we'd extend it to allow This explicitly requests the secondary driver running with elevated privileges. The same concept for disk storage Would be extended to allow The same for host devices, and for nwfilter. With such functionality present, it logically then also extends to cover connections to daemons running in different namespaces. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 0/9] Second take on slirp-helper & dbus-vmstate
On Tue, Feb 25, 2020 at 10:55 AM wrote: > > From: Marc-André Lureau > > Hi, > > The series "[libvirt] [PATCH v2 00/23] Use a slirp helper process" has > been merged and partially reverted. Meanwhile, qemu dbus-vmstate > design has been changed and merged upstream. > > This new series fixes the slirp-helper support. The significant change > is that dbus-vmstate now requires a bus (instead of the earlier > peer-to-peer connection). The current series doesn't attempt to > enforce strict policies on the bus. As long as you can connect to the > bus, you can send/receive from/to anyone. A follow-up series should > implement the recommendations from > https://qemu.readthedocs.io/en/latest/interop/dbus.html#security. > > The libslirp-rs slirp-helper hasn't yet received an official release. > For testing, you may: > $ cargo install --features=all --git > https://gitlab.freedesktop.org/slirp/libslirp-rs > > The resulting binary should be ~/.cargo/bin/slirp-helper, so qemu.conf > slirp_helper location should be adjusted. With that in place, a VM > with user networking (slirp) should now start with the helper process. > > thanks > > v2: > - merge most suggestions/changes from Michal Privoznik review of v1. > - added "WIP: qemu_slirp: update to follow current spec" > > Marc-André Lureau (9): > qemu: remove dbus-vmstate code > qemu-conf: add configurable dbus-daemon location > qemu-conf: add dbusStateDir > qemu: add a DBus daemon helper unit > domain: save/restore the state of dbus-daemon running > qemu: prepare and stop the dbus daemon > qemu: add dbus-vmstate helper migration support > qemu-slirp: register helper for migration > WIP: qemu-slirp: update to follow current spec > ping ? thanks > m4/virt-driver-qemu.m4 | 6 + > src/qemu/libvirtd_qemu.aug | 1 + > src/qemu/qemu.conf | 3 + > src/qemu/qemu_alias.c | 17 +- > src/qemu/qemu_alias.h | 3 +- > src/qemu/qemu_command.c| 81 +++-- > src/qemu/qemu_command.h| 6 +- > src/qemu/qemu_conf.c | 7 + > src/qemu/qemu_conf.h | 2 + > src/qemu/qemu_dbus.c | 264 + > src/qemu/qemu_dbus.h | 25 ++- > src/qemu/qemu_domain.c | 30 ++-- > src/qemu/qemu_domain.h | 8 +- > src/qemu/qemu_extdevice.c | 4 +- > src/qemu/qemu_hotplug.c| 165 +- > src/qemu/qemu_hotplug.h| 17 +- > src/qemu/qemu_migration.c | 57 ++- > src/qemu/qemu_monitor.c| 21 +++ > src/qemu/qemu_monitor.h| 3 + > src/qemu/qemu_monitor_json.c | 15 ++ > src/qemu/qemu_monitor_json.h | 5 + > src/qemu/qemu_process.c| 6 + > src/qemu/qemu_slirp.c | 157 +++-- > src/qemu/qemu_slirp.h | 4 +- > src/qemu/test_libvirtd_qemu.aug.in | 1 + > 25 files changed, 544 insertions(+), 364 deletions(-) > > -- > 2.25.0.rc2.1.g09a9a1a997 > -- Marc-André Lureau
Re: [RFC] qemu: convert DomainLogContext class to use GObject
On Tue, Mar 10, 2020 at 02:02:47PM +0100, Peter Krempa wrote: > On Tue, Mar 10, 2020 at 12:53:59 +, Daniel Berrange wrote: > > On Tue, Mar 10, 2020 at 01:48:39PM +0100, Peter Krempa wrote: > > > On Tue, Mar 10, 2020 at 12:42:47 +, Daniel Berrange wrote: > > > > On Tue, Mar 10, 2020 at 01:33:42PM +0100, Peter Krempa wrote: > > > > > On Tue, Mar 10, 2020 at 17:30:01 +0530, Gaurav Agrawal wrote: > > > > > > --- > > > > > > src/qemu/qemu_domain.c | 36 > > > > > > src/qemu/qemu_domain.h | 6 -- > > > > > > src/qemu/qemu_process.c | 4 ++-- > > > > > > 3 files changed, 26 insertions(+), 20 deletions(-) > > [...] > > > > > > > > > > > There's no reason to break coding style rules in this kind of refactor > > > > > nor to make it inconsistent in span of 20 lines. > > > > > > > > If you're refering to the use of underscores, then this is expected > > > > and indeed required because these method names are auto-generated > > > > by the GObject macros. This should only be the case for three > > > > specific methods (_init, _class_init and _get_type), with the rest > > > > all following normal style. See the virIdentity conversion for > > > > the prior example of this. > > > > > > Yeah, I meant mainly that (also the newline after the type). > > > > > > Can't we at least keep camel-case for the type itself? > > > > > > 'qemuDomainLogContext_init' > > > > > > That way at least looking for the symbol name will be less painful. > > > > We had considered that in the original virIdentity conversion, but > > decided mixing two different naming styles in one identifier was > > even more ugly. > > While ugly I still think that for greppability/searchability it's way > better to keep the object name using camel case. In the end as you've > pointed out it's just for the specific methods named above. Making it > look ugly is IMO less worse than missing something when trying to > understand how the code works. IIRC, Ján raised the issue of avoiding mixed camelcase/snakecase namings originally, so CC'ing to see if he's ok with changing back again ? Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 03/13] qemu: blockjob: Store list of bitmaps disabled prior to commit
On Thu, Mar 05, 2020 at 10:43:15 +0100, Pavel Mores wrote:
> On Wed, Mar 04, 2020 at 06:26:31PM +0100, Peter Krempa wrote:
> > Starting a commit job will require disabling bitmaps in the base image
> > so that they are not dirtied by the commit job. We need to store a list
> > of the bitmaps so that we can later re-enable them.
> >
> > Add a field and status XML handling code as well as a test.
> >
> > Signed-off-by: Peter Krempa
> > ---
> > src/qemu/qemu_blockjob.h | 2 ++
> > src/qemu/qemu_domain.c| 26 +++
> > .../blockjob-blockdev-in.xml | 4 +++
> > 3 files changed, 32 insertions(+)
> >
> > diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h
> > index 72c7fa053e..e2e28ca4d3 100644
> > --- a/src/qemu/qemu_blockjob.h
> > +++ b/src/qemu/qemu_blockjob.h
> > @@ -88,6 +88,8 @@ struct _qemuBlockJobCommitData {
> > virStorageSourcePtr top;
> > virStorageSourcePtr base;
> > bool deleteCommittedImages;
> > +char **disabledBitmapsBase; /* a NULL-terminated list of bitmap names
> > which
> > + were disabled in @base for the commit
> > job */
> > };
>
> Is it guaranteed that the new member 'disabledBitmapsBase' is properly
> initialised? How about something along the lines of
>
>
> diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
> index 2e53821d43..f9fc83430e 100644
> --- a/src/qemu/qemu_blockjob.c
> +++ b/src/qemu/qemu_blockjob.c
> @@ -312,6 +312,7 @@ qemuBlockJobDiskNewCommit(virDomainObjPtr vm,
> job->data.commit.top = top;
> job->data.commit.base = base;
> job->data.commit.deleteCommittedImages = delete_imgs;
> +job->data.commit.disabledBitmapsBase = NULL;
> job->jobflags = jobflags;
>
> if (qemuBlockJobRegister(job, vm, disk, true) < 0)
>
>
> Even if explicit initialisation is somehow not needed here, I'd consider that
> fact worth a line of explanation in the commit message.
g_new0 and VIR_ALLOC (which uses calloc) always zero-out the memory
which is allocated. This means that setting integers to 0, pointers to
NULL and booleans to false is always assumed when allocating a new
object and we rarely re-initialize it explicitly if ever.
Re: [RFC] qemu: convert DomainLogContext class to use GObject
On a Tuesday in 2020, Gaurav Agrawal wrote: --- src/qemu/qemu_domain.c | 36 src/qemu/qemu_domain.h | 6 -- src/qemu/qemu_process.c | 4 ++-- 3 files changed, 26 insertions(+), 20 deletions(-) [...] @@ -10632,7 +10635,8 @@ qemuDomainLogContextPtr qemuDomainLogContextNew(virQEMUDriverPtr driver, return ctxt; error: -virObjectUnref(ctxt); +if (ctxt) +g_object_unref(ctxt); g_object_unref is safe to call with a NULL argument, the "if (ctxt)" check is not needed here. return NULL; } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 3929ee9ca1..3c270b87a2 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -37,6 +37,8 @@ #include "virmdev.h" #include "virchrdev.h" #include "virobject.h" +#include "internal.h" The "internal.h" addition is not necessary for this patch - all the types are in glib-object. In the virIdentity conversion, other parts of the include file relied on "internal.h" being included indirectly through "virobject.h". +#include Please put the includes in angle brackets at the beginning of the file: https://libvirt.org/hacking.html#includes Otherwise the patch looks good to me. Jano #include "logging/log_manager.h" #include "virdomainmomentobjlist.h" #include "virenum.h" signature.asc Description: PGP signature
Re: [RFC] qemu: convert DomainLogContext class to use GObject
On a Tuesday in 2020, Daniel P. Berrangé wrote: On Tue, Mar 10, 2020 at 02:02:47PM +0100, Peter Krempa wrote: On Tue, Mar 10, 2020 at 12:53:59 +, Daniel Berrange wrote: > On Tue, Mar 10, 2020 at 01:48:39PM +0100, Peter Krempa wrote: > > On Tue, Mar 10, 2020 at 12:42:47 +, Daniel Berrange wrote: > > > On Tue, Mar 10, 2020 at 01:33:42PM +0100, Peter Krempa wrote: > > > > On Tue, Mar 10, 2020 at 17:30:01 +0530, Gaurav Agrawal wrote: > > > > > --- > > > > > src/qemu/qemu_domain.c | 36 > > > > > src/qemu/qemu_domain.h | 6 -- > > > > > src/qemu/qemu_process.c | 4 ++-- > > > > > 3 files changed, 26 insertions(+), 20 deletions(-) [...] > > > > > > > > There's no reason to break coding style rules in this kind of refactor > > > > nor to make it inconsistent in span of 20 lines. > > > > > > If you're refering to the use of underscores, then this is expected > > > and indeed required because these method names are auto-generated > > > by the GObject macros. This should only be the case for three > > > specific methods (_init, _class_init and _get_type), with the rest > > > all following normal style. See the virIdentity conversion for > > > the prior example of this. > > > > Yeah, I meant mainly that (also the newline after the type). > > > > Can't we at least keep camel-case for the type itself? > > > > 'qemuDomainLogContext_init' > > > > That way at least looking for the symbol name will be less painful. > > We had considered that in the original virIdentity conversion, but > decided mixing two different naming styles in one identifier was > even more ugly. While ugly I still think that for greppability/searchability it's way better to keep the object name using camel case. In the end as you've pointed out it's just for the specific methods named above. Making it look ugly is IMO less worse than missing something when trying to understand how the code works. IIRC, Ján raised the issue of avoiding mixed camelcase/snakecase namings originally, so CC'ing to see if he's ok with changing back again ? I don't care about it enough to send another e-mail after this one :) However the case convetion is a part of the macro documentation: https://developer.gnome.org/gobject/unstable/gobject-Type-Information.html#G-DEFINE-TYPE:CAPS Jano Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| signature.asc Description: PGP signature
Re: [RFC] qemu: convert DomainLogContext class to use GObject
On Tue, Mar 10, 2020 at 06:20:49PM +0100, Ján Tomko wrote: > On a Tuesday in 2020, Gaurav Agrawal wrote: > > --- > > src/qemu/qemu_domain.c | 36 > > src/qemu/qemu_domain.h | 6 -- > > src/qemu/qemu_process.c | 4 ++-- > > 3 files changed, 26 insertions(+), 20 deletions(-) > > > > [...] > > > @@ -10632,7 +10635,8 @@ qemuDomainLogContextPtr > > qemuDomainLogContextNew(virQEMUDriverPtr driver, > > return ctxt; > > > > error: > > -virObjectUnref(ctxt); > > +if (ctxt) > > +g_object_unref(ctxt); > > g_object_unref is safe to call with a NULL argument, the "if (ctxt)" > check is not needed here. I'm not so sure on that. g_clear_object API docs explicitly say that it is OK if the object is NULL: https://developer.gnome.org/gobject/stable/gobject-The-Base-Object-Type.html#g-clear-object but the g_object_unref docs are completely silent on this matter: https://developer.gnome.org/gobject/stable/gobject-The-Base-Object-Type.html#g-object-ref Thus I've always assumed a NULL check was required for g_object_unref Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
[libvirt PATCH 1/3] rpc: fix dispatch for node device APIs for virt drivers
Despite their names, the following APIs:
virNodeDeviceDettach
virNodeDeviceDetachFlags
virNodeDeviceReAttach
virNodeDeviceReset
are all handled by the virt drivers, not the node device driver.
A bug in the RPC generator meant that these APIs were sent to
the nodedev driver for handling. This caused breakage with the
split daemons, since nothing was available to process them.
Signed-off-by: Daniel P. Berrangé
---
src/rpc/gendispatch.pl | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl
index 987a136566..c140ed712c 100755
--- a/src/rpc/gendispatch.pl
+++ b/src/rpc/gendispatch.pl
@@ -171,7 +171,13 @@ sub get_conn_method {
if ($proc =~ /Connect.*Network/) {
return "remoteGetNetworkConn";
}
-if ($proc =~ /Node.*Device/) {
+# Carefully whitelist a few APIs with NodeDevice name
+# prefix which actually get handled by the virt drivers
+if ($proc =~ /Node.*Device/ &&
+!($proc =~ /NodeDeviceReset/ ||
+ $proc =~ /NodeDeviceReAttach/ ||
+ $proc =~ /NodeDeviceDettach/ ||
+ $proc =~ /NodeDeviceDetachFlags/)) {
return "remoteGetNodeDevConn";
}
if ($proc =~ /Connect.*NWFilter/) {
--
2.24.1
[libvirt PATCH 2/3] rpc: avoid name lookup when dispatching node device APIs
The node device APIs are a little unusual because we don't use a
"remote_nonnull_node_device" object on the wire, instead we just
have a "remote_string" for the device name. This meant dispatcher
code generation needed special cases. In doing so we mistakenly
used the virNodeDeviceLookupByName() API which gets dispatched
into the driver, instead of get_nonnull_node_device() which
directly populates a virNodeDevicePtr object.
This wasn't a problem with monolithic libvirtd, as the
virNodeDeviceLookupByName() API call was trivially satisfied
by the registered driver, albeit with an extra (undesirable)
authentication check. With the split daemons, the call to
virNodeDeviceLookupByName() fails in virtqemud, because the
node device driver obviously doesn't exist in that daemon.
Signed-off-by: Daniel P. Berrangé
---
src/remote/remote_daemon_dispatch.c | 7 +++
src/rpc/gendispatch.pl | 2 +-
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/remote/remote_daemon_dispatch.c
b/src/remote/remote_daemon_dispatch.c
index 2741a32f63..226049fed6 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -95,6 +95,7 @@ static virNWFilterBindingPtr
get_nonnull_nwfilter_binding(virConnectPtr conn, re
static virDomainCheckpointPtr get_nonnull_domain_checkpoint(virDomainPtr dom,
remote_nonnull_domain_checkpoint checkpoint);
static virDomainSnapshotPtr get_nonnull_domain_snapshot(virDomainPtr dom,
remote_nonnull_domain_snapshot snapshot);
static virNodeDevicePtr get_nonnull_node_device(virConnectPtr conn,
remote_nonnull_node_device dev);
+static virNodeDevicePtr get_nonnull_node_device_name(virConnectPtr conn,
remote_nonnull_string devdev);
static void make_nonnull_domain(remote_nonnull_domain *dom_dst, virDomainPtr
dom_src);
static void make_nonnull_network(remote_nonnull_network *net_dst,
virNetworkPtr net_src);
static void make_nonnull_network_port(remote_nonnull_network_port *port_dst,
virNetworkPortPtr port_src);
@@ -7291,6 +7292,12 @@ get_nonnull_node_device(virConnectPtr conn,
remote_nonnull_node_device dev)
return virGetNodeDevice(conn, dev.name);
}
+static virNodeDevicePtr
+get_nonnull_node_device_name(virConnectPtr conn, remote_nonnull_string devname)
+{
+return virGetNodeDevice(conn, devname);
+}
+
static void
make_nonnull_domain(remote_nonnull_domain *dom_dst, virDomainPtr dom_src)
{
diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl
index c140ed712c..0b2ae59910 100755
--- a/src/rpc/gendispatch.pl
+++ b/src/rpc/gendispatch.pl
@@ -571,7 +571,7 @@ elsif ($mode eq "server") {
$has_node_device = 1;
push(@vars_list, "virNodeDevicePtr dev = NULL");
push(@getters_list,
- "if (!(dev = virNodeDeviceLookupByName($conn_var,
args->name)))\n" .
+ "if (!(dev = get_nonnull_node_device_name($conn_var,
args->name)))\n" .
"goto cleanup;\n");
push(@args_list, "dev");
push(@free_list,
--
2.24.1
[libvirt PATCH 0/3] nodedev: fix device reset/detach/reattach with split daemons
The virNodeDevice{Reset,ReAttach,Dettach} methods are a little
unusual because they are registered by the virt driver, but
use the virNodeDevicePtr object associated with the nodedev
driver.
In the split daemon world, we tried to dispatch them to the
virnodedevd daemon which has no impl registered. Dispatching
them to the virt driver has some quirks, we need to ensure
we have two virNodeDevicePtr instances, once for the virConnectPtr
of the virt driver and one for the virConnectPtr of the nodedev
driver.
Daniel P. Berrangé (3):
rpc: fix dispatch for node device APIs for virt drivers
rpc: avoid name lookup when dispatching node device APIs
qemu: lookup node device against nodedev driver before getting XML
src/libxl/libxl_driver.c| 63 +++--
src/qemu/qemu_driver.c | 63 +++--
src/remote/remote_daemon_dispatch.c | 7
src/rpc/gendispatch.pl | 10 -
4 files changed, 135 insertions(+), 8 deletions(-)
--
2.24.1
[libvirt PATCH 3/3] qemu: lookup node device against nodedev driver before getting XML
Some of the node device APIs are a little odd because they accept a virNodeDevicePtr object but are still implemented by the virt drivers. The first thing the virt drivers need to do is get the XML config associated with the node device, and that means talking to the node device driver. This worked previously because with monolithic libvirtd, both the virt driver and node device driver were in the same daemon and thus a single virConnectPtr can talk to both drivers. With the split daemon world though, the virNodeDevicePtr passed into the APIs is associated with the QEMU driver virConnectPtr, which has no ability to invoke APIs against the node device driver. We must thus get a duplicate virNodeDevicePtr object which is associated with a virConnectPtr for the node device driver. Signed-off-by: Daniel P. Berrangé --- src/libxl/libxl_driver.c | 63 ++-- src/qemu/qemu_driver.c | 63 ++-- 2 files changed, 120 insertions(+), 6 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index f2387e2a20..eca45da097 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -5787,10 +5787,23 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev, char *xml = NULL; libxlDriverPrivatePtr driver = dev->conn->privateData; virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; +virConnectPtr nodeconn = NULL; +virNodeDevicePtr nodedev = NULL; virCheckFlags(0, -1); -xml = virNodeDeviceGetXMLDesc(dev, 0); +if (!(nodeconn = virGetConnectNodeDev())) +goto cleanup; + +/* 'dev' is associated with the QEMU virConnectPtr, + * so for split daemons, we need to get a copy that + * is associated with the virnodedevd daemon. + */ +if (!(nodedev = virNodeDeviceLookupByName( + nodeconn, virNodeDeviceGetName(dev +goto cleanup; + +xml = virNodeDeviceGetXMLDesc(nodedev, 0); if (!xml) goto cleanup; @@ -5798,6 +5811,8 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev, if (!def) goto cleanup; +/* ACL check must happen against original 'dev', + * not the new 'nodedev' we acquired */ if (virNodeDeviceDetachFlagsEnsureACL(dev->conn, def) < 0) goto cleanup; @@ -5823,6 +5838,10 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev, cleanup: virPCIDeviceFree(pci); virNodeDeviceDefFree(def); +if (nodedev) +virNodeDeviceFree(nodedev); +if (nodeconn) +virConnectClose(nodeconn); VIR_FREE(xml); return ret; } @@ -5843,8 +5862,21 @@ libxlNodeDeviceReAttach(virNodeDevicePtr dev) char *xml = NULL; libxlDriverPrivatePtr driver = dev->conn->privateData; virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; +virConnectPtr nodeconn = NULL; +virNodeDevicePtr nodedev = NULL; + +if (!(nodeconn = virGetConnectNodeDev())) +goto cleanup; -xml = virNodeDeviceGetXMLDesc(dev, 0); +/* 'dev' is associated with the QEMU virConnectPtr, + * so for split daemons, we need to get a copy that + * is associated with the virnodedevd daemon. + */ +if (!(nodedev = virNodeDeviceLookupByName( + nodeconn, virNodeDeviceGetName(dev +goto cleanup; + +xml = virNodeDeviceGetXMLDesc(nodedev, 0); if (!xml) goto cleanup; @@ -5852,6 +5884,8 @@ libxlNodeDeviceReAttach(virNodeDevicePtr dev) if (!def) goto cleanup; +/* ACL check must happen against original 'dev', + * not the new 'nodedev' we acquired */ if (virNodeDeviceReAttachEnsureACL(dev->conn, def) < 0) goto cleanup; @@ -5870,6 +5904,10 @@ libxlNodeDeviceReAttach(virNodeDevicePtr dev) cleanup: virPCIDeviceFree(pci); virNodeDeviceDefFree(def); +if (nodedev) +virNodeDeviceFree(nodedev); +if (nodeconn) +virConnectClose(nodeconn); VIR_FREE(xml); return ret; } @@ -5884,8 +5922,21 @@ libxlNodeDeviceReset(virNodeDevicePtr dev) char *xml = NULL; libxlDriverPrivatePtr driver = dev->conn->privateData; virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; +virConnectPtr nodeconn = NULL; +virNodeDevicePtr nodedev = NULL; + +if (!(nodeconn = virGetConnectNodeDev())) +goto cleanup; + +/* 'dev' is associated with the QEMU virConnectPtr, + * so for split daemons, we need to get a copy that + * is associated with the virnodedevd daemon. + */ +if (!(nodedev = virNodeDeviceLookupByName( + nodeconn, virNodeDeviceGetName(dev +goto cleanup; -xml = virNodeDeviceGetXMLDesc(dev, 0); +xml = virNodeDeviceGetXMLDesc(nodedev, 0); if (!xml) goto cleanup; @@ -5893,6 +5944,8 @@ libxlNodeDeviceReset(virNodeDevicePtr dev) if (!def) goto cleanup; +/* ACL check must happen against original 'dev', + * not the new 'nodedev' we acquired */ if (virNodeDevi
Re: qemu:///embed and isolation from global components
On Tue, 2020-03-10 at 16:00 +, Daniel P. Berrangé wrote: > The split daemon model is intended to allow us to address this > long standing design flaw, by allowing the QEMU session driver > to optionally talk to a secondary driver running with different > privileges, instead of the instance running with the same privs. > > So currently we have > > > > > > This refers to a secondary driver running at the same privilege > level. > > I expected we'd extend it to allow > > > > > > This explicitly requests the secondary driver running with elevated > privileges. This makes perfect sense to me, and in fact I believe you're basically suggesting what I was arguing for earlier :) In your scenario, when you don't specify a scope you get the same one as the primary driver is using (this matches the current behavior): so if you are using qemu:///session, every will use network:///session unless you explicitly specify scope='system', at which point network:///system will be used; in the same fashion, if you're connected to qemu:///embed, the default for s should be network:///embed, with the possibility to use either network:///session (with scope='session') or, most likely, network:///system (with scope='system'). virtlogd would probably have to be treated a bit differently since it needs to be running even before the first guest is started, but other than that the same general principle should apply. > With such functionality present, it logically then also extends to > cover connections to daemons running in different namespaces. I'm still unclear on how this scenario, which would apparently have multiple eg. privileged virtnetworkd, running at the same time but in different network namespaces, would work. -- Andrea Bolognani / Red Hat / Virtualization
