Re: [libvirt] [PATCH v4 0/8] Virtio-crypto device support

[Longpeng (Mike)]

On 2017/10/25 23:14, Matthew Rosato wrote:

> On 07/07/2017 04:07 AM, Longpeng(Mike) wrote:
>> As virtio-crypto has been supported in QEMU 2.8 and the frontend
>> driver has been merged in linux 4.10, so it's necessary to support
>> virtio-crypto in libvirt.
>>
>> ---
> 
> Hi Mike,
> 
> Seems like this topic has gone quiet..  Is there a v5 in the works?
> 


Hi Matt,

V5 is always in our plan, but we want to make the virtio-crypto spec (the latest
version is V20) upstream first.

I mainly work on an amazing and interesting project these two weeks, so even the
virtio-crypto spec is delayed.

I'll take some time to work on the V21 spec these days.

> Matt
> 
> 
> 


-- 
Regards,
Longpeng(Mike)

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [[RFC] 0/8] Implement async QEMU event handling in libvirtd.

[Prerna]

On Wed, Oct 25, 2017 at 4:12 PM, Jiri Denemark  wrote:

> On Tue, Oct 24, 2017 at 10:34:53 -0700, Prerna Saxena wrote:
> >
> > As noted in
> > https://www.redhat.com/archives/libvir-list/2017-May/msg00016.html
> > libvirt-QEMU driver handles all async events from the main loop.
> > Each event handling needs the per-VM lock to make forward progress. In
> > the case where an async event is received for the same VM which has an
> > RPC running, the main loop is held up contending for the same lock.
> >
> > This impacts scalability, and should be addressed on priority.
> >
> > Note that libvirt does have a 2-step deferred handling for a few event
> > categories, but (1) That is insufficient since blockign happens before
> > the handler could disambiguate which one needs to be posted to this
> > other queue.
> > (2) There needs to be homogeniety.
> >
> > The current series builds a framework for recording and handling VM
> > events.
> > It initializes per-VM event queue, and a global event queue pointing to
> > events from all the VMs. Event handling is staggered in 2 stages:
> > - When an event is received, it is enqueued in the per-VM queue as well
> >   as the global queues.
> > - The global queue is built into the QEMU Driver as a threadpool
> >   (currently with a single thread).
> > - Enqueuing of a new event triggers the global event worker thread, which
> >   then attempts to take a lock for this event's VM.
> > - If the lock is available, the event worker runs the function
> handling
> >   this event type. Once done, it dequeues this event from the global
> >   as well as per-VM queues.
> > - If the lock is unavailable(ie taken by RPC thread), the event
> worker
> >   thread leaves this as-is and picks up the next event.
>
> If I get it right, the event is either processed immediately when its VM
> object is unlocked or it has to wait until the current job running on
> the VM object finishes even though the lock may be released before that.
> Correct? If so, this needs to be addressed.
>

In most cases, the lock is released just before we end the API. However, it
is a small change that can be made.

>
> > - Once the RPC thread completes, it looks for events pertaining to the
> >   VM in the per-VM event queue. It then processes the events serially
> >   (holding the VM lock) until there are no more events remaining for
> >   this VM. At this point, the per-VM lock is relinquished.
> >
> > Patch Series status:
> > Strictly RFC only. No compilation issues. I have not had a chance to
> > (stress) test it after rebase to latest master.
> > Note that documentation and test coverage is TBD, since a few open
> > points remain.
> >
> > Known issues/ caveats:
> > - RPC handling time will become non-deterministic.
> > - An event will only be "notified" to a client once the RPC for same VM
> completes.
> > - Needs careful consideration in all cases where a QMP event is used to
> >   "signal" an RPC thread, else will deadlock.
>
> This last issue is actually a show stopper here. We need to make sure
> QMP events are processed while a job is still active on the same domain.
> Otherwise thinks kile block jobs and migration, which are long running
> jobs driven by events, will break.
>
> Jirka
>
Completely agree, which is why I have explicitly mentioned this.
However, I do not completely follow why it needs to be this way. Can the
block job APIs between QEMU <--> libvirt be fixed so that such behaviour is
avoided ?

Regards,
Prerna
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] [PATCH/QEMU] s390x/kvm: use cpu_model_available for guarded storage on compat machines

[Halil Pasic]

On 10/20/2017 04:54 PM, Christian Borntraeger wrote:
> Starting a guest with
>
> hvm
>   
>   
> 
> on an IBM z14 results in
> 
> "qemu-system-s390x: Some features requested in the CPU model are not
> available in the configuration: gs"
> 
> This is because guarded storage is fenced for compat machines that did not 
> have
> guarded storage support, but libvirt expands the cpu model according to the
> latest available machine.
> 
> While this prevents future migration abort (by not starting the guest at all),
> not being able to start a "host-model" guest is very much unexpected.  As it
> turns out, even if we would modify libvirt to not expand the cpu model to
> contain "gs" for compat machines, it cannot guarantee that a migration will
> succeed. For example if the kernel changes its features (or the user has
> nested=1 on one host but not on the other) the migration will fail
> nevertheless.  So instead of fencing "gs" for machines <= 2.9 lets allow it 
> for
> all machine types that support the CPU model. This will make "host-model"
> runnable all the time, while relying on the CPU model to reject invalid
> migration attempts.
> 
> Suggested-by: David Hildenbrand 
> Signed-off-by: Christian Borntraeger 

I've tried to review this patch. Unfortunately I don't have access to a
z14, so I could not try the most interesting scenarios out.

The idea of the patch is very clear, but I don't understand the bigger gs
feature context fully.

>From what I read in the code, the attempt to enable the gs capability in
the kernel is made regardless of the cpu model. If the attempt was
successful kvm_s390_get_gs will keep returning true. That would in turn
affect migration, as far as I see (usages of kvm_s390_get_gs). I could
not figure out how does gs being turned off via cpu-model (-cpu
z14,gs=off) does turn of gs support -- at least not the details. I wanted
to give a timely review, so I've limited myself there. 

>From what I see, this patch does what it advertises, and since I think
it's the right thing to do in the current situation I gonna give it an:
Acked-by: Halil Pasic 

At the same time, I would prefer the commit message being reworded. IMHO
this patch is a good stop-gap measure, but essentially it trades an
annoying and obvious bug for a subtle and hopefully painless one.

Let me explain this last statement. For starters, I  do share some of the
concerns Boris has voiced.  I won't repeat those. Same goes for the
example Christian paraphrased previously, and the the fear of an implicit
requirement for having to support a Cartesian product of the advertised
machine-types and cpu-models (for each qemu binary).

In my eyes, a cpu isn't all that different from the other devices which
get attached to a board (represented by machine-type). So I don't see why
should it be exempt from the usual compatibility requirements tied to
machine-types (for the sake of stability and compatibility). (I basically
mean: no new features are added to a device in the context of a given
(fully qualified) machine-type (with new QEMU -- binary -- versions). As
far as I understand all (other) devices shall respect these requirements.
Or am I wrong? If I'm not, please enlighten me, how is a CPU
fundamentally different than let's say a FLIC.

A related thing is, that to implement some features indicated/controlled
via cpu-model features, we need to add capabilities to certain devices.
Now if the devices shall obey the 'no new features for the same
machine-type' rule, but the cpu-model feature shall obey our new
'retroactively introduce/enable for all machines supporting cpu-models'
rule, I think we have a conflict.  An example for what I'm talking about
is zPCI, AIS and FLIC. In case of the AIS and the FLIC, AFAIK the
conflict was resolved so that the AIS feature/code of the FLIC is not
subject to usual compat-macro mechanism. Another example, AP facility in
not just about the CPU instructions, but also about a device.

It's also true for the last paragraph: I might very well be wrong, and if
I am please do tell (where is the hole in  my reasoning). I will try to
re-check my statements tomorrow -- again trying to deliver today along
the lines better a small bird today than a big one tomorrow).

And another question. If we adopt this introducing features for
machine-types retroactively, how should the machine-type versions be
understood like? My current understanding is, that the machine-type
(version) is supposed to limit the observable changes when upgrading
the binary to the bare minimum (basically possibly modified timings -- which
can't be avoided -- and bug-fixes) for the sake of updating the
binary being as safe as possible.

Last but not least, I have to say, I'm neither an domain expert for
cpu-models nor for libvirt and it's models. For that reason, I've
personally asked Jason to do a more detailed review on this -- and am
hoping to wiggle out with 

Re: [libvirt] [PATCH v5 1/4] numa: describe siblings distances within cells

[Jim Fehlig]

On 10/12/2017 01:31 PM, Wim Ten Have wrote:

From: Wim ten Have 

Add support for describing NUMA distances in a domain's  
XML description.

Below is an example of a 4 node setup:

   
 
   
 
   
   
   
   
 
   
   
 
   
   
   
   
 
   
   
 
   
   
   
   
 
   
 
   
   
   
   
 
   
 
   

A  defines a NUMA node.  describes the NUMA distance
from the  to the other NUMA nodes (the s).  For example,
in above XML description, the distance between NUMA node0  and NUMA node2  is 31.

Valid distance value are '10 <= value <= 255'.  A distance value of 10
represents the distance to the node itself.  A distance value of 20
represents the default value for remote nodes but other values are
possible depending on the physical topology of the system.

When distances are not fully described, any missing sibling distance
values will default to 10 for local nodes and 20 for remote nodes.

Signed-off-by: Wim ten Have 
---
Changes on v1:
- Add changes to docs/formatdomain.html.in describing schema update.
Changes on v2:
- Automatically apply distance symmetry maintaining cell <-> sibling.
- Check for maximum '255' on numaDistanceValue.
- Automatically complete empty distance ranges.
- Check that sibling_id's are in range with cell identifiers.
- Allow non-contiguous ranges, starting from any node id.
- Respect parameters as ATTRIBUTE_NONNULL fix functions and callers.
- Add and apply topology for LOCAL_DISTANCE=10 and REMOTE_DISTANCE=20.
Changes on v3
- Add UNREACHABLE if one locality is unreachable from another.
- Add code cleanup aligning function naming in a separated patch.
- Add numa related driver code in a separated patch.
- Remove  from numaDistanceValue schema/basictypes.rng
- Correct doc changes.
Changes on v4
- Fix symmetry error under virDomainNumaDefNodeDistanceParseXML()
---
  docs/formatdomain.html.in   |  63 -
  docs/schemas/basictypes.rng |   7 ++
  docs/schemas/cputypes.rng   |  18 
  src/conf/numa_conf.c| 221 +++-
  4 files changed, 305 insertions(+), 4 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index c0e3c2221..ab2a89c6a 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1529,7 +1529,68 @@
  
  
  

-  This guest NUMA specification is currently available only for QEMU/KVM.
+  This guest NUMA specification is currently available only for
+  QEMU/KVM and Xen.  Whereas Xen driver also allows for a distinct
+  description of NUMA arranged sibling cell
+  distances Since 3.8.0.


Now 3.9.0.


+
+
+
+  Under NUMA h/w architecture, distinct resources such as memory


For the website docs, I think it is better to write "hardware" instead of "h/w".


+  create a designated distance between cell and
+  siblings that now can be described with the help of
+  distances. A detailed description can be found within
+  the ACPI (Advanced Configuration and Power Interface Specification)
+  within the chapter explaining the system's SLIT (System Locality
+  Distance Information Table).
+
+
+
+...
+cpu
+  ...
+  numa
+cell id='0' cpus='0,4-7' memory='512000' unit='KiB'
+  distances
+sibling id='0' value='10'/
+sibling id='1' value='21'/
+sibling id='2' value='31'/
+sibling id='3' value='41'/
+  /distances
+/cell
+cell id='1' cpus='1,8-10,12-15' memory='512000' unit='KiB' 
memAccess='shared'
+  distances
+sibling id='0' value='21'/
+sibling id='1' value='10'/
+sibling id='2' value='21'/
+sibling id='3' value='31'/
+  /distances
+/cell
+cell id='2' cpus='2,11' memory='512000' unit='KiB' 
memAccess='shared'
+  distances
+sibling id='0' value='31'/
+sibling id='1' value='21'/
+sibling id='2' value='10'/
+sibling id='3' value='21'/
+  /distances
+/cell
+cell id='3' cpus='3' memory='512000' unit='KiB'
+  distances
+sibling id='0' value='41'/
+sibling id='1' value='31'/
+sibling id='2' value='21'/
+sibling id='3' value='10'/
+  /distances
+/cell
+  /numa
+  ...
+/cpu
+...
+
+
+  Under Xen driver, if no distances are given to describe
+  the SLIT data between different cells, it will default to a scheme
+  using 10 for local and 20 for remote distances.
  
  
  Events configuration

diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng
index 1ea667cdf..1a18cd31b 100644
--- a/docs/schemas/basictypes.rng
+++ b/docs/schemas/basictypes.rng
@@ -77,6 +77,13 @@
  

  
+  

+
+  10
+  255
+
+  
+

  
  

Re: [libvirt] [PATCH v2 3/3] vbox: Read runtime RDP port and handle autoport

[Dawid Zamirski]

On Wed, 2017-10-25 at 17:35 -0400, John Ferlan wrote:
> 
> On 10/24/2017 05:09 PM, Dawid Zamirski wrote:
> > VirutalBox has a IVRDEServerInfo structure available that
> > gives the effective runtime port that the VM is using when it's
> > running. This is useful when the "TCP/Ports" VBox property was set
> > to
> > port range (e.g. via autoport = "yes" or via VBoxManage) in which
> > case it would be impossible to get the "active" port otherwise.
> > ---
> >  src/vbox/vbox_common.c|   3 +-
> >  src/vbox/vbox_tmpl.c  | 134
> > +++---
> >  src/vbox/vbox_uniformed_api.h |   2 +-
> >  3 files changed, 104 insertions(+), 35 deletions(-)
> > 
> > diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
> > index 92ee37164..d542f2b76 100644
> > --- a/src/vbox/vbox_common.c
> > +++ b/src/vbox/vbox_common.c
> > @@ -3326,7 +3326,8 @@ vboxDumpDisplay(virDomainDefPtr def,
> > vboxDriverPtr data, IMachine *machine)
> >  if (VIR_ALLOC(graphics) < 0)
> >  goto cleanup;
> >  
> > -gVBoxAPI.UIVRDEServer.GetPorts(data, VRDEServer,
> > graphics);
> > +gVBoxAPI.UIVRDEServer.GetPorts(data, VRDEServer, machine,
> > graphics);
> > +gVBoxAPI.UISession.Close(data->vboxSession);
> 
> But @data is used shortly after this and I don't see in the calling
> tree
> a corresponding UISession.Open* of some type or am I missing it in
> some
> called function?
> 
> 
> The rest looks good - just need to know about this.  I can remove
> before
> pushing or make some other sort of simple adjustment.

Yep this should be removed - it's a left over from my old internal
patch [1], that I forgot to remove when preparing for upstream
submission. It was originally preceded with OpenExisting (aka
LockMachine) to get the IConsole - the new patch does it internally in
the vboxGetActiveVRDEServerPort function.

https://github.com/datto/libvirt/commit/a3cb830bfce10b1a614c18a6ac50783
45433d900#diff-747d3af65e7ac81a564b7cb4fcd01eb6R3516

Thank you,
Dawid

> 
> John
> 
> (I'm at KVM Forum in Prague - so normal work schedule is a bit off)
> 
> >  
> > 
> > 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 1/3] vbox: Remove old unflexible macros

[John Ferlan]

On 10/24/2017 05:09 PM, Dawid Zamirski wrote:
> The VBOX_SESSION_OPEN/CLOSE macros are only called in
> _vboxDomainSnapshotRestore and they are unflexible because:
> 
> * assume the caller will have variable named "data"
> * can only create Write lock type
> 
> As per above, it's not that hard to simply use the VBOX API directly.
> ---
>  src/vbox/vbox_tmpl.c | 10 ++
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 

Reviewed-by: John Ferlan 

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 2/3] vbox: Make autoport set RDP port range.

[John Ferlan]

On 10/24/2017 05:09 PM, Dawid Zamirski wrote:
> From: Dawid Zamirski 
> 
> Originally autoport in vbox driver was setting the port to default value
> (3389) which caused multiple VM instances use the same port. Since
> libvirt XML does not allow to set port ranges, this patch changes the
> "autoport" behavior to set VBox's "TCP/Ports" property to an arbitrary
> port range (3389-3689) to avoid that issue.
> ---
>  src/vbox/vbox_tmpl.c | 15 ---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 

Reviewed-by: John Ferlan 

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 3/3] vbox: Read runtime RDP port and handle autoport

[John Ferlan]

On 10/24/2017 05:09 PM, Dawid Zamirski wrote:
> VirutalBox has a IVRDEServerInfo structure available that
> gives the effective runtime port that the VM is using when it's
> running. This is useful when the "TCP/Ports" VBox property was set to
> port range (e.g. via autoport = "yes" or via VBoxManage) in which
> case it would be impossible to get the "active" port otherwise.
> ---
>  src/vbox/vbox_common.c|   3 +-
>  src/vbox/vbox_tmpl.c  | 134 
> +++---
>  src/vbox/vbox_uniformed_api.h |   2 +-
>  3 files changed, 104 insertions(+), 35 deletions(-)
> 
> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
> index 92ee37164..d542f2b76 100644
> --- a/src/vbox/vbox_common.c
> +++ b/src/vbox/vbox_common.c
> @@ -3326,7 +3326,8 @@ vboxDumpDisplay(virDomainDefPtr def, vboxDriverPtr 
> data, IMachine *machine)
>  if (VIR_ALLOC(graphics) < 0)
>  goto cleanup;
>  
> -gVBoxAPI.UIVRDEServer.GetPorts(data, VRDEServer, graphics);
> +gVBoxAPI.UIVRDEServer.GetPorts(data, VRDEServer, machine, graphics);
> +gVBoxAPI.UISession.Close(data->vboxSession);

But @data is used shortly after this and I don't see in the calling tree
a corresponding UISession.Open* of some type or am I missing it in some
called function?


The rest looks good - just need to know about this.  I can remove before
pushing or make some other sort of simple adjustment.

John

