Re: [libvirt] [PATCH v5 2/3] nwfilter: Convert _virNWFilterObjList to use virObjectRWLockable

2018-02-12 Thread Michal Privoznik
On 02/08/2018 02:34 PM, John Ferlan wrote:
> 
> 
> On 02/08/2018 08:13 AM, Michal Privoznik wrote:
>> On 02/06/2018 08:20 PM, John Ferlan wrote:
>>> Implement the self locking object list for nwfilter object lists
>>> that uses two hash tables to store the nwfilter object by UUID or
>>> by Name.
>>>
>>> As part of this alter the uuid argument to virNWFilterObjLookupByUUID
>>> to expect an already formatted uuidstr.
>>>
>>> Alter the existing list traversal code to implement the hash table
>>> find/lookup/search functionality.
>>>
>>> Signed-off-by: John Ferlan 
>>> ---
>>>  src/conf/virnwfilterobj.c  | 405 
>>> -
>>>  src/conf/virnwfilterobj.h  |   2 +-
>>>  src/nwfilter/nwfilter_driver.c |   5 +-
>>>  3 files changed, 283 insertions(+), 129 deletions(-)
>>
>>
>>> @@ -425,24 +483,26 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr 
>>> nwfilters,
>>>  return NULL;
>>>  }
>>>  
>>> -
>>> -/* Get a READ lock and immediately promote to WRITE while we adjust
>>> - * data within. */
>>> -if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
>>> +/* We're about to make some changes to objects on the list - so get the
>>> + * list READ lock in order to Find the object and WRITE lock the object
>>> + * since both paths would immediately promote it anyway */
>>> +virObjectRWLockRead(nwfilters);
>>> +if ((obj = virNWFilterObjListFindByNameLocked(nwfilters, def->name))) {
>>> +virObjectRWLockWrite(obj);
>>> +virObjectRWUnlock(nwfilters);
>>>  
>>>  objdef = obj->def;
>>>  if (virNWFilterDefEqual(def, objdef)) {
>>> -virNWFilterObjPromoteToWrite(obj);
>>>  virNWFilterDefFree(objdef);
>>>  obj->def = def;
>>>  virNWFilterObjDemoteFromWrite(obj);
>>>  return obj;
>>>  }
>>>  
>>> -/* Set newDef and run the trigger with a demoted lock since it may 
>>> need
>>> - * to grab a read lock on this object and promote before 
>>> returning. */
>>> -virNWFilterObjPromoteToWrite(obj);
>>>  obj->newDef = def;
>>> +
>>> +/* Demote while the trigger runs since it may need to grab a read
>>> + * lock on this object and promote before returning. */
>>>  virNWFilterObjDemoteFromWrite(obj);
>>>  
>>>  /* trigger the update on VMs referencing the filter */
>>> @@ -462,39 +522,113 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr 
>>> nwfilters,
>>>  return obj;
>>>  }
>>>  
>>> +/* Promote the nwfilters to add a new object */
>>> +virObjectRWUnlock(nwfilters);
>>> +virObjectRWLockWrite(nwfilters);
>>
>> How can this work? What if there's another thread waiting for the write
>> lock (e.g. just to define an NWFilter)? The Unlock(nwfilters) wakes up
>> the other thread, it does its job, unlocks the list waking us in turn.
>> So we lock @nwfilters for writing and add something into the hash table
>> - without all the checks (e.g. duplicity check) done earlier.
>>
> 
> I dunno - works in the tests I've run (libvirt-tck and avocado).
> 

Oh, now I see why. The nwfilterDefineXML() API still grabs
nwfilterDriverLock() and hence there's nobody else wanting the write
lock since they are waiting on the driver lock. However, I think that
relying on updateLock (i.e. virNWFilterWriteLockFilterUpdates()) is enough.

Michal

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


Re: [libvirt] [PATCH v2] vcpupin: add clear feature

2018-02-12 Thread Daniel P . Berrangé
On Mon, Feb 12, 2018 at 03:54:21AM -0500, Yi Wang wrote:
> We can't clear vcpupin settings of XML once we did vcpupin
> command, this is not convenient under some condition such
> as migration to a host with less CPUs.
> 
> This patch introduces clear feature, which can clear vcpuin
> setting of XML using a 'c' option.
> 
> Signed-off-by: Yi Wang 
> Signed-off-by: Xi Xu 
> ---
>  include/libvirt/libvirt-domain.h | 11 +++
>  src/qemu/qemu_driver.c   | 32 

I'm not seeing a good reason for these change. There's no concept of clearing
CPU pinning - the default is simply "pinned" to all possible CPUs, and the
callers should already be able to request all possile CPUs with the current
API design.

>  tools/virsh-domain.c |  5 -
>  tools/virsh.pod  |  1 +
>  4 files changed, 40 insertions(+), 9 deletions(-)
> 
> diff --git a/include/libvirt/libvirt-domain.h 
> b/include/libvirt/libvirt-domain.h
> index 4048acf..46f4e77 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -1847,6 +1847,17 @@ int virDomainSetVcpusFlags  
> (virDomainPtr domain,
>  int virDomainGetVcpusFlags  (virDomainPtr domain,
>   unsigned int flags);
>  
> +/* Flags for controlling virtual CPU pinning.  */
> +typedef enum {
> +/* See virDomainModificationImpact for these flags.  */
> +VIR_DOMAIN_VCPU_PIN_CURRENT = VIR_DOMAIN_AFFECT_CURRENT,
> +VIR_DOMAIN_VCPU_PIN_LIVE= VIR_DOMAIN_AFFECT_LIVE,
> +VIR_DOMAIN_VCPU_PIN_CONFIG  = VIR_DOMAIN_AFFECT_CONFIG,
> +
> +/* Additionally, these flags may be bitwise-OR'd in.  */
> +VIR_DOMAIN_VCPU_PIN_CLEAR = (1 << 2), /* Clear vcpus pin info */
> +} virDomainVcpuPinFlags;
> +
>  int virDomainPinVcpu(virDomainPtr domain,
>   unsigned int vcpu,
>   unsigned char *cpumap,
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index bbce5bd..fe1f62f 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5056,7 +5056,8 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm,
>int vcpu,
>virQEMUDriverPtr driver,
>virQEMUDriverConfigPtr cfg,
> -  virBitmapPtr cpumap)
> +  virBitmapPtr cpumap,
> +  bool clear)
>  {
>  virBitmapPtr tmpmap = NULL;
>  virDomainVcpuDefPtr vcpuinfo;
> @@ -5069,6 +5070,7 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm,
>  int eventNparams = 0;
>  int eventMaxparams = 0;
>  int ret = -1;
> +virBitmapPtr targetMap = NULL;
>  
>  if (!qemuDomainHasVcpuPids(vm)) {
>  virReportError(VIR_ERR_OPERATION_INVALID,
> @@ -5083,10 +5085,15 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm,
>  goto cleanup;
>  }
>  
> -if (!(tmpmap = virBitmapNewCopy(cpumap)))
> -goto cleanup;
> +if (clear) {
> +targetMap = virHostCPUGetOnlineBitmap();
> +} else {
> +targetMap = cpumap;
> +if (!(tmpmap = virBitmapNewCopy(cpumap)))
> +goto cleanup;
> +}
>  
> -if (!(str = virBitmapFormat(cpumap)))
> +if (!(str = virBitmapFormat(targetMap)))
>  goto cleanup;
>  
>  if (vcpuinfo->online) {
> @@ -5095,11 +5102,11 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm,
>  if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_VCPU, 
> vcpu,
> false, _vcpu) < 0)
>  goto cleanup;
> -if (qemuSetupCgroupCpusetCpus(cgroup_vcpu, cpumap) < 0)
> +if (qemuSetupCgroupCpusetCpus(cgroup_vcpu, targetMap) < 0)
>  goto cleanup;
>  }
>  
> -if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, vcpu), cpumap) < 
> 0)
> +if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, vcpu), targetMap) 
> < 0)
>  goto cleanup;
>  }
>  
> @@ -5128,6 +5135,8 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm,
>  virCgroupFree(_vcpu);
>  VIR_FREE(str);
>  qemuDomainEventQueue(driver, event);
> +if (clear)
> +virBitmapFree(targetMap);
>  return ret;
>  }
>  
> @@ -5148,9 +5157,11 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
>  virBitmapPtr pcpumap = NULL;
>  virDomainVcpuDefPtr vcpuinfo = NULL;
>  virQEMUDriverConfigPtr cfg = NULL;
> +bool clear = false;
>  
>  virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> -  VIR_DOMAIN_AFFECT_CONFIG, -1);
> +  VIR_DOMAIN_AFFECT_CONFIG |
> +  VIR_DOMAIN_VCPU_PIN_CLEAR, -1);
>  
>  cfg = virQEMUDriverGetConfig(driver);
>  
> @@ -5166,6 +5177,8 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
>  if (virDomainObjGetDefs(vm, flags, , ) < 0)
>  goto 

Re: [libvirt] [PATCH] virlog: determine the hostname on startup CVE-2018-XXX

2018-02-12 Thread Daniel P . Berrangé
On Sat, Feb 10, 2018 at 07:06:22AM +0100, Michal Privoznik wrote:
> On 02/07/2018 02:13 PM, Daniel P. Berrangé wrote:
> > On Wed, Feb 07, 2018 at 09:58:21AM +0530, P J P wrote:
> >> +-- On Mon, 5 Feb 2018, Daniel P. Berrangé wrote --+
> >> | From: Lubomir Rintel 
> >> | 
> >> | At later point it might not be possible or even safe to use 
> >> getaddrinfo(). It
> >> | can in turn result in a load of NSS module.
> >> | 
> >> | Notably, on a LXC container startup we may find ourselves with the guest
> >> | filesystem already having replaced the host one. Loading a NSS module
> >> | from the guest tree could allow a malicous guest to escape the
> >> | confinement of its container environment because libvirt will not yet
> >> | have locked it down.
> >> | ---
> >> | 
> >> | NB, we're still awaiting CVE allocation before pushing to git
> >>
> >> 'CVE-2018-6764' has been assigned to this issue by Mitre.
> > 
> > Thanks, I have pushed this patch now
> > 
> 
> Actually, I think we need to revisit this patch. It makes
> gethostbyname() deadlock if libvirt NSS plugin is enabled. Here's the
> stack trace of what's going on (I've ran `getent hosts libvirt.org`):
> 
> #0  0x7f6e714b57e7 in futex_wait (private=0, expected=1, 
> futex_word=0x7f6e71f2fb80 ) at 
> ../sysdeps/unix/sysv/linux/futex-internal.h:61
> #1  futex_wait_simple (private=0, expected=1, futex_word=0x7f6e71f2fb80 
> ) at ../sysdeps/nptl/futex-internal.h:135
> #2  __pthread_once_slow (once_control=0x7f6e71f2fb80 , 
> init_routine=0x7f6e71d09949 ) at pthread_once.c:105
> #3  0x7f6e71d16e7d in virOnce (once=0x7f6e71f2fb80 , 
> init=0x7f6e71d09949 ) at util/virthread.c:48
> #4  0x7f6e71d0997c in virLogInitialize () at util/virlog.c:285
> #5  0x7f6e71d0a09a in virLogVMessage (source=0x7f6e71f2f130 , 
> priority=VIR_LOG_INFO, filename=0x7f6e71d2184e "util/virobject.c", 
> linenr=254, funcname=0x7f6e71d21a58 <__func__.7842> "virObjectNew", 
> metadata=0x0,
> fmt=0x7f6e71d218d8 "OBJECT_NEW: obj=%p classname=%s", 
> vargs=0x7ffe45fab6e0) at util/virlog.c:582
> #6  0x7f6e71d09ffd in virLogMessage (source=0x7f6e71f2f130 , 
> priority=VIR_LOG_INFO, filename=0x7f6e71d2184e "util/virobject.c", 
> linenr=254, funcname=0x7f6e71d21a58 <__func__.7842> "virObjectNew", 
> metadata=0x0,
> fmt=0x7f6e71d218d8 "OBJECT_NEW: obj=%p classname=%s") at util/virlog.c:542
> #7  0x7f6e71d0db22 in virObjectNew (klass=0x56348c51c500) at 
> util/virobject.c:254
> #8  0x7f6e71d0dbf1 in virObjectLockableNew (klass=0x56348c51c500) at 
> util/virobject.c:272
> #9  0x7f6e71d0d3e5 in virMacMapNew (file=0x56348c520e70 
> "/var/lib/libvirt/dnsmasq//virbr0.macs") at util/virmacmap.c:319
> #10 0x7f6e71cdc50a in findLease (name=0x7ffe45fac1d0 "burns", af=0, 
> address=0x7ffe45fab980, naddress=0x7ffe45fab988, found=0x7ffe45fab973, 
> errnop=0x7ffe45fabb88) at nss/libvirt_nss.c:301
> #11 0x7f6e71cdcc56 in _nss_libvirt_gethostbyname4_r (name=0x7ffe45fac1d0 
> "burns", pat=0x7ffe45fabb98, buffer=0x7ffe45fabd10 "& ", buflen=1024, 
> errnop=0x7ffe45fabb88, herrnop=0x7ffe45fabbb0, ttlp=0x0) at 
> nss/libvirt_nss.c:522
> #12 0x7f6e724631fc in gaih_inet (name=name@entry=0x7ffe45fac1d0 "burns", 
> service=, req=req@entry=0x7ffe45fac1a0, 
> pai=pai@entry=0x7ffe45fabc98, naddrs=naddrs@entry=0x7ffe45fabc94, 
> tmpbuf=tmpbuf@entry=0x7ffe45fabd00)
> at ../sysdeps/posix/getaddrinfo.c:825
> #13 0x7f6e72464697 in __GI_getaddrinfo (name=, 
> service=, hints=0x7ffe45fac1a0, pai=0x7ffe45fac198) at 
> ../sysdeps/posix/getaddrinfo.c:2370
> #14 0x7f6e71d19e81 in virGetHostnameImpl (quiet=true) at 
> util/virutil.c:717
> #15 0x7f6e71d1a057 in virGetHostnameQuiet () at util/virutil.c:759
> #16 0x7f6e71d09936 in virLogOnceInit () at util/virlog.c:279
> #17 0x7f6e71d09952 in virLogOnce () at util/virlog.c:285
> #18 0x7f6e714b5829 in __pthread_once_slow (once_control=0x7f6e71f2fb80 
> , init_routine=0x7f6e71d09949 ) at 
> pthread_once.c:116
> #19 0x7f6e71d16e7d in virOnce (once=0x7f6e71f2fb80 , 
> init=0x7f6e71d09949 ) at util/virthread.c:48
> #20 0x7f6e71d0997c in virLogInitialize () at util/virlog.c:285
> #21 0x7f6e71d0a09a in virLogVMessage (source=0x7f6e71f2f130 , 
> priority=VIR_LOG_INFO, filename=0x7f6e71d2184e "util/virobject.c", 
> linenr=254, funcname=0x7f6e71d21a58 <__func__.7842> "virObjectNew", 
> metadata=0x0,
> fmt=0x7f6e71d218d8 "OBJECT_NEW: obj=%p classname=%s", 
> vargs=0x7ffe45fac410) at util/virlog.c:582
> #22 0x7f6e71d09ffd in virLogMessage (source=0x7f6e71f2f130 , 
> priority=VIR_LOG_INFO, filename=0x7f6e71d2184e "util/virobject.c", 
> linenr=254, funcname=0x7f6e71d21a58 <__func__.7842> "virObjectNew", 
> metadata=0x0,
> fmt=0x7f6e71d218d8 "OBJECT_NEW: obj=%p classname=%s") at util/virlog.c:542
> #23 0x7f6e71d0db22 in virObjectNew (klass=0x56348c51c500) at 
> util/virobject.c:254
> #24 0x7f6e71d0dbf1 in virObjectLockableNew (klass=0x56348c51c500) at 
> util/virobject.c:272
> #25 

Re: [libvirt] [PATCH] virlog: Allow opting out from logging

2018-02-12 Thread Daniel P . Berrangé
On Sat, Feb 10, 2018 at 09:02:59AM +0100, Michal Privoznik wrote:
> After 759b4d1b0fe5f we are getting hostname in virLogOnceInit().
> Problem with this approach is in the NSS module because the
> module calls some internal APIs which occasionally want to log
> something. This results in virLogInitialize() to be called which
> in turn ends up calling virGetHostnameQuiet() and effectively the
> control gets to NSS plugin again which calls some internal APIs
> which occasionally want to log something. You can see the
> deadlock now.
> 
> One way out from this is to turn logging into no-op. Which kind
> of makes sense - the NSS module shouldn't log anything. To
> achieve this, the virLogVMessage() function is provided with
> separate no-op implementation if DISABLE_LOGGING_FOR_NSS macro is
> set.

It shouldn't log anything in normal usage, but I could see the
ability to turn on logging as useful if debugging something in
the NSS module.  So don't think we should compile it out.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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


[libvirt] [PATCH 3/6] virNWFilterObj: Turn into virObjectLockable

2018-02-12 Thread Michal Privoznik
The virNWFilterObj requires recursive locks, otherwise it is
regular virObject. So when creating the object we must call
virObjectRecursiveLockableNew(). Other than that, this is pure
replacement of virNWFilterObj*() APIs with thei virObject*()
counterparts.

Based-on-work-of: John Ferlan 
Signed-off-by: Michal Privoznik 
---
 src/conf/virnwfilterobj.c  | 121 -
 src/conf/virnwfilterobj.h  |   6 --
 src/libvirt_private.syms   |   2 -
 src/nwfilter/nwfilter_driver.c |  10 +--
 src/nwfilter/nwfilter_gentech_driver.c |  10 +--
 5 files changed, 67 insertions(+), 82 deletions(-)

diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
index 87d7e7270..a472ff531 100644
--- a/src/conf/virnwfilterobj.c
+++ b/src/conf/virnwfilterobj.c
@@ -34,7 +34,7 @@
 VIR_LOG_INIT("conf.virnwfilterobj");
 
 struct _virNWFilterObj {
-virMutex lock;
+virObjectLockable parent;
 
 bool wantRemoved;
 
@@ -42,32 +42,54 @@ struct _virNWFilterObj {
 virNWFilterDefPtr newDef;
 };
 
+static virClassPtr virNWFilterObjClass;
+static void virNWFilterObjDispose(void *obj);
+
 struct _virNWFilterObjList {
 size_t count;
 virNWFilterObjPtr *objs;
 };
 
+static int virNWFilterObjOnceInit(void)
+{
+if (!(virNWFilterObjClass = virClassNew(virClassForObjectLockable(),
+"virNWFilterObj",
+sizeof(virNWFilterObj),
+virNWFilterObjDispose)))
+return -1;
+
+return 0;
+}
+
+VIR_ONCE_GLOBAL_INIT(virNWFilterObj)
+
 
 static virNWFilterObjPtr
 virNWFilterObjNew(void)
 {
 virNWFilterObjPtr obj;
 
-if (VIR_ALLOC(obj) < 0)
+if (virNWFilterObjInitialize() < 0)
 return NULL;
 
-if (virMutexInitRecursive(>lock) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   "%s", _("cannot initialize mutex"));
-VIR_FREE(obj);
+if (!(obj = virObjectRecursiveLockableNew(virNWFilterObjClass)))
 return NULL;
-}
 
-virNWFilterObjLock(obj);
+virObjectLock(obj);
 return obj;
 }
 
 
+static void
+virNWFilterObjDispose(void *opaque)
+{
+virNWFilterObjPtr obj = opaque;
+
+virNWFilterDefFree(obj->def);
+virNWFilterDefFree(obj->newDef);
+}
+
+
 virNWFilterDefPtr
 virNWFilterObjGetDef(virNWFilterObjPtr obj)
 {
@@ -89,27 +111,12 @@ virNWFilterObjWantRemoved(virNWFilterObjPtr obj)
 }
 
 
-static void
-virNWFilterObjFree(virNWFilterObjPtr obj)
-{
-if (!obj)
-return;
-
-virNWFilterDefFree(obj->def);
-virNWFilterDefFree(obj->newDef);
-
-virMutexDestroy(>lock);
-
-VIR_FREE(obj);
-}
-
-
 void
 virNWFilterObjListFree(virNWFilterObjListPtr nwfilters)
 {
 size_t i;
 for (i = 0; i < nwfilters->count; i++)
-virNWFilterObjFree(nwfilters->objs[i]);
+virObjectUnref(nwfilters->objs[i]);
 VIR_FREE(nwfilters->objs);
 VIR_FREE(nwfilters);
 }
@@ -132,18 +139,18 @@ virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters,
 {
 size_t i;
 
-virNWFilterObjUnlock(obj);
+virObjectUnlock(obj);
 
 for (i = 0; i < nwfilters->count; i++) {
-virNWFilterObjLock(nwfilters->objs[i]);
+virObjectLock(nwfilters->objs[i]);
 if (nwfilters->objs[i] == obj) {
-virNWFilterObjUnlock(nwfilters->objs[i]);
-virNWFilterObjFree(nwfilters->objs[i]);
+virObjectUnlock(nwfilters->objs[i]);
+virObjectUnref(nwfilters->objs[i]);
 
 VIR_DELETE_ELEMENT(nwfilters->objs, i, nwfilters->count);
 break;
 }
-virNWFilterObjUnlock(nwfilters->objs[i]);
+virObjectUnlock(nwfilters->objs[i]);
 }
 }
 
@@ -158,11 +165,11 @@ virNWFilterObjListFindByUUID(virNWFilterObjListPtr 
nwfilters,
 
 for (i = 0; i < nwfilters->count; i++) {
 obj = nwfilters->objs[i];
-virNWFilterObjLock(obj);
+virObjectLock(obj);
 def = obj->def;
 if (!memcmp(def->uuid, uuid, VIR_UUID_BUFLEN))
 return obj;
-virNWFilterObjUnlock(obj);
+virObjectUnlock(obj);
 }
 
 return NULL;
@@ -179,11 +186,11 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr 
nwfilters,
 
 for (i = 0; i < nwfilters->count; i++) {
 obj = nwfilters->objs[i];
-virNWFilterObjLock(obj);
+virObjectLock(obj);
 def = obj->def;
 if (STREQ_NULLABLE(def->name, name))
 return obj;
-virNWFilterObjUnlock(obj);
+virObjectUnlock(obj);
 }
 
 return NULL;
@@ -205,7 +212,7 @@ 
virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters,
 if (virNWFilterObjWantRemoved(obj)) {
 virReportError(VIR_ERR_NO_NWFILTER,
_("Filter '%s' is in use."), filtername);
-virNWFilterObjUnlock(obj);
+virObjectUnlock(obj);
 return NULL;
   

[libvirt] [PATCH 4/6] conf: Introduce and use virNWFilterObjEndAPI

2018-02-12 Thread Michal Privoznik
Have every API that is getting a virNWFilterObj from
virNWFilterObjList grab a reference and at the same time wrap
Unlock() + Unref() calls into virNWFilterObjEndAPI().

Based-on-work-of: John Ferlan 
Signed-off-by: Michal Privoznik 
---
 src/conf/virnwfilterobj.c  | 38 +-
 src/conf/virnwfilterobj.h  |  3 +++
 src/libvirt_private.syms   |  1 +
 src/nwfilter/nwfilter_driver.c | 14 +
 src/nwfilter/nwfilter_gentech_driver.c | 11 +-
 5 files changed, 38 insertions(+), 29 deletions(-)

diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
index a472ff531..6a54628b6 100644
--- a/src/conf/virnwfilterobj.c
+++ b/src/conf/virnwfilterobj.c
@@ -90,6 +90,18 @@ virNWFilterObjDispose(void *opaque)
 }
 
 
+void
+virNWFilterObjEndAPI(virNWFilterObjPtr *obj)
+{
+if (!*obj)
+return;
+
+virObjectUnlock(*obj);
+virObjectUnref(*obj);
+*obj = NULL;
+}
+
+
 virNWFilterDefPtr
 virNWFilterObjGetDef(virNWFilterObjPtr obj)
 {
@@ -144,8 +156,7 @@ virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters,
 for (i = 0; i < nwfilters->count; i++) {
 virObjectLock(nwfilters->objs[i]);
 if (nwfilters->objs[i] == obj) {
-virObjectUnlock(nwfilters->objs[i]);
-virObjectUnref(nwfilters->objs[i]);
+virNWFilterObjEndAPI(>objs[i]);
 
 VIR_DELETE_ELEMENT(nwfilters->objs, i, nwfilters->count);
 break;
@@ -168,7 +179,7 @@ virNWFilterObjListFindByUUID(virNWFilterObjListPtr 
nwfilters,
 virObjectLock(obj);
 def = obj->def;
 if (!memcmp(def->uuid, uuid, VIR_UUID_BUFLEN))
-return obj;
+return virObjectRef(obj);
 virObjectUnlock(obj);
 }
 
@@ -189,7 +200,7 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr 
nwfilters,
 virObjectLock(obj);
 def = obj->def;
 if (STREQ_NULLABLE(def->name, name))
-return obj;
+return virObjectRef(obj);
 virObjectUnlock(obj);
 }
 
@@ -212,7 +223,7 @@ 
virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters,
 if (virNWFilterObjWantRemoved(obj)) {
 virReportError(VIR_ERR_NO_NWFILTER,
_("Filter '%s' is in use."), filtername);
-virObjectUnlock(obj);
+virNWFilterObjEndAPI();
 return NULL;
 }
 
@@ -247,7 +258,7 @@ _virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr 
nwfilters,
 if (obj) {
 rc = _virNWFilterObjListDefLoopDetect(nwfilters, obj->def,
   filtername);
-virObjectUnlock(obj);
+virNWFilterObjEndAPI();
 if (rc < 0)
 break;
 }
@@ -329,10 +340,10 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr 
nwfilters,
_("filter with same UUID but different name "
  "('%s') already exists"),
objdef->name);
-virObjectUnlock(obj);
+virNWFilterObjEndAPI();
 return NULL;
 }
-virObjectUnlock(obj);
+virNWFilterObjEndAPI();
 } else {
 if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
 char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -342,7 +353,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
 virReportError(VIR_ERR_OPERATION_FAILED,
_("filter '%s' already exists with uuid %s"),
def->name, uuidstr);
-virObjectUnlock(obj);
+virNWFilterObjEndAPI();
 return NULL;
 }
 }
