Re: [libvirt] [jenkins-ci PATCH 03/17] guests: Install EPEL on CentOS7
On Tue, 2019-10-01 at 16:28 +0200, Fabiano Fidêncio wrote:
> +- name: Enable EPEL
> + command: '{{ package_manager }} install epel-release -y'
> + args:
> +warn: no
> + when:
> +- os_name == 'CentOS'
> +
At this point in the playbook the package module has been properly
bootstrapped (as evidenced by the fact that it's used immediately
afterwards), so we can do this like
- name: Enable EPEL repository
package:
name: epel-release
state: latest
when:
- os_name == 'CentOS'
With that changed,
Reviewed-by: Andrea Bolognani
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH 02/17] guests: CentOS7 provides python3
On Tue, 2019-10-01 at 16:28 +0200, Fabiano Fidêncio wrote: > As latest CentOS7 release includes python3, let's update python3 > mapping. > > Signed-off-by: Fabiano Fidêncio > --- > guests/vars/mappings.yml | 1 - > 1 file changed, 1 deletion(-) \o/ Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 12/12] tests: add a test for driver.c:virConnectValidateURIPath()
On 9/27/19 5:32 PM, Daniel Henrique Barboza wrote:
On 9/27/19 5:33 PM, Cole Robinson wrote:
On 9/26/19 10:56 AM, Daniel Henrique Barboza wrote:
Signed-off-by: Daniel Henrique Barboza
---
I've made this test file to make sure I wasn't messing
up with the logic at patch 8. The idea of having this
test seems okay, but probably I could do it shorter/cleaner.
Feel free to discard it if it's not applicable or
unnecessary. Adding this new file make the whole patch series,
aimed at reduce code repetition and lines of code, add more
lines than before. The irony is real.
It will reduce lines if you fold the data.entityName = X bit into the
TEST_ calls, passing the string value as the first argument for example
Got it.
We don't have a ton of fine grained unittesting like this in libvirt,
but it doesn't hurt. Though in this case it seems kinda overkill IMO
to call it for possible combo that it's used in. I think it's better
to just call it in all the invocations to give full code coverage. If
we want to test the individual driver usage of the function then we
would want to find a way to test those entry points directly IMO.
Maybe others have opinions.
Do you mean we should just test the actual usage of the function in the
code
instead of testing all possible fail scenarios? For example, the code
does not
call virConnectValidateURIPath("/system", X , false) anywhere, so it's
ok to
not test them since it's not being exercised anyway - otherwise we're
entering TDD territory (which I don't mind - kind of a fan TBH), testing
all
possible stuff just for sake of testing.
Is that what you're saying? I'm fine with it, and it will cut a good
chunk of
lines in this file too.
I was thinking, add test cases that are needed to hit every bit of logic
in the function. So
privileged=false, /session path
privileged=false, non-/session path failure
privileged=true, /system path
privileged=true, non-/system path failure
privileged=true, vbox /session
privileged=true, qemu /session
privileged=true, other driver /session failure
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1] src/driver.c: remove duplicated code in virGetConnect* functions
On 9/27/19 11:09 AM, Daniel Henrique Barboza wrote:
All the 6 virGetConnect* functions in driver.c shares the
same code base. This patch creates a new static function
virGetConnectGeneric() that contains the common code to
be used with all other virGetConnect*.
Signed-off-by: Daniel Henrique Barboza
---
CC'ing Cole Robinson since he reviewed similar patches
a few days ago.
src/driver.c | 100 +--
1 file changed, 25 insertions(+), 75 deletions(-)
diff --git a/src/driver.c b/src/driver.c
index ed2d943ddf..8a4bc8ff66 100644
--- a/src/driver.c
+++ b/src/driver.c
@@ -29,6 +29,7 @@
#include "virfile.h"
#include "virlog.h"
#include "virmodule.h"
+#include "virstring.h"
#include "virthread.h"
#include "configmake.h"
@@ -96,112 +97,61 @@ virConnectCacheOnceInit(void)
VIR_ONCE_GLOBAL_INIT(virConnectCache);
-virConnectPtr virGetConnectInterface(void)
+static virConnectPtr
+virGetConnectGeneric(virThreadLocal thread, const char *name)
{
virThreadLocal is a struct behind the typedef, so better to make this
virThreadLocalPtr. That's also what all other functions that use
virThreadLocal take as an argument
virConnectPtr conn;
if (virConnectCacheInitialize() < 0)
return NULL;
-conn = virThreadLocalGet();
+conn = virThreadLocalGet();
+
if (conn) {
-VIR_DEBUG("Return cached interface connection %p", conn);
+VIR_DEBUG("Return cached %s connection %p", name, conn);
virObjectRef(conn);
} else {
-conn = virConnectOpen(geteuid() == 0 ? "interface:///system" :
"interface:///session");
-VIR_DEBUG("Opened new interface connection %p", conn);
+VIR_AUTOFREE(char *) uri = NULL;
+const char *uriPath = geteuid() == 0 ? "/system" : "/session";
+
+if (virAsprintf(, "%s//%s", name, uriPath) < 0)
+return NULL;
+
Missing a : here
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [jenkins-ci PATCH] Fix target list for osinfo-db builds
Commit 570143fc41fa removed all unsuitable Debian-based
distributions, but failed to account for the fact that CentOS 7
doesn't have a Meson version suitable to build osinfo-db-tools
and as such can't possibly build osinfo-db.
Signed-off-by: Andrea Bolognani
---
Pushed as a CI fix.
guests/playbooks/build/projects/osinfo-db.yml | 25 ++-
jenkins/projects/osinfo-db.yaml | 21
2 files changed, 24 insertions(+), 22 deletions(-)
diff --git a/guests/playbooks/build/projects/osinfo-db.yml
b/guests/playbooks/build/projects/osinfo-db.yml
index 872b448..1952827 100644
--- a/guests/playbooks/build/projects/osinfo-db.yml
+++ b/guests/playbooks/build/projects/osinfo-db.yml
@@ -1,7 +1,15 @@
---
- set_fact:
name: osinfo-db
-machines: '{{ all_machines }}'
+machines:
+ - libvirt-debian-10
+ - libvirt-debian-sid
+ - libvirt-fedora-29
+ - libvirt-fedora-30
+ - libvirt-fedora-rawhide
+ - libvirt-freebsd-11
+ - libvirt-freebsd-12
+ - libvirt-freebsd-current
archive_format: xz
git_url: '{{ git_urls["osinfo-db"][git_remote] }}'
@@ -13,21 +21,14 @@
$MAKE install OSINFO_DB_TARGET="--system"
- include: '{{ playbook_base }}/jobs/generic-check-job.yml'
vars:
-# osinfo-db tests are Python 3 only, so they can't be ran on CentOS 7;
-machines:
- - libvirt-debian-10
- - libvirt-debian-sid
- - libvirt-fedora-29
- - libvirt-fedora-30
- - libvirt-fedora-rawhide
- - libvirt-freebsd-11
- - libvirt-freebsd-12
- - libvirt-freebsd-current
command: |
$MAKE check
- include: '{{ playbook_base }}/jobs/generic-rpm-job.yml'
vars:
-machines: '{{ rpm_machines }}'
+machines:
+ - libvirt-fedora-29
+ - libvirt-fedora-30
+ - libvirt-fedora-rawhide
command: |
{{ strip_buildrequires }}
rpmbuild --clean --define "_topdir `pwd`/rpmbuild" --define "_sourcedir
`pwd`" -ba osinfo-db.spec
diff --git a/jenkins/projects/osinfo-db.yaml b/jenkins/projects/osinfo-db.yaml
index 37c1d84..7074fd1 100644
--- a/jenkins/projects/osinfo-db.yaml
+++ b/jenkins/projects/osinfo-db.yaml
@@ -1,7 +1,13 @@
---
- project:
name: osinfo-db
-machines: '{all_machines}'
+machines:
+ - libvirt-debian-10
+ - libvirt-fedora-29
+ - libvirt-fedora-30
+ - libvirt-fedora-rawhide
+ - libvirt-freebsd-11
+ - libvirt-freebsd-12
title: osinfo database
archive_format: xz
git_url: '{git_urls[osinfo-db][default]}'
@@ -13,19 +19,14 @@
$MAKE install OSINFO_DB_TARGET="--system"
- generic-check-job:
parent_jobs: 'osinfo-db-build'
- # osinfo-db tests are Python 3 only, so they can't be ran on CentOS
7;
- machines:
-- libvirt-debian-10
-- libvirt-fedora-29
-- libvirt-fedora-30
-- libvirt-fedora-rawhide
-- libvirt-freebsd-11
-- libvirt-freebsd-12
command: |
$MAKE check
- generic-rpm-job:
parent_jobs: 'osinfo-db-check'
- machines: '{rpm_machines}'
+ machines:
+- libvirt-fedora-29
+- libvirt-fedora-30
+- libvirt-fedora-rawhide
command: |
{strip_buildrequires}
rpmbuild --clean --define "_topdir `pwd`/rpmbuild" --define
"_sourcedir `pwd`" -ba osinfo-db.spec
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 00/15] CPU Model Baseline and Comparison for s390x
On 10/2/19 11:48 AM, Jiri Denemark wrote: > On Thu, Sep 19, 2019 at 16:24:51 -0400, Collin Walling wrote: >> Note: since I've made some changes to a lot of these patches / split >> up some patches, I've decided to hold off on adding any r-b's in case >> there is a specific change that someone does not agree with. >> >> Changelog: >> >> - Properly refactored code from CPU model expansion function >> - Introduced a cleanup patch for CPU model expansion function >> - Introduced patches that modifies the refactored code to suit >> needs for baseline/comparison >> - CPU expansion function now accepts a virCPUDefPtr >> - Removed props parsing from CPU model comparison (they weren't >> used) >> - Cleaner error reporting when baselining/comparing with erroneous >> CPU models / features >> - Various cleanups based on feedback > > Thanks. All but 15/15 are acked now and I will push them as soon as we > leave the pre-release freeze period. The patch 15/15 needs a little bit > of tweaking, but it is not essential and it can be pushed later as a > follow-up patch. Alternatively, if you want to work on it while we are > frozen, you can use the s390-cpu-apis branch (based on current master) > of my staging repo https://gitlab.com/jirkade/libvirt.git > > Jirka Much appreciated. I'll fix 15/15 and post it as a follow-up. Thank you for your time and offering to take on the cleanups. > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list > -- Respectfully, - Collin Walling -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH 01/17] guests: Deal with multilib path
On Tue, 2019-10-01 at 16:28 +0200, Fabiano Fidêncio wrote:
> +# Add the multilib path to the LD_LIBRARY_PATH, PKG_CONFIG_PATH, and
> GI_TYPELIB_PATH
> +# The multilib path can be discovered either using `dpkg-architecture` (on
> Debian based
> +# machines) or by calling `rpm --eval '%{_lib}'` (on rpm based machines).
This comment is a bit verbose and basically describes the code below
step by step which is unnecessary, so I've rewritten it to read
These search paths need to encode the OS architecture in some way
in order to work, so use the appropriate tool to obtain this
information and adjust them accordingly
and pushed the patch with my
Reviewed-by: Andrea Bolognani
I'll review the rest of the series as time permits.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH 0/2] Fix project and target list
On Wed, Oct 2, 2019 at 4:16 PM Andrea Bolognani wrote: > > Andrea Bolognani (2): > guests: Fix project list > Fix target list for builds > > guests/host_vars/libvirt-centos-7/main.yml | 1 - > guests/host_vars/libvirt-debian-9/main.yml | 2 -- > guests/host_vars/libvirt-ubuntu-16/main.yml | 1 - > guests/host_vars/libvirt-ubuntu-18/main.yml | 2 -- > guests/playbooks/build/projects/osinfo-db.yml| 3 --- > guests/playbooks/build/projects/virt-manager.yml | 4 > jenkins/projects/osinfo-db.yaml | 1 - > jenkins/projects/virt-manager.yaml | 2 -- > 8 files changed, 16 deletions(-) > > -- > 2.21.0 > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list Reviewed-by: Fabiano Fidêncio -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 00/15] CPU Model Baseline and Comparison for s390x
On Thu, Sep 19, 2019 at 16:24:51 -0400, Collin Walling wrote: > Note: since I've made some changes to a lot of these patches / split > up some patches, I've decided to hold off on adding any r-b's in case > there is a specific change that someone does not agree with. > > Changelog: > > - Properly refactored code from CPU model expansion function > - Introduced a cleanup patch for CPU model expansion function > - Introduced patches that modifies the refactored code to suit > needs for baseline/comparison > - CPU expansion function now accepts a virCPUDefPtr > - Removed props parsing from CPU model comparison (they weren't > used) > - Cleaner error reporting when baselining/comparing with erroneous > CPU models / features > - Various cleanups based on feedback Thanks. All but 15/15 are acked now and I will push them as soon as we leave the pre-release freeze period. The patch 15/15 needs a little bit of tweaking, but it is not essential and it can be pushed later as a follow-up patch. Alternatively, if you want to work on it while we are frozen, you can use the s390-cpu-apis branch (based on current master) of my staging repo https://gitlab.com/jirkade/libvirt.git Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 15/15] qemu_driver: improve comparison/baseline error reporting
On Thu, Sep 19, 2019 at 16:25:06 -0400, Collin Walling wrote:
> Providing an erroneous CPU definition in the XML file provided to the
> hypervisor-cpu-compare/baseline command will result in a verbose
> internal error. Let's add some sanity checking before executing the QMP
> commands to provide a cleaner message if something is wrong with the
> CPU model or features.
>
> Signed-off-by: Collin Walling
> ---
> src/qemu/qemu_driver.c | 62
> ++
> 1 file changed, 62 insertions(+)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 153b2f2..6298c48 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -13703,6 +13703,55 @@ qemuConnectCompareCPU(virConnectPtr conn,
> }
>
>
> +static int
> +qemuConnectCheckCPUModel(qemuMonitorPtr mon,
> + virCPUDefPtr cpu,
> + bool reportError)
> +{
> +qemuMonitorCPUModelInfoPtr modelInfo = NULL;
> +qemuMonitorCPUModelExpansionType type;
> +size_t i, j;
> +int ret = -1;
> +
> +/* Collect CPU model name and features known by QEMU */
> +type = QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL;
> +if (qemuMonitorGetCPUModelExpansion(mon, type, cpu,
> +true, false, ) < 0)
> +goto cleanup;
> +
> +/* Sanity check CPU model */
> +if (!modelInfo) {
> +if (reportError)
> +virReportError(VIR_ERR_CPU_INCOMPATIBLE,
> + _("Unknown CPU model: %s"), cpu->model);
This is not really related to CPU compatibility. Except for taking a CPU
model (or feature below) from a newer QEMU and comparing it on an old
one. But this is indistinguishable from an incorrect input. For this
reason and for consistency among all architectures we should just report
this as a normal error (e.g., VIR_ERR_CONFIG_UNSUPPORTED).
> +goto cleanup;
> +}
> +
> +/* Sanity check CPU features */
> +for (i = 0; i < cpu->nfeatures; i++) {
> +const char *feat = cpu->features[i].name;
> +
> +for (j = 0; j < modelInfo->nprops; j++) {
> +const char *prop = modelInfo->props[j].name;
> +if (STREQ(feat, prop))
> +break;
> +}
> +
> +if (j == modelInfo->nprops) {
> +if (reportError)
> +virReportError(VIR_ERR_CPU_INCOMPATIBLE,
> + _("Unknown CPU feature: %s"), feat);
Ditto.
> +goto cleanup;
> +}
> +}
> +ret = 0;
> +
> + cleanup:
> +qemuMonitorCPUModelInfoFree(modelInfo);
> +return ret;
> +}
> +
> +
> static virCPUCompareResult
> qemuConnectCPUModelComparison(virQEMUCapsPtr qemuCaps,
>const char *libDir,
> @@ -13723,6 +13772,13 @@ qemuConnectCPUModelComparison(virQEMUCapsPtr
> qemuCaps,
> if (qemuProcessQMPStart(proc) < 0)
> goto cleanup;
>
> +if (qemuConnectCheckCPUModel(proc->mon, cpu_a, failIncompatible) ||
> +qemuConnectCheckCPUModel(proc->mon, cpu_b, failIncompatible)) {
That said, you don't need to pass failIncompatible to
qemuConnectCheckCPUModel...
> +if (!failIncompatible)
> +ret = VIR_CPU_COMPARE_INCOMPATIBLE;
and you can drop this conditional.
> +goto cleanup;
> +}
> +
> if (qemuMonitorGetCPUModelComparison(proc->mon, cpu_a, cpu_b, ) <
> 0)
> goto cleanup;
>
> @@ -13913,6 +13969,9 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps,
> if (qemuProcessQMPStart(proc) < 0)
> goto cleanup;
>
> +if (qemuConnectCheckCPUModel(proc->mon, cpus[0], true))
> +goto cleanup;
> +
> if (VIR_ALLOC(baseline) < 0)
> goto error;
>
> @@ -13921,6 +13980,9 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps,
>
> for (i = 1; i < ncpus; i++) {
>
> +if (qemuConnectCheckCPUModel(proc->mon, cpus[i], true))
> +goto error;
> +
Also in all four cases you should use qemuConnectCheckCPUModel() < 0
check.
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 09/15] qemu_driver: hook up query-cpu-model-baseline
On 10/2/19 9:52 AM, Jiri Denemark wrote: > On Thu, Sep 19, 2019 at 16:25:00 -0400, Collin Walling wrote: >> This command is hooked into the virsh hypervisor-cpu-baseline command. >> The CPU models provided in the XML sent to the command will be baselined >> via the query-cpu-model-baseline QMP command. The resulting CPU model >> will be reported. >> >> Signed-off-by: Collin Walling >> Reviewed-by: Daniel Henrique Barboza >> --- >> src/qemu/qemu_driver.c | 88 >> ++ >> 1 file changed, 88 insertions(+) >> Thank you for the reviews and for providing a cleanup patch for this one. I will tend to these ASAP. -- Respectfully, - Collin Walling -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: fixed handling of targetless disks (e.g. empty CDROM) in 'domblkinfo' cmd
On Wed, Oct 02, 2019 at 08:34:10AM -0300, Daniel Henrique Barboza wrote:
>
>
> On 10/1/19 11:45 AM, Pavel Mores wrote:
> > virDomainGetBlockInfo() returns error if called on a disk with no target
> > (a targetless disk might be a removable media drive with no media in it,
> > for instance an empty CDROM drive).
> >
> > So far this caused the virsh domblkinfo --all command to abort and ignore
> > any remaining (not yet displayed) disk devices. This patch fixes it by
> > ignoring virDomainGetBlockInfo() errors for CDROM and floppy drives,
> > similar to how it's done for network drives.
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=1619625
> >
> > Signed-off-by: Pavel Mores
> > ---
> > tools/virsh-domain-monitor.c | 11 +++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
> > index 0e2c4191d7..0f495c1a3f 100644
> > --- a/tools/virsh-domain-monitor.c
> > +++ b/tools/virsh-domain-monitor.c
> > @@ -473,6 +473,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
> > char *cap = NULL;
> > char *alloc = NULL;
> > char *phy = NULL;
> > +char *device_type = NULL;
>
> I believe you need to use VIR_AUTO(char *) here. Even considering that
> the macro will be deprecated in the near future for glib stuff, by using
> VIR_AUTO here:
>
> - you'll make it easier to introduce the glib replacement, since
> s/VIR_AUTO/ is a trivial change;
>
> - you won't be adding more VIR_FREE() on top of existing cases that will
> need to be addressed in the future.
Was that the consensus from the migration debate? If so I'm happy to fix my
patch.
pvl
>
>
> Thanks,
>
>
> DHB
>
>
> > vshTablePtr table = NULL;
> > if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
> > @@ -510,6 +511,8 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
> > rc = virDomainGetBlockInfo(dom, target, , 0);
> > if (rc < 0) {
> > +device_type = virXPathString("string(./@device)", ctxt);
> > +
> > /* If protocol is present that's an indication of a
> > networked
> >* storage device which cannot provide statistics, so
> > generate
> >* 0 based data and get the next disk. */
> > @@ -518,9 +521,16 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
> > virGetLastErrorDomain() == VIR_FROM_STORAGE) {
> > memset(, 0, sizeof(info));
> > vshResetLibvirtError();
> > +} else if (device_type != NULL &&
> > +(STRCASEEQ(device_type, "cdrom") ||
> > +STRCASEEQ(device_type, "floppy"))) {
> > +memset(, 0, sizeof(info));
> > +vshResetLibvirtError();
> > } else {
> > goto cleanup;
> > }
> > +
> > +VIR_FREE(device_type);
> > }
> > if (!cmdDomblkinfoGet(ctl, , , , , human))
> > @@ -556,6 +566,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
> > VIR_FREE(target);
> > VIR_FREE(protocol);
> > VIR_FREE(disks);
> > +VIR_FREE(device_type);
> > xmlXPathFreeContext(ctxt);
> > xmlFreeDoc(xmldoc);
> > return ret;
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [jenkins-ci PATCH 0/2] Fix project and target list
Andrea Bolognani (2): guests: Fix project list Fix target list for builds guests/host_vars/libvirt-centos-7/main.yml | 1 - guests/host_vars/libvirt-debian-9/main.yml | 2 -- guests/host_vars/libvirt-ubuntu-16/main.yml | 1 - guests/host_vars/libvirt-ubuntu-18/main.yml | 2 -- guests/playbooks/build/projects/osinfo-db.yml| 3 --- guests/playbooks/build/projects/virt-manager.yml | 4 jenkins/projects/osinfo-db.yaml | 1 - jenkins/projects/virt-manager.yaml | 2 -- 8 files changed, 16 deletions(-) -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [jenkins-ci PATCH 1/2] guests: Fix project list
We've disabled osinfo-db-tools and libosinfo builds on some targets after they switched to Meson, but while doing so we forgot to also disable osinfo-db and virt-manager, which depend on them. Signed-off-by: Andrea Bolognani --- guests/host_vars/libvirt-centos-7/main.yml | 1 - guests/host_vars/libvirt-debian-9/main.yml | 2 -- guests/host_vars/libvirt-ubuntu-16/main.yml | 1 - guests/host_vars/libvirt-ubuntu-18/main.yml | 2 -- 4 files changed, 6 deletions(-) diff --git a/guests/host_vars/libvirt-centos-7/main.yml b/guests/host_vars/libvirt-centos-7/main.yml index 6156414..4e3ce85 100644 --- a/guests/host_vars/libvirt-centos-7/main.yml +++ b/guests/host_vars/libvirt-centos-7/main.yml @@ -7,7 +7,6 @@ projects: - libvirt-ocaml - libvirt-perl - libvirt-python - - osinfo-db - virt-viewer package_format: 'rpm' diff --git a/guests/host_vars/libvirt-debian-9/main.yml b/guests/host_vars/libvirt-debian-9/main.yml index cb19395..c388ee8 100644 --- a/guests/host_vars/libvirt-debian-9/main.yml +++ b/guests/host_vars/libvirt-debian-9/main.yml @@ -10,8 +10,6 @@ projects: - libvirt-python - libvirt-sandbox - libvirt-tck - - osinfo-db - - virt-manager - virt-viewer package_format: 'deb' diff --git a/guests/host_vars/libvirt-ubuntu-16/main.yml b/guests/host_vars/libvirt-ubuntu-16/main.yml index 0407ae3..bed11d4 100644 --- a/guests/host_vars/libvirt-ubuntu-16/main.yml +++ b/guests/host_vars/libvirt-ubuntu-16/main.yml @@ -10,7 +10,6 @@ projects: - libvirt-python - libvirt-sandbox - libvirt-tck - - osinfo-db - virt-viewer package_format: 'deb' diff --git a/guests/host_vars/libvirt-ubuntu-18/main.yml b/guests/host_vars/libvirt-ubuntu-18/main.yml index 4a95f45..199b2bb 100644 --- a/guests/host_vars/libvirt-ubuntu-18/main.yml +++ b/guests/host_vars/libvirt-ubuntu-18/main.yml @@ -10,8 +10,6 @@ projects: - libvirt-python - libvirt-sandbox - libvirt-tck - - osinfo-db - - virt-manager - virt-viewer package_format: 'deb' -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [jenkins-ci PATCH 2/2] Fix target list for builds
Targets that don't build osinfo-db-tools and libosinfo should not
build osinfo-db and virt-manager.
Signed-off-by: Andrea Bolognani
---
guests/playbooks/build/projects/osinfo-db.yml| 3 ---
guests/playbooks/build/projects/virt-manager.yml | 4
jenkins/projects/osinfo-db.yaml | 1 -
jenkins/projects/virt-manager.yaml | 2 --
4 files changed, 10 deletions(-)
diff --git a/guests/playbooks/build/projects/osinfo-db.yml
b/guests/playbooks/build/projects/osinfo-db.yml
index 856e478..872b448 100644
--- a/guests/playbooks/build/projects/osinfo-db.yml
+++ b/guests/playbooks/build/projects/osinfo-db.yml
@@ -15,7 +15,6 @@
vars:
# osinfo-db tests are Python 3 only, so they can't be ran on CentOS 7;
machines:
- - libvirt-debian-9
- libvirt-debian-10
- libvirt-debian-sid
- libvirt-fedora-29
@@ -24,8 +23,6 @@
- libvirt-freebsd-11
- libvirt-freebsd-12
- libvirt-freebsd-current
- - libvirt-ubuntu-16
- - libvirt-ubuntu-18
command: |
$MAKE check
- include: '{{ playbook_base }}/jobs/generic-rpm-job.yml'
diff --git a/guests/playbooks/build/projects/virt-manager.yml
b/guests/playbooks/build/projects/virt-manager.yml
index f955f4c..0078fbe 100644
--- a/guests/playbooks/build/projects/virt-manager.yml
+++ b/guests/playbooks/build/projects/virt-manager.yml
@@ -5,7 +5,6 @@
# Ubuntu 16.04 has Python 3 but not the libxml2 bindings, so it can't
# build the project either
machines:
- - libvirt-debian-9
- libvirt-debian-10
- libvirt-debian-sid
- libvirt-fedora-29
@@ -14,7 +13,6 @@
- libvirt-freebsd-11
- libvirt-freebsd-12
- libvirt-freebsd-current
- - libvirt-ubuntu-18
archive_format: gz
git_url: '{{ git_urls["virt-manager"][git_remote] }}'
@@ -29,13 +27,11 @@
# so skip the test suite there for the time being. See
# https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=224902
machines:
- - libvirt-debian-9
- libvirt-debian-10
- libvirt-debian-sid
- libvirt-fedora-29
- libvirt-fedora-30
- libvirt-fedora-rawhide
- - libvirt-ubuntu-18
- include: '{{ playbook_base }}/jobs/python-distutils-rpm-job.yml'
vars:
machines:
diff --git a/jenkins/projects/osinfo-db.yaml b/jenkins/projects/osinfo-db.yaml
index 256c62d..37c1d84 100644
--- a/jenkins/projects/osinfo-db.yaml
+++ b/jenkins/projects/osinfo-db.yaml
@@ -15,7 +15,6 @@
parent_jobs: 'osinfo-db-build'
# osinfo-db tests are Python 3 only, so they can't be ran on CentOS
7;
machines:
-- libvirt-debian-9
- libvirt-debian-10
- libvirt-fedora-29
- libvirt-fedora-30
diff --git a/jenkins/projects/virt-manager.yaml
b/jenkins/projects/virt-manager.yaml
index 2577ea9..fe6b903 100644
--- a/jenkins/projects/virt-manager.yaml
+++ b/jenkins/projects/virt-manager.yaml
@@ -5,7 +5,6 @@
# Ubuntu 16.04 has Python 3 but not the libxml2 bindings, so it can't
# build the project either
machines:
- - libvirt-debian-9
- libvirt-debian-10
- libvirt-fedora-29
- libvirt-fedora-30
@@ -28,7 +27,6 @@
# so skip the test suite there for the time being. See
# https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=224902
machines:
-- libvirt-debian-9
- libvirt-debian-10
- libvirt-fedora-29
- libvirt-fedora-30
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 10/15] qemu_driver: expand cpu features after baseline
On Thu, Sep 19, 2019 at 16:25:01 -0400, Collin Walling wrote: > Perform a full CPU model expansion on the result of the baselined > model name when the features flag is present. > > Signed-off-by: Collin Walling > --- > src/qemu/qemu_driver.c | 15 ++- > 1 file changed, 14 insertions(+), 1 deletion(-) Reviewed-by: Jiri Denemark -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 11/15] qemu_monitor: implement query-cpu-model-comparison
On Thu, Sep 19, 2019 at 16:25:02 -0400, Collin Walling wrote: > Interfaces with QEMU to compare CPU models. The command takes two CPU > models, A and B, that are given a model name and an optional list of > CPU features. Through the query-cpu-model-comparison command issued > via QMP, a result is produced that contains the comparison evaluation > string (identical, superset, subset, incompatible). > > The list of properties (aka CPU features) that is returned from the QMP > response is ignored. > > Signed-off-by: Collin Walling > Reviewed-by: Daniel Henrique Barboza > --- > src/qemu/qemu_monitor.c | 14 ++ > src/qemu/qemu_monitor.h | 5 + > src/qemu/qemu_monitor_json.c | 42 ++ > src/qemu/qemu_monitor_json.h | 6 ++ > 4 files changed, 67 insertions(+) Reviewed-by: Jiri Denemark -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 14/15] qemu_driver: hook up query-cpu-model-comparison
On Thu, Sep 19, 2019 at 16:25:05 -0400, Collin Walling wrote:
> This command is hooked into the virsh hypervisor-cpu-compare command.
> As such, the CPU model XML provided to the command will be compared
> to the hypervisor CPU contained in the QEMU capabilities file for the
> appropriate QEMU binary (for s390x, this CPU definition can be observed
> via virsh domcapabilities).
>
> QMP will report that the XML CPU is either identical to, a subset of,
> or incompatible with the hypervisor CPU. s390 can also report that
> the XML CPU is a "superset" of the hypervisor CPU. This response is
> presented as incompatible, as this CPU model would not be able to run
> on the hypervisor.
>
> Signed-off-by: Collin Walling
> Reviewed-by: Daniel Henrique Barboza
> Reviewed-by: Boris Fiuczynski
> ---
> src/qemu/qemu_driver.c | 52
> ++
> 1 file changed, 52 insertions(+)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 93f1767..153b2f2 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
...
> @@ -13714,9 +13753,11 @@ qemuConnectCompareHypervisorCPU(virConnectPtr conn,
> {
> int ret = VIR_CPU_COMPARE_ERROR;
> virQEMUDriverPtr driver = conn->privateData;
> +VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg =
> virQEMUDriverGetConfig(driver);
> virQEMUCapsPtr qemuCaps = NULL;
> bool failIncompatible;
> virCPUDefPtr hvCPU;
> +virCPUDefPtr cpu;
This needs to be initialized to NULL.
> virArch arch;
> virDomainVirtType virttype;
>
> @@ -13751,6 +13792,16 @@ qemuConnectCompareHypervisorCPU(virConnectPtr conn,
>
> if (ARCH_IS_X86(arch)) {
> ret = virCPUCompareXML(arch, hvCPU, xmlCPU, failIncompatible);
> +
> +} else if (ARCH_IS_S390(arch) &&
> + virQEMUCapsGet(qemuCaps,
> QEMU_CAPS_QUERY_CPU_MODEL_COMPARISON)) {
> +
Some extra empty lines.
> +if (virCPUDefParseXMLString(xmlCPU, VIR_CPU_TYPE_AUTO, ) < 0)
> +goto cleanup;
> +
> +ret = qemuConnectCPUModelComparison(qemuCaps, cfg->libDir,
> +cfg->user, cfg->group,
> +hvCPU, cpu, failIncompatible);
> } else {
> virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> _("comparing with the hypervisor CPU is not supported
> "
> @@ -13758,6 +13809,7 @@ qemuConnectCompareHypervisorCPU(virConnectPtr conn,
> }
>
> cleanup:
> +virCPUDefFree(cpu);
> virObjectUnref(qemuCaps);
> return ret;
> }
Reviewed-by: Jiri Denemark
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 13/15] cpu_conf: xml to cpu definition parse helper
On Thu, Sep 19, 2019 at 16:25:04 -0400, Collin Walling wrote: > Implement an XML to virCPUDefPtr helper that handles the ctxt > prerequisite for virCPUDefParseXML. > > This does not alter any functionality. > > Signed-off-by: Collin Walling > Reviewed-by: Bjoern Walk > Reviewed-by: Daniel Henrique Barboza > --- > src/conf/cpu_conf.c | 29 + > src/conf/cpu_conf.h | 5 + > src/cpu/cpu.c| 14 +- > src/libvirt_private.syms | 1 + > 4 files changed, 36 insertions(+), 13 deletions(-) Reviewed-by: Jiri Denemark -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 09/15] qemu_driver: hook up query-cpu-model-baseline
On Thu, Sep 19, 2019 at 16:25:00 -0400, Collin Walling wrote:
> This command is hooked into the virsh hypervisor-cpu-baseline command.
> The CPU models provided in the XML sent to the command will be baselined
> via the query-cpu-model-baseline QMP command. The resulting CPU model
> will be reported.
>
> Signed-off-by: Collin Walling
> Reviewed-by: Daniel Henrique Barboza
> ---
> src/qemu/qemu_driver.c | 88
> ++
> 1 file changed, 88 insertions(+)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 1e041a8..2a5a3ca 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -13810,6 +13810,83 @@ qemuConnectBaselineCPU(virConnectPtr conn
> ATTRIBUTE_UNUSED,
> }
>
>
> +static int
> +qemuConnectStealCPUModelFromInfo(virCPUDefPtr dst,
> + qemuMonitorCPUModelInfoPtr *src)
> +{
> +qemuMonitorCPUModelInfoPtr info = *src;
> +size_t i;
> +int ret = 0;
We usually initialize ret to -1 and set it to zero at the very end when
everything is done rather than changing it to -1 on each error path.
> +
> +virCPUDefFreeModel(dst);
> +
> +VIR_STEAL_PTR(dst->model, info->name);
> +
> +for (i = 0; i < info->nprops; i++) {
> +char *name = info->props[i].name;
> +
> +if (info->props[i].type == QEMU_MONITOR_CPU_PROPERTY_BOOLEAN &&
> +info->props[i].value.boolean &&
> +virCPUDefAddFeature(dst, name, VIR_CPU_FEATURE_REQUIRE) < 0) {
> +virCPUDefFree(dst);
This would cause double free in the caller.
> +ret = -1;
> +break;
> +}
> +}
> +
Adding cleanup label here would be better.
> +qemuMonitorCPUModelInfoFree(info);
> +*src = NULL;
You can avoid this by using VIR_STEAL_PTR(info, *src) at the beginning.
> +return ret;
> +}
> +
> +
> +static virCPUDefPtr
> +qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps,
> +const char *libDir,
> +uid_t runUid,
> +gid_t runGid,
> +int ncpus,
> +virCPUDefPtr *cpus)
I think I mentioned in my previous review (probably not in this exact
context, though) we usually pass an array followed by the number of
elements.
> +{
> +qemuProcessQMPPtr proc;
> +virCPUDefPtr baseline = NULL;
> +qemuMonitorCPUModelInfoPtr result = NULL;
> +size_t i;
> +
> +if (!(proc = qemuProcessQMPNew(virQEMUCapsGetBinary(qemuCaps),
> + libDir, runUid, runGid, false)))
> +goto cleanup;
> +
> +if (qemuProcessQMPStart(proc) < 0)
> +goto cleanup;
> +
> +if (VIR_ALLOC(baseline) < 0)
> +goto error;
> +
> +if (virCPUDefCopyModel(baseline, cpus[0], false))
> +goto error;
> +
> +for (i = 1; i < ncpus; i++) {
> +
Extra empty line.
> +if (qemuMonitorGetCPUModelBaseline(proc->mon, baseline,
> + cpus[i], ) < 0)
> +goto error;
> +
> +/* result is freed regardless of this function's success */
I think this comment would be better placed as a documentation of the
new function.
> +if (qemuConnectStealCPUModelFromInfo(baseline, ) < 0)
> +goto error;
> +}
> +
You could steal from baseline to ret, free baseline unconditionally and
return ret to get rid of the error label.
> + cleanup:
> +qemuProcessQMPFree(proc);
> +return baseline;
> +
> + error:
> +virCPUDefFree(baseline);
> +goto cleanup;
> +}
> +
> +
> static char *
> qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
> const char *emulator,
> @@ -13821,6 +13898,7 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
> unsigned int flags)
> {
> virQEMUDriverPtr driver = conn->privateData;
> +VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg =
> virQEMUDriverGetConfig(driver);
> virCPUDefPtr *cpus = NULL;
> virQEMUCapsPtr qemuCaps = NULL;
> virArch arch;
> @@ -13875,6 +13953,16 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
> if (!(cpu = virCPUBaseline(arch, cpus, ncpus, cpuModels,
> (const char **)features, migratable)))
> goto cleanup;
> +
> +} else if (ARCH_IS_S390(arch) &&
> + virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_BASELINE))
> {
> +
> +if (!(cpu = qemuConnectCPUModelBaseline(qemuCaps, cfg->libDir,
> +cfg->user, cfg->group,
> +ncpus,
> +cpus)))
> +goto cleanup;
> +
Three extra empty lines in this hunk.
> } else {
> virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> _("computing baseline hypervisor CPU is
Re: [libvirt] [PATCH v5 07/15] qemu_monitor: implement query-cpu-model-baseline
On Thu, Sep 19, 2019 at 16:24:58 -0400, Collin Walling wrote: > Interfaces with QEMU to baseline CPU models. The command takes two > CPU models, A and B, that are given a model name and an optional list > of CPU features. Through the query-cpu-model-baseline command issued > via QMP, a result is produced that contains a new baselined CPU model > that is guaranteed to run on both A and B. > > Signed-off-by: Collin Walling > Reviewed-by: Daniel Henrique Barboza > --- > src/qemu/qemu_monitor.c | 14 ++ > src/qemu/qemu_monitor.h | 5 + > src/qemu/qemu_monitor_json.c | 42 ++ > src/qemu/qemu_monitor_json.h | 6 ++ > 4 files changed, 67 insertions(+) Reviewed-by: Jiri Denemark -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 03/15] qemu_monitor: use cpu def instead of char for expansion
On Thu, Sep 19, 2019 at 16:24:54 -0400, Collin Walling wrote: > When expanding a CPU model via query-cpu-model-expansion, any features > that were a part of the original model are discarded. For exmaple, > when expanding modelA with features f1, f2, a full expansion may reveal > feature f3, but the expanded model will not include f1 or f2. > > Let's pass a virCPUDefPtr to the expansion function in preparation for > taking features into consideration. > > Signed-off-by: Collin Walling > --- > src/qemu/qemu_capabilities.c | 9 +++-- > src/qemu/qemu_monitor.c | 7 +++ > src/qemu/qemu_monitor.h | 2 +- > src/qemu/qemu_monitor_json.c | 8 > src/qemu/qemu_monitor_json.h | 2 +- > tests/cputest.c | 7 ++- > 6 files changed, 22 insertions(+), 13 deletions(-) Reviewed-by: Jiri Denemark -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 06/15] qemu_monitor: make qemuMonitorJSONParseCPUModelData command-agnostic
On Thu, Sep 19, 2019 at 16:24:57 -0400, Collin Walling wrote: > Modify the error messages in qemuMonitorJSONParseCPUModelData to print > the command name provided to the function. > > Signed-off-by: Collin Walling > --- > src/qemu/qemu_monitor_json.c | 15 --- > 1 file changed, 8 insertions(+), 7 deletions(-) Reviewed-by: Jiri Denemark -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 05/15] qemu_monitor: allow cpu props to be optional
On Thu, Sep 19, 2019 at 16:24:56 -0400, Collin Walling wrote: > Some older s390 CPU models (e.g. z900) will not report props as a > response from query-cpu-model-expansion. As such, we should make the > props field optional when parsing the return data from the QMP response. > > Signed-off-by: Collin Walling > --- > src/qemu/qemu_capabilities.c | 10 -- > src/qemu/qemu_monitor.c | 4 +++- > src/qemu/qemu_monitor.h | 1 + > src/qemu/qemu_monitor_json.c | 24 > src/qemu/qemu_monitor_json.h | 3 ++- > tests/cputest.c | 6 +- > 6 files changed, 35 insertions(+), 13 deletions(-) Reviewed-by: Jiri Denemark -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 02/15] qemu_monitor: expansion cleanups
On Thu, Sep 19, 2019 at 16:24:53 -0400, Collin Walling wrote: > With refactoring most of the expansion function, let's take care of > some additional cleanups. > > Signed-off-by: Collin Walling > --- > src/qemu/qemu_monitor_json.c | 37 ++--- > 1 file changed, 14 insertions(+), 23 deletions(-) Reviewed-by: Jiri Denemark -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 04/15] qemu_monitor: add features to CPU model for QMP command
On Thu, Sep 19, 2019 at 16:24:55 -0400, Collin Walling wrote: > query-cpu-model-baseline/comparison will accept a list of features > as part of the command. Since CPUs may be defined with CPU feature > policies, let's parse it to the appropriate boolean that the QMP > command expects. > > A feature that is set to required, force, or if it is a hypervisor > CPU feature (-1), then set the property value to true. Otherwise > (optional, disabled) set the value to false. > > Signed-off-by: Collin Walling > --- > src/qemu/qemu_monitor_json.c | 30 +- > 1 file changed, 25 insertions(+), 5 deletions(-) Reviewed-by: Jiri Denemark -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 01/15] qemu_monitor: refactor cpu model expansion
On Thu, Sep 19, 2019 at 16:24:52 -0400, Collin Walling wrote: > Refactor some code in qemuMonitorJSONGetCPUModelExpansion to be later > used for the comparison and baseline functions. > > Signed-off-by: Collin Walling > Reviewed-by: Bjoern Walk > Reviewed-by: Daniel Henrique Barboza > --- > src/qemu/qemu_monitor_json.c | 143 > --- > 1 file changed, 94 insertions(+), 49 deletions(-) Reviewed-by: Jiri Denemark -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: fixed handling of targetless disks (e.g. empty CDROM) in 'domblkinfo' cmd
On Wed, Oct 02, 2019 at 02:11:39PM +0200, Pavel Mores wrote:
On Wed, Oct 02, 2019 at 01:15:12PM +0200, Ján Tomko wrote:
Real estate in the commit message summary is precious, so the e.g.
part belongs in the commit message.
Right.
Also, (unless some strange usage sneaked into our XML), deals
with the host-side where says how the device appears in the
guest, so 'targetless' is wrong here.
On Tue, Oct 01, 2019 at 04:45:02PM +0200, Pavel Mores wrote:
> virDomainGetBlockInfo() returns error if called on a disk with no target
s/target/source/
Yes, sorry for the mixup.
> (a targetless disk might be a removable media drive with no media in it,
> for instance an empty CDROM drive).
>
> So far this caused the virsh domblkinfo --all command to abort and ignore
Most of virsh commands correspond 1:1 to a libvirt API. It would be
nicer if that was the case here, since now we have to guess what
happened in the daemon, because we try to treat --all as
--all-that-are-sensible-and-supported
Could you please elaborate? I don't have enough context yet, I'm not sure what
in particular the above means for this patch.
I mean that I do not consider the answer clear-cut.
For a single device invocation a failure in the libvirt API makes the virsh
command
fail. For multi-device, we have to decide which failures are OK or not.
> any remaining (not yet displayed) disk devices. This patch fixes it by
> ignoring virDomainGetBlockInfo() errors for CDROM and floppy drives,
> similar to how it's done for network drives.
>
I don't think that's an approach that should be copied, see below
> https://bugzilla.redhat.com/show_bug.cgi?id=1619625
>
> Signed-off-by: Pavel Mores
> ---
> tools/virsh-domain-monitor.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
> index 0e2c4191d7..0f495c1a3f 100644
> --- a/tools/virsh-domain-monitor.c
> +++ b/tools/virsh-domain-monitor.c
> @@ -473,6 +473,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
> char *cap = NULL;
> char *alloc = NULL;
> char *phy = NULL;
> +char *device_type = NULL;
> vshTablePtr table = NULL;
>
> if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
> @@ -510,6 +511,8 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
> rc = virDomainGetBlockInfo(dom, target, , 0);
>
> if (rc < 0) {
> +device_type = virXPathString("string(./@device)", ctxt);
> +
> /* If protocol is present that's an indication of a networked
> * storage device which cannot provide statistics, so generate
> * 0 based data and get the next disk. */
This comment is misleading, we should be capable of getting statistics
from gluster even for inactive domains. Which is probably the reason we
only look for the presence of protocol if the API failed.
> @@ -518,9 +521,16 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
> virGetLastErrorDomain() == VIR_FROM_STORAGE) {
> memset(, 0, sizeof(info));
> vshResetLibvirtError();
> +} else if (device_type != NULL &&
> +(STRCASEEQ(device_type, "cdrom") ||
> +STRCASEEQ(device_type, "floppy"))) {
> +memset(, 0, sizeof(info));
> +vshResetLibvirtError();
What if the cdrom is not empty and the error was something else?
Whatever error occurs, you get dashes instead of actual numbers.
Yes, the question is whether an error for something else than an empty
source should be reported.
To preserve that, doing a virXPathBoolean query on the source element should
say whether it makes sense to invoke the API or not. (Maybe fill in
dashes instead of zeroes as proposed in the discussion last time [0])
Don't know if that's what you're talking about but I already get a dashed
output. For instance:
virsh # domblkinfo --all archlinux
Target Capacity Allocation Physical
--
vda 16106127360 6850265088 16108814336
sda - --
fda - --
If querying beforehand is the right way I'm happy to fix the patch.
a) if we care about preserving the individual errors, please check for
source and skip the API call instead
b) if we do not, we can ignore all errors and simplify the code
Either way looks nicer to me than introducing a specific 'ResetError()'
call.
Jano
Alternatively, we can resurrect that patch [1] that optimistically
ignored all errors and save ourselves the trouble of having to add
another exception here later.
Jano
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/2] qemu: make attaching disk partition to VM illegal
On Wed, Oct 02, 2019 at 02:04:28PM +0200, Michal Privoznik wrote: > On 10/2/19 1:11 PM, Daniel P. Berrangé wrote: > > On Wed, Oct 02, 2019 at 10:45:28AM +0200, Michal Privoznik wrote: > > > On 9/30/19 3:41 PM, Pavel Mores wrote: > > > > The way in which the qemu driver generates aliases for disks involves > > > > ignoring the partition number part of a target dev name. This means > > > > that > > > > all partitions of a block device and the device itself all end up with > > > > the > > > > same alias. If multiple such disks are specified in XML, the resulting > > > > name clash makes qemu invocation fail. > > > > > > > > Since attaching partitions to qemu VMs doesn't seem to make much sense > > > > anyway, disallow partitions in target specifications altogether. > > > > > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1346265 > > > > > > > > Signed-off-by: Pavel Mores > > > > --- > > > >src/qemu/qemu_domain.c| 15 +++ > > > >.../disk-attaching-partition-nosupport.xml| 27 > > > > +++ > > > >tests/qemuxml2argvtest.c | 1 + > > > >3 files changed, 43 insertions(+) > > > >create mode 100644 > > > > tests/qemuxml2argvdata/disk-attaching-partition-nosupport.xml > > > > > > > > > I think it is fine for freeze, so go ahead with your proposed fix. > > > > Thanks, I've made the change and pushed these. > > Congratulations Pavel on your first libvirt contribution! Cheers, and apologies for the additional work you had with it. :-) pvl -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/1] qemu_command: tidy up qemuBuildHostdevCommandLine loop
The current 'for' loop with 5 consecutive 'ifs' inside
qemuBuildHostdevCommandLine can be a bit smarter:
- all 5 'ifs' fails if hostdev->mode is not equal to
VIR_DOMAIN_HOSTDEV_MODE_SUBSYS. This check can be moved to the
start of the loop, failing to the next element immediately
in case it fails;
- all 5 'ifs' checks for a specific subsys->type to build the proper
command line argument (virHostdevIsSCSIDevice and virHostdevIsMdevDevice
do that but within a helper). Problem is that the code will keep
checking for matches even if one was already found, and there is
no way a hostdev will fit more than one 'if' (i.e. a hostdev can't
have 2+ different types). This means that a SUBSYS_TYPE_USB will
create its command line argument in the first 'if', then all other
conditionals will surely fail but will end up being checked anyway.
This can be avoided by adding 'continue' statements inside the first
4 conditionals.
Signed-off-by: Daniel Henrique Barboza
---
Arguably safe for freeze since it's trivial, but not a deal
breaker if postponed. I am fine with both.
src/qemu/qemu_command.c | 20 ++--
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 8c979fdf65..0357481dd1 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5471,20 +5471,23 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
virDomainHostdevSubsysPtr subsys = >source.subsys;
VIR_AUTOFREE(char *) devstr = NULL;
+if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
+continue;
+
/* USB */
-if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
-subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) {
+if (subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) {
virCommandAddArg(cmd, "-device");
if (!(devstr =
qemuBuildUSBHostdevDevStr(def, hostdev, qemuCaps)))
return -1;
virCommandAddArg(cmd, devstr);
+
+continue;
}
/* PCI */
-if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
-subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) {
+if (subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) {
int backend = subsys->u.pci.backend;
if (backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
@@ -5514,6 +5517,8 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
if (!devstr)
return -1;
virCommandAddArg(cmd, devstr);
+
+continue;
}
/* SCSI */
@@ -5543,11 +5548,12 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
if (!(devstr = qemuBuildSCSIHostdevDevStr(def, hostdev)))
return -1;
virCommandAddArg(cmd, devstr);
+
+continue;
}
/* SCSI_host */
-if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
-subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST) {
+if (subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST) {
if (hostdev->source.subsys.u.scsi_host.protocol ==
VIR_DOMAIN_HOSTDEV_SUBSYS_SCSI_HOST_PROTOCOL_TYPE_VHOST) {
VIR_AUTOFREE(char *) vhostfdName = NULL;
@@ -5573,6 +5579,8 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
virCommandAddArg(cmd, devstr);
}
+
+continue;
}
/* MDEV */
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: fixed handling of targetless disks (e.g. empty CDROM) in 'domblkinfo' cmd
On Wed, Oct 02, 2019 at 01:15:12PM +0200, Ján Tomko wrote:
> Real estate in the commit message summary is precious, so the e.g.
> part belongs in the commit message.
Right.
> Also, (unless some strange usage sneaked into our XML), deals
> with the host-side where says how the device appears in the
> guest, so 'targetless' is wrong here.
>
> On Tue, Oct 01, 2019 at 04:45:02PM +0200, Pavel Mores wrote:
> > virDomainGetBlockInfo() returns error if called on a disk with no target
>
> s/target/source/
Yes, sorry for the mixup.
> > (a targetless disk might be a removable media drive with no media in it,
> > for instance an empty CDROM drive).
> >
> > So far this caused the virsh domblkinfo --all command to abort and ignore
>
> Most of virsh commands correspond 1:1 to a libvirt API. It would be
> nicer if that was the case here, since now we have to guess what
> happened in the daemon, because we try to treat --all as
> --all-that-are-sensible-and-supported
Could you please elaborate? I don't have enough context yet, I'm not sure what
in particular the above means for this patch.
> > any remaining (not yet displayed) disk devices. This patch fixes it by
> > ignoring virDomainGetBlockInfo() errors for CDROM and floppy drives,
> > similar to how it's done for network drives.
> >
>
> I don't think that's an approach that should be copied, see below
>
> > https://bugzilla.redhat.com/show_bug.cgi?id=1619625
> >
> > Signed-off-by: Pavel Mores
> > ---
> > tools/virsh-domain-monitor.c | 11 +++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
> > index 0e2c4191d7..0f495c1a3f 100644
> > --- a/tools/virsh-domain-monitor.c
> > +++ b/tools/virsh-domain-monitor.c
> > @@ -473,6 +473,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
> > char *cap = NULL;
> > char *alloc = NULL;
> > char *phy = NULL;
> > +char *device_type = NULL;
> > vshTablePtr table = NULL;
> >
> > if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
> > @@ -510,6 +511,8 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
> > rc = virDomainGetBlockInfo(dom, target, , 0);
> >
> > if (rc < 0) {
> > +device_type = virXPathString("string(./@device)", ctxt);
> > +
> > /* If protocol is present that's an indication of a
> > networked
> > * storage device which cannot provide statistics, so
> > generate
> > * 0 based data and get the next disk. */
>
> This comment is misleading, we should be capable of getting statistics
> from gluster even for inactive domains. Which is probably the reason we
> only look for the presence of protocol if the API failed.
>
> > @@ -518,9 +521,16 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
> > virGetLastErrorDomain() == VIR_FROM_STORAGE) {
> > memset(, 0, sizeof(info));
> > vshResetLibvirtError();
> > +} else if (device_type != NULL &&
> > +(STRCASEEQ(device_type, "cdrom") ||
> > +STRCASEEQ(device_type, "floppy"))) {
> > +memset(, 0, sizeof(info));
> > +vshResetLibvirtError();
>
> What if the cdrom is not empty and the error was something else?
Whatever error occurs, you get dashes instead of actual numbers.
> To preserve that, doing a virXPathBoolean query on the source element should
> say whether it makes sense to invoke the API or not. (Maybe fill in
> dashes instead of zeroes as proposed in the discussion last time [0])
Don't know if that's what you're talking about but I already get a dashed
output. For instance:
virsh # domblkinfo --all archlinux
Target Capacity Allocation Physical
--
vda 16106127360 6850265088 16108814336
sda - --
fda - --
If querying beforehand is the right way I'm happy to fix the patch.
> Alternatively, we can resurrect that patch [1] that optimistically
> ignored all errors and save ourselves the trouble of having to add
> another exception here later.
>
> Jano
>
> > } else {
> > goto cleanup;
> > }
> > +
> > +VIR_FREE(device_type);
> > }
> >
> > if (!cmdDomblkinfoGet(ctl, , , , , human))
>
> [0] https://www.redhat.com/archives/libvir-list/2018-August/msg01332.html
> [1] https://www.redhat.com/archives/libvir-list/2018-August/msg01300.html
>
> > @@ -556,6 +566,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
> > VIR_FREE(target);
> > VIR_FREE(protocol);
> > VIR_FREE(disks);
> > +VIR_FREE(device_type);
> > xmlXPathFreeContext(ctxt);
> > xmlFreeDoc(xmldoc);
> > return ret;
> > --
> > 2.21.0
> >
> > --
> > libvir-list mailing list
> > libvir-list@redhat.com
>
Re: [libvirt] [PATCH v2 1/2] qemu: make attaching disk partition to VM illegal
On 10/2/19 1:11 PM, Daniel P. Berrangé wrote: On Wed, Oct 02, 2019 at 10:45:28AM +0200, Michal Privoznik wrote: On 9/30/19 3:41 PM, Pavel Mores wrote: The way in which the qemu driver generates aliases for disks involves ignoring the partition number part of a target dev name. This means that all partitions of a block device and the device itself all end up with the same alias. If multiple such disks are specified in XML, the resulting name clash makes qemu invocation fail. Since attaching partitions to qemu VMs doesn't seem to make much sense anyway, disallow partitions in target specifications altogether. https://bugzilla.redhat.com/show_bug.cgi?id=1346265 Signed-off-by: Pavel Mores --- src/qemu/qemu_domain.c| 15 +++ .../disk-attaching-partition-nosupport.xml| 27 +++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 43 insertions(+) create mode 100644 tests/qemuxml2argvdata/disk-attaching-partition-nosupport.xml I think it is fine for freeze, so go ahead with your proposed fix. Thanks, I've made the change and pushed these. Congratulations Pavel on your first libvirt contribution! Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: fixed handling of targetless disks (e.g. empty CDROM) in 'domblkinfo' cmd
On 10/1/19 11:45 AM, Pavel Mores wrote:
virDomainGetBlockInfo() returns error if called on a disk with no target
(a targetless disk might be a removable media drive with no media in it,
for instance an empty CDROM drive).
So far this caused the virsh domblkinfo --all command to abort and ignore
any remaining (not yet displayed) disk devices. This patch fixes it by
ignoring virDomainGetBlockInfo() errors for CDROM and floppy drives,
similar to how it's done for network drives.
https://bugzilla.redhat.com/show_bug.cgi?id=1619625
Signed-off-by: Pavel Mores
---
tools/virsh-domain-monitor.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index 0e2c4191d7..0f495c1a3f 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -473,6 +473,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
char *cap = NULL;
char *alloc = NULL;
char *phy = NULL;
+char *device_type = NULL;
I believe you need to use VIR_AUTO(char *) here. Even considering that
the macro will be deprecated in the near future for glib stuff, by using
VIR_AUTO here:
- you'll make it easier to introduce the glib replacement, since
s/VIR_AUTO/ is a trivial change;
- you won't be adding more VIR_FREE() on top of existing cases that will
need to be addressed in the future.
Thanks,
DHB
vshTablePtr table = NULL;
if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
@@ -510,6 +511,8 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
rc = virDomainGetBlockInfo(dom, target, , 0);
if (rc < 0) {
+device_type = virXPathString("string(./@device)", ctxt);
+
/* If protocol is present that's an indication of a networked
* storage device which cannot provide statistics, so generate
* 0 based data and get the next disk. */
@@ -518,9 +521,16 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
virGetLastErrorDomain() == VIR_FROM_STORAGE) {
memset(, 0, sizeof(info));
vshResetLibvirtError();
+} else if (device_type != NULL &&
+(STRCASEEQ(device_type, "cdrom") ||
+STRCASEEQ(device_type, "floppy"))) {
+memset(, 0, sizeof(info));
+vshResetLibvirtError();
} else {
goto cleanup;
}
+
+VIR_FREE(device_type);
}
if (!cmdDomblkinfoGet(ctl, , , , , human))
@@ -556,6 +566,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
VIR_FREE(target);
VIR_FREE(protocol);
VIR_FREE(disks);
+VIR_FREE(device_type);
xmlXPathFreeContext(ctxt);
xmlFreeDoc(xmldoc);
return ret;
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: fixed handling of targetless disks (e.g. empty CDROM) in 'domblkinfo' cmd
Real estate in the commit message summary is precious, so the e.g.
part belongs in the commit message.
Also, (unless some strange usage sneaked into our XML), deals
with the host-side where says how the device appears in the
guest, so 'targetless' is wrong here.
On Tue, Oct 01, 2019 at 04:45:02PM +0200, Pavel Mores wrote:
virDomainGetBlockInfo() returns error if called on a disk with no target
s/target/source/
(a targetless disk might be a removable media drive with no media in it,
for instance an empty CDROM drive).
So far this caused the virsh domblkinfo --all command to abort and ignore
Most of virsh commands correspond 1:1 to a libvirt API. It would be
nicer if that was the case here, since now we have to guess what
happened in the daemon, because we try to treat --all as
--all-that-are-sensible-and-supported
any remaining (not yet displayed) disk devices. This patch fixes it by
ignoring virDomainGetBlockInfo() errors for CDROM and floppy drives,
similar to how it's done for network drives.
I don't think that's an approach that should be copied, see below
https://bugzilla.redhat.com/show_bug.cgi?id=1619625
Signed-off-by: Pavel Mores
---
tools/virsh-domain-monitor.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index 0e2c4191d7..0f495c1a3f 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -473,6 +473,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
char *cap = NULL;
char *alloc = NULL;
char *phy = NULL;
+char *device_type = NULL;
vshTablePtr table = NULL;
if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
@@ -510,6 +511,8 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
rc = virDomainGetBlockInfo(dom, target, , 0);
if (rc < 0) {
+device_type = virXPathString("string(./@device)", ctxt);
+
/* If protocol is present that's an indication of a networked
* storage device which cannot provide statistics, so generate
* 0 based data and get the next disk. */
This comment is misleading, we should be capable of getting statistics
from gluster even for inactive domains. Which is probably the reason we
only look for the presence of protocol if the API failed.
@@ -518,9 +521,16 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
virGetLastErrorDomain() == VIR_FROM_STORAGE) {
memset(, 0, sizeof(info));
vshResetLibvirtError();
+} else if (device_type != NULL &&
+(STRCASEEQ(device_type, "cdrom") ||
+STRCASEEQ(device_type, "floppy"))) {
+memset(, 0, sizeof(info));
+vshResetLibvirtError();
What if the cdrom is not empty and the error was something else?
To preserve that, doing a virXPathBoolean query on the source element should
say whether it makes sense to invoke the API or not. (Maybe fill in
dashes instead of zeroes as proposed in the discussion last time [0])
Alternatively, we can resurrect that patch [1] that optimistically
ignored all errors and save ourselves the trouble of having to add
another exception here later.
Jano
} else {
goto cleanup;
}
+
+VIR_FREE(device_type);
}
if (!cmdDomblkinfoGet(ctl, , , , , human))
[0] https://www.redhat.com/archives/libvir-list/2018-August/msg01332.html
[1] https://www.redhat.com/archives/libvir-list/2018-August/msg01300.html
@@ -556,6 +566,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
VIR_FREE(target);
VIR_FREE(protocol);
VIR_FREE(disks);
+VIR_FREE(device_type);
xmlXPathFreeContext(ctxt);
xmlFreeDoc(xmldoc);
return ret;
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/2] qemu: make attaching disk partition to VM illegal
On Wed, Oct 02, 2019 at 10:45:28AM +0200, Michal Privoznik wrote:
> On 9/30/19 3:41 PM, Pavel Mores wrote:
> > The way in which the qemu driver generates aliases for disks involves
> > ignoring the partition number part of a target dev name. This means that
> > all partitions of a block device and the device itself all end up with the
> > same alias. If multiple such disks are specified in XML, the resulting
> > name clash makes qemu invocation fail.
> >
> > Since attaching partitions to qemu VMs doesn't seem to make much sense
> > anyway, disallow partitions in target specifications altogether.
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=1346265
> >
> > Signed-off-by: Pavel Mores
> > ---
> > src/qemu/qemu_domain.c| 15 +++
> > .../disk-attaching-partition-nosupport.xml| 27 +++
> > tests/qemuxml2argvtest.c | 1 +
> > 3 files changed, 43 insertions(+)
> > create mode 100644
> > tests/qemuxml2argvdata/disk-attaching-partition-nosupport.xml
> >
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index e8e895d9aa..545c985f40 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -5880,6 +5880,8 @@ qemuDomainDeviceDefValidateDisk(const
> > virDomainDiskDef *disk,
> > {
> > const char *driverName = virDomainDiskGetDriver(disk);
> > virStorageSourcePtr n;
> > +int idx;
> > +int partition;
> > if (disk->src->shared && !disk->src->readonly &&
> > !qemuBlockStorageSourceSupportsConcurrentAccess(disk->src)) {
> > @@ -5948,6 +5950,19 @@ qemuDomainDeviceDefValidateDisk(const
> > virDomainDiskDef *disk,
> > return -1;
> > }
> > +if (virDiskNameParse(disk->dst, , ) < 0) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("invalid disk target '%s'"), disk->dst);
> > +return -1;
> > +}
> > +
> > +if (partition != 0) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("can't attach disk partition '%s', please attach
> > whole disk instead"),
> > + disk->dst);
>
> Hopefully it's not too late, but this error message and the commit title
> neither don't look okay. You can attach /dev/sda1 to a domain, but we don't
> want you to put "sda1" into the disk target, we want you to name it just
> "sda". So perhaps "Refuse partitions in disk targets" as commit tile and
> "invalid disk target '%s', partitions can't appear in disk targets" for the
> error message looks better?
>
> The patch looks goot otherwise. Currently we are in a freeze, but since this
> is a bug fix it can go in. Objections anybody? I volunteer to merge it.
I think it is fine for freeze, so go ahead with your proposed fix.
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/2] qemu: make attaching disk partition to VM illegal
On 9/30/19 3:41 PM, Pavel Mores wrote:
The way in which the qemu driver generates aliases for disks involves
ignoring the partition number part of a target dev name. This means that
all partitions of a block device and the device itself all end up with the
same alias. If multiple such disks are specified in XML, the resulting
name clash makes qemu invocation fail.
Since attaching partitions to qemu VMs doesn't seem to make much sense
anyway, disallow partitions in target specifications altogether.
https://bugzilla.redhat.com/show_bug.cgi?id=1346265
Signed-off-by: Pavel Mores
---
src/qemu/qemu_domain.c| 15 +++
.../disk-attaching-partition-nosupport.xml| 27 +++
tests/qemuxml2argvtest.c | 1 +
3 files changed, 43 insertions(+)
create mode 100644
tests/qemuxml2argvdata/disk-attaching-partition-nosupport.xml
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index e8e895d9aa..545c985f40 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5880,6 +5880,8 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef
*disk,
{
const char *driverName = virDomainDiskGetDriver(disk);
virStorageSourcePtr n;
+int idx;
+int partition;
if (disk->src->shared && !disk->src->readonly &&
!qemuBlockStorageSourceSupportsConcurrentAccess(disk->src)) {
@@ -5948,6 +5950,19 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef
*disk,
return -1;
}
+if (virDiskNameParse(disk->dst, , ) < 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("invalid disk target '%s'"), disk->dst);
+return -1;
+}
+
+if (partition != 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("can't attach disk partition '%s', please attach whole
disk instead"),
+ disk->dst);
Hopefully it's not too late, but this error message and the commit title
neither don't look okay. You can attach /dev/sda1 to a domain, but we
don't want you to put "sda1" into the disk target, we want you to name
it just "sda". So perhaps "Refuse partitions in disk targets" as commit
tile and "invalid disk target '%s', partitions can't appear in disk
targets" for the error message looks better?
The patch looks goot otherwise. Currently we are in a freeze, but since
this is a bug fix it can go in. Objections anybody? I volunteer to merge it.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