(I'm at KVM Forum in Prague - so normal work schedule is a bit off)

>  
>  graphics->type = VIR_DOMAIN_GRAPHICS_TYPE_RDP;
>  
> diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
> index 2b3f2e3eb..c7682f13c 100644
> --- a/src/vbox/vbox_tmpl.c
> +++ b/src/vbox/vbox_tmpl.c
> @@ -221,29 +221,6 @@ _vboxIIDFromArrayItem(vboxDriverPtr data, vboxIID *iid,
>  _vboxIIDFromArrayItem(data, iid, array, idx)
>  #define DEBUGIID(msg, strUtf16) DEBUGPRUnichar(msg, strUtf16)
>  
> -/**
> - * Converts Utf-16 string to int
> - */
> -static int PRUnicharToInt(PCVBOXXPCOM pFuncs, PRUnichar *strUtf16)
> -{
> -char *strUtf8 = NULL;
> -int ret = 0;
> -
> -if (!strUtf16)
> -return -1;
> -
> -pFuncs->pfnUtf16ToUtf8(strUtf16, );
> -if (!strUtf8)
> -return -1;
> -
> -if (virStrToLong_i(strUtf8, NULL, 10, ) < 0)
> -ret = -1;
> -
> -pFuncs->pfnUtf8Free(strUtf8);
> -
> -return ret;
> -}
> -
>  /**
>   * Converts int to Utf-16 string
>   */
> @@ -280,6 +257,54 @@ static virDomainState _vboxConvertState(PRUint32 state)
>  }
>  }
>  
> +
> +static int
> +vboxGetActiveVRDEServerPort(ISession *session, IMachine *machine)
> +{
> +nsresult rc;
> +PRInt32 port = -1;
> +IVRDEServerInfo *vrdeInfo = NULL;
> +IConsole *console = NULL;
> +
> +rc = machine->vtbl->LockMachine(machine, session, LockType_Shared);
> +if (NS_FAILED(rc)) {
> +VIR_WARN("Could not obtain shared lock on VBox VM, rc=%08x", rc);
> +return -1;
> +}
> +
> +rc = session->vtbl->GetConsole(session, );
> +if (NS_FAILED(rc)) {
> +VIR_WARN("Could not get VBox session console, rc=%08x", rc);
> +goto cleanup;
> +}
> +
> +/* it may be null if VM is not running */
> +if (!console)
> +goto cleanup;
> +
> +rc = console->vtbl->GetVRDEServerInfo(console, );
> +
> +if (NS_FAILED(rc) || !vrdeInfo) {
> +VIR_WARN("Could not get VBox VM VRDEServerInfo, rc=%08x", rc);
> +goto cleanup;
> +}
> +
> +rc = vrdeInfo->vtbl->GetPort(vrdeInfo, );
> +
> +if (NS_FAILED(rc)) {
> +VIR_WARN("Could not read port from VRDEServerInfo, rc=%08x", rc);
> +goto cleanup;
> +}
> +
> + cleanup:
> +VBOX_RELEASE(console);
> +VBOX_RELEASE(vrdeInfo);
> +session->vtbl->UnlockMachine(session);
> +
> +return port;
> +}
> +
> +
>  static int
>  _vboxDomainSnapshotRestore(virDomainPtr dom,
>IMachine *machine,
> @@ -1576,24 +1601,67 @@ _vrdeServerSetEnabled(IVRDEServer *VRDEServer, PRBool 
> enabled)
>  }
>  
>  static nsresult
> -_vrdeServerGetPorts(vboxDriverPtr data ATTRIBUTE_UNUSED,
> -IVRDEServer *VRDEServer, virDomainGraphicsDefPtr 
> graphics)
> +_vrdeServerGetPorts(vboxDriverPtr data, IVRDEServer *VRDEServer,
> +IMachine *machine, virDomainGraphicsDefPtr graphics)
>  {
>  nsresult rc;
>  PRUnichar *VRDEPortsKey = NULL;
>  PRUnichar *VRDEPortsValue = NULL;
> +PRInt32 port = -1;
> +ssize_t nmatches = 0;
> +char **matches = NULL;
> +char *portUtf8 = NULL;
> +
> +/* get active (effective) port - available only when VM is running and 
> has
> + * the VBOX extensions installed (without extenstions RDP server
> + * functionality is disabled)
> + */
> +port = vboxGetActiveVRDEServerPort(data->vboxSession, machine);
>  
> +if (port > 0)
> +graphics->data.rdp.port = port;
> +
> +/* get the port (or port range) set in VM properties, this 

[libvirt] [PATCH] qemu: logrotate: drop minsize directive

[Jim Fehlig]

On a cloud host it is possible to create 100's of unique instances
per day, each leaving behind a /var/log/libvirt/qemu/instance-name.log
file that is < 100k. With the current 'minsize 100k' directive, these
files are never rotated and hence never removed. Over months of time,
tens of thousands of these files can accumulate on the host.

Dropping 'minsize 100k' allows rotating small files, which will
increase the number of log files, but 'rotate 4' ensures they will
be removed after a month.

Signed-off-by: Jim Fehlig 
---
 daemon/libvirtd.qemu.logrotate.in | 1 -
 1 file changed, 1 deletion(-)

diff --git a/daemon/libvirtd.qemu.logrotate.in 
b/daemon/libvirtd.qemu.logrotate.in
index 15cf019b2..cdb399ef2 100644
--- a/daemon/libvirtd.qemu.logrotate.in
+++ b/daemon/libvirtd.qemu.logrotate.in
@@ -5,5 +5,4 @@
 compress
 delaycompress
 copytruncate
-minsize 100k
 }
-- 
2.14.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] Libvirt xl to xml converter only picks up first occurrence of an option

[Jim Fehlig]

On 10/20/2017 08:46 AM, Wei Liu wrote:

Hi Jim


Hi Wei,

Sorry for the delay. Catching up on mail after some days off...


I discovered that libvirt's native config file to xml converter for
libxl only pick up the first occurrence of an option.

For example in a xl cfg file:

extra = "abc"
...
extra = "def"

xl will pick up "def" because that shows up later and takes precedence,
while the converter picks up "abc".

I think this is a bug in the converter and should be fixed.


Thanks for the report. I took a quick peek at libvirt's generic config parser 
and afaict it only searches for the first occurrence of a setting. The parser 
does support flags though, so we could add something like 
VIR_CONF_FLAG_{FIRST,LAST}_DUPLICATE. (Better name suggestions welcomed.)


I've opened a bug report against openSUSE Factory to track this

https://bugzilla.opensuse.org/show_bug.cgi?id=1065118

Regards,
Jim




Thanks
Wei.



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH go-xml] Add bootp option to NetworkDHCP

[Michal Rostecki]

From: Michal Rostecki 

Support specyfing concrete file and TFTP server
for PXE boot.

Signed-off-by: Michal Rostecki 
---
 network.go  | 6 ++
 network_test.go | 7 +++
 2 files changed, 13 insertions(+)

diff --git a/network.go b/network.go
index 32f125a..10c4dfc 100644
--- a/network.go
+++ b/network.go
@@ -78,9 +78,15 @@ type NetworkDHCPHost struct {
IP   string `xml:"ip,attr,omitempty"`
 }
 
+type NetworkBootp struct {
+   File   string `xml:"file,attr,omitempty"`
+   Server string `xml:"server,attr,omitempty"`
+}
+
 type NetworkDHCP struct {
Ranges []NetworkDHCPRange `xml:"range"`
Hosts  []NetworkDHCPHost  `xml:"host"`
+   Bootp  []NetworkBootp `xml:"bootp"`
 }
 
 type NetworkIP struct {
diff --git a/network_test.go b/network_test.go
index b1bd168..3efb278 100644
--- a/network_test.go
+++ b/network_test.go
@@ -100,6 +100,12 @@ var networkTestData = []struct {
IP:   
"192.168.122.10",
},
},
+   Bootp: []NetworkBootp{
+   NetworkBootp{
+   File:   
"pxelinux.0",
+   Server: 
"192.168.122.1",
+   },
+   },
},
},
NetworkIP{
@@ -179,6 +185,7 @@ var networkTestData = []struct {
``,
`  `,
`  `,
+   `  `,
``,
`  `,
`  `,
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/4] virt-aa-helper: fix paths for usb hostdevs

[Jamie Strandboge]

On Tue, 2017-10-17 at 09:04 +0200, Christian Ehrhardt wrote:
> On Fri, Sep 29, 2017 at 4:58 PM, Michal Privoznik  m>
> wrote:
> 
> > On 09/20/2017 04:59 PM, Christian Ehrhardt wrote:
> > > If users only specified vendor (the common case) then
> > > parsing
> > > the xml via virDomainHostdevSubsysUSBDefParseXML would only set
> > > these.
> > > Bus and Device would much later be added when the devices are
> > > prepared
> > > to be added.
> > > 
> > > Due to that a hot-add of a usb hostdev works as the device is
> > > prepared
> > > and virt-aa-helper processes the new internal xml. But on an
> > > initial
> > > guest start at the time virt-aa-helper renders the apparmor rules
> > > the
> > > bus/device id's are not set yet:
> > > 
> > > p ctl->def->hostdevs[0]->source.subsys.u.usb
> > > $12 = {autoAddress = false, bus = 0, device = 0, vendor = 1921,
> > > product
> > > = 21888}
> > > 
> > > That causes rules to be wrong:
> > >   "/dev/bus/usb/000/000" rw,
> > > 
> > > The fix calls virHostdevFindUSBDevice after reading the XML from
> > > irt-aa-helper to only add apparmor rules for devices that could
> > > be found
> > > and now are fully known to be able to write the rule correctly.
> > > 
> > > It uncondtionally sets virHostdevFindUSBDevice mandatory
> > > attribute as
> > > adding an apparmor rule for a device not found makes no sense no
> > > matter
> > > what startup policy it has set.
> > > 
> > > Signed-off-by: Christian Ehrhardt  > > om>
> > > ---
> > >  src/security/virt-aa-helper.c | 4 
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/src/security/virt-aa-helper.c
> > 
> > b/src/security/virt-aa-helper.c
> > > index 7944dc1..d1518ea 100644
> > > --- a/src/security/virt-aa-helper.c
> > > +++ b/src/security/virt-aa-helper.c
> > > @@ -55,6 +55,7 @@
> > >  #include "virrandom.h"
> > >  #include "virstring.h"
> > >  #include "virgettext.h"
> > > +#include "virhostdev.h"
> > > 
> > >  #include "storage/storage_source.h"
> > > 
> > > @@ -1069,6 +1070,9 @@ get_files(vahControl * ctl)
> > >  if (usb == NULL)
> > >  continue;
> > > 
> > > +if (virHostdevFindUSBDevice(dev, true, ) <
> > > 0)
> > > +continue;
> > > +
> > 
> > Shouldn't we rather fail in this case? Or, what happens if
> > startupPolicy
> > of the device is set to 'optional'? I think we need to error out
> > here
> > (although, we've probably errored out earlier in the process).
> > 
> 
> Hi,
> sorry for the late reply, but I was finally getting some time off for
> a few
> days.
> I intentionally decided not to error out to avoid a new "source" of
> issues.
> Compare the two options we have:
> 1. continue if not finding the device
>   1.1 likely case we found it, rule will be correct - good
>   1.2 we don't find it (for whatever reason) - we are "as bad" as
> before
> this fix, but not worse
> 2. error out if not finding the device
>   2.1 likely case we found it, rule will be correct - good
>   2.2 we don't find it (for whatever reason) - we are now failing
> completely
> 
> What I don't like about 2.2 is that there might be cases things would
> have
> been kind of ok, depsite whatever dark usb magic hit some special
> setup.
> In those cases if we error out we add a new chance to fail.
> And as there are often too many unknowns, so I chose the safer
> option.
> 
I agree with your assessment and simply continuing if not found rather
than erroring. Patch LGTM. +1

Thanks for chasing this one down.

-- 
Jamie Strandboge | http://www.canonical.com

signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/4] virt-aa-helper: allow spaces in vm names

[Jamie Strandboge]

On Wed, 2017-09-20 at 16:59 +0200, Christian Ehrhardt wrote:
> libvirt allows spaces in vm names, there were issues in the past but
> it
> seems not removed so the assumption has to be that spaces are
> continuing
> to be allowed.
> 
> Therefore virt-aa-helper should not reject spaces in vm names anymore
> if
> it is goign to be refused causing issues then the parser or xml
> schema
> should do so.
> Apparmor rules are in quotes, so a space in a path based on the name
> works.
> 
> Signed-off-by: Christian Ehrhardt 
> ---
>  src/security/virt-aa-helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-
> helper.c
> index d1518ea..5f4519d 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -449,7 +449,7 @@ valid_name(const char *name)
>  {
>  /* just try to filter out any dangerous characters in the name
> that can be
>   * used to subvert the profile */
> -const char *bad = " /[]*";
> +const char *bad = "/[]*";
>  
>  if (strlen(name) == 0)
>  return -1;

Your justification seems reasonable. It does mean that we'll need
always quote rules that use def->name and looking at virt-aa-helper.c,
that seems to be the case.

All that said, I was surprised that tests/virt-aa-helper-test didn't
need to be updated, but, indeed, this is a testing gap.

+1 as is, but perhaps in a follow-up patch you could expand bad to be:

  const char *bad = "/[]{}?^,\"*";

'{', '}', '?', '^', ',' and '"' are characters used in AARE (see
'Globbing' in 'man apparmor.d') and add tests to tests/virt-aa-helper-
test for this.

Thanks!

-- 
Jamie Strandboge | http://www.canonical.com

signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 4/4] virt-aa-helper: put static rules in quotes

[Jamie Strandboge]

On Wed, 2017-09-20 at 16:59 +0200, Christian Ehrhardt wrote:
> To avoid any issues later on if paths ever change (unlikely but
> possible) and to match the style of other generated rules the paths
> of the static rules have to be quoted as well.
> 
> Signed-off-by: Christian Ehrhardt 
> ---
>  src/security/virt-aa-helper.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-
> helper.c
> index 5f4519d..95906e6 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -1149,11 +1149,11 @@ get_files(vahControl * ctl)
>  }
>  }
>  if (needsvhost)
> -virBufferAddLit(, "  /dev/vhost-net rw,\n");
> +virBufferAddLit(, "  \"/dev/vhost-net\" rw,\n");
>  
>  if (needsVfio) {
> -virBufferAddLit(, "  /dev/vfio/vfio rw,\n");
> -virBufferAddLit(, "  /dev/vfio/[0-9]* rw,\n");
> +virBufferAddLit(, "  \"/dev/vfio/vfio\" rw,\n");
> +virBufferAddLit(, "  \"/dev/vfio/[0-9]*\" rw,\n");
>  }
>  
>  if (ctl->newfile)

LGTM +1
-- 
Jamie Strandboge | http://www.canonical.com

signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/4] virt-aa-helper: fix libusb access to udev usb data

[Jamie Strandboge]

On Wed, 2017-09-20 at 16:59 +0200, Christian Ehrhardt wrote:
> 
> +  # libusb needs udev data about usb devices (~equal to content of
> lsusb -v)
> +  /run/udev/data/c16[6,7]* r,
> +  /run/udev/data/c18[0,8,9]* r,
>  
This read-only access looks fine to me. +1

-- 
Jamie Strandboge | http://www.canonical.com

signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] virt-aa-helper: fix libusb access to udev usb descriptions

[Jamie Strandboge]

On Wed, 2017-10-25 at 14:42 +0200, Christian Ehrhardt wrote:
> In bf3a4140 "virt-aa-helper: fix libusb access to udev usb data" the
> libusb access to properly detect the device/bus ids was fixed.
> 
> The path /run/udev/data/+usb* contains a subset of that information
> we
> already allow to be read and are currently not needed for the
> function
> qemu needs libusb for. But on the init of libusb all those files are
> still read so a lot of apparmor denials can be seen when using usb
> host
> devices, like:
>   apparmor="DENIED" operation="open" name="/run/udev/data/+usb:2-
> 1.2:1.0"
>   comm="qemu-system-x86" requested_mask="r" denied_mask="r"
> 
> Today we could silence the warnings with a deny rule without breaking
> current use cases. But since the data in there is only a subset of
> those
> it can read already it is no additional information exposure. And on
> the
> other hand a future udev/libusb/qemu combination might need it so
> allow
> the access in the default apparmor profile.
> 
> Signed-off-by: Christian Ehrhardt 
> ---
>  examples/apparmor/libvirt-qemu | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/examples/apparmor/libvirt-qemu
> b/examples/apparmor/libvirt-qemu
> index b341e31..97dd2d4 100644
> --- a/examples/apparmor/libvirt-qemu
> +++ b/examples/apparmor/libvirt-qemu
> @@ -32,6 +32,7 @@
># libusb needs udev data about usb devices (~equal to content of
> lsusb -v)
>/run/udev/data/c16[6,7]* r,
>/run/udev/data/c18[0,8,9]* r,
> +  /run/udev/data/+usb* r,

This read-only access seems perfectly fine to me. +1

-- 
Jamie Strandboge | http://www.canonical.com

signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] virt-aa-helper: grant locking permission on -f

[Jamie Strandboge]