@@ -367,7 +378,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
 /* trigger the update on VMs referencing the filter */
 if (virNWFilterTriggerVMFilterRebuild() < 0) {
 obj->newDef = NULL;
-virObjectUnlock(obj);
+virNWFilterObjEndAPI();
 return NULL;
 }
 
@@ -382,10 +393,10 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr 
nwfilters,
 
 if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs,
 nwfilters->count, obj) < 0) {
-virObjectUnlock(obj);
-virObjectUnref(obj);
+virNWFilterObjEndAPI();
 return NULL;
 }
+virObjectRef(obj);
 obj->def = def;
 
 return obj;
@@ -558,8 +569,7 @@ virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr 
nwfilters,
 continue;
 
 obj = virNWFilterObjListLoadConfig(nwfilters, configDir, 
entry->d_name);
-if (obj)
-virObjectUnlock(obj);
+virNWFilterObjEndAPI();
 }
 
 VIR_DIR_CLOSE(dir);
diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h
index 9faba264a..0281bc5f5 100644
--- a/src/conf/virnwfilterobj.h
+++ 

Re: [libvirt] [PATCH] virutil: Introduce virGetHostnameSimple()

2018-02-12 Thread Michal Privoznik
On 02/12/2018 11:42 AM, Daniel P. Berrangé wrote:
> On Mon, Feb 12, 2018 at 11:29:21AM +0100, Michal Privoznik wrote:
>> After 759b4d1b0fe5f we are getting hostname in virLogOnceInit().
>> Problem with this approach is in the NSS module because the
>> module calls some internal APIs which occasionally want to log
>> something. This results in virLogInitialize() to be called which
>> in turn ends up calling virGetHostnameQuiet() and effectively the
>> control gets to NSS plugin again which calls some internal APIs
>> which occasionally want to log something. You can see the
>> deadlock now.
>>
>> One way out of this is to call only gethostname() and not whole
>> virGetHostnameQuiet() machinery.
> 
> The extra bits in virGetHostname() only exist for the sake of
> the QEMU migration code. The source call gethostname() on the
> target host and wants to make sure it doesn't return "localhost"
> or something that resolves to "127.0.0.1", otherwise the source
> host would end up migrating to itself instead of the actual
> target host.  We should really just move that extra stuff into
> the migration code and leave virGetHostname() simple, instead
> of having a virGetHostnameSimple(). That's more than I would
> want todo for this CVE fix though, as it would complicate the
> backporting. So I feel my patch to inline hostname() call in
> the logging code is more suitable in short term, but after
> that we could do a big refactor.

Okay, lets got with your version then.

Michal

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

[libvirt] [PATCH 1/6] virObject: use virReportSystemError if applicable

2018-02-12 Thread Michal Privoznik
When initializing a mutex (either regular or RW) the virMutexInit() and
virRWLockInit() functions set errno and return -1. It's a pity we don't
use virReportSystemError() in that case rather plain virReportError().

Signed-off-by: Michal Privoznik 
---
 src/util/virobject.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/util/virobject.c b/src/util/virobject.c
index 1723df6b2..b2fc63aec 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -273,8 +273,8 @@ virObjectLockableNew(virClassPtr klass)
 return NULL;
 
 if (virMutexInit(>lock) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Unable to initialize mutex"));
+virReportSystemError(errno, "%s",
+ _("Unable to initialize mutex"));
 virObjectUnref(obj);
 return NULL;
 }
@@ -299,8 +299,8 @@ virObjectRWLockableNew(virClassPtr klass)
 return NULL;
 
 if (virRWLockInit(>lock) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Unable to initialize RW lock"));
+virReportSystemError(errno, "%s",
+ _("Unable to initialize RW lock"));
 virObjectUnref(obj);
 return NULL;
 }
-- 
2.13.6

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


Re: [libvirt] [PATCH v3 08/15] conf: expand network device callbacks to cover resolving NIC type

2018-02-12 Thread Daniel P . Berrangé
On Fri, Feb 09, 2018 at 05:09:36PM -0500, John Ferlan wrote:
> 
> 
> On 02/05/2018 10:28 AM, Daniel P. Berrangé wrote:
> > Currently the QEMU driver will call directly into the network driver
> > impl to modify resolve the atual type of NICs with type=network. It
> > has todo this before it has allocated the actual NIC. This introduces
> > a callback system to allow us to decouple the QEMU driver from the
> > network driver.
> > 
> > This is a short term step, as it ought to be possible to achieve the
> > same end goal by simply querying XML via the public network API. The
> > QEMU code in question though, has no virConnectPtr conveniently
> > available at this time.
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  src/conf/domain_conf.c | 17 -
> >  src/conf/domain_conf.h | 15 ++-
> >  src/libvirt_private.syms   |  1 +
> >  src/network/bridge_driver.c| 10 +-
> >  src/network/bridge_driver.h|  5 -
> >  src/qemu/qemu_alias.c  |  3 +--
> >  src/qemu/qemu_domain_address.c |  3 +--
> >  tests/Makefile.am  | 11 +--
> >  tests/qemuxml2argvtest.c   |  4 
> >  9 files changed, 51 insertions(+), 18 deletions(-)
> > 
> 
> I realized while reviewing Andrea's changes just now that the
> xml2argvtest is missing!  and I think I know why...
> 
> [...]
> 
> > diff --git a/tests/Makefile.am b/tests/Makefile.am
> > index 497bd21a25..d013aed5eb 100644
> > --- a/tests/Makefile.am
> > +++ b/tests/Makefile.am
> > @@ -280,7 +280,7 @@ test_libraries += virmocklibxl.la
> >  endif WITH_LIBXL
> >  
> >  if WITH_QEMU
> > -test_programs += qemuxml2argvtest qemuxml2xmltest \
> 
> hmmm...
> 
> > +test_programs += qemuxml2xmltest \
> > qemuargv2xmltest qemuhelptest domainsnapshotxml2xmltest \
> > qemumonitortest qemumonitorjsontest qemuhotplugtest \
> > qemuagenttest qemucapabilitiestest qemucaps2xmltest \
> > @@ -288,6 +288,11 @@ test_programs += qemuxml2argvtest qemuxml2xmltest \
> > qemucommandutiltest \
> > qemublocktest \
> > $(NULL)
> > +if WITH_NETWORK
> > +# Dep on the network driver callback for resolving NIC
> > +# actual type. XXX remove this dep.
> > +test_programs += qemuxml2xmltest
> 
> Was this supposed to be += qemuxml2argvtest  ??

Sigh, rebase / merge error :-(


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH v5 2/3] nwfilter: Convert _virNWFilterObjList to use virObjectRWLockable

2018-02-12 Thread Michal Privoznik
On 02/09/2018 12:47 PM, Stefan Berger wrote:
> On 02/09/2018 01:48 AM, Michal Privoznik wrote:
>> On 02/08/2018 10:13 PM, Stefan Berger wrote:
>>> On 02/08/2018 08:13 AM, Michal Privoznik wrote:
 On 02/06/2018 08:20 PM, John Ferlan wrote:
> Implement the self locking object list for nwfilter object lists
> that uses two hash tables to store the nwfilter object by UUID or
> by Name.
>
> As part of this alter the uuid argument to virNWFilterObjLookupByUUID
> to expect an already formatted uuidstr.
>
> Alter the existing list traversal code to implement the hash table
> find/lookup/search functionality.
>
> Signed-off-by: John Ferlan 
> ---
>    src/conf/virnwfilterobj.c  | 405
> -
>    src/conf/virnwfilterobj.h  |   2 +-
>    src/nwfilter/nwfilter_driver.c |   5 +-
>    3 files changed, 283 insertions(+), 129 deletions(-)
> @@ -425,24 +483,26 @@
> virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
>    return NULL;
>    }
>    -
> -    /* Get a READ lock and immediately promote to WRITE while we
> adjust
> - * data within. */
> -    if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
> +    /* We're about to make some changes to objects on the list - so
> get the
> + * list READ lock in order to Find the object and WRITE lock the
> object
> + * since both paths would immediately promote it anyway */
> +    virObjectRWLockRead(nwfilters);
> +    if ((obj = virNWFilterObjListFindByNameLocked(nwfilters,
> def->name))) {
> +    virObjectRWLockWrite(obj);
> +    virObjectRWUnlock(nwfilters);
>  objdef = obj->def;
>    if (virNWFilterDefEqual(def, objdef)) {
> -    virNWFilterObjPromoteToWrite(obj);
>    virNWFilterDefFree(objdef);
>    obj->def = def;
>    virNWFilterObjDemoteFromWrite(obj);
>    return obj;
>    }
>    -    /* Set newDef and run the trigger with a demoted lock
> since it may need
> - * to grab a read lock on this object and promote before
> returning. */
> -    virNWFilterObjPromoteToWrite(obj);
>    obj->newDef = def;
> +
> +    /* Demote while the trigger runs since it may need to grab a
> read
> + * lock on this object and promote before returning. */
>    virNWFilterObjDemoteFromWrite(obj);
>  /* trigger the update on VMs referencing the filter */
> @@ -462,39 +522,113 @@
> virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
>    return obj;
>    }
>    +    /* Promote the nwfilters to add a new object */
> +    virObjectRWUnlock(nwfilters);
> +    virObjectRWLockWrite(nwfilters);
 How can this work? What if there's another thread waiting for the write
 lock (e.g. just to define an NWFilter)? The Unlock(nwfilters) wakes up
 the other thread, it does its job, unlocks the list waking us in turn.
 So we lock @nwfilters for writing and add something into the hash table
 - without all the checks (e.g. duplicity check) done earlier.
>>> Could we keep the read lock and grab a 2nd lock as a write-lock? It
>>> wouldn't be a virObject call, though.
>> That is not possible because write & read lock must exclude each other
>> by definition.
> 
> We can grab lock A and then lock B. Lock B would either be a read/write
> lock locked as a write lock or would be an exclusive lock. Why would
> that not work?

Oh, I'm misunderstanding. What do you mean by locks A and B?
virNWFilterObj and virNWFilterObjList locks or something else?

Michal

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

[libvirt] [PATCH] log: fix deadlock obtaining hostname (related CVE-2018-6764)

2018-02-12 Thread Daniel P . Berrangé
The fix for CVE-2018-6764 introduced a potential deadlock scenario
that gets triggered by the NSS module when virGetHostname() calls
getaddrinfo to resolve the hostname:

 #0  0x7f6e714b57e7 in futex_wait
 #1  futex_wait_simple
 #2  __pthread_once_slow
 #3  0x7f6e71d16e7d in virOnce
 #4  0x7f6e71d0997c in virLogInitialize
 #5  0x7f6e71d0a09a in virLogVMessage
 #6  0x7f6e71d09ffd in virLogMessage
 #7  0x7f6e71d0db22 in virObjectNew
 #8  0x7f6e71d0dbf1 in virObjectLockableNew
 #9  0x7f6e71d0d3e5 in virMacMapNew
 #10 0x7f6e71cdc50a in findLease
 #11 0x7f6e71cdcc56 in _nss_libvirt_gethostbyname4_r
 #12 0x7f6e724631fc in gaih_inet
 #13 0x7f6e72464697 in __GI_getaddrinfo
 #14 0x7f6e71d19e81 in virGetHostnameImpl
 #15 0x7f6e71d1a057 in virGetHostnameQuiet
 #16 0x7f6e71d09936 in virLogOnceInit
 #17 0x7f6e71d09952 in virLogOnce
 #18 0x7f6e714b5829 in __pthread_once_slow
 #19 0x7f6e71d16e7d in virOnce
 #20 0x7f6e71d0997c in virLogInitialize
 #21 0x7f6e71d0a09a in virLogVMessage
 #22 0x7f6e71d09ffd in virLogMessage
 #23 0x7f6e71d0db22 in virObjectNew
 #24 0x7f6e71d0dbf1 in virObjectLockableNew
 #25 0x7f6e71d0d3e5 in virMacMapNew
 #26 0x7f6e71cdc50a in findLease
 #27 0x7f6e71cdc839 in _nss_libvirt_gethostbyname3_r
 #28 0x7f6e71cdc724 in _nss_libvirt_gethostbyname2_r
 #29 0x7f6e7248f72f in __gethostbyname2_r
 #30 0x7f6e7248f494 in gethostbyname2
 #31 0x56348c30c36d in hosts_keys
 #32 0x56348c30b7d2 in main

Fortunately the extra stuff virGetHostname does is totally irrelevant to
the needs of the logging code, so we can just inline a call to the
native hostname() syscall directly.

Signed-off-by: Daniel P. Berrangé 
---
 src/util/virlog.c | 20 ++--
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/src/util/virlog.c b/src/util/virlog.c
index 8f1e4800dd..4f66cc5e5c 100644
--- a/src/util/virlog.c
+++ b/src/util/virlog.c
@@ -64,7 +64,7 @@
 VIR_LOG_INIT("util.log");
 
 static regex_t *virLogRegex;
-static char *virLogHostname;
+static char virLogHostname[HOST_NAME_MAX+1];
 
 
 #define VIR_LOG_DATE_REGEX "[0-9]{4}-[0-9]{2}-[0-9]{2}"
