Re: [libvirt] [PATCH alt] conf: Allow user define their own alias
On Fri, Sep 29, 2017 at 09:06:01AM +0200, Michal Privoznik wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1434451 > > It comes handy for management application to be able to have a > per-device label so that it can uniquely identify devices it > cares about. The advantage of this approach is that we don't have > to generate aliases at define time (non trivial amount of work > and problems). The only thing we do is parse the user supplied > tag and format it back. For instance: > > > > > > > > > > Signed-off-by: Michal Privoznik > --- > > An alternative approach to: > > https://www.redhat.com/archives/libvir-list/2017-September/msg00765.html > > Honestly, I prefer this one as it's simpler and we don't have to care about > devices changing their aliases on cold plug. I mean, on cold (un-)plug we'd > have to regenerate the aliases so it might be hard to track certain device. > But with this approach, it's no problem. Can you elaborate on you mean by the cold plug problem here ? Are you saying that if you cold plug a single device, we would regenerate aliases on every single other device too ? I would surely think we can avoid that kind of thing. To make hotplug easier for applications we could add a V2 hotplug API which *returns* the fully-expanded device XML. That way applications would immediately learn the alias of the newly hotplugged device in a simple manner. While we were at it, this would give us opportunity to make the v2 hotplug API support hotplugging of multiple devices at once. This is a feature we've needed for PCI assignment where some devices have to be co-assigned at the same time. 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 3/3] conf: Allow users to define UUID for devices
On Tue, Oct 03, 2017 at 04:03:20PM +0200, Martin Kletzander wrote: > On Tue, Oct 03, 2017 at 02:53:46PM +0100, Daniel P. Berrange wrote: > > On Tue, Oct 03, 2017 at 02:11:44PM +0200, Martin Kletzander wrote: > > > On Tue, Oct 03, 2017 at 12:58:59PM +0200, Michal Privoznik wrote: > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1434451 > > > > > > > > It comes handy for management application to be able to have a > > > > per-device label so that it can uniquely identify devices it > > > > cares about. The advantage of this approach is that we don't have > > > > to generate aliases at define time (non trivial amount of work > > > > and problems). The only thing we do is parse the user supplied > > > > UUID and format it back. For instance: > > > > > > > > > > > > > > > > > > > > > > > > 1efaf08b-9317-4b0f-b227-912e4bd9f483 > > > > > > > > > > > > > > > > Signed-off-by: Michal Privoznik > > > > --- > > > > > > > > This is just a very basic implementation. If I get a green light on > > > > this, I can > > > > implement the feature further, i.e. allow device lookup on the UUID. For > > > > instance: > > > > > > > > virsh domiftune fedora $UUID $bandwidth > > > > > > > > I'm thinking that part of the problem we're having with agreeing how to > > deal with this RFE is that we're over-analysing semantics, by wondering > > whether its a unique name or UUID, its relation to alias, whether it has > > bearing on APIs. > > > > How about we change tack, and do what we did when we needed application > > specific information at the top level - just declare a general purpose > > element and say it is a completely opaque blob. Libvirt will > > *never* do anything with it, other than to preserve it exactly as is. > > No API will ever use the metadata in any way. Libvirt will never try to > > guarantee uniqueness of metadata for each device. It can be JSON or a > > gziped microsoft word document for all we care. Entirely upto the app > > developer to decide what metadata is saved and guarantee uniqueness if > > they so desired. > > > > That is kind of what I was aiming for. But in order for it to be cleaner and > easier to use from user as well (and not only mgmt apps) I thought the > metadata > would just be one identifier. If you want to store more metadata for the > device, then you can do all that in the domain metadata and just reference the > particular device using the identifier if mgmt app wants to do that. Yes that is certainly possible. The caveats are that we still need a unique identifier for the device, and the metadata update is not atomic wrt to device hotplug. The plus side of the global metadata is that we have APIs to update it on the fly already, and its fully namespaced to allow multiple independant data sets to be stored. I don't think lack of atomicity is a big deal as you could order it so that you update metadata before doing the hotplug. Then worst case you have a device mentioned in metadata that doesn't exist, which is easy enough to detect. 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 3/3] conf: Allow users to define UUID for devices
On Tue, Oct 03, 2017 at 02:11:44PM +0200, Martin Kletzander wrote: > On Tue, Oct 03, 2017 at 12:58:59PM +0200, Michal Privoznik wrote: > > https://bugzilla.redhat.com/show_bug.cgi?id=1434451 > > > > It comes handy for management application to be able to have a > > per-device label so that it can uniquely identify devices it > > cares about. The advantage of this approach is that we don't have > > to generate aliases at define time (non trivial amount of work > > and problems). The only thing we do is parse the user supplied > > UUID and format it back. For instance: > > > > > > > > > > > > 1efaf08b-9317-4b0f-b227-912e4bd9f483 > > > > > > > > Signed-off-by: Michal Privoznik > > --- > > > > This is just a very basic implementation. If I get a green light on this, I > > can > > implement the feature further, i.e. allow device lookup on the UUID. For > > instance: > > > > virsh domiftune fedora $UUID $bandwidth I'm thinking that part of the problem we're having with agreeing how to deal with this RFE is that we're over-analysing semantics, by wondering whether its a unique name or UUID, its relation to alias, whether it has bearing on APIs. How about we change tack, and do what we did when we needed application specific information at the top level - just declare a general purpose element and say it is a completely opaque blob. Libvirt will *never* do anything with it, other than to preserve it exactly as is. No API will ever use the metadata in any way. Libvirt will never try to guarantee uniqueness of metadata for each device. It can be JSON or a gziped microsoft word document for all we care. Entirely upto the app developer to decide what metadata is saved and guarantee uniqueness if they so desired. 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] spec: Delay supported_platform check
On Tue, Oct 03, 2017 at 01:48:04PM +0200, Jiri Denemark wrote: > Building RPM should only be allowed on a supported platform, but > unpacking the source and applying all patches can be done anywhere. > > Signed-off-by: Jiri Denemark > --- > libvirt.spec.in | 9 + > 1 file changed, 5 insertions(+), 4 deletions(-) ACK 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 1/3] hyperv: Fix hypervInitConnection error reporting
On Tue, Oct 03, 2017 at 11:04:56AM +0200, Ladi Prosek wrote: > "%s is not a Hyper-V server" is not a correct generalization of all possible > error conditions of hypervEnumAndPull. For example: > > $ virsh --connect hyperv://localhost/?transport=http > Enter username for localhost [administrator]: > Enter administrator's password for localhost: > error: failed to connect to the hypervisor > error: internal error: localhost is not a Hyper-V server > > This commit fixes hypervEnumAndPull to set a meaningful internal error on all > error paths and removes the general virReportError. > > The same scenario with the fix: > > $ virsh --connect hyperv://localhost/?transport=http > Enter username for localhost [administrator]: > Enter administrator's password for localhost: > error: failed to connect to the hypervisor > error: internal error: Transport error during enumeration: User, password or > similar was not accepted (26) > > Signed-off-by: Ladi Prosek > --- > src/hyperv/hyperv_driver.c | 2 -- > src/hyperv/hyperv_wmi.c| 1 + > 2 files changed, 1 insertion(+), 2 deletions(-) > > diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c > index 09912610c..52274239c 100644 > --- a/src/hyperv/hyperv_driver.c > +++ b/src/hyperv/hyperv_driver.c > @@ -105,8 +105,6 @@ hypervInitConnection(virConnectPtr conn, hypervPrivate > *priv, > priv->wmiVersion = HYPERV_WMI_VERSION_V1; > > if (hypervEnumAndPull(priv, &wqlQuery, &computerSystem) < 0) { > -virReportError(VIR_ERR_INTERNAL_ERROR, > - _("%s is not a Hyper-V server"), > conn->uri->server); This bit is ok... > goto cleanup; > } > } > diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c > index 980a00e28..6a51eff20 100644 > --- a/src/hyperv/hyperv_wmi.c > +++ b/src/hyperv/hyperv_wmi.c > @@ -985,6 +985,7 @@ hypervEnumAndPull(hypervPrivate *priv, hypervWqlQueryPtr > wqlQuery, > hypervObject *object; > > if (virBufferCheckError(wqlQuery->query) < 0) { > +virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid query")); ...but virBufferCheckError already reports an error, so this is invalid. 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] [libvirt-jenkins-ci PATCH 05/18] ansible: Add libvirt-cim project
On Tue, Oct 03, 2017 at 09:18:32AM +0200, Andrea Bolognani wrote: > On Mon, 2017-10-02 at 16:57 +0100, Daniel P. Berrange wrote: > > > +packages: > > > + - libcmpiutil-devel > > > + - libconfig-devel > > > + - libuuid-devel > > > + - libxml2-devel > > > + - libxslt > > > + - wget > > > + > > > +extra_packages: > > > + - libvirt-devel > > > > This isn't right - we should never install libvirt-devel on any of the > > build hosts. Downstream projects are chained up to build against the > > version of libvirt we just built. The same applies for other deps > > we build that are used by other downstream pieces. > > That's why it's in 'extra_packages' rather than 'packages' ;) > > Again, this is only used during development. > > See group_vars/all/main.yml: > > # Wether to build software. While this is useful for figuring out > # build dependencies, it should not be turned on in production > # because it causes extra packages to be installed, which can > # interfere with the CI jobs > build: false > > and tasks/packages.yml: > > - name: '{{ project }}: Install extra packages' > package: > name: '{{ item }}' > state: present > with_items: > '{{ extra_packages }}' > when: > - extra_packages is defined > # Only extra additional packages if we're going to build > - build > > Yuck, I accidentally that comment. You get the idea though. > > > Is there any way to get inheritance between these 'vars' files, so we don't > > copy+paste the same content for every Fedora/CentOS version ? > > Mh, I have the feeling it might complicate things rather than > make them nicer. I can look into it, though. One of the big wins when I moved the configs out of Jenkins native config format, into the Jenkins Job Builder config format is that we could eliminate all the duplication between the configs. So from that POV it feels unpleasant to be re-adding lots of duplication. In the very last patch you add a bunch of files which define aliases for the various dependancies, and map those to the distro specific package name eg +cyrus-sasl: + - cyrus-sasl # FreeBSD + - cyrus-sasl-devel # CentOS, Fedora + - libsasl2-dev # Debian, Ubuntu Given these data maps, it seems like we ought to be able to define the build pre-requisites once in terms of the alias names, and then expand that into the distro specific package lists, thus avoiding a per-distro list of deps for each module 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] [libvirt-jenkins-ci PATCH 03/18] ansible: Add libosinfo project
On Tue, Oct 03, 2017 at 09:07:15AM +0200, Andrea Bolognani wrote: > On Mon, 2017-10-02 at 17:04 +0100, Daniel P. Berrange wrote: > > > +- name: '{{ project }}: Run sanity checks' > > > + command: '{{ make }} -j{{ smp }} syntax-check' > > > + args: > > > +chdir: '{{ project }}' > > > + when: > > > +- project == 'libosinfo' > > > > This looks like its duplicating the rules we already have > > defined in the jobs/ files for Jenkins. Why do we need > > this ? > > It's explained in the commit message for patch 2: > > Building projects is entirely controlled by Jenkins in a production > environment, but it can be useful to trigger builds via Ansible > during development to figure out build dependencies and for sanity > checking purposes. > > I'm not adamant on keeping this around, as it was mainly a tool > for me to use during development, but I figure it might come in > handy later on when some project picks up new (optional) build > dependencies or we need to add a new project, and I'd rather not > have to reinvent the wheel when that happens. > > Note that none of those tasks is executed unless you manually set > the 'build' variable to true, so they will not interfere in any > way with regular CI usage. The rules we have defined in the jobs & projects files are in nice parsable format. Could we have something that just reads that data and then generates these ansible rules for building outside jenkins 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] [libvirt-jenkins-ci PATCH 03/18] ansible: Add libosinfo project
On Mon, Oct 02, 2017 at 05:10:41PM +0200, Andrea Bolognani wrote: > Signed-off-by: Andrea Bolognani > --- > ansible/host_vars/libvirt-centos-7/main.yml | 1 + > ansible/host_vars/libvirt-debian-8/main.yml | 1 + > ansible/host_vars/libvirt-debian-9/main.yml | 1 + > ansible/host_vars/libvirt-fedora-25/main.yml | 1 + > ansible/host_vars/libvirt-fedora-26/main.yml | 1 + > ansible/host_vars/libvirt-fedora-rawhide/main.yml | 1 + > ansible/host_vars/libvirt-freebsd-11/main.yml | 1 + > ansible/host_vars/libvirt-ubuntu-14/main.yml | 1 + > ansible/host_vars/libvirt-ubuntu-16/main.yml | 1 + > ansible/tasks/build.yml | 40 > +++ > ansible/vars/libosinfo/CentOS-7.yml | 19 +++ > ansible/vars/libosinfo/Debian-8.yml | 19 +++ > ansible/vars/libosinfo/Debian-9.yml | 19 +++ > ansible/vars/libosinfo/Fedora-25.yml | 19 +++ > ansible/vars/libosinfo/Fedora-26.yml | 19 +++ > ansible/vars/libosinfo/Fedora-Rawhide.yml | 19 +++ > ansible/vars/libosinfo/FreeBSD-11.yml | 19 +++ > ansible/vars/libosinfo/Ubuntu-14.yml | 19 +++ > ansible/vars/libosinfo/Ubuntu-16.yml | 19 +++ > 19 files changed, 220 insertions(+) > create mode 100644 ansible/vars/libosinfo/CentOS-7.yml > create mode 100644 ansible/vars/libosinfo/Debian-8.yml > create mode 100644 ansible/vars/libosinfo/Debian-9.yml > create mode 100644 ansible/vars/libosinfo/Fedora-25.yml > create mode 100644 ansible/vars/libosinfo/Fedora-26.yml > create mode 100644 ansible/vars/libosinfo/Fedora-Rawhide.yml > create mode 100644 ansible/vars/libosinfo/FreeBSD-11.yml > create mode 100644 ansible/vars/libosinfo/Ubuntu-14.yml > create mode 100644 ansible/vars/libosinfo/Ubuntu-16.yml > > diff --git a/ansible/tasks/build.yml b/ansible/tasks/build.yml > index edb0064..2210261 100644 > --- a/ansible/tasks/build.yml > +++ b/ansible/tasks/build.yml > @@ -12,3 +12,43 @@ >command: git clean -xdf >args: > chdir: '{{ project }}' > + > +# C build > + > +- name: '{{ project }}: Prepare configure options' > + set_fact: > +configure_options: [] > + > +- name: '{{ project }}: Prepare configure options' > + set_fact: > +configure_options: '{{ configure_options }} + [ "{{ features[item] }}" ]' > + with_items: > +'{{ features }}' > + when: > +- features is defined > + > +- name: '{{ project }}: Prepare configure options' > + set_fact: > +configure_options: '{{ configure_options | join(" ") }}' > + > +- name: '{{ project }}: Run autogen.sh' > + command: './autogen.sh {{ configure_options }}' > + args: > +chdir: '{{ project }}' > + environment: > + when: > +- project == 'libosinfo' > + > +- name: '{{ project }}: Build project' > + command: '{{ make }} -j{{ smp }}' > + args: > +chdir: '{{ project }}' > + when: > +- project == 'libosinfo' > + > +- name: '{{ project }}: Run sanity checks' > + command: '{{ make }} -j{{ smp }} syntax-check' > + args: > +chdir: '{{ project }}' > + when: > +- project == 'libosinfo' This looks like its duplicating the rules we already have defined in the jobs/ files for Jenkins. Why do we need this ? 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] [libvirt-jenkins-ci PATCH 05/18] ansible: Add libvirt-cim project
On Mon, Oct 02, 2017 at 05:10:43PM +0200, Andrea Bolognani wrote: > Signed-off-by: Andrea Bolognani > --- > ansible/host_vars/libvirt-centos-6/main.yml | 1 + > ansible/host_vars/libvirt-centos-7/main.yml | 1 + > ansible/host_vars/libvirt-fedora-25/main.yml | 1 + > ansible/host_vars/libvirt-fedora-26/main.yml | 1 + > ansible/host_vars/libvirt-fedora-rawhide/main.yml | 1 + > ansible/tasks/build.yml | 19 +++ > ansible/tasks/packages.yml| 11 +++ > ansible/vars/libvirt-cim/CentOS-6.yml | 11 +++ > ansible/vars/libvirt-cim/CentOS-7.yml | 11 +++ > ansible/vars/libvirt-cim/Fedora-25.yml| 11 +++ > ansible/vars/libvirt-cim/Fedora-26.yml| 11 +++ > ansible/vars/libvirt-cim/Fedora-Rawhide.yml | 11 +++ > 12 files changed, 86 insertions(+), 4 deletions(-) > create mode 100644 ansible/vars/libvirt-cim/CentOS-6.yml > create mode 100644 ansible/vars/libvirt-cim/CentOS-7.yml > create mode 100644 ansible/vars/libvirt-cim/Fedora-25.yml > create mode 100644 ansible/vars/libvirt-cim/Fedora-26.yml > create mode 100644 ansible/vars/libvirt-cim/Fedora-Rawhide.yml > > diff --git a/ansible/host_vars/libvirt-centos-6/main.yml > b/ansible/host_vars/libvirt-centos-6/main.yml > index 4d53bb3..f6fe574 100644 > --- a/ansible/host_vars/libvirt-centos-6/main.yml > +++ b/ansible/host_vars/libvirt-centos-6/main.yml > @@ -2,3 +2,4 @@ > projects: >- base >- libvirt > + - libvirt-cim > diff --git a/ansible/host_vars/libvirt-centos-7/main.yml > b/ansible/host_vars/libvirt-centos-7/main.yml > index 7508f19..ec2e50f 100644 > --- a/ansible/host_vars/libvirt-centos-7/main.yml > +++ b/ansible/host_vars/libvirt-centos-7/main.yml > @@ -3,3 +3,4 @@ projects: >- base >- libosinfo >- libvirt > + - libvirt-cim > diff --git a/ansible/host_vars/libvirt-fedora-25/main.yml > b/ansible/host_vars/libvirt-fedora-25/main.yml > index 7508f19..ec2e50f 100644 > --- a/ansible/host_vars/libvirt-fedora-25/main.yml > +++ b/ansible/host_vars/libvirt-fedora-25/main.yml > @@ -3,3 +3,4 @@ projects: >- base >- libosinfo >- libvirt > + - libvirt-cim > diff --git a/ansible/host_vars/libvirt-fedora-26/main.yml > b/ansible/host_vars/libvirt-fedora-26/main.yml > index 7508f19..ec2e50f 100644 > --- a/ansible/host_vars/libvirt-fedora-26/main.yml > +++ b/ansible/host_vars/libvirt-fedora-26/main.yml > @@ -3,3 +3,4 @@ projects: >- base >- libosinfo >- libvirt > + - libvirt-cim > diff --git a/ansible/host_vars/libvirt-fedora-rawhide/main.yml > b/ansible/host_vars/libvirt-fedora-rawhide/main.yml > index 7508f19..ec2e50f 100644 > --- a/ansible/host_vars/libvirt-fedora-rawhide/main.yml > +++ b/ansible/host_vars/libvirt-fedora-rawhide/main.yml > @@ -3,3 +3,4 @@ projects: >- base >- libosinfo >- libvirt > + - libvirt-cim > diff --git a/ansible/tasks/build.yml b/ansible/tasks/build.yml > index be9ecd3..2a26024 100644 > --- a/ansible/tasks/build.yml > +++ b/ansible/tasks/build.yml > @@ -38,7 +38,15 @@ >environment: >when: > - ( project == 'libosinfo' or > -project == 'libvirt' ) > +project == 'libvirt' or > +project == 'libvirt-cim' ) > + > +- name: '{{ project }}: Run configure' > + command: './configure {{ configure_options }}' > + args: > +chdir: '{{ project }}' > + when: > + - project == 'libvirt-cim' > > - name: '{{ project }}: Build project' >command: '{{ make }} -j{{ smp }}' > @@ -46,7 +54,8 @@ > chdir: '{{ project }}' >when: > - ( project == 'libosinfo' or > -project == 'libvirt' ) > +project == 'libvirt' or > +project == 'libvirt-cim' ) > > - name: '{{ project }}: Run sanity checks' >command: '{{ make }} -j{{ smp }} syntax-check' > @@ -63,7 +72,8 @@ > chdir: '{{ project }}' >when: > - ( ( project == 'libvirt' and > - os_name != 'FreeBSD' ) ) > + os_name != 'FreeBSD' ) or > +project == 'libvirt-cim' ) > > - name: '{{ project }}: Build RPM package' >command: '{{ make }} -j{{ smp }} rpm' > @@ -72,4 +82,5 @@ >when: > - ( os_name == 'CentOS' or > os_name == 'Fedora' ) > -- ( project == 'libvirt' ) > +- ( project == 'libvirt' or > +project == 'libvirt-cim' ) > diff --git a/ansible/tasks/packages.yml b/ansible/tasks/packages.yml > index 630d794..7a66899 100644 > --- a/ansible/tasks/packages.yml > +++ b/ansible/tasks/packages.yml > @@ -9,3 +9,14 @@ > state: present >with_items: > '{{ packages }}' > + > +- name: '{{ project }}: Install extra packages' > + package: > +name: '{{ item }}' > +state: present > + with_items: > +'{{ extra_packages }}' > + when: > +- extra_packages is defined > +# Only extra additional packages if we're going to build later on > +- build > diff --git a/ansible/vars/libvirt-cim/C
Re: [libvirt] [PATCH go-xml] qemu: extend serial console source
On Mon, Oct 02, 2017 at 11:34:18AM +, Jeroen Simonetti wrote: > October 2 2017 12:47 PM, "Daniel P. Berrange" wrote: > >> This will change the type of `DomainSerial.Source` from > >> `*DomainChardevSource` to a new `*DomainSerialSource`. > >> > >> This is done to add support for networked serial ports and > >> keep the original DomainChardevSource unchanged. > > > > I'm not sure I see the point of this. Any chardev backed device type > > (parallel, serial, vmchannel) should be allowed to use network > > backends - there's no restriction that says its only for serial > > ports. > > > > I was not aware that all chardev backed devices should be able to use network > backend. > Does this mean I can simple extend the DomainChardevSource, add the required > options > and there will be no naming conflict? > > If so, this would mean there is no need for a breaking change. > If you would be so kind to confirm the above, I will send in a new patch with > the > additional fields. Yes, just add extra fields to the DomainChardevSource 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 go-xml] qemu: extend serial console source
On Mon, Oct 02, 2017 at 12:10:40PM +0200, Jeroen Simonetti wrote: > *Warning* this is a BWC breaking change! That's fine - we don't promise API compat for this module. > This will change the type of `DomainSerial.Source` from > `*DomainChardevSource` to a new `*DomainSerialSource`. > > This is done to add support for networked serial ports and > keep the original DomainChardevSource unchanged. I'm not sure I see the point of this. Any chardev backed device type (parallel, serial, vmchannel) should be allowed to use network backends - there's no restriction that says its only for serial ports. > > DomainSerialSource contains all possible xml variations > for a serial device source. > > Signed-off-by: Jeroen Simonetti > --- > domain.go | 32 ++-- > domain_test.go | 30 -- > 2 files changed, 54 insertions(+), 8 deletions(-) > > diff --git a/domain.go b/domain.go > index 8c2cc1b..3221423 100644 > --- a/domain.go > +++ b/domain.go > @@ -309,12 +309,32 @@ type DomainConsole struct { > } > > type DomainSerial struct { > - XMLName xml.Name `xml:"serial"` > - Typestring `xml:"type,attr"` > - Source *DomainChardevSource `xml:"source"` > - Target *DomainSerialTarget `xml:"target"` > - Alias *DomainAlias `xml:"alias"` > - Address *DomainAddress `xml:"address"` > + XMLName xml.Name `xml:"serial"` > + Type string`xml:"type,attr"` > + Source *DomainSerialSource `xml:"source"` > + Protocol *DomainSerialProtocol `xml:"protocol"` > + Target *DomainSerialTarget `xml:"target"` > + Alias*DomainAlias `xml:"alias"` > + Address *DomainAddress`xml:"address"` > +} > + > +type DomainSerialSource struct { > + Mode string `xml:"mode,attr,omitempty"` > + Path string `xml:"path,attr,omitempty"` > + Append string `xml:"append,attr,omitempty"` > + Host string `xml:"host,attr,omitempty"` > + Service string `xml:"service,attr,omitempty"` > + TLS string `xml:"tls,attr,omitempty"` > + SecLabel *DomainSerialSourceSecLabel `xml:"seclabel"` > +} > + > +type DomainSerialProtocol struct { > + Type string `xml:"type,attr"` > +} > + > +type DomainSerialSourceSecLabel struct { > + Model string `xml:"model,attr,omitempty"` > + Relabel string `xml:"relabel,attr,omitempty"` > } > > type DomainChannel struct { > diff --git a/domain_test.go b/domain_test.go > index 4fe6bfe..d301ace 100644 > --- a/domain_test.go > +++ b/domain_test.go > @@ -314,9 +314,28 @@ var domainTestData = []struct { > }, > DomainSerial{ > Type: "file", > - Source: &DomainChardevSource{ > + Source: &DomainSerialSource{ > Path: > "/tmp/serial.log", > Append: "off", > + SecLabel: > &DomainSerialSourceSecLabel{ > + Model: "dac", > + Relabel: "no", > + }, > + }, > + Target: &DomainSerialTarget{ > + Port: &serialPort, > + }, > + }, > + DomainSerial{ > + Type: "tcp", > + Source: &DomainSerialSource{ > + Mode:"bind", > + Host:"127.0.0.1", > + Service: "1234", > + TLS: "yes", > + }, > + Protocol: &DomainSerialProtocol{ > + Type: "telnet", > }, > Target: &DomainSerialTarget{ > Port: &serialPort, > @@ -382,7 +401,14 @@ var domainTestData = []struct { > ` `, > ``, > ``, > - ` append="off">`, > +
Re: [libvirt] [Qemu-devel] libvirt/QEMU/SEV interaction
On Sat, Sep 30, 2017 at 12:16:55AM +0300, Michael S. Tsirkin wrote: > On Fri, Sep 29, 2017 at 02:48:45PM -0500, Richard Relph wrote: > > On 9/29/17 2:34 PM, Michael S. Tsirkin wrote: > > > On Wed, Sep 27, 2017 at 02:06:10PM -0500, Richard Relph wrote: > > > > Whether the "BIOS" is a "static shim" as Michael suggests, or a full > > > > BIOS, > > > > or even a BIOS+kernel+initrd is really not too significant. What is > > > > significant is that the GO has a basis for trusting all code that is > > > > imported in to their VM by the CP. And that NONE of the code provided > > > > by the > > > > CP is "unknown" and unauditable by the GO. If the CP has a way to inject > > > > code unknown to the GO in to the guest VM, the trust model is broken and > > > > both GO and CP suffer the consequences. > > > > > > Absolutely. > > > > > > > When the CP needs to update the BIOS image, they will have to inform > > > > the GO > > > > and allow the GO to establish trust in the CP's new BIOS image somehow. > > > > > > This GO update on every BIOS change is imho is not a workable model. You > > > want something like checking the BIOS signature instead. And since > > > hardware is all hash based, you need the shim to do it in software. > > > > A BIOS "signed" by the CP doesn't meet the security requirement. It is code > > that is "unknown" to the GO. > > There is a misunderstanding here. > > BIOS would not be signed by a CP. It would be signed by a trusted > software vendor e.g. by Red Hat. > > > The (legitimate) CP does NOT want to be in that position of trust. If they > > are, then some government somewhere is going to insist that they sign a BIOS > > that allows the government to spy on the GO's VMs, and steal secrets from > > it. Or some hacker admin will do it "for fun". > > > > How often do large public CPs really change their BIOSes? My sense is that > > large public CPs prefer stability over "latest and greatest". > > CPs just do dnf update. Software vendors change BIOSes. No they really don't do that. Cloud providers will stick on a golden image of RHEL. If and when they need to do an upgrade, they won't usually do a 'dnf update' because that imposes risk of breaking VMs on that node with no fallback path. Instead they would usually deploy brand new nodes with the new software version, and then live migrate VMs off the old hosts. > And we do change them. Look at number of revisions for seabios in e.g. > Fedora. More importantly we might need to change them quickly e.g. > because of a security issue. Adding the need to coordinate with all GOs > is not going to work. Neither can QEMU support booting old BIOS > versions on new machine types indefinitely. I don't think Fedora is a good fit for SEV - a long term supported distro like RHEL is the more apt target, and there you are looking at 6 months lifetime, except in rare cases of having a BIOS security flaw to pyush out. > > > And, perhaps more importantly, if a CP are able to sell a "more secure" VM, > > one that justifies a higher price per vCPU hour, wouldn't that warrant some > > changes in the "insecure" model being used today? > > > > Richard > > Absolutely. CPs have no business signing images. But it is just not > really feasible for software vendors to distribute hashes. Why can't software vendors distribute hashes - they are best placed to, particularly when a single vendor is supplying the whole stack of base OS, virt and OpenStack as a single coordinated product. 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] [Qemu-devel] libvirt/QEMU/SEV interaction
On Fri, Sep 29, 2017 at 02:48:45PM -0500, Richard Relph wrote: > On 9/29/17 2:34 PM, Michael S. Tsirkin wrote: > > On Wed, Sep 27, 2017 at 02:06:10PM -0500, Richard Relph wrote: > > > Whether the "BIOS" is a "static shim" as Michael suggests, or a full BIOS, > > > or even a BIOS+kernel+initrd is really not too significant. What is > > > significant is that the GO has a basis for trusting all code that is > > > imported in to their VM by the CP. And that NONE of the code provided by > > > the > > > CP is "unknown" and unauditable by the GO. If the CP has a way to inject > > > code unknown to the GO in to the guest VM, the trust model is broken and > > > both GO and CP suffer the consequences. > > > > Absolutely. > > > > > When the CP needs to update the BIOS image, they will have to inform the > > > GO > > > and allow the GO to establish trust in the CP's new BIOS image somehow. > > > > This GO update on every BIOS change is imho is not a workable model. You > > want something like checking the BIOS signature instead. And since > > hardware is all hash based, you need the shim to do it in software. > > A BIOS "signed" by the CP doesn't meet the security requirement. It is code > that is "unknown" to the GO. > > The (legitimate) CP does NOT want to be in that position of trust. If they > are, then some government somewhere is going to insist that they sign a BIOS > that allows the government to spy on the GO's VMs, and steal secrets from > it. Or some hacker admin will do it "for fun". > > How often do large public CPs really change their BIOSes? My sense is that > large public CPs prefer stability over "latest and greatest". It is hard to generalize, but from a RHEL POV, we typically do major updates of the virt stack every ~6 months, and these will include BIOS updates. So if a cloud vendor is following the RHEL update stream actively that's the kind of cadence you'd expect. The gotcha would come if there were out-of-band security updates for BIOS which caused it to be updated before the 6 month window. Fortunately I've not see these happen often, so I don't think its a fatal problem. IOW, I tend to agree with you that this is not really a blocking problem to the use of SEV in cloud. > And, perhaps more importantly, if a CP are able to sell a "more secure" VM, > one that justifies a higher price per vCPU hour, wouldn't that warrant some > changes in the "insecure" model being used today? Yes. 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 alt] conf: Allow user define their own alias
On Fri, Sep 29, 2017 at 03:56:53PM +0200, Michal Privoznik wrote: > On 09/29/2017 01:52 PM, Daniel P. Berrange wrote: > > On Fri, Sep 29, 2017 at 12:09:52PM +0100, Daniel P. Berrange wrote: > >> On Fri, Sep 29, 2017 at 01:04:34PM +0200, Michal Privoznik wrote: > >>> On 09/29/2017 10:49 AM, Daniel P. Berrange wrote: > >>>> On Fri, Sep 29, 2017 at 09:06:01AM +0200, Michal Privoznik wrote: > >>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1434451 > >>>>> > >>>>> It comes handy for management application to be able to have a > >>>>> per-device label so that it can uniquely identify devices it > >>>>> cares about. The advantage of this approach is that we don't have > >>>>> to generate aliases at define time (non trivial amount of work > >>>>> and problems). The only thing we do is parse the user supplied > >>>>> tag and format it back. For instance: > >>>>> > >>>>> > >>>>> > >>>>> > >>>>> > >>>>> > >>>> > >>>> I really do not like this - having two arbitrary string alias names is > >>>> just crazy. > >>> > >>> Why is that? We have plenty of elements that do not match to anything at > >>> hypervisor level. Firstly, there's metadata. Secondly, aliases in lxc > >>> driver don't match anything either. And one can argue that the link in > >>> qemu can be broken too. I mean, it's just for now that the aliases we > >>> report happen to be device IDs we put onto the qemu's cmd line. Not to > >>> mention devices that we put there and not report in the domain XML => no > >>> IDs visible there. > >> > >>>> If we want to add a second attribute to uniquely identify > >>>> devices, then it should be a UUID IMHO, so it at least has some tangible > >>>> benefit instead of just duplicating the existing id format > >>> > >>> I'm failing to see why UUID is better than any arbitrary string. You > >>> mean we would generate the UUIDs if not supplied by user? > >> > >> Some hypervisors could map UUIDs to individual devices, so it is a more > >> generally useful concept. > > > > Also if we have APIs that accept an 'alias' string, we cannot extend them > > to support the user's own 'alias' unless we guarantee the user supplied > > alias is different from the alias we give to QEMU. > > We can if we document that libvirt generated aliases take precedence > over user ones. That way we can keep the backward compatibility. No, that's fragile. If libvirt were ever to change its alias name for something there is a risk it might clash with an app name, which previously did not clash. > > If we used UUID format, > > however, we don't have any ambiguity between a string that's an alias and > > a string that's a UUID. > > So IIUC users would be able to specify UUID for devices they want and > for the rest libvirt will generate new one? That'll make XMLs a lot > bigger. If we will not generate UUIDs, but just store ones provided by > user this also makes sense then. Although, dealing with UUIDs is very > user unfriendly, but that ship sailed a long time ago :-P I would not generate UUIDs - only have them if the app asked for them, or if the hypervisor we're using has assigned them itself. 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 alt] conf: Allow user define their own alias
On Fri, Sep 29, 2017 at 12:09:52PM +0100, Daniel P. Berrange wrote: > On Fri, Sep 29, 2017 at 01:04:34PM +0200, Michal Privoznik wrote: > > On 09/29/2017 10:49 AM, Daniel P. Berrange wrote: > > > On Fri, Sep 29, 2017 at 09:06:01AM +0200, Michal Privoznik wrote: > > >> https://bugzilla.redhat.com/show_bug.cgi?id=1434451 > > >> > > >> It comes handy for management application to be able to have a > > >> per-device label so that it can uniquely identify devices it > > >> cares about. The advantage of this approach is that we don't have > > >> to generate aliases at define time (non trivial amount of work > > >> and problems). The only thing we do is parse the user supplied > > >> tag and format it back. For instance: > > >> > > >> > > >> > > >> > > >> > > >> > > > > > > I really do not like this - having two arbitrary string alias names is > > > just crazy. > > > > Why is that? We have plenty of elements that do not match to anything at > > hypervisor level. Firstly, there's metadata. Secondly, aliases in lxc > > driver don't match anything either. And one can argue that the link in > > qemu can be broken too. I mean, it's just for now that the aliases we > > report happen to be device IDs we put onto the qemu's cmd line. Not to > > mention devices that we put there and not report in the domain XML => no > > IDs visible there. > > > > If we want to add a second attribute to uniquely identify > > > devices, then it should be a UUID IMHO, so it at least has some tangible > > > benefit instead of just duplicating the existing id format > > > > I'm failing to see why UUID is better than any arbitrary string. You > > mean we would generate the UUIDs if not supplied by user? > > Some hypervisors could map UUIDs to individual devices, so it is a more > generally useful concept. Also if we have APIs that accept an 'alias' string, we cannot extend them to support the user's own 'alias' unless we guarantee the user supplied alias is different from the alias we give to QEMU. If we used UUID format, however, we don't have any ambiguity between a string that's an alias and a string that's a UUID. 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 alt] conf: Allow user define their own alias
On Fri, Sep 29, 2017 at 01:04:34PM +0200, Michal Privoznik wrote: > On 09/29/2017 10:49 AM, Daniel P. Berrange wrote: > > On Fri, Sep 29, 2017 at 09:06:01AM +0200, Michal Privoznik wrote: > >> https://bugzilla.redhat.com/show_bug.cgi?id=1434451 > >> > >> It comes handy for management application to be able to have a > >> per-device label so that it can uniquely identify devices it > >> cares about. The advantage of this approach is that we don't have > >> to generate aliases at define time (non trivial amount of work > >> and problems). The only thing we do is parse the user supplied > >> tag and format it back. For instance: > >> > >> > >> > >> > >> > >> > > > > I really do not like this - having two arbitrary string alias names is > > just crazy. > > Why is that? We have plenty of elements that do not match to anything at > hypervisor level. Firstly, there's metadata. Secondly, aliases in lxc > driver don't match anything either. And one can argue that the link in > qemu can be broken too. I mean, it's just for now that the aliases we > report happen to be device IDs we put onto the qemu's cmd line. Not to > mention devices that we put there and not report in the domain XML => no > IDs visible there. > > If we want to add a second attribute to uniquely identify > > devices, then it should be a UUID IMHO, so it at least has some tangible > > benefit instead of just duplicating the existing id format > > I'm failing to see why UUID is better than any arbitrary string. You > mean we would generate the UUIDs if not supplied by user? Some hypervisors could map UUIDs to individual devices, so it is a more generally useful concept. 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 python] Unify whitespace around *_ALLOW_THREADS macros
On Fri, Sep 29, 2017 at 01:28:20AM +0300, Nir Soffer wrote: > Most of the code treats libvirt API calls as separate block, keeping one > blank line before the LIBVIRT_BEGIN_ALLOW_THREAD, and one blank line > after LIBVIRT_END_ALLOW_THREADS. Unify the whitespace so all calls > wrapped with these macros are treated as a separate block. > --- > libvirt-override.c | 115 > - > 1 file changed, 87 insertions(+), 28 deletions(-) Reviewed-by: Daniel P. Berrange and pushed to git. 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 alt] conf: Allow user define their own alias
On Fri, Sep 29, 2017 at 09:06:01AM +0200, Michal Privoznik wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1434451 > > It comes handy for management application to be able to have a > per-device label so that it can uniquely identify devices it > cares about. The advantage of this approach is that we don't have > to generate aliases at define time (non trivial amount of work > and problems). The only thing we do is parse the user supplied > tag and format it back. For instance: > > > > > > I really do not like this - having two arbitrary string alias names is just crazy. If we want to add a second attribute to uniquely identify devices, then it should be a UUID IMHO, so it at least has some tangible benefit instead of just duplicating the existing id format 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] gnulib tests in libvirt broken by newer glibc 2.26
On Thu, Sep 28, 2017 at 04:41:37PM -0700, Paul Eggert wrote: > That patch essentially negates the point of the test, which is that getopt > should be visible from unistd.h. I'd rather fix the problem than nuke the > test. > > Could you explain what the Gnulib problem is here? I can't really see it in > your email. A self-contained example would help. > > For what it's worth, I could not reproduce the problem on Fedora 26 by doing > this in Gnulib (this tells 'configure' to use Gnulib-supplied getopt.h and > getopt.c): Fedora 26 only has glibc 2.25 - you need to have Fedora rawhide to get the broken behaviour, as that has glibc 2.26.90 > ./gnulib-tool --create-testdir --dir foo getopt-posix > cd foo > ./configure gl_cv_func_getopt_posix=no ac_cv_header_getopt_h=no > make > make check 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
[libvirt] [PATCH] Fix vxhs test to have stable certificate dir
The test suite has hardcoded /etc/pki/qemu as the cert dir, but this only works if configure has --sysconfdir=/etc passed. We must set the vxhs cert dir to a stable path in the test suite. Signed-off-by: Daniel P. Berrange --- Pushed as a build fix .../qemuxml2argv-disk-drive-network-tlsx509-vxhs.args | 4 ++-- tests/qemuxml2argvtest.c | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args index 572c9f36c..a75272454 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args @@ -20,7 +20,7 @@ server,nowait \ -no-acpi \ -boot c \ -usb \ --object tls-creds-x509,id=objvirtio-disk0_tls0,dir=/etc/pki/qemu,\ +-object tls-creds-x509,id=objvirtio-disk0_tls0,dir=/etc/pki/libvirt-vxhs,\ endpoint=client,verify-peer=yes \ -drive file.driver=vxhs,file.tls-creds=objvirtio-disk0_tls0,\ file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,file.server.type=tcp,\ @@ -28,7 +28,7 @@ file.server.host=192.168.0.1,file.server.port=,format=raw,if=none,\ id=drive-virtio-disk0,cache=none \ -device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\ id=virtio-disk0 \ --object tls-creds-x509,id=objvirtio-disk1_tls0,dir=/etc/pki/qemu,\ +-object tls-creds-x509,id=objvirtio-disk1_tls0,dir=/etc/pki/libvirt-vxhs,\ endpoint=client,verify-peer=yes \ -drive file.driver=vxhs,file.tls-creds=objvirtio-disk1_tls0,\ file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc252,file.server.type=tcp,\ diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 1958ad428..7271ea07e 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -599,6 +599,9 @@ mymain(void) VIR_FREE(driver.config->chardevTLSx509certdir); if (VIR_STRDUP_QUIET(driver.config->chardevTLSx509certdir, "/etc/pki/libvirt-chardev") < 0) return EXIT_FAILURE; +VIR_FREE(driver.config->vxhsTLSx509certdir); +if (VIR_STRDUP_QUIET(driver.config->vxhsTLSx509certdir, "/etc/pki/libvirt-vxhs") < 0) +return EXIT_FAILURE; VIR_FREE(driver.config->hugetlbfs); if (VIR_ALLOC_N(driver.config->hugetlbfs, 2) < 0) -- 2.13.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] gnulib tests in libvirt broken by newer glibc 2.26
On Wed, Sep 27, 2017 at 11:36:20PM +0200, Christian Ehrhardt wrote: > Hi, > there seems to be an incompatibility to the last glibc due to [1]. > > Eventually this breaks gnulib unittests (and maybe more). [snip] We should have detected this a while ago sinc Fedora rawhide has 2.26, in fact it has pre-release of 2.27. I've just found however that our Jenkins job config had a mistake in it so it wasn't setting VIR_TEST_EXPENSIVE=1 and so the gnulib tests were being skipped. I've fixed jenkins now, so presumably we'll shortly get a failure in jenkins :-) 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] [RFC] docs: Discourage usage of cache mode=passthrough
On Thu, Sep 21, 2017 at 01:14:04PM -0400, Laine Stump wrote: > On 09/19/2017 03:37 PM, Eduardo Habkost wrote: > > Cache mode=passthrough can result in a broken cache topology if > > the domain topology is not exactly the same as the host topology. > > Warn about that in the documentation. > > > > Bug report for reference: > > https://bugzilla.redhat.com/show_bug.cgi?id=1184125 > > > > Signed-off-by: Eduardo Habkost > > --- > > docs/formatdomain.html.in | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > > index 57ec2ff34..9c21892f3 100644 > > --- a/docs/formatdomain.html.in > > +++ b/docs/formatdomain.html.in > > @@ -1478,7 +1478,9 @@ > > > >passthrough > >The real CPU cache data reported by the host CPU will be > > -passed through to the virtual CPU. > > +passed through to the virtual CPU. Using this mode is not > > +recommended unless the domain CPU and NUMA topology is > > exactly > > +the same as the host CPU and NUMA topology. > > To me this sounds like it should be forbidden by libvirt, rather than > just documented as "bad". (I haven't followed any previous discussion on > the topic though, so maybe I'm over-reacting). In high performance setups, people pin guest vCPUs to host pCPUs and set the vCPU topology to match the host pCPU topology they've pinned to. So ohaving a cache mode that matches this topology is just fine. It simply isn't something you want as a default for the more typical floating vCPUs scenarios. 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 v9 4/4] qemu: Add TLS support for Veritas HyperScale (VxHS)
On Wed, Sep 27, 2017 at 05:08:51PM +0200, Peter Krempa wrote: > On Wed, Sep 27, 2017 at 11:05:01 -0400, John Ferlan wrote: > > > > > > On 09/27/2017 10:25 AM, Peter Krempa wrote: > > > On Tue, Sep 19, 2017 at 21:32:46 -0400, John Ferlan wrote: > > >> From: Ashish Mittal > > >> > > >> Alter qemu command line generation in order to possibly add TLS for > > >> a suitably configured domain. > > >> > > >> Sample TLS args generated by libvirt - > > >> > > >> -object tls-creds-x509,id=objvirtio-disk0_tls0,dir=/etc/pki/qemu,\ > > >> endpoint=client,verify-peer=yes \ > > >> -drive file.driver=vxhs,file.tls-creds=objvirtio-disk0_tls0,\ > > >> file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,\ > > >> file.server.type=tcp,file.server.host=192.168.0.1,\ > > >> file.server.port=,format=raw,if=none,\ > > >> id=drive-virtio-disk0,cache=none \ > > >> -device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\ > > >> id=virtio-disk0 > > >> > > >> Update the qemuxml2argvtest with a couple of examples. One for a > > >> simple case and the other a bit more complex where multiple VxHS disks > > >> are added where at least one uses a VxHS that doesn't require TLS > > >> credentials and thus sets the domain disk source attribute "tls = 'no'". > > >> > > >> Update the hotplug to be able to handle processing the tlsAlias whether > > >> it's to add the TLS object when hotplugging a disk or to remove the TLS > > >> object when hot unplugging a disk. The hot plug/unplug code is largely > > >> generic, but the addition code does make the VXHS specific checks only > > >> because it needs to grab the correct config directory and generate the > > >> object as the command line would do. > > >> > > >> Signed-off-by: Ashish Mittal > > >> Signed-off-by: John Ferlan > > >> --- > > >> src/qemu/qemu_block.c | 8 +++ > > >> src/qemu/qemu_command.c| 33 + > > >> src/qemu/qemu_hotplug.c| 79 > > >> ++ > > >> ...-disk-drive-network-tlsx509-multidisk-vxhs.args | 43 > > >> ...v-disk-drive-network-tlsx509-multidisk-vxhs.xml | 50 ++ > > >> ...muxml2argv-disk-drive-network-tlsx509-vxhs.args | 30 > > >> tests/qemuxml2argvtest.c | 7 ++ > > >> 7 files changed, 250 insertions(+) > > >> create mode 100644 > > >> tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args > > >> create mode 100644 > > >> tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.xml > > >> create mode 100644 > > >> tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args > > [...] > > > >> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > > >> index 7dd6e5fd9..7751a608d 100644 > > >> --- a/src/qemu/qemu_hotplug.c > > >> +++ b/src/qemu/qemu_hotplug.c > > >> @@ -156,6 +156,52 @@ qemuDomainPrepareDisk(virQEMUDriverPtr driver, > > >> > > >> > > >> static int > > >> +qemuDomainAddDiskSrcTLSObject(virQEMUDriverPtr driver, > > >> + virDomainObjPtr vm, > > >> + virStorageSourcePtr src, > > >> + const char *srcalias) > > >> +{ > > >> +int ret = -1; > > >> +qemuDomainObjPrivatePtr priv = vm->privateData; > > >> +virJSONValuePtr tlsProps = NULL; > > >> + > > >> +/* NB: Initial implementation doesn't require/use a secret to > > >> decrypt > > >> + * a server certificate, so there's no need to manage a tlsSecAlias > > > > > > client certificate > > > > > > > No it's the server certificate (server-key.pem) that needs the secret in > > order to be decrypted. > > I think both can be encrypted. What I wanted to point out is that it > does not make sense to refer to the server certificate in terms of disks > since they are clients only. I don't think so - AFAIK, the 'password' provided to gnutls in gnutls_certificate_set_x509_key_file2 is only used for the 'key', not the 'cert' data. 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 python] Release the GIL during virDomainGetMemoryStats
On Tue, Sep 26, 2017 at 08:05:59PM +0300, Nir Soffer wrote: > We discovered that the entire python process get stuck for about 30 > seconds when calling virDomain.getMemoryStats() if libvirt is stuck in > virConnect.getAllDomainStats() on inaccessible storage. This blocking > cause a horrible mess in oVirt. > > This patches adds the standard *_ALLOW_THREADS around the call to avoid > this unwanted blocking. > > Signed-off-by: Nir Soffer > --- > libvirt-override.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/libvirt-override.c b/libvirt-override.c > index 9eba4ed..0e33afb 100644 > --- a/libvirt-override.c > +++ b/libvirt-override.c > @@ -363,8 +363,11 @@ libvirt_virDomainMemoryStats(PyObject *self > ATTRIBUTE_UNUSED, > return NULL; > domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain); > > +LIBVIRT_BEGIN_ALLOW_THREADS; > nr_stats = virDomainMemoryStats(domain, stats, > VIR_DOMAIN_MEMORY_STAT_NR, 0); > +LIBVIRT_END_ALLOW_THREADS; > + > if (nr_stats == -1) > return VIR_PY_NONE; Reviewed-by: Daniel P. Berrange I'll squash in the same fix for virDomainGetDiskErrors too 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 go-xml] Add qemu udp unicast tunneling support
On Tue, Sep 26, 2017 at 09:30:17PM +0200, Jeroen Simonetti wrote: > Signed-off-by: Jeroen Simonetti > --- > domain.go | 22 ++ > domain_test.go | 41 + > 2 files changed, 55 insertions(+), 8 deletions(-) Reviewed-by: Daniel P. Berrange Will push to git - thanks for your contribution. 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 go-xml] 1, add memoryBacking for domain. 2, add hugepages, nosharepages, locked, source, access and allocation for memoryBacking. 3, add test case for memoryBacking.
NB, please try to keep the commit message first lin at 72 chars or less, and then have a blank line, before writing the rest of the commit message. I'll fix this one up when pushing. On Tue, Sep 26, 2017 at 10:12:35AM +0800, zhenwei.pi wrote: > Signed-off-by: zhenwei.pi > --- > domain.go | 76 > +++--- > domain_test.go | 38 + > 2 files changed, 95 insertions(+), 19 deletions(-) > > diff --git a/domain_test.go b/domain_test.go > index 3ff1dc1..51d413b 100644 > --- a/domain_test.go > +++ b/domain_test.go > @@ -435,6 +435,33 @@ var domainTestData = []struct { > Value: 16384, > Slots: 2, > }, > + MemorySource: &DomainMemorySource{ > + Type: "file|anonymous", > + }, > + MemoryAccess: &DomainMemoryAccess{ > + Mode: "shared|private", > + }, > + MemoryAllocation: &DomainMemoryAllocation{ > + Mode: "immediate|ondemand", > + }, In these examples, you should pick either 'file' or 'anonymous', not include them both. Likewise for the access & allocation data. I'll fix this when pushing Reviewed-by: Daniel P. Berrange 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 python] Fix comparisons between signed & unsigned integers
On Tue, Sep 26, 2017 at 01:36:13PM +0200, Martin Kletzander wrote: > On Tue, Sep 26, 2017 at 11:16:05AM +0100, Daniel P. Berrange wrote: > > When python3 builds C modules, it adds the -Wsign-compare flag to GCC. > > This creates lots of warnings where we compare a 'size_t' value against > > an 'int' value due to signed/unsigned difference. Change all the size_t > > types to ssize_t to address this. > > > > Signed-off-by: Daniel P. Berrange > > Hm, I wanted this to be the case when we started to force using size_t for > i,j,k > variables, but looks like I was the only one who wanted ssize_t (plus we would > be able to do that only in some places, so automatic enforcement without > -Wsign-compare would not be easily possible) In libvirt.git we disabled -Wsign-compare, but in python the CFlags aren't under our direct control - python distutils decides AFAICT. > ACK 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 4/4] qemu: hot-unplug of watchdog
On Tue, Sep 26, 2017 at 11:38:26AM +0200, Michal Privoznik wrote: > On 09/12/2017 04:29 PM, John Ferlan wrote: > > > > > > On 09/05/2017 07:45 AM, Michal Privoznik wrote: > >> https://bugzilla.redhat.com/show_bug.cgi?id=1447169 > >> > >> Signed-off-by: Michal Privoznik > >> --- > >> src/qemu/qemu_driver.c | 4 +- > >> src/qemu/qemu_hotplug.c| 61 > >> ++ > >> src/qemu/qemu_hotplug.h| 3 ++ > >> tests/qemuhotplugtest.c| 7 ++- > >> .../qemuhotplug-watchdog-full.xml | 3 ++ > >> 5 files changed, 76 insertions(+), 2 deletions(-) > >> create mode 100644 > >> tests/qemuhotplugtestdevices/qemuhotplug-watchdog-full.xml > >> > >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > >> index 626148dba..4c636b956 100644 > >> --- a/src/qemu/qemu_driver.c > >> +++ b/src/qemu/qemu_driver.c > >> @@ -7692,12 +7692,14 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, > >> case VIR_DOMAIN_DEVICE_SHMEM: > >> ret = qemuDomainDetachShmemDevice(driver, vm, dev->data.shmem); > >> break; > >> +case VIR_DOMAIN_DEVICE_WATCHDOG: > >> +ret = qemuDomainDetachWatchdog(driver, vm, dev->data.watchdog); > >> +break; > >> > >> case VIR_DOMAIN_DEVICE_FS: > >> case VIR_DOMAIN_DEVICE_INPUT: > >> case VIR_DOMAIN_DEVICE_SOUND: > >> case VIR_DOMAIN_DEVICE_VIDEO: > >> -case VIR_DOMAIN_DEVICE_WATCHDOG: > >> case VIR_DOMAIN_DEVICE_GRAPHICS: > >> case VIR_DOMAIN_DEVICE_HUB: > >> case VIR_DOMAIN_DEVICE_SMARTCARD: > >> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > >> index 989146eb9..44472ce9f 100644 > >> --- a/src/qemu/qemu_hotplug.c > >> +++ b/src/qemu/qemu_hotplug.c > >> @@ -4360,6 +4360,25 @@ qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver, > >> } > >> > >> > >> +static int > >> +qemuDomainRemoveWatchdog(virQEMUDriverPtr driver, > >> + virDomainObjPtr vm, > >> + virDomainWatchdogDefPtr watchdog) > >> +{ > >> +virObjectEventPtr event = NULL; > >> + > >> +VIR_DEBUG("Removing watchdog %s from domain %p %s", > > > > Is "Removing watchdog watchdog0 from ..." a bit superfluous? > > > > Perhaps just "Removing '%s' from ..." > > > >> + watchdog->info.alias, vm, vm->def->name); > >> + > > > > This would/could be the place for the virDomainAuditWatchdog for > > "detach" too. > > > >> +event = virDomainEventDeviceRemovedNewFromObj(vm, > >> watchdog->info.alias); > >> +qemuDomainEventQueue(driver, event); > >> +qemuDomainReleaseDeviceAddress(vm, &watchdog->info, NULL); > >> +virDomainWatchdogDefFree(vm->def->watchdog); > >> +vm->def->watchdog = NULL; > >> +return 0; > >> +} > >> + > >> + > >> int > >> qemuDomainRemoveDevice(virQEMUDriverPtr driver, > >> virDomainObjPtr vm, > >> @@ -5076,6 +5095,48 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, > >> } > >> > >> > >> +int > >> +qemuDomainDetachWatchdog(virQEMUDriverPtr driver, > >> + virDomainObjPtr vm, > >> + virDomainWatchdogDefPtr dev) > >> +{ > >> +int ret = -1; > >> +virDomainWatchdogDefPtr watchdog; > > > > Why not watchdog = vm->def->watchdog; here? > > > >> +qemuDomainObjPrivatePtr priv = vm->privateData; > >> + > >> +/* While domains can have up to one watchdog, the one supplied by user > > > > s/by/by the/ > > > >> + * doesn't necessarily match the one domain has. Refuse to detach in > >> such > >> + * case. */ > >> +if (!(vm->def->watchdog && > >> + STREQ_NULLABLE(dev->info.alias, > >> + vm->def->watchdog->info.alias))) { > > > > if (!watchdog || STRNEQ_NULLABLE(dev->info.alias, watchdog->info.alias)) > > > > Trying to think why NULLABLE would be necessary... So it seems it's > > required that that input XML has the alias - something not quite right > > with that... > > Is that so? We match devices based on their alias in a lot of places. > Users are expected to pass the full device XML anyway. So it's up to us > how we pick the right device to be detached. For instance, when > detaching a vNIC we match MAC addresses and ignore the rest, since MAC > identifies the device uniquely. So for instance, if the detach XML > provided by user has set but the one in domain doesn't have > it, we detach the device anyway. One can argue this is a buggy > behaviour. But I just don't think we should care. There's a line we have > to draw between protecting users from themselves and too complex and > mostly useless code. So we've documented that users are supposed to pass > the device XML as is in the domain XML. We can protect ourselves from the danger of user passing incomplete device XML by not using the passed in device XML directly. IOW, once we parse the XML to create our virDom
[libvirt] [PATCH python] Fix comparisons between signed & unsigned integers
When python3 builds C modules, it adds the -Wsign-compare flag to GCC. This creates lots of warnings where we compare a 'size_t' value against an 'int' value due to signed/unsigned difference. Change all the size_t types to ssize_t to address this. Signed-off-by: Daniel P. Berrange --- libvirt-lxc-override.c | 2 +- libvirt-override.c | 108 - libvirt-utils.c| 8 ++-- typewrappers.c | 2 +- 4 files changed, 60 insertions(+), 60 deletions(-) diff --git a/libvirt-lxc-override.c b/libvirt-lxc-override.c index 316a500..60c2e48 100644 --- a/libvirt-lxc-override.c +++ b/libvirt-lxc-override.c @@ -63,7 +63,7 @@ libvirt_lxc_virDomainLxcOpenNamespace(PyObject *self ATTRIBUTE_UNUSED, unsigned int flags; int c_retval; int *fdlist = NULL; -size_t i; +ssize_t i; if (!PyArg_ParseTuple(args, (char *)"OI:virDomainLxcOpenNamespace", &pyobj_domain, &flags)) diff --git a/libvirt-override.c b/libvirt-override.c index 9eba4ed..bde7f4b 100644 --- a/libvirt-override.c +++ b/libvirt-override.c @@ -187,7 +187,7 @@ libvirt_virDomainGetCPUStats(PyObject *self ATTRIBUTE_UNUSED, PyObject *error = NULL; int ncpus = -1, start_cpu = 0; int sumparams = 0, nparams = -1; -size_t i; +ssize_t i; int i_retval; unsigned int flags; bool totalflag; @@ -354,7 +354,7 @@ libvirt_virDomainMemoryStats(PyObject *self ATTRIBUTE_UNUSED, virDomainPtr domain; PyObject *pyobj_domain; unsigned int nr_stats; -size_t i; +ssize_t i; virDomainMemoryStatStruct stats[VIR_DOMAIN_MEMORY_STAT_NR]; PyObject *info; PyObject *key = NULL, *val = NULL; @@ -365,7 +365,7 @@ libvirt_virDomainMemoryStats(PyObject *self ATTRIBUTE_UNUSED, nr_stats = virDomainMemoryStats(domain, stats, VIR_DOMAIN_MEMORY_STAT_NR, 0); -if (nr_stats == -1) +if (nr_stats == (unsigned int)-1) return VIR_PY_NONE; /* convert to a Python dictionary */ @@ -1204,7 +1204,7 @@ libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED, virDomainInfo dominfo; virVcpuInfoPtr cpuinfo = NULL; unsigned char *cpumap = NULL; -size_t cpumaplen, i; +ssize_t cpumaplen, i; int i_retval, cpunum; if (!PyArg_ParseTuple(args, (char *)"O:virDomainGetVcpus", @@ -1274,7 +1274,7 @@ libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED, } for (i = 0; i < dominfo.nrVirtCpu; i++) { PyObject *info = PyTuple_New(cpunum); -size_t j; +ssize_t j; if (info == NULL) goto cleanup; @@ -1384,7 +1384,7 @@ libvirt_virDomainGetVcpuPinInfo(PyObject *self ATTRIBUTE_UNUSED, PyObject *pyobj_domain, *pycpumaps = NULL, *error = NULL; virDomainInfo dominfo; unsigned char *cpumaps = NULL; -size_t cpumaplen, vcpu, pcpu; +ssize_t cpumaplen, vcpu, pcpu; unsigned int flags; int i_retval, cpunum; @@ -1496,8 +1496,8 @@ libvirt_virDomainGetEmulatorPinInfo(PyObject *self ATTRIBUTE_UNUSED, PyObject *pyobj_domain; PyObject *pycpumap; unsigned char *cpumap; -size_t cpumaplen; -size_t pcpu; +ssize_t cpumaplen; +ssize_t pcpu; unsigned int flags; int ret; int cpunum; @@ -1560,7 +1560,7 @@ libvirt_virDomainGetIOThreadInfo(PyObject *self ATTRIBUTE_UNUSED, PyObject *py_iothrinfo = NULL; virDomainIOThreadInfoPtr *iothrinfo = NULL; unsigned int flags; -size_t pcpu, i; +ssize_t pcpu, i; int niothreads, cpunum; if (!PyArg_ParseTuple(args, (char *)"OI:virDomainGetIOThreadInfo", @@ -1846,7 +1846,7 @@ virConnectCredCallbackWrapper(virConnectCredentialPtr cred, PyObject *pycb; PyObject *pyret = NULL; int ret = -1; -size_t i; +ssize_t i; LIBVIRT_ENSURE_THREAD_STATE; @@ -1953,7 +1953,7 @@ libvirt_virConnectOpenAuth(PyObject *self ATTRIBUTE_UNUSED, auth.ncredtype = PyList_Size(pycredtype); if (auth.ncredtype) { -size_t i; +ssize_t i; if (VIR_ALLOC_N(auth.credtype, auth.ncredtype) < 0) return PyErr_NoMemory(); for (i = 0; i < auth.ncredtype; i++) { @@ -2046,7 +2046,7 @@ libvirt_virConnectGetCPUModelNames(PyObject *self ATTRIBUTE_UNUSED, virConnectPtr conn; PyObject *rv = NULL, *pyobj_conn; char **models = NULL; -size_t i; +ssize_t i; unsigned int flags = 0; const char *arch = NULL; @@ -2117,7 +2117,7 @@ libvirt_virConnectListDomainsID(PyObject *self ATTRIBUTE_UNUSED, { PyObject *py_retval; int *ids = NULL, c_retval; -size_t i; +ssize_t i; virConnectPtr conn; PyObject *pyobj_conn; @@ -2172,7 +2172,7 @@ libvirt_virConnectListAllDomains(PyObject *self ATTRIBUTE_UNUSED, virConnectPtr conn; virDomainPtr *doms = NULL; int c_retval = 0; -size_t i; +
Re: [libvirt] [PATCH v2 0/6] libvirt-python/libvirtaio patches for 3.8.0
On Tue, Sep 26, 2017 at 12:45:16AM +0200, Wojtek Porczyk wrote: > On Mon, Sep 25, 2017 at 12:14:24PM +0100, Daniel P. Berrange wrote: > > On Fri, Sep 22, 2017 at 05:54:04PM +0200, Wojtek Porczyk wrote: > > > Hi libvirt-list, > > > > > > I'd like to submit a second iteration of libvirtaio series for 3.8.0. > > > There are some improvements to have better control over closing > > > connections, which are important in test suites which repeatedly open > > > and close connections. Also there are some minor bugfixes and better > > > logging. > > > > There's something strange in this posting - the patches I received are all > > missing 'libvir-list@redhat.com' in the To: field - its almost like this > > was Bcc'd to the list which means anyone replying doesn't copy the list > > address on their reply. Even more odd though, I got a second copy of > > patch 3 which does have the To field set. > > I'm sorry, this was my fault. I sent the whole series by [b]ouncing this in > mutt, because it otherwise mangles Message-Id and In-Reply-To: fields. The > previous posting of this series has this problem. I did it two times, since > there was a delay in delivery (I don't know why, maybe greylisting) and > I worried that the first batch was rejected for the lack of To: field, which > I forgot at the time of git format-patch. The delay wasn't anything you did - the mail server was having significant delays processing mail at the latter half of last week, and thus randomly delaying stuff for people. FWIW, if possible with your local network/mail setup, for sending patches we recommend using 'git send-email' rather than doing it from a mail client since it gets threading right automagically. Anyway I've pushed this series to git master, so thanks for your continued contributions :-) 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 6/6] libvirtaio: add .drain() coroutine
On Tue, Sep 26, 2017 at 12:47:00AM +0200, Wojtek Porczyk wrote: > On Mon, Sep 25, 2017 at 02:53:01PM +0100, Daniel P. Berrange wrote: > > On Thu, Sep 14, 2017 at 02:41:12AM +0200, Wojtek Porczyk wrote: > > > The intended use is to ensure that the implementation is empty, which is > > > one way to ensure that all connections were properly closed and file > > > descriptors reclaimed. > > > > > > Signed-off-by: Wojtek Porczyk > > > --- > (snip) > > > +@asyncio.coroutine > > > +def drain(self): > > > +'''Wait for the implementation to become idle. > > > + > > > +This is a coroutine. > > > +''' > > > +self.log.debug('drain()') > > > +if self._pending: > > > +yield from self._finished.wait() > > > +self.log.debug('drain ended') > > > > What is responsible for calling 'drain' ? > > Users of the library, and they do it at their pleasure. This is to allow the > loop to actually run the scheduled tasks. After calling virConnect.close() the > handles/timeouts aren't actually closed until the loop run the scheduled > callbacks. In practice a simple `yield` in a coroutine would be also > sufficient since respective Tasks are in the _ready queue and all run during > next loop cycle, but that's not guaranteed in asyncio specification. Ah I see. Reviewed-by: Daniel P. Berrange 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
[libvirt] [PATCH python] Avoid implicit treatment of an arithmetic result as a boolean
Latest GCC versions are unhappy with us treating an integer arithmetic result as a boolean: libvirt-utils.c: In function ‘virReallocN’: libvirt-utils.c:111:23: warning: ‘*’ in boolean context, suggest ‘&&’ instead [-Wint-in-bool-context] if (!tmp && (size * count)) { ~~^~~~ Add an explicit comparison '!= 0' to keep it happy, since its suggestion to use '&&' is nonsense. Signed-off-by: Daniel P. Berrange --- libvirt-utils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libvirt-utils.c b/libvirt-utils.c index b8ac5e9..f7b4478 100644 --- a/libvirt-utils.c +++ b/libvirt-utils.c @@ -108,7 +108,7 @@ virReallocN(void *ptrptr, return -1; } tmp = realloc(*(void**)ptrptr, size * count); -if (!tmp && (size * count)) { +if (!tmp && ((size * count) != 0)) { return -1; } *(void**)ptrptr = tmp; -- 2.13.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/6] libvirtaio: add more debug logging
On Tue, Sep 26, 2017 at 12:46:02AM +0200, Wojtek Porczyk wrote: > On Mon, Sep 25, 2017 at 02:43:56PM +0100, Daniel P. Berrange wrote: > > On Thu, Aug 24, 2017 at 09:22:49PM +0200, Wojtek Porczyk wrote: > > > This logging is helpful for tracing problems with unclosed connections > > > and leaking file descriptors. > > > > > > Signed-off-by: Wojtek Porczyk > > > --- > > > libvirtaio.py | 33 + > > > 1 file changed, 25 insertions(+), 8 deletions(-) > > > > > > diff --git a/libvirtaio.py b/libvirtaio.py > > > index 7c8c396..46de9ab 100644 > > > --- a/libvirtaio.py > > > +++ b/libvirtaio.py > > > @@ -74,7 +74,7 @@ class Callback(object): > > > def close(self): > > > '''Schedule *ff* callback''' > > > self.impl.log.debug('callback %d close(), scheduling ff', > > > self.iden) > > > -self.impl.schedule_ff_callback(self.opaque) > > > +self.impl.schedule_ff_callback(self.iden, self.opaque) > > > > > > # > > > # file descriptors > > > @@ -275,6 +275,10 @@ class virEventAsyncIOImpl(object): > > > self.descriptors = DescriptorDict(self) > > > self.log = logging.getLogger(self.__class__.__name__) > > > > > > +def __repr__(self): > > > +return '<{} callbacks={} descriptors={}>'.format( > > > +type(self).__name__, self.callbacks, self.descriptors) > > > + > > > def register(self): > > > '''Register this instance as event loop implementation''' > > > # pylint: disable=bad-whitespace > > > @@ -284,9 +288,18 @@ class virEventAsyncIOImpl(object): > > > self._add_timeout, self._update_timeout, > > > self._remove_timeout) > > > return self > > > > > > -def schedule_ff_callback(self, opaque): > > > +def schedule_ff_callback(self, iden, opaque): > > > '''Schedule a ff callback from one of the handles or timers''' > > > -self.loop.call_soon(libvirt.virEventInvokeFreeCallback, opaque) > > > +ensure_future(self._ff_callback(iden, opaque), loop=self.loop) > > > > This use of ensure_future puts a min python version of 3.4 on the code. Is > > there a strong reason to prefer that over the previous call_soon code ? > > 1) There is no technical reason whatsoever. This is an artifact from the > previous iteration. > > 2) In fact ensure_future() does not make it incompatible with python<3.4 > because of the try/except import at the beginning of the file (in python<3.4.3 > the function is called asyncio.async, imported as ensure_future). Ah ok, I missed that bit of existing code. That's fine then: Reviewed-by: Daniel P. Berrange 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 6/6] libvirtaio: add .drain() coroutine
On Thu, Sep 14, 2017 at 02:41:12AM +0200, Wojtek Porczyk wrote: > The intended use is to ensure that the implementation is empty, which is > one way to ensure that all connections were properly closed and file > descriptors reclaimed. > > Signed-off-by: Wojtek Porczyk > --- > libvirtaio.py | 36 ++-- > 1 file changed, 34 insertions(+), 2 deletions(-) > > diff --git a/libvirtaio.py b/libvirtaio.py > index 97a7f6c..1c432dd 100644 > --- a/libvirtaio.py > +++ b/libvirtaio.py > @@ -269,10 +269,27 @@ class virEventAsyncIOImpl(object): > self.descriptors = DescriptorDict(self) > self.log = logging.getLogger(self.__class__.__name__) > > +# NOTE invariant: _finished.is_set() iff _pending == 0 > +self._pending = 0 > +self._finished = asyncio.Event(loop=loop) > +self._finished.set() > + > def __repr__(self): > return '<{} callbacks={} descriptors={}>'.format( > type(self).__name__, self.callbacks, self.descriptors) > > +def _pending_inc(self): > +'''Increase the count of pending affairs. Do not use directly.''' > +self._pending += 1 > +self._finished.clear() > + > +def _pending_dec(self): > +'''Decrease the count of pending affairs. Do not use directly.''' > +assert self._pending > 0 > +self._pending -= 1 > +if self._pending == 0: > +self._finished.set() > + > def register(self): > '''Register this instance as event loop implementation''' > # pylint: disable=bad-whitespace > @@ -293,7 +310,20 @@ class virEventAsyncIOImpl(object): > This is a coroutine. > ''' > self.log.debug('ff_callback(iden=%d, opaque=...)', iden) > -return libvirt.virEventInvokeFreeCallback(opaque) > +ret = libvirt.virEventInvokeFreeCallback(opaque) > +self._pending_dec() > +return ret > + > +@asyncio.coroutine > +def drain(self): > +'''Wait for the implementation to become idle. > + > +This is a coroutine. > +''' > +self.log.debug('drain()') > +if self._pending: > +yield from self._finished.wait() > +self.log.debug('drain ended') What is responsible for calling 'drain' ? > > def is_idle(self): > '''Returns False if there are leftovers from a connection > @@ -301,7 +331,7 @@ class virEventAsyncIOImpl(object): > Those may happen if there are sematical problems while closing > a connection. For example, not deregistered events before .close(). > ''' > -return not self.callbacks > +return not self.callbacks and not self._pending > > def _add_handle(self, fd, event, cb, opaque): > '''Register a callback for monitoring file handle events > @@ -324,6 +354,7 @@ class virEventAsyncIOImpl(object): > fd, event, callback.iden) > self.callbacks[callback.iden] = callback > self.descriptors[fd].add_handle(callback) > +self._pending_inc() > return callback.iden > > def _update_handle(self, watch, event): > @@ -378,6 +409,7 @@ class virEventAsyncIOImpl(object): > timeout, callback.iden) > self.callbacks[callback.iden] = callback > callback.update(timeout=timeout) > +self._pending_inc() > return callback.iden > > def _update_timeout(self, timer, timeout): 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 2/6] libvirtaio: cache the list of callbacks when calling
On Thu, Aug 31, 2017 at 09:20:40PM +0200, Wojtek Porczyk wrote: > When the callback causes something that results in changes wrt > registered handles, python aborts iteration. > > Relevant error message: > > Exception in callback None() > handle: > Traceback (most recent call last): > File "/usr/lib64/python3.5/asyncio/events.py", line 126, in _run > self._callback(*self._args) > File "/usr/lib64/python3.5/site-packages/libvirtaio.py", line 99, in > _handle > for callback in self.callbacks.values(): > RuntimeError: dictionary changed size during iteration > > QubesOS/qubes-issues#2805 > Signed-off-by: Wojtek Porczyk > --- > libvirtaio.py | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libvirtaio.py b/libvirtaio.py > index 46de9ab..d962e64 100644 > --- a/libvirtaio.py > +++ b/libvirtaio.py > @@ -96,7 +96,7 @@ class Descriptor(object): > > :param int event: The event (from libvirt's constants) being > dispatched > ''' > -for callback in self.callbacks.values(): > +for callback in list(self.callbacks.values()): > if callback.event is not None and callback.event & event: > callback.cb(callback.iden, self.fd, event, callback.opaque) Reviewed-by: Daniel P. Berrange 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/6] libvirtaio: add more debug logging
On Thu, Aug 24, 2017 at 09:22:49PM +0200, Wojtek Porczyk wrote: > This logging is helpful for tracing problems with unclosed connections > and leaking file descriptors. > > Signed-off-by: Wojtek Porczyk > --- > libvirtaio.py | 33 + > 1 file changed, 25 insertions(+), 8 deletions(-) > > diff --git a/libvirtaio.py b/libvirtaio.py > index 7c8c396..46de9ab 100644 > --- a/libvirtaio.py > +++ b/libvirtaio.py > @@ -74,7 +74,7 @@ class Callback(object): > def close(self): > '''Schedule *ff* callback''' > self.impl.log.debug('callback %d close(), scheduling ff', self.iden) > -self.impl.schedule_ff_callback(self.opaque) > +self.impl.schedule_ff_callback(self.iden, self.opaque) > > # > # file descriptors > @@ -275,6 +275,10 @@ class virEventAsyncIOImpl(object): > self.descriptors = DescriptorDict(self) > self.log = logging.getLogger(self.__class__.__name__) > > +def __repr__(self): > +return '<{} callbacks={} descriptors={}>'.format( > +type(self).__name__, self.callbacks, self.descriptors) > + > def register(self): > '''Register this instance as event loop implementation''' > # pylint: disable=bad-whitespace > @@ -284,9 +288,18 @@ class virEventAsyncIOImpl(object): > self._add_timeout, self._update_timeout, self._remove_timeout) > return self > > -def schedule_ff_callback(self, opaque): > +def schedule_ff_callback(self, iden, opaque): > '''Schedule a ff callback from one of the handles or timers''' > -self.loop.call_soon(libvirt.virEventInvokeFreeCallback, opaque) > +ensure_future(self._ff_callback(iden, opaque), loop=self.loop) This use of ensure_future puts a min python version of 3.4 on the code. Is there a strong reason to prefer that over the previous call_soon code ? 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 3/6] libvirtaio: do not double-add callbacks
On Thu, Sep 14, 2017 at 02:24:29AM +0200, Wojtek Porczyk wrote: > This was a harmless bug, without any impact, but it is wrong to manage > the collection of callbacks from it's members. > > Signed-off-by: Wojtek Porczyk > --- > libvirtaio.py | 9 - > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/libvirtaio.py b/libvirtaio.py > index d962e64..239561d 100644 > --- a/libvirtaio.py > +++ b/libvirtaio.py > @@ -63,11 +63,6 @@ class Callback(object): > self.cb = cb > self.opaque = opaque > > -assert self.iden not in self.impl.callbacks, \ > -'found {} callback: {!r}'.format( > -self.iden, self.impl.callbacks[self.iden]) > -self.impl.callbacks[self.iden] = self > - > def __repr__(self): > return '<{} iden={}>'.format(self.__class__.__name__, self.iden) > > @@ -324,6 +319,8 @@ class virEventAsyncIOImpl(object): > ''' > callback = FDCallback(self, cb, opaque, > descriptor=self.descriptors[fd], event=event) > +assert callback.iden not in self.callbacks > + > self.log.debug('add_handle(fd=%d, event=%d, cb=..., opaque=...) = > %d', > fd, event, callback.iden) > self.callbacks[callback.iden] = callback > @@ -376,6 +373,8 @@ class virEventAsyncIOImpl(object): > > https://libvirt.org/html/libvirt-libvirt-event.html#virEventAddTimeoutFunc > ''' > callback = TimeoutCallback(self, cb, opaque) > +assert callback.iden not in self.callbacks > + > self.log.debug('add_timeout(timeout=%d, cb=..., opaque=...) = %d', > timeout, callback.iden) > self.callbacks[callback.iden] = callback Reviewed-by: Daniel P. Berrange 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] Print hex values with '0x' prefix and octal with '0' in debug messages
On Mon, Sep 25, 2017 at 02:12:33PM +0200, Pino Toscano wrote: > On Monday, 25 September 2017 12:45:51 CEST Daniel P. Berrange wrote: > > Seeing a log message saying 'flags=93' is ambiguous & confusing unless > > you happen to know that libvirt always prints flags as hex. Change our > > debug messages so that they always add a '0x' prefix when printing flags, > > and '0' prefix when printing mode. A few other misc places gain a '0x' > > prefix in error messages too. > > > > Signed-off-by: Daniel P. Berrange > > --- > > po/af.po | 4 +- > > po/am.po | 4 +- > > po/anp.po | 4 +- > > po/ar.po | 4 +- > > po/as.po | 12 +- > > po/ast.po | 4 +- > > po/bal.po | 4 +- > > po/be.po | 4 +- > > po/bg.po | 4 +- > > po/bn.po | 4 +- > > po/bn_IN.po | 12 +- > > po/bo.po | 4 +- > > po/br.po | 4 +- > > po/brx.po | 4 +- > > po/bs.po | 4 +- > > po/ca.po | 12 +- > > po/cs.po | 4 +- > > po/cy.po | 4 +- > > po/da.po | 4 +- > > po/de.po | 12 +- > > po/de_CH.po | 4 +- > > po/el.po | 4 +- > > po/en_GB.po | 4 +- > > po/eo.po | 4 +- > > po/es.po | 12 +- > > po/et.po | 4 +- > > po/eu.po | 4 +- > > po/fa.po | 4 +- > > po/fi.po | 4 +- > > po/fr.po | 4 +- > > po/gl.po | 4 +- > > po/gu.po | 12 +- > > po/he.po | 4 +- > > po/hi.po | 12 +- > > po/hr.po | 4 +- > > po/hu.po | 4 +- > > po/ia.po | 4 +- > > po/id.po | 4 +- > > po/ilo.po | 4 +- > > po/is.po | 4 +- > > po/it.po | 12 +- > > po/ja.po | 12 +- > > po/ka.po | 4 +- > > po/kk.po | 4 +- > > po/km.po | 4 +- > > po/kn.po | 12 +- > > po/ko.po | 12 +- > > po/kw.po | 4 +- > > po/k...@kkcor.po| 4 +- > > po/k...@uccor.po| 4 +- > > po/kw_GB.po | 4 +- > > po/ky.po | 4 +- > > po/libvirt.pot| 4 +- > > po/lt.po | 4 +- > > po/lv.po | 4 +- > > po/mai.po | 4 +- > > po/mk.po | 4 +- > > po/ml.po | 12 +- > > po/mn.po | 4 +- > > po/mr.po | 4 +- > > po/ms.po | 4 +- > > po/nb.po | 4 +- > > po/nds.po | 4 +- > > po/ne.po | 4 +- > > po/nl.po | 12 +- > > po/nn.po | 4 +- > > po/nso.po | 4 +- > > po/or.po | 4 +- > > po/pa.po | 12 +- > > po/pl.po | 12 +- > > po/pt.po | 4 +- > > po/pt_BR.po | 12 +- > > po/ro.po | 4 +- > > po/ru.po | 8 +- > > po/si.po | 4 +- > > po/sk.po | 4 +- > > po/sl.po | 4 +- > > po/sq.po | 4 +- > > po/sr.po | 4 +- > > po/s...@latin.po| 4 +- > > po/sv.po
Re: [libvirt] [PATCH v2 0/6] libvirt-python/libvirtaio patches for 3.8.0
On Fri, Sep 22, 2017 at 05:54:04PM +0200, Wojtek Porczyk wrote: > Hi libvirt-list, > > I'd like to submit a second iteration of libvirtaio series for 3.8.0. > There are some improvements to have better control over closing > connections, which are important in test suites which repeatedly open > and close connections. Also there are some minor bugfixes and better > logging. There's something strange in this posting - the patches I received are all missing 'libvir-list@redhat.com' in the To: field - its almost like this was Bcc'd to the list which means anyone replying doesn't copy the list address on their reply. Even more odd though, I got a second copy of patch 3 which does have the To field set. 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] Print hex values with '0x' prefix and octal with '0' in debug messages
On Mon, Sep 25, 2017 at 01:27:24PM +0200, Ján Tomko wrote: > On Mon, Sep 25, 2017 at 11:45:51AM +0100, Daniel P. Berrange wrote: > > Seeing a log message saying 'flags=93' is ambiguous & confusing unless > > you happen to know that libvirt always prints flags as hex. Change our > > debug messages so that they always add a '0x' prefix when printing flags, > > and '0' prefix when printing mode. A few other misc places gain a '0x' > > prefix in error messages too. > > > > Signed-off-by: Daniel P. Berrange > > --- > > ACK > > On top of these, the flagx= typo in virNodeAllocPages might deserve to > be included in this patch. Not sure about virAdmServerSetThreadPoolParameters > which is the only one using hex format for nparams: > > src/libvirt-admin.c=826=virAdmServerSetThreadPoolParameters(virAdmServerPtr > srv, > -- > src/libvirt-admin.c:831:VIR_DEBUG("srv=%p, params=%p, nparams=%x, > flags=0x%x", > src/libvirt-admin.c-832- srv, params, nparams, flags); Yeah, nparams should definitely be %d > -- > src/libvirt-domain.c=11901=virDomainSetGuestVcpus(virDomainPtr domain, > -- > src/libvirt-domain.c:11906:VIR_DOMAIN_DEBUG(domain, "cpumap='%s' state=%x > flags=0x%x", > src/libvirt-domain.c-11907- NULLSTR(cpumap), state, > flags); Heh, 'state' is just a boolean 0 or 1, so should relaly just be %d > -- > src/libvirt-host.c=1448=virNodeAllocPages(virConnectPtr conn, > -- > src/libvirt-host.c-1456-VIR_DEBUG("conn=%p npages=%u pageSizes=%p > pageCounts=%p " > src/libvirt-host.c:1457: "startCell=%d cellCount=%u flagx=%x", > src/libvirt-host.c-1458- conn, npages, pageSizes, pageCounts, > startCell, > src/libvirt-host.c-1459- cellCount, flags); Ok, I'll squash these 3 changes in. 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 2/2] qemu: block: Use correct alias when extracting disk node names
On Mon, Sep 25, 2017 at 11:53:07AM +0200, Peter Krempa wrote: > For some arcane reason we don't use the alias from disk->info.alias > directly but prepend drive in front of it. 'disk->info.alias' is used as the ID for the frontend device (eg virtio-blk) device. IDs must be globally unique in QEMU, so we can't use the same ID for the backend ('drive') object, hence we must invent a different ID, which we do by prepending 'drive'. 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 go-xml] add VCPUs support in Domain
On Mon, Sep 25, 2017 at 04:21:14PM +0800, zhenwei.pi wrote: > Signed-off-by: zhenwei.pi > --- > domain.go | 12 > domain_test.go | 24 > 2 files changed, 36 insertions(+) Reviewed-by: Daniel P. Berrange I'll push to git shortly. 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 libvirt-cim 1/2] maint: Rename autoconfiscate.sh to autogen.sh
On Fri, Sep 22, 2017 at 07:27:33PM +0200, Andrea Bolognani wrote: > On Fri, 2017-09-22 at 12:58 -0400, John Ferlan wrote: > > By killing it - I was thinking more along the lines of removing it from > > our CI infrastructure and just letting the repo sit silently without > > updates, but that ship sailed today. We could still remove it from the > > CI mix as if something breaks - there's not going to be anyone that > > really wants to fix it. > > Well, there hasn't been a single release since 2013, and any > actual development since 2014... > > Maybe it's really time to move on and free up some cycles for > the CI workers, especially since there are going to be more > jobs running on there pretty soon. Time consumed by the CI job is < 2% of total time required to do a full rebuild when libvirt sees a git change. Almost every other module takes a greater amount of time. This drops still further when you add in the time consumed by changes to non-libvirt core modules which don't trigger the CIM binding. Realisticaly the only thing that will have a measurable improvement on our CI system at this point is obtaining more hardware. 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 libvirt-cim 1/2] maint: Rename autoconfiscate.sh to autogen.sh
On Fri, Sep 22, 2017 at 10:08:44AM -0400, John Ferlan wrote: > > > On 09/22/2017 09:28 AM, Andrea Bolognani wrote: > > While the "autoconfiscate" name is very clever and cute, the > > de-facto standard name for this kind of script is "autogen", and > > deviating from it means having to special-case the libvirt-cim > > project when, for example, setting up a CI environment. > > > > Signed-off-by: Andrea Bolognani > > --- > > README | 2 +- > > autoconfiscate.sh => autogen.sh | 0 > > 2 files changed, 1 insertion(+), 1 deletion(-) > > rename autoconfiscate.sh => autogen.sh (100%) > > > > libvirt-cim has it's own mailing list (libvirt-...@redhat.com) and > achives: https://www.redhat.com/archives/libvirt-cim/index.html. > > The last email there (april 2015) and last patch (aug 2014) - I venture > to say perhaps the "doesn't anyone really care" starts to factor in. > Then of course, is there a way (need/desire) to kill it completely? IBM was the only company that ever cared about CIM for virtualization. Everyone else wanted an API that was easy to use instead ;-P Even IBM has lost interest now, so it is mostly a historical curiosity at this point. I wouldn't delete it, but also wouldn't spend alot of time in it. If someone does have a desire to update the code in any way, go at it and we can review it in the normal manner - no point waiting for the original maintainers to review IMHO. 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 v1 4/7] qemu: Allow regeneration of aliases
On Fri, Sep 22, 2017 at 10:25:46AM +0200, Peter Krempa wrote: > On Fri, Sep 22, 2017 at 10:18:02 +0200, Michal Privoznik wrote: > > On 09/21/2017 07:18 PM, Daniel P. Berrange wrote: > > > On Thu, Sep 21, 2017 at 06:05:19PM +0200, Peter Krempa wrote: > > >> On Thu, Sep 21, 2017 at 16:47:08 +0200, Michal Privoznik wrote: > > >>> In the near future the qemuAssignDeviceAliases() function is > > >>> going to be called multiple times: once at the domain define > > >>> time, then in domain prepare phase. To avoid regenerating the > > >>> same aliases the second time we need to be able to tell the > > >>> function which time it is being called. > > >>> > > >>> Signed-off-by: Michal Privoznik > > >>> --- > > >>> src/qemu/qemu_alias.c | 113 > > >>> ++-- > > >>> src/qemu/qemu_alias.h | 4 +- > > >>> src/qemu/qemu_driver.c | 2 +- > > >>> src/qemu/qemu_process.c | 2 +- > > >>> tests/qemuhotplugtest.c | 2 +- > > >>> 5 files changed, 115 insertions(+), 8 deletions(-) > > >>> > > > > >> So if you want to make aliases persistent, at least this will not work > > >> properly with device coldplug. > > > > Ah, good point. > > > > >> > > >> If you have two devices, detach the first one, then the second one moves > > >> to index 0 but will still have alias ending with 1. Then if you cold-add > > >> another device that will be put into index 1, and when starting the VM > > >> it will get assigned the same alias as the one which has the old one. > > >> > > >> This applies to all devices where the alias depends on the ordering. > > > > > > Yep, we would need to maintain a hash table remembering all currently > > > assigned aliases, and then increment the counter until we find a free > > > one for that dev type. > > > > Alternatively, every time we want to assign an alias for a device we > > traverse its siblings and see if it's taken. > > > > for (i = 0; ; i++) { > > alias = "device$i"; > > for (j = 0; j < def->ndevice; j++) { > > if (STREQ(def->device[j]->info.alias, alias)) > > continue; > > break > > } > > if (j != def->ndevice) { > > /* alias is free */ > > device->alias = alias; > > break; > > } > > } > > You can also generate them of a global sequence number. Qemu does not > care that they are not consecutive. It might trigger some OCD based eye > twitching, but it's way less yucky. Yeah a single global counter would be reasonable IMHO. Still has a little cpu time to initialize it when libvirtd starts up & loads persistent configs, but that's not unreasonable. 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 v1 4/7] qemu: Allow regeneration of aliases
On Thu, Sep 21, 2017 at 06:05:19PM +0200, Peter Krempa wrote: > On Thu, Sep 21, 2017 at 16:47:08 +0200, Michal Privoznik wrote: > > In the near future the qemuAssignDeviceAliases() function is > > going to be called multiple times: once at the domain define > > time, then in domain prepare phase. To avoid regenerating the > > same aliases the second time we need to be able to tell the > > function which time it is being called. > > > > Signed-off-by: Michal Privoznik > > --- > > src/qemu/qemu_alias.c | 113 > > ++-- > > src/qemu/qemu_alias.h | 4 +- > > src/qemu/qemu_driver.c | 2 +- > > src/qemu/qemu_process.c | 2 +- > > tests/qemuhotplugtest.c | 2 +- > > 5 files changed, 115 insertions(+), 8 deletions(-) > > > > diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c > > index e1431e2a2..00868c50f 100644 > > --- a/src/qemu/qemu_alias.c > > +++ b/src/qemu/qemu_alias.c > > @@ -481,25 +481,99 @@ qemuAssignDeviceShmemAlias(virDomainDefPtr def, > > } > > > > > > +/* > > + * qemuAssignDeviceAliases: > > + * @def: domain definition > > + * @qemuCaps: qemu capabilities > > + * @regenerate: forcibly regenerate aliases > > + * > > + * Assign aliases to domain devices. If @regenerate is false only > > + * the missing aliases are generated leaving already assigned > > + * aliases untouched. If @regenerate is true any preexisting > > + * aliases are overwritten. > > + * > > + * Returns 0 on success, -1 otherwise. > > + */ > > int > > -qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) > > +qemuAssignDeviceAliases(virDomainDefPtr def, > > +virQEMUCapsPtr qemuCaps, > > +bool regenerate) > > { > > size_t i; > > > > +if (regenerate) { > > +/* If we need to regenerate the aliases, we have to free > > + * them all upfront because there are some dependencies, > > + * e.g. an interface can be a hostdev and so on. > > + * Therefore, if we'd do the freeing in the loop below we > > + * might not start from zero. */ > > +for (i = 0; i < def->ndisks; i++) > > +VIR_FREE(def->disks[i]->info.alias); > > +for (i = 0; i < def->nnets; i++) > > +VIR_FREE(def->nets[i]->info.alias); > > +for (i = 0; i < def->nfss; i++) > > +VIR_FREE(def->fss[i]->info.alias); > > +for (i = 0; i < def->nsounds; i++) > > +VIR_FREE(def->sounds[i]->info.alias); > > +for (i = 0; i < def->nhostdevs; i++) > > +VIR_FREE(def->hostdevs[i]->info->alias); > > +for (i = 0; i < def->nredirdevs; i++) > > +VIR_FREE(def->redirdevs[i]->info.alias); > > +for (i = 0; i < def->nvideos; i++) > > +VIR_FREE(def->videos[i]->info.alias); > > +for (i = 0; i < def->ncontrollers; i++) > > +VIR_FREE(def->controllers[i]->info.alias); > > +for (i = 0; i < def->ninputs; i++) > > +VIR_FREE(def->inputs[i]->info.alias); > > +for (i = 0; i < def->nparallels; i++) > > +VIR_FREE(def->parallels[i]->info.alias); > > +for (i = 0; i < def->nserials; i++) > > +VIR_FREE(def->serials[i]->info.alias); > > +for (i = 0; i < def->nchannels; i++) > > +VIR_FREE(def->channels[i]->info.alias); > > +for (i = 0; i < def->nconsoles; i++) > > +VIR_FREE(def->consoles[i]->info.alias); > > +for (i = 0; i < def->nhubs; i++) > > +VIR_FREE(def->hubs[i]->info.alias); > > +for (i = 0; i < def->nshmems; i++) > > +VIR_FREE(def->shmems[i]->info.alias); > > +for (i = 0; i < def->nsmartcards; i++) > > +VIR_FREE(def->smartcards[i]->info.alias); > > +if (def->watchdog) > > +VIR_FREE(def->watchdog->info.alias); > > +if (def->memballoon) > > +VIR_FREE(def->memballoon->info.alias); > > +for (i = 0; i < def->nrngs; i++) > > +VIR_FREE(def->rngs[i]->info.alias); > > +if (def->tpm) > > +VIR_FREE(def->tpm->info.alias); > > +for (i = 0; i < def->nmems; i++) > > +VIR_FREE(def->mems[i]->info.alias); > > +} > > + > > + > > for (i = 0; i < def->ndisks; i++) { > > +if (def->disks[i]->info.alias && !regenerate) > > +continue; > > if (qemuAssignDeviceDiskAlias(def, def->disks[i], qemuCaps) < 0) > > return -1; > > } > > for (i = 0; i < def->nnets; i++) { > > +if (def->nets[i]->info.alias && !regenerate) > > +continue; > > if (qemuAssignDeviceNetAlias(def, def->nets[i], -1) < 0) > > return -1; > > } > > > > for (i = 0; i < def->nfss; i++) { > > +if (def->fss[i]->info.alias && !regenerate) > > +continue; > > if (qemuAssignDeviceFSAlias(def->fss[i], i) < 0) > > return -1; > > } > > f
[libvirt] [PATCH] Fix commandhelper build on win32
For win32 we need EXIT_AM_SKIP which is in testutils.h. We must define NO_LIBVIRT to prevent replacement of fprintf with virFilePrintf as we can't link to libvirt_util.la Signed-off-by: Daniel P. Berrange --- Pushed as a build fix tests/commandhelper.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index e9e6353f3..1da2834aa 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -28,6 +28,8 @@ #include #include "internal.h" +#define NO_LIBVIRT +#include "testutils.h" #ifndef WIN32 -- 2.13.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH libvirt-glib 0/5] Portability fixes
On Wed, Sep 20, 2017 at 07:54:45PM +0200, Andrea Bolognani wrote: > Currently, trying to build libvirt-glib on FreeBSD results in all > sorts of interesting failures. > > After applying this series, it's possible to build libvirt-glib > and run the test suite successfully. > > There's even a couple of clean-ups thrown in for good measure :) > > Andrea Bolognani (5): > maint: Drop autobuild.sh > configure: Bump required libvirt version to 1.2.5 > configure: Look for Perl interpreter path > tests: Don't rely on non-portable paths > scripts: Fix sha-bang lines For all 5 patches: Reviewed-by: Daniel P. Berrange 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
[libvirt] [PATCH] iohelper: avoid calling read() with misaligned buffers for O_DIRECT
The iohelper currently calls saferead() to get data from the underlying file. This has a problem with O_DIRECT when hitting end-of-file. saferead() is asked to read 1MB, but the first read() it does may return only a few KB, so it'll try another read() to fill the remaining buffer. Unfortunately the buffer pointer passed into this 2nd read() is likely not aligned to the extent that O_DIRECT requires, so rather than seeing '0' for end-of-file, we'll get -1 + EINVAL due to misaligned buffer. The way the iohelper is currently written, it already handles getting short reads, so there is actually no need to use saferead() at all. We can simply call read() directly. The benefit of this is that we can now write() the data immediately so when we go into the subsequent reads() we'll always have a correctly aligned buffer. Technically the file position ought to be aligned for O_DIRECT too, but this does not appear to matter when at end-of-file. Tested-by: Nikolay Shirokovskiy Signed-off-by: Daniel P. Berrange --- src/util/iohelper.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/util/iohelper.c b/src/util/iohelper.c index fe15a92e6..5416d4506 100644 --- a/src/util/iohelper.c +++ b/src/util/iohelper.c @@ -109,7 +109,9 @@ runIO(const char *path, int fd, int oflags) while (1) { ssize_t got; -if ((got = saferead(fdin, buf, buflen)) < 0) { +if ((got = read(fdin, buf, buflen)) < 0) { +if (errno == EINTR) +continue; virReportSystemError(errno, _("Unable to read %s"), fdinname); goto cleanup; } -- 2.13.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] iohelper: fix reading with O_DIRECT
On Wed, Sep 20, 2017 at 06:00:59PM +0300, Nikolay Shirokovskiy wrote: > > > On 20.09.2017 17:48, Daniel P. Berrange wrote: > > > > IOW can simplify your patch to just this I believe:> > > diff --git a/src/util/iohelper.c b/src/util/iohelper.c > > index fe15a92e6..5416d4506 100644 > > --- a/src/util/iohelper.c > > +++ b/src/util/iohelper.c > > @@ -109,7 +109,9 @@ runIO(const char *path, int fd, int oflags) > > while (1) { > > ssize_t got; > > > > -if ((got = saferead(fdin, buf, buflen)) < 0) { > > +if ((got = read(fdin, buf, buflen)) < 0) { > > +if (errno == EINTR) > > +continue; > > virReportSystemError(errno, _("Unable to read %s"), fdinname); > > goto cleanup; > > } > > But we still read half the buffer at the file end and then try misaligned read > after that. No, we shouldn't be mis-aligned. The 'buf' variable we use is aligned correctly. If we read half a buffer, we'll then write that data back to libvirtd. So when we go back into read(), we'll be reading into the start of 'buf' again, so we're still correctly aligned. This is the key difference against saferead() - that tries to fill up the buffer before we can write any data, whereas with this patch we'll always write whatever we read immediately 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] iohelper: fix reading with O_DIRECT
On Wed, Sep 20, 2017 at 04:45:35PM +0300, Nikolay Shirokovskiy wrote: > > > On 20.09.2017 16:41, Daniel P. Berrange wrote: > > On Wed, Sep 20, 2017 at 04:35:58PM +0300, Nikolay Shirokovskiy wrote: > >> > >> > >> On 20.09.2017 16:30, Daniel P. Berrange wrote: > >>> On Wed, Sep 20, 2017 at 02:58:55PM +0300, Nikolay Shirokovskiy wrote: > >>>> saferead is not suitable for direct reads. If file size is not multiple > >>>> of align size then we get EINVAL on the read(2) that is supposed to > >>>> return 0 because read buffer will not be aligned at this point. > >>>> > >>>> Let's not read again after partial read and check that we read > >>>> everything by comparing the number of total bytes read against file size. > >>> > >>> What scenario did you actually hit this problem in ?IIUC, we should > >>> only be using O_DIRECT against block devices or plain files, and in both > >>> those cases we should never see short-read unless we hit EOF. > >> > >> Yes. But saferead is generic function and rereads file after short read. > >> Here we got EINVAL because of misalignement. > > > > I understand that, but how have you actually hit this in the real world. > > AFAICT, the short-read and subsequent problem with misalignemt should be > > impossible to hit, because any files we use O_DIRECT on should not ever > > return a shortread. > > virsh restore --bypass-cache just fails (at least on my kernel). The problem > is that if the state file size is not multiple of buffer then last read > will definetly be short read. Ah ok I see what's happening - its a short read in the sense that we have hit end-of-file, not a short read in the middle of the file. 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] iohelper: fix reading with O_DIRECT
On Wed, Sep 20, 2017 at 02:58:55PM +0300, Nikolay Shirokovskiy wrote: > saferead is not suitable for direct reads. If file size is not multiple > of align size then we get EINVAL on the read(2) that is supposed to > return 0 because read buffer will not be aligned at this point. > > Let's not read again after partial read and check that we read > everything by comparing the number of total bytes read against file size. > --- > > Diff from v1: > - a couple of cosmetic changes > > src/util/iohelper.c | 42 +- > 1 file changed, 37 insertions(+), 5 deletions(-) > > diff --git a/src/util/iohelper.c b/src/util/iohelper.c > index fe15a92..9aa6ae0 100644 > --- a/src/util/iohelper.c > +++ b/src/util/iohelper.c > @@ -56,6 +56,7 @@ runIO(const char *path, int fd, int oflags) > unsigned long long total = 0; > bool direct = O_DIRECT && ((oflags & O_DIRECT) != 0); > off_t end = 0; > +off_t cur; > > #if HAVE_POSIX_MEMALIGN > if (posix_memalign(&base, alignMask + 1, buflen)) { > @@ -78,10 +79,22 @@ runIO(const char *path, int fd, int oflags) > fdoutname = "stdout"; > /* To make the implementation simpler, we give up on any > * attempt to use O_DIRECT in a non-trivial manner. */ > -if (direct && ((end = lseek(fd, 0, SEEK_CUR)) != 0)) { > -virReportSystemError(end < 0 ? errno : EINVAL, "%s", > - _("O_DIRECT read needs entire seekable > file")); > -goto cleanup; > +if (direct) { > +if ((cur = lseek(fd, 0, SEEK_CUR)) != 0) { > +virReportSystemError(cur < 0 ? errno : EINVAL, "%s", > + _("O_DIRECT read needs entire seekable > file")); > +goto cleanup; > +} > + > +if ((end = lseek(fd, 0, SEEK_END)) < 0) { > +virReportSystemError(errno, "%s", _("can not seek file > end")); > +goto cleanup; > +} > + > +if (lseek(fd, cur, SEEK_SET) < 0) { > +virReportSystemError(errno, "%s", _("can not seek file > begin")); > +goto cleanup; > +} > } > break; > case O_WRONLY: > @@ -109,7 +122,26 @@ runIO(const char *path, int fd, int oflags) > while (1) { > ssize_t got; > > -if ((got = saferead(fdin, buf, buflen)) < 0) { > +/* in case of O_DIRECT we cannot read again to check for EOF > + * after partial buffer read as it is done in saferead */ > +if (direct && fdin == fd && end - total < buflen) { > +if (total == end) > +break; > + > +while ((got = read(fd, buf, buflen)) < 0 && errno == EINTR) > +; > + > +if (got < end - total) { > +virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Unable to read last chunk from %s"), > + fdinname); > +goto cleanup; > +} > +} else { > +got = saferead(fdin, buf, buflen); > +} > + > +if (got < 0) { > virReportSystemError(errno, _("Unable to read %s"), fdinname); > goto cleanup; > } So the ultimate problem here is that when saferead() hits EOF, it tries to read some more, expecting to see ret == 0, but the buffer that saferead is passing into read() is not suitably aligned, so we get an error report despite not having any data to read. What's fun is that the runIO method already handles the fact that saferead() may return less data than was asked for, so using the saferead() function is pretty much pointless. There is thus no need to have separate codepaths for O_DIRECT vs normal, if we change iohelper to just use read() unconditionally and let it handle all return values with its existing code. IOW can simplify your patch to just this I believe: diff --git a/src/util/iohelper.c b/src/util/iohelper.c index fe15a92e6..5416d4506 100644 --- a/src/util/iohelper.c +++ b/src/util/iohelper.c @@ -109,7 +109,9 @@ runIO(const char *path, int fd, int oflags) while (1) { ssize_t got; -if ((got = saferead(fdin, buf, buflen)) < 0) { +if ((got = read(fdin, buf, buflen)) < 0) { +if (errno == EINTR) +continue; virReportSystemError(errno, _("Unable to read %s"), fdinname); goto cleanup; } 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] iohelper: fix reading with O_DIRECT
On Wed, Sep 20, 2017 at 04:35:58PM +0300, Nikolay Shirokovskiy wrote: > > > On 20.09.2017 16:30, Daniel P. Berrange wrote: > > On Wed, Sep 20, 2017 at 02:58:55PM +0300, Nikolay Shirokovskiy wrote: > >> saferead is not suitable for direct reads. If file size is not multiple > >> of align size then we get EINVAL on the read(2) that is supposed to > >> return 0 because read buffer will not be aligned at this point. > >> > >> Let's not read again after partial read and check that we read > >> everything by comparing the number of total bytes read against file size. > > > > What scenario did you actually hit this problem in ?IIUC, we should > > only be using O_DIRECT against block devices or plain files, and in both > > those cases we should never see short-read unless we hit EOF. > > Yes. But saferead is generic function and rereads file after short read. > Here we got EINVAL because of misalignement. I understand that, but how have you actually hit this in the real world. AFAICT, the short-read and subsequent problem with misalignemt should be impossible to hit, because any files we use O_DIRECT on should not ever return a shortread. 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] iohelper: fix reading with O_DIRECT
On Wed, Sep 20, 2017 at 02:58:55PM +0300, Nikolay Shirokovskiy wrote: > saferead is not suitable for direct reads. If file size is not multiple > of align size then we get EINVAL on the read(2) that is supposed to > return 0 because read buffer will not be aligned at this point. > > Let's not read again after partial read and check that we read > everything by comparing the number of total bytes read against file size. What scenario did you actually hit this problem in ?IIUC, we should only be using O_DIRECT against block devices or plain files, and in both those cases we should never see short-read unless we hit EOF. 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] Stop linking tests/commandhelper to libvirt code
On Wed, Sep 20, 2017 at 12:08:59PM +0100, Daniel P. Berrange wrote: > The commandhelper binary is a helper for commandtest that > validates what file handles were inherited. For this to > work reliably we must not have any libraries that leak > file descriptors into commandhelper. Unfortunately some > versions of gnutls will intentionally open file handles > at library load time via a constructor function. > > We previously hacked around this in > > commit 4cbc15d037e1cd8abf5c4aa6acc30d83ae13e34d > Author: Martin Kletzander > Date: Fri May 2 09:55:52 2014 +0200 > > tests: don't fail with newer gnutls > > gnutls-3.3.0 and newer leaves 2 FDs open in order to be backwards > compatible when it comes to chrooted binaries [1]. Linking > commandhelper with gnutls then leaves these two FDs open and > commandtest fails thanks to that. This patch does not link > commandhelper with libvirt.la, but rather only the utilities making > the test pass. > > Based on suggestion from Daniel [2]. > > [1] http://lists.gnutls.org/pipermail/gnutls-help/2014-April/003429.html > [2] https://www.redhat.com/archives/libvir-list/2014-April/msg01119.html > > That fix relied on fact that while libvirt.so linked with > gnutls, libvirt_util.la did not link to it. With the > introduction of the util/vircrypto.c file that assumption > is no longer valid. We must not link to libvirt_util.la > at all - only gnulib and libc can (hopefully) be relied > on not to open random file descriptors in constructors. > > Signed-off-by: Daniel P. Berrange > --- BTW, technically a build breaker fix for the CI problems that are fallout from my previous CI build breaker fix. Leaving this for review before pushing since it is a somewhat larger fix. > cfg.mk| 8 > tests/Makefile.am | 6 -- > tests/commandhelper.c | 29 + > 3 files changed, 21 insertions(+), 22 deletions(-) > > diff --git a/cfg.mk b/cfg.mk > index 56cb14b..f2a053f 100644 > --- a/cfg.mk > +++ b/cfg.mk > @@ -,7 +,7 @@ $(srcdir)/src/admin/admin_client.h: > $(srcdir)/src/admin/admin_protocol.x > exclude_file_name_regexp--sc_avoid_strcase = ^tools/vsh\.h$$ > > > _src1=libvirt-stream|qemu/qemu_monitor|util/vir(command|file|fdstream)|xen/xend_internal|rpc/virnetsocket|lxc/lxc_controller|locking/lock_daemon|logging/log_daemon > -_test1=shunloadtest|virnettlscontexttest|virnettlssessiontest|vircgroupmock > +_test1=shunloadtest|virnettlscontexttest|virnettlssessiontest|vircgroupmock|commandhelper > exclude_file_name_regexp--sc_avoid_write = \ > > ^(src/($(_src1))|daemon/libvirtd|tools/virsh-console|tests/($(_test1)))\.c$$ > > @@ -1146,10 +1146,10 @@ exclude_file_name_regexp--sc_prohibit_asprintf = \ > > ^(cfg\.mk|bootstrap.conf$$|examples/|src/util/virstring\.[ch]$$|tests/vircgroupmock\.c$$) > > exclude_file_name_regexp--sc_prohibit_strdup = \ > - > ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c$$) > + > ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c|tests/commandhelper\.c$$) > > exclude_file_name_regexp--sc_prohibit_close = \ > - > (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir.+mock\.c)$$) > + > (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir.+mock\.c|tests/commandhelper\.c)$$) > > exclude_file_name_regexp--sc_prohibit_empty_lines_at_EOF = \ > > (^tests/(qemuhelp|virhostcpu|virpcitest)data/|docs/js/.*\.js|docs/fonts/.*\.woff|\.diff|tests/virconfdata/no-newline\.conf$$) > @@ -1173,7 +1173,7 @@ exclude_file_name_regexp--sc_prohibit_select = \ > ^cfg\.mk$$ > > exclude_file_name_regexp--sc_prohibit_raw_allocation = \ > - > ^(docs/hacking\.html\.in|src/util/viralloc\.[ch]|examples/.*|tests/(securityselinuxhelper|(vircgroup|nss)mock)\.c|tools/wireshark/src/packet-libvirt\.c)$$ > + > ^(docs/hacking\.html\.in|src/util/viralloc\.[ch]|examples/.*|tests/(securityselinuxhelper|(vircgroup|nss)mock|commandhelper)\.c|tools/wireshark/src/packet-libvirt\.c)$$ > > exclude_file_name_regexp--sc_prohibit_readlink = \ >^src/(util/virutil|lxc/lxc_container)\.c$$ > diff --git a/tests/Makefile.am b/tests/Makefile.am > index 8138885..0b2305d 100644 > --- a/tests/Makefile.am > +++ b/tests/Makefile.am > @@ -946,12 +946,14 @@ commandtest_SOURCES = \ > commandtest.c testutils.h testutils.c > commandtest_LDADD = $(LDADDS) > > +# Must not link to any libvirt modules - libc / gnulib only > +# otherwise external libraries might unexpectedly leak > +# file descriptors into commandhelper invalidating t
[libvirt] [PATCH] Stop linking tests/commandhelper to libvirt code
The commandhelper binary is a helper for commandtest that validates what file handles were inherited. For this to work reliably we must not have any libraries that leak file descriptors into commandhelper. Unfortunately some versions of gnutls will intentionally open file handles at library load time via a constructor function. We previously hacked around this in commit 4cbc15d037e1cd8abf5c4aa6acc30d83ae13e34d Author: Martin Kletzander Date: Fri May 2 09:55:52 2014 +0200 tests: don't fail with newer gnutls gnutls-3.3.0 and newer leaves 2 FDs open in order to be backwards compatible when it comes to chrooted binaries [1]. Linking commandhelper with gnutls then leaves these two FDs open and commandtest fails thanks to that. This patch does not link commandhelper with libvirt.la, but rather only the utilities making the test pass. Based on suggestion from Daniel [2]. [1] http://lists.gnutls.org/pipermail/gnutls-help/2014-April/003429.html [2] https://www.redhat.com/archives/libvir-list/2014-April/msg01119.html That fix relied on fact that while libvirt.so linked with gnutls, libvirt_util.la did not link to it. With the introduction of the util/vircrypto.c file that assumption is no longer valid. We must not link to libvirt_util.la at all - only gnulib and libc can (hopefully) be relied on not to open random file descriptors in constructors. Signed-off-by: Daniel P. Berrange --- cfg.mk| 8 tests/Makefile.am | 6 -- tests/commandhelper.c | 29 + 3 files changed, 21 insertions(+), 22 deletions(-) diff --git a/cfg.mk b/cfg.mk index 56cb14b..f2a053f 100644 --- a/cfg.mk +++ b/cfg.mk @@ -,7 +,7 @@ $(srcdir)/src/admin/admin_client.h: $(srcdir)/src/admin/admin_protocol.x exclude_file_name_regexp--sc_avoid_strcase = ^tools/vsh\.h$$ _src1=libvirt-stream|qemu/qemu_monitor|util/vir(command|file|fdstream)|xen/xend_internal|rpc/virnetsocket|lxc/lxc_controller|locking/lock_daemon|logging/log_daemon -_test1=shunloadtest|virnettlscontexttest|virnettlssessiontest|vircgroupmock +_test1=shunloadtest|virnettlscontexttest|virnettlssessiontest|vircgroupmock|commandhelper exclude_file_name_regexp--sc_avoid_write = \ ^(src/($(_src1))|daemon/libvirtd|tools/virsh-console|tests/($(_test1)))\.c$$ @@ -1146,10 +1146,10 @@ exclude_file_name_regexp--sc_prohibit_asprintf = \ ^(cfg\.mk|bootstrap.conf$$|examples/|src/util/virstring\.[ch]$$|tests/vircgroupmock\.c$$) exclude_file_name_regexp--sc_prohibit_strdup = \ - ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c$$) + ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c|tests/commandhelper\.c$$) exclude_file_name_regexp--sc_prohibit_close = \ - (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir.+mock\.c)$$) + (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir.+mock\.c|tests/commandhelper\.c)$$) exclude_file_name_regexp--sc_prohibit_empty_lines_at_EOF = \ (^tests/(qemuhelp|virhostcpu|virpcitest)data/|docs/js/.*\.js|docs/fonts/.*\.woff|\.diff|tests/virconfdata/no-newline\.conf$$) @@ -1173,7 +1173,7 @@ exclude_file_name_regexp--sc_prohibit_select = \ ^cfg\.mk$$ exclude_file_name_regexp--sc_prohibit_raw_allocation = \ - ^(docs/hacking\.html\.in|src/util/viralloc\.[ch]|examples/.*|tests/(securityselinuxhelper|(vircgroup|nss)mock)\.c|tools/wireshark/src/packet-libvirt\.c)$$ + ^(docs/hacking\.html\.in|src/util/viralloc\.[ch]|examples/.*|tests/(securityselinuxhelper|(vircgroup|nss)mock|commandhelper)\.c|tools/wireshark/src/packet-libvirt\.c)$$ exclude_file_name_regexp--sc_prohibit_readlink = \ ^src/(util/virutil|lxc/lxc_container)\.c$$ diff --git a/tests/Makefile.am b/tests/Makefile.am index 8138885..0b2305d 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -946,12 +946,14 @@ commandtest_SOURCES = \ commandtest.c testutils.h testutils.c commandtest_LDADD = $(LDADDS) +# Must not link to any libvirt modules - libc / gnulib only +# otherwise external libraries might unexpectedly leak +# file descriptors into commandhelper invalidating the +# test logic assumptions commandhelper_SOURCES = \ commandhelper.c commandhelper_LDADD = \ $(NO_INDIRECT_LDFLAGS) \ - $(PROBES_O) \ - ../src/libvirt_util.la \ $(GNULIB_LIBS) commandhelper_LDFLAGS = -static diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 015efda..e9e6353 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -28,11 +28,6 @@ #include #include "internal.h" -#include "virutil.h" -#include "viralloc.h" -#include "virfile.h" -#include "testutils.h" -#include "virstring.h" #ifndef WIN32 @@ -50,11 +45,13 @@ static int envsort(const void *a, const void *b) char *bkey; int
Re: [libvirt] [PATCH] Link libvirt_util.la with gnutls
On Wed, Sep 20, 2017 at 11:35:14AM +0200, Martin Kletzander wrote: > On Wed, Sep 20, 2017 at 09:41:24AM +0100, Daniel P. Berrange wrote: > > The util/vircrypto.c file uses gnutls, so we must directly link > > libvirt_util.la with gnutls to avoid errors on OS which do not > > resolve symbols against indirectly linked libraries. > > > > This fixes a build failure on Ubuntu Trusty > > > > CCLD storagevolxml2argvtest > > /usr/bin/ld: ../src/.libs/libvirt_util.a(libvirt_util_la-vircrypto.o): > > undefined reference to symbol 'gnutls_strerror@@GNUTLS_1_4' > > > > //usr/lib/x86_64-linux-gnu/libgnutls.so.26: error adding symbols: DSO > > missing from command line > > > > Thanks for fixing that. Do you know how this can be changed? Is it > some setting for glibc or is it the way it is compiled? It is behaviour of the "ld" linker https://wiki.debian.org/ToolChain/DSOLinking Debian/Ubuntu have --no-add-needed/--no-copy-dt-needed-entries enabled by default in their linker builds, which IIUC, differs from what is the default on Fedora/RHEL. IIUC, it is the --no-copy-dt-needed-entries that caused this particular build failure. > > > Signed-off-by: Daniel P. Berrange > > --- > > > > Pushed as a build breaker fix > > > > src/Makefile.am | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/Makefile.am b/src/Makefile.am > > index c3c7a8f04..173fba1e6 100644 > > --- a/src/Makefile.am > > +++ b/src/Makefile.am > > @@ -1224,7 +1224,7 @@ libvirt_util_la_LIBADD = $(CAPNG_LIBS) $(YAJL_LIBS) > > $(LIBNL_LIBS) \ > > $(THREAD_LIBS) $(AUDIT_LIBS) $(DEVMAPPER_LIBS) \ > > $(LIB_CLOCK_GETTIME) $(DBUS_LIBS) $(WIN32_EXTRA_LIBS) > > $(LIBXML_LIBS) \ > > $(SECDRIVER_LIBS) $(NUMACTL_LIBS) $(ACL_LIBS) \ > > - $(POLKIT_LIBS) > > + $(POLKIT_LIBS) $(GNUTLS_LIBS) 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
[libvirt] [PATCH] Link libvirt_util.la with gnutls
The util/vircrypto.c file uses gnutls, so we must directly link libvirt_util.la with gnutls to avoid errors on OS which do not resolve symbols against indirectly linked libraries. This fixes a build failure on Ubuntu Trusty CCLD storagevolxml2argvtest /usr/bin/ld: ../src/.libs/libvirt_util.a(libvirt_util_la-vircrypto.o): undefined reference to symbol 'gnutls_strerror@@GNUTLS_1_4' //usr/lib/x86_64-linux-gnu/libgnutls.so.26: error adding symbols: DSO missing from command line Signed-off-by: Daniel P. Berrange --- Pushed as a build breaker fix src/Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Makefile.am b/src/Makefile.am index c3c7a8f04..173fba1e6 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1224,7 +1224,7 @@ libvirt_util_la_LIBADD = $(CAPNG_LIBS) $(YAJL_LIBS) $(LIBNL_LIBS) \ $(THREAD_LIBS) $(AUDIT_LIBS) $(DEVMAPPER_LIBS) \ $(LIB_CLOCK_GETTIME) $(DBUS_LIBS) $(WIN32_EXTRA_LIBS) $(LIBXML_LIBS) \ $(SECDRIVER_LIBS) $(NUMACTL_LIBS) $(ACL_LIBS) \ - $(POLKIT_LIBS) + $(POLKIT_LIBS) $(GNUTLS_LIBS) noinst_LTLIBRARIES += libvirt_conf.la -- 2.13.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Revert "travis: Limit git depth to 5 commits"
On Tue, Sep 19, 2017 at 03:49:19PM +0200, Andrea Bolognani wrote: > Turns out a build job can be stuck waiting for a macOS worker to > become available for a pretty long time: if more than 5 commits > have been pushed in the meantime, the clone will be too shallow > for the worker to find the commit it's supposed to verify, and > the build job will fail. It could even hit us more generally - travis throttles how many parallel jobs can be run. So if two (or more) people push patch series with more than 5 patches a few minutes apart, we might not have finished all queued jobs. > > See https://travis-ci.org/libvirt/libvirt/jobs/277244110 for an > example of the failure described. > > This reverts commit 2e975abdc9bbc9e965486e8486cc17a771cdaeb3. > > Signed-off-by: Andrea Bolognani > --- > .travis.yml | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/.travis.yml b/.travis.yml > index b3e73bcf7..480419dfd 100644 > --- a/.travis.yml > +++ b/.travis.yml > @@ -75,7 +75,6 @@ addons: >- zfs-fuse > > git: > - depth: 5 >submodules: true > > env: Reviewed-by: Daniel P. Berrange 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
[libvirt] [PATCH python v2] Add travis build config
Enable builds on several python versions, and against several versions of libvirt. Ideally we would build all the way back to 0.9.11, since that is the min supported libvirt for python binding. It is not possible to build this old libvirt version on modern distros though, so using 1.2.0 as the oldest for now. Signed-off-by: Daniel P. Berrange --- .travis.yml | 55 +++ requirements-test.txt | 2 ++ setup.py | 15 +++--- 3 files changed, 65 insertions(+), 7 deletions(-) create mode 100644 .travis.yml create mode 100644 requirements-test.txt diff --git a/.travis.yml b/.travis.yml new file mode 100644 index 000..f608ca1 --- /dev/null +++ b/.travis.yml @@ -0,0 +1,55 @@ +language: python +os: linux + +python: + - 2.6 + - 2.7 + - 3.2 + - 3.6 + +env: + - LIBVIRT=1.2.0 EXT=gz + - LIBVIRT=2.0.0 EXT=xz + - LIBVIRT=3.6.0 EXT=xz + +install: + - sudo apt-get -qqy build-dep libvirt libxml2-dev + - sudo apt-get -qqy install curl + - pip install -r requirements-test.txt + - curl -O -s https://libvirt.org/sources/libvirt-${LIBVIRT}.tar.${EXT} + - tar -xf libvirt-${LIBVIRT}.tar.${EXT} + - pushd libvirt-${LIBVIRT} + - | +./configure --prefix=`pwd`/../libvirt-vroot \ +--without-libvirtd \ +--without-esx \ +--without-vbox \ +--without-libxl \ +--without-xen \ +--without-qemu \ +--without-lxc \ +--without-hyperv \ +--without-macvtap \ +--disable-werror + - make + - make install + - popd + +script: + - LD_LIBRARY_PATH=`pwd`/libvirt-vroot/lib PKG_CONFIG_PATH=`pwd`/libvirt-vroot/lib/pkgconfig python setup.py build sdist test + +notifications: + irc: +# The channel name "irc.oftc.net#virt" is encrypted against libvirt/libvirt-python +# to prevent IRC notifications from github forks. This was created using: +# $ travis encrypt -r "libvirt/libvirt-python" "irc.oftc.net#virt" +channels: + - secure: "K4JrbRpz4CHtZ1vjthVwseT8K6INJgjtZethP4DN1jOpm1uC5esbe1Q1qJOfB92JbMcdM6DNjrVg5eyTJj35aD9UoGpTUcPMsYrhlTPHZtfAuLv/at2eB2XRmETlhiXHgI6LizX6gTiwGW5ZHYwGChzumWxu141d/L9harNh9R6z8XH9uJpkNdOAIsJcwS56XGZ74CKsrqF5dK6ZYPIyP+i7gPO67gEWo0oD6TiJKR908fw03ZiXarIFmLRlk4MbHywLRF0byfD0gg2Ht/tDX73+59QXjLKo/GvQecwoU8UuuFRJlyhUfvm1JYYydnS+O7fPJvI0FWlYFY7i76aeVqkARHRpHknFueT6kZADOmiyMLuvdr+gWVuyIdX33vVJtDm4T1OtNMG/wy9EUZUU1vEu+gHhaRkf/O0GkMj0Hac4I14BGyd/Wdhto6zWojFiMEG/HRHey6l15MBQu49QyW/YMyWi/LeBWXuCUgwQ/ij5EPgsn36OxCafV9zMz0oXZskwX6rJGQRZsdgdwYvt2hP3muLaJbwVyT0bGlOJDJieOa/LVKOXPcQm26aGfyMuLgm0//E9v++6W1IDKh6+BNsfTKAwTxlAvJyz6Bns3XuUJUxUz2+uQVSS6S3EwEZUJ+yHDd2F4sX5OP1L7TWIOWFbI4vQK90ZZ7/jgiYQbwo=" +on_success: change +on_failure: always + email: +# The list name 'libvirt...@redhat.com" is encrypted against libvirt/libvirt-python +# to prevent IRC notifications from github forks. This was created using: +# $ travis encrypt -r "libvirt/libvirt-python" "libvirt...@redhat.com" +recipients: + - secure: "l6TTLcEcXdDEldHE2NgSIdt6a0k99ug3hp2W4IlnqJWJfIk/87nysJtLNrA0va20pPApCa3iJfMq4PUmBGiIIimTN0/KgC7tONDraogXhCbgfZp9Ejy/57TXxygSp4oum2kDw/c5uLnfrFV/xcn1fk6hvH6CD3bVcJPOQ/mc5FSKLqN5UzwqNnMpMTtG9qxCwfXJ/Bdm9fbURfezC7djcYRwRfPUe3TSD0L76G2HnQnSy4RqR3KFSjQHFPnSGM5IbsokbOaFKCyp/pHOt7QomQaY7YAPX/K9O+eP+hkkp6DGADkkumHctcgnMoyxpahf7pNKw9S8JYabH2NwREIq8whbp9Mo+R4rYO2ozroLWHaboYs/pBLrs606ivTwOmWGRCpJdCmmKTiZNyo6MRrwiOM6x+2YHUTMOa2kVheRNzaaxMFzHPW2kZ20bujPhfViJsRYj9flo5GJXJLyjluGZK5RjrguNJeIh8VJNBiSHW37uj7drmNBsqMad+65mf/4xtGITBqhz5Spx5R9UMZbuiJvcm8GasJMMdQ+bCfuWYjF2nZvSvFEr54Ii1YrDp6FKQ8YG1aD1/D8Z0/b3pLd/8Pn+M9yIWyO/Sto5TbSUjxBTmTStuDmtYE5uu1miYebvgJH5MovWPBegYgrfI417kPJgCG3q/R0YcZFMKFfQyo=" diff --git a/requirements-test.txt b/requirements-test.txt new file mode 100644 index 000..7435c54 --- /dev/null +++ b/requirements-test.txt @@ -0,0 +1,2 @@ +nose +lxml diff --git a/setup.py b/setup.py index f33ff1a..e8c498c 100755 --- a/setup.py +++ b/setup.py @@ -290,15 +290,16 @@ class my_test(Command): 'lib' + plat_specifier) def find_nosetests_path(self): -paths = [ -"/usr/bin/nosetests-%d.%d" % (sys.version_info[0], - sys.version_info[1]), -"/usr/bin/nosetests-%d" % (sys.version_info[0]), -"/usr/bin/nosetests", +binaries = [ +"nosetests-%d.%d" % (sys.version_info[0], + sys.version_info[1]), +"nosetests-%d" % (sys.version_info[0]), +"nosetests", ] -for path in paths: -if os.path.exists(path): +for binary in binaries:
Re: [libvirt] [PATCH python] Add travis build config
On Tue, Sep 19, 2017 at 03:06:58PM +0200, Martin Kletzander wrote: > On Tue, Sep 19, 2017 at 12:56:55PM +0100, Daniel P. Berrange wrote: > > Enable builds on several python versions, and against several versions > > of libvirt. Ideally we would build all the way back to 0.9.11, since > > that is the min supported libvirt for python binding. It is not possible > > to build this old libvirt version on modern distros though, so using > > 1.2.0 as the oldest for now. > > > > Signed-off-by: Daniel P. Berrange > > --- > > .travis.yml | 55 +++ > > setup.py| 13 +++-- > > 2 files changed, 62 insertions(+), 6 deletions(-) > > create mode 100644 .travis.yml > > > > diff --git a/.travis.yml b/.travis.yml > > new file mode 100644 > > index 000..203d91d > > --- /dev/null > > +++ b/.travis.yml > > @@ -0,0 +1,55 @@ > > +language: python > > +os: linux > > + > > +python: > > + - 2.6 > > + - 2.7 > > + - 3.2 > > + - 3.6 > > + > > +env: > > + - LIBVIRT=1.2.0 EXT=gz > > + - LIBVIRT=2.0.0 EXT=xz > > + - LIBVIRT=3.6.0 EXT=xz > > Spacing should be same on all lines. > > > + > > +install: > > + - sudo apt-get -qqy build-dep libvirt libxml2-dev > > + - sudo apt-get -qqy install curl > > + - pip install lxml nose > > It'd be nice to have pip install -r requirements.txt here (and that file > added, of course). > > > + - curl -O -s https://libvirt.org/sources/libvirt-${LIBVIRT}.tar.${EXT} > > + - tar -xf libvirt-${LIBVIRT}.tar.${EXT} > > + - pushd libvirt-${LIBVIRT} > > + - | > > +./configure --prefix=`pwd`/../libvirt-vroot \ > > +--without-libvirtd \ > > +--without-esx \ > > +--without-vbox \ > > +--without-libxl \ > > +--without-xen \ > > +--without-qemu \ > > Will this prevent building the qemu-specific APIs? I'm too lazy to > check it out and I think there is no harm in seeing that after this is > pushed, though =) No, we don't ever disable the public APIs > Both this and the one above should be regenerated, of course. Unless > they were, I don't know how to check those. The encrypted data was correct - only the comments were wrong 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
[libvirt] [PATCH python] Add travis build config
Enable builds on several python versions, and against several versions of libvirt. Ideally we would build all the way back to 0.9.11, since that is the min supported libvirt for python binding. It is not possible to build this old libvirt version on modern distros though, so using 1.2.0 as the oldest for now. Signed-off-by: Daniel P. Berrange --- .travis.yml | 55 +++ setup.py| 13 +++-- 2 files changed, 62 insertions(+), 6 deletions(-) create mode 100644 .travis.yml diff --git a/.travis.yml b/.travis.yml new file mode 100644 index 000..203d91d --- /dev/null +++ b/.travis.yml @@ -0,0 +1,55 @@ +language: python +os: linux + +python: + - 2.6 + - 2.7 + - 3.2 + - 3.6 + +env: + - LIBVIRT=1.2.0 EXT=gz + - LIBVIRT=2.0.0 EXT=xz + - LIBVIRT=3.6.0 EXT=xz + +install: + - sudo apt-get -qqy build-dep libvirt libxml2-dev + - sudo apt-get -qqy install curl + - pip install lxml nose + - curl -O -s https://libvirt.org/sources/libvirt-${LIBVIRT}.tar.${EXT} + - tar -xf libvirt-${LIBVIRT}.tar.${EXT} + - pushd libvirt-${LIBVIRT} + - | +./configure --prefix=`pwd`/../libvirt-vroot \ +--without-libvirtd \ +--without-esx \ +--without-vbox \ +--without-libxl \ +--without-xen \ +--without-qemu \ +--without-lxc \ +--without-hyperv \ +--without-macvtap \ +--disable-werror + - make + - make install + - popd + +script: + - LD_LIBRARY_PATH=`pwd`/libvirt-vroot/lib PKG_CONFIG_PATH=`pwd`/libvirt-vroot/lib/pkgconfig python setup.py build sdist test + +notifications: + irc: +# The channel name "irc.oftc.net#virt" is encrypted against libvirt/libvirt +# to prevent IRC notifications from github forks. This was created using: +# $ travis encrypt -r "libvirt/libvirt" "irc.oftc.net#virt" +channels: + - secure: "K4JrbRpz4CHtZ1vjthVwseT8K6INJgjtZethP4DN1jOpm1uC5esbe1Q1qJOfB92JbMcdM6DNjrVg5eyTJj35aD9UoGpTUcPMsYrhlTPHZtfAuLv/at2eB2XRmETlhiXHgI6LizX6gTiwGW5ZHYwGChzumWxu141d/L9harNh9R6z8XH9uJpkNdOAIsJcwS56XGZ74CKsrqF5dK6ZYPIyP+i7gPO67gEWo0oD6TiJKR908fw03ZiXarIFmLRlk4MbHywLRF0byfD0gg2Ht/tDX73+59QXjLKo/GvQecwoU8UuuFRJlyhUfvm1JYYydnS+O7fPJvI0FWlYFY7i76aeVqkARHRpHknFueT6kZADOmiyMLuvdr+gWVuyIdX33vVJtDm4T1OtNMG/wy9EUZUU1vEu+gHhaRkf/O0GkMj0Hac4I14BGyd/Wdhto6zWojFiMEG/HRHey6l15MBQu49QyW/YMyWi/LeBWXuCUgwQ/ij5EPgsn36OxCafV9zMz0oXZskwX6rJGQRZsdgdwYvt2hP3muLaJbwVyT0bGlOJDJieOa/LVKOXPcQm26aGfyMuLgm0//E9v++6W1IDKh6+BNsfTKAwTxlAvJyz6Bns3XuUJUxUz2+uQVSS6S3EwEZUJ+yHDd2F4sX5OP1L7TWIOWFbI4vQK90ZZ7/jgiYQbwo=" +on_success: change +on_failure: always + email: +# The list name 'libvirt...@redhat.com" is encrypted against libvirt/libvirt +# to prevent IRC notifications from github forks. This was created using: +# $ travis encrypt -r "libvirt/libvirt" "libvirt...@redhat.com" +recipients: + - secure: "l6TTLcEcXdDEldHE2NgSIdt6a0k99ug3hp2W4IlnqJWJfIk/87nysJtLNrA0va20pPApCa3iJfMq4PUmBGiIIimTN0/KgC7tONDraogXhCbgfZp9Ejy/57TXxygSp4oum2kDw/c5uLnfrFV/xcn1fk6hvH6CD3bVcJPOQ/mc5FSKLqN5UzwqNnMpMTtG9qxCwfXJ/Bdm9fbURfezC7djcYRwRfPUe3TSD0L76G2HnQnSy4RqR3KFSjQHFPnSGM5IbsokbOaFKCyp/pHOt7QomQaY7YAPX/K9O+eP+hkkp6DGADkkumHctcgnMoyxpahf7pNKw9S8JYabH2NwREIq8whbp9Mo+R4rYO2ozroLWHaboYs/pBLrs606ivTwOmWGRCpJdCmmKTiZNyo6MRrwiOM6x+2YHUTMOa2kVheRNzaaxMFzHPW2kZ20bujPhfViJsRYj9flo5GJXJLyjluGZK5RjrguNJeIh8VJNBiSHW37uj7drmNBsqMad+65mf/4xtGITBqhz5Spx5R9UMZbuiJvcm8GasJMMdQ+bCfuWYjF2nZvSvFEr54Ii1YrDp6FKQ8YG1aD1/D8Z0/b3pLd/8Pn+M9yIWyO/Sto5TbSUjxBTmTStuDmtYE5uu1miYebvgJH5MovWPBegYgrfI417kPJgCG3q/R0YcZFMKFfQyo=" diff --git a/setup.py b/setup.py index f33ff1a..cc3a09a 100755 --- a/setup.py +++ b/setup.py @@ -290,15 +290,16 @@ class my_test(Command): 'lib' + plat_specifier) def find_nosetests_path(self): -paths = [ -"/usr/bin/nosetests-%d.%d" % (sys.version_info[0], +binaries = [ +"nosetests-%d.%d" % (sys.version_info[0], sys.version_info[1]), -"/usr/bin/nosetests-%d" % (sys.version_info[0]), -"/usr/bin/nosetests", +"nosetests-%d" % (sys.version_info[0]), +"nosetests", ] -for path in paths: -if os.path.exists(path): +for binary in binaries: +path = distutils.spawn.find_executable(binary) +if path != None: return path raise Exception("Cannot find any nosetests binary") -- 2.13.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH go-xml] Add support for domain hostdev and test code
On Fri, Sep 15, 2017 at 02:12:44PM +0800, zhenwei.pi wrote: > Signed-off-by: zhenwei.pi > --- > domain.go | 36 > domain_test.go | 44 > 2 files changed, 80 insertions(+) > > diff --git a/domain.go b/domain.go > index bead49a..1bcc9cc 100644 > --- a/domain.go > +++ b/domain.go > @@ -407,6 +407,29 @@ type DomainRNG struct { > Backend *DomainRNGBackend `xml:"backend"` > } > > +type DomainHostdevAdapter struct { > + Name string `xml:"name,attr,omitempty"` > +} > + > +type DomainHostdevSource struct { > + Protocol string`xml:"protocol,attr,omitempty"` > + Name string`xml:"name,attr,omitempty"` > + Wwpn string`xml:"wwpn,attr,omitempty"` This should be WWPN > + Adapter *DomainHostdevAdapter `xml:"adapter"` > + Address *DomainAddress`xml:"address"` > +} > + > +type DomainHostdev struct { > + XMLName xml.Name `xml:"hostdev"` > + Modestring `xml:"mode,attr"` > + Typestring `xml:"type,attr"` > + Sgiostring `xml:"sgio,attr,omitempty"` SGIO > + Rawio string `xml:"rawio,attr,omitempty"` And RawIO > + Managed string `xml:"managed,attr,omitempty"` > + Source *DomainHostdevSource `xml:"source"` > + Address *DomainAddress `xml:"address"` > +} > diff --git a/domain_test.go b/domain_test.go > index d1b107d..73dd47b 100644 > --- a/domain_test.go > +++ b/domain_test.go > @@ -37,6 +37,13 @@ type Address struct { > Function HexUint > } > > +type ScsiAddress struct { Should be SCSI ACK, and I'll fix the capitalization when pushing 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 python] Skip sparseRecvAll / sparseSendAll in sanity test
On Mon, Sep 18, 2017 at 11:47:24AM +0200, Michal Privoznik wrote: > On 09/14/2017 02:50 PM, Daniel P. Berrange wrote: > > The sanity test check aims to ensure that every function listed in > > the Python code maps to a corresponding C function. The Sparse > > send/recv methods are special though - we're never calling the > > corresponding C APIs, instead we have a pure python impl. > > > > Signed-off-by: Daniel P. Berrange > > --- > > sanitytest.py | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/sanitytest.py b/sanitytest.py > > index deec200..a5cb01b 100644 > > --- a/sanitytest.py > > +++ b/sanitytest.py > > @@ -351,7 +351,8 @@ for klass in gotfunctions: > > for func in sorted(gotfunctions[klass]): > > # These are pure python methods with no C APi > > if func in ["connect", "getConnect", "domain", "getDomain", > > -"virEventInvokeFreeCallback"]: > > +"virEventInvokeFreeCallback", > > +"sparseRecvAll", "sparseSendAll"]: > > continue > > > > key = "%s.%s" % (klass, func) > > > > Well, what if somebody builds -python against older libvirt that lacks > virStreamSparseRecvAll()? Are they facing an exception because cmod is > unable to find that func? Yes, the same is true of any of the functions where we manually written the Python API. To be fully correct we ought to annotate the manually written python code and then process the .py file to strip out the APIs we can't support. Patches welcome for doing that 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] [Qemu-devel] libvirt/QEMU/SEV interaction
On Mon, Sep 18, 2017 at 11:43:57AM +0200, Erik Skultety wrote: > [...] > > > > > > > > c) what existing communicate interface can be used between libvirt > > > and qemu > > > > to get the measurement ? can we add a new qemu monitor command > > > > 'get_sev_measurement' to get the measurement ? (step 10) > > > > > > Yes, QMP commands seeem most likely. > > > > > > > d) how to pass the secret blob from libvirt to qemu ? should we > > > consider > > > > adding a new object (sev-guest-secret) -- libvirt can add the > > > object through > > > > qemu monitor. > > > > > > Yeah, that looks like a viable option too. > > > > So I could see a flow like the following: > > > > > > 1. mgmt tool calls virConnectGetCapabilities. This returns an XML > > document that includes the following > > > > > > ...other bits... > > > > ...hex encoded PDH key... > > > > > > > > 2. mgmt tool requests to start a guest calling virCreateXML(), > > passing VIR_DOMAIN_START_PAUSED. The XML would include > > > > > > ...hex encode DH key... > > ..hex encode info... > > ...int32 value.. > > > > > > > > if is provided and VIR_DOMAIN_START_PAUSED is missing, > > libvirt would report an error and refuse to start the guest > > > > 3. Libvirt generates the QEMU cli arg to enable SEV using > > the XML data and starts QEMU, leaving CPUs paused > > > > 4. QEMU emits a SEV_MEASURE event containing the measurement > > blob > > Speaking of which, I expect QEMU to have a QMP command to retrieve the > measurement, in which case I think libvirt has to provide an API for the user > to retrieve the measurement in case libvirtd crashes somewhere between setting > up QEMU and waiting for the measurement event from QEMU, or simply because the > GO missed the event for some unspecified reason. Yeah, that's a good point - we also ought to have a pause-reason that reflects that it is paused due to waiting for SEV secrets. > > > > > 5. Libvirt catches the QEMU event and emits its own > > VIR_CONNECT_DOMAIN_EVENT_SEV_MEASURE event containing > > the measurement blob > > > > 6. GO does its validation of the measurement > > > > 7a If validation failed, then virDomainDestroy() to stop QEMU > > > > 7b If validation succeeed > > > > Optionally call > > > > virDomainSetSEVSecret() > > Given the fact that we're likely introducing a new element to the XML > config, I'm more inclined to utilizing the existing virSecret interfaces (as > was originally suggested) instead of creating a vendor-specific API. You could > have an optional secret sub-element within the element and libvirt would > simply check if that secret has a value set, once the GO issues > virDomainResume(). Any particular reason for having a specific API for this > that > I'm missing? Initially I was intending to suggest extensive use of virSecret, but it turns out that despite being called a "secret", none of the SEV data we are passing around needs protection. Either it is safe to be public, or it is already encrypted. So essentially we just have some data blobs we need to pass into QEMU. I didn't feel we ought to be abusing virSecret as a general purpose mechanism for passing in opaque data blobs which do not need any kind of protection. 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] RFC: libvirt support for QEMU live patching
On Mon, Sep 18, 2017 at 09:37:14AM +0200, Martin Kletzander wrote: > On Fri, Sep 15, 2017 at 09:18:18AM +0100, Daniel P. Berrange wrote: > > On Fri, Sep 15, 2017 at 01:27:31PM +0530, Madhu Pavan wrote: > > > Hi, > > > QEMU live patching should be just a matter of updating the QEMU RPM > > > package > > > and then live migrating the VMs to another QEMU instance on the same host > > > (which would point to the just installed new QEMU executable). > > > I think it will be useful to support it from libvirt side. After some > > > searching I found a > > > RFC patch posted in Nov 2013. Here is the link to it > > > https://www.redhat.com/archives/libvir-list/2013-November/msg00372.html > > > Approach followed in above mentioned link is as follows: > > > 1. newDef = deep copy oldVm definition > > > 2. newVm = create VM using newDef, start QEMU process with all vCPUs > > > paused > > > 3. oldVm migrate to newVm using unix socket > > > 4. shutdown oldVm > > > 5. newPid = newVm->pid > > > 6. finalDef = live deep copy of newVm definition > > > 7. Drop the newVm from qemu domain table without shutting down QEMU > > > process > > > 8. Assign finalDef to oldVm > > > 9. oldVm attaches to QEMU process newPid using finalDef > > > 10.resume all vCPUs in oldVm > > > > > > I can see it didn't get communities approval for having problems in > > > handling > > > UUID > > > of the vm's. To fix the problem we need to teach libvirt to manage two > > > qemu > > > processes > > > at once both tied to same UUID. I would like to know if there is any > > > interested approach > > > to get this done. I would like to send patches on this. > > > > > > Is there any specific reason why it is not been pursued for the last 4 > > > year? > > > > It isn't possible to make it work correctly in the general case, because > > both QEMU processes want to own the same files on disk. eg both might want > > to listen on a UNIX socket /foo/bar, but only one can do this. If you let > > the new QEMU delete the original QEMU's sockets, then you either break or > > delay incoming connections during the migration time, and you make it > > impossible to roll back on failure, or both. This kind of thing is not > > something that's acceptable for the usage scenerio described, which would > > need to be bulletproof to be usable in production. > > > > Can't we utilize namespaces for this? Lot of the things could be > separated, so we could fire up a new VM that's "containerized" like > this, migrate to it and then fire up a new one and migrate back. If the > first migration fails then we can still fallback. If it does not, then > the second one "should" not either. As well as increasing complexity and thus chances of failure, this also doubles the performance impact on the guest. More generally I think the high level idea of in-place live-upgrades for guests on a host is flawed. Even if we had the ability to in-place upgrade QEMU for running guest, there are always going to be cases where a security upgrade requires you to reboot the host system - kernel/glibc flaw, or fixes to other servicves that can't be restarted eg dbus. Any person hosting VMs will always always need to be able to handle migrating VMs to a *separate* host to deal with such security updates. For added complexity, determinining which security upgrades could be done by restarting QEMU vs which need to have a full host restart is not trivial, unless intimately familiar with the software stack. So from an administrative / operational POV, defining two separate procedures for dealing with upgrades is unwise. You have the chance of picking the wrong one and leaving a security fix accidentally unpatched, or if you take one path 95% of the time and the other path 5% of the time, chances are you're going to screw up the less used path due to lack of practice. It makes more sense to simplify the operational protocols by standardizing of cross-host migration, followed by host reboot, for applying patches. It simplifies the decision matrix removing complexity, and ensures you are well praticed following the same approach every time. The only cost is having 1 spare server against which to perform the migrations, but if your VMs are at all important, you will have spare hardware already to deal with unforseen hardware problems. If you absolutely can't afford spare hardware, you could just run your entire "host" OS in a container, and then spin up a second "host" OS on the same machine and migrate into that. From libvirt's POV this is still cross-host migration, so needs no special case. 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] spec: Use %license when available
On Fri, Sep 15, 2017 at 09:09:05AM +0100, Daniel P. Berrange wrote: > On Thu, Sep 14, 2017 at 05:43:37PM -0400, Cole Robinson wrote: > > This is required by the fedora packaging guidelines: > > > > https://fedoraproject.org/wiki/Packaging:LicensingGuidelines > > > > This macro isn't available on stock RHEL6 so provide a backcompat > > definition This doesn't seem to have worked https://ci.centos.org/view/libvirt/job/libvirt-master-rpm/systems=libvirt-centos-6/585/console 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] [libvirt-sandbox PATCH 1/2] Drop library/ from image path
On Wed, Jun 07, 2017 at 08:02:04AM +0200, Guido Günther wrote: > If one pastes from the output of virt-sansbox-image > > $ virt-sandbox-image list > docker:/library/ubuntu?tag=17.04 > docker:/library/debian?tag=latest > > verbatim > > $ virt-sandbox-image run -c qemu:///session > docker:/library/debian?tag=latest > > This fails like > > /home//.local/share/libvirt/images/library/debian:qbeilwxard.qcow2: > Could not create file: No such file or directory > > so strip off any leading components. > --- > libvirt-sandbox/image/sources/docker.py | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Daniel P. Berrange 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] [libvirt-sandbox PATCH 2/2] Sanitize domain name
On Wed, Jun 07, 2017 at 08:02:05AM +0200, Guido Günther wrote: > If one pastes from the output of virt-sansbox-image > > $ virt-sandbox-image list > docker:/library/ubuntu?tag=17.04 > docker:/library/debian?tag=latest > > verbatim > > $ virt-sandbox-image run -c qemu:///session > docker:/library/debian?tag=latest > > This fails like > > Unable to start sandbox: Failed to create domain: XML error: name > library/debian:qbeilwxard cannot contain '/' > > so don't allow invalid chars like '/' in domain names > --- > libvirt-sandbox/image/cli.py | 8 ++-- > 1 file changed, 6 insertions(+), 2 deletions(-) Reviewed-by: Daniel P. Berrange 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] RFC: libvirt support for QEMU live patching
On Fri, Sep 15, 2017 at 01:27:31PM +0530, Madhu Pavan wrote: > Hi, > QEMU live patching should be just a matter of updating the QEMU RPM package > and then live migrating the VMs to another QEMU instance on the same host > (which would point to the just installed new QEMU executable). > I think it will be useful to support it from libvirt side. After some > searching I found a > RFC patch posted in Nov 2013. Here is the link to it > https://www.redhat.com/archives/libvir-list/2013-November/msg00372.html > Approach followed in above mentioned link is as follows: > 1. newDef = deep copy oldVm definition > 2. newVm = create VM using newDef, start QEMU process with all vCPUs paused > 3. oldVm migrate to newVm using unix socket > 4. shutdown oldVm > 5. newPid = newVm->pid > 6. finalDef = live deep copy of newVm definition > 7. Drop the newVm from qemu domain table without shutting down QEMU process > 8. Assign finalDef to oldVm > 9. oldVm attaches to QEMU process newPid using finalDef > 10.resume all vCPUs in oldVm > > I can see it didn't get communities approval for having problems in handling > UUID > of the vm's. To fix the problem we need to teach libvirt to manage two qemu > processes > at once both tied to same UUID. I would like to know if there is any > interested approach > to get this done. I would like to send patches on this. > > Is there any specific reason why it is not been pursued for the last 4 year? It isn't possible to make it work correctly in the general case, because both QEMU processes want to own the same files on disk. eg both might want to listen on a UNIX socket /foo/bar, but only one can do this. If you let the new QEMU delete the original QEMU's sockets, then you either break or delay incoming connections during the migration time, and you make it impossible to roll back on failure, or both. This kind of thing is not something that's acceptable for the usage scenerio described, which would need to be bulletproof to be usable in production. 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] spec: Use %license when available
On Thu, Sep 14, 2017 at 05:43:37PM -0400, Cole Robinson wrote: > This is required by the fedora packaging guidelines: > > https://fedoraproject.org/wiki/Packaging:LicensingGuidelines > > This macro isn't available on stock RHEL6 so provide a backcompat > definition > > https://bugzilla.redhat.com/show_bug.cgi?id=1483293 > > Reported-by: Ville Skyttä > Signed-off-by: Cole Robinson > --- > libvirt.spec.in | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) Reviewed-by: Daniel P. Berrange 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] spec: Own %{_libdir}/libvirt{, /connection-driver} dirs
On Thu, Sep 14, 2017 at 05:43:06PM -0400, Cole Robinson wrote: > From: Ville Skyttä > > Owning all created directories is a requirement of the Fedora > packaging guidelines > > https://bugzilla.redhat.com/show_bug.cgi?id=1483293 > Signed-off-by: Cole Robinson > --- > libvirt.spec.in | 2 ++ > 1 file changed, 2 insertions(+) Reviewed-by: Daniel P. Berrange 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 7/8] travis: Install more build dependencies
On Thu, Sep 14, 2017 at 04:09:37PM +0200, Andrea Bolognani wrote: > On Thu, 2017-09-14 at 14:50 +0100, Daniel P. Berrange wrote: > > > @@ -5,37 +5,60 @@ addons: > > >apt: > > > # Please keep this list sorted alphabetically > > > packages: > [...] > > > + - zfs-fuse > > > > Did you check if travis actually lets you install all these ? They have > > to whitelist each individual package that's desired. So if some of these > > aren't in the whitelist, adding them here will have no effect IIUC. > > That's kind of insane. They don't fully trust the security of the container env in which they populate the packages I guess. > Anyway, it looks like all the packages in the list are allowed, > so whatever :) Ok, Reviewed-by: Daniel P. Berrange 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 4/8] travis: Split building command
On Thu, Sep 14, 2017 at 04:45:30PM +0200, Andrea Bolognani wrote: > On Thu, 2017-09-14 at 14:48 +0100, Daniel P. Berrange wrote: > > > script: > > > - - make -j3 && make -j3 syntax-check && make -j3 check > > > + - make -j3 > > > + - make -j3 syntax-check > > > + - make -j3 check > > > > > > # Environments here are run in addition to the main environment defined > > > above > > > matrix: > > > > The downside of this is that if syntax-check fails, but check succeeeds, > > you now have to search through the middle of the logfile to find the > > failure, instead of just jumping straight to the end. > > > > So I'm on the fence about this patch - I'd have a slight preference for > > existing behaviour of failing fast to keep errors at the end of the log > > I can drop this patch and have the end result look like > > script: > # Many unit tests still fail on macOS, and there are a bunch of issues > # with syntax-check as well, so skip them for now > - make -j3 && if [ "$TRAVIS_OS_NAME" != "osx" ]; then make -j3 > syntax-check && make -j3 check; fi > > Would you like that better? Yep, I think so 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 5/8] travis: Don't have a separate script definition for macOS
On Thu, Sep 14, 2017 at 03:09:05PM +0200, Andrea Bolognani wrote: > Make single commands OS-dependent instead. > > Signed-off-by: Andrea Bolognani > --- > .travis.yml | 10 -- > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/.travis.yml b/.travis.yml > index c2526bc6d..e93fc73b2 100644 > --- a/.travis.yml > +++ b/.travis.yml > @@ -77,8 +77,10 @@ before_script: >- ./autogen.sh > script: >- make -j3 > - - make -j3 syntax-check > - - make -j3 check > + # Many unit tests still fail on macOS, and there are a bunch of issues > + # with syntax-check as well, so skip them for now > + - if [ "$TRAVIS_OS_NAME" != "osx" ]; then make -j3 syntax-check; fi > + - if [ "$TRAVIS_OS_NAME" != "osx" ]; then make -j3 check; fi > > # Environments here are run in addition to the main environment defined above > matrix: > @@ -91,10 +93,6 @@ matrix: >dist: trusty > - compiler: clang >os: osx > - script: > -# many unit tests fail & so does syntax-check, so skip for now > -# one day we must fix it though > -- make -j3 > > after_failure: >- echo > '' > -- Reviewed-by: Daniel P. Berrange 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 6/8] travis: Improve test matrix
On Thu, Sep 14, 2017 at 03:09:06PM +0200, Andrea Bolognani wrote: > The default distribution is apparently ignored if an explicit test > matrix is provided, so we haven't actually been testing the precise > plus gcc combo. > > Signed-off-by: Andrea Bolognani > --- > .travis.yml | 10 -- > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/.travis.yml b/.travis.yml > index e93fc73b2..154727fa3 100644 > --- a/.travis.yml > +++ b/.travis.yml > @@ -1,8 +1,5 @@ > sudo: false > language: c > -dist: precise > -compiler: > - - gcc > cache: ccache > addons: >apt: > @@ -82,15 +79,16 @@ script: >- if [ "$TRAVIS_OS_NAME" != "osx" ]; then make -j3 syntax-check; fi >- if [ "$TRAVIS_OS_NAME" != "osx" ]; then make -j3 check; fi > > -# Environments here are run in addition to the main environment defined above That'll teach me to cut+paste from QEMU without checking reality :-) > matrix: >include: > +- compiler: gcc > + dist: precise > +- compiler: gcc > + dist: trusty > - compiler: clang >dist: precise > - compiler: clang >dist: trusty > -- compiler: gcc > - dist: trusty > - compiler: clang >os: osx Reviewed-by: Daniel P. Berrange 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 4/8] travis: Split building command
On Thu, Sep 14, 2017 at 03:09:04PM +0200, Andrea Bolognani wrote: > The build will fail if any of the commands fail, but this way we > might catch more errors in a single run. > > Signed-off-by: Andrea Bolognani > --- > .travis.yml | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/.travis.yml b/.travis.yml > index ba8ff49a1..c2526bc6d 100644 > --- a/.travis.yml > +++ b/.travis.yml > @@ -76,7 +76,9 @@ before_install: > before_script: >- ./autogen.sh > script: > - - make -j3 && make -j3 syntax-check && make -j3 check > + - make -j3 > + - make -j3 syntax-check > + - make -j3 check > > # Environments here are run in addition to the main environment defined above > matrix: The downside of this is that if syntax-check fails, but check succeeeds, you now have to search through the middle of the logfile to find the failure, instead of just jumping straight to the end. So I'm on the fence about this patch - I'd have a slight preference for existing behaviour of failing fast to keep errors at the end of the log None the less Reviewed-by: Daniel P. Berrange 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 7/8] travis: Install more build dependencies
On Thu, Sep 14, 2017 at 03:09:07PM +0200, Andrea Bolognani wrote: > Since configure automatically picks up as many optional dependencies > as possible, installing more packages allows us to improve our test > coverage. > > Signed-off-by: Andrea Bolognani > --- > .travis.yml | 37 ++--- > 1 file changed, 30 insertions(+), 7 deletions(-) > > diff --git a/.travis.yml b/.travis.yml > index 154727fa3..f56e6ae6f 100644 > --- a/.travis.yml > +++ b/.travis.yml > @@ -5,37 +5,60 @@ addons: >apt: > # Please keep this list sorted alphabetically > packages: > + - autoconf > + - automake >- autopoint > + - ccache >- dnsmasq-base > -# - dwarves > + - ebtables > + - gcc >- gettext > + - glusterfs-client > + - libacl1-dev >- libapparmor-dev > + - libattr1-dev >- libaudit-dev >- libavahi-client-dev > + - libblkid-dev > + - libc6-dev >- libcap-ng-dev > + - libcurl4-gnutls-dev > + - libdbus-1-dev >- libdevmapper-dev > - - libgcrypt11-dev > + - libfuse-dev >- libgnutls-dev > - - libncurses5-dev >- libnetcf-dev >- libnl-3-dev >- libnl-route-3-dev >- libnuma-dev > - - libparted0-dev > - - libpcap0.8-dev > + - libopenwsman-dev > + - libparted-dev > + - libpcap-dev >- libpciaccess-dev >- librbd-dev >- libreadline-dev >- libsasl2-dev > + - libssh2-1-dev > + - libssl-dev > + - libtool >- libudev-dev >- libxen-dev >- libxml2-dev >- libxml2-utils >- libyajl-dev >- lvm2 > - - uuid-dev > + - make > + - nfs-common > + - open-iscsi > + - parted > + - patch > + - pkg-config > + - policykit-1 > + - scrub > + - sheepdog > + - systemtap-sdt-dev >- xsltproc > - - zlib1g-dev > + - zfs-fuse > > notifications: >irc: Did you check if travis actually lets you install all these ? They have to whitelist each individual package that's desired. So if some of these aren't in the whitelist, adding them here will have no effect IIUC. 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 3/8] travis: Don't abort build due to -Wvariadic-macros
On Thu, Sep 14, 2017 at 03:09:03PM +0200, Andrea Bolognani wrote: > The openwsman header files are at fault here, but precise is entirely > unmaintained at this point so the issue will never be fixed. > > Better to ignore the error and have coverage over the Hyper-V driver > than disabling it: if code that would trigger the warning will be > added to libvirt, the CentOS CI will catch it. > > Signed-off-by: Andrea Bolognani > --- > .travis.yml | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/.travis.yml b/.travis.yml > index 367baf861..ba8ff49a1 100644 > --- a/.travis.yml > +++ b/.travis.yml > @@ -65,6 +65,9 @@ env: > # The custom $PATH is just to pick up some extra binaries installed > # through homebrew on macOS and it's completely harmless on Linux > - PATH="/usr/local/opt/gettext/bin:/usr/local/opt/rpcgen/bin:$PATH" > +# The hyperv driver fails to build with clang on precise due to this > +# error being raised in one of openwsman header files > +- CFLAGS="-Wno-error=variadic-macros" > - VIR_TEST_DEBUG=1 > > before_install: Reviewed-by: Daniel P. Berrange 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 2/8] travis: Move variables to 'env' section
On Thu, Sep 14, 2017 at 03:09:02PM +0200, Andrea Bolognani wrote: > Signed-off-by: Andrea Bolognani > --- > .travis.yml | 12 +--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/.travis.yml b/.travis.yml > index 8831f742c..367baf861 100644 > --- a/.travis.yml > +++ b/.travis.yml > @@ -60,14 +60,20 @@ git: >depth: 5 >submodules: true > > +env: > + global: > +# The custom $PATH is just to pick up some extra binaries installed > +# through homebrew on macOS and it's completely harmless on Linux > +- PATH="/usr/local/opt/gettext/bin:/usr/local/opt/rpcgen/bin:$PATH" > +- VIR_TEST_DEBUG=1 > + > before_install: >- if [ "$TRAVIS_OS_NAME" == "osx" ]; then brew update && brew install > gnutls libgcrypt yajl gettext rpcgen ; fi > > -# the custom PATH is just to pick up OS-X homebrew & its harmless on Linux > before_script: > - - PATH="/usr/local/opt/gettext/bin:/usr/local/opt/rpcgen/bin:$PATH" > ./autogen.sh > + - ./autogen.sh > script: > - - VIR_TEST_DEBUG=1 make -j3 && make -j3 syntax-check && make -j3 check > + - make -j3 && make -j3 syntax-check && make -j3 check > > # Environments here are run in addition to the main environment defined above > matrix: Reviewed-by: Daniel P. Berrange 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 1/8] travis: Limit git depth to 5 commits
On Thu, Sep 14, 2017 at 03:09:01PM +0200, Andrea Bolognani wrote: > We don't need 50 commits for our purposes, so might as well save some > bandwidth and possibly some time by making the clone more shallow. > > Signed-off-by: Andrea Bolognani > --- > .travis.yml | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/.travis.yml b/.travis.yml > index f59bd19c8..8831f742c 100644 > --- a/.travis.yml > +++ b/.travis.yml > @@ -57,6 +57,7 @@ notifications: >- secure: > "QcU9eP96P0RlDNzVRZl/4sxyydPStGzECrpgJhr2IPB/7pHk23yaBrmUsq9S830tB+jwLGma1IscNB8uf7Sf7WY+cYIpfR8v030OffWnaipo/Gcs0dpnlfURWHjOFQI3RJzGEihsqvbwUFOwsM+3IDyO3qdWaiT6cN2Tj9ROlwYCySSX5YWzLyX7arBZ4lp8ESs7ohQaEwp2cegnMP2oGPJJe4SebvlCDjHZbjkU5aEradwUWnRQDJZWTKknpNLArVFxN2/ixp6f/MGY4DmkHoDweio6mHIPN5zTs5Jt32aiX6wDBa+bBa4v8TCRqzhYkQ63ZZhNV8bY5Uf9ufTdyvt96yIANyakd85b1QpMdAX76IyJi1l0/Uub6DTQZAcq3vK7iPjGeTVSpyoXrqTfGy4JxMjqDoocpWvv8ALX1wrYI/HfN2R2Aepw9jModTimOsebYhJ1yMhSt8qnh5AQNftGKL2JBKoA1LWdU2YJ5fO1bGjKNiVEkGFQTPYFWrYCUY5JcT+s5WCzNeMNm8s9na8liYhGl3WtS3rPr5M8bof+BMsBhG2hQ0loduc94x2GkvyhQZUgRbqrwNR+y4hn+rWFC3hBzzyiAULs43vY/PJ+eBdKEf3VAc0MkhQ8GgXGSA61fR6aXYonroI/WnBVItwDmUnnMfSziZXxk09GLl4=" > > git: > + depth: 5 >submodules: true > > before_install: Reviewed-by: Daniel P. Berrange 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] [libvirt-python][PATCH] maint: Require libvirt-3.4.0 at least
On Thu, Sep 14, 2017 at 03:05:15PM +0200, Michal Privoznik wrote: > On 09/14/2017 02:42 PM, Daniel P. Berrange wrote: > > On Thu, Sep 14, 2017 at 01:41:08PM +0200, Michal Privoznik wrote: > >> Currently, we require 0.9.11. However, some APIs are missing > >> there and thus sanity check fails: > >> > >> DEBUG: /usr/bin/python sanitytest.py build/lib.linux-s390x-2.7 > >> /usr/share/libvirt/api/libvirt-api.xml > >> DEBUG: FAIL virStream.sparseRecvAll (Python API not mapped to C) > >> DEBUG: FAIL virStream.sparseSendAll (Python API not mapped to C) > >> DEBUG: error: command '/usr/bin/python' failed with exit status 1 > >> > >> I'm not sure how to fix that so raising minimal required libvirt > >> version is the solution. > >> > >> Signed-off-by: Michal Privoznik > >> --- > >> setup.py | 2 +- > >> README | 2 +- > >> libvirt-override.c | 149 > >> - > >> 3 files changed, 2 insertions(+), 151 deletions(-) > >> > >> diff --git a/setup.py b/setup.py > >> index f33ff1a..f929eb2 100755 > >> --- a/setup.py > >> +++ b/setup.py > >> @@ -17,7 +17,7 @@ import re > >> import shutil > >> import time > >> > >> -MIN_LIBVIRT = "0.9.11" > >> +MIN_LIBVIRT = "3.4.0" > > > > NACK, we cannot do this - it will break many people and apps (OpenStack > > in particular) who expect latest libvirt on pypi to work with historical > > C libs. > > I don't know how pypi works, but if somebody distributes just > libvirt-python and doesn't ship libvirt.so too, such process is broken > already because libvirt-python could have been compiled with one version > of libvirt while user might be running a different one. So shipping > libvirt.so is the only way. And since libvirt-python doesn't add any new > features compared to bare libvirt, why on earth would somebody want to > run latest libvirt-python but an ancient libvirt? It doesn't make much > sense to update one without other. Pypi only ships the source tar.gz. When you pip install libvirt-python it is then built against the libvirt C library you have installed. The newer libvirt-python bindings sometimes fix bugs wrt bindings of previously existing APIs, so there is a reason to run newer libvirt-python. In addition building libvirt-python from pypi instead of relying on distro installed RPMs of libvirt-python lets apps use arbitrary versions of python, not just the one the distro built against. This all works very well and is relied upon by OpenStack. 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
[libvirt] [PATCH python] Skip sparseRecvAll / sparseSendAll in sanity test
The sanity test check aims to ensure that every function listed in the Python code maps to a corresponding C function. The Sparse send/recv methods are special though - we're never calling the corresponding C APIs, instead we have a pure python impl. Signed-off-by: Daniel P. Berrange --- sanitytest.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sanitytest.py b/sanitytest.py index deec200..a5cb01b 100644 --- a/sanitytest.py +++ b/sanitytest.py @@ -351,7 +351,8 @@ for klass in gotfunctions: for func in sorted(gotfunctions[klass]): # These are pure python methods with no C APi if func in ["connect", "getConnect", "domain", "getDomain", -"virEventInvokeFreeCallback"]: +"virEventInvokeFreeCallback", +"sparseRecvAll", "sparseSendAll"]: continue key = "%s.%s" % (klass, func) -- 2.13.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-python][PATCH] maint: Require libvirt-3.4.0 at least
On Thu, Sep 14, 2017 at 01:41:08PM +0200, Michal Privoznik wrote: > Currently, we require 0.9.11. However, some APIs are missing > there and thus sanity check fails: > > DEBUG: /usr/bin/python sanitytest.py build/lib.linux-s390x-2.7 > /usr/share/libvirt/api/libvirt-api.xml > DEBUG: FAIL virStream.sparseRecvAll (Python API not mapped to C) > DEBUG: FAIL virStream.sparseSendAll (Python API not mapped to C) > DEBUG: error: command '/usr/bin/python' failed with exit status 1 > > I'm not sure how to fix that so raising minimal required libvirt > version is the solution. > > Signed-off-by: Michal Privoznik > --- > setup.py | 2 +- > README | 2 +- > libvirt-override.c | 149 > - > 3 files changed, 2 insertions(+), 151 deletions(-) > > diff --git a/setup.py b/setup.py > index f33ff1a..f929eb2 100755 > --- a/setup.py > +++ b/setup.py > @@ -17,7 +17,7 @@ import re > import shutil > import time > > -MIN_LIBVIRT = "0.9.11" > +MIN_LIBVIRT = "3.4.0" NACK, we cannot do this - it will break many people and apps (OpenStack in particular) who expect latest libvirt on pypi to work with historical C libs. 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 go-xml 0/2] DomainAddress parsing fix
On Wed, Sep 13, 2017 at 01:57:44PM +0100, Dean Smith wrote: > *** BLURB HERE *** > I have taken Anton Kryukovs fix from #13 on github and am submitting > along with addition of 1.8 and 1.9 as travis go version targets > > Example of failing running on 1.9, which passes on 1.7 can be found > at > g...@github.com:deasmi/libvirt-go-xml.git > branch parsefailtest Thanks I've pushed these patches 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 2/2] qemu: Use secret objects to pass iSCSI passwords
On Wed, Sep 13, 2017 at 03:24:42PM +0200, Peter Krempa wrote: > On Wed, Sep 13, 2017 at 09:05:43 -0400, John Ferlan wrote: > > > > I was thinking more about virstoragefile.c and virstoragetest.c. It's > > easy enough to fetch the user/password-secret in > > virStorageSourceParseBackingJSONiSCSI: > > > > +const char *user = virJSONValueObjectGetString(json, "user"); > > +const char *secret = virJSONValueObjectGetString(json, > > "password-secret"); > > > > But there's not much one can do with it as you get (for example): > > > > user = "myname" > > secret = "virtio-disk1-secret0" > > > > but an would look like: > > > > > > > > > > > > So to a degree it's not testable to build the XML as virstoragetest > > does. Sine the secret can be built up using the disk alias, it's perhaps > > not even worth storing away. > > Yes, the field is rather useless. I'm even surprised that it's recorded > into the backing file string. It may be used as a notification that > authentication is required, but without actually adding the secret with > correct name, the thing won't work anyways. That is a bug in QEMU - we should not be recording the 'password-secret" field in the backing store that is embedded in the qcow2 file. The IDs of the secret objects are only meaningful for the current invokation of QEMU / qemu-img / etc. So if a backing store needs a password, then libvirt needs to explicitly set the password-secret for the backing store, recursively 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] Yet another RFC for CAT
On Tue, Sep 12, 2017 at 11:33:53AM +0200, Martin Kletzander wrote: > On Thu, Sep 07, 2017 at 11:02:21AM +0800, 乔立勇(Eli Qiao) wrote: > > > I'm concerned about the idea of not checking 'from' for collisions, > > > if there's allowed a mix of guests with & within 'from'. > > > > eg consider > > > > > > * Initially 24 MB of cache is free, starting at 8MB > > > * run guest A from=8M, size=8M > > > * run guest B size=8M > > > => libvirt sets from=16M, so doesn't clash with A > > > * stop guest A > > > * run guest C size=8M > > > => libvirt sets from=8M, so doesn't clash with B > > > * restart guest A > > > => now clashes with guest C, whereas if you had > > > left guest A running, then C would have > > > got from=24MB and avoided clash > > > > > > IOW, if we're to allow users to set 'from', I think we need to > > > have an explicit flag to indicate whether this is an exclusive > > > or shared allocation. That way guest A would set 'exclusive', > > > and so at least see an error when it got a clash with guest > > > C in the example. > > > > > > > +1 > > > > OK, I didn't like the exclusive/shared allocation at first when I > thought about it, but getting back to it it looks like it could save us > from unwanted behaviour. I think that if you are setting up stuff like > this you should already know what you will be running and where. I > thought that specifying 'from' also means that it can be shared because > you have an idea about the machines running on the host. But that's not > nice and user friendly. > > What I'm concerned about is the difference between the following > scenarios: > > * run guest A from=0M size=8M allocation=exclusive > * run guest B from=0M size=8M allocation=shared > > and > > * run guest A from=0M size=8M allocation=shared > * run guest B from=0M size=8M allocation=shared > > When starting guest B, how do you know whether to error out or not? I'm > not considering collecting information on all domains as that "does not > scale" (as the cool kids would say it these days). The only idea I have > is naming the group accordingly, e.g.: > > libvirt-qemu-domain-3-testvm-vcpu0-3+emu+io2-7+shared/ > > gross name, but users immediately know what it is for. I think we have to track usage state across all VMs globally. Think of this in the same way as we think of VNC port allocation, or NWFilter creation, or PCI/USB device allocation. These are all global resources and we have to track them as such. I don't see any way to avoid that in the cache mgmt too. I don't really see a big problem with scalability here as the logic we'd have to run to acqurie/release allocations is not going to be computationally expensive, so contention on the mutexes durnig startup should be light. > > > > - After starting a domain, fill in any missing information about the > > > > allocation (I'm generalizing here, but fro now it would only be the > > > > optional "from" attribute) > > > > > > > > - Add settings not only for vCPUs, but also for other threads as we do > > > > with pinning, schedulers, etc. > > > > > > > > Thanks Martin to propose this again. > > > > I have started this RFC since the beginning of the year, and made several > > junior patches, but fail to get merged. > > > > While recently I (together with my team) have started a software "Resource > > Management Daemon" to manage resource like last level cache, do cache > > allocation and cache usage monitor, it's accept tcp/unix socket REST API > > request and talk with /sys/fs/resctrl interface to manage all CAT stuff. > > > > RMD will hidden the complexity usage in CAT and it support not only VM > > but also other applications and containers. > > > > RMD will open source soon in weeks, and could be leveraged in libvirt > > or other management software which want to have control of fine granularity > > resource. > > > > We have done an integration POC with OpenStack Nova, and would like > > to get into integrate too. > > > > Would like to see if libvirt can integrate with RMD too. > > > > I'm afraid there was such effort from Marcelo called resctrltool, > however it was denied for some reason. Daniel could elaborate if you'd > like to know more, I can't really recall the reasoning behind it. We didn't want to exec external python programs because that certainly *does* have bad scalability, terrible error reporting facilities and need to parse ill defined data formats from stdout, etc. It doesn't magically solve the complexity, just moves it elsewhere where we have less ability to tailor it to fit into libvirt's model. 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-l
Re: [libvirt] New QEMU daemon for persistent reservations
On Mon, Sep 11, 2017 at 05:47:28PM +0200, Paolo Bonzini wrote: > On 11/09/2017 17:33, Daniel P. Berrange wrote: > > On Mon, Sep 11, 2017 at 05:27:20PM +0200, Paolo Bonzini wrote: > >> On 11/09/2017 17:23, Daniel P. Berrange wrote: > >>>> On the other hand, the daemon has CAP_SYS_RAWIO and CAP_SYS_ADMIN, so if > >>>> you get memory corruption all bets are probably off anyway. > >>> That's where the benefit of strict selinux labelling comes in. If we had > >>> strict labelling of the individual paths below the device, then even if > >>> the daemon got corrupted, the policy would prevent it from doing any > >>> damage to the system beyond calling ioctl() the individual paths it had > >>> been granted. It wouldn't be able to access devices associated with > >>> the host OS mounts, or other non-VM related or non-multipath related > >>> block devices. > >> > >> Sure, but those capabilities let you do a lot of nasty things > >> indirectly, even within the constraints of the SELinux policy. > >> > >> For example, if you are able to reconfigure device mapper, you can > >> convince the kernel to write to any block device---even if you cannot > >> open it. IDWEFAL (I don't write exploits for a living) but I'm sure > >> that's just scraping the surface. > > > > Surely we would not write an SELinux policy that allows this daemon > > to reconfigure device mapper. > > > > IIUC, all this daemon should need is the ability to request persistent > > reservations on the individual paths associated with the mpath device. > > > > Is it not possible to write a SElinux policy which allows that, without > > also allowing reconfiguration of device mapper. > > As far as I know, querying and reconfiguring the device mapper are both > done with ioctls on /dev/mapper/control, and both require CAP_SYS_ADMIN. > > Maybe future versions of Linux could change it to require CAP_SYS_ADMIN > only for reconfiguration, so that the PR helper daemon does not require > the capability anymore. However, that would be independent from > SELinux, which only controls "ioctl" access without finer-grain choice > of which ioctls to allow. I don't suppose that the LUNS behind a mpath device end up being surface in /sys/block anywhere do they ? > I understand that you want to protect in depth, but unfortunately this > only works if all layers are aware of SELinux. Luckily the daemon is > much, much smaller than QEMU, and so is the attack surface. It would be ok if we think its possible to lock it down later (once any needed kernel enhancements are developed), without having to change the interaction between QEMU / libvirt / helper daemon. I'm beginning to feel that is ok. 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] New QEMU daemon for persistent reservations
On Mon, Sep 11, 2017 at 05:20:10PM +0200, Paolo Bonzini wrote: > On 11/09/2017 16:32, Daniel P. Berrange wrote: > >> This is already handled via SCM_RIGHTS and is part of the design of the > >> helper daemon. QEMU cannot even open mpath devices which are not > >> accessible according to its SELinux category or device cgroup. > > > > Ah so the daemon relies on the fact that the client was not permitted > > to open another file. So the only FD it can receive from the client is > > one that was associated with a permitted mpath device. > > > > That would be sufficient to protect against a malicious qemu process > > trying to explicitly pass it invalid FD data. It wouldn't be sufficient > > to be safe against a QEMU process that somehow managed to trigger a bug > > that caused it to corrupt memory and thus trick into opening a different > > file. For the latter we would need to have some stricter policy about > > the helper daemon and labelling on files. > > Exactly. The passed file descriptor acts as a "capability"; the daemon > goes from there to the paths through fstat on the file descriptor > followed by /dev/mapper/control APIs (mostly issued by libmpathpersist). > > On the other hand, the daemon has CAP_SYS_RAWIO and CAP_SYS_ADMIN, so if > you get memory corruption all bets are probably off anyway. That's where the benefit of strict selinux labelling comes in. If we had strict labelling of the individual paths below the device, then even if the daemon got corrupted, the policy would prevent it from doing any damage to the system beyond calling ioctl() the individual paths it had been granted. It wouldn't be able to access devices associated with the host OS mounts, or other non-VM related or non-multipath related block devices. 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] New QEMU daemon for persistent reservations
On Mon, Sep 11, 2017 at 05:27:20PM +0200, Paolo Bonzini wrote: > On 11/09/2017 17:23, Daniel P. Berrange wrote: > >> On the other hand, the daemon has CAP_SYS_RAWIO and CAP_SYS_ADMIN, so if > >> you get memory corruption all bets are probably off anyway. > > That's where the benefit of strict selinux labelling comes in. If we had > > strict labelling of the individual paths below the device, then even if > > the daemon got corrupted, the policy would prevent it from doing any > > damage to the system beyond calling ioctl() the individual paths it had > > been granted. It wouldn't be able to access devices associated with > > the host OS mounts, or other non-VM related or non-multipath related > > block devices. > > Sure, but those capabilities let you do a lot of nasty things > indirectly, even within the constraints of the SELinux policy. > > For example, if you are able to reconfigure device mapper, you can > convince the kernel to write to any block device---even if you cannot > open it. IDWEFAL (I don't write exploits for a living) but I'm sure > that's just scraping the surface. Surely we would not write an SELinux policy that allows this daemon to reconfigure device mapper. IIUC, all this daemon should need is the ability to request persistent reservations on the individual paths associated with the mpath device. Is it not possible to write a SElinux policy which allows that, without also allowing reconfiguration of device mapper. 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] New QEMU daemon for persistent reservations
On Mon, Sep 11, 2017 at 04:23:27PM +0200, Paolo Bonzini wrote: > On 11/09/2017 14:09, Daniel P. Berrange wrote: > > On Mon, Sep 11, 2017 at 01:44:59PM +0200, Paolo Bonzini wrote: > >> On 11/09/2017 12:46, Daniel P. Berrange wrote: > >>>> So the helper is a bit different from QEMU with respect to both SELinux > >>>> MCS labeling and the devices cgroup. Access limitation comes from only > >>>> ever operating on devices that have been passed on the socket. SELinux > >>>> MCS could be used so that only the "right" QEMU can connect to each > >>>> instance of the helper, though. > >>> I wonder how this interacts with SELinux. IIUC if we grant access to > >>> the multipath device file, the kernel won't normally require us to > >>> grant access the underlying paths. I/O is just allowed because they > >>> are a member of the mpath device. Hopefully this applies to the > >>> persistent reservations too ? > >> > >> No, persistent reservations are special; they have to be registered > >> independently on each path. (As I said, this was the original reason to > >> have a separate daemon: implementing it in QEMU would be not just a bad > >> idea because you need CAP_SYS_ADMIN, it would be impossible for > >> libvirt-started guests because of sVirt and device cgroups). > >> > >> So I think the helper daemon needs to be granted blanket ioctl access to > >> devices, without using either device cgroups or MCS. > > > > If it was a single helper daemon for all guests, that would be ok to be > > granted access to all devices. If we did that though, the daemon would > > have to be SELinux aware. ie, libvirt would have to talk to it and say > > that svirt_t:s0:c236,c660 is permitted /dev/mpath/foo, and it would > > have to validate the requests from the socket to QEMU to make sure that > > QEMU is not requesting access to other mpath devices. > > This is already handled via SCM_RIGHTS and is part of the design of the > helper daemon. QEMU cannot even open mpath devices which are not > accessible according to its SELinux category or device cgroup. Ah so the daemon relies on the fact that the client was not permitted to open another file. So the only FD it can receive from the client is one that was associated with a permitted mpath device. That would be sufficient to protect against a malicious qemu process trying to explicitly pass it invalid FD data. It wouldn't be sufficient to be safe against a QEMU process that somehow managed to trigger a bug that caused it to corrupt memory and thus trick into opening a different file. For the latter we would need to have some stricter policy about the helper daemon and labelling on files. 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] New QEMU daemon for persistent reservations
On Mon, Sep 11, 2017 at 01:44:59PM +0200, Paolo Bonzini wrote: > On 11/09/2017 12:46, Daniel P. Berrange wrote: > >> So the helper is a bit different from QEMU with respect to both SELinux > >> MCS labeling and the devices cgroup. Access limitation comes from only > >> ever operating on devices that have been passed on the socket. SELinux > >> MCS could be used so that only the "right" QEMU can connect to each > >> instance of the helper, though. > > I wonder how this interacts with SELinux. IIUC if we grant access to > > the multipath device file, the kernel won't normally require us to > > grant access the underlying paths. I/O is just allowed because they > > are a member of the mpath device. Hopefully this applies to the > > persistent reservations too ? > > No, persistent reservations are special; they have to be registered > independently on each path. (As I said, this was the original reason to > have a separate daemon: implementing it in QEMU would be not just a bad > idea because you need CAP_SYS_ADMIN, it would be impossible for > libvirt-started guests because of sVirt and device cgroups). > > So I think the helper daemon needs to be granted blanket ioctl access to > devices, without using either device cgroups or MCS. If it was a single helper daemon for all guests, that would be ok to be granted access to all devices. If we did that though, the daemon would have to be SELinux aware. ie, libvirt would have to talk to it and say that svirt_t:s0:c236,c660 is permitted /dev/mpath/foo, and it would have to validate the requests from the socket to QEMU to make sure that QEMU is not requesting access to other mpath devices. If it was a one helper daemon per QEMU instance, then we would want to directly confine it to just the underlying devices associated with the mpath device allowed for that QEMU instance. This would imply that we needed to label the underlying devices with the MCS label to match the VM. 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] New QEMU daemon for persistent reservations
On Sun, Sep 10, 2017 at 11:52:24AM +0200, Paolo Bonzini wrote: > On 29/08/2017 14:41, Daniel P. Berrange wrote: > > On Tue, Aug 22, 2017 at 06:27:40PM +0200, Paolo Bonzini wrote: > >> Hi all, > >> > >> I am adding a new daemon to QEMU, that QEMU can connect to in order to > >> issue persistent reservation commands. > > > > Can you elaborate on what this daemon does ? > > > > IIUC, by 'persistent reservation' you are referring to SCSI LUN > > reservations ? If so, the security model needs to involve more > > than just policy on the socket that QEMU uses to talk to the > > PR daemon. We need to be able to control what device nodes this > > daemon is permitted to access. > > > > For iSCSI backed disks, the daemon might need to use the libiscsi > > driver, instead of assuming there is a device node on the local > > host too. > > libiscsi-backed disks can already issue persistent reservations. The > daemon is only needed for physical disks, as an alternative to > sgio='unfiltered' or (yuck) running QEMU with CAP_SYS_RAWIO. > > > Libvirt has a generic lock manager facility and I've previously > > though might be able to be extended to acquire SCSI reservations > > for disks which are backed by SCSI/iSCSI, but never tried to > > work on this. > > This is an interesting idea, but it is unrelated to this work, which is > about guests who manage the locking on their own (through peristent > reservations). Ah ok, I missed that this was about allowing the guest todo reservations itself, rather than QEMU using it for disk locking. > > I'm not sure if want to have QEMU spawning this daemon itself or not. > > Definitely not. The daemon is not suid root. > > > On the one hand if QEMU spawns the daemon, then it means the daemon > > inherits the SELinux policy of QEMU by default, so is restricted to > > only access the devices that QEMU has been granted. > > Libvirt could also give a confined policy to the helper, but there are > some complications because of multipath. When passed a multipath > device, the daemon forwards the PR command to _all_ paths, not just the > currently active one, similar to the mpathpersist(1) command. (In fact > this was the original usecase; support for non-multipath devices was > just an easy and useful extension.) > > So the helper is a bit different from QEMU with respect to both SELinux > MCS labeling and the devices cgroup. Access limitation comes from only > ever operating on devices that have been passed on the socket. SELinux > MCS could be used so that only the "right" QEMU can connect to each > instance of the helper, though. I wonder how this interacts with SELinux. IIUC if we grant access to the multipath device file, the kernel won't normally require us to grant access the underlying paths. I/O is just allowed because they are a member of the mpath device. Hopefully this applies to the persistent reservations too ? 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] virsh: help: Drop 'id' from possible values for argument
On Mon, Sep 11, 2017 at 10:01:45AM +0200, Erik Skultety wrote: > At the moment, we can only rename inactive domains. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1490164 > > Signed-off-by: Erik Skultety > --- > > Theoretically, we could also remove this check from qemu_driver.c as it's a > useless check, given that a VM which passes the 'active' check is going to be > a > persistent domain: > > if (!vm->persistent) { > virReportError(VIR_ERR_OPERATION_INVALID, "%s", >_("cannot rename a transient domain")); > goto endjob; > } > > Also, virshCommandOptDomainBy could be used instead of virshCommandOptDomain > in > this case, but we might as well enable rename for active domains as well, who > knows and I don't think this is worth any more attention that just a help > string > tweak. > > tools/virsh-domain.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c > index f235c66b0..84c8dccae 100644 > --- a/tools/virsh-domain.c > +++ b/tools/virsh-domain.c > @@ -10151,7 +10151,7 @@ static const vshCmdInfo info_domrename[] = { > }; > > static const vshCmdOptDef opts_domrename[] = { > -VIRSH_COMMON_OPT_DOMAIN_FULL, > +VIRSH_COMMON_OPT_DOMAIN(N_("domain name or uuid")), > {.name = "new-name", > .type = VSH_OT_DATA, > .flags = VSH_OFLAG_REQ, NACK. Virsh is a tool that runs client side, where as the check for non-running status is done server side. An old virsh may be talking to a new libvirtd which allows it for running guests. IOW, virsh should not make this kind of assumption about the QEMU driver impl 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] [Qemu-devel] libvirt/QEMU/SEV interaction
On Fri, Sep 08, 2017 at 10:48:10AM -0500, Brijesh Singh wrote: > > So I could see a flow like the following: > > > The flow looks good > > > > > > >1. mgmt tool calls virConnectGetCapabilities. This returns an XML > > document that includes the following > > > > > > ...other bits... > > > > ...hex encoded PDH key... > > > > > > > >2. mgmt tool requests to start a guest calling virCreateXML(), > > passing VIR_DOMAIN_START_PAUSED. The XML would include > > > > > > ...hex encode DH key... > > ..hex encode info... > > ...int32 value.. > > > > > > > > if is provided and VIR_DOMAIN_START_PAUSED is missing, > > libvirt would report an error and refuse to start the guest > > > > > One thing which is not clear to me is, how do we know that we are asked > to launch SEV guest? Are you thinking that tag in the XML will > hint libvirt that GO has asked to launch a SEV guest? Yes, the existance of the tag is the indicator that informs libvirt that SEV *must* be used for the guest. > >3. Libvirt generates the QEMU cli arg to enable SEV using > > the XML data and starts QEMU, leaving CPUs paused > > > > > I am looking at [1] to get the feel for how do we model it in the XML. > As you can see I am using ad-hoc to create the sev-guest > object. Currently, sev-guest object accepts the following properties: > > dh-cert-file: > session-info-file: > policy: > > I believe the new XML model will influence the property input type, > Any recommendation on how do model this part ? thank you so much. That looks ok to me - even if QEMU wants the data provided in files on disk, libvirt can just create the files on the fly from the data it has in the element in the XML file. Since they're only needed during startup, libvirt can then easily delete the files the moment QEMU has completed its startup. > > [1] https://libvirt.org/formatdomain.html#elementsCPU > > > >4. QEMU emits a SEV_MEASURE event containing the measurement > > blob > > > >5. Libvirt catches the QEMU event and emits its own > > VIR_CONNECT_DOMAIN_EVENT_SEV_MEASURE event containing > > the measurement blob > > > >6. GO does its validation of the measurement > > > >7a If validation failed, then virDomainDestroy() to stop QEMU > > > >7b If validation succeeed > > > > Optionally call > > > > virDomainSetSEVSecret() > > > > providing the optional secret, then > > > > virDomainResume() > > > > to let QEMU continue > > > > > > > > > > Regards, > > Daniel > > 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] [Qemu-devel] libvirt/QEMU/SEV interaction
On Fri, Sep 08, 2017 at 01:45:06PM +, Relph, Richard wrote: > A few answers in line… > > On 9/8/17, 8:16 AM, "Daniel P. Berrange" wrote: > > On Fri, Sep 08, 2017 at 06:57:30AM -0500, Brijesh Singh wrote: > > Hi All, > > > > (sorry for the long message) > > > > CPUs from AMD EPYC family supports Secure Encrypted Virtualization (SEV) > > feature - the feature allows running encrypted VMs. To enable the > feature, > > I have been submitting patches to Linux kernel [1], Qemu [2] and OVMF > [3]. > > We have been making some good progress in getting patches accepted > upstream > > in Linux and OVMF trees. SEV builds upon SME (Secure Memory Encryption) > > feature -- SME support just got pulled into 4.14 merge window. The base > > SEV patches are accepted in OVMF tree -- now we have SEV aware guest > BIOS. > > I am getting ready to take off "RFC" tag from remaining patches to get > them > > reviewed and accepted. > > > > The boot flow for launching an SEV guest is a bit different from a > typical > > guest launch. In order to launch SEV guest from virt-manager or other > > high-level VM management tools, we need to design and implement new > > interface between libvirt and qemu, and probably add new APIs in libvirt > > to be used by VM management tools. I am new to the libvirt and need some > > expert advice while designing this interface. A pictorial representation > > for a SEV guest launch flow is available in SEV Spec Appendix A [4]. > > > > A typical flow looks like this: > > > > 1. Guest owner (GO) asks the cloud provider to launch SEV guest. > > 2. VM tool asks libvirt to provide its Platform Diffie-Hellman (PDH) > key. > > 3. libvirt opens /dev/sev device to get its PDH and return the blob to > the > >caller. > > What sort of size are we talking about for the PDH ? > > The PDH blob is described in reference 4. It’s 0x824 bytes long… a bit over > 2K bytes. > PDH is “Platform Diffie-Hellman” key, public portion. > > There's a few ways libvirt could report it > > 1. As an XML element in the host capabilities XML > 2. As an XML element in the emulator capabilities XML > 3. Via a newly added host API > > > 4. VM tool gives its PDH to GO. > > 5. GO provides its DH key, session-info and guest policy. > > Are steps 4 & 5 strictly required to be in this order, or is it > possible > > Steps 4 and 5 must occur in that order. The data sent by the GO in the > “session info” is encrypted and integrity protected with keys that the > GO derives from the GO private Diffie-Hellman key and the PDH public > Diffie-Hellman key. > > What are the security requirements around the DH key, session info > and guest policy ? Are any of them sensitive data which needs to be > kept private from untrustworthy users. Also are all three of these > items different for every guest launched, or are some of them > likely to be the same for every guest ? > > The PDH is not sensitive. It is relatively static for the platform. > (It only changes when the platform owner chooses to change the apparent > identity of the platform that will actually run the virtual machine.) > The 128-byte session info data is encrypted and integrity protected by > the GO. It need not be additionally encrypted or integrity protected. > It should vary for EVERY guest launch. > The 4-byte guest policy must be sent in the clear as some components > may want to observe the policy bits. It may change from guest to guest, > but there will likely only be a few common values. Given this, and the pretty small data sizes, I think this info could in fact all be provided inline in the XML config - either hex or base64 encoded for the binary blobs. > > 8. libvirt launches the guest with "-S" > > All libvirt guests get launched with -S, to give libvirt chance to do some > setup before starting vCPUs. Normally vCPUs are started by default, but > the VIR_DOMAIN_START_PAUSED flag allows the mgmt app to tell libvirt to > leave vCPUS paused. > > Alternatively, if libvirt sees presencese of 'sev' config for the guest, > it could automatically leave it in PAUSED state regardless of the > VIR_DOMAIN_START_PAUSED flag. > > While using the LAUNCH_MEASURE and LAUNCH_SECRET operations is expected > to be the common case, they are optional. I would recommend requiring > the upstream software to ex
Re: [libvirt] [Qemu-devel] libvirt/QEMU/SEV interaction
On Fri, Sep 08, 2017 at 06:57:30AM -0500, Brijesh Singh wrote: > Hi All, > > (sorry for the long message) > > CPUs from AMD EPYC family supports Secure Encrypted Virtualization (SEV) > feature - the feature allows running encrypted VMs. To enable the feature, > I have been submitting patches to Linux kernel [1], Qemu [2] and OVMF [3]. > We have been making some good progress in getting patches accepted upstream > in Linux and OVMF trees. SEV builds upon SME (Secure Memory Encryption) > feature -- SME support just got pulled into 4.14 merge window. The base > SEV patches are accepted in OVMF tree -- now we have SEV aware guest BIOS. > I am getting ready to take off "RFC" tag from remaining patches to get them > reviewed and accepted. > > The boot flow for launching an SEV guest is a bit different from a typical > guest launch. In order to launch SEV guest from virt-manager or other > high-level VM management tools, we need to design and implement new > interface between libvirt and qemu, and probably add new APIs in libvirt > to be used by VM management tools. I am new to the libvirt and need some > expert advice while designing this interface. A pictorial representation > for a SEV guest launch flow is available in SEV Spec Appendix A [4]. > > A typical flow looks like this: > > 1. Guest owner (GO) asks the cloud provider to launch SEV guest. > 2. VM tool asks libvirt to provide its Platform Diffie-Hellman (PDH) key. > 3. libvirt opens /dev/sev device to get its PDH and return the blob to the > caller. What sort of size are we talking about for the PDH ? There's a few ways libvirt could report it 1. As an XML element in the host capabilities XML 2. As an XML element in the emulator capabilities XML 3. Via a newly added host API > 4. VM tool gives its PDH to GO. > 5. GO provides its DH key, session-info and guest policy. Are steps 4 & 5 strictly required to be in this order, or is it possible What are the security requirements around the DH key, session info and guest policy ? Are any of them sensitive data which needs to be kept private from untrustworthy users. Also are all three of these items different for every guest launched, or are some of them likely to be the same for every guest ? eg, would the same guest policy blob be used for every guest, with only session info changing ? Also what sort of size are we talking about for each of these data items, KBs, 10's of KB, 100's of KBs or larger ? The security and data size can influence our design approach from the libvirt POV. > 6. VM tool somehow communicates the GO provided information to libvirt. Essentially we have two choices 1. inline in the guest XML config description passed to libvirt when defining the guest XML 2. out of band, ahead of time, via some other API prior to defining the guest XML, which is then referenced in guest XML. THis could be done using the virSecret APIs, where we create 3 secrets, one each for DH key, session-info and guets policy. The UUID of the secret could be specified in the guest XML. This has flexibility of allowing the same secrets ot be used for many guests (if this is valid for SEV) > 7. libvirt adds "sev-guest" object in its xml file with all the information > obtained from #5 > > (currently my xml file looks like this) > > > value='sev-guest,id=sev0,policy=,dh-key-file=,session-file=/> > > > > 8. libvirt launches the guest with "-S" All libvirt guests get launched with -S, to give libvirt chance to do some setup before starting vCPUs. Normally vCPUs are started by default, but the VIR_DOMAIN_START_PAUSED flag allows the mgmt app to tell libvirt to leave vCPUS paused. Alternatively, if libvirt sees presencese of 'sev' config for the guest, it could automatically leave it in PAUSED state regardless of the VIR_DOMAIN_START_PAUSED flag. > 9. While creating the SEV guest qemu does the following > i) create encryption context using GO's DH, session-info and guest policy > (LAUNCH_START) > ii) encrypts the guest bios (LAUNCH_UPDATE_DATA) > iii) calls LAUNCH_MEASUREMENT to get the encrypted bios measurement > 10. By some interface we must propagate the measurement all the way to GO > before libvirt starts the guest. Again, what kind of size data are we talking about for athe "measurement" blob ? a KB, 10's of KB, or more ? My first gut instinct would be for QEMU to emit a QMP event when it has the measurement available. The event could include the actual data blob, or we can could add an explicit QMP command to fetch the data blob. Libvirt could listen for this QEMU event, and in turn emit an event from libvirt with the same data, which the mgmt tool can finally give to the GO. > 11. GO verifies the measurement and if measurement matches then it may > give a secret blob -- which must be injected into the guest before > libvirt starts the VM. If verification failed, GO will request cloud > provider to destroy the VM. So w
Re: [libvirt] [PATCH v3] [libvirt-jenkins-ci] Build on supported Fedora releases (25-26)
On Thu, Sep 07, 2017 at 01:46:03PM +0200, Andrea Bolognani wrote: > On Thu, 2017-09-07 at 11:58 +0100, Daniel P. Berrange wrote: > > > Fedora 26 has been released in the meantime, which means we can > > > get rid of two builders instead of one! Someone will have to > > > prepare the 'libvirt-fedora-26' builder, though, because it > > > doesn't exist at the moment :) > > > > Until someone actually creates the new builders for F26 and > > almost time for F27 now too, I don't think we should really > > be turning off existing builders. > > I posted this in part to raise the issue of the Fedora 26 builder > not being available yet, but I disagree on the fact that we can't > get rid of Fedora 23 and 24 until that's in place. Building on > two unsupported Fedora releases doesn't really buy us anything > except for more load on the already tightly packed CI hosts. I would be ok with killing F23, as that brings us back to the long term load point where we have 2 stable Fedoras tested + rawhide. > > We really badly need someone to write a kickstart file that > > can 100% automate the provisioning of Fedora VMs suitable > > for running our CI. Then we can quickly deploy builers when > > new Fedora comes out and get to point where we're always > > 100% aligned with testing on the 2 current supported releases > > + rawhide. > > I've been working on Ansible playbooks for my development machines > and I've intended from the very beginning to make them generic > enough that I could use them for my own builders too with only > minimal changes. > > Since I'm going to spend time on that regardless, I'm perfectly > fine with coming up with something that can turn a freshly-installed > Fedora guest into a builder suitable for the CentOS CI. > > As for the installation step, I've never used kickstart but I was > thinking of taking advantage of the amazing work the virt-builder > maintainers have been doing in quickly preparing templates after > a new Fedora version has been released. > > How does that sound? Sure, using virt-builder as a starting point is fine too - the key is simply that it be 100% automatable. 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 v3] [libvirt-jenkins-ci] Build on supported Fedora releases (25-26)
On Thu, Sep 07, 2017 at 12:48:22PM +0200, Andrea Bolognani wrote: > Fedora 23 has been out of support for quite a while now, with > Fedora 24 recently joining it with the release of Fedora 26 > which, on the other hand, is fully supported and a prime candidate > for building libvirt. > > Signed-off-by: Andrea Bolognani > --- > Yash first attempted this last December[1], without much luck. > > Fedora 26 has been released in the meantime, which means we can > get rid of two builders instead of one! Someone will have to > prepare the 'libvirt-fedora-26' builder, though, because it > doesn't exist at the moment :) Until someone actually creates the new builders for F26 and almost time for F27 now too, I don't think we should really be turning off existing builders. We really badly need someone to write a kickstart file that can 100% automate the provisioning of Fedora VMs suitable for running our CI. Then we can quickly deploy builers when new Fedora comes out and get to point where we're always 100% aligned with testing on the 2 current supported releases + rawhide. 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 2/2] travis: Install gettext
On Thu, Sep 07, 2017 at 11:29:15AM +0200, Andrea Bolognani wrote: > On Wed, 2017-09-06 at 14:25 +0100, Daniel P. Berrange wrote: > > On Wed, Sep 06, 2017 at 03:13:30PM +0200, Andrea Bolognani wrote: > > > msgmerge(1) and friends are required to build libvirt, so the > > > corresponding package should be installed in the Travis worker. > > > > It is only used if you regenerate .po files. 99% of the time > > developers or apps bulding libvirt won't do that, avoiding any > > need for msgmerge. 'make dist' triggers msgmerge, but we don't > > run that in travis - only make & make check > > Is that so? I tried running > > git clean -xdf && ./autogen.sh && make > > on a Debian guest where the gettext package is not installed, > and sure enough the build failed. > > IIUC *.po files are compiled to *.gmo using msgfmt(1), which is > part of the gettext package, and we need to build those along > with the rest because they are (correctly) not tracked in git. > > If you compare [471.1], the last where the build succeeded, and > [472.1], the first where it failed, you can see that even though > they share the same configuration the gettext tools are found by > configure in the former but not in the latter. > > It looks like Travis updated their 'precise' worker in between > those two builds - it jumped from 2.5.0 to 2.9.3: maybe the new > worker is more minimal and doesn't include the gettext package? > > That would explain why we suddenly need to be explicit about it > instead of getting it for free. Yeah, looks like you are right. 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