On Tue, 2017-10-24 at 16:54 +0200, Christian Ehrhardt wrote:
> Hot-adding disks does not parse the full XML to generate apparmor
> rules.
> Instead it uses -f  to append a generic rule for that file
> path.
> 
> 580cdaa7: "virt-aa-helper: locking disk files for qemu 2.10"
> implemented
> the qemu 2.10 requirement to allow locking on disks images that are
> part of
> the domain xml.
> 
> But on attach-device a user will still trigger an apparmor deny by
> going
> through virt-aa-helper -f, to fix that add the lock "k" permission to
> the
> append file case of virt-aa-helper.
> 
> Signed-off-by: Christian Ehrhardt 
> ---
>  src/security/virt-aa-helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-
> helper.c
> index ef1bf01..ee3913d 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -1157,7 +1157,7 @@ get_files(vahControl * ctl)
>  }
>  
>  if (ctl->newfile)
> -if (vah_add_file(, ctl->newfile, "rw") != 0)
> +if (vah_add_file(, ctl->newfile, "rwk") != 0)
>  goto cleanup;
>  
>  if (virBufferError()) {
> @@ -1341,7 +1341,7 @@ main(int argc, char **argv)
>  vah_error(ctl, 1, _("profile exists"));
>  
>  if (ctl->append && ctl->newfile) {
> -if (vah_add_file(, ctl->newfile, "rw") != 0)
> +if (vah_add_file(, ctl->newfile, "rwk") != 0)
>  goto cleanup;
>  } else {
>  if (ctl->def->virtType == VIR_DOMAIN_VIRT_QEMU ||

This looks good to me. +1

-- 
Jamie Strandboge | http://www.canonical.com

signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] [PATCH/QEMU] s390x/kvm: use cpu_model_available for guarded storage on compat machines

[Jason J. Herne]

On 10/20/2017 10:54 AM, Christian Borntraeger wrote:

Starting a guest with

 hvm
   
   

on an IBM z14 results in

"qemu-system-s390x: Some features requested in the CPU model are not
available in the configuration: gs"

This is because guarded storage is fenced for compat machines that did not have
guarded storage support, but libvirt expands the cpu model according to the
latest available machine.

While this prevents future migration abort (by not starting the guest at all),
not being able to start a "host-model" guest is very much unexpected.  As it
turns out, even if we would modify libvirt to not expand the cpu model to
contain "gs" for compat machines, it cannot guarantee that a migration will
succeed. For example if the kernel changes its features (or the user has
nested=1 on one host but not on the other) the migration will fail
nevertheless.  So instead of fencing "gs" for machines <= 2.9 lets allow it for
all machine types that support the CPU model. This will make "host-model"
runnable all the time, while relying on the CPU model to reject invalid
migration attempts.

...

-if (gs_allowed()) {
+if (cpu_model_allowed()) {
  if (kvm_vm_enable_cap(s, KVM_CAP_S390_GS, 0) == 0) {
  cap_gs = 1;


Ok, honestly, I dislike this idea because it means we are effectively 
lying now. We will happily accept a +gs cpu model with a 2.9 compat 
machine when the point of compat machines is to mimick the older version 
of Qemu which does not support GS.  Yes, model checking will prevent the 
worst side effects, namely, exploding migrations.


I'd far prefer a solution that makes host-model dependent on the machine 
type. But I understand some of the backlash on that idea. Still, it 
seems like the cleanest approach even if it will be more work.


With all of that said, I know we want this fixed and your patch seems to 
fix the problem. So if you need an R-b...


Reviewed-by: Jason J. Herne 

Can we have a new tag? :-D
Reviewed-by-with-reservations: Jason J. Herne 

--
-- Jason J. Herne (jjhe...@linux.vnet.ibm.com)

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] AppArmor: add rules needed with additional mediation features brought by Linux 4.14.

[intrigeri]

intrigeri:
> +  network unix dgram,
> +  network unix stream,

Hold on, these two rules are probably not needed (chances are that
they were needed due to a bug in the AppArmor parser, that got fixed
in 2.11.1). I'll double-check tomorrow. Sorry for the noise!

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH/QEMU] s390x/kvm: use cpu_model_available for guarded storage on compat machines

[Marc Hartmayer]

On Wed, Oct 25, 2017 at 05:50 PM +0200, David Hildenbrand  
wrote:
> On 25.10.2017 17:09, Boris Fiuczynski wrote:
>> On 10/25/2017 12:23 PM, David Hildenbrand wrote:
>>> On 25.10.2017 12:18, Christian Borntraeger wrote:
 Ping, I plan to submit belows patch for 2.11. We can then still look into
 a libvirt<->qemu interface for limiting host-model depending on machine 
 versions
 (or not).
>>>
>>> I think this would be sufficient for now.
>>>
>>> Having different host models, depending on the the machine type sounds
>>> wrong. But maybe we'll need it in the future.
>>>
>>
>> David, I disagree if your proposal is to generally tolerate new cpu
>> features in old machine types. This *might* work for gs but how do you
>> guaranty that guests do not behave differently/wrong when suddenly new
>> cpu features are made available to guest when (re-)starting them.
>> That is my feedback for the qemu side of this mater.
>
> Just re-reading this section, so what you mean is:
>
> a) guest is started, host model is copied and used. guest works.
> b) guest is stopped.
> c) QEMU/KVM/HW is updated.
> d) guest is started, new host model is copied. guest no longer works.
>
> d) could be because there are now some additional features with e.g.
> broken guest implementation or now missing features.
>
>
> What you propose (if I am not wrong) is a to bind features somehow to a
> QEMU machine. I think that should never be done. You could not catch now
> missing features.

What exactly do you mean by the last sentence?

>
> What would you think about a persistent host-model copy option? So
> instead of copying at every start, only copy and replace it once in the XML?
>
> Easy to specify by the user and no CPU feature changes will happend
> "blindly".
>
>
> --
>
> Thanks,
>
> David
>
--
Beste Grüße / Kind regards
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] AppArmor: add rules needed with additional mediation features brought by Linux 4.14.

[intrigeri]

---
 examples/apparmor/libvirt-qemu  | 2 ++
 examples/apparmor/usr.sbin.libvirtd | 9 +
 2 files changed, 11 insertions(+)

diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu
index b341e31f42..5994a35042 100644
--- a/examples/apparmor/libvirt-qemu
+++ b/examples/apparmor/libvirt-qemu
@@ -16,6 +16,8 @@
   network inet stream,
   network inet6 stream,
 
+  signal (receive) set=("term") peer=/usr/sbin/libvirtd,
+
   /dev/net/tun rw,
   /dev/kvm rw,
   /dev/ptmx rw,
diff --git a/examples/apparmor/usr.sbin.libvirtd 
b/examples/apparmor/usr.sbin.libvirtd
index 819068ffc3..17b5ee38ff 100644
--- a/examples/apparmor/usr.sbin.libvirtd
+++ b/examples/apparmor/usr.sbin.libvirtd
@@ -30,6 +30,8 @@
   # Needed for vfio
   capability sys_resource,
 
+  mount,
+
   network inet stream,
   network inet dgram,
   network inet6 stream,
@@ -37,11 +39,18 @@
   network packet dgram,
   network packet raw,
 
+  network netlink raw,
+  network unix dgram,
+  network unix stream,
+
   ptrace (trace) peer=unconfined,
   ptrace (trace) peer=/usr/sbin/libvirtd,
   ptrace (trace) peer=/usr/sbin/dnsmasq,
   ptrace (trace) peer=libvirt-*,
 
+  signal (send) set=("hup") peer=/usr/sbin/dnsmasq,
+  signal (send) set=("term") peer=libvirt-*,
+
   # Very lenient profile for libvirtd since we want to first focus on confining
   # the guests. Guests will have a very restricted profile.
   / r,
-- 
2.15.0.rc2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH/QEMU] s390x/kvm: use cpu_model_available for guarded storage on compat machines

[David Hildenbrand]

On 25.10.2017 17:09, Boris Fiuczynski wrote:
> On 10/25/2017 12:23 PM, David Hildenbrand wrote:
>> On 25.10.2017 12:18, Christian Borntraeger wrote:
>>> Ping, I plan to submit belows patch for 2.11. We can then still look into
>>> a libvirt<->qemu interface for limiting host-model depending on machine 
>>> versions
>>> (or not).
>>
>> I think this would be sufficient for now.
>>
>> Having different host models, depending on the the machine type sounds
>> wrong. But maybe we'll need it in the future.
>>
> 
> David, I disagree if your proposal is to generally tolerate new cpu 
> features in old machine types. This *might* work for gs but how do you 
> guaranty that guests do not behave differently/wrong when suddenly new 
> cpu features are made available to guest when (re-)starting them.
> That is my feedback for the qemu side of this mater.

Just re-reading this section, so what you mean is:

a) guest is started, host model is copied and used. guest works.
b) guest is stopped.
c) QEMU/KVM/HW is updated.
d) guest is started, new host model is copied. guest no longer works.

d) could be because there are now some additional features with e.g.
broken guest implementation or now missing features.


What you propose (if I am not wrong) is a to bind features somehow to a
QEMU machine. I think that should never be done. You could not catch now
missing features.

What would you think about a persistent host-model copy option? So
instead of copying at every start, only copy and replace it once in the XML?

Easy to specify by the user and no CPU feature changes will happend
"blindly".


-- 

Thanks,

David

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH/QEMU] s390x/kvm: use cpu_model_available for guarded storage on compat machines

[David Hildenbrand]

On 25.10.2017 17:09, Boris Fiuczynski wrote:
> On 10/25/2017 12:23 PM, David Hildenbrand wrote:
>> On 25.10.2017 12:18, Christian Borntraeger wrote:
>>> Ping, I plan to submit belows patch for 2.11. We can then still look into
>>> a libvirt<->qemu interface for limiting host-model depending on machine 
>>> versions
>>> (or not).
>>
>> I think this would be sufficient for now.
>>
>> Having different host models, depending on the the machine type sounds
>> wrong. But maybe we'll need it in the future.
>>
> 
> David, I disagree if your proposal is to generally tolerate new cpu 
> features in old machine types. This *might* work for gs but how do you 
> guaranty that guests do not behave differently/wrong when suddenly new 
> cpu features are made available to guest when (re-)starting them.
> That is my feedback for the qemu side of this mater.

My point would be that it seems to work for all existing architectures
(so far as I am aware) and this one problem is easily fixed (and stems
from old CPU feature compatibility handling). So my question would be,
are there any potential CPU features that make such handling necessary
right now or in the near future?

> 
> Regarding the libvirt side of this:
> When looking at https://libvirt.org/formatdomain.html#elementsCPU I 
> found the following sentence:
> Since the CPU definition is copied just before starting a domain, 
> exactly the same XML can be used on different hosts while still 
> providing the best guest CPU each host supports.
> 
> My interpretation of "the best guest CPU each host supports" is that 
> besides limiting factors like hardware, kernel and qemu capabilities the 
> requested machine type for the guest is a limiting factor as well.

I understand "what the host supports" as combination of hw+kernel+qemu.
But the definition can be interpreted differently. I don't think that
the requested machine has to be taken into account at this point.
(Again, do you have any real examples where this would be applicable?)

> 
> Nevertheless if my interpretation is found to be incorrect than we 
> should think about another new cpu mode that includes the machine type 
> into the "best guest CPU" detection.

Which use case? I just want to understand how the current solution could
be problematic? (besides the problem we had, which is easily fixed)

> My assumption is that we must not require the users to know which cpu 
> model they manually need to define to match a specific machine type
> AND we want to guarantee that guests run without risking any side 
> effects by tolerating any additional cpu features.

That's why I think CPU models should be independent of the used QEMU
machine. It just over complicates things as we have seen.

Especially suddenly having multiple "host" CPU models depending on the
machine type, confusing. If we can, we should keep it simple.

-- 

Thanks,

David

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH go-xml] Add support for CPUTune in Domain

[Michal Rostecki]

From: Michal Rostecki 

Signed-off-by: Victoria Efimova 
Signed-off-by: Ivan Shvedunov 
Signed-off-by: Miha Pleško 
Signed-off-by: Michal Rostecki 
---
 domain.go  | 19 +++
 domain_test.go | 20 
 2 files changed, 39 insertions(+)

diff --git a/domain.go b/domain.go
index bacab11..3efd68c 100644
--- a/domain.go
+++ b/domain.go
@@ -722,6 +722,24 @@ type DomainFeatureList struct {
SMM*DomainFeatureState  `xml:"smm"`
 }
 
+type DomainCPUTuneShares struct {
+   Value uint `xml:",chardata"`
+}
+
+type DomainCPUTunePeriod struct {
+   Value uint64 `xml:",chardata"`
+}
+
+type DomainCPUTuneQuota struct {
+   Value int64 `xml:",chardata"`
+}
+
+type DomainCPUTune struct {
+   Shares *DomainCPUTuneShares `xml:"shares"`
+   Period *DomainCPUTunePeriod `xml:"period"`
+   Quota  *DomainCPUTuneQuota  `xml:"quota"`
+}
+
 type DomainQEMUCommandlineArg struct {
Value string `xml:"value,attr"`
 }
@@ -751,6 +769,7 @@ type Domain struct {
MemoryBacking   *DomainMemoryBacking `xml:"memoryBacking"`
VCPU*DomainVCPU  `xml:"vcpu"`
VCPUs   *DomainVCPUs `xml:"vcpus"`
+   CPUTune *DomainCPUTune   `xml:"cputune"`
Resource*DomainResource  `xml:"resource"`
SysInfo *DomainSysInfo   `xml:"sysinfo"`
OS  *DomainOS`xml:"os"`
diff --git a/domain_test.go b/domain_test.go
index dbebe42..b427b50 100644
--- a/domain_test.go
+++ b/domain_test.go
@@ -1335,6 +1335,26 @@ var domainTestData = []struct {
``,
},
},
+   {
+   Object: {
+   Name: "test",
+   CPUTune: {
+   Shares: {Value: 1024},
+   Period: {Value: 50},
+   Quota:  {Value: -1},
+   },
+   },
+   Expected: []string{
+   ``,
+   `  test`,
+   `  `,
+   `1024`,
+   `50`,
+   `-1`,
+   `  `,
+   ``,
+   },
+   },
 