@@ -261,6 +261,8 @@ virLogPriorityString(virLogPriority lvl)
 static int
 virLogOnceInit(void)
 {
+int r;
+
 if (virMutexInit() < 0)
 return -1;
 
@@ -275,8 +277,17 @@ virLogOnceInit(void)
 /* We get and remember the hostname early, because at later time
  * it might not be possible to load NSS modules via getaddrinfo()
  * (e.g. at container startup the host filesystem will not be
- * accessible anymore. */
-virLogHostname = virGetHostnameQuiet();
+ * accessible anymore.
+ * Must not use virGetHostname though as that causes re-entrancy
+ * problems if it triggers logging codepaths
+ */
+r = gethostname(virLogHostname, sizeof(virLogHostname));
+if (r == -1) {
+ignore_value(virStrcpy(virLogHostname,
+   "(unknown)", sizeof(virLogHostname)));
+} else {
+NUL_TERMINATE(virLogHostname);
+}
 
 virLogUnlock();
 return 0;
@@ -475,9 +486,6 @@ virLogHostnameString(char **rawmsg,
 {
 char *hoststr;
 
-if (!virLogHostname)
-return -1;
-
 if (virAsprintfQuiet(, "hostname: %s", virLogHostname) < 0)
 return -1;
 
-- 
2.14.3

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

Re: [libvirt] [PATCH] virutil: Introduce virGetHostnameSimple()

2018-02-12 Thread Daniel P . Berrangé
On Mon, Feb 12, 2018 at 11:29:21AM +0100, Michal Privoznik wrote:
> After 759b4d1b0fe5f we are getting hostname in virLogOnceInit().
> Problem with this approach is in the NSS module because the
> module calls some internal APIs which occasionally want to log
> something. This results in virLogInitialize() to be called which
> in turn ends up calling virGetHostnameQuiet() and effectively the
> control gets to NSS plugin again which calls some internal APIs
> which occasionally want to log something. You can see the
> deadlock now.
> 
> One way out of this is to call only gethostname() and not whole
> virGetHostnameQuiet() machinery.

The extra bits in virGetHostname() only exist for the sake of
the QEMU migration code. The source call gethostname() on the
target host and wants to make sure it doesn't return "localhost"
or something that resolves to "127.0.0.1", otherwise the source
host would end up migrating to itself instead of the actual
target host.  We should really just move that extra stuff into
the migration code and leave virGetHostname() simple, instead
of having a virGetHostnameSimple(). That's more than I would
want todo for this CVE fix though, as it would complicate the
backporting. So I feel my patch to inline hostname() call in
the logging code is more suitable in short term, but after
that we could do a big refactor.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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


Re: [libvirt] [PATCH 2/6] virObject: Introduce virObjectRecursiveLockableNew

2018-02-12 Thread Michal Privoznik
On 02/12/2018 01:10 PM, Peter Krempa wrote:
> On Mon, Feb 12, 2018 at 11:52:49 +0100, Michal Privoznik wrote:
>> Sometimes we need the lock in virObjectLockable to be recursive.
>> Because of the nature of pthreads we don't need a special class
>> for that - the pthread_* APIs don't distinguish between normal
>> and recursive locks.
>>
>> Based-on-work-of: John Ferlan 
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/libvirt_private.syms |  1 +
>>  src/util/virobject.c | 22 +++---
>>  src/util/virobject.h |  4 
>>  3 files changed, 24 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 3b14d7d15..fcf378105 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -2417,6 +2417,7 @@ virObjectListFreeCount;
>>  virObjectLock;
>>  virObjectLockableNew;
>>  virObjectNew;
>> +virObjectRecursiveLockableNew;
> 
> I think this was NACK'd last time since we did not want to promote usage
> of recursive locks in the code. If we provide an object that provides
> recursive locking we de-facto promote this usage.
> 
> As Pavel stated in his review. Ideally the NWfilter code will be
> converted to a less convoluted locking not requiring recursive locks
> prior to this so that we don't have to add recursive locking at all.
> 

I think that is far from happening. And I don't see any difference
between virObjectRecursiveLockableNew() and
virMutexInitRecursive() in terms of promoting something. Can you shed
more light where do you see the difference?

Michal

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


Re: [libvirt] [PATCH 2/6] virObject: Introduce virObjectRecursiveLockableNew

2018-02-12 Thread John Ferlan


On 02/12/2018 05:52 AM, Michal Privoznik wrote:
> Sometimes we need the lock in virObjectLockable to be recursive.
> Because of the nature of pthreads we don't need a special class
> for that - the pthread_* APIs don't distinguish between normal
> and recursive locks.
> 
> Based-on-work-of: John Ferlan 
> Signed-off-by: Michal Privoznik 
> ---
>  src/libvirt_private.syms |  1 +
>  src/util/virobject.c | 22 +++---
>  src/util/virobject.h |  4 
>  3 files changed, 24 insertions(+), 3 deletions(-)
> 

Adding recursive locks to virObject* was already rejected once before:

Patch:
https://www.redhat.com/archives/libvir-list/2017-May/msg01183.html

Responses:
https://www.redhat.com/archives/libvir-list/2017-May/msg01212.html
https://www.redhat.com/archives/libvir-list/2017-May/msg01215.html

When you added RWLock's in commit '77f4593b' (afterwards) - I began
thinking - hey the NWFilter code could use those. So I essentially
ditched the effort that used trylock:

https://www.redhat.com/archives/libvir-list/2017-July/msg00673.html

in favor of one that used RWLocks:

https://www.redhat.com/archives/libvir-list/2017-October/msg00264.html

Unlike previous alterations to driver/vir*objlist code - I found that I
needed to change the vir*Obj before changing the vir*ObjList, although I
have forgotten why at this point. Eventually I also discovered why
avocado test was causing libvirtd to go defunct (as noted in the cover
of the v3). That was due to the way the test restarts libvirtd prior to
each run and an unrelated issue in nodedev w/r/t initialization, see:

https://www.redhat.com/archives/libvir-list/2017-December/msg00312.html

John

> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 3b14d7d15..fcf378105 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2417,6 +2417,7 @@ virObjectListFreeCount;
>  virObjectLock;
>  virObjectLockableNew;
>  virObjectNew;
> +virObjectRecursiveLockableNew;
>  virObjectRef;
>  virObjectRWLockableNew;
>  virObjectRWLockRead;
> diff --git a/src/util/virobject.c b/src/util/virobject.c
> index b2fc63aec..1d82e826b 100644
> --- a/src/util/virobject.c
> +++ b/src/util/virobject.c
> @@ -257,8 +257,9 @@ virObjectNew(virClassPtr klass)
>  }
>  
>  
> -void *
> -virObjectLockableNew(virClassPtr klass)
> +static void *
> +virObjectLockableNewInternal(virClassPtr klass,
> + bool recursive)
>  {
>  virObjectLockablePtr obj;
>  
> @@ -272,7 +273,8 @@ virObjectLockableNew(virClassPtr klass)
>  if (!(obj = virObjectNew(klass)))
>  return NULL;
>  
> -if (virMutexInit(>lock) < 0) {
> +if ((!recursive && virMutexInit(>lock) < 0) ||
> +(recursive && virMutexInitRecursive(>lock) < 0)) {
>  virReportSystemError(errno, "%s",
>   _("Unable to initialize mutex"));
>  virObjectUnref(obj);
> @@ -283,6 +285,20 @@ virObjectLockableNew(virClassPtr klass)
>  }
>  
>  
> +void *
> +virObjectLockableNew(virClassPtr klass)
> +{
> +return virObjectLockableNewInternal(klass, false);
> +}
> +
> +
> +void *
> +virObjectRecursiveLockableNew(virClassPtr klass)
> +{
> +return virObjectLockableNewInternal(klass, true);
> +}
> +
> +
>  void *
>  virObjectRWLockableNew(virClassPtr klass)
>  {
> diff --git a/src/util/virobject.h b/src/util/virobject.h
> index ac6cf22f9..367d505ae 100644
> --- a/src/util/virobject.h
> +++ b/src/util/virobject.h
> @@ -116,6 +116,10 @@ void *
>  virObjectLockableNew(virClassPtr klass)
>  ATTRIBUTE_NONNULL(1);
>  
> +void *
> +virObjectRecursiveLockableNew(virClassPtr klass)
> +ATTRIBUTE_NONNULL(1);
> +
>  void *
>  virObjectRWLockableNew(virClassPtr klass)
>  ATTRIBUTE_NONNULL(1);
> 

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


Re: [libvirt] [PATCH REBASE 1/2] tests: Add some tests for PCI controller options

2018-02-12 Thread Andrea Bolognani
On Sun, 2018-02-11 at 08:12 -0500, John Ferlan wrote:
> On 02/05/2018 11:08 AM, Andrea Bolognani wrote:
> > The input configurations set all existing options for all PCI
> > controllers, to see what ends up showing up in the output.
> 
> Not quite sure I understand the need. The only capability not currently
> used in some way is QEMU_CAPS_I440FX_PCI_HOLE64_SIZE.

The idea is that existing tests only cover valid PCI controller
options being handled correctly, not what happens when you specify
an option which is not relevant to the PCI controller at hand.

> I guess there's nothing wrong with adding them other than test bloat.
> Still there's nothing to "force" someone that adds some new thing to
> update one of the three if some new option/capability is added.

That's correct. Can you think of a way to make sure that happens?

> Ironically adding (for example) QEMU_CAPS_Q35_PCI_HOLE64_SIZE to
> i440fx-* doesn't cause any failure. I didn't try other ones.

Of course it wouldn't: capabilities are a property of the QEMU
binary, and the same QEMU binary is used to run both i440fx and
q35 guests. Even for something entirely outlandish like adding a
q35-specific capability to a pSeries test you shouldn't see any
failure unless you actually attempt to use the QEMU feature the
capability is tied to.

> I'm not opposed to this being added, but do you think we should add
> something does the opposite check?   That the wrong PCI adapter on the
> wrong machine?  The wrong option on the wrong adapter is a question for
> the next patch though...

We should already have at least *some* coverage for that, eg.
pseries-serial-invalid-machine and similar. I'm certainly not
volunteering to go over all controller and device and machine
types and write tests for all combinations :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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


[libvirt] [PATCH] virutil: Introduce virGetHostnameSimple()

2018-02-12 Thread Michal Privoznik
After 759b4d1b0fe5f we are getting hostname in virLogOnceInit().
Problem with this approach is in the NSS module because the
module calls some internal APIs which occasionally want to log
something. This results in virLogInitialize() to be called which
in turn ends up calling virGetHostnameQuiet() and effectively the
control gets to NSS plugin again which calls some internal APIs
which occasionally want to log something. You can see the
deadlock now.

One way out of this is to call only gethostname() and not whole
virGetHostnameQuiet() machinery.

Signed-off-by: Michal Privoznik 
---
 src/libvirt_private.syms |  1 +
 src/util/virlog.c|  2 +-
 src/util/virutil.c   | 46 ++
 src/util/virutil.h   |  1 +
 4 files changed, 37 insertions(+), 13 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 3b14d7d15..3ef55f809 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3016,6 +3016,7 @@ virGetGroupList;
 virGetGroupName;
 virGetHostname;
 virGetHostnameQuiet;
+virGetHostnameSimple;
 virGetListenFDs;
 virGetSelfLastChanged;
 virGetSystemPageSize;
diff --git a/src/util/virlog.c b/src/util/virlog.c
index 8f1e4800d..fc854ffeb 100644
--- a/src/util/virlog.c
+++ b/src/util/virlog.c
@@ -276,7 +276,7 @@ virLogOnceInit(void)
  * it might not be possible to load NSS modules via getaddrinfo()
  * (e.g. at container startup the host filesystem will not be
  * accessible anymore. */
-virLogHostname = virGetHostnameQuiet();
+virLogHostname = virGetHostnameSimple();
 
 virLogUnlock();
 return 0;
diff --git a/src/util/virutil.c b/src/util/virutil.c
index cd6fbf2f3..22adecd53 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -684,26 +684,23 @@ static char *
 virGetHostnameImpl(bool quiet)
 {
 int r;
-char hostname[HOST_NAME_MAX+1], *result = NULL;
+char *result;
 struct addrinfo hints, *info;
 
-r = gethostname(hostname, sizeof(hostname));
-if (r == -1) {
+if (!(result = virGetHostnameSimple())) {
 if (!quiet)
 virReportSystemError(errno,
  "%s", _("failed to determine host name"));
 return NULL;
 }
-NUL_TERMINATE(hostname);
 
-if (STRPREFIX(hostname, "localhost") || strchr(hostname, '.')) {
+if (STRPREFIX(result, "localhost") || strchr(result, '.')) {
 /* in this case, gethostname returned localhost (meaning we can't
  * do any further canonicalization), or it returned an FQDN (and
  * we don't need to do any further canonicalization).  Return the
  * string as-is; it's up to callers to check whether "localhost"
  * is allowed.
  */
-ignore_value(VIR_STRDUP_QUIET(result, hostname));
 goto cleanup;
 }
 
@@ -714,12 +711,11 @@ virGetHostnameImpl(bool quiet)
 memset(, 0, sizeof(hints));
 hints.ai_flags = AI_CANONNAME|AI_CANONIDN;
 hints.ai_family = AF_UNSPEC;
-r = getaddrinfo(hostname, NULL, , );
+r = getaddrinfo(result, NULL, , );
 if (r != 0) {
 if (!quiet)
 VIR_WARN("getaddrinfo failed for '%s': %s",
- hostname, gai_strerror(r));
-ignore_value(VIR_STRDUP_QUIET(result, hostname));
+ result, gai_strerror(r));
 goto cleanup;
 }
 
@@ -727,15 +723,16 @@ virGetHostnameImpl(bool quiet)
 sa_assert(info);
 
 if (info->ai_canonname == NULL ||
-STRPREFIX(info->ai_canonname, "localhost"))
+STRPREFIX(info->ai_canonname, "localhost")) {
 /* in this case, we tried to canonicalize and we ended up back with
  * localhost.  Ignore the canonicalized name and just return the
  * original hostname
  */
-ignore_value(VIR_STRDUP_QUIET(result, hostname));
-else
+} else {
 /* Caller frees this string. */
+VIR_FREE(result);
 ignore_value(VIR_STRDUP_QUIET(result, info->ai_canonname));
+}
 
 freeaddrinfo(info);
 
@@ -760,6 +757,31 @@ virGetHostnameQuiet(void)
 }
 
 
+/**
+ * virGetHostnameSimple:
+ *
+ * Plain wrapper over gethostname(). The difference to
+ * virGetHostname() is that this function doesn't try to
+ * canonicalize the hostname.
+ *
+ * Returns: hostname string (caller must free),
+ *  NULL on error.
+ */
+char *
+virGetHostnameSimple(void)
+{
+char hostname[HOST_NAME_MAX+1];
+char *ret;
+
+if (gethostname(hostname, sizeof(hostname)) == -1)
+return NULL;
+
+NUL_TERMINATE(hostname);
+ignore_value(VIR_STRDUP_QUIET(ret, hostname));
+return ret;
+}
+
+
 char *
 virGetUserDirectory(void)
 {
diff --git a/src/util/virutil.h b/src/util/virutil.h
index be0f6b0ea..57148374b 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -136,6 +136,7 @@ static inline int pthread_sigmask(int how,
 
 char *virGetHostname(void);
 char *virGetHostnameQuiet(void);
+char 

Re: [libvirt] [PATCH v2] qemu: Expose rx/tx_queue_size in qemu.conf too

2018-02-12 Thread Martin Kletzander

On Thu, Feb 01, 2018 at 09:58:17AM -0500, John Ferlan wrote:



On 02/01/2018 09:04 AM, Michal Privoznik wrote:

In 2074ef6cd4a2 and c56cdf259 (and friends) we've added two
attributes to virtio NICs: rx_queue_size and tx_queue_size.
However, sysadmins might want to set these on per-host basis but
don't necessarily have an access to domain XML (e.g. because they
are generated by some other app). So let's expose them under


Define "sysadmins" here.  If there's a management app running that
covers some hosts, then the app is the sysadmin.  Who will be setting
these in the conf file?  Some person?  Manually?  I don't think so.  My
bet is that it will be the same mgmt app that generates the XML.  Or the
same person who deploys the app, but still manually.  If that app is
unable to have a default, then why should libvirt be?


qemu.conf (the settings from domain XML still take precedence as
they are more specific ones).

Signed-off-by: Michal Privoznik 
---

diff to v1:
- Reworded docs and config file comment
- Simplified logic in qemuBuildNicDevStr a bit
- Make the values require qemu with corresponding capabilities

 docs/formatdomain.html.in  | 14 --
 src/qemu/libvirtd_qemu.aug |  4 +++
 src/qemu/qemu.conf |  6 +
 src/qemu/qemu_command.c| 55 +++---
 src/qemu/qemu_command.h|  3 ++-
 src/qemu/qemu_conf.c   |  4 +++
 src/qemu/qemu_conf.h   |  3 +++
 src/qemu/qemu_hotplug.c|  2 +-
 src/qemu/test_libvirtd_qemu.aug.in |  2 ++
 9 files changed, 74 insertions(+), 19 deletions(-)



Reviewed-by: John Ferlan 



Sorry I missed this earlier, I'm going through my e-mail backlog kinda
too late.  But I don't think this should be released.  Or anything
similar.  If we make this change, then where do we draw the line for
setting defaults in qemu.conf?  Will we eventually have defaults for
first interface, memory size or even whole XML that get's parsed before
the user-supplied one?  I know it's too late to NACK this, but please
consider reverting it before next release.

Also feel free to discuss if you have arguments.  Thank you.


John

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


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


Re: [libvirt] [PATCH 2/6] virObject: Introduce virObjectRecursiveLockableNew

2018-02-12 Thread Peter Krempa
On Mon, Feb 12, 2018 at 11:52:49 +0100, Michal Privoznik wrote:
> Sometimes we need the lock in virObjectLockable to be recursive.
> Because of the nature of pthreads we don't need a special class
> for that - the pthread_* APIs don't distinguish between normal
> and recursive locks.
> 
> Based-on-work-of: John Ferlan 
> Signed-off-by: Michal Privoznik 
> ---
>  src/libvirt_private.syms |  1 +
>  src/util/virobject.c | 22 +++---
>  src/util/virobject.h |  4 
>  3 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 3b14d7d15..fcf378105 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2417,6 +2417,7 @@ virObjectListFreeCount;
>  virObjectLock;
>  virObjectLockableNew;
>  virObjectNew;
> +virObjectRecursiveLockableNew;

I think this was NACK'd last time since we did not want to promote usage
of recursive locks in the code. If we provide an object that provides
recursive locking we de-facto promote this usage.

As Pavel stated in his review. Ideally the NWfilter code will be
converted to a less convoluted locking not requiring recursive locks
prior to this so that we don't have to add recursive locking at all.


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] qemu: Expose rx/tx_queue_size in qemu.conf too

2018-02-12 Thread Michal Privoznik
On 02/12/2018 01:05 PM, Daniel P. Berrangé wrote:
> On Mon, Feb 12, 2018 at 12:44:52PM +0100, Martin Kletzander wrote:
>> On Thu, Feb 01, 2018 at 09:58:17AM -0500, John Ferlan wrote:
>>>
>>>
>>> On 02/01/2018 09:04 AM, Michal Privoznik wrote:
 In 2074ef6cd4a2 and c56cdf259 (and friends) we've added two
 attributes to virtio NICs: rx_queue_size and tx_queue_size.
 However, sysadmins might want to set these on per-host basis but
 don't necessarily have an access to domain XML (e.g. because they
 are generated by some other app). So let's expose them under
>>
>> Define "sysadmins" here.  If there's a management app running that
>> covers some hosts, then the app is the sysadmin.  Who will be setting
>> these in the conf file?  Some person?  Manually?  I don't think so.  My
>> bet is that it will be the same mgmt app that generates the XML.  Or the
>> same person who deploys the app, but still manually.  If that app is
>> unable to have a default, then why should libvirt be?
> 
> IIUC, this feature came in as a request to solve a limitation that
> exists in OpenStack nova not having support for setting this feature
> for guest XML.
> 
> IMHO the right fix for that request is to address the feature gap
> in Nova, not add this to libvirt to workaround fact that Nova has
> not supported this yet.

Agreed. Lets revert the commit. Sorry for the noise.

Michal

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

Re: [libvirt] [PATCH] log: fix deadlock obtaining hostname (related CVE-2018-6764)

2018-02-12 Thread Daniel P . Berrangé
On Mon, Feb 12, 2018 at 11:52:43AM +0100, Michal Privoznik wrote:
> On 02/12/2018 11:08 AM, Daniel P. Berrangé wrote:
> > The fix for CVE-2018-6764 introduced a potential deadlock scenario
> > that gets triggered by the NSS module when virGetHostname() calls
> > getaddrinfo to resolve the hostname:
> > 
> >  #0  0x7f6e714b57e7 in futex_wait
> >  #1  futex_wait_simple
> >  #2  __pthread_once_slow
> >  #3  0x7f6e71d16e7d in virOnce
> >  #4  0x7f6e71d0997c in virLogInitialize
> >  #5  0x7f6e71d0a09a in virLogVMessage
> >  #6  0x7f6e71d09ffd in virLogMessage
> >  #7  0x7f6e71d0db22 in virObjectNew
> >  #8  0x7f6e71d0dbf1 in virObjectLockableNew
> >  #9  0x7f6e71d0d3e5 in virMacMapNew
> >  #10 0x7f6e71cdc50a in findLease
> >  #11 0x7f6e71cdcc56 in _nss_libvirt_gethostbyname4_r
> >  #12 0x7f6e724631fc in gaih_inet
> >  #13 0x7f6e72464697 in __GI_getaddrinfo
> >  #14 0x7f6e71d19e81 in virGetHostnameImpl
> >  #15 0x7f6e71d1a057 in virGetHostnameQuiet
> >  #16 0x7f6e71d09936 in virLogOnceInit
> >  #17 0x7f6e71d09952 in virLogOnce
> >  #18 0x7f6e714b5829 in __pthread_once_slow
> >  #19 0x7f6e71d16e7d in virOnce
> >  #20 0x7f6e71d0997c in virLogInitialize
> >  #21 0x7f6e71d0a09a in virLogVMessage
> >  #22 0x7f6e71d09ffd in virLogMessage
> >  #23 0x7f6e71d0db22 in virObjectNew
> >  #24 0x7f6e71d0dbf1 in virObjectLockableNew
> >  #25 0x7f6e71d0d3e5 in virMacMapNew
> >  #26 0x7f6e71cdc50a in findLease
> >  #27 0x7f6e71cdc839 in _nss_libvirt_gethostbyname3_r
> >  #28 0x7f6e71cdc724 in _nss_libvirt_gethostbyname2_r
> >  #29 0x7f6e7248f72f in __gethostbyname2_r
> >  #30 0x7f6e7248f494 in gethostbyname2
> >  #31 0x56348c30c36d in hosts_keys
> >  #32 0x56348c30b7d2 in main
> > 
> > Fortunately the extra stuff virGetHostname does is totally irrelevant to
> > the needs of the logging code, so we can just inline a call to the
> > native hostname() syscall directly.
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  src/util/virlog.c | 20 ++--
> >  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> ACK

FYI I'll also squash in a change to cfg.mk to avoid syntax-check failure

diff --git a/cfg.mk b/cfg.mk
index 78f805b27e..920b609172 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -1183,7 +1183,7 @@ 
_src2=src/(util/vircommand|libvirt|lxc/lxc_controller|locking/lock_daemon|loggin
 exclude_file_name_regexp--sc_prohibit_fork_wrappers = \
   (^($(_src2)|tests/testutils|daemon/libvirtd)\.c$$)
 
-exclude_file_name_regexp--sc_prohibit_gethostname = ^src/util/virutil\.c$$
+exclude_file_name_regexp--sc_prohibit_gethostname = 
^src/util/vir(util|log)\.c$$
 
 exclude_file_name_regexp--sc_prohibit_internal_functions = \
   ^src/(util/(viralloc|virutil|virfile)\.[hc]|esx/esx_vi\.c)$$


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[libvirt] [PATCH] tests: fix running of qemuxml2argvtest program

2018-02-12 Thread Daniel P . Berrangé
The previous commit:

  commit a455d41e3e1c1af3a36ccdbb2e3f2356cc58993e
  Author: Daniel P. Berrangé 
  Date:   Thu Jan 25 09:35:50 2018 +

conf: expand network device callbacks to cover resolving NIC type

mistakenly dropped qemuxml2argvtest from the tests due to a typo.

Signed-off-by: Daniel P. Berrangé 
---
 tests/Makefile.am | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/Makefile.am b/tests/Makefile.am
index d013aed5eb..4ff3fa742a 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -291,7 +291,7 @@ test_programs += qemuxml2xmltest \
 if WITH_NETWORK
 # Dep on the network driver callback for resolving NIC
 # actual type. XXX remove this dep.
-test_programs += qemuxml2xmltest
+test_programs += qemuxml2argvtest
 endif WITH_NETWORK
 test_helpers += qemucapsprobe
 test_libraries += libqemumonitortestutils.la \
-- 
2.14.3

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

Re: [libvirt] [PATCH] tests: fix running of qemuxml2argvtest program

2018-02-12 Thread Michal Privoznik
On 02/12/2018 12:11 PM, Daniel P. Berrangé wrote:
> The previous commit:
> 
>   commit a455d41e3e1c1af3a36ccdbb2e3f2356cc58993e
>   Author: Daniel P. Berrangé 
>   Date:   Thu Jan 25 09:35:50 2018 +
> 
> conf: expand network device callbacks to cover resolving NIC type
> 
> mistakenly dropped qemuxml2argvtest from the tests due to a typo.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  tests/Makefile.am | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Ooops. ACK.

Michal

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

Re: [libvirt] [PATCH v2] qemu: Expose rx/tx_queue_size in qemu.conf too

2018-02-12 Thread Daniel P . Berrangé
On Mon, Feb 12, 2018 at 12:44:52PM +0100, Martin Kletzander wrote:
> On Thu, Feb 01, 2018 at 09:58:17AM -0500, John Ferlan wrote:
> > 
> > 
> > On 02/01/2018 09:04 AM, Michal Privoznik wrote:
> > > In 2074ef6cd4a2 and c56cdf259 (and friends) we've added two
> > > attributes to virtio NICs: rx_queue_size and tx_queue_size.
> > > However, sysadmins might want to set these on per-host basis but
> > > don't necessarily have an access to domain XML (e.g. because they
> > > are generated by some other app). So let's expose them under
> 
> Define "sysadmins" here.  If there's a management app running that
> covers some hosts, then the app is the sysadmin.  Who will be setting
> these in the conf file?  Some person?  Manually?  I don't think so.  My
> bet is that it will be the same mgmt app that generates the XML.  Or the
> same person who deploys the app, but still manually.  If that app is
> unable to have a default, then why should libvirt be?

IIUC, this feature came in as a request to solve a limitation that
exists in OpenStack nova not having support for setting this feature
for guest XML.

IMHO the right fix for that request is to address the feature gap
in Nova, not add this to libvirt to workaround fact that Nova has
not supported this yet.

> > > qemu.conf (the settings from domain XML still take precedence as
> > > they are more specific ones).
> > > 
> > > Signed-off-by: Michal Privoznik 
> > > ---
> > > 
> > > diff to v1:
> > > - Reworded docs and config file comment
> > > - Simplified logic in qemuBuildNicDevStr a bit
> > > - Make the values require qemu with corresponding capabilities
> > > 
> > >  docs/formatdomain.html.in  | 14 --
> > >  src/qemu/libvirtd_qemu.aug |  4 +++
> > >  src/qemu/qemu.conf |  6 +
> > >  src/qemu/qemu_command.c| 55 
> > > +++---
> > >  src/qemu/qemu_command.h|  3 ++-
> > >  src/qemu/qemu_conf.c   |  4 +++
> > >  src/qemu/qemu_conf.h   |  3 +++
> > >  src/qemu/qemu_hotplug.c|  2 +-
> > >  src/qemu/test_libvirtd_qemu.aug.in |  2 ++
> > >  9 files changed, 74 insertions(+), 19 deletions(-)
> > > 
> > 
> > Reviewed-by: John Ferlan 
> > 
> 
> Sorry I missed this earlier, I'm going through my e-mail backlog kinda
> too late.  But I don't think this should be released.  Or anything
> similar.  If we make this change, then where do we draw the line for
> setting defaults in qemu.conf?  Will we eventually have defaults for
> first interface, memory size or even whole XML that get's parsed before
> the user-supplied one?  I know it's too late to NACK this, but please
> consider reverting it before next release.

Agreed, I'd like to see this reverted too as it is a bad precedent and
there's no obvious reason why Nova can't simply support this feature
itself. Even if nova doesn't want to make it user configurable, it
could be set as a default in the nova.conf file, or driven via information
it acquires from Neutron.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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


Re: [libvirt] [PATCH REBASE 2/2] qemu: Clean up PCI controller options

2018-02-12 Thread Andrea Bolognani
On Sun, 2018-02-11 at 08:22 -0500, John Ferlan wrote:
> On 02/05/2018 11:08 AM, Andrea Bolognani wrote:
> > Most of the options are only applicable to one or two controller
> > types, so they should be filtered out everywhere else.
> > 
> > This will reduce user confusion and, in at least one corner case,
> > prevent guests from disappearing on daemon restart.
> > 
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1483816
> 
> So, if I'm reading things correctly - rather than fail because someone
> set an option on a controller (and sometimes for a machine) for which
> it's not supported, the choice is to ignore the value and enforce the
> default.
> 
> This does seem to go against what's been traditionally done (at least my
> recollection of it) for other XML options being wrongly applied to some
> other device.
> 
> Even the bz is a big vague:
> 
> Expected results:
> Schema for the 'target' field in  should
> not accept 'chassis' and 'port' parameters for 'q35' machine type
> 
> But I'd read that as some sort of failure is expected rather than
> acceptance and quietly changing.

Yeah, I'm not sure why I implemented it that way myself. I'll
move everything to a Validate() sub-callback and error out if any
unexpected option is set for a controller.

That will make the test cases added in 1/2 slightly less useful,
though, as it will error out on the first such option instead of
showing that it's resetting all of them.

> > +static void
> > +qemuDomainPCIControllerCleanupOpts(const virDomainDef *def,
> > +   virDomainControllerDefPtr cont)
> > +{
> > +if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI)
> > +return;
> 
> This is redundant with [1]
> 
> > @@ -4799,6 +4914,8 @@ 
> > qemuDomainControllerDefPostParse(virDomainControllerDefPtr cont,
> >  
> >  case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
> 
> [1] only called when type is PCI...

I prefer not embedding in a function knowledge about its callers,
because callers change all the time. In this specific case, I guess
the name is explicit enough that nobody would try calling it on a
USB controller, so we can probably skip the check.

-- 
Andrea Bolognani / Red Hat / Virtualization

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


[libvirt] [PATCH 2/6] virObject: Introduce virObjectRecursiveLockableNew

2018-02-12 Thread Michal Privoznik
Sometimes we need the lock in virObjectLockable to be recursive.
Because of the nature of pthreads we don't need a special class
for that - the pthread_* APIs don't distinguish between normal
and recursive locks.

Based-on-work-of: John Ferlan 
Signed-off-by: Michal Privoznik 
---
 src/libvirt_private.syms |  1 +
 src/util/virobject.c | 22 +++---
 src/util/virobject.h |  4 
 3 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 3b14d7d15..fcf378105 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2417,6 +2417,7 @@ virObjectListFreeCount;
 virObjectLock;
 virObjectLockableNew;
 virObjectNew;
+virObjectRecursiveLockableNew;
 virObjectRef;
 virObjectRWLockableNew;
 virObjectRWLockRead;
diff --git a/src/util/virobject.c b/src/util/virobject.c
index b2fc63aec..1d82e826b 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -257,8 +257,9 @@ virObjectNew(virClassPtr klass)
 }
 
 
-void *
-virObjectLockableNew(virClassPtr klass)
+static void *
+virObjectLockableNewInternal(virClassPtr klass,
+ bool recursive)
 {
 virObjectLockablePtr obj;
 
@@ -272,7 +273,8 @@ virObjectLockableNew(virClassPtr klass)
 if (!(obj = virObjectNew(klass)))
 return NULL;
 
-if (virMutexInit(>lock) < 0) {
+if ((!recursive && virMutexInit(>lock) < 0) ||
+(recursive && virMutexInitRecursive(>lock) < 0)) {
 virReportSystemError(errno, "%s",
  _("Unable to initialize mutex"));
 virObjectUnref(obj);
@@ -283,6 +285,20 @@ virObjectLockableNew(virClassPtr klass)
 }
 
 
+void *
+virObjectLockableNew(virClassPtr klass)
+{
+return virObjectLockableNewInternal(klass, false);
+}
+
+
+void *
+virObjectRecursiveLockableNew(virClassPtr klass)
+{
+return virObjectLockableNewInternal(klass, true);
+}
+
+
 void *
 virObjectRWLockableNew(virClassPtr klass)
 {
diff --git a/src/util/virobject.h b/src/util/virobject.h
index ac6cf22f9..367d505ae 100644
--- a/src/util/virobject.h
+++ b/src/util/virobject.h
@@ -116,6 +116,10 @@ void *
 virObjectLockableNew(virClassPtr klass)
 ATTRIBUTE_NONNULL(1);
 
+void *
+virObjectRecursiveLockableNew(virClassPtr klass)
+ATTRIBUTE_NONNULL(1);
+
 void *
 virObjectRWLockableNew(virClassPtr klass)
 ATTRIBUTE_NONNULL(1);
-- 
2.13.6

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


Re: [libvirt] [PATCH] log: fix deadlock obtaining hostname (related CVE-2018-6764)

2018-02-12 Thread Michal Privoznik
On 02/12/2018 11:08 AM, Daniel P. Berrangé wrote:
> The fix for CVE-2018-6764 introduced a potential deadlock scenario
> that gets triggered by the NSS module when virGetHostname() calls
> getaddrinfo to resolve the hostname:
> 
>  #0  0x7f6e714b57e7 in futex_wait
>  #1  futex_wait_simple
>  #2  __pthread_once_slow
>  #3  0x7f6e71d16e7d in virOnce
>  #4  0x7f6e71d0997c in virLogInitialize
>  #5  0x7f6e71d0a09a in virLogVMessage
>  #6  0x7f6e71d09ffd in virLogMessage
>  #7  0x7f6e71d0db22 in virObjectNew
>  #8  0x7f6e71d0dbf1 in virObjectLockableNew
>  #9  0x7f6e71d0d3e5 in virMacMapNew
>  #10 0x7f6e71cdc50a in findLease
>  #11 0x7f6e71cdcc56 in _nss_libvirt_gethostbyname4_r
>  #12 0x7f6e724631fc in gaih_inet
>  #13 0x7f6e72464697 in __GI_getaddrinfo
>  #14 0x7f6e71d19e81 in virGetHostnameImpl
>  #15 0x7f6e71d1a057 in virGetHostnameQuiet
>  #16 0x7f6e71d09936 in virLogOnceInit
>  #17 0x7f6e71d09952 in virLogOnce
>  #18 0x7f6e714b5829 in __pthread_once_slow
>  #19 0x7f6e71d16e7d in virOnce
>  #20 0x7f6e71d0997c in virLogInitialize
>  #21 0x7f6e71d0a09a in virLogVMessage
>  #22 0x7f6e71d09ffd in virLogMessage
>  #23 0x7f6e71d0db22 in virObjectNew
>  #24 0x7f6e71d0dbf1 in virObjectLockableNew
>  #25 0x7f6e71d0d3e5 in virMacMapNew
>  #26 0x7f6e71cdc50a in findLease
>  #27 0x7f6e71cdc839 in _nss_libvirt_gethostbyname3_r
>  #28 0x7f6e71cdc724 in _nss_libvirt_gethostbyname2_r
>  #29 0x7f6e7248f72f in __gethostbyname2_r
>  #30 0x7f6e7248f494 in gethostbyname2
>  #31 0x56348c30c36d in hosts_keys
>  #32 0x56348c30b7d2 in main
> 
> Fortunately the extra stuff virGetHostname does is totally irrelevant to
> the needs of the logging code, so we can just inline a call to the
> native hostname() syscall directly.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/util/virlog.c | 20 ++--
>  1 file changed, 14 insertions(+), 6 deletions(-)

ACK

Michal

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

Re: [libvirt] [PATCH v5 1/3] nwfilter: Convert _virNWFilterObj to use virObjectRWLockable

2018-02-12 Thread Michal Privoznik
On 02/09/2018 02:00 PM, John Ferlan wrote:
> 
> 
> On 02/09/2018 03:41 AM, Michal Privoznik wrote:
>> On 02/08/2018 04:06 PM, John Ferlan wrote:
>>> [...]
>>>
>>> +static void
>>> +virNWFilterObjPromoteToWrite(virNWFilterObjPtr obj)
>>> +{
>>> +virObjectRWUnlock(obj);
>>> +virObjectRWLockWrite(obj);
>>> +}
>>
>> How can this not deadlock? This will work only if @obj is locked exactly
>> once. But since we allow recursive locks any lock() that happens in the
>> 2nd level must deadlock with this. On the other hand, there's no such
>> case in the code currently. But that doesn't stop anybody from calling
>> PromoteWrite() without understanding its limitations.
>>
>> Maybe the fact that we need recursive lock for NWFilterObj means it's
>> not a good candidate for virObjectRWLocable? It is still a good
>> candidate for virObject though.
>>
>> Or if you want to spend extra time on this, maybe introducing
>> virObjectRecursiveLockable may be worth it (terrible name, I know).
>>
>>
>
> I dunno, worked for me. The helper is being called from a thread that
> has taken out a READ lock at most one time and needs to get the WRITE
> lock in order to change things. If something else has the READ lock that
> thread waits until the other thread releases the READ lock as far as I
> understand things.

 Yes, I can see that. It's just that since the original lock is recursive
 I expected things to get recursive and thus I've been thinking how would
 this work there, nested in 2nd, 3rd, .. level. But as noted earlier, the
 places where lock promoting is used are kind of safe since all the
 locking is done at the first level.

>>>
>>> The reason I chose rwlocks and taking a read lock by default in the
>>> FindByUUID and FindByName (as opposed to other vir*FindBy* API's which
>>> return a write locked object) is because I know there's the potential
>>> for other code to be doing Reads and rwlocks allowed for that without
>>> the need to create any sort of recursive lockable object or any sort of
>>> API to perform "trylock"'s (all things I tried until the epiphany to use
>>> rwlocks was reached when you added them for other drivers and their list
>>> protection).
>>>
>>> In the long/short run I figured that having a common way to get the lock
>>> and then deciding to change it from Read to Write as necessary would
>>> work just fine for the purpose that was needed.
>>
>> This is where I disagree. I don't think it's safe to promote a lock by
>> unlocking it. The moment the lock is unlocked another thread will lock
>> the object and all the assumptions we made/checked about the object we
>> made when we had the object locked are gone. So we would need to
>> reiterate the decision making process.
>>
> 
> OK - so let's think about where/why/when we promote the lock and what
> the checks/balances/consequences are. 

I know. I am saying that all along that with the current situation it is
safe to have lock promoting. But in general (and possibly to save future
us from nasty deadlocks) it is not. And as I say in comment for 2/3 this
code is guarded by nwfilter driver lock anyway. So there is no race
possible anyway. However, I think I have some patches ready that
implement what is needed here. I'm gonna post them shortly and give you
all the credit - they are heavily based on your work.

Michal

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


[libvirt] [PATCH 6/6] nwfilter: Remove need for nwfilterDriverLock in some API's

2018-02-12 Thread Michal Privoznik
From: John Ferlan 

Now that nwfilters object list is self locking, it's no longer
necessary to hold the driver level lock for certain API's.

Signed-off-by: John Ferlan 
Signed-off-by: Michal Privoznik 
---
 src/nwfilter/nwfilter_driver.c | 47 +++---
 1 file changed, 12 insertions(+), 35 deletions(-)

diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
index 093844c9e..d49c32e4c 100644
--- a/src/nwfilter/nwfilter_driver.c
+++ b/src/nwfilter/nwfilter_driver.c
@@ -295,6 +295,10 @@ nwfilterStateReload(void)
 /* shut down all threads -- they will be restarted if necessary */
 virNWFilterLearnThreadsTerminate(true);
 
+/* Serialization of virNWFilterObjListLoadAllConfigs is extremely
+ * important as it relates to virNWFilterObjListFindInstantiateFilter
+ * processing via virNWFilterTriggerVMFilterRebuild that occurs during
+ * virNWFilterObjListAssignDef */
 nwfilterDriverLock();
 virNWFilterWriteLockFilterUpdates();
 virNWFilterCallbackDriversLock();
@@ -452,11 +456,7 @@ nwfilterLookupByUUID(virConnectPtr conn,
 virNWFilterDefPtr def;
 virNWFilterPtr nwfilter = NULL;
 
-nwfilterDriverLock();
-obj = nwfilterObjFromNWFilter(uuid);
-nwfilterDriverUnlock();
-
-if (!obj)
+if (!(obj = nwfilterObjFromNWFilter(uuid)))
 return NULL;
 def = virNWFilterObjGetDef(obj);
 
@@ -479,11 +479,7 @@ nwfilterLookupByName(virConnectPtr conn,
 virNWFilterDefPtr def;
 virNWFilterPtr nwfilter = NULL;
 
-nwfilterDriverLock();
-obj = virNWFilterObjListFindByName(driver->nwfilters, name);
-nwfilterDriverUnlock();
-
-if (!obj) {
+if (!(obj = virNWFilterObjListFindByName(driver->nwfilters, name))) {
 virReportError(VIR_ERR_NO_NWFILTER,
_("no nwfilter with matching name '%s'"), name);
 return NULL;
@@ -517,17 +513,12 @@ nwfilterConnectListNWFilters(virConnectPtr conn,
  char **const names,
  int maxnames)
 {
-int nnames;
-
 if (virConnectListNWFiltersEnsureACL(conn) < 0)
 return -1;
 
-nwfilterDriverLock();
-nnames = virNWFilterObjListGetNames(driver->nwfilters, conn,
-virConnectListNWFiltersCheckACL,
-names, maxnames);
-nwfilterDriverUnlock();
-return nnames;
+return virNWFilterObjListGetNames(driver->nwfilters, conn,
+  virConnectListNWFiltersCheckACL,
+  names, maxnames);
 }
 
 
@@ -536,19 +527,13 @@ nwfilterConnectListAllNWFilters(virConnectPtr conn,
 virNWFilterPtr **nwfilters,
 unsigned int flags)
 {
-int ret;
-
 virCheckFlags(0, -1);
 
 if (virConnectListAllNWFiltersEnsureACL(conn) < 0)
 return -1;
 
-nwfilterDriverLock();
-ret = virNWFilterObjListExport(conn, driver->nwfilters, nwfilters,
-   virConnectListAllNWFiltersCheckACL);
-nwfilterDriverUnlock();
-
-return ret;
+return virNWFilterObjListExport(conn, driver->nwfilters, nwfilters,
+virConnectListAllNWFiltersCheckACL);
 }
 
 static virNWFilterPtr
@@ -566,7 +551,6 @@ nwfilterDefineXML(virConnectPtr conn,
 return NULL;
 }
 
-nwfilterDriverLock();
 virNWFilterWriteLockFilterUpdates();
 virNWFilterCallbackDriversLock();
 
@@ -594,7 +578,6 @@ nwfilterDefineXML(virConnectPtr conn,
 
 virNWFilterCallbackDriversUnlock();
 virNWFilterUnlockFilterUpdates();
-nwfilterDriverUnlock();
 return nwfilter;
 }
 
@@ -606,7 +589,6 @@ nwfilterUndefine(virNWFilterPtr nwfilter)
 virNWFilterDefPtr def;
 int ret = -1;
 
-nwfilterDriverLock();
 virNWFilterWriteLockFilterUpdates();
 virNWFilterCallbackDriversLock();
 
@@ -634,7 +616,6 @@ nwfilterUndefine(virNWFilterPtr nwfilter)
 virNWFilterObjEndAPI();
 virNWFilterCallbackDriversUnlock();
 virNWFilterUnlockFilterUpdates();
-nwfilterDriverUnlock();
 return ret;
 }
 
@@ -649,11 +630,7 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter,
 
 virCheckFlags(0, NULL);
 
-nwfilterDriverLock();
-obj = nwfilterObjFromNWFilter(nwfilter->uuid);
-nwfilterDriverUnlock();
-
-if (!obj)
+if (!(obj = nwfilterObjFromNWFilter(nwfilter->uuid)))
 return NULL;
 def = virNWFilterObjGetDef(obj);
 
-- 
2.13.6

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


[libvirt] [PATCH 0/6] Alternative way of turning NWFilterObj(list) into virObject

2018-02-12 Thread Michal Privoznik
This is heavily based on John's work (except 1/6). The difference to his
patches is in using virObjectRecursiveLockableNew() for recursive locks
instead of RW locks and lock promoting. Also, in 6/6 I was more
confident and removed driver lock from define/undefine APIs.

John Ferlan (1):
  nwfilter: Remove need for nwfilterDriverLock in some API's

Michal Privoznik (5):
  virObject: use virReportSystemError if applicable
  virObject: Introduce virObjectRecursiveLockableNew
  virNWFilterObj: Turn into virObjectLockable
  conf: Introduce and use virNWFilterObjEndAPI
  nwfilter: Convert _virNWFilterObjList to use virObjectRWLockable

 cfg.mk |   1 -
 src/conf/virdomainobjlist.c|   3 +-
 src/conf/virnwfilterobj.c  | 504 +
 src/conf/virnwfilterobj.h  |  12 +-
 src/libvirt_private.syms   |   5 +-
 src/nwfilter/nwfilter_driver.c |  65 ++---
 src/nwfilter/nwfilter_gentech_driver.c |  11 +-
 src/util/virobject.c   |  30 +-
 src/util/virobject.h   |   4 +
 9 files changed, 382 insertions(+), 253 deletions(-)

-- 
2.13.6

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


[libvirt] [PATCH 5/6] nwfilter: Convert _virNWFilterObjList to use virObjectRWLockable

2018-02-12 Thread Michal Privoznik
Based-on-work-of: John Ferlan 
Signed-off-by: Michal Privoznik 
---
 cfg.mk |   1 -
 src/conf/virdomainobjlist.c|   3 +-
 src/conf/virnwfilterobj.c  | 409 +++--
 src/conf/virnwfilterobj.h  |   3 -
 src/libvirt_private.syms   |   1 -
 src/nwfilter/nwfilter_driver.c |   4 +-
 6 files changed, 279 insertions(+), 142 deletions(-)

diff --git a/cfg.mk b/cfg.mk
index 78f805b27..89779fb05 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -242,7 +242,6 @@ useless_free_options = \
 # y virNWFilterIncludeDefFree
 # n virNWFilterFreeName (returns int)
 # y virNWFilterObjFree
-# n virNWFilterObjListFree FIXME
 # y virNWFilterRuleDefFree
 # n virNWFilterRuleFreeInstanceData (typedef)
 # y virNWFilterRuleInstFree
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 87a742b1e..4d3cc94b3 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -206,7 +206,8 @@ virDomainObjPtr 
virDomainObjListFindByName(virDomainObjListPtr doms,
 
 virObjectRWLockRead(doms);
 obj = virHashLookup(doms->objsName, name);
-virObjectRef(obj);
+if (obj)
+virObjectRef(obj);
 virObjectRWUnlock(doms);
 if (obj) {
 virObjectLock(obj);
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
index 6a54628b6..bb4d0a036 100644
--- a/src/conf/virnwfilterobj.c
+++ b/src/conf/virnwfilterobj.c
@@ -43,11 +43,20 @@ struct _virNWFilterObj {
 };
 
 static virClassPtr virNWFilterObjClass;
+static virClassPtr virNWFilterObjListClass;
 static void virNWFilterObjDispose(void *obj);
+static void virNWFilterObjListDispose(void *obj);
 
 struct _virNWFilterObjList {
-size_t count;
-virNWFilterObjPtr *objs;
+virObjectRWLockable parent;
+
+/* uuid string -> virNWFilterObj  mapping
+ * for O(1), lockless lookup-by-uuid */
+virHashTable *objs;
+
+/* name -> virNWFilterObj mapping for O(1),
+ * lockless lookup-by-name */
+virHashTable *objsName;
 };
 
 static int virNWFilterObjOnceInit(void)
@@ -58,6 +67,12 @@ static int virNWFilterObjOnceInit(void)
 virNWFilterObjDispose)))
 return -1;
 
+if (!(virNWFilterObjListClass = virClassNew(virClassForObjectRWLockable(),
+"virNWFilterObjList",
+sizeof(virNWFilterObjList),
+virNWFilterObjListDispose)))
+return -1;
+
 return 0;
 }
 
@@ -123,14 +138,14 @@ virNWFilterObjWantRemoved(virNWFilterObjPtr obj)
 }
 
 
-void
-virNWFilterObjListFree(virNWFilterObjListPtr nwfilters)
+static void
+virNWFilterObjListDispose(void *obj)
 {
-size_t i;
-for (i = 0; i < nwfilters->count; i++)
-virObjectUnref(nwfilters->objs[i]);
-VIR_FREE(nwfilters->objs);
-VIR_FREE(nwfilters);
+
+virNWFilterObjListPtr nwfilters = obj;
+
+virHashFree(nwfilters->objs);
+virHashFree(nwfilters->objsName);
 }
 
 
@@ -139,8 +154,18 @@ virNWFilterObjListNew(void)
 {
 virNWFilterObjListPtr nwfilters;
 
-if (VIR_ALLOC(nwfilters) < 0)
+if (virNWFilterObjInitialize() < 0)
 return NULL;
+
+if (!(nwfilters = virObjectRWLockableNew(virNWFilterObjListClass)))
+return NULL;
+
+if (!(nwfilters->objs = virHashCreate(10, virObjectFreeHashData)) ||
+!(nwfilters->objsName = virHashCreate(10, virObjectFreeHashData))) {
+virObjectUnref(nwfilters);
+return NULL;
+}
+
 return nwfilters;
 }
 
@@ -149,20 +174,35 @@ void
 virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters,
  virNWFilterObjPtr obj)
 {
-size_t i;
+char uuidstr[VIR_UUID_STRING_BUFLEN];
 
+virUUIDFormat(obj->def->uuid, uuidstr);
+virObjectRef(obj);
 virObjectUnlock(obj);
 
-for (i = 0; i < nwfilters->count; i++) {
-virObjectLock(nwfilters->objs[i]);
-if (nwfilters->objs[i] == obj) {
-virNWFilterObjEndAPI(>objs[i]);
+virObjectRWLockWrite(nwfilters);
+virObjectLock(obj);
+virHashRemoveEntry(nwfilters->objs, uuidstr);
+virHashRemoveEntry(nwfilters->objsName, obj->def->name);
+virObjectUnlock(obj);
+virObjectUnref(obj);
+virObjectRWUnlock(nwfilters);
+}
+
+
+static virNWFilterObjPtr
+virNWFilterObjListFindByUUIDLocked(virNWFilterObjListPtr nwfilters,
+   const unsigned char *uuid)
+{
+virNWFilterObjPtr obj = NULL;
+char uuidstr[VIR_UUID_STRING_BUFLEN];
+
+virUUIDFormat(uuid, uuidstr);
 
-VIR_DELETE_ELEMENT(nwfilters->objs, i, nwfilters->count);
-break;
-}
-virObjectUnlock(nwfilters->objs[i]);
-}
+obj = virHashLookup(nwfilters->objs, uuidstr);
+if (obj)
+virObjectRef(obj);
+return obj;
 }
 
 
@@ -170,20 +210,27 @@ virNWFilterObjPtr
 

Re: [libvirt] [PATCH 00/14] Basic implementation of persistent reservations

2018-02-12 Thread Michal Privoznik
On 01/18/2018 05:04 PM, Michal Privoznik wrote:
>

Ping?

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


Re: [libvirt] [PATCH v2] vcpupin: add clear feature

2018-02-12 Thread Peter Krempa
On Mon, Feb 12, 2018 at 13:42:02 +, Daniel Berrange wrote:
> On Mon, Feb 12, 2018 at 02:31:46PM +0100, Peter Krempa wrote:
> > On Mon, Feb 12, 2018 at 09:39:26 +, Daniel Berrange wrote:
> > > On Mon, Feb 12, 2018 at 03:54:21AM -0500, Yi Wang wrote:
> > > > We can't clear vcpupin settings of XML once we did vcpupin
> > > > command, this is not convenient under some condition such
> > > > as migration to a host with less CPUs.
> > > > 
> > > > This patch introduces clear feature, which can clear vcpuin
> > > > setting of XML using a 'c' option.
> > > > 
> > > > Signed-off-by: Yi Wang 
> > > > Signed-off-by: Xi Xu 
> > > > ---
> > > >  include/libvirt/libvirt-domain.h | 11 +++
> > > >  src/qemu/qemu_driver.c   | 32 
> > > 
> > > I'm not seeing a good reason for these change. There's no concept of 
> > > clearing
> > > CPU pinning - the default is simply "pinned" to all possible CPUs, and the
> > > callers should already be able to request all possile CPUs with the 
> > > current
> > > API design.
> > 
> > Well, the thing is that in some cases the default is not pinned to all
> > pCPUs, but rather can be taken from other configuration points, e.g.
> > global VM pinning bitmap.
> 
> Which global VM pinning bitmap are you referring to ?

2

The above 'cpuset' is the default pinning for all vcpus, unless a vcpu
specifies individual other pinning.

> IIRC, when we spawned QEMU we explicitly set it to use all pCPUs, so
> that it doesn't inherit whatever affinity libvirtd itself has. I see
> we slightly tuned that to only include CPUs that are currently online
> (as reported in /sys/devices/system/cpu/online).
> 
> Except I see it is even more complicated.  We only use the online
> bitmap check if virHostCPUHasBitmap() returns true. We also potentially
> asked numad to give us default placement.

Yes, that is the second possible source of pinning information if the
user requests automatic pinning.

> So as implemented this _CLEAR flag is still potentially significantly
> different to what the original affinity was set to, which I think is
> pretty bad semantically.

I did not look at the implementation here, but basically this should do
the same logic as virDomainDefGetVcpuPinInfoHelper does to determine
which bitmap is actually used. The hierarchy is:

1) specific per-vcpu bitmap
2) automatic pinning from numad if enabled
3) 
4) all physical vcpus present on the host.

The above algorithm is used in all the pinning code to determine the
effective bitmap

> 
> > As of such the current code does not allow restoring such state since
> > pinning to all pCPUs might be a desired legitimate configuration so I've
> > removed the hack that deleted the pinning for such configuration a long
> > time ago. This means that an explicit removal of the pinning might be a
> > desired behaviour of the API, since abusing of any other value to
> > restore the default state is not a good idea.
> 
> True, but I think it rather a hack to modify this API with a flag _CLEAR
> that essentially means "ignore the bitmap parameter", as opposed to
> creating an API virDomainClearCPUAffinity(dom).

Yes, this is probably a better solution. The API needs a 'vcpu' argument
though:

virDomainClearVcpuPin(virDomainPtr dom,
  unsigned int vcpu,
  unsigned int flags)

> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 0/3] Fix parsing and formatting of 'UnixSocketAddress' qapi type

