Re: [libvirt] [PATCH v2 3/3] conf: Allow users to define UUID for devices

[Daniel P. Berrange]

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 <mpriv...@redhat.com>
> > > > ---
> > > >
> > > > 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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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, , ) < 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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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 

Re: [libvirt] [PATCH go-xml] qemu: extend serial console source

[Daniel P. Berrange]

On Mon, Oct 02, 2017 at 11:34:18AM +, Jeroen Simonetti wrote:
> October 2 2017 12:47 PM, "Daniel P. Berrange" <berra...@redhat.com> 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

[Daniel P. Berrange]

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: {
> + Source: {
>   Path:   
> "/tmp/serial.log",
>   Append: "off",
> + SecLabel: 
> {
> + Model:   "dac",
> + Relabel: "no",
> + },
> + },
> + Target: {
> + Port: ,
> + },
> + },
> + DomainSerial{
> + Type: "tcp",
> + Source: {
> + Mode:"bind",
> + Host:"127.0.0.1",
> + Service: "1234",
> + TLS: "yes",
> + },
> + Protocol: {
> + Type: "telnet",
>   },
>   Target: {
>   Port: ,
> @@ -382,7 +401,14 @@ var domainTestData = []struct {
>   `  `,
>   ``,
>   ``,
> - `   append="off">`,
> + `  `,
> + ` relabel="no">`,
> + `  `,
> + `  

Re: [libvirt] [Qemu-devel] libvirt/QEMU/SEV interaction

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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 <berra...@redhat.com>

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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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 <berra...@redhat.com>
---

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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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)

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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 <nir...@gmail.com>
> ---
>  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 <berra...@redhat.com>

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

[Daniel P. Berrange]

On Tue, Sep 26, 2017 at 09:30:17PM +0200, Jeroen Simonetti wrote:
> Signed-off-by: Jeroen Simonetti <jer...@simonetti.nl>
> ---
>  domain.go  | 22 ++
>  domain_test.go | 41 +
>  2 files changed, 55 insertions(+), 8 deletions(-)

Reviewed-by: Daniel P. Berrange <berra...@redhat.com>

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.

[Daniel P. Berrange]

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 <zhenwei...@youruncloud.com>
> ---
>  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: {
> + Type: "file|anonymous",
> + },
> + MemoryAccess: {
> + Mode: "shared|private",
> + },
> + MemoryAllocation: {
> + 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 <berra...@redhat.com>


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

[Daniel P. Berrange]

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 <berra...@redhat.com>
> 
> 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

[Daniel P. Berrange]

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, >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 

[libvirt] [PATCH python] Fix comparisons between signed & unsigned integers

[Daniel P. Berrange]

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 <berra...@redhat.com>
---
 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",
   _domain, ))
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

Re: [libvirt] [PATCH v2 0/6] libvirt-python/libvirtaio patches for 3.8.0

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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 <w...@invisiblethingslab.com>
> > > ---
> (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 <berra...@redhat.com>



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

[Daniel P. Berrange]

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 <berra...@redhat.com>
---
 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

[Daniel P. Berrange]

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 <w...@invisiblethingslab.com>
> > > ---
> > >  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 <berra...@redhat.com>


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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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 <w...@invisiblethingslab.com>
> ---
>  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 <berra...@redhat.com>


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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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 <w...@invisiblethingslab.com>
> ---
>  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 <berra...@redhat.com>


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

[Daniel P. Berrange]

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 <berra...@redhat.com>
> > ---
> >  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  |  12 

Re: [libvirt] [PATCH v2 0/6] libvirt-python/libvirtaio patches for 3.8.0

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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 <berra...@redhat.com>
> > ---
> 
> 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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

On Mon, Sep 25, 2017 at 04:21:14PM +0800, zhenwei.pi wrote:
> Signed-off-by: zhenwei.pi <zhenwei...@youruncloud.com>
> ---
>  domain.go  | 12 
>  domain_test.go | 24 
>  2 files changed, 36 insertions(+)

Reviewed-by: Daniel P. Berrange <berra...@redhat.com>

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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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 <mpriv...@redhat.com>
> > >>> ---
> > >>>  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

[Daniel P. Berrange]

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;

[libvirt] [PATCH] Fix commandhelper build on win32

[Daniel P. Berrange]

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 <berra...@redhat.com>
---

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

[Daniel P. Berrange]

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 <berra...@redhat.com>


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

[Daniel P. Berrange]

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 <nshirokovs...@virtuozzo.com>
Signed-off-by: Daniel P. Berrange <berra...@redhat.com>
---
 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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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(, 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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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 <mklet...@redhat.com>
>   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 <berra...@redhat.com>
> ---

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

[libvirt] [PATCH] Stop linking tests/commandhelper to libvirt code

[Daniel P. Berrange]

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 <mklet...@redhat.com>
  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 <berra...@redhat.com>
---
 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 vo

Re: [libvirt] [PATCH] Link libvirt_util.la with gnutls

[Daniel P. Berrange]

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 <berra...@redhat.com>
> > ---
> > 
> > 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

[Daniel P. Berrange]

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 <berra...@redhat.com>
---

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"

[Daniel P. Berrange]

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 <abolo...@redhat.com>
> ---
>  .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 <berra...@redhat.com>

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

[Daniel P. Berrange]

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 <berra...@redhat.com>
---
 .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 b

Re: [libvirt] [PATCH python] Add travis build config

[Daniel P. Berrange]

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 <berra...@redhat.com>
> > ---
> > .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

[Daniel P. Berrange]

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 <berra...@redhat.com>
---
 .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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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 <berra...@redhat.com>
> > ---
> >  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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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 <berra...@redhat.com>

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

[Daniel P. Berrange]

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 <berra...@redhat.com>


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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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ä <ville.sky...@iki.fi>
> Signed-off-by: Cole Robinson <crobi...@redhat.com>
> ---
>  libvirt.spec.in | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrange <berra...@redhat.com>


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

[Daniel P. Berrange]

On Thu, Sep 14, 2017 at 05:43:06PM -0400, Cole Robinson wrote:
> From: Ville Skyttä <ville.sky...@iki.fi>
> 
> 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 <crobi...@redhat.com>
> ---
>  libvirt.spec.in | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Daniel P. Berrange <berra...@redhat.com>


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

[Daniel P. Berrange]

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 <berra...@redhat.com>

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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

On Thu, Sep 14, 2017 at 03:09:05PM +0200, Andrea Bolognani wrote:
> Make single commands OS-dependent instead.
> 
> Signed-off-by: Andrea Bolognani <abolo...@redhat.com>
> ---
>  .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 <berra...@redhat.com>


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

[Daniel P. Berrange]

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 <abolo...@redhat.com>
> ---
>  .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 <berra...@redhat.com>

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

[Daniel P. Berrange]

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 <abolo...@redhat.com>
> ---
>  .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 <berra...@redhat.com>

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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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 <abolo...@redhat.com>
> ---
>  .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 <berra...@redhat.com>

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

[Daniel P. Berrange]

On Thu, Sep 14, 2017 at 03:09:02PM +0200, Andrea Bolognani wrote:
> Signed-off-by: Andrea Bolognani <abolo...@redhat.com>
> ---
>  .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 <berra...@redhat.com>

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

[Daniel P. Berrange]

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 <abolo...@redhat.com>
> ---
>  .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 <berra...@redhat.com>


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

[Daniel P. Berrange]

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 <mpriv...@redhat.com>
> >> ---
> >>  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

[Daniel P. Berrange]

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 <berra...@redhat.com>
---
 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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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

Re: [libvirt] New QEMU daemon for persistent reservations

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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" <berra...@redhat.com> 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 sof

Re: [libvirt] [Qemu-devel] libvirt/QEMU/SEV interaction

[Daniel P. Berrange]

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 

Re: [libvirt] [PATCH v3] [libvirt-jenkins-ci] Build on supported Fedora releases (25-26)

[Daniel P. Berrange]

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)

[Daniel P. Berrange]

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

[Daniel P. Berrange]

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

Re: [libvirt] [PATCH 2/2] travis: Install gettext

[Daniel P. Berrange]

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


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