/* Tests for sub-documents that can be hotplugged */
{
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH/QEMU] s390x/kvm: use cpu_model_available for guarded storage on compat machines

[Boris Fiuczynski]

On 10/25/2017 12:23 PM, David Hildenbrand wrote:

On 25.10.2017 12:18, Christian Borntraeger wrote:

Ping, I plan to submit belows patch for 2.11. We can then still look into
a libvirt<->qemu interface for limiting host-model depending on machine versions
(or not).


I think this would be sufficient for now.

Having different host models, depending on the the machine type sounds
wrong. But maybe we'll need it in the future.



David, I disagree if your proposal is to generally tolerate new cpu 
features in old machine types. This *might* work for gs but how do you 
guaranty that guests do not behave differently/wrong when suddenly new 
cpu features are made available to guest when (re-)starting them.

That is my feedback for the qemu side of this mater.

Regarding the libvirt side of this:
When looking at https://libvirt.org/formatdomain.html#elementsCPU I 
found the following sentence:
Since the CPU definition is copied just before starting a domain, 
exactly the same XML can be used on different hosts while still 
providing the best guest CPU each host supports.


My interpretation of "the best guest CPU each host supports" is that 
besides limiting factors like hardware, kernel and qemu capabilities the 
requested machine type for the guest is a limiting factor as well.


Nevertheless if my interpretation is found to be incorrect than we 
should think about another new cpu mode that includes the machine type 
into the "best guest CPU" detection.
My assumption is that we must not require the users to know which cpu 
model they manually need to define to match a specific machine type
AND we want to guarantee that guests run without risking any side 
effects by tolerating any additional cpu features.


--
Mit freundlichen Grüßen/Kind regards
   Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 0/8] Virtio-crypto device support

[Matthew Rosato]

On 07/07/2017 04:07 AM, Longpeng(Mike) wrote:
> As virtio-crypto has been supported in QEMU 2.8 and the frontend
> driver has been merged in linux 4.10, so it's necessary to support
> virtio-crypto in libvirt.
> 
> ---

Hi Mike,

Seems like this topic has gone quiet..  Is there a v5 in the works?

Matt

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] virt-aa-helper: fix libusb access to udev usb descriptions

[Christian Ehrhardt]

In bf3a4140 "virt-aa-helper: fix libusb access to udev usb data" the
libusb access to properly detect the device/bus ids was fixed.

The path /run/udev/data/+usb* contains a subset of that information we
already allow to be read and are currently not needed for the function
qemu needs libusb for. But on the init of libusb all those files are
still read so a lot of apparmor denials can be seen when using usb host
devices, like:
  apparmor="DENIED" operation="open" name="/run/udev/data/+usb:2-1.2:1.0"
  comm="qemu-system-x86" requested_mask="r" denied_mask="r"

Today we could silence the warnings with a deny rule without breaking
current use cases. But since the data in there is only a subset of those
it can read already it is no additional information exposure. And on the
other hand a future udev/libusb/qemu combination might need it so allow
the access in the default apparmor profile.

Signed-off-by: Christian Ehrhardt 
---
 examples/apparmor/libvirt-qemu | 1 +
 1 file changed, 1 insertion(+)

diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu
index b341e31..97dd2d4 100644
--- a/examples/apparmor/libvirt-qemu
+++ b/examples/apparmor/libvirt-qemu
@@ -32,6 +32,7 @@
   # libusb needs udev data about usb devices (~equal to content of lsusb -v)
   /run/udev/data/c16[6,7]* r,
   /run/udev/data/c18[0,8,9]* r,
+  /run/udev/data/+usb* r,
 
   # WARNING: this gives the guest direct access to host hardware and specific
   # portions of shared memory. This is required for sound using ALSA with kvm,
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/2] qemuDomainSetInterfaceParameters: Explicitly reject unsupported net types

[Michal Privoznik]

For instance, NET_TYPE_MCAST doesn't support setting QoS. Instead
of claiming success and doing nothing, we should be explicit
about that and report an error.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_driver.c | 21 +
 1 file changed, 21 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 32a416f9e..54a93711a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -108,6 +108,7 @@
 #include "virnuma.h"
 #include "dirname.h"
 #include "network/bridge_driver.h"
+#include "netdev_bandwidth_conf.h"
 
 #define VIR_FROM_THIS VIR_FROM_QEMU
 
@@ -11106,6 +11107,8 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom,
 virNetDevBandwidthPtr bandwidth = NULL, newBandwidth = NULL;
 virQEMUDriverConfigPtr cfg = NULL;
 bool inboundSpecified = false, outboundSpecified = false;
+int actualType;
+bool qosSupported = true;
 
 virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
   VIR_DOMAIN_AFFECT_CONFIG, -1);
@@ -11149,6 +11152,24 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom,
 !(persistentNet = virDomainNetFind(persistentDef, device)))
 goto endjob;
 
+if (net) {
+actualType = virDomainNetGetActualType(net);
+qosSupported = virNetDevSupportBandwidth(actualType);
+}
+
+if (qosSupported && persistentNet) {
+actualType = virDomainNetGetActualType(persistentNet);
+qosSupported = virNetDevSupportBandwidth(actualType);
+}
+
+if (!qosSupported) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("setting bandwidth on interfaces of "
+ "type '%s' is not implemented yet"),
+   virDomainNetTypeToString(actualType));
+goto endjob;
+}
+
 if ((VIR_ALLOC(bandwidth) < 0) ||
 (VIR_ALLOC(bandwidth->in) < 0) ||
 (VIR_ALLOC(bandwidth->out) < 0))
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/2] virNetDevSupportBandwidth: Enable QoS for vhostuser

[Michal Privoznik]

Since vhostuser type is really a tap that is just plugged into
different type of bridge, supporting QoS is trivial.

Signed-off-by: Michal Privoznik 
---
 src/conf/netdev_bandwidth_conf.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/conf/netdev_bandwidth_conf.h b/src/conf/netdev_bandwidth_conf.h
index 30f988953..c37828065 100644
--- a/src/conf/netdev_bandwidth_conf.h
+++ b/src/conf/netdev_bandwidth_conf.h
@@ -46,9 +46,9 @@ static inline bool virNetDevSupportBandwidth(virDomainNetType 
type)
 case VIR_DOMAIN_NET_TYPE_NETWORK:
 case VIR_DOMAIN_NET_TYPE_DIRECT:
 case VIR_DOMAIN_NET_TYPE_ETHERNET:
+case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
 return true;
 case VIR_DOMAIN_NET_TYPE_USER:
-case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
 case VIR_DOMAIN_NET_TYPE_SERVER:
 case VIR_DOMAIN_NET_TYPE_CLIENT:
 case VIR_DOMAIN_NET_TYPE_MCAST:
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 0/2] qemu: Fix QoS for vhostuser

[Michal Privoznik]

Setting QoS on the fly does work for vhostuser. However, it doesn't when
starting a domain. So when looking into this, I realized that we should be more
explicit about types of interface that do support QoS.

Michal Privoznik (2):
  qemuDomainSetInterfaceParameters: Explicitly reject unsupported net
types
  virNetDevSupportBandwidth: Enable QoS for vhostuser

 src/conf/netdev_bandwidth_conf.h |  2 +-
 src/qemu/qemu_driver.c   | 21 +
 2 files changed, 22 insertions(+), 1 deletion(-)

-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 02/22] conf: Add usability blockers to virDomainCapsCPUModel

[Marc Hartmayer]

On Wed, Oct 25, 2017 at 10:56 AM +0200, Marc Hartmayer 
 wrote:
> On Fri, Oct 13, 2017 at 08:14 PM +0200, Jiri Denemark  
> wrote:
>> When a hypervisor marks a CPU model as unusable on the current host, it
>> may also give us a list of features which prevent the model from being
>> usable. Storing this list in virDomainCapsCPUModel will help the CPU
>> driver with creating a host-model CPU configuration.
>>
>> Signed-off-by: Jiri Denemark 
>> Reviewed-by: John Ferlan 
>> ---
[...snip...]

>>
>> diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h
>> index 82183c4524..8c71dec21e 100644
>> --- a/src/conf/domain_capabilities.h
>> +++ b/src/conf/domain_capabilities.h
>> @@ -116,6 +116,7 @@ typedef virDomainCapsCPUModel *virDomainCapsCPUModelPtr;
>>  struct _virDomainCapsCPUModel {
>>  char *name;
>>  virDomainCapsCPUUsable usable;
>> +char **blockers; /* NULL-terminated list of usability blockers */
>>  };
>
> I know this is an "old" thread and already pushed. But I think you have
> to free the blockers list in virDomainCapsCPUModelsDispose as well. No?

Sorry for the inconvenience. I'll send a patch for it :)

>>
>>  typedef struct _virDomainCapsCPUModels virDomainCapsCPUModels;
>> @@ -171,11 +172,13 @@ virDomainCapsCPUModelsPtr 
>> virDomainCapsCPUModelsFilter(virDomainCapsCPUModelsPtr
>> const char 
>> **blacklist);
>>  int virDomainCapsCPUModelsAddSteal(virDomainCapsCPUModelsPtr cpuModels,
>> char **name,
>> -   virDomainCapsCPUUsable usable);
>> +   virDomainCapsCPUUsable usable,
>> +   char ***blockers);
>>  int virDomainCapsCPUModelsAdd(virDomainCapsCPUModelsPtr cpuModels,
>>const char *name,
>>ssize_t nameLen,
>> -  virDomainCapsCPUUsable usable);
>
> [...snip]
>
> --
> Beste Grüße / Kind regards
>Marc Hartmayer
>
> IBM Deutschland Research & Development GmbH
> Vorsitzende des Aufsichtsrats: Martina Koederitz
> Geschäftsführung: Dirk Wittkopp
> Sitz der Gesellschaft: Böblingen
> Registergericht: Amtsgericht Stuttgart, HRB 243294
--
Beste Grüße / Kind regards
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [[RFC] 0/8] Implement async QEMU event handling in libvirtd.

[Jiri Denemark]

On Tue, Oct 24, 2017 at 10:34:53 -0700, Prerna Saxena wrote:
> 
> As noted in
> https://www.redhat.com/archives/libvir-list/2017-May/msg00016.html
> libvirt-QEMU driver handles all async events from the main loop.
> Each event handling needs the per-VM lock to make forward progress. In
> the case where an async event is received for the same VM which has an
> RPC running, the main loop is held up contending for the same lock.
> 
> This impacts scalability, and should be addressed on priority.
> 
> Note that libvirt does have a 2-step deferred handling for a few event
> categories, but (1) That is insufficient since blockign happens before
> the handler could disambiguate which one needs to be posted to this
> other queue.
> (2) There needs to be homogeniety.
> 
> The current series builds a framework for recording and handling VM
> events.
> It initializes per-VM event queue, and a global event queue pointing to
> events from all the VMs. Event handling is staggered in 2 stages:
> - When an event is received, it is enqueued in the per-VM queue as well
>   as the global queues.
> - The global queue is built into the QEMU Driver as a threadpool
>   (currently with a single thread).
> - Enqueuing of a new event triggers the global event worker thread, which
>   then attempts to take a lock for this event's VM.
> - If the lock is available, the event worker runs the function handling
>   this event type. Once done, it dequeues this event from the global
>   as well as per-VM queues.
> - If the lock is unavailable(ie taken by RPC thread), the event worker 
>   thread leaves this as-is and picks up the next event.

If I get it right, the event is either processed immediately when its VM
object is unlocked or it has to wait until the current job running on
the VM object finishes even though the lock may be released before that.
Correct? If so, this needs to be addressed.

> - Once the RPC thread completes, it looks for events pertaining to the
>   VM in the per-VM event queue. It then processes the events serially
>   (holding the VM lock) until there are no more events remaining for
>   this VM. At this point, the per-VM lock is relinquished.
> 
> Patch Series status:
> Strictly RFC only. No compilation issues. I have not had a chance to
> (stress) test it after rebase to latest master.
> Note that documentation and test coverage is TBD, since a few open
> points remain.
> 
> Known issues/ caveats:
> - RPC handling time will become non-deterministic.
> - An event will only be "notified" to a client once the RPC for same VM 
> completes.
> - Needs careful consideration in all cases where a QMP event is used to
>   "signal" an RPC thread, else will deadlock.