2018-02-12 Thread Peter Krempa
Fix the naming mistake, add tests and remove useless comment.

Peter Krempa (3):
  storage: Fix formatting and parsing of qemu type 'UnixSocketAddress'
  virstoragetest: Add test case for NBD over unix socket with new syntax
  qemu: block: Remove misleading part of comment in
qemuBlockStorageSourceBuildJSONSocketAddress

 src/qemu/qemu_block.c |  7 +--
 src/util/virstoragefile.c |  2 +-
 tests/virstoragetest.c| 13 +++--
 3 files changed, 13 insertions(+), 9 deletions(-)

-- 
2.15.0

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


Re: [libvirt] [PATCH] Revert "qemu: Expose rx/tx_queue_size in qemu.conf too"

2018-02-12 Thread Peter Krempa
On Mon, Feb 12, 2018 at 14:56:49 +, Daniel Berrange wrote:
> This reverts commit 038eb472a0d970a17ccf4343ead0666df5c92f9d.
> 
> On reflection adding defaults for arbitrary guest XML device config
> settings to the qemu.conf is not a sustainable path. Removing the
> support for rx/tx queue size so that it doesn't set a bad precedent.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  docs/formatdomain.html.in  | 14 ++
>  src/qemu/libvirtd_qemu.aug |  4 ---
>  src/qemu/qemu.conf |  6 -
>  src/qemu/qemu_command.c| 55 
> +++---
>  src/qemu/qemu_command.h|  3 +--
>  src/qemu/qemu_conf.c   |  4 ---
>  src/qemu/qemu_conf.h   |  3 ---
>  src/qemu/qemu_hotplug.c|  2 +-
>  src/qemu/test_libvirtd_qemu.aug.in |  2 --
>  9 files changed, 19 insertions(+), 74 deletions(-)

ACK


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 02/14] qemuDomainDiskChangeSupported: Deny changing reservations

2018-02-12 Thread Peter Krempa
On Thu, Jan 18, 2018 at 17:04:34 +0100, Michal Privoznik wrote:
> Couple of reasons for that:
> 
> a) there's no monitor command to change path where the pr-helper
> connects to, or
> b) there's no monitor command to introduce a new pr-helper for a
> disk that already exists.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/libvirt_private.syms  |  1 +
>  src/qemu/qemu_domain.c|  8 
>  src/util/virstoragefile.c | 18 ++
>  src/util/virstoragefile.h |  2 ++
>  4 files changed, 29 insertions(+)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index b0dc76b91..424f2ef64 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2719,6 +2719,7 @@ virStorageNetHostTransportTypeToString;
>  virStorageNetProtocolTypeToString;
>  virStoragePRDefFormat;
>  virStoragePRDefFree;
> +virStoragePRDefIsEqual;
>  virStoragePRDefParseNode;
>  virStorageSourceBackingStoreClear;
>  virStorageSourceClear;
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 441bf5935..e8539dcab 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -6865,6 +6865,14 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk,
>  CHECK_EQ(src->readonly, "readonly", true);
>  CHECK_EQ(src->shared, "shared", true);
>  
> +if (!virStoragePRDefIsEqual(disk->src->pr,
> +orig_disk->src->pr)) {
> +virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> +   _("cannot modify field '%s' of the disk"),
> +   "reservations");

Is there a particular reason to format 'reservations' separately?

Translations should not translate it, but doing this is generally deemed
translation unfriendly.

Rest looks good to me.


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] Should we switch to a different JSON library?

2018-02-12 Thread Pino Toscano
On Tuesday, 7 November 2017 14:05:25 CET Martin Kletzander wrote:
>  - Jansson [3] - I really like this one.  The API seems very intuitive,
>  it has nice documentation [4] in readthedocs (and I'm
>  not talking about the visual style, but how easy is to
>  find information), it can be used for formatting JSON
>  in a similar way we are doing it.  It has json_auto_t
>  (optional) type that uses the attribute cleanup for
>  automatic scope dereference (just in case we want to
>  use it), it has iterators... did I tell you I like this
>  one a lot?
> 
> What do you (others) think of switching the JSON library?  Do you know
> about any other projects that could be used considering license,
> platform support, etc.?  Also feel free to fix any mistakes I might have
> posted.  I double-checked it, but you know, "trust, but verify".

FYI: libguestfs just switched to Jansson [1], so any version starting
from 1.39.1 will use it instead of Yajl.  In case of libguestfs, yajl
was used directly, without wrappers of any sort, and the switch was
straightforward.

[1] 
https://github.com/libguestfs/libguestfs/commit/bd1c5c9f4dcf38458099db8a0bf4659a07ef055d

-- 
Pino Toscano

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 v2] vcpupin: add clear feature

2018-02-12 Thread Daniel P . Berrangé
On Mon, Feb 12, 2018 at 02:31:46PM +0100, Peter Krempa wrote:
> On Mon, Feb 12, 2018 at 09:39:26 +, Daniel Berrange wrote:
> > On Mon, Feb 12, 2018 at 03:54:21AM -0500, Yi Wang wrote:
> > > We can't clear vcpupin settings of XML once we did vcpupin
> > > command, this is not convenient under some condition such
> > > as migration to a host with less CPUs.
> > > 
> > > This patch introduces clear feature, which can clear vcpuin
> > > setting of XML using a 'c' option.
> > > 
> > > Signed-off-by: Yi Wang 
> > > Signed-off-by: Xi Xu 
> > > ---
> > >  include/libvirt/libvirt-domain.h | 11 +++
> > >  src/qemu/qemu_driver.c   | 32 
> > 
> > I'm not seeing a good reason for these change. There's no concept of 
> > clearing
> > CPU pinning - the default is simply "pinned" to all possible CPUs, and the
> > callers should already be able to request all possile CPUs with the current
> > API design.
> 
> Well, the thing is that in some cases the default is not pinned to all
> pCPUs, but rather can be taken from other configuration points, e.g.
> global VM pinning bitmap.

Which global VM pinning bitmap are you referring to ?

IIRC, when we spawned QEMU we explicitly set it to use all pCPUs, so
that it doesn't inherit whatever affinity libvirtd itself has. I see
we slightly tuned that to only include CPUs that are currently online
(as reported in /sys/devices/system/cpu/online).

Except I see it is even more complicated.  We only use the online
bitmap check if virHostCPUHasBitmap() returns true. We also potentially
asked numad to give us default placement.

So as implemented this _CLEAR flag is still potentially significantly
different to what the original affinity was set to, which I think is
pretty bad semantically.

> As of such the current code does not allow restoring such state since
> pinning to all pCPUs might be a desired legitimate configuration so I've
> removed the hack that deleted the pinning for such configuration a long
> time ago. This means that an explicit removal of the pinning might be a
> desired behaviour of the API, since abusing of any other value to
> restore the default state is not a good idea.

True, but I think it rather a hack to modify this API with a flag _CLEAR
that essentially means "ignore the bitmap parameter", as opposed to
creating an API virDomainClearCPUAffinity(dom).

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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


[libvirt] [PATCH] Revert "qemu: Expose rx/tx_queue_size in qemu.conf too"

2018-02-12 Thread Daniel P . Berrangé
This reverts commit 038eb472a0d970a17ccf4343ead0666df5c92f9d.

On reflection adding defaults for arbitrary guest XML device config
settings to the qemu.conf is not a sustainable path. Removing the
support for rx/tx queue size so that it doesn't set a bad precedent.

Signed-off-by: Daniel P. Berrangé 
---
 docs/formatdomain.html.in  | 14 ++
 src/qemu/libvirtd_qemu.aug |  4 ---
 src/qemu/qemu.conf |  6 -
 src/qemu/qemu_command.c| 55 +++---
 src/qemu/qemu_command.h|  3 +--
 src/qemu/qemu_conf.c   |  4 ---
 src/qemu/qemu_conf.h   |  3 ---
 src/qemu/qemu_hotplug.c|  2 +-
 src/qemu/test_libvirtd_qemu.aug.in |  2 --
 9 files changed, 19 insertions(+), 74 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 6707744bda..3ec1173c6f 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -5456,12 +5456,7 @@ qemu-kvm -net nic,model=? /dev/null
 some restrictions on actual value. For instance, latest
 QEMU (as of 2016-09-01) requires value to be a power of two
 from [256, 1024] range.
-Since 2.3.0 (QEMU and KVM only)
-Additionally, since 4.1.0 the
-value can be set in the qemu.conf file in order
-to override the hypervisor default value. Note that XML has
-higher precedence because it's more specific.
-
+Since 2.3.0 (QEMU and KVM only)
 
 In general you should leave this option alone, unless you
 are very certain you know what you are doing.
@@ -5477,12 +5472,7 @@ qemu-kvm -net nic,model=? /dev/null
 range. In addition to that, this may work only for a subset of
 interface types, e.g. aforementioned QEMU enables this option
 only for vhostuser type.
-Since 3.7.0 (QEMU and KVM only)
-Additionally, since 4.1.0 the
-value can be set in the qemu.conf file in order
-to override the hypervisor default value. Note that XML has
-higher precedence because it's more specific.
-
+Since 3.7.0 (QEMU and KVM only)
 
 In general you should leave this option alone, unless you
 are very certain you know what you are doing.
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index 084290296a..c19bf3a43a 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -118,9 +118,6 @@ module Libvirtd_qemu =
let vxhs_entry = bool_entry "vxhs_tls"
  | str_entry "vxhs_tls_x509_cert_dir"
 
-   let virtio_entry = int_entry "rx_queue_size"
- | int_entry "tx_queue_size"
-
(* Each entry in the config is one of the following ... *)
let entry = default_tls_entry
  | vnc_entry
@@ -140,7 +137,6 @@ module Libvirtd_qemu =
  | gluster_debug_level_entry
  | memory_entry
  | vxhs_entry
- | virtio_entry
 
let comment = [ label "#comment" . del /#[ \t]*/ "# " .  store /([^ 
\t\n][^\n]*)?/ . del /\n/ "\n" ]
let empty = [ label "#empty" . eol ]
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index 62c4265ea9..43dd561cca 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -775,9 +775,3 @@
 # This directory is used for memoryBacking source if configured as file.
 # NOTE: big files will be stored here
 #memory_backing_dir = "/var/lib/libvirt/qemu/ram"
-
-# The following two values set the default RX/TX ring buffer size for virtio
-# interfaces. These values are taken unless overridden in domain XML. For more
-# info consult docs to corresponding attributes from domain XML.
-#rx_queue_size = 1024
-#tx_queue_size = 1024
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 2bcaa5fc22..f7925c93a8 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3351,8 +3351,7 @@ qemuBuildNicStr(virDomainNetDefPtr net,
 
 
 char *
-qemuBuildNicDevStr(virQEMUDriverConfigPtr cfg,
-   virDomainDefPtr def,
+qemuBuildNicDevStr(virDomainDefPtr def,
virDomainNetDefPtr net,
int vlan,
unsigned int bootindex,
@@ -3472,41 +3471,21 @@ qemuBuildNicDevStr(virQEMUDriverConfigPtr cfg,
 virBufferAsprintf(, ",mq=on,vectors=%zu", 2 * vhostfdSize + 2);
 }
 }
-if (usingVirtio) {
-unsigned int rx_queue_size = net->driver.virtio.rx_queue_size;
-
-if (rx_queue_size == 0)
-rx_queue_size = cfg->rx_queue_size;
-
-if (rx_queue_size) {
-if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_RX_QUEUE_SIZE)) 
{
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("virtio rx_queue_size option is "
- "not supported with this QEMU binary"));
-goto error;
-}
-
-

Re: [libvirt] [PATCH 00/14] Basic implementation of persistent reservations

2018-02-12 Thread Peter Krempa
On Thu, Jan 18, 2018 at 17:04:32 +0100, Michal Privoznik wrote:
> QEMU added support for SCSI persistent reservations. The way QEMU
> implemented that makes it slightly harder for libvirt to adapt to - it's
> a small binary that needs to start before qemu so that it can connect to
> it. Basically, what libvirt does is:
> 
> 1) start qemu-pr-helper (the small binary from above),
> 2) records its PID for tracking purposes
> 3) start qemu
> 
> and when shutting down a domain the process is reversed:
> 
> 1) shut down qemu process,
> 2) kill qemu-pr-helper (PID was saved earlier).
> 
> Now, here also lies the last missing piece to he puzzle - what if
> qemu-pr-helper dies while qemu is running? After some discussion with
> Paolo it was agreed that qemu will emit an event for libvirt so that we
> can respawn the process. Anyway, that shouldn't be a show stopper for
> these patches (if it was the patch set might get too big).

Well, in general we should not allow these patches without that
sub-feature so that we don't have a version of libvirt out there which
will not handle that part.

> 
> Please test, review and comment.
> 
> As usual, you can find the patches on my github too:
> 
> https://github.com/zippy2/libvirt/tree/pr
> 
> Michal Privoznik (14):
>   virstoragefile: Introduce virStoragePRDef
>   qemuDomainDiskChangeSupported: Deny changing reservations
>   qemu: Introduce pr-manager-helper capability
>   qemu_domain: Introduce qemuDomainDiskPRObject
>   qemu: Generate alias for pr-helper
>   qemu: Store prAlias and prPath in status XML
>   qemu: Generate cmd line at startup
>   qemu: Introduce pr_helper to qemu.conf
>   qemu: Start PR daemons on domain startup
>   qemu: Track PR daemons PIDs in status XML
>   qemu_domain: Introduce qemuDomainGetPRUsageCount
>   qemu_process.c: Introduce qemuProcessSetupPRDaemon
>   qemu_hotplug: Hotplug of reservations
>   qemu_hotplug: Hotunplug of reservations

Ordering of the patches should be modified in such way, that the feature
can be enabled only once all the supporting code is already in.

E.g. hotplug should be fixed prior to enabling it, or command line
generators should not allow formatting it prior to libvirtd being able
to start the PR daemon.


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/3] virstoragetest: Add test case for NBD over unix socket with new syntax

2018-02-12 Thread Peter Krempa
Use the new syntax which uses the 'UnixSocket' type in qemu.

Signed-off-by: Peter Krempa 
---
 tests/virstoragetest.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index ea3d2833dd..87519495f0 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -1494,6 +1494,15 @@ mymain(void)
"\n"
"  \n"
"\n");
+TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"nbd\","
+   "\"server\": { \"type\":\"unix\","
+ 
"\"path\":\"/path/socket\""
+   "}"
+  "}"
+"}",
+   "\n"
+   "  \n"
+   "\n");
 TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"ssh\","
"\"host\":\"example.org\","
"\"port\":\"6000\","
-- 
2.15.0

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


[libvirt] [PATCH 3/3] qemu: block: Remove misleading part of comment in qemuBlockStorageSourceBuildJSONSocketAddress

2018-02-12 Thread Peter Krempa
The array indexes are formatted if the JSON->commandline translator is
translating an array type. It does not at all depend on this function.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_block.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index eb63139ca0..3f37483c1e 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -472,11 +472,6 @@ qemuBlockStorageSourceGetURI(virStorageSourcePtr src)
  * Formats @hosts into a json object conforming to the 'SocketAddress' type
  * in qemu.
  *
- * This function can be used when only 1 src->nhosts is expected in order
- * to build a command without the array indices after "server.". That is
- * to see "server.type", "server.host", and "server.port" instead of
- * "server.#.type", "server.#.host", and "server.#.port".
- *
  * Returns a virJSONValuePtr for a single server.
  */
 static virJSONValuePtr
-- 
2.15.0

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


Re: [libvirt] [PATCH v2] vcpupin: add clear feature

2018-02-12 Thread Peter Krempa
On Mon, Feb 12, 2018 at 09:39:26 +, Daniel Berrange wrote:
> On Mon, Feb 12, 2018 at 03:54:21AM -0500, Yi Wang wrote:
> > We can't clear vcpupin settings of XML once we did vcpupin
> > command, this is not convenient under some condition such
> > as migration to a host with less CPUs.
> > 
> > This patch introduces clear feature, which can clear vcpuin
> > setting of XML using a 'c' option.
> > 
> > Signed-off-by: Yi Wang 
> > Signed-off-by: Xi Xu 
> > ---
> >  include/libvirt/libvirt-domain.h | 11 +++
> >  src/qemu/qemu_driver.c   | 32 
> 
> I'm not seeing a good reason for these change. There's no concept of clearing
> CPU pinning - the default is simply "pinned" to all possible CPUs, and the
> callers should already be able to request all possile CPUs with the current
> API design.

Well, the thing is that in some cases the default is not pinned to all
pCPUs, but rather can be taken from other configuration points, e.g.
global VM pinning bitmap.

As of such the current code does not allow restoring such state since
pinning to all pCPUs might be a desired legitimate configuration so I've
removed the hack that deleted the pinning for such configuration a long
time ago. This means that an explicit removal of the pinning might be a
desired behaviour of the API, since abusing of any other value to
restore the default state is not a good idea.


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/3] storage: Fix formatting and parsing of qemu type 'UnixSocketAddress'

2018-02-12 Thread Peter Krempa
The documentation for the JSON/qapi type 'UnixSocketAddress' states that
the unix socket path field is named 'path'. We used 'socket' by
mistake. Fix both the formatter and parser and test suite.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1544325

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_block.c | 2 +-
 src/util/virstoragefile.c | 2 +-
 tests/virstoragetest.c| 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 585f0255ee..eb63139ca0 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -509,7 +509,7 @@ 
qemuBlockStorageSourceBuildJSONSocketAddress(virStorageNetHostDefPtr host,
 case VIR_STORAGE_NET_HOST_TRANS_UNIX:
 if (virJSONValueObjectCreate(,
  "s:type", "unix",
- "s:socket", host->socket,
+ "s:path", host->socket,
  NULL) < 0)
 goto cleanup;
 break;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 7f878039ba..5705bb055b 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2893,7 +2893,7 @@ 
virStorageSourceParseBackingJSONSocketAddress(virStorageNetHostDefPtr host,
 } else if (STREQ(type, "unix")) {
 host->transport = VIR_STORAGE_NET_HOST_TRANS_UNIX;

-if (!(socket = virJSONValueObjectGetString(json, "socket"))) {
+if (!(socket = virJSONValueObjectGetString(json, "path"))) {
 virReportError(VIR_ERR_INVALID_ARG, "%s",
_("missing socket path for udp backing server in "
  "JSON backing volume definition"));
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index 6eed7134ed..ea3d2833dd 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -1420,7 +1420,7 @@ mymain(void)
   "\"port\":\"1234\""
 "},"
 "{ \"type\":\"unix\","
-  
"\"socket\":\"/path/socket\""
+  
"\"path\":\"/path/socket\""
 "},"
 "{ \"type\":\"tcp\","
   
"\"host\":\"example.com\""
@@ -1441,7 +1441,7 @@ mymain(void)
  "\"port\":\"1234\""
"},"
"{ \"type\":\"unix\","
- "\"socket\":\"/path/socket\""
+ "\"path\":\"/path/socket\""
"},"
"{ \"type\":\"inet\","
  "\"host\":\"example.com\""
-- 
2.15.0

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


Re: [libvirt] [PATCH 04/14] qemu_domain: Introduce qemuDomainDiskPRObject

2018-02-12 Thread Peter Krempa
On Thu, Jan 18, 2018 at 17:04:36 +0100, Michal Privoznik wrote:
> This is an extended definition of virStoragePRDef because it
> contains runtime information (like path to pr helper socket, its
> pid and alias). Since these are driver dependant we should have a
> driver specific structure instead of putting all of that into
> driver agnostic structure.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 120 
> +
>  src/qemu/qemu_domain.h |  18 
>  2 files changed, 138 insertions(+)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index e8539dcab..7fa8c93b7 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -65,6 +65,7 @@
>  #endif
>  #include 
>  #include 
> +#include 
>  #if defined(HAVE_SYS_MOUNT_H)
>  # include 
>  #endif
> @@ -1829,6 +1830,9 @@ qemuDomainObjPrivateDataClear(qemuDomainObjPrivatePtr 
> priv)
>  
>  virBitmapFree(priv->migrationCaps);
>  priv->migrationCaps = NULL;
> +
> +virHashFree(priv->prHelpers);
> +priv->prHelpers = NULL;
>  }
>  
>  
> @@ -10917,6 +10921,122 @@ 
> qemuDomainCheckMigrationCapabilities(virQEMUDriverPtr driver,
>  }
>  
>  
> +static void
> +qemuDomainDiskPRObjectHashFree(void *payload,
> +   const void *name)
> +{
> +qemuDomainDiskPRObjectPtr tmp = payload;
> +
> +if (tmp->managed &&
> +tmp->pid != (pid_t) -1) {
> +VIR_DEBUG("Forcibly killing pr-manager: %s", (const char *) name);
> +virProcessKillPainfully(tmp->pid, true);

Is this really a good idea? If you restart libvirtd gracefully, this
will kill all the PR daemons.

Also I'd prefer if all the process management code would be in
qemu_process.c.


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 03/14] qemu: Introduce pr-manager-helper capability

2018-02-12 Thread Peter Krempa
On Thu, Jan 18, 2018 at 17:04:35 +0100, Michal Privoznik wrote:
> The capability tracks if qemu has pr-manager-helper object.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_capabilities.c | 2 ++
>  src/qemu/qemu_capabilities.h | 1 +
>  2 files changed, 3 insertions(+)

ACK


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/1] includes: function parameter names same in headers

2018-02-12 Thread Daniel P . Berrangé
On Mon, Feb 12, 2018 at 12:20:55PM -0600, Chris Venteicher wrote:
> Headers use same function parameter names as definition code.
> ---
>  include/libvirt/libvirt-domain.h| 26 +-
>  include/libvirt/libvirt-event.h |  4 ++--
>  include/libvirt/libvirt-host.h  |  4 ++--
>  include/libvirt/libvirt-interface.h |  4 ++--
>  include/libvirt/libvirt-network.h   |  6 +++---
>  include/libvirt/libvirt-nwfilter.h  |  2 +-
>  include/libvirt/libvirt-qemu.h  |  2 +-
>  include/libvirt/libvirt-secret.h|  4 ++--
>  include/libvirt/libvirt-storage.h   | 12 ++--
>  include/libvirt/libvirt-stream.h| 22 +++---
>  10 files changed, 43 insertions(+), 43 deletions(-)
> 
> diff --git a/include/libvirt/libvirt-domain.h 
> b/include/libvirt/libvirt-domain.h
> index 4048acf38..60ec35d87 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h

> @@ -1161,11 +1161,11 @@ int virDomainFree   
> (virDomainPtr domain);
>   */
>  int virDomainSuspend(virDomainPtr domain);
>  int virDomainResume (virDomainPtr domain);
> -int virDomainPMSuspendForDuration (virDomainPtr domain,
> +int virDomainPMSuspendForDuration (virDomainPtr dom,

This changes domain -> dom , but then

>
> -int virDomainSetInterfaceParameters (virDomainPtr dom,
> +int virDomainSetInterfaceParameters (virDomainPtr domain,

This changes dom -> domain.

IMHO this is no better than we started with.  We should aim for consistency
naming, so rather than only changing the .h, we should change .c and .h
to match preferred naming.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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


Re: [libvirt] Should we switch to a different JSON library?

2018-02-12 Thread Ján Tomko

On Mon, Feb 12, 2018 at 02:38:02PM +0100, Pino Toscano wrote:

On Tuesday, 7 November 2017 14:05:25 CET Martin Kletzander wrote:

 - Jansson [3] - I really like this one.  The API seems very intuitive,
 it has nice documentation [4] in readthedocs (and I'm
 not talking about the visual style, but how easy is to
 find information), it can be used for formatting JSON
 in a similar way we are doing it.  It has json_auto_t
 (optional) type that uses the attribute cleanup for
 automatic scope dereference (just in case we want to
 use it), it has iterators... did I tell you I like this
 one a lot?

What do you (others) think of switching the JSON library?  Do you know
about any other projects that could be used considering license,
platform support, etc.?  Also feel free to fix any mistakes I might have
posted.  I double-checked it, but you know, "trust, but verify".


FYI: libguestfs just switched to Jansson [1], so any version starting
from 1.39.1 will use it instead of Yajl.  In case of libguestfs, yajl
was used directly, without wrappers of any sort, and the switch was
straightforward.

[1] 
https://github.com/libguestfs/libguestfs/commit/bd1c5c9f4dcf38458099db8a0bf4659a07ef055d



I do have a working virjson.c implementation in my local git waiting to be
polished and sent. The issues with libvirt were:
* virjson.c storing numbers as a string
* backwards compatibility (AFAIK we want to support building on
 RHEL/CentOS 6, which did not have recent enough jansson - for the
 _foreach macros, at least 2.5 is needed)
 If we really need to maintain two implementations side-by-side,
 one of them should have an expiration date.

I don't see any version check in that libguestfs commit, what are the
compatibility requirements there?

Jan


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 03/11] qemu: Move GIC checks to qemuDomainDefValidateFeatures()

2018-02-12 Thread John Ferlan


On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
> Keep them along with other arch/machine type checks for
> features instead of waiting until command line generation
> time.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_command.c  |  7 ---
>  src/qemu/qemu_domain.c   | 11 ++-
>  tests/qemuxml2argvtest.c | 12 ++--
>  3 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 24b434a45..529079be0 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7192,13 +7192,6 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
>  
>  if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON) 
> {
>  if (def->gic_version != VIR_GIC_VERSION_NONE) {
> -if (!qemuDomainIsVirt(def)) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -   _("gic-version option is available "
> - "only for ARM virt machine"));
> -goto cleanup;
> -}
> -
>  /* The default GIC version (GICv2) should not be specified on
>   * the QEMU commandline for backwards compatibility reasons 
> */
>  if (def->gic_version != VIR_GIC_VERSION_2) {
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index dd36b42eb..98cba8789 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -3252,6 +3252,16 @@ qemuDomainDefValidateFeatures(const virDomainDef *def)
>  }
>  break;
>  
> +case VIR_DOMAIN_FEATURE_GIC:
> +if (def->features[i] == VIR_TRISTATE_SWITCH_ON &&
> +!qemuDomainIsVirt(def)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("The '%s' feature is only supported for %s 
> guests"),

s/for %s guests/for '%s' guests/ ??

> +   featureName, "mach-virt");

Not that I think it matters greatly, the error message changes from ARM
specifically to mach-virt... I guess I'm just used to seeing 'ARM' or
'aarch64' and not 'mach-virt' (although the XML would be AIUI ""). Suffice to say it caused me to wonder
and I have to imagine it would do the same for anyone getting that message.

I don't have a strong opinion one way or other and it's not really
easily "decode-able" based on someone just reading the "

John

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


Re: [libvirt] Should we switch to a different JSON library?

2018-02-12 Thread Daniel P . Berrangé
On Mon, Feb 12, 2018 at 07:47:00PM +0100, Ján Tomko wrote:
> On Mon, Feb 12, 2018 at 02:38:02PM +0100, Pino Toscano wrote:
> > On Tuesday, 7 November 2017 14:05:25 CET Martin Kletzander wrote:
> > >  - Jansson [3] - I really like this one.  The API seems very intuitive,
> > >  it has nice documentation [4] in readthedocs (and I'm
> > >  not talking about the visual style, but how easy is to
> > >  find information), it can be used for formatting JSON
> > >  in a similar way we are doing it.  It has json_auto_t
> > >  (optional) type that uses the attribute cleanup for
> > >  automatic scope dereference (just in case we want to
> > >  use it), it has iterators... did I tell you I like this
> > >  one a lot?
> > > 
> > > What do you (others) think of switching the JSON library?  Do you know
> > > about any other projects that could be used considering license,
> > > platform support, etc.?  Also feel free to fix any mistakes I might have
> > > posted.  I double-checked it, but you know, "trust, but verify".
> > 
> > FYI: libguestfs just switched to Jansson [1], so any version starting
> > from 1.39.1 will use it instead of Yajl.  In case of libguestfs, yajl
> > was used directly, without wrappers of any sort, and the switch was
> > straightforward.
> > 
> > [1] 
> > https://github.com/libguestfs/libguestfs/commit/bd1c5c9f4dcf38458099db8a0bf4659a07ef055d
> > 
> 
> I do have a working virjson.c implementation in my local git waiting to be
> polished and sent. The issues with libvirt were:
> * virjson.c storing numbers as a string
> * backwards compatibility (AFAIK we want to support building on
>  RHEL/CentOS 6, which did not have recent enough jansson - for the
>  _foreach macros, at least 2.5 is needed)
>  If we really need to maintain two implementations side-by-side,
>  one of them should have an expiration date.

IMHO we only need to keepp yajl if RHEL6 doesn't have suitable jansson.
So ideally we'd figure out how to get that working, so we can ditch
yajl straightaway.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH 07/14] qemu: Generate cmd line at startup

2018-02-12 Thread Peter Krempa
On Thu, Jan 18, 2018 at 17:04:39 +0100, Michal Privoznik wrote:
> This is the easier part. All we need to do here is put -object
> pr-manger-helper,id=$alias,path=$socketPath and then just
> reference the object in -drive file.pr-manger=$alias.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_command.c| 59 
> ++
>  .../disk-virtio-scsi-reservations-not-managed.args | 28 ++
>  .../disk-virtio-scsi-reservations.args | 29 +++
>  tests/qemuxml2argvtest.c   |  8 +++
>  4 files changed, 124 insertions(+)
>  create mode 100644 
> tests/qemuxml2argvdata/disk-virtio-scsi-reservations-not-managed.args
>  create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index b8aede32d..13f2e4fd0 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1515,6 +1515,20 @@ qemuDiskSourceGetProps(virStorageSourcePtr src)
>  }
>  
>  
> +static void
> +qemuBuildDriveSourcePR(virBufferPtr buf,
> +   virStorageSourcePtr src)
> +{
> +qemuDomainStorageSourcePrivatePtr srcPriv = 
> QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
> +
> +if (!src->pr ||
> +src->pr->enabled != VIR_TRISTATE_BOOL_YES)

This code should really just look whether the alias is allocated rather
than duplicating the logic of when it's used.

> +return;
> +
> +virBufferAsprintf(buf, ",file.pr-manager=%s", srcPriv->prAlias);

According qemu.git/qapi/block-core.json, the pr-manager field is
supported only for '@BlockdevOptionsFile'. This means that you need to
check that it's used only for local paths which eventually translate to
BlockdevOptionsFile. Otherwise qemu will refuse this with any network
based disk. virStorageSourceIsLocalStorage is your friend.


> +
>  static int
>  qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
>  virQEMUCapsPtr qemuCaps,
> @@ -1591,6 +1605,8 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
>  
>  if (disk->src->debug)
>  virBufferAsprintf(buf, ",file.debug=%d", disk->src->debugLevel);
> +
> +qemuBuildDriveSourcePR(buf, disk->src);
>  } else {
>  if (!(source = virQEMUBuildDriveCommandlineFromJSON(srcprops)))
>  goto cleanup;
> @@ -10033,6 +10049,46 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
>  }
>  
>  
> +struct qemuBuildMasterPRCommandLineData {
> +virCommandPtr cmd;
> +};

Do you really need a single member struct for the void pointer?

> +
> +
> +static int
> +qemuBuildMasterPRCommandLineHelper(void *payload,
> +   const void *name,
> +   void *opaque)
> +{
> +qemuDomainDiskPRObjectPtr obj = payload;
> +struct qemuBuildMasterPRCommandLineData *data = opaque;
> +virBuffer buf = VIR_BUFFER_INITIALIZER;
> +const char *alias = name;
> +
> +virBufferAsprintf(, "pr-manager-helper,id=%s,path=%s", alias, 
> obj->path);
> +virCommandAddArg(data->cmd, "-object");
> +virCommandAddArgBuffer(data->cmd, );
> +return 0;
> +}
> +


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 13/14] qemu_hotplug: Hotplug of reservations

2018-02-12 Thread Peter Krempa
On Thu, Jan 18, 2018 at 17:04:45 +0100, Michal Privoznik wrote:
> Surprisingly, nothing special is happening here. If we are the
> first to use the managed helper then spawn it. If not, we're
> almost done.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_hotplug.c | 97 
> +
>  1 file changed, 97 insertions(+)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 6b245bd6a..ded33 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -350,6 +350,84 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
>  }
>  
>  
> +static int
> +qemuBuildPRDefInfoProps(virDomainObjPtr vm,
> +virDomainDiskDefPtr disk,
> +virJSONValuePtr *prmgrProps,
> +const char **prAlias)
> +{
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +qemuDomainStorageSourcePrivatePtr srcPriv;
> +qemuDomainDiskPRObjectPtr tmp;
> +virJSONValuePtr props = NULL;
> +int ret = -1;

This function used in conjucntion with the JSON->commandline formatter
should be used instead of qemuBuildMasterPRCommandLineHelper so that we
don't have multiple implementations which need to be kept in sync ...


> +
> +srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
> +
> +*prmgrProps = NULL;
> +
> +if (!priv->prHelpers ||
> +!srcPriv->prAlias ||
> +!(tmp = virHashLookup(priv->prHelpers, srcPriv->prAlias)))
> +return 0;
> +
> +if (qemuDomainGetPRUsageCount(vm->def, srcPriv->prAlias) != 0) {
> +/* We're not the first ones to use pr-manager with that
> + * alias.  Return early and leave @prmgrProps NULL so
> + * that caller knows this. */
> +return 0;
> +}

Obviously you'll still need a wrapper for this logic ...


> +
> +if (virJSONValueObjectCreate(,
> + "s:path", tmp->path,
> + NULL) < 0)

... but we should not have two different implementations of this.

> +goto cleanup;
> +
> +if (qemuProcessSetupPRDaemon(vm, tmp, srcPriv->prAlias) < 0)
> +goto cleanup;
> +
> +*prAlias = srcPriv->prAlias;
> +*prmgrProps = props;
> +props = NULL;
> +ret = 0;
> + cleanup:
> +virJSONValueFree(props);
> +return ret;
> +}
> +
> +
> +static void
> +qemuDestroyPRDefObject(virDomainObjPtr vm,
> +   virDomainDiskDefPtr disk)
> +{
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +qemuDomainStorageSourcePrivatePtr srcPriv;
> +qemuDomainDiskPRObjectPtr tmp;
> +
> +srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
> +
> +if (!priv->prHelpers ||
> +!srcPriv->prAlias ||
> +!(tmp = virHashLookup(priv->prHelpers, srcPriv->prAlias)))
> +return;
> +
> +if (qemuDomainGetPRUsageCount(vm->def, srcPriv->prAlias) > 1) {
> +/* The function might return one or even zero if the
> + * pr-manager is unused depending whether the disk was
> + * added to domain def already or not. Anyway, if the
> + * return value is greater than one we are certain that
> + * there's another disk using it so return early. */

Shouldn't we use a different approach rather than comments like this?


> +return;
> +}
> +
> +/* This also kills the pr-manager daemon. See
> + * qemuDomainDiskPRObjectHashFree. */
> +virHashRemoveEntry(priv->prHelpers, srcPriv->prAlias);
> +
> +VIR_FREE(srcPriv->prAlias);
> +}
> +
> +
>  /**
>   * qemuDomainAttachDiskGeneric:
>   *


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 02/14] qemuDomainDiskChangeSupported: Deny changing reservations

2018-02-12 Thread Peter Krempa
On Mon, Feb 12, 2018 at 17:19:24 +0100, Andrea Bolognani wrote:
> On Mon, 2018-02-12 at 16:40 +0100, Peter Krempa wrote:
> > > @@ -6865,6 +6865,14 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr 
> > > disk,
> > >  CHECK_EQ(src->readonly, "readonly", true);
> > >  CHECK_EQ(src->shared, "shared", true);
> > >  
> > > +if (!virStoragePRDefIsEqual(disk->src->pr,
> > > +orig_disk->src->pr)) {
> > > +virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> > > +   _("cannot modify field '%s' of the disk"),
> > > +   "reservations");
> > 
> > Is there a particular reason to format 'reservations' separately?
> > 
> > Translations should not translate it, but doing this is generally deemed
> > translation unfriendly.
> 
> Wouldn't that actually be better for translators? Assuming there's
> going to be more than a single field, you can translate the message
> once and it will work for all instances.
> 
> Of course that only works if, like in this case, the variable part
> itself does not need translation.

Um yes, in this case it will actually help due to the surrounding code
using that pattern a lot and also tha the field should not be
translated.

I retract my comment.

ACK


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 09/14] qemu: Start PR daemons on domain startup

2018-02-12 Thread Daniel P . Berrangé
On Mon, Feb 12, 2018 at 05:54:19PM +0100, Peter Krempa wrote:
> On Thu, Jan 18, 2018 at 17:04:41 +0100, Michal Privoznik wrote:
> > Before we exec() qemu we have to spawn pr-helper processes for
> > all managed reservations (well, technically there can only one).
> > The only caveat there is that we should place the process into
> > the same namespace and cgroup as qemu (so that it shares the same
> > view of the system). But we can do that only after we've forked.
> > That means calling the setup function between fork() and exec().
> > 
> > Signed-off-by: Michal Privoznik 
> > ---
> >  src/qemu/qemu_process.c | 151 
> > 
> >  1 file changed, 151 insertions(+)
> > 
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index 25ec464d3..02608c1f3 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> > @@ -2507,6 +2507,151 @@ qemuProcessSetupEmulator(virDomainObjPtr vm)
> >  }
> >  
> >  
> > +static int
> > +qemuProcessSetupOnePRDaemonHook(void *opaque)
> > +{
> > +virDomainObjPtr vm = opaque;
> > +size_t i, nfds = 0;
> > +int *fds = NULL;
> > +int ret = -1;
> > +
> > +if (virProcessGetNamespaces(vm->pid, , ) < 0)
> 
> If this detects '0' namespaces ...
> 
> > +return ret;
> > +
> > +if (virProcessSetNamespaces(nfds, fds) < 0)
> 
> ... this call will be unhappy.

Namespaces have been around since at least RHEL-5 vintage so I reckon
we can assume non-zero number of namespaces. In fact perhaps we should
just make virProcessGetNamespaces() return an explicit error in the
unlikely case it finds zero namespaces.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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


Re: [libvirt] [PATCH 09/14] qemu: Start PR daemons on domain startup

2018-02-12 Thread Peter Krempa
On Mon, Feb 12, 2018 at 17:10:23 +, Daniel Berrange wrote:
> On Mon, Feb 12, 2018 at 05:54:19PM +0100, Peter Krempa wrote:
> > On Thu, Jan 18, 2018 at 17:04:41 +0100, Michal Privoznik wrote:
> > > Before we exec() qemu we have to spawn pr-helper processes for
> > > all managed reservations (well, technically there can only one).
> > > The only caveat there is that we should place the process into
> > > the same namespace and cgroup as qemu (so that it shares the same
> > > view of the system). But we can do that only after we've forked.
> > > That means calling the setup function between fork() and exec().
> > > 
> > > Signed-off-by: Michal Privoznik 
> > > ---
> > >  src/qemu/qemu_process.c | 151 
> > > 
> > >  1 file changed, 151 insertions(+)
> > > 
> > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > > index 25ec464d3..02608c1f3 100644
> > > --- a/src/qemu/qemu_process.c
> > > +++ b/src/qemu/qemu_process.c
> > > @@ -2507,6 +2507,151 @@ qemuProcessSetupEmulator(virDomainObjPtr vm)
> > >  }
> > >  
> > >  
> > > +static int
> > > +qemuProcessSetupOnePRDaemonHook(void *opaque)
> > > +{
> > > +virDomainObjPtr vm = opaque;
> > > +size_t i, nfds = 0;
> > > +int *fds = NULL;
> > > +int ret = -1;
> > > +
> > > +if (virProcessGetNamespaces(vm->pid, , ) < 0)
> > 
> > If this detects '0' namespaces ...
> > 
> > > +return ret;
> > > +
> > > +if (virProcessSetNamespaces(nfds, fds) < 0)
> > 
> > ... this call will be unhappy.
> 
> Namespaces have been around since at least RHEL-5 vintage so I reckon
> we can assume non-zero number of namespaces. In fact perhaps we should
> just make virProcessGetNamespaces() return an explicit error in the
> unlikely case it finds zero namespaces.

Fair enough, but it still does not look like the right usage semantics
in this case.


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 06/14] qemu: Store prAlias and prPath in status XML

2018-02-12 Thread Peter Krempa
On Thu, Jan 18, 2018 at 17:04:38 +0100, Michal Privoznik wrote:
> Now that we generate pr-manger alias and socket path store them
> in status XML so that they are preserved across daemon restarts.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 72 
> --
>  1 file changed, 70 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index e8d2adf56..97996b053 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2449,6 +2449,74 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
>  }
>  
>  
> +static int
> +qemuStorageSourcePrivateDataParsePR(xmlXPathContextPtr ctxt,
> +virStorageSourcePtr src)
> +{
> +qemuDomainStorageSourcePrivatePtr srcPriv;
> +
> +if (!src->pr ||
> +src->pr->enabled != VIR_TRISTATE_BOOL_YES)
> +return 0;

This logic should not be here. If there is the field parse it. If it's
not don't parse it.

> +
> +if (!src->privateData &&
> +!(src->privateData = qemuDomainStorageSourcePrivateNew()))
> +return -1;

Hmmm, we probably should make sure that this is always allocated in the
qemu driver as a separate patch preceeding this series.

> +
> +srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
> +
> +if (!(srcPriv->prAlias = virXPathString("string(./prAlias)", ctxt)))
> +return -1;
> +
> +return 0;
> +}
> +
> +
> +static int
> +qemuStorageSourcePrivateDataFormatPR(virStorageSourcePtr src,
> + virBufferPtr buf)
> +{
> +qemuDomainStorageSourcePrivatePtr srcPriv;
> +
> +if (!src->pr ||
> +src->pr->enabled != VIR_TRISTATE_BOOL_YES)
> +return 0;

Same here, this logic should not be here. Presence of the alias should
be the key.

> +
> +srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
> +
> +virBufferAsprintf(buf, "%s\n", srcPriv->prAlias);
> +return 0;
> +}



signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 05/14] qemu: Generate alias for pr-helper

2018-02-12 Thread Peter Krempa
On Thu, Jan 18, 2018 at 17:04:37 +0100, Michal Privoznik wrote:
> While we're not generating the command line just yet (look for
> the next commit), we can generate the alias for pr-manager.
> A domain can have up to one managed pr-manager (in which case
> socket path is decided by libvirt and pr-helper is spawned by
> libvirt too), but up to ndisks of unmanaged ones (one per disk
> basically). In case of the former we can generate a simple alias
> and be sure it'll not conflict. But in case of the latter to
> avoid any conflicts let's generate alias that's based on
> something unique - like disk target.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 61 
> ++
>  src/qemu/qemu_domain.h |  3 +++
>  2 files changed, 64 insertions(+)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 7fa8c93b7..e8d2adf56 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -972,6 +972,8 @@ qemuDomainStorageSourcePrivateDispose(void *obj)
>  
>  qemuDomainSecretInfoFree(>secinfo);
>  qemuDomainSecretInfoFree(>encinfo);
> +
> +VIR_FREE(priv->prAlias);
>  }
>  
>  
> @@ -11037,6 +11039,62 @@ 
> qemuDomainDiskPRObjectKillAll(qemuDomainObjPrivatePtr priv)
>  }
>  
>  
> +static int
> +qemuDomainPrepareDiskPR(qemuDomainObjPrivatePtr priv,
> +virDomainDiskDefPtr disk)
> +{
> +qemuDomainStorageSourcePrivatePtr srcPriv;
> +virStoragePRDefPtr prd = disk->src->pr;
> +char *prPath = NULL;
> +bool managed;
> +int ret = -1;
> +
> +if (!prd ||
> +prd->enabled != VIR_TRISTATE_BOOL_YES)
> +return 0;
> +
> +if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_PR_MANAGER_HELPER)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("reservations not supported with this QEMU 
> binary"));
> +goto cleanup;
> +}
> +
> +if (!disk->src->privateData &&
> +!(disk->src->privateData = qemuDomainStorageSourcePrivateNew()))
> +return -1;
> +
> +srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
> +managed = prd->managed == VIR_TRISTATE_BOOL_YES;
> +
> +if (managed) {
> +/* Generate PR helper socket path & alias that are same
> + * for each disk in the domain. */
> +
> +if (VIR_STRDUP(srcPriv->prAlias, "pr-helper0") < 0)
> +return -1;
> +
> +if (virAsprintf(, "%s/pr-helper0.sock", priv->libDir) < 0)
> +return -1;
> +
> +} else {
> +if (virAsprintf(>prAlias, "pr-helper-%s", disk->dst) < 0)
> +return -1;


I though that unmanaged PRs would be shared which would make sense given
the data structure you've introduced in patch 4. Since this code would
in that case make problems with hotplug I looked back at that code.

So, this means that there's exactly one managed PR daemon and every
unmanaged PR daemon has possibly multiple instances/connections from
qemu.

So, that brings me to the question: Do you really need the prHelpers
hash table? If the instances are duplicated, you can store the data in
virStorageSource (in the private data) and don't really need the table.

If there's intent to add sharing of the pr objects in qemu, you'll need
to rething this code, since generating the alias will create problems
when you hotunplug a shared PR instance and try to plug back a disk with
a different one.


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 09/14] qemu: Start PR daemons on domain startup

2018-02-12 Thread Peter Krempa
On Thu, Jan 18, 2018 at 17:04:41 +0100, Michal Privoznik wrote:
> Before we exec() qemu we have to spawn pr-helper processes for
> all managed reservations (well, technically there can only one).
> The only caveat there is that we should place the process into
> the same namespace and cgroup as qemu (so that it shares the same
> view of the system). But we can do that only after we've forked.
> That means calling the setup function between fork() and exec().
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_process.c | 151 
> 
>  1 file changed, 151 insertions(+)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 25ec464d3..02608c1f3 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -2507,6 +2507,151 @@ qemuProcessSetupEmulator(virDomainObjPtr vm)
>  }
>  
>  
> +static int
> +qemuProcessSetupOnePRDaemonHook(void *opaque)
> +{
> +virDomainObjPtr vm = opaque;
> +size_t i, nfds = 0;
> +int *fds = NULL;
> +int ret = -1;
> +
> +if (virProcessGetNamespaces(vm->pid, , ) < 0)

If this detects '0' namespaces ...

> +return ret;
> +
> +if (virProcessSetNamespaces(nfds, fds) < 0)

... this call will be unhappy.

> +goto cleanup;
> +
> +ret = 0;
> + cleanup:
> +for (i = 0; i < nfds; i++)
> +VIR_FORCE_CLOSE(fds[i]);
> +VIR_FREE(fds);
> +return ret;
> +}
> +
> +
> +static int
> +qemuProcessSetupOnePRDaemon(void *payload,
> +const void *name,
> +void *opaque)
> +{
> +qemuDomainDiskPRObjectPtr prObj = payload;
> +virDomainObjPtr vm = opaque;
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +virQEMUDriverPtr driver = priv->driver;
> +virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> +char *pidfile = NULL;
> +pid_t cpid = -1;
> +virCommandPtr cmd = NULL;
> +virTimeBackOffVar timebackoff;
> +const unsigned long long timeout = 50; /* ms */
> +int ret = -1;
> +
> +if (!prObj->managed)
> +return 0;

Again. It does not make much sense to me to have the hash table and
everything if this is ever going to be called for the managed daemon.

> +
> +if (!virFileIsExecutable(cfg->prHelperName)) {
> +virReportSystemError(errno, _("'%s' is not a suitable pr helper"),

This error message does not really report what it's checking.

> + cfg->prHelperName);
> +goto cleanup;
> +}
> +
> +if (!(pidfile = virPidFileBuildPath(cfg->stateDir, name)))
> +goto cleanup;
> +
> +if (unlink(pidfile) < 0 &&
> +errno != ENOENT) {
> +virReportSystemError(errno,
> + _("Cannot remove stale PID file %s"),
> + pidfile);
> +goto cleanup;
> +}
> +
> +if (!(cmd = virCommandNewArgList(cfg->prHelperName,
> + "-k", prObj->path,
> + NULL)))
> +goto cleanup;
> +
> +virCommandDaemonize(cmd);
> +virCommandSetPidFile(cmd, pidfile);
> +virCommandSetPreExecHook(cmd, qemuProcessSetupOnePRDaemonHook, vm);
> +
> +#ifdef CAP_SYS_RAWIO
> +virCommandAllowCap(cmd, CAP_SYS_RAWIO);
> +#else
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("Raw I/O is not supported on this platform"));
> +goto cleanup;