This last issue is actually a show stopper here. We need to make sure
QMP events are processed while a job is still active on the same domain.
Otherwise thinks kile block jobs and migration, which are long running
jobs driven by events, will break.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH go-xml] Add bootp option to NetworkDHCP

[Michal Rostecki]

From: Michal Rostecki 

Support specyfing concrete file and TFTP server
for PXE boot.

Signed-off-by: Michal Rostecki 
---
 network.go  | 6 ++
 network_test.go | 7 +++
 2 files changed, 13 insertions(+)

diff --git a/network.go b/network.go
index 32f125a..10c4dfc 100644
--- a/network.go
+++ b/network.go
@@ -78,9 +78,15 @@ type NetworkDHCPHost struct {
IP   string `xml:"ip,attr,omitempty"`
 }
 
+type NetworkBootp struct {
+   File   string `xml:"file,attr,omitempty"`
+   Server string `xml:"server,attr,omitempty"`
+}
+
 type NetworkDHCP struct {
Ranges []NetworkDHCPRange `xml:"range"`
Hosts  []NetworkDHCPHost  `xml:"host"`
+   Bootp  []NetworkBootp `xml:"bootp"`
 }
 
 type NetworkIP struct {
diff --git a/network_test.go b/network_test.go
index b1bd168..3efb278 100644
--- a/network_test.go
+++ b/network_test.go
@@ -100,6 +100,12 @@ var networkTestData = []struct {
IP:   
"192.168.122.10",
},
},
+   Bootp: []NetworkBootp{
+   NetworkBootp{
+   File:   
"pxelinux.0",
+   Server: 
"192.168.122.1",
+   },
+   },
},
},
NetworkIP{
@@ -179,6 +185,7 @@ var networkTestData = []struct {
``,
`  `,
`  `,
+   `  `,
``,
`  `,
`  `,
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH/QEMU] s390x/kvm: use cpu_model_available for guarded storage on compat machines

[David Hildenbrand]

On 25.10.2017 12:18, Christian Borntraeger wrote:
> Ping, I plan to submit belows patch for 2.11. We can then still look into
> a libvirt<->qemu interface for limiting host-model depending on machine 
> versions
> (or not).

I think this would be sufficient for now.

Having different host models, depending on the the machine type sounds
wrong. But maybe we'll need it in the future.


-- 

Thanks,

David

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH/QEMU] s390x/kvm: use cpu_model_available for guarded storage on compat machines

[Christian Borntraeger]

Ping, I plan to submit belows patch for 2.11. We can then still look into
a libvirt<->qemu interface for limiting host-model depending on machine versions
(or not).

On 10/20/2017 04:54 PM, Christian Borntraeger wrote:
> Starting a guest with
>
> hvm
>   
>   
> 
> on an IBM z14 results in
> 
> "qemu-system-s390x: Some features requested in the CPU model are not
> available in the configuration: gs"
> 
> This is because guarded storage is fenced for compat machines that did not 
> have
> guarded storage support, but libvirt expands the cpu model according to the
> latest available machine.
> 
> While this prevents future migration abort (by not starting the guest at all),
> not being able to start a "host-model" guest is very much unexpected.  As it
> turns out, even if we would modify libvirt to not expand the cpu model to
> contain "gs" for compat machines, it cannot guarantee that a migration will
> succeed. For example if the kernel changes its features (or the user has
> nested=1 on one host but not on the other) the migration will fail
> nevertheless.  So instead of fencing "gs" for machines <= 2.9 lets allow it 
> for
> all machine types that support the CPU model. This will make "host-model"
> runnable all the time, while relying on the CPU model to reject invalid
> migration attempts.
> 
> Suggested-by: David Hildenbrand 
> Signed-off-by: Christian Borntraeger 
> ---
>  hw/s390x/s390-virtio-ccw.c | 8 
>  include/hw/s390x/s390-virtio-ccw.h | 3 ---
>  target/s390x/kvm.c | 2 +-
>  3 files changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index fabe4a6..ae5b01a 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -414,7 +414,6 @@ static void ccw_machine_class_init(ObjectClass *oc, void 
> *data)
>  s390mc->ri_allowed = true;
>  s390mc->cpu_model_allowed = true;
>  s390mc->css_migration_enabled = true;
> -s390mc->gs_allowed = true;
>  mc->init = ccw_init;
>  mc->reset = s390_machine_reset;
>  mc->hot_add_cpu = s390_hot_add_cpu;
> @@ -495,12 +494,6 @@ bool cpu_model_allowed(void)
>  return get_machine_class()->cpu_model_allowed;
>  }
> 
> -bool gs_allowed(void)
> -{
> -/* for "none" machine this results in true */
> -return get_machine_class()->gs_allowed;
> -}
> -
>  static char *machine_get_loadparm(Object *obj, Error **errp)
>  {
>  S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
> @@ -740,7 +733,6 @@ static void ccw_machine_2_9_class_options(MachineClass 
> *mc)
>  {
>  S390CcwMachineClass *s390mc = S390_MACHINE_CLASS(mc);
> 
> -s390mc->gs_allowed = false;
>  ccw_machine_2_10_class_options(mc);
>  SET_MACHINE_COMPAT(mc, CCW_COMPAT_2_9);
>  s390mc->css_migration_enabled = false;
> diff --git a/include/hw/s390x/s390-virtio-ccw.h 
> b/include/hw/s390x/s390-virtio-ccw.h
> index a9a90c2..ac896e3 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -40,15 +40,12 @@ typedef struct S390CcwMachineClass {
>  bool ri_allowed;
>  bool cpu_model_allowed;
>  bool css_migration_enabled;
> -bool gs_allowed;
>  } S390CcwMachineClass;
> 
>  /* runtime-instrumentation allowed by the machine */
>  bool ri_allowed(void);
>  /* cpu model allowed by the machine */
>  bool cpu_model_allowed(void);
> -/* guarded-storage allowed by the machine */
> -bool gs_allowed(void);
> 
>  /**
>   * Returns true if (vmstate based) migration of the channel subsystem
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 4c85ed8..020a7ea 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -363,7 +363,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>  cap_ri = 1;
>  }
>  }
> -if (gs_allowed()) {
> +if (cpu_model_allowed()) {
>  if (kvm_vm_enable_cap(s, KVM_CAP_S390_GS, 0) == 0) {
>  cap_gs = 1;
>  }
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 2/2] virtlogd: add missing netserver refcount increment on reload

[John Ferlan]

On 10/25/2017 05:15 AM, Nikolay Shirokovskiy wrote:
> 
> 
> On 25.10.2017 12:06, John Ferlan wrote:
>>
>>
>> On 10/24/2017 06:47 AM, Nikolay Shirokovskiy wrote:
>>> After virNetDaemonAddServerPostExec call in virtlogd we should have
>>> netserver refcount set to 2. One goes to netdaemon servers hashtable
>>> and one goes to virtlogd own reference to netserver. Let's add
>>> missing increment in virNetDaemonAddServerPostExec itself while holding
>>> daemon lock.
>>>
>>> We also have to unref new extra ref after virtlockd call to 
>>> virNetDaemonAddServerPostExec.
>>> ---
>>>  src/locking/lock_daemon.c | 1 +
>>>  src/rpc/virnetdaemon.c| 1 +
>>>  2 files changed, 2 insertions(+)
>>>
>>> diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
>>> index fe3eaf9..41a06b2 100644
>>> --- a/src/locking/lock_daemon.c
>>> +++ b/src/locking/lock_daemon.c
>>> @@ -275,6 +275,7 @@ virLockDaemonNewPostExecRestart(virJSONValuePtr object, 
>>> bool privileged)
>>>virLockDaemonClientFree,
>>>(void*)(intptr_t)(privileged 
>>> ? 0x1 : 0x0
>>>  goto error;
>>> +virObjectUnref(srv);
>>
>> I need to think a bit more about this one... different model in lockd
>> vs. logd over @srv usage especially w/r/t this particular path.
>>
>> I see that in this path eventually something calls virNetDaemonGetServer
>> instead of storing the @srv in order to add lockProgram... In any case,> I 
>> guess I'd be worried/concerned that something could remove @srv
>> causing the Hash table code to unref/delete the srv... Also, in the
>> cleanup: path of main() wouldn't the Unref(srv) cause problems?
> 
> But this unref only balance newly added ref. If there are other
> problems with reference conting in virtlockd - its a different
> concern.
> 

Ok - so what I've learned is that virLockDaemonPostExecRestart returns
rv == 1 which indicates a successful restore resulting in a call to
virNetDaemonGetServer which will do a virObjectRef anyway leaving us
with (theoretically) 2 refs prior to the code that adds programs to @srv
(and less concern from my part of the subsequent Unref in cleanup: here).

If we saved srv in lockd like logd does, then we wouldn't need to call
virNetDaemonGetServer resulting in the same eventual result except for a
window after the Unref here where the refcnt == 1 until
virNetDaemonGetServer is run. Since I believe nothing could happen in
between that would cause the Unref due to HashFree being called, then I
think we're OK.

The concern being is if virNetDaemonGetServer didn't find the server,
then @srv would be NULL and the subsequent call to
virNetServerAddProgram for lockProgram would fail miserably, but I don't
think that can happen theoretically at least!

John


>>
>> Again- need to think a bit longer.
>>
>> John
>>
>>>  
>>>  return lockd;
>>>  
>>> diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
>>> index 495bc4b..f3e3ffe 100644
>>> --- a/src/rpc/virnetdaemon.c
>>> +++ b/src/rpc/virnetdaemon.c
>>> @@ -312,6 +312,7 @@ virNetDaemonAddServerPostExec(virNetDaemonPtr dmn,
>>>  
>>>  if (virHashAddEntry(dmn->servers, serverName, srv) < 0)
>>>  goto error;
>>> +virObjectRef(srv);
>>>  
>>>  virJSONValueFree(object);
>>>  virObjectUnlock(dmn);
>>>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 2/2] virtlogd: add missing netserver refcount increment on reload

[Nikolay Shirokovskiy]

On 25.10.2017 12:06, John Ferlan wrote:
> 
> 
> On 10/24/2017 06:47 AM, Nikolay Shirokovskiy wrote:
>> After virNetDaemonAddServerPostExec call in virtlogd we should have
>> netserver refcount set to 2. One goes to netdaemon servers hashtable
>> and one goes to virtlogd own reference to netserver. Let's add
>> missing increment in virNetDaemonAddServerPostExec itself while holding
>> daemon lock.
>>
>> We also have to unref new extra ref after virtlockd call to 
>> virNetDaemonAddServerPostExec.
>> ---
>>  src/locking/lock_daemon.c | 1 +
>>  src/rpc/virnetdaemon.c| 1 +
>>  2 files changed, 2 insertions(+)
>>
>> diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
>> index fe3eaf9..41a06b2 100644
>> --- a/src/locking/lock_daemon.c
>> +++ b/src/locking/lock_daemon.c
>> @@ -275,6 +275,7 @@ virLockDaemonNewPostExecRestart(virJSONValuePtr object, 
>> bool privileged)
>>virLockDaemonClientFree,
>>(void*)(intptr_t)(privileged 
>> ? 0x1 : 0x0
>>  goto error;
>> +virObjectUnref(srv);
> 
> I need to think a bit more about this one... different model in lockd
> vs. logd over @srv usage especially w/r/t this particular path.
> 
> I see that in this path eventually something calls virNetDaemonGetServer
> instead of storing the @srv in order to add lockProgram... In any case,> I 
> guess I'd be worried/concerned that something could remove @srv
> causing the Hash table code to unref/delete the srv... Also, in the
> cleanup: path of main() wouldn't the Unref(srv) cause problems?

But this unref only balance newly added ref. If there are other
problems with reference conting in virtlockd - its a different
concern.

> 
> Again- need to think a bit longer.
> 
> John
> 
>>  
>>  return lockd;
>>  
>> diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
>> index 495bc4b..f3e3ffe 100644
>> --- a/src/rpc/virnetdaemon.c
>> +++ b/src/rpc/virnetdaemon.c
>> @@ -312,6 +312,7 @@ virNetDaemonAddServerPostExec(virNetDaemonPtr dmn,
>>  
>>  if (virHashAddEntry(dmn->servers, serverName, srv) < 0)
>>  goto error;
>> +virObjectRef(srv);
>>  
>>  virJSONValueFree(object);
>>  virObjectUnlock(dmn);
>>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 2/2] virtlogd: add missing netserver refcount increment on reload

[John Ferlan]

On 10/24/2017 06:47 AM, Nikolay Shirokovskiy wrote:
> After virNetDaemonAddServerPostExec call in virtlogd we should have
> netserver refcount set to 2. One goes to netdaemon servers hashtable
> and one goes to virtlogd own reference to netserver. Let's add
> missing increment in virNetDaemonAddServerPostExec itself while holding
> daemon lock.
> 
> We also have to unref new extra ref after virtlockd call to 
> virNetDaemonAddServerPostExec.
> ---
>  src/locking/lock_daemon.c | 1 +
>  src/rpc/virnetdaemon.c| 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
> index fe3eaf9..41a06b2 100644
> --- a/src/locking/lock_daemon.c
> +++ b/src/locking/lock_daemon.c
> @@ -275,6 +275,7 @@ virLockDaemonNewPostExecRestart(virJSONValuePtr object, 
> bool privileged)
>virLockDaemonClientFree,
>(void*)(intptr_t)(privileged ? 
> 0x1 : 0x0
>  goto error;
> +virObjectUnref(srv);

I need to think a bit more about this one... different model in lockd
vs. logd over @srv usage especially w/r/t this particular path.

I see that in this path eventually something calls virNetDaemonGetServer
instead of storing the @srv in order to add lockProgram... In any case,
I guess I'd be worried/concerned that something could remove @srv
causing the Hash table code to unref/delete the srv... Also, in the
cleanup: path of main() wouldn't the Unref(srv) cause problems?

Again- need to think a bit longer.

John

>  
>  return lockd;
>  
> diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
> index 495bc4b..f3e3ffe 100644
> --- a/src/rpc/virnetdaemon.c
> +++ b/src/rpc/virnetdaemon.c
> @@ -312,6 +312,7 @@ virNetDaemonAddServerPostExec(virNetDaemonPtr dmn,
>  
>  if (virHashAddEntry(dmn->servers, serverName, srv) < 0)
>  goto error;
> +virObjectRef(srv);
>  
>  virJSONValueFree(object);
>  virObjectUnlock(dmn);
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 4/4] qemu: monitor: check monitor not closed upon send

[Nikolay Shirokovskiy]

Close monitor sets monitor error if another thread is awating the
response to propagate error condition to that thread. However
if there is no such thread error will not be set. Now if API
thread try to send a message it will hang. This can easily happen
for example if API thread does not reach the point when it take
domain lock and qemu driver is shutdowned.

Let's add checks for whether monitor is closed to send routine
and remove passing of this condition thru setting monitor error.
---
 src/qemu/qemu_monitor.c | 27 +--
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 64efb89..63e6f74 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1000,22 +1000,9 @@ qemuMonitorClose(qemuMonitorPtr mon)
 }
 
 /* In case another thread is waiting for its monitor command to be
- * processed, we need to wake it up with appropriate error set.
+ * processed, we need to wake it up.
  */
 if (mon->msg) {
-if (mon->lastError.code == VIR_ERR_OK) {
-virErrorPtr err = virSaveLastError();
-
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
-   _("QEMU monitor was closed"));
-virCopyLastError(>lastError);
-if (err) {
-virSetError(err);
-virFreeError(err);
-} else {
-virResetLastError();
-}
-}
 mon->msg->finished = 1;
 virCondSignal(>notify);
 }
@@ -1047,6 +1034,12 @@ qemuMonitorSend(qemuMonitorPtr mon,
 {
 int ret = -1;
 
+if (mon->fd < 0) {
+virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+   _("QEMU monitor was closed"));
+return -1;
+}
+
 /* Check whether qemu quit unexpectedly */
 if (mon->lastError.code != VIR_ERR_OK) {
 VIR_DEBUG("Attempt to send command while error is set %s",
@@ -1070,6 +1063,12 @@ qemuMonitorSend(qemuMonitorPtr mon,
 }
 }
 
+if (mon->fd < 0) {
+virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+   _("QEMU monitor was closed"));
+goto cleanup;
+}
+
 if (mon->lastError.code != VIR_ERR_OK) {
 VIR_DEBUG("Send command resulted in error %s",
   NULLSTR(mon->lastError.message));
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/4] libvirt: introduce hypervisor driver shutdown function

[Nikolay Shirokovskiy]

This function is called by daemon before shutting down netdaemon
threads that serves client requests to make sure all these threads
will be able to shutdown.
---
 daemon/libvirtd.c|  2 ++
 src/driver-state.h   |  4 
 src/libvirt.c| 18 ++
 src/libvirt_internal.h   |  1 +
 src/libvirt_private.syms |  1 +
 5 files changed, 26 insertions(+)

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 589b321..d2bbe1e 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -1504,6 +1504,8 @@ int main(int argc, char **argv) {
 virObjectUnref(lxcProgram);
 virObjectUnref(qemuProgram);
 virObjectUnref(adminProgram);
+if (driversInitialized)
+virStateShutdown();
 virNetDaemonClose(dmn);
 virObjectUnref(srv);
 virObjectUnref(srvAdm);
diff --git a/src/driver-state.h b/src/driver-state.h
index 1cb3e4f..ea549a7 100644
--- a/src/driver-state.h
+++ b/src/driver-state.h
@@ -42,6 +42,9 @@ typedef int
 typedef int
 (*virDrvStateStop)(void);
 
+typedef void
+(*virDrvStateShutdown)(void);
+
 typedef struct _virStateDriver virStateDriver;
 typedef virStateDriver *virStateDriverPtr;
 
@@ -52,6 +55,7 @@ struct _virStateDriver {
 virDrvStateCleanup stateCleanup;
 virDrvStateReload stateReload;
 virDrvStateStop stateStop;
+virDrvStateShutdown stateShutdown;
 };
 
 
diff --git a/src/libvirt.c b/src/libvirt.c
index 536d56f..330c5ce 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -812,6 +812,24 @@ virStateCleanup(void)
 
 
 /**
+ * virStateShutdown:
+ *
+ * Run each virtualization driver's shutdown method.
+ *
+ */
+void
+virStateShutdown(void)
+{
+int r;
+
+for (r = virStateDriverTabCount - 1; r >= 0; r--) {
+if (virStateDriverTab[r]->stateShutdown)
+virStateDriverTab[r]->stateShutdown();
+}
+}
+
+
+/**
  * virStateReload:
  *
  * Run each virtualization driver's reload method.
diff --git a/src/libvirt_internal.h b/src/libvirt_internal.h
index 62f490a..9863b07 100644
--- a/src/libvirt_internal.h
+++ b/src/libvirt_internal.h
@@ -36,6 +36,7 @@ int virStateInitialize(bool privileged,
 int virStateCleanup(void);
 int virStateReload(void);
 int virStateStop(void);
+void virStateShutdown(void);
 
 /* Feature detection.  This is a libvirt-private interface for determining
  * what features are supported by the driver.
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 448d962..84af751 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1194,6 +1194,7 @@ virSetSharedStorageDriver;
 virStateCleanup;
 virStateInitialize;
 virStateReload;
+virStateShutdown;
 virStateStop;
 virStreamInData;
 
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/4] qemu: implement state driver shutdown function

[Nikolay Shirokovskiy]

Shutdown function should help API calls to finish when
event loop is not running anymore. For this reason let's
close agent and qemu monitors. These function will unblock
API calls wating for response from qemu process or qemu agent.

Closing agent monitor and setting priv->agent to NULL when
waiting for response is normal usecase (handling EOF from
agent is handled the same way for example).

However we can not do the same for qemu monitor. This monitor is normally
closed and unrefed during qemuProcessStop under destroy job so other threads
do not deal with priv->mon. But if we take extra reference to monitor
we are good. This can lead to double close but this function looks like to
be idempotent.
---
 src/qemu/qemu_driver.c | 39 +++
 1 file changed, 39 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 32a416f..c9adb58 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1067,6 +1067,44 @@ qemuStateStop(void)
 return ret;
 }
 
+
+static int
+qemuDomainDisconnect(virDomainObjPtr vm, void *opaque ATTRIBUTE_UNUSED)
+{
+
+qemuDomainObjPrivatePtr priv;
+
+virObjectLock(vm);
+priv = vm->privateData;
+
+if (priv->mon) {
+/* Take extra reference to monitor so it won't be disposed
+ * by qemuMonitorClose last unref. */
+virObjectRef(priv->mon);
+qemuMonitorClose(priv->mon);
+}
+
+if (priv->agent) {
+/* Other threads are ready for priv->agent to became NULL meanwhile */
+qemuAgentClose(priv->agent);
+priv->agent = NULL;
+}
+
+virObjectUnlock(vm);
+return 0;
+}
+
+
+static void
+qemuStateShutdown(void)
+{
+if (!qemu_driver)
+return;
+
+virDomainObjListForEach(qemu_driver->domains, qemuDomainDisconnect, NULL);
+}
+
+
 /**
  * qemuStateCleanup:
  *
@@ -21286,6 +21324,7 @@ static virStateDriver qemuStateDriver = {
 .stateCleanup = qemuStateCleanup,
 .stateReload = qemuStateReload,
 .stateStop = qemuStateStop,
+.stateShutdown = qemuStateShutdown,
 };
 
 int qemuRegister(void)
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 0/4] libvirtd: fix hang on termination in qemu driver

[Nikolay Shirokovskiy]

Libvirtd termination can hang. For example if some API call in qemu
driver awaiting monitor response it will never finish because event
loop does not functional during termination. As a result we hang
in virNetDaemonClose call during termination as this call finishes RPC
threads.

Let's ask hypervisor drivers to finish all API calls by calling
introduced state driver shutdown function before call to virNetDaemonClose.

Nikolay Shirokovskiy (4):
  libvirt: introduce hypervisor driver shutdown function
  qemu: implement state driver shutdown function
  qemu: agent: fix monitor close during first sync
  qemu: monitor: check monitor not closed upon send

 daemon/libvirtd.c|  2 ++
 src/driver-state.h   |  4 
 src/libvirt.c| 18 ++
 src/libvirt_internal.h   |  1 +
 src/libvirt_private.syms |  1 +
 src/qemu/qemu_agent.c| 14 +++---
 src/qemu/qemu_driver.c   | 39 +++
 src/qemu/qemu_monitor.c  | 27 +--
 8 files changed, 85 insertions(+), 21 deletions(-)

-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 3/4] qemu: agent: fix monitor close during first sync

[Nikolay Shirokovskiy]

Normally if first agent sync is failed we retry. First sync
can also be failed due to agent was closed. In this case we should
fail sync otherwise second attempt will hang.
---
 src/qemu/qemu_agent.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 5d125c4..e1440ec 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -987,17 +987,17 @@ qemuAgentGuestSync(qemuAgentPtr mon)
 goto cleanup;
 
 if (!sync_msg.rxObject) {
-if (sync_msg.first) {
+if (!mon->running) {
+virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s",
+   _("Guest agent disappeared while executing 
command"));
+goto cleanup;
+} else if (sync_msg.first) {
 VIR_FREE(sync_msg.txBuffer);
 memset(_msg, 0, sizeof(sync_msg));
 goto retry;
 } else {
-if (mon->running)
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Missing monitor reply object"));
-else
-virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s",
-   _("Guest agent disappeared while executing 
command"));
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Missing monitor reply object"));
 goto cleanup;
 }
 }
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 02/22] conf: Add usability blockers to virDomainCapsCPUModel

[Marc Hartmayer]

On Fri, Oct 13, 2017 at 08:14 PM +0200, Jiri Denemark  
wrote:
> When a hypervisor marks a CPU model as unusable on the current host, it
> may also give us a list of features which prevent the model from being
> usable. Storing this list in virDomainCapsCPUModel will help the CPU
> driver with creating a host-model CPU configuration.
>
> Signed-off-by: Jiri Denemark 
> Reviewed-by: John Ferlan 
> ---
>
> Notes:
> Version 2:
> - no change
>
>  src/conf/domain_capabilities.c | 30 ++
>  src/conf/domain_capabilities.h |  7 +--
>  src/qemu/qemu_capabilities.c   | 11 ++-
>  tests/domaincapstest.c |  6 +++---
>  4 files changed, 36 insertions(+), 18 deletions(-)
>
> diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
> index f62038b96c..be34576204 100644
> --- a/src/conf/domain_capabilities.c
> +++ b/src/conf/domain_capabilities.c
> @@ -163,7 +163,8 @@ virDomainCapsCPUModelsCopy(virDomainCapsCPUModelsPtr old)
>  for (i = 0; i < old->nmodels; i++) {
>  if (virDomainCapsCPUModelsAdd(cpuModels,
>old->models[i].name, -1,
> -  old->models[i].usable) < 0)
> +  old->models[i].usable,
> +  old->models[i].blockers) < 0)
>  goto error;
>  }
>
> @@ -195,7 +196,8 @@ virDomainCapsCPUModelsFilter(virDomainCapsCPUModelsPtr 
> old,
>
>  if (virDomainCapsCPUModelsAdd(cpuModels,
>old->models[i].name, -1,
> -  old->models[i].usable) < 0)
> +  old->models[i].usable,
> +  old->models[i].blockers) < 0)
>  goto error;
>  }
>
> @@ -210,7 +212,8 @@ virDomainCapsCPUModelsFilter(virDomainCapsCPUModelsPtr 
> old,
>  int
>  virDomainCapsCPUModelsAddSteal(virDomainCapsCPUModelsPtr cpuModels,
> char **name,
> -   virDomainCapsCPUUsable usable)
> +   virDomainCapsCPUUsable usable,
> +   char ***blockers)
>  {
>  if (VIR_RESIZE_N(cpuModels->models, cpuModels->nmodels_max,
>   cpuModels->nmodels, 1) < 0)
> @@ -218,6 +221,10 @@ virDomainCapsCPUModelsAddSteal(virDomainCapsCPUModelsPtr 
> cpuModels,
>
>  cpuModels->models[cpuModels->nmodels].usable = usable;
>  VIR_STEAL_PTR(cpuModels->models[cpuModels->nmodels].name, *name);
> +
> +if (blockers)
> +VIR_STEAL_PTR(cpuModels->models[cpuModels->nmodels].blockers, 
> *blockers);
> +
>  cpuModels->nmodels++;
>  return 0;
>  }
> @@ -227,20 +234,27 @@ int
>  virDomainCapsCPUModelsAdd(virDomainCapsCPUModelsPtr cpuModels,
>const char *name,
>ssize_t nameLen,
> -  virDomainCapsCPUUsable usable)
> +  virDomainCapsCPUUsable usable,
> +  char **blockers)
>  {
> -char *copy = NULL;
> +char *nameCopy = NULL;
> +char **blockersCopy = NULL;
>
> -if (VIR_STRNDUP(copy, name, nameLen) < 0)
> +if (VIR_STRNDUP(nameCopy, name, nameLen) < 0)
>  goto error;
>
> -if (virDomainCapsCPUModelsAddSteal(cpuModels, , usable) < 0)
> +if (virStringListCopy(, (const char **)blockers) < 0)
> +goto error;
> +
> +if (virDomainCapsCPUModelsAddSteal(cpuModels, ,
> +   usable, ) < 0)
>  goto error;
>
>  return 0;
>
>   error:
> -VIR_FREE(copy);
> +VIR_FREE(nameCopy);
> +virStringListFree(blockersCopy);
>  return -1;
>  }
>
> diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h
> index 82183c4524..8c71dec21e 100644
> --- a/src/conf/domain_capabilities.h
> +++ b/src/conf/domain_capabilities.h
> @@ -116,6 +116,7 @@ typedef virDomainCapsCPUModel *virDomainCapsCPUModelPtr;
>  struct _virDomainCapsCPUModel {
>  char *name;
>  virDomainCapsCPUUsable usable;
> +char **blockers; /* NULL-terminated list of usability blockers */
>  };