This also looks like worth putting in the validation code for this so
that we don't really get to here to find out.


> +#endif
> +
> +if (virCommandRun(cmd, NULL) < 0)
> +goto cleanup;
> +
> +if (virPidFileReadPath(pidfile, ) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("pr helper %s didn't show up"), (const char *) 
> name);
> +goto cleanup;
> +}
> +
> +if (virTimeBackOffStart(, 1, timeout) < 0)
> +goto cleanup;
> +while (virTimeBackOffWait()) {
> +if (virFileExists(prObj->path))
> +break;
> +
> +if (virProcessKill(cpid, 0) == 0)
> +continue;
> +
> +virReportSystemError(errno, "%s",
> + _("pr helper died unexpectedly"));
> +goto cleanup;
> +}

Sooo, after the timeout you assume success? That does not seem like a
reasonable idea.

> +
> +if (priv->cgroup &&
> +virCgroupAddMachineTask(priv->cgroup, cpid) < 0)
> +goto cleanup;
> +
> +if (qemuSecurityDomainSetPathLabel(driver->securityManager,
> +   vm->def, prObj->path, true) < 0)
> +goto cleanup;
> +
> +prObj->pid = cpid;
> +ret = 0;
> + cleanup:
> +if (ret < 0) {
> +virCommandAbort(cmd);
> +virProcessKillPainfully(cpid, true);
> +}
> +virCommandFree(cmd);
> +if (unlink(pidfile) < 0 &&
> +errno != ENOENT &&
> +!virGetLastError())
> +   

[libvirt] [PATCH] qemu: Fix indentation in qemuBuildControllerDevStr()

2018-02-12 Thread Andrea Bolognani
Add braces around the multi-line body as well, in compliance
with our coding style.

Signed-off-by: Andrea Bolognani 
---
Pushed as trivial.

 src/qemu/qemu_command.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index f7925c93a..6c73cd7bf 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2743,9 +2743,10 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
 virBufferAsprintf(, "%s,bus_nr=%d,id=%s",
   modelName, pciopts->busNr,
   def->info.alias);
-if (pciopts->numaNode != -1)
-   virBufferAsprintf(, ",numa_node=%d",
- pciopts->numaNode);
+if (pciopts->numaNode != -1) {
+virBufferAsprintf(, ",numa_node=%d",
+  pciopts->numaNode);
+}
 break;
 case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
 case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
-- 
2.14.3

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


Re: [libvirt] [PATCH 10/14] qemu: Track PR daemons PIDs in status XML