I know this is an "old" thread and already pushed. But I think you have
to free the blockers list in virDomainCapsCPUModelsDispose as well. No?

>
>  typedef struct _virDomainCapsCPUModels virDomainCapsCPUModels;
> @@ -171,11 +172,13 @@ virDomainCapsCPUModelsPtr 
> virDomainCapsCPUModelsFilter(virDomainCapsCPUModelsPtr
> const char 
> **blacklist);
>  int virDomainCapsCPUModelsAddSteal(virDomainCapsCPUModelsPtr cpuModels,
> char **name,
> -   virDomainCapsCPUUsable usable);
> +   virDomainCapsCPUUsable usable,
> +   

Re: [libvirt] [PATCH v3 1/2] libvirtd: fix crash on termination

[John Ferlan]

On 10/25/2017 04:24 AM, Nikolay Shirokovskiy wrote:
> 
> 
> On 25.10.2017 11:07, John Ferlan wrote:
>>
>>
>> On 10/24/2017 06:47 AM, Nikolay Shirokovskiy wrote:
>>> The problem is incorrect order of qemu driver shutdown and shutdown
>>> of netserver threads that serve client requests (thru qemu driver
>>> particularly).
>>>
>>> Net server threads are shutdowned upon dispose which is triggered
>>> by last daemon object unref at the end of main function. At the same
>>> time qemu driver is shutdowned earlier in virStateCleanup. As a result
>>> netserver threads see invalid driver object in the middle of request
>>> processing.
>>>
>>> Let's move shutting down netserver threads earlier to virNetDaemonClose.
>>>
>>> Note: order of last daemon unref and virStateCleanup
>>> is introduced in 85c3a182 for a valid reason.
>>> ---
>>>
>>> One can use next patch to trigger crash on termination. Call domstats 
>>> function 
>>> and then send TERM to libvirtd.
>>>
>>> Patch to trigger crash
>>> #   diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> #   index cf5e4ad..39a57aa 100644
>>> #   --- a/src/qemu/qemu_driver.c
>>> #   +++ b/src/qemu/qemu_driver.c
>>> #   @@ -20144,6 +20144,8 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
>>> #domflags = 0;
>>> #vm = vms[i];
>>> #
>>> #   +sleep(5);
>>> #   +
>>> #virObjectLock(vm);
>>> #
>>> #if (HAVE_JOB(privflags) &&
>>>
>>>  src/rpc/virnetdaemon.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
>>> index e3b9390..495bc4b 100644
>>> --- a/src/rpc/virnetdaemon.c
>>> +++ b/src/rpc/virnetdaemon.c
>>> @@ -880,6 +880,8 @@ virNetDaemonClose(virNetDaemonPtr dmn)
>>>  virObjectLock(dmn);
>>>  
>>>  virHashForEach(dmn->servers, daemonServerClose, NULL);
>>> +virHashFree(dmn->servers);
>>
>> Should this be virHashRemoveAll?
> 
> Yep, it would be better.
> 
> I guess no need to resend the series...

I can change it locally before pushing - just wanted to check

John

> 
> Nikolay
> 
>>
>> Or I wonder if the virHashFree in virNetDaemonDispose.
>>
>> John
>>
>> (I'm at KVM Forum this week so responses will be delayed)
>>
>>> +dmn->servers = NULL;
>>>  
>>>  virObjectUnlock(dmn);
>>>  }
>>>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 1/2] libvirtd: fix crash on termination

[Nikolay Shirokovskiy]

On 25.10.2017 11:07, John Ferlan wrote:
> 
> 
> On 10/24/2017 06:47 AM, Nikolay Shirokovskiy wrote:
>> The problem is incorrect order of qemu driver shutdown and shutdown
>> of netserver threads that serve client requests (thru qemu driver
>> particularly).
>>
>> Net server threads are shutdowned upon dispose which is triggered
>> by last daemon object unref at the end of main function. At the same
>> time qemu driver is shutdowned earlier in virStateCleanup. As a result
>> netserver threads see invalid driver object in the middle of request
>> processing.
>>
>> Let's move shutting down netserver threads earlier to virNetDaemonClose.
>>
>> Note: order of last daemon unref and virStateCleanup
>> is introduced in 85c3a182 for a valid reason.
>> ---
>>
>> One can use next patch to trigger crash on termination. Call domstats 
>> function 
>> and then send TERM to libvirtd.
>>
>> Patch to trigger crash
>> #   diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> #   index cf5e4ad..39a57aa 100644
>> #   --- a/src/qemu/qemu_driver.c
>> #   +++ b/src/qemu/qemu_driver.c
>> #   @@ -20144,6 +20144,8 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
>> #domflags = 0;
>> #vm = vms[i];
>> #
>> #   +sleep(5);
>> #   +
>> #virObjectLock(vm);
>> #
>> #if (HAVE_JOB(privflags) &&
>>
>>  src/rpc/virnetdaemon.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
>> index e3b9390..495bc4b 100644
>> --- a/src/rpc/virnetdaemon.c
>> +++ b/src/rpc/virnetdaemon.c
>> @@ -880,6 +880,8 @@ virNetDaemonClose(virNetDaemonPtr dmn)
>>  virObjectLock(dmn);
>>  
>>  virHashForEach(dmn->servers, daemonServerClose, NULL);
>> +virHashFree(dmn->servers);
> 
> Should this be virHashRemoveAll?

Yep, it would be better.

I guess no need to resend the series...

Nikolay

> 
> Or I wonder if the virHashFree in virNetDaemonDispose.
> 
> John
> 
> (I'm at KVM Forum this week so responses will be delayed)
> 
>> +dmn->servers = NULL;
>>  
>>  virObjectUnlock(dmn);
>>  }
>>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 1/2] libvirtd: fix crash on termination

[John Ferlan]

On 10/24/2017 06:47 AM, Nikolay Shirokovskiy wrote:
> The problem is incorrect order of qemu driver shutdown and shutdown
> of netserver threads that serve client requests (thru qemu driver
> particularly).
> 
> Net server threads are shutdowned upon dispose which is triggered
> by last daemon object unref at the end of main function. At the same
> time qemu driver is shutdowned earlier in virStateCleanup. As a result
> netserver threads see invalid driver object in the middle of request
> processing.
> 
> Let's move shutting down netserver threads earlier to virNetDaemonClose.
> 
> Note: order of last daemon unref and virStateCleanup
> is introduced in 85c3a182 for a valid reason.
> ---
> 
> One can use next patch to trigger crash on termination. Call domstats 
> function 
> and then send TERM to libvirtd.
> 
> Patch to trigger crash
> #   diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> #   index cf5e4ad..39a57aa 100644
> #   --- a/src/qemu/qemu_driver.c
> #   +++ b/src/qemu/qemu_driver.c
> #   @@ -20144,6 +20144,8 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
> #domflags = 0;
> #vm = vms[i];
> #
> #   +sleep(5);
> #   +
> #virObjectLock(vm);
> #
> #if (HAVE_JOB(privflags) &&
> 
>  src/rpc/virnetdaemon.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
> index e3b9390..495bc4b 100644
> --- a/src/rpc/virnetdaemon.c
> +++ b/src/rpc/virnetdaemon.c
> @@ -880,6 +880,8 @@ virNetDaemonClose(virNetDaemonPtr dmn)
>  virObjectLock(dmn);
>  
>  virHashForEach(dmn->servers, daemonServerClose, NULL);
> +virHashFree(dmn->servers);

Should this be virHashRemoveAll?

Or I wonder if the virHashFree in virNetDaemonDispose.

John

(I'm at KVM Forum this week so responses will be delayed)

> +dmn->servers = NULL;
>  
>  virObjectUnlock(dmn);
>  }
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 REBASE 0/2] qemu: report block job errors from qemu to the user

[Nikolay Shirokovskiy]

ping

On 08.09.2017 10:59, Nikolay Shirokovskiy wrote:
> So that you can see nice report on migration:
> 
> "error: operation failed: migration of disk sda failed: No space left on 
> device"
> 
> diff from v2:
> 
> 1. split into 2 patches
> 2. change formal documentation where it is present accordingly
> 3. add variable initialization for safety
> 
> Nikolay Shirokovskiy (2):
>   qemu: prepare blockjob complete event error usage
>   qemu: report drive mirror errors on migration
> 
>  src/qemu/qemu_blockjob.c | 14 +--
>  src/qemu/qemu_blockjob.h |  3 ++-
>  src/qemu/qemu_domain.c   |  1 +
>  src/qemu/qemu_domain.h   |  1 +
>  src/qemu/qemu_driver.c   |  4 ++--
>  src/qemu/qemu_migration.c| 55 
> +++-
>  src/qemu/qemu_monitor.c  |  5 ++--
>  src/qemu/qemu_monitor.h  |  4 +++-
>  src/qemu/qemu_monitor_json.c |  4 +++-
>  src/qemu/qemu_process.c  |  4 
>  10 files changed, 70 insertions(+), 25 deletions(-)
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list