2018-02-12 Thread Peter Krempa
On Thu, Jan 18, 2018 at 17:04:42 +0100, Michal Privoznik wrote:
> We need to keep track of spawned processes so that we can kill
> them when qemu process dies.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 130 
> +
>  1 file changed, 130 insertions(+)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 97996b053..4079165f9 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -131,6 +131,10 @@ static virClassPtr qemuDomainSaveCookieClass;
>  static void qemuDomainLogContextDispose(void *obj);
>  static void qemuDomainSaveCookieDispose(void *obj);
>  
> +static void
> +qemuDomainDiskPRObjectHashFree(void *payload,
> +   const void *name);
> +
>  static int
>  qemuDomainOnceInit(void)
>  {
> @@ -1954,6 +1958,46 @@ qemuDomainObjPrivateXMLFormatAllowReboot(virBufferPtr 
> buf,
>  }
>  
>  
> +static int
> +qemuDomainObjPrivateXMLFormatOnePR(void *payload,
> +   const void *name,
> +   void *data)
> +{
> +qemuDomainDiskPRObjectPtr prObj = payload;
> +virBufferPtr buf = data;
> +
> +virBufferAddLit(buf, "\n");
> +virBufferAdjustIndent(buf, 2);
> +virBufferAsprintf(buf, "%s\n", (const char *) name);
> +virBufferAsprintf(buf, "%s\n",
> +  
> virTristateBoolTypeToString(virTristateBoolFromBool(prObj->managed)));
> +virBufferEscapeString(buf, "%s\n", prObj->path);
> +virBufferAsprintf(buf, "%lld\n", (long long) prObj->pid);

You don't really need any of this data for the unmanaged ones and need
the PID and path for the managed manager. So this will also need some
tweaking.



signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/1] includes: function parameter names same in headers

2018-02-12 Thread Chris Venteicher
Headers use same function parameter names as definition code.
---
 include/libvirt/libvirt-domain.h| 26 +-
 include/libvirt/libvirt-event.h |  4 ++--
 include/libvirt/libvirt-host.h  |  4 ++--
 include/libvirt/libvirt-interface.h |  4 ++--
 include/libvirt/libvirt-network.h   |  6 +++---
 include/libvirt/libvirt-nwfilter.h  |  2 +-
 include/libvirt/libvirt-qemu.h  |  2 +-
 include/libvirt/libvirt-secret.h|  4 ++--
 include/libvirt/libvirt-storage.h   | 12 ++--
 include/libvirt/libvirt-stream.h| 22 +++---
 10 files changed, 43 insertions(+), 43 deletions(-)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 4048acf38..60ec35d87 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -1088,7 +1088,7 @@ int virConnectNumOfDomains  
(virConnectPtr conn);
 /*
  * Get connection from domain.
  */
-virConnectPtr   virDomainGetConnect (virDomainPtr domain);
+virConnectPtr   virDomainGetConnect (virDomainPtr dom);
 
 /*
  * Domain creation and destruction
@@ -1109,7 +1109,7 @@ virDomainPtrvirDomainLookupByID 
(virConnectPtr conn,
 virDomainPtrvirDomainLookupByUUID   (virConnectPtr conn,
  const unsigned char *uuid);
 virDomainPtrvirDomainLookupByUUIDString (virConnectPtr conn,
- const char *uuid);
+ const char *uuidstr);
 
 typedef enum {
 VIR_DOMAIN_SHUTDOWN_DEFAULT= 0,/* hypervisor choice */
@@ -1161,11 +1161,11 @@ int virDomainFree   
(virDomainPtr domain);
  */
 int virDomainSuspend(virDomainPtr domain);
 int virDomainResume (virDomainPtr domain);
-int virDomainPMSuspendForDuration (virDomainPtr domain,
+int virDomainPMSuspendForDuration (virDomainPtr dom,
unsigned int target,
unsigned long long 
duration,
unsigned int flags);
-int virDomainPMWakeup   (virDomainPtr domain,
+int virDomainPMWakeup   (virDomainPtr dom,
  unsigned int flags);
 /*
  * Domain save/restore
@@ -1626,11 +1626,11 @@ int virDomainInterfaceStats 
(virDomainPtr dom,
  */
 # define VIR_DOMAIN_BANDWIDTH_OUT_BURST "outbound.burst"
 
-int virDomainSetInterfaceParameters (virDomainPtr dom,
+int virDomainSetInterfaceParameters (virDomainPtr domain,
  const char *device,
  virTypedParameterPtr 
params,
  int nparams, unsigned 
int flags);
-int virDomainGetInterfaceParameters (virDomainPtr dom,
+int virDomainGetInterfaceParameters (virDomainPtr domain,
  const char *device,
  virTypedParameterPtr 
params,
  int *nparams, 
unsigned int flags);
@@ -1697,7 +1697,7 @@ struct _virDomainBlockInfo {
 * offset, similar to 'ls')*/
 };
 
-int virDomainGetBlockInfo(virDomainPtr dom,
+int virDomainGetBlockInfo(virDomainPtr domain,
   const char *disk,
   virDomainBlockInfoPtr info,
   unsigned int flags);
@@ -1869,7 +1869,7 @@ int virDomainPinEmulator   
(virDomainPtr domain,
 unsigned int flags);
 
 int virDomainGetEmulatorPinInfo (virDomainPtr domain,
- unsigned char *cpumaps,
+ unsigned char *cpumap,
  int maplen,
  unsigned int flags);
 
@@ -1889,7 +1889,7 @@ struct _virDomainIOThreadInfo {
 
 void virDomainIOThreadInfoFree(virDomainIOThreadInfoPtr info);
 
-int  virDomainGetIOThreadInfo(virDomainPtr domain,
+int  virDomainGetIOThreadInfo(virDomainPtr dom,
virDomainIOThreadInfoPtr **info,
unsigned int flags);
 

Re: [libvirt] [PATCH 02/14] qemuDomainDiskChangeSupported: Deny changing reservations

2018-02-12 Thread Andrea Bolognani
On Mon, 2018-02-12 at 16:40 +0100, Peter Krempa wrote:
> > @@ -6865,6 +6865,14 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr 
> > disk,
> >  CHECK_EQ(src->readonly, "readonly", true);
> >  CHECK_EQ(src->shared, "shared", true);
> >  
> > +if (!virStoragePRDefIsEqual(disk->src->pr,
> > +orig_disk->src->pr)) {
> > +virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> > +   _("cannot modify field '%s' of the disk"),
> > +   "reservations");
> 
> Is there a particular reason to format 'reservations' separately?
> 
> Translations should not translate it, but doing this is generally deemed
> translation unfriendly.

Wouldn't that actually be better for translators? Assuming there's
going to be more than a single field, you can translate the message
once and it will work for all instances.

Of course that only works if, like in this case, the variable part
itself does not need translation.

-- 
Andrea Bolognani / Red Hat / Virtualization

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


Re: [libvirt] [PATCH 12/14] qemu_process.c: Introduce qemuProcessSetupPRDaemon

2018-02-12 Thread Peter Krempa
On Thu, Jan 18, 2018 at 17:04:44 +0100, Michal Privoznik wrote:
> Again, for hotplug we need to be able to spawn just one process.
> Not all of them. Expose the static function we already have for
> that.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_process.c | 9 +
>  src/qemu/qemu_process.h | 4 
>  2 files changed, 13 insertions(+)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 02608c1f3..b4c4c64fa 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -2635,6 +2635,15 @@ qemuProcessSetupOnePRDaemon(void *payload,
>  }
>  
>  
> +int
> +qemuProcessSetupPRDaemon(virDomainObjPtr vm,
> + qemuDomainDiskPRObjectPtr prObj,
> + const char *prAlias)
> +{
> +return qemuProcessSetupOnePRDaemon(prObj, prAlias, vm);

Do we really need a wrapper that only modifies argument order?


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] Google Summer of Code 2018

2018-02-12 Thread Michal Privoznik
Dear list,

Libvirt has been accepted to this year's GSoC. Yay! For those interested
in mentoring let me know (if you haven't already). For those interested
in participating as students now it's the best time to view our ideas
page [1] and contact corresponding mentors. If you have your own idea
that's even better! Let me know and we will find you a mentor. Also, it
might be worth reading page dedicated to this year's run [2].

Michal

1: https://wiki.libvirt.org/page/Google_Summer_of_Code_Ideas
2: https://wiki.libvirt.org/page/Google_Summer_of_Code_2018

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


Re: [libvirt] [PATCH 02/11] qemu: Use switch in qemuDomainDefValidateFeatures()

2018-02-12 Thread John Ferlan


On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
> The compiler can make sure we are handling all features.
> 
> While reworking the logic, also change error messages to a more
> consistent style.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/libvirt_private.syms |  2 ++
>  src/qemu/qemu_domain.c   | 57 
> 
>  2 files changed, 45 insertions(+), 14 deletions(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 0bce0bbfb..17c3b71e0 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -345,6 +345,8 @@ virDomainDiskSetDriver;
>  virDomainDiskSetFormat;
>  virDomainDiskSetSource;
>  virDomainDiskSetType;

You'll have a merge conflict right about here...

Reviewed-by: John Ferlan 

John

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


Re: [libvirt] [PATCH 01/11] qemu: Move feature verification from PostParse() to Validate()

2018-02-12 Thread John Ferlan


On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
> We want to perform all feature verification in a single spot, but
> some of it (eg. GIC) is currently being performed at command line
> generation time, and moving it to PostParse() would cause guests
> to disappear. Moving verification to Validate() allows us to
> side-step the issue.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_domain.c | 54 
> +-
>  1 file changed, 27 insertions(+), 27 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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


[libvirt] [PATCH 0/1] Function declarations match definitions

2018-02-12 Thread Chris Venteicher
Interested in fixing mismatches between function parameter names in declarations
(header files) and definitions (.c files)?

This is a cosmetic / redability fix and does not change functionality.

This is patch 1 of larger set of 22 patches that fix parameter names across the
libvirt source code.  If there is interest I can send the entire patch set.

These are cosmetic redability fixes automatically generated by clang-tidy.

make check and make syntax-check pass successfully after the 22 patches are
applied.

This is my first submission so please point out any issues and I will be glad to
fix.

Chris Venteicher (1):
  includes: function parameter names same in headers

 include/libvirt/libvirt-domain.h| 26 +-
 include/libvirt/libvirt-event.h |  4 ++--
 include/libvirt/libvirt-host.h  |  4 ++--
 include/libvirt/libvirt-interface.h |  4 ++--
 include/libvirt/libvirt-network.h   |  6 +++---
 include/libvirt/libvirt-nwfilter.h  |  2 +-
 include/libvirt/libvirt-qemu.h  |  2 +-
 include/libvirt/libvirt-secret.h|  4 ++--
 include/libvirt/libvirt-storage.h   | 12 ++--
 include/libvirt/libvirt-stream.h| 22 +++---
 10 files changed, 43 insertions(+), 43 deletions(-)

-- 
2.14.1

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


Re: [libvirt] [PATCH 06/11] conf: Integrate all features ABI checks in the switch

2018-02-12 Thread John Ferlan


On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
> There are a few stray checks which still live outside of the
> switch in virDomainDefFeaturesCheckABIStability() for no good
> reason. Move them inside the switch, and update the error
> messages to be consistent while at it.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/conf/domain_conf.c | 105 
> -
>  1 file changed, 60 insertions(+), 45 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 9f019c906..170c56665 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -21328,7 +21328,6 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr 
> src,
>  
>  switch ((virDomainFeature) i) {
>  case VIR_DOMAIN_FEATURE_ACPI:
> -case VIR_DOMAIN_FEATURE_APIC:
>  case VIR_DOMAIN_FEATURE_PAE:
>  case VIR_DOMAIN_FEATURE_HAP:
>  case VIR_DOMAIN_FEATURE_VIRIDIAN:
> @@ -21338,10 +21337,7 @@ 
> virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
>  case VIR_DOMAIN_FEATURE_PVSPINLOCK:
>  case VIR_DOMAIN_FEATURE_PMU:
>  case VIR_DOMAIN_FEATURE_VMPORT:
> -case VIR_DOMAIN_FEATURE_GIC:
>  case VIR_DOMAIN_FEATURE_SMM:
> -case VIR_DOMAIN_FEATURE_IOAPIC:
> -case VIR_DOMAIN_FEATURE_HPT:
>  case VIR_DOMAIN_FEATURE_VMCOREINFO:
>  if (src->features[i] != dst->features[i]) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> @@ -21366,28 +21362,69 @@ 
> virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
>  }
>  break;
>  
> -case VIR_DOMAIN_FEATURE_LAST:
> +case VIR_DOMAIN_FEATURE_GIC:
> +if (src->features[i] != dst->features[i] ||
> +src->gic_version != dst->gic_version) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("State of feature '%s:%s' differs: "
> + "source: '%s:%s', destination: '%s:%s'"),
> +   featureName, "version",
> +   
> virTristateSwitchTypeToString(src->features[i]),
> +   virGICVersionTypeToString(src->gic_version),
> +   
> virTristateSwitchTypeToString(dst->features[i]),
> +   virGICVersionTypeToString(dst->gic_version));
> +return false;

Similar to previous, should these become "State of feature '%s'
attribute 'version' differs: "

likewise for other error messages Leave it up to you to decide though.

Reviewed-by: John Ferlan 

John

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


Re: [libvirt] [PATCH 08/11] qemu: Fix GIC behavior for the default case

2018-02-12 Thread John Ferlan


On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
> When no GIC version is specified, we currently default to GIC v2;
> however, that's not a great default, since guests will fail to
> start if the hardware only supports GIC v3.
> 
> Change the behavior so that a sensible default is chosen instead.
> That basically means using the same algorithm whether the user
> didn't explicitly enable the GIC feature or they explicitly
> enabled it but didn't specify any GIC version.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_domain.c | 25 
> +++---
>  .../qemuxml2argvdata/aarch64-gic-default-both.args |  2 +-
>  tests/qemuxml2argvdata/aarch64-gic-default-v3.args |  2 +-
>  .../aarch64-gic-default-both.xml   |  2 +-
>  .../qemuxml2xmloutdata/aarch64-gic-default-v3.xml  |  2 +-
>  5 files changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 98cba8789..ee720d328 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2838,13 +2838,14 @@ static void
>  qemuDomainDefEnableDefaultFeatures(virDomainDefPtr def,
> virQEMUCapsPtr qemuCaps)
>  {
> -virGICVersion version;
> -
> -/* The virt machine type always uses GIC: if the relevant element
> +/* The virt machine type always uses GIC: if the relevant information
>   * was not included in the domain XML, we need to choose a suitable
>   * GIC version ourselves */
> -if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ABSENT 
> &&
> -qemuDomainIsVirt(def)) {
> +if ((def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ABSENT 
> &&
> + qemuDomainIsVirt(def)) ||
> +(def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON &&

And patch3 (e.g. verification) takes care of us if this is "on", but
!qemuDomainIsVirt(def) ends up being true... It's a tangled web.

> + def->gic_version == VIR_GIC_VERSION_NONE)) {
> +virGICVersion version;
>  
>  VIR_DEBUG("Looking for usable GIC version in domain capabilities");
>  for (version = VIR_GIC_VERSION_LAST - 1;
> @@ -2878,17 +2879,17 @@ qemuDomainDefEnableDefaultFeatures(virDomainDefPtr 
> def,
>  }
>  }
>  

FWIW: There's a hunk above here which notes something about bz
1414081... If one goes and reads that bz, one finds it's closed notabug.

In any case, there's a lot of verbiage there.. Can any of it be shaved?

What's being done seems reasonable, but if we can take the opportunity
to revisit the comment as well - that'd be good.

For what's here as long as the above comment doesn't cause you to have
an oh sh* moment...


Reviewed-by: John Ferlan 

John

> +/* Use the default GIC version (GICv2) as a last-ditch attempt
> + * if no match could be found above */
> +if (def->gic_version == VIR_GIC_VERSION_NONE) {
> +VIR_DEBUG("Using GIC version 2 (default)");
> +def->gic_version = VIR_GIC_VERSION_2;
> +}
> +
>  /* Even if we haven't found a usable GIC version in the domain
>   * capabilities, we still want to enable this */
>  def->features[VIR_DOMAIN_FEATURE_GIC] = VIR_TRISTATE_SWITCH_ON;
>  }
> -
> -/* Use the default GIC version (GICv2) if no version was specified */
> -if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON &&
> -def->gic_version == VIR_GIC_VERSION_NONE) {
> -VIR_DEBUG("Using GIC version 2 (default)");
> -def->gic_version = VIR_GIC_VERSION_2;
> -}
>  }
>  
>  
[...]

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


[libvirt] [PATCHv2 0/1] Function declarations match definitions

2018-02-12 Thread Chris Venteicher
Interested in fixing mismatches between function parameter names in declarations
(header files) and definitions (.c files)?

This is a cosmetic / redability fix and does not change functionality.

This is patch 1 of larger set of 22 patches that fix parameter names across the
libvirt source code.  If there is interest I can send the entire patch set.

These are cosmetic redability fixes automatically generated by clang-tidy.

make check and make syntax-check pass successfully after the 22 patches are
applied.

This is my first submission so please point out any issues and I will be glad to
fix.

Chris Venteicher (1):
  include: function parameter names same in declaration

 include/libvirt/libvirt-domain.h   | 18 +++---
 include/libvirt/libvirt-event.h|  4 ++--
 include/libvirt/libvirt-host.h |  4 ++--
 include/libvirt/libvirt-network.h  |  4 ++--
 include/libvirt/libvirt-nwfilter.h |  2 +-
 include/libvirt/libvirt-qemu.h |  2 +-
 include/libvirt/libvirt-secret.h   |  4 ++--
 include/libvirt/libvirt-storage.h  | 12 +-
 include/libvirt/libvirt-stream.h   | 22 -
 src/libvirt-domain.c   | 48 +++---
 src/libvirt-network.c  | 10 
 11 files changed, 65 insertions(+), 65 deletions(-)

-- 
2.14.1

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


Re: [libvirt] [PATCH 11/11] tests: Clean up HPT tests

2018-02-12 Thread John Ferlan


On 02/06/2018 11:43 AM, Andrea Bolognani wrote:
> Give them better names and remove some redundancy.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  ...es-hpt-resizing.args => pseries-features-hpt.args} |  1 -
>  .../pseries-features-hpt.xml} |  0
>  ...chine.xml => pseries-features-invalid-machine.xml} |  2 +-
>  tests/qemuxml2argvdata/pseries-hpt-resizing.xml   | 19 
> ---
>  tests/qemuxml2argvtest.c  | 11 +++
>  tests/qemuxml2xmloutdata/pseries-features-hpt.xml |  1 +
>  tests/qemuxml2xmltest.c   |  3 +--
>  7 files changed, 6 insertions(+), 31 deletions(-)
>  rename tests/qemuxml2argvdata/{pseries-hpt-resizing.args => 
> pseries-features-hpt.args} (96%)
>  rename tests/{qemuxml2xmloutdata/pseries-hpt-resizing.xml => 
> qemuxml2argvdata/pseries-features-hpt.xml} (100%)
>  rename tests/qemuxml2argvdata/{pseries-hpt-resizing-invalid-machine.xml => 
> pseries-features-invalid-machine.xml} (86%)
>  delete mode 100644 tests/qemuxml2argvdata/pseries-hpt-resizing.xml
>  create mode 12 tests/qemuxml2xmloutdata/pseries-features-hpt.xml
> 

Reviewed-by: John Ferlan 

John

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


Re: [libvirt] [PATCH REBASE 1/2] tests: Add some tests for PCI controller options

2018-02-12 Thread John Ferlan


On 02/12/2018 04:32 AM, Andrea Bolognani wrote:
> On Sun, 2018-02-11 at 08:12 -0500, John Ferlan wrote:
>> On 02/05/2018 11:08 AM, Andrea Bolognani wrote:
>>> The input configurations set all existing options for all PCI
>>> controllers, to see what ends up showing up in the output.
>>
>> Not quite sure I understand the need. The only capability not currently
>> used in some way is QEMU_CAPS_I440FX_PCI_HOLE64_SIZE.
> 
> The idea is that existing tests only cover valid PCI controller
> options being handled correctly, not what happens when you specify
> an option which is not relevant to the PCI controller at hand.
> 

Well I understood you're going for a put the stake in the ground in
order to supply that valid list of options for the pci controllers on
the related machines.

>> I guess there's nothing wrong with adding them other than test bloat.
>> Still there's nothing to "force" someone that adds some new thing to
>> update one of the three if some new option/capability is added.
> 
> That's correct. Can you think of a way to make sure that happens?
> 

and of course that got me to wondering if it was possible in any way to
have some way that the "i440fx" xml2{xml|argv} to know that it's
"covering" all it's known and supported relative options.  Secondarily
to know when someone adds something to a machine that won't support it
before they get to the point of actually running and things falling over
dead because of the wrong configuration provided.

Nothing sprang quickly to mind to try other than adding comments which
we all know are ignored anyway ;-0...

I'm not against this patch going in, but if someone has agita over a
small about of test bloat to list everything "currently" possible for a
machine type then they should speak up!

>> Ironically adding (for example) QEMU_CAPS_Q35_PCI_HOLE64_SIZE to
>> i440fx-* doesn't cause any failure. I didn't try other ones.
> 
> Of course it wouldn't: capabilities are a property of the QEMU
> binary, and the same QEMU binary is used to run both i440fx and
> q35 guests. Even for something entirely outlandish like adding a
> q35-specific capability to a pSeries test you shouldn't see any
> failure unless you actually attempt to use the QEMU feature the
> capability is tied to.
> 
>> I'm not opposed to this being added, but do you think we should add
>> something does the opposite check?   That the wrong PCI adapter on the
>> wrong machine?  The wrong option on the wrong adapter is a question for
>> the next patch though...
> 
> We should already have at least *some* coverage for that, eg.
> pseries-serial-invalid-machine and similar. I'm certainly not
> volunteering to go over all controller and device and machine
> types and write tests for all combinations :)
> 

We have something - good...

John

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


Re: [libvirt] [PATCH 04/11] conf: Use switch in virDomainDefFeaturesCheckABIStability()

2018-02-12 Thread John Ferlan


On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
> The compiler can make sure we are handling all features.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/conf/domain_conf.c | 41 +
>  1 file changed, 33 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 05/11] conf: Validate VIR_DOMAIN_FEATURE_CAPABILITIES properly

2018-02-12 Thread John Ferlan


On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
> Unlike most other features, VIR_DOMAIN_FEATURE_CAPABILITIES is
> of type virDomainCapabilitiesPolicy instead of virTristateSwitch,
> so we need to handle it separately for the error message to make
> sense.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/conf/domain_conf.c | 13 -
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index e4d01b869..9f019c906 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -21336,7 +21336,6 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr 
> src,
>  case VIR_DOMAIN_FEATURE_HYPERV:
>  case VIR_DOMAIN_FEATURE_KVM:
>  case VIR_DOMAIN_FEATURE_PVSPINLOCK:
> -case VIR_DOMAIN_FEATURE_CAPABILITIES:
>  case VIR_DOMAIN_FEATURE_PMU:
>  case VIR_DOMAIN_FEATURE_VMPORT:
>  case VIR_DOMAIN_FEATURE_GIC:
> @@ -21355,6 +21354,18 @@ 
> virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
>  }
>  break;
>  
> +case VIR_DOMAIN_FEATURE_CAPABILITIES:
> +if (src->features[i] != dst->features[i]) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("State of feature '%s:%s' differs: "
> + "source: '%s', destination: '%s'"),
> +   featureName, "policy",

OK - based on what I saw Peter ACK for Michal's patches related to his
similar usage, fine. Still looks strange especially since we're talking
about a named XML attribute which we document as "policy". I just hope
there isn't some language variant that alters the meaning.  It does look
strange to me "State of feature 'capabilities:policy' differs: "... I
almost wonder if "State of feature 'capabilities' attribute 'policy'
differs: " would help (or really matter).

Also, based on the tests/domainschemadata/domain-caps-features.xml:





/me wonders how many more yaks might get shaved if we needed to check
differences w/r/t the caps_features array?

IOW: If src->features[i] != VIR_DOMAIN_CAPABILITIES_POLICY_DEFAULT, then
should we be checking each of the VIR_DOMAIN_CAPS_FEATURE_LAST to ensure
that they're not different too.

Not something required for this patch, but perhaps a follow-up...

Reviewed-by: John Ferlan 


> +   
> virDomainCapabilitiesPolicyTypeToString(src->features[i]),
> +   
> virDomainCapabilitiesPolicyTypeToString(dst->features[i]));
> +return false;
> +}
> +break;
> +
>  case VIR_DOMAIN_FEATURE_LAST:
>  break;
>  }
> 

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


Re: [libvirt] [PATCH 07/11] tests: Improve GIC tests

2018-02-12 Thread John Ferlan


On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
> Account for the fact that the default might change based on what
> GIC versions are supported by QEMU. That's not the case at the
> moment, but it will be soon.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  tests/qemuxml2argvdata/aarch64-gic-default-both.args  | 1 +
>  tests/qemuxml2argvdata/aarch64-gic-default-both.xml   | 1 +
>  tests/qemuxml2argvdata/aarch64-gic-default-v2.args| 1 +
>  tests/qemuxml2argvdata/aarch64-gic-default-v2.xml | 1 +
>  tests/qemuxml2argvdata/aarch64-gic-default-v3.args| 1 +
>  tests/qemuxml2argvdata/aarch64-gic-default-v3.xml | 1 +
>  tests/qemuxml2argvtest.c  | 6 +++---
>  tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml | 1 +
>  tests/qemuxml2xmloutdata/aarch64-gic-default-v2.xml   | 1 +
>  tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml   | 1 +
>  tests/qemuxml2xmltest.c   | 6 +++---
>  11 files changed, 15 insertions(+), 6 deletions(-)
>  create mode 12 tests/qemuxml2argvdata/aarch64-gic-default-both.args
>  create mode 12 tests/qemuxml2argvdata/aarch64-gic-default-both.xml
>  create mode 12 tests/qemuxml2argvdata/aarch64-gic-default-v2.args
>  create mode 12 tests/qemuxml2argvdata/aarch64-gic-default-v2.xml
>  create mode 12 tests/qemuxml2argvdata/aarch64-gic-default-v3.args
>  create mode 12 tests/qemuxml2argvdata/aarch64-gic-default-v3.xml
>  create mode 12 tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml
>  create mode 12 tests/qemuxml2xmloutdata/aarch64-gic-default-v2.xml
>  create mode 12 tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml
> 

Reviewed-by: John Ferlan 

John

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


[libvirt] [PATCHv2 1/1] include: function parameter names same in declaration

2018-02-12 Thread Chris Venteicher
Headers use same function parameter names as definition code.

In some cases in libvirt-domain and libvirt-network an established
naming pattern in the header files was more consistent and informative
in which case the implementation was modified in the c file.
---
 include/libvirt/libvirt-domain.h   | 18 +++---
 include/libvirt/libvirt-event.h|  4 ++--
 include/libvirt/libvirt-host.h |  4 ++--
 include/libvirt/libvirt-network.h  |  4 ++--
 include/libvirt/libvirt-nwfilter.h |  2 +-
 include/libvirt/libvirt-qemu.h |  2 +-
 include/libvirt/libvirt-secret.h   |  4 ++--
 include/libvirt/libvirt-storage.h  | 12 +-
 include/libvirt/libvirt-stream.h   | 22 -
 src/libvirt-domain.c   | 48 +++---
 src/libvirt-network.c  | 10 
 11 files changed, 65 insertions(+), 65 deletions(-)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 4048acf38..0f905bc24 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -1109,7 +1109,7 @@ virDomainPtrvirDomainLookupByID 
(virConnectPtr conn,
 virDomainPtrvirDomainLookupByUUID   (virConnectPtr conn,
  const unsigned char *uuid);
 virDomainPtrvirDomainLookupByUUIDString (virConnectPtr conn,
- const char *uuid);
+ const char *uuidstr);
 
 typedef enum {
 VIR_DOMAIN_SHUTDOWN_DEFAULT= 0,/* hypervisor choice */
@@ -1626,11 +1626,11 @@ int virDomainInterfaceStats 
(virDomainPtr dom,
  */
 # define VIR_DOMAIN_BANDWIDTH_OUT_BURST "outbound.burst"
 
-int virDomainSetInterfaceParameters (virDomainPtr dom,
+int virDomainSetInterfaceParameters (virDomainPtr domain,
  const char *device,
  virTypedParameterPtr 
params,
  int nparams, unsigned 
int flags);
-int virDomainGetInterfaceParameters (virDomainPtr dom,
+int virDomainGetInterfaceParameters (virDomainPtr domain,
  const char *device,
  virTypedParameterPtr 
params,
  int *nparams, 
unsigned int flags);
@@ -1697,7 +1697,7 @@ struct _virDomainBlockInfo {
 * offset, similar to 'ls')*/
 };
 
-int virDomainGetBlockInfo(virDomainPtr dom,
+int virDomainGetBlockInfo(virDomainPtr domain,
   const char *disk,
   virDomainBlockInfoPtr info,
   unsigned int flags);
@@ -1869,7 +1869,7 @@ int virDomainPinEmulator   
(virDomainPtr domain,
 unsigned int flags);
 
 int virDomainGetEmulatorPinInfo (virDomainPtr domain,
- unsigned char *cpumaps,
+ unsigned char *cpumap,
  int maplen,
  unsigned int flags);
 
@@ -2298,11 +2298,11 @@ void 
virDomainStatsRecordListFree(virDomainStatsRecordPtr *stats);
  */
 # define VIR_PERF_PARAM_EMULATION_FAULTS  "emulation_faults"
 
-int virDomainGetPerfEvents(virDomainPtr dom,
+int virDomainGetPerfEvents(virDomainPtr domain,
virTypedParameterPtr *params,
int *nparams,
unsigned int flags);
-int virDomainSetPerfEvents(virDomainPtr dom,
+int virDomainSetPerfEvents(virDomainPtr domain,
virTypedParameterPtr params,
int nparams,
unsigned int flags);
@@ -3130,14 +3130,14 @@ typedef enum {
   * completed job */
 } virDomainGetJobStatsFlags;
 
-int virDomainGetJobInfo(virDomainPtr dom,
+int virDomainGetJobInfo(virDomainPtr domain,
 virDomainJobInfoPtr info);
 int virDomainGetJobStats(virDomainPtr domain,
  int *type,
  virTypedParameterPtr *params,
  int *nparams,
  unsigned int flags);
-int virDomainAbortJob(virDomainPtr dom);
+int virDomainAbortJob(virDomainPtr domain);
 
 typedef enum {
 VIR_DOMAIN_JOB_OPERATION_UNKNOWN = 0,
diff --git a/include/libvirt/libvirt-event.h 

Re: [libvirt] [PATCH 09/11] conf: Improve IOAPIC feature handling

2018-02-12 Thread John Ferlan


On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
> Instead of storing separately whether the feature is enabled
> or not and what driver should be used, store both of them in
> a single place.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/conf/domain_conf.c  | 29 +
>  src/conf/domain_conf.h  | 11 ++-
>  src/qemu/qemu_command.c |  5 +++--
>  src/qemu/qemu_domain.c  |  2 +-
>  4 files changed, 23 insertions(+), 24 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 170c56665..19884ec13 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -900,6 +900,7 @@ VIR_ENUM_IMPL(virDomainLoader,
>  
>  VIR_ENUM_IMPL(virDomainIOAPIC,
>VIR_DOMAIN_IOAPIC_LAST,
> +  "none",
>"qemu",
>"kvm")
>  
> @@ -5913,8 +5914,7 @@ virDomainDefValidateInternal(const virDomainDef *def)
>  
>  if (def->iommu &&
>  def->iommu->intremap == VIR_TRISTATE_SWITCH_ON &&
> -(def->features[VIR_DOMAIN_FEATURE_IOAPIC] != VIR_TRISTATE_SWITCH_ON 
> ||
> - def->ioapic != VIR_DOMAIN_IOAPIC_QEMU)) {
> +def->features[VIR_DOMAIN_FEATURE_IOAPIC] != VIR_DOMAIN_IOAPIC_QEMU) {
>  virReportError(VIR_ERR_XML_ERROR, "%s",
> _("IOMMU interrupt remapping requires split I/O APIC "
>   "(ioapic driver='qemu')"));
> @@ -19224,14 +19224,13 @@ virDomainDefParseXML(xmlDocPtr xml,
>  tmp = virXMLPropString(nodes[i], "driver");
>  if (tmp) {
>  int value = virDomainIOAPICTypeFromString(tmp);
> -if (value < 0) {
> +if (value < 0 || value == VIR_DOMAIN_IOAPIC_NONE) {

"none" wouldn't be legal XML according to "iopic" in domaincommon.rng. I
think this is where things get tricky - I'm fine with the changes as is,
but that interaction between domain_conf xml parsing and the rng schema
gets me wondering about whether that "none" value should be in the rng.
Perhaps there's another opinion on this that may want to pipe in now...

>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("Unknown driver mode: %s"),
> tmp);
>  goto error;
>  }
> -def->ioapic = value;
> -def->features[val] = VIR_TRISTATE_SWITCH_ON;
> +def->features[val] = value;
>  VIR_FREE(tmp);
>  }
>  break;
> @@ -21408,16 +21407,13 @@ 
> virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
>  break;
>  
>  case VIR_DOMAIN_FEATURE_IOAPIC:
> -if (src->features[i] != dst->features[i] ||
> -src->ioapic != dst->ioapic) {
> +if (src->features[i] != dst->features[i]) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("State of feature '%s:%s' differs: "
> - "source: '%s:%s', destination: '%s:%s'"),
> + "source: '%s', destination: '%s'"),
> featureName, "driver",
> -   
> virTristateSwitchTypeToString(src->features[i]),
> -   virDomainIOAPICTypeToString(src->ioapic),
> -   
> virTristateSwitchTypeToString(dst->features[i]),
> -   virDomainIOAPICTypeToString(dst->ioapic));
> +   virDomainIOAPICTypeToString(src->features[i]),
> +   
> virDomainIOAPICTypeToString(dst->features[i]));
>  return false;
>  }
>  break;
> @@ -26943,10 +26939,11 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>  break;
>  
>  case VIR_DOMAIN_FEATURE_IOAPIC:
> -if (def->features[i] == VIR_TRISTATE_SWITCH_ON) {
> -virBufferAsprintf(buf, "\n",
> -  
> virDomainIOAPICTypeToString(def->ioapic));
> -}
> +if (def->features[i] == VIR_DOMAIN_IOAPIC_NONE)
> +break;
> +
> +virBufferAsprintf(buf, "\n",
> +  
> virDomainIOAPICTypeToString(def->features[i]));
>  break;
>  
>  case VIR_DOMAIN_FEATURE_HPT:
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 21e004515..20f0efc36 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1861,7 +1861,8 @@ struct _virDomainLoaderDef {
>  void virDomainLoaderDefFree(virDomainLoaderDefPtr loader);
>  
>  typedef enum {
> -VIR_DOMAIN_IOAPIC_QEMU = 0,
> +VIR_DOMAIN_IOAPIC_NONE = 0,
> +VIR_DOMAIN_IOAPIC_QEMU,
>  VIR_DOMAIN_IOAPIC_KVM,
>  
>  VIR_DOMAIN_IOAPIC_LAST
> @@ -2352,9 +2353,10 @@ 

Re: [libvirt] [PATCH 10/11] conf: Improve HPT feature handling

2018-02-12 Thread John Ferlan


On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
> Instead of storing separately whether the feature is enabled
> or not and what resizing policy should be used, store both of
> them in a single place.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/conf/domain_conf.c  | 26 --
>  src/conf/domain_conf.h  |  4 ++--
>  src/qemu/qemu_command.c |  4 ++--
>  src/qemu/qemu_domain.c  |  2 +-
>  4 files changed, 17 insertions(+), 19 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 19884ec13..c1d549594 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -906,6 +906,7 @@ VIR_ENUM_IMPL(virDomainIOAPIC,
>  
>  VIR_ENUM_IMPL(virDomainHPTResizing,
>VIR_DOMAIN_HPT_RESIZING_LAST,
> +  "none",
>"enabled",
>"disabled",
>"required",
> @@ -19239,14 +19240,13 @@ virDomainDefParseXML(xmlDocPtr xml,
>  tmp = virXMLPropString(nodes[i], "resizing");
>  if (tmp) {
>  int value = virDomainHPTResizingTypeFromString(tmp);
> -if (value < 0) {
> +if (value < 0 || value == VIR_DOMAIN_HPT_RESIZING_NONE) {

Similar note as w/ previous and rng schema. Here too I'm fine with this
way, but if someone else isn't hopefully they speak up.

>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("Unknown HPT resizing setting: %s"),
> tmp);
>  goto error;
>  }
> -def->hpt_resizing = value;
> -def->features[val] = VIR_TRISTATE_SWITCH_ON;
> +def->features[val] = value;
>  VIR_FREE(tmp);
>  }
>  break;
> @@ -21377,16 +21377,13 @@ 
> virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
>  break;
>  
>  case VIR_DOMAIN_FEATURE_HPT:
> -if (src->features[i] != dst->features[i] ||
> -src->hpt_resizing != dst->hpt_resizing) {
> +if (src->features[i] != dst->features[i]) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("State of feature '%s:%s' differs: "
> - "source: '%s:%s', destination: '%s:%s'"),
> + "source: '%s', destination: '%s'"),
> featureName, "resizing",
> -   
> virTristateSwitchTypeToString(src->features[i]),
> -   
> virDomainHPTResizingTypeToString(src->hpt_resizing),
> -   
> virTristateSwitchTypeToString(dst->features[i]),
> -   
> virDomainHPTResizingTypeToString(dst->hpt_resizing));
> +   
> virDomainHPTResizingTypeToString(src->features[i]),
> +   
> virDomainHPTResizingTypeToString(dst->features[i]));
>  return false;
>  }
>  break;
> @@ -26947,10 +26944,11 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>  break;
>  
>  case VIR_DOMAIN_FEATURE_HPT:
> -if (def->features[i] == VIR_TRISTATE_SWITCH_ON) {
> -virBufferAsprintf(buf, "\n",
> -  
> virDomainHPTResizingTypeToString(def->hpt_resizing));
> -}
> +if (def->features[i] == VIR_DOMAIN_HPT_RESIZING_NONE)
> +break;
> +
> +virBufferAsprintf(buf, "\n",
> +  
> virDomainHPTResizingTypeToString(def->features[i]));
>  break;
>  
>  /* coverity[dead_error_begin] */
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 20f0efc36..4e9044ae6 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1871,7 +1871,8 @@ typedef enum {
>  VIR_ENUM_DECL(virDomainIOAPIC);
>  
>  typedef enum {
> -VIR_DOMAIN_HPT_RESIZING_ENABLED = 0,
> +VIR_DOMAIN_HPT_RESIZING_NONE = 0,
> +VIR_DOMAIN_HPT_RESIZING_ENABLED,
>  VIR_DOMAIN_HPT_RESIZING_DISABLED,
>  VIR_DOMAIN_HPT_RESIZING_REQUIRED,
>  
> @@ -2364,7 +2365,6 @@ struct _virDomainDef {

The comment a few lines above here should be updated to include:

 * VIR_DOMAIN_FEATURE_HPT is of type virDomainHPTResizing

Reviewed-by: John Ferlan 

John

>  unsigned int hyperv_spinlocks;
>  virGICVersion gic_version;
>  char *hyperv_vendor_id;
> -virDomainHPTResizing hpt_resizing;
>  
>  /* These options are of type virTristateSwitch: ON = keep, OFF = drop */
>  int caps_features[VIR_DOMAIN_CAPS_FEATURE_LAST];

[...]

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


[libvirt] [PATCH v2] vcpupin: add clear feature

2018-02-12 Thread Yi Wang
We can't clear vcpupin settings of XML once we did vcpupin
command, this is not convenient under some condition such
as migration to a host with less CPUs.

This patch introduces clear feature, which can clear vcpuin
setting of XML using a 'c' option.

Signed-off-by: Yi Wang 
Signed-off-by: Xi Xu 
---
 include/libvirt/libvirt-domain.h | 11 +++
 src/qemu/qemu_driver.c   | 32 
 tools/virsh-domain.c |  5 -
 tools/virsh.pod  |  1 +
 4 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 4048acf..46f4e77 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -1847,6 +1847,17 @@ int virDomainSetVcpusFlags  
(virDomainPtr domain,
 int virDomainGetVcpusFlags  (virDomainPtr domain,
  unsigned int flags);
 
+/* Flags for controlling virtual CPU pinning.  */
+typedef enum {
+/* See virDomainModificationImpact for these flags.  */
+VIR_DOMAIN_VCPU_PIN_CURRENT = VIR_DOMAIN_AFFECT_CURRENT,
+VIR_DOMAIN_VCPU_PIN_LIVE= VIR_DOMAIN_AFFECT_LIVE,
+VIR_DOMAIN_VCPU_PIN_CONFIG  = VIR_DOMAIN_AFFECT_CONFIG,
+
+/* Additionally, these flags may be bitwise-OR'd in.  */
+VIR_DOMAIN_VCPU_PIN_CLEAR = (1 << 2), /* Clear vcpus pin info */
+} virDomainVcpuPinFlags;
+
 int virDomainPinVcpu(virDomainPtr domain,
  unsigned int vcpu,
  unsigned char *cpumap,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index bbce5bd..fe1f62f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5056,7 +5056,8 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm,
   int vcpu,
   virQEMUDriverPtr driver,
   virQEMUDriverConfigPtr cfg,
-  virBitmapPtr cpumap)
+  virBitmapPtr cpumap,
+  bool clear)
 {
 virBitmapPtr tmpmap = NULL;
 virDomainVcpuDefPtr vcpuinfo;
@@ -5069,6 +5070,7 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm,
 int eventNparams = 0;
 int eventMaxparams = 0;
 int ret = -1;
+virBitmapPtr targetMap = NULL;
 
 if (!qemuDomainHasVcpuPids(vm)) {
 virReportError(VIR_ERR_OPERATION_INVALID,
@@ -5083,10 +5085,15 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm,
 goto cleanup;
 }
 
-if (!(tmpmap = virBitmapNewCopy(cpumap)))
-goto cleanup;
+if (clear) {
+targetMap = virHostCPUGetOnlineBitmap();
+} else {
+targetMap = cpumap;
+if (!(tmpmap = virBitmapNewCopy(cpumap)))
+goto cleanup;
+}
 
-if (!(str = virBitmapFormat(cpumap)))
+if (!(str = virBitmapFormat(targetMap)))
 goto cleanup;
 
 if (vcpuinfo->online) {
@@ -5095,11 +5102,11 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm,
 if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_VCPU, vcpu,
false, _vcpu) < 0)
 goto cleanup;
-if (qemuSetupCgroupCpusetCpus(cgroup_vcpu, cpumap) < 0)
+if (qemuSetupCgroupCpusetCpus(cgroup_vcpu, targetMap) < 0)
 goto cleanup;
 }
 
-if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, vcpu), cpumap) < 0)
+if (virProcessSetAffinity(qemuDomainGetVcpuPid(vm, vcpu), targetMap) < 
0)
 goto cleanup;
 }
 
@@ -5128,6 +5135,8 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm,
 virCgroupFree(_vcpu);
 VIR_FREE(str);
 qemuDomainEventQueue(driver, event);
+if (clear)
+virBitmapFree(targetMap);
 return ret;
 }
 
@@ -5148,9 +5157,11 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
 virBitmapPtr pcpumap = NULL;
 virDomainVcpuDefPtr vcpuinfo = NULL;
 virQEMUDriverConfigPtr cfg = NULL;
+bool clear = false;
 
 virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
-  VIR_DOMAIN_AFFECT_CONFIG, -1);
+  VIR_DOMAIN_AFFECT_CONFIG |
+  VIR_DOMAIN_VCPU_PIN_CLEAR, -1);
 
 cfg = virQEMUDriverGetConfig(driver);
 
@@ -5166,6 +5177,8 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
 if (virDomainObjGetDefs(vm, flags, , ) < 0)
 goto endjob;
 
+clear = !!(flags & VIR_DOMAIN_VCPU_PIN_CLEAR);
+
 if ((def && def->virtType == VIR_DOMAIN_VIRT_QEMU) ||
 (persistentDef && persistentDef->virtType == VIR_DOMAIN_VIRT_QEMU)) {
 virReportError(VIR_ERR_OPERATION_FAILED, "%s",
@@ -5191,7 +5204,7 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
 }
 
 if (def &&
-qemuDomainPinVcpuLive(vm, def, vcpu, driver, cfg, pcpumap) < 0)
+qemuDomainPinVcpuLive(vm, def, vcpu, driver, cfg, pcpumap, clear) < 0)
 goto endjob;
 
 if (persistentDef) {
@@ 

Re: [libvirt] [PATCH v4 2/8] libxl: pass driver config to libxlMakeDomBuildInfo

2018-02-12 Thread Jim Fehlig

On 02/08/2018 03:58 PM, Marek Marczykowski-Górecki wrote:

Preparation for global nestedhvm configuration - libxlMakeDomBuildInfo
needs access to libxlDriverConfig.
No functional change.

Adjusting tests require slightly more mockup functions, because of
libxlDriverConfigNew() call.

---
Changes since v3:
  - new patch, preparation
---
  src/libxl/libxl_conf.c |  8 +---
  src/libxl/libxl_conf.h |  2 +-
  src/libxl/libxl_domain.c   |  2 +-
  tests/libxlxml2domconfigtest.c | 20 +---
  tests/virmocklibxl.c   | 25 +
  5 files changed, 45 insertions(+), 12 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 2d2a707..8cced29 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -271,10 +271,11 @@ libxlMakeChrdevStr(virDomainChrDefPtr def, char **buf)
  
  static int

  libxlMakeDomBuildInfo(virDomainDefPtr def,
-  libxl_ctx *ctx,
+  libxlDriverConfigPtr cfg,
virCapsPtr caps,
libxl_domain_config *d_config)
  {
+libxl_ctx *ctx = cfg->ctx;
  libxl_domain_build_info *b_info = _config->b_info;
  int hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM;
  size_t i;
@@ -2288,16 +2289,17 @@ int
  libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
 virDomainDefPtr def,
 const char *channelDir LIBXL_ATTR_UNUSED,
-   libxl_ctx *ctx,
+   libxlDriverConfigPtr cfg,


I can't recall if the only reason we were avoiding passing a 
libxlDriverConfigPtr is the extra mocking. If so, that's not reason enough to 
avoid it.



 virCapsPtr caps,
 libxl_domain_config *d_config)
  {
+libxl_ctx *ctx = cfg->ctx;
  libxl_domain_config_init(d_config);
  
  if (libxlMakeDomCreateInfo(ctx, def, _config->c_info) < 0)

  return -1;
  
-if (libxlMakeDomBuildInfo(def, ctx, caps, d_config) < 0)

+if (libxlMakeDomBuildInfo(def, cfg, caps, d_config) < 0)
  return -1;
  
  #ifdef LIBXL_HAVE_VNUMA

diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
index 264df11..8eefe06 100644
--- a/src/libxl/libxl_conf.h
+++ b/src/libxl/libxl_conf.h
@@ -216,7 +216,7 @@ int
  libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
 virDomainDefPtr def,
 const char *channelDir LIBXL_ATTR_UNUSED,
-   libxl_ctx *ctx,
+   libxlDriverConfigPtr cfg,
 virCapsPtr caps,
 libxl_domain_config *d_config);
  
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c

index 395c8a9..0a60444 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -1253,7 +1253,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver,
  goto cleanup_dom;
  
  if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def,

-   cfg->channelDir, cfg->ctx, cfg->caps, _config) 
< 0)
+   cfg->channelDir, cfg, cfg->caps, _config) < 0)


If we are going to pass the entire libxlDriverConfigPtr to 
libxlBuildDomainConfig(), the channelDir and caps parameters can be dropped.


Regards,
Jim


  goto cleanup_dom;
  
  if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, _config) < 0)

diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c
index bd4c3af..0105550 100644
--- a/tests/libxlxml2domconfigtest.c
+++ b/tests/libxlxml2domconfigtest.c
@@ -56,8 +56,8 @@ testCompareXMLToDomConfig(const char *xmlfile,
  int ret = -1;
  libxl_domain_config actualconfig;
  libxl_domain_config expectconfig;
+libxlDriverConfigPtr cfg;
  xentoollog_logger *log = NULL;
-libxl_ctx *ctx = NULL;
  virPortAllocatorPtr gports = NULL;
  virDomainXMLOptionPtr xmlopt = NULL;
  virDomainDefPtr vmdef = NULL;
@@ -68,10 +68,16 @@ testCompareXMLToDomConfig(const char *xmlfile,
  libxl_domain_config_init();
  libxl_domain_config_init();
  
+if (!(cfg = libxlDriverConfigNew()))

+goto cleanup;
+
  if (!(log = (xentoollog_logger *)xtl_createlogger_stdiostream(stderr, 
XTL_DEBUG, 0)))
  goto cleanup;
  
-if (libxl_ctx_alloc(, LIBXL_VERSION, 0, log) < 0)

+/* replace logger with stderr one */
+libxl_ctx_free(cfg->ctx);
+
+if (libxl_ctx_alloc(>ctx, LIBXL_VERSION, 0, log) < 0)
  goto cleanup;
  
  if (!(gports = virPortAllocatorNew("vnc", 5900, 6000,

@@ -85,22 +91,22 @@ testCompareXMLToDomConfig(const char *xmlfile,
  NULL, VIR_DOMAIN_XML_INACTIVE)))
  goto cleanup;
  
-if (libxlBuildDomainConfig(gports, vmdef, NULL, ctx, caps, ) < 0)

+if (libxlBuildDomainConfig(gports, vmdef, NULL, cfg, caps, ) 
< 0)
  goto cleanup;
  
-if (!(actualjson = 

Re: [libvirt] [PATCH v4 1/8] libxl: fix libxlDriverConfigDispose for partially constructed object

2018-02-12 Thread Jim Fehlig

On 02/08/2018 03:58 PM, Marek Marczykowski-Górecki wrote:

libxlDriverConfigNew() use libxlDriverConfigDispose() for cleanup in
case of errors. Do not call libxlLoggerFree() on not allocated logger
(NULL).

---
Changes since v3:
  - new patch, mostly unrelated, but found while adjusting tests


Trivial. I'd push it, but you need to add a S-O-B to patches now. See commit 
d64d5ccb06.


Reviewed-by: Jim Fehlig 

Regards,
Jim


---
  src/libxl/libxl_conf.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 970cff2..2d2a707 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -80,7 +80,8 @@ libxlDriverConfigDispose(void *obj)
  
  virObjectUnref(cfg->caps);

  libxl_ctx_free(cfg->ctx);
-libxlLoggerFree(cfg->logger);
+if (cfg->logger)
+libxlLoggerFree(cfg->logger);
  
  VIR_FREE(cfg->configDir);

  VIR_FREE(cfg->autostartDir);



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

[libvirt] [PATCH v2 0/2] Support network stats for hostdev(SR-IOV) in Switchdev mode

2018-02-12 Thread Jai Singh Rana
With availability of switchdev model in linux, it is possible to capture
stats for hostdev SR-IOV VFs using its VF representor interface name on
host for nics supporting switchdev model.

These stats are supported by adding helper APIs for getting VF
Representor name based on BDF info in 'hostdev' and querying required
net sysfs entries on host. These helper APIs are then used in
qemu_driver to get the hostdev interface stats for pci SR-IOV device.

[1] https://www.kernel.org/doc/Documentation/networking/switchdev.txt

Jai Singh Rana (2):
  util: Add helper APIs to get/verify VF Representor name
  qemu: conf: Network stats support for hostdev VF Representor

 po/POTFILES.in  |   1 +
 src/Makefile.am |   1 +
 src/conf/domain_conf.c  |   7 ++
 src/libvirt_private.syms|   5 +
 src/qemu/qemu_driver.c  |  34 ++-
 src/util/virhostdev.c   |  11 +++
 src/util/virhostdev.h   |   6 ++
 src/util/virnetdevhostdev.c | 224 
 src/util/virnetdevhostdev.h |  33 +++
 9 files changed, 318 insertions(+), 4 deletions(-)
 create mode 100644 src/util/virnetdevhostdev.c
 create mode 100644 src/util/virnetdevhostdev.h

-- 
2.13.6

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


[libvirt] [PATCH v2 1/2] util: Add helper APIs to get/verify VF Representor name

2018-02-12 Thread Jai Singh Rana
Switchdev VF Representor interface name on host is derived based on BDF
of pci SR-IOV device in 'hostdev' and querying required net sysfs
entries on host.
---
v2 includes commented code cleanup in virnetdevhostdev.c

 po/POTFILES.in  |   1 +
 src/Makefile.am |   1 +
 src/libvirt_private.syms|   5 +
 src/util/virhostdev.c   |  11 +++
 src/util/virhostdev.h   |   6 ++
 src/util/virnetdevhostdev.c | 224 
 src/util/virnetdevhostdev.h |  33 +++
 7 files changed, 281 insertions(+)
 create mode 100644 src/util/virnetdevhostdev.c
 create mode 100644 src/util/virnetdevhostdev.h

diff --git a/po/POTFILES.in b/po/POTFILES.in
index 285955469..73ce73397 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -237,6 +237,7 @@ src/util/virnetdevmacvlan.c
 src/util/virnetdevmidonet.c
 src/util/virnetdevopenvswitch.c
 src/util/virnetdevtap.c
+src/util/virnetdevhostdev.c
 src/util/virnetdevveth.c
 src/util/virnetdevvportprofile.c
 src/util/virnetlink.c
diff --git a/src/Makefile.am b/src/Makefile.am
index db68e01db..0f3c3f1bc 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -148,6 +148,7 @@ UTIL_SOURCES = \
util/virnetdev.h util/virnetdev.c \
util/virnetdevbandwidth.h util/virnetdevbandwidth.c \
util/virnetdevbridge.h util/virnetdevbridge.c \
+   util/virnetdevhostdev.h util/virnetdevhostdev.c \
util/virnetdevip.h util/virnetdevip.c \
util/virnetdevmacvlan.c util/virnetdevmacvlan.h \
util/virnetdevmidonet.h util/virnetdevmidonet.c \
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 3b14d7d15..d9bc8ad72 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2288,6 +2288,11 @@ virNetDevBridgeSetSTPDelay;
 virNetDevBridgeSetVlanFiltering;
 
 
+# util/virnetdevhostdev.h
+virNetdevHostdevCheckVFRepIFName;
+virNetdevHostdevGetVFRepIFName;
+
+
 # util/virnetdevip.h
 virNetDevIPAddrAdd;
 virNetDevIPAddrDel;
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index a12224c58..b6d824d07 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -351,6 +351,17 @@ virHostdevNetDevice(virDomainHostdevDefPtr hostdev,
 }
 
 
+/* Non static wrapper for virHostdevNetDevice to use outside virhostdev */
+int
+virHostdevNetDeviceWrapper(virDomainHostdevDefPtr hostdev,
+int pfNetDevIdx,
+char **linkdev,
+int *vf)
+{
+return virHostdevNetDevice(hostdev, pfNetDevIdx, linkdev, vf);
+}
+
+
 static bool
 virHostdevIsPCINetDevice(virDomainHostdevDefPtr hostdev)
 {
diff --git a/src/util/virhostdev.h b/src/util/virhostdev.h
index 54e1c66be..7dc860a85 100644
--- a/src/util/virhostdev.h
+++ b/src/util/virhostdev.h
@@ -202,5 +202,11 @@ int virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr 
mgr,
 int virHostdevPCINodeDeviceReset(virHostdevManagerPtr mgr,
  virPCIDevicePtr pci)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+int
+virHostdevNetDeviceWrapper(virDomainHostdevDefPtr hostdev,
+int pfNetDevIdx,
+char **linkdev,
+int *vf)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4);
 
 #endif /* __VIR_HOSTDEV_H__ */
diff --git a/src/util/virnetdevhostdev.c b/src/util/virnetdevhostdev.c
new file mode 100644
index 0..243c78a97
--- /dev/null
+++ b/src/util/virnetdevhostdev.c
@@ -0,0 +1,224 @@
+/*
+ * virnetdevhostdev.c: utilities to get/verify Switchdev VF Representor
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ */
+
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#include "virhostdev.h"
+#include "virnetdevhostdev.h"
+#include "viralloc.h"
+#include "virstring.h"
+#include "virfile.h"
+#include "virerror.h"
+#include "virlog.h"
+
+#define VIR_FROM_THIS VIR_FROM_NONE
+
+VIR_LOG_INIT("util.netdevhostdev");
+
+#ifndef IFNAMSIZ
+#define IFNAMSIZ 16
+#endif
+
+#define IFSWITCHIDSIZ 20
+
+#ifndef SYSFS_NET_DIR
+#define SYSFS_NET_DIR "/sys/class/net/"
+#endif
+
+
+/**
+ * virNetdevHostdevNetSysfsPath
+ *
+ * @pf_name: netdev name of the physical function (PF)
+ * @vf: virtual function (VF) number for the device of interest
+ * @vf_representor: name of the VF representor interface
+ *
+ * Finds the VF representor name 

[libvirt] [PATCH v2 2/2] qemu: conf: Network stats support for hostdev VF Representor

2018-02-12 Thread Jai Singh Rana
In case of , return stats if its a Switchdev
VF Representor interface of pci SR-IOV device.
---
v2 fixes bracket spacing in domain_conf.c

 src/conf/domain_conf.c |  7 +++
 src/qemu/qemu_driver.c | 34 ++
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index fb732a0c2..b553c5a2f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -58,6 +58,7 @@
 #include "virnetdev.h"
 #include "virnetdevmacvlan.h"
 #include "virhostdev.h"
+#include "virnetdevhostdev.h"
 #include "virmdev.h"
 
 #define VIR_FROM_THIS VIR_FROM_DOMAIN
@@ -28112,6 +28113,12 @@ virDomainNetFind(virDomainDefPtr def, const char 
*device)
 return net;
 }
 
+/* Give a try to hostdev */
+for (i = 0; i < def->nnets; i++) {
+if (!virNetdevHostdevCheckVFRepIFName(def->hostdevs[i], device))
+return def->nets[i];
+}
+
 virReportError(VIR_ERR_INVALID_ARG,
_("'%s' is not a known interface"), device);
 return NULL;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index bbce5bd81..24484ab92 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -67,6 +67,7 @@
 #include "virhostcpu.h"
 #include "virhostmem.h"
 #include "virnetdevtap.h"
+#include "virnetdevhostdev.h"
 #include "virnetdevopenvswitch.h"
 #include "capabilities.h"
 #include "viralloc.h"
@@ -11153,6 +11154,11 @@ qemuDomainInterfaceStats(virDomainPtr dom,
 if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_VHOSTUSER) {
 if (virNetDevOpenvswitchInterfaceStats(net->ifname, stats) < 0)
 goto cleanup;
+} else if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
+if (virNetdevHostdevVFRepInterfaceStats(device, stats,
+!virDomainNetTypeSharesHostView
+(net)) < 0)
+goto cleanup;
 } else {
 if (virNetDevTapInterfaceStats(net->ifname, stats,
!virDomainNetTypeSharesHostView(net)) < 
0)
@@ -19794,6 +19800,7 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr driver 
ATTRIBUTE_UNUSED,
 {
 size_t i;
 struct _virDomainInterfaceStats tmp;
+char *vf_representor_ifname = NULL;
 int ret = -1;
 
 if (!virDomainObjIsActive(dom))
@@ -19806,21 +19813,40 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr driver 
ATTRIBUTE_UNUSED,
 virDomainNetDefPtr net = dom->def->nets[i];
 virDomainNetType actualType;
 
-if (!net->ifname)
+actualType = virDomainNetGetActualType(net);
+
+if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
+if (virNetdevHostdevGetVFRepIFName(dom->def->hostdevs[i],
+   _representor_ifname))
+continue;
+}
+else if (!net->ifname)
 continue;
 
 memset(, 0, sizeof(tmp));
 
-actualType = virDomainNetGetActualType(net);
 
-QEMU_ADD_NAME_PARAM(record, maxparams,
-"net", "name", i, net->ifname);
+if (actualType != VIR_DOMAIN_NET_TYPE_HOSTDEV)
+QEMU_ADD_NAME_PARAM(record, maxparams,
+"net", "name", i, net->ifname);
+else
+QEMU_ADD_NAME_PARAM(record, maxparams,
+"net", "name", i, vf_representor_ifname);
 
 if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) {
 if (virNetDevOpenvswitchInterfaceStats(net->ifname, ) < 0) {
 virResetLastError();
 continue;
 }
+} else if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
+if (virNetdevHostdevVFRepInterfaceStats
+(vf_representor_ifname, ,
+ !virDomainNetTypeSharesHostView(net)) < 0) {
+VIR_FREE(vf_representor_ifname);
+virResetLastError();
+continue;
+}
+VIR_FREE(vf_representor_ifname);
 } else {
 if (virNetDevTapInterfaceStats(net->ifname, ,

!virDomainNetTypeSharesHostView(net)) < 0) {
-- 
2.13.6

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


Re: [libvirt] [PATCH] qemu: cpu: fix fullCPU to include all emulatable qemu features

2018-02-12 Thread Nikolay Shirokovskiy
ping

On 20.11.2017 15:55, Nikolay Shirokovskiy wrote:
> On Core i5 650 guest fail to start with error [1] if guest cpu config is taken
> from domcapabilities and check is set to partial.
> 
> The problem is in qemu caps fullCPU calculation in 
> virQEMUCapsInitHostCPUModel.
> It is supposed to include features emulated by qemu and missed on host. Some 
> of
> such features may be not included however.
> 
> For mentioned cpu host cpu is detected as Westmere and guest cpu as
> SandyBridge. x2apic is missed on host and provided by installed qemu. The
> feature is not mentioned in guest cpu features explicitly because SandyBridge
> model include it. As a result fullCPU does not include x2apic too.
> 
> Solution is to expand guest cpu features before updating fullCPU features.
> 
> [1] error: the CPU is incompatible with host CPU: Host CPU does not
> provide required features: x2apic, tsc-deadline
> ---
>  src/qemu/qemu_capabilities.c | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 9c1eeac..edba716 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -3475,6 +3475,7 @@ virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps,
>  virDomainVirtType type)
>  {
>  virCPUDefPtr cpu = NULL;
> +virCPUDefPtr cpuExpanded = NULL;
>  virCPUDefPtr migCPU = NULL;
>  virCPUDefPtr hostCPU = NULL;
>  virCPUDefPtr fullCPU = NULL;
> @@ -3504,9 +3505,13 @@ virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps,
>NULL, NULL)))
>  goto error;
>  
> -for (i = 0; i < cpu->nfeatures; i++) {
> -if (cpu->features[i].policy == VIR_CPU_FEATURE_REQUIRE &&
> -virCPUDefUpdateFeature(fullCPU, cpu->features[i].name,
> +if (!(cpuExpanded = virCPUDefCopy(cpu)) ||
> +virCPUExpandFeatures(qemuCaps->arch, cpuExpanded) < 0)
> +goto error;
> +
> +for (i = 0; i < cpuExpanded->nfeatures; i++) {
> +if (cpuExpanded->features[i].policy == VIR_CPU_FEATURE_REQUIRE &&
> +virCPUDefUpdateFeature(fullCPU, 
> cpuExpanded->features[i].name,
> VIR_CPU_FEATURE_REQUIRE) < 0)
>  goto error;
>  }
> @@ -3528,6 +3533,7 @@ virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps,
>  virQEMUCapsSetHostModel(qemuCaps, type, cpu, migCPU, fullCPU);
>  
>   cleanup:
> +virCPUDefFree(cpuExpanded);
>  virCPUDefFree(hostCPU);
>  return;
>  
> 

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