Re: [libvirt] [PATCH v3] qemu: Remove network type limitation for qemuARPGetInterfaces

2018-09-25 Thread Chen Hanxiao


At 2018-09-25 17:37:24, "Lin Ma"  wrote:
>Let's ignore the checking of interface type when we call the function
>qemuARPGetInterfaces to get IP from host's arp table.
>
>Based on suggestion from Laine.
>https://www.redhat.com/archives/libvir-list/2018-September/msg00684.html
>
>Signed-off-by: Lin Ma 
>---
> src/qemu/qemu_driver.c | 3 ---
> 1 file changed, 3 deletions(-)
>
>diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>index 10d6bca186..3110e74e0e 100644
>--- a/src/qemu/qemu_driver.c
>+++ b/src/qemu/qemu_driver.c
>@@ -20945,9 +20945,6 @@ qemuARPGetInterfaces(virDomainObjPtr vm,
> goto cleanup;
> 
> for (i = 0; i < vm->def->nnets; i++) {
>-if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK)
>-continue;
>-
> virMacAddrFormat(&(vm->def->nets[i]->mac), macaddr);
> for (j = 0; j < table->n; j++) {
> virArpTableEntry entry = table->t[j];
>-- 
>2.19.0

Reviewed-by: Chen Hanxiao 

Regards,
- Chen

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


Re: [libvirt] [PATCH 0/4] Small changes to virdbus

2018-09-25 Thread John Ferlan



On 9/21/18 9:02 AM, Marc Hartmayer wrote:
> Marc Hartmayer (4):
>   virdbus: Grab a ref as long as the while loop is executed
>   virdbus: Unref the D-Bus connection when closing
>   virdbus: Report a debug message that dbus_watch_handle() has failed
>   virdbus: Use the mnemonic macros for dbus_bool_t values
> 
>  src/util/virdbus.c | 12 
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 

Reviewed-by: John Ferlan 

(series) and pushed,

John

although /me wonders what happens to @info in virDBusAddWatch when the
call to virEventAddHandle fails, dbus_watch_set_data(watch, NULL, NULL)
is called and the code returns FALSE. I assume it's leaked, different
problem though ;-)

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


Re: [libvirt] Plans for next release

2018-09-25 Thread John Ferlan



On 9/25/18 9:38 AM, Daniel Veillard wrote:
> On Tue, Sep 25, 2018 at 09:42:20AM +0200, Michal Privoznik wrote:
>> On 09/24/2018 03:42 PM, Daniel Veillard wrote:
>>>   that's next week ! If we want to release around Oct 1st, I would
>>> suggest to enter freeze this Wed, probably in the morning europe time,
>>> then plan for an RC2 friday morning and if everything goes well, have
>>> the final release next monday.
>>>
>>>   I hope that works for everybody !
>>
>> Well, I introduced metadata locking in this release and as usual there
>> are some follow up patches waiting for review:
>>
>> https://www.redhat.com/archives/libvir-list/2018-September/msg01134.html
>>
>> Also, Bjoern reported some issues with the feature (/me wonders if it's
>> s390 specific) and he said he'll debug this week. Anyway, those patches
>> and any patch that he'll write can be viewed as a bug fix and therefore
>> can be merged during freeze.
> 
>   Okay, let's proceed as expected, on Friday we can assess if Monday is still
> right time for release, or if needed just postpone after Monday.
> 

I've reviewed the patches Michal posted - so that part is a fait
accompli ;-)

IMO: Since usage of the metadata lock logic requires some amount of
setup (e.g. changing a qemu.conf value), it's easy enough to indicate
that the support isn't ready. In fact, in Michal's response to my review
for patch 22/23:

https://www.redhat.com/archives/libvir-list/2018-September/msg00795.html

essentially indicates there's more to come in selinux.

So, I assume we can still call this "feature" as a work in progress.
Perhaps we need a new section in news.xml or some other means to
indicate we have "experimental code" with "limited support" (similar to
the QEMU development preview and/or the x- prefixed capabilities). IOW:
a buyer/user beware if you trip across this, then you are on your own -
until of course we fix all the "known" issues.

So in summary, I believe the issues raised thus far are primarily
metadata locking related, so should we really hold up the release for
something that isn't fully complete or listed in news.xml as a new feature?

John

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


Re: [libvirt] [PATCH 5/4] security: Don't try to lock NULL paths

2018-09-25 Thread John Ferlan



On 9/25/18 3:34 AM, Michal Privoznik wrote:
> It may happen that in the list of paths/disk sources to relabel
> there is a disk source. If that is the case, the path is NULL. In
> that case, we shouldn't try to lock the path. It's likely a
> network disk anyway and therefore there is nothing to lock.

I think this needs a tweak to reference commit 6d855abc1 which only
filtered if the provided @p was a directory. This adds another filter
when @p is NULL such as would be the case with networked storage.

NB: The storage source is only passed for DAC and not selinux. The DAC
code makes a some valiant attempts at src->path if not Local too. The
selinux code has lots of branches and callers which seem to validly pass
a path, but I could have missed a path or some nuance.

The "key" is chasing virSecurityDACChownListAppend and
virSecuritySELinuxContextListAppend where the list->[n]items is
populated via VIR_APPEND_ELEMENT. Expect to spend some time on the chase!

You already have an R-by and I don't have anything else to provide on
this particular one other than yeah, better safe than sorry and passing
NULL.  Although I have to imagine the stat(NULL, ) in virFileIsDir
wouldn't have been pleased.

John

> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/security/security_dac.c | 3 ++-
>  src/security/security_selinux.c | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 876cca0f2f..04168feb3d 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -216,7 +216,8 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
>  for (i = 0; i < list->nItems; i++) {
>  const char *p = list->items[i]->path;
>  
> -if (virFileIsDir(p))
> +if (!p ||
> +virFileIsDir(p))
>  continue;
>  
>  VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p);
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 3c847d8dcb..3adbeada14 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -227,7 +227,8 @@ virSecuritySELinuxTransactionRun(pid_t pid 
> ATTRIBUTE_UNUSED,
>  for (i = 0; i < list->nItems; i++) {
>  const char *p = list->items[i]->path;
>  
> -if (virFileIsDir(p))
> +if (!p ||
> +virFileIsDir(p))
>  continue;
>  
>  VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p);
> 

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


Re: [libvirt] [PATCH 4/4] security: Always spawn process for transactions

2018-09-25 Thread John Ferlan



On 9/21/18 5:29 AM, Michal Privoznik wrote:
> There is this latent bug which can result in virtlockd killing
> libvirtd. The problem is when the following steps occur:
> 
>Parent |  Child
> --+
> 1) virSecurityManagerMetadataLock(path);  |
>This results in connection FD to virtlockd to be   |
>dup()-ed and saved for later use.  |
>   |
> 2) Another thread calling fork(); |
>This results in the FD from step 1) to be cloned   |
>   |
> 3) virSecurityManagerMetadataUnlock(path);|
>Here, the FD is closed, but the connection is  |
>still kept open because child process has  |
>duplicated FD  |
>   |
> 4) virSecurityManagerMetadataLock(differentPath); |
>Again, new connection is opened, FD is dup()-ed|
>   |
> 5)| exec() or exit()
> 
> Step 5) results in closing the connection from 1) to be closed,
> finally. However, at this point virtlockd looks at its internal
> records if PID from 1) does not still own any resources. Well it
> does - in step 4) it locked differentPath. This results in
> virtlockd killing libvirtd.
> 
> Note that this happens because we do some relabelling even before
> fork() at which point vm->pid is still -1. When this value is
> passed down to transaction code it simply runs the transaction
> in parent process (=libvirtd), which means the owner of metadata
> locks is the parent process.
> 
> Signed-off-by: Michal Privoznik 
> 
> Signed-off-by: Michal Privoznik 

Always good to double up on the S-o-b's when there's locks and forks
involved. That makes sure the fork gets one too ;-)


> ---
>  src/security/security_dac.c | 12 ++--
>  src/security/security_selinux.c | 12 ++--
>  2 files changed, 12 insertions(+), 12 deletions(-)
> 

I read the cover, it's not simpler than the above. It also indicates
that patches 1-3 don't have any bearing. So to me that says - 2 series,
one the artsy/fartsy cleanup variety and the second this edge/corner
condition chase based on timing and/or other interactions.

My first question is - have you bisected your changes? Since this only
seems to matter for metadata locking, you don't necessarily have to
start too early, but I would assume that prior to commit 8b8aefb3 there
is no issue.

My second question is - given my analysis in patch2, was testing of this
patch done with or without patch 2 and 3? Hard to review this without
having my patch2 analysis rattling around in short term memory that
works. At this point, I'm thinking they were in place and may have had
an impact especially since that dup() call was removed, but it's
mentioned during this commit message, which is really confusing.

Prior to patch2/3, virNetSocketDupFD has 4 callers of which only one
calls with 'false' and that's the path having problems.

libxlMigrateDstReceive (using true)
libxlDomainMigrationSrcPerform (using true)
qemuMigrationSrcConnect (using true)
virNetClientDupFD (using false)

Since your testing certainly doesn't include the first 3, I'll focus on
the last one which has but one caller:

virLockManagerLockDaemonAcquire

meaning without patches 2-3, we got a "dup()" socket back instead of
changing the existing socket to F_DUPFD_CLOEXEC. I think I just talked
myself in a circle, but perhaps gives you an understanding of the
struggle with all the patches in one series.

> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 5aea386e7c..876cca0f2f 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -561,12 +561,12 @@ virSecurityDACTransactionCommit(virSecurityManagerPtr 
> mgr ATTRIBUTE_UNUSED,
>  goto cleanup;
>  }
>  
> -if ((pid == -1 &&
> - virSecurityDACTransactionRun(pid, list) < 0) ||
> -(pid != -1 &&
> - virProcessRunInMountNamespace(pid,
> -   virSecurityDACTransactionRun,
> -   list) < 0))
> +if (pid == -1)
> +pid = getpid();
> +
> +if (virProcessRunInMountNamespace(pid,
> +  virSecurityDACTransactionRun,
> +  list) < 0)

Going back to commit d41c1621, it essentially states:

"To differentiate whether transaction code should fork() or not the @pid
argument now accepts -1 (which means do not fork)."

This commit then backtracks or undoes that by using a getpid() causing
every transaction to fork.

The "logic" for the decision to pass pid == -1 or not is introduced 

Re: [libvirt] [PATCH 3/4] virLockManagerLockDaemonAcquire: Duplicate client FD with CLOEXEC flag

2018-09-25 Thread John Ferlan



On 9/21/18 5:29 AM, Michal Privoznik wrote:
> There is one caller (virSecurityManagerMetadataLock) which
> duplicates the connection FD and wants to have the flag set.
> However, trying to set the flag after dup() is not safe as
> another thread might fork() meanwhile. Therefore, switch to
> duplicating with the flag set and only let callers refine this
> later.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/locking/domain_lock.c   | 18 ++
>  src/locking/lock_driver_lockd.c |  2 +-
>  src/rpc/virnetclient.c  |  9 +
>  src/rpc/virnetclient.h  |  2 +-
>  4 files changed, 21 insertions(+), 10 deletions(-)
> 

> diff --git a/src/locking/domain_lock.c b/src/locking/domain_lock.c
> index 705b132457..db20fa86a3 100644
> --- a/src/locking/domain_lock.c
> +++ b/src/locking/domain_lock.c
> @@ -188,6 +188,24 @@ int virDomainLockProcessStart(virLockManagerPluginPtr 
> plugin,
>  ret = virLockManagerAcquire(lock, NULL, flags,
>  dom->def->onLockFailure, fd);
>  
> +if (ret >= 0 && fd && *fd >= 0 && virSetInherit(*fd, true) < 0) {

Not quite sure 'how' ret > 0, but I'll go with it considering other
consumers perform the same check.

> +int saved_errno = errno;
> +virErrorPtr origerr;
> +
> +virErrorPreserveLast();
> +
> +if (virLockManagerRelease(lock, NULL, 0) < 0)
> +VIR_WARN("Unable to release acquired resourced in cleanup path");

*resource(s)

> +
> +virErrorRestore();
> +errno = saved_errno;
> +
> +virReportSystemError(errno, "%s",
> + _("Cannot disable close-on-exec flag"));
> +
> +ret = -1;
> +}
> +

OK so this perhaps *is* the only thing you're after. Discounting patch2,
you get a dup()'d socket and you then remove the CLOEXEC from it.

The rest of the patch doesn't seem to matter.  Perhaps some day there is
a virNetClientDupFD consumer that wants to pass @true, then they have to
add back all that you removed.

John

>  virLockManagerFree(lock);
>  
>  return ret;
> diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
> index 0c672b05b1..9b1943daa6 100644
> --- a/src/locking/lock_driver_lockd.c
> +++ b/src/locking/lock_driver_lockd.c
> @@ -796,7 +796,7 @@ static int 
> virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
>  goto cleanup;
>  
>  if (fd &&
> -(*fd = virNetClientDupFD(client, false)) < 0)
> +(*fd = virNetClientDupFD(client)) < 0)
>  goto cleanup;
>  
>  if (!(flags & VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY)) {
> diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
> index 40ed3fde6d..6b0ddbeaad 100644
> --- a/src/rpc/virnetclient.c
> +++ b/src/rpc/virnetclient.c
> @@ -712,19 +712,12 @@ int virNetClientGetFD(virNetClientPtr client)
>  }
>  
>  
> -int virNetClientDupFD(virNetClientPtr client, bool cloexec)
> +int virNetClientDupFD(virNetClientPtr client)
>  {
>  int fd;
>  
>  virObjectLock(client);
> -
>  fd = virNetSocketDupFD(client->sock);
> -if (!cloexec && fd >= 0 && virSetInherit(fd, true)) {
> -virReportSystemError(errno, "%s",
> - _("Cannot disable close-on-exec flag"));
> -VIR_FORCE_CLOSE(fd);
> -}
> -
>  virObjectUnlock(client);
>  return fd;
>  }
> diff --git a/src/rpc/virnetclient.h b/src/rpc/virnetclient.h
> index 9cf32091f5..3702f7fe5a 100644
> --- a/src/rpc/virnetclient.h
> +++ b/src/rpc/virnetclient.h
> @@ -95,7 +95,7 @@ void virNetClientSetCloseCallback(virNetClientPtr client,
>virFreeCallback ff);
>  
>  int virNetClientGetFD(virNetClientPtr client);
> -int virNetClientDupFD(virNetClientPtr client, bool cloexec);
> +int virNetClientDupFD(virNetClientPtr client);
>  
>  bool virNetClientHasPassFD(virNetClientPtr client);
>  
> 

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


Re: [libvirt] [PATCH 2/4] virNetSocket: Be more safe with fork() around virNetSocketDupFD()

2018-09-25 Thread John Ferlan



On 9/21/18 5:29 AM, Michal Privoznik wrote:
> If there was a caller which would dup the client FD without
> CLOEXEC flag and later decided to change the flag it wouldn't be
> safe to do because fork() might have had occurred meantime.

blank line

> Switch to the other pattern - always dup FD with the flag set and
> let callers clear the flag if they need to do so.

I don't see this last sentence as happening in this patch...

> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/libxl/libxl_migration.c |  4 ++--
>  src/qemu/qemu_migration.c   |  2 +-
>  src/rpc/virnetclient.c  | 10 +-
>  src/rpc/virnetsocket.c  |  7 ++-
>  src/rpc/virnetsocket.h  |  2 +-
>  5 files changed, 15 insertions(+), 10 deletions(-)
> 

I'm with Marc on this - documenting virNetSocketDupFD is a good thing.
Although it could be considered out of the scope of what you're doing,
since you are changing the semantics describing the nuance that fork
brings about w/r/t fd duplication in the code rather than the commit
message doesn't make someone have to go back and look at the commit
message to gain more insight.  It also should provide the reader with
the knowledge of what virSetInherit would provide to remove if so desired.

Of course while you're at it, documenting virNetClientDupFD similarly
would be quite beneficial.

> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
> index fc7ccb53d0..5eb8eb1203 100644
> --- a/src/libxl/libxl_migration.c
> +++ b/src/libxl/libxl_migration.c
> @@ -310,7 +310,7 @@ libxlMigrateDstReceive(virNetSocketPtr sock,
>  }
>  VIR_DEBUG("Accepted migration connection."
>"  Spawning thread to process migration data");
> -recvfd = virNetSocketDupFD(client_sock, true);
> +recvfd = virNetSocketDupFD(client_sock);
>  virObjectUnref(client_sock);
>  
>  /*
> @@ -1254,7 +1254,7 @@ libxlDomainMigrationSrcPerform(libxlDriverPrivatePtr 
> driver,
>  goto cleanup;
>  }
>  
> -sockfd = virNetSocketDupFD(sock, true);
> +sockfd = virNetSocketDupFD(sock);
>  virObjectUnref(sock);
>  
>  if (virDomainLockProcessPause(driver->lockManager, vm, >lockState) 
> < 0)
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 825a9d399b..129be0a11a 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -3303,7 +3303,7 @@ qemuMigrationSrcConnect(virQEMUDriverPtr driver,
>  if (virNetSocketNewConnectTCP(host, port,
>AF_UNSPEC,
>) == 0) {
> -spec->dest.fd.qemu = virNetSocketDupFD(sock, true);
> +spec->dest.fd.qemu = virNetSocketDupFD(sock);
>  virObjectUnref(sock);
>  }
>  if (qemuSecurityClearSocketLabel(driver->securityManager, vm->def) < 0 ||
> diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
> index b4d8fb2187..40ed3fde6d 100644
> --- a/src/rpc/virnetclient.c
> +++ b/src/rpc/virnetclient.c
> @@ -715,8 +715,16 @@ int virNetClientGetFD(virNetClientPtr client)
>  int virNetClientDupFD(virNetClientPtr client, bool cloexec)
>  {
>  int fd;
> +
>  virObjectLock(client);
> -fd = virNetSocketDupFD(client->sock, cloexec);
> +
> +fd = virNetSocketDupFD(client->sock);
> +if (!cloexec && fd >= 0 && virSetInherit(fd, true)) {
> +virReportSystemError(errno, "%s",
> + _("Cannot disable close-on-exec flag"));
> +VIR_FORCE_CLOSE(fd);
> +}
> +

Caveat: my sock knowledge of setting or duplication of the socket fd
isn't that deep, especially as it relates to fork "mechanics".

So prior to this change, the call to virNetSocketDupFD(fd, false) would:

fd = dup(sock->fd);

Or essentially create a new socket fd from the existing one copying
everything it already has...

With this change you have (discounting some new error paths)

fd = fcntl(sock->fd, F_DUPFD_CLOEXEC, 0);
fcntl(fd, F_SETFD, ~FD_CLOEXEC)

which seems to be a nop and/or unnecessary to me.

Beyond that if the sock->fd didn't have CLOEXEC already, then what
impact does briefly changing CLOEXEC have on something that arrives
while set?

And of course if sock->fd had CLOEXEC and then you clear it on the
'shared' socket, that doesn't seem right either.

Seems to me the dup() does the trick and then lets the caller decide
what CLOEXEC semantics they want with the duplicated fd without perhaps
changing the sock->fd semantics.

>  virObjectUnlock(client);
>  return fd;
>  }
> diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
> index 55de3b2aad..27ffa23bcd 100644
> --- a/src/rpc/virnetsocket.c
> +++ b/src/rpc/virnetsocket.c
> @@ -1372,14 +1372,11 @@ int virNetSocketGetFD(virNetSocketPtr sock)
>  }
>  
>  
> -int virNetSocketDupFD(virNetSocketPtr sock, bool cloexec)
> +int virNetSocketDupFD(virNetSocketPtr sock)
>  {
>  int fd;
>  
> -if (cloexec)
> -fd = fcntl(sock->fd, F_DUPFD_CLOEXEC, 0);
> -

Re: [libvirt] [PATCH 1/4] security: Grab a reference to virSecurityManager for transactions

2018-09-25 Thread John Ferlan



On 9/21/18 5:29 AM, Michal Privoznik wrote:
> This shouldn't be needed per-se. Security manager shouldn't
> disappear during transactions - it's immutable. However, it
> doesn't hurt to grab a reference either - transaction code uses
> it after all.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/security/security_dac.c | 5 +++--
>  src/security/security_selinux.c | 5 +++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 

FWIW: I agree w/ Marc's assessment.  You need a patch 0.5 ;-) to add the
VIR_FREE(list->items) for selinux.  It should reference commit ca25026


> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 2dbaf29ff5..5aea386e7c 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -141,6 +141,7 @@ virSecurityDACChownListFree(void *opaque)
>  VIR_FREE(list->items[i]);
>  }
>  VIR_FREE(list->items);
> +virObjectUnref(list->manager);
>  VIR_FREE(list);
>  }
>  
> @@ -511,12 +512,12 @@ virSecurityDACTransactionStart(virSecurityManagerPtr 
> mgr)
>  if (VIR_ALLOC(list) < 0)
>  return -1;
>  
> -list->manager = mgr;
> +list->manager = virObjectRef(mgr);

If you move ^^^ to below vvv, then I think the VIR_FREE could still
apply. Realistically all that's happening by calling ListFree is the
Unref of list->manager.  Same for _selinux.

In fact, that'd probably be the more proper order with the Ref(mgr)
being done.

With all that,

Reviewed-by: John Ferlan 

John

I also assume you can add the VIR_FREE(list->items) to _selinux with an
appropriate commit message as a pre-patch and that too has my R-By.

>  
>  if (virThreadLocalSet(, list) < 0) {
>  virReportSystemError(errno, "%s",
>   _("Unable to set thread local variable"));
> -VIR_FREE(list);
> +virSecurityDACChownListFree(list);
>  return -1;
>  }
>  
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 056637e4cb..31e42afee7 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -156,6 +156,7 @@ virSecuritySELinuxContextListFree(void *opaque)
>  for (i = 0; i < list->nItems; i++)
>  virSecuritySELinuxContextItemFree(list->items[i]);
>  
> +virObjectUnref(list->manager);
>  VIR_FREE(list);
>  }
>  
> @@ -1054,12 +1055,12 @@ 
> virSecuritySELinuxTransactionStart(virSecurityManagerPtr mgr)
>  if (VIR_ALLOC(list) < 0)
>  return -1;
>  
> -list->manager = mgr;
> +list->manager = virObjectRef(mgr);
>  
>  if (virThreadLocalSet(, list) < 0) {
>  virReportSystemError(errno, "%s",
>   _("Unable to set thread local variable"));
> -VIR_FREE(list);
> +virSecuritySELinuxContextListFree(list);
>  return -1;
>  }
>  
> 

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


[libvirt] [PATCH 4/7] configure: remove check for regex.h

2018-09-25 Thread Ján Tomko
Introduced by:
commit 542039fab09bd46f419702667cd342ae8f88b498
Fully support mingw builds

Made redundant by:
commit ec8a2d0327ee214111bca04c39ab61a9fc247f28
regex: gnulib guarantees that we have regex support

Signed-off-by: Ján Tomko 
---
 configure.ac | 1 -
 1 file changed, 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 2d2c783dc2..8e77a1c9d2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -360,7 +360,6 @@ AC_CHECK_HEADERS([\
   net/ethernet.h \
   netinet/tcp.h \
   pwd.h \
-  regex.h \
   stdarg.h \
   syslog.h \
   sys/mount.h \
-- 
2.16.4

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

[libvirt] [PATCH 2/7] configure: sort AC_CHECK_HEADERS argument list

2018-09-25 Thread Ján Tomko
Signed-off-by: Ján Tomko 
---
 configure.ac | 22 +++---
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/configure.ac b/configure.ac
index f23faf8b8f..00e2de7b1f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -353,23 +353,23 @@ AC_CHECK_FUNCS_ONCE([\
 
 dnl Availability of various common headers (non-fatal if missing).
 AC_CHECK_HEADERS([\
+  ifaddrs.h \
+  libtasn1.h \
+  linux/magic.h \
+  mntent.h \
+  net/ethernet.h \
+  netinet/tcp.h \
   pwd.h \
   regex.h \
-  sys/un.h \
-  sys/poll.h \
+  stdarg.h \
   syslog.h \
-  mntent.h \
-  net/ethernet.h \
-  linux/magic.h \
-  sys/un.h \
+  sys/mount.h \
+  sys/poll.h \
   sys/syscall.h \
   sys/sysctl.h \
-  netinet/tcp.h \
-  ifaddrs.h \
-  libtasn1.h \
   sys/ucred.h \
-  sys/mount.h \
-  stdarg.h \
+  sys/un.h \
+  sys/un.h \
   ])
 dnl Check whether endian provides handy macros.
 AC_CHECK_DECLS([htole64], [], [], [[#include ]])
-- 
2.16.4

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

[libvirt] [PATCH 3/7] configure: remove duplicit check for sys/un.h

2018-09-25 Thread Ján Tomko
Commit 7c08fcc4 added this one.

Signed-off-by: Ján Tomko 
---
 configure.ac | 1 -
 1 file changed, 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 00e2de7b1f..2d2c783dc2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -369,7 +369,6 @@ AC_CHECK_HEADERS([\
   sys/sysctl.h \
   sys/ucred.h \
   sys/un.h \
-  sys/un.h \
   ])
 dnl Check whether endian provides handy macros.
 AC_CHECK_DECLS([htole64], [], [], [[#include ]])
-- 
2.16.4

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

[libvirt] [PATCH 7/7] configure: remove regexec and getgrnam_r checks

2018-09-25 Thread Ján Tomko
Introduced by:
commit 635ae38979bbb2659a21dfaa57e7c762ae8d4e88
commit 1b745219c7507595d4a09c89aa6a331eaa039e31
But their HAVE_ counterparts were never used.

Signed-off-by: Ján Tomko 
---
 configure.ac | 2 --
 1 file changed, 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 89bb6f86fc..39600017ab 100644
--- a/configure.ac
+++ b/configure.ac
@@ -327,7 +327,6 @@ AC_CHECK_FUNCS_ONCE([\
   fallocate \
   geteuid \
   getgid \
-  getgrnam_r \
   getmntent_r \
   getpwuid_r \
   getrlimit \
@@ -338,7 +337,6 @@ AC_CHECK_FUNCS_ONCE([\
   posix_fallocate \
   posix_memalign \
   prlimit \
-  regexec \
   sched_getaffinity \
   setgroups \
   setns \
-- 
2.16.4

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

[libvirt] [PATCH 6/7] configure: do not check for kill

2018-09-25 Thread Ján Tomko
Introduced by:
commit 3c37a171a2dea8048dfee90e1be2875f30f7eb15
Add check for kill() to fix build of cgroups on win32

Made redundant by:
commit 02f1fd41f60c90b636ba0e18b37d4624fe47135d
cgroup macros refactoring, part 1

Signed-off-by: Ján Tomko 
---
 configure.ac | 1 -
 1 file changed, 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 4993013daa..89bb6f86fc 100644
--- a/configure.ac
+++ b/configure.ac
@@ -333,7 +333,6 @@ AC_CHECK_FUNCS_ONCE([\
   getrlimit \
   getuid \
   if_indextoname \
-  kill \
   mmap \
   newlocale \
   posix_fallocate \
-- 
2.16.4

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

[libvirt] [PATCH 5/7] configure: remove check for poll.h

2018-09-25 Thread Ján Tomko
Introduced by:
commit b38d045dea41ea1fb41e546e61388116eddb6b3c
Remove use of sys/poll.h on mingw

Made redundant by:
  commit 0c97e70b74434b4baca9bbfc19c14bf7ff2ef304
Update event loop example programs to demonstrate best practice

Signed-off-by: Ján Tomko 
---
 configure.ac | 1 -
 1 file changed, 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 8e77a1c9d2..4993013daa 100644
--- a/configure.ac
+++ b/configure.ac
@@ -363,7 +363,6 @@ AC_CHECK_HEADERS([\
   stdarg.h \
   syslog.h \
   sys/mount.h \
-  sys/poll.h \
   sys/syscall.h \
   sys/sysctl.h \
   sys/ucred.h \
-- 
2.16.4

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

[libvirt] [PATCH 1/7] configure: split common header and function checks

2018-09-25 Thread Ján Tomko
Use one line per entry, to work better with line-based git history.

Signed-off-by: Ján Tomko 
---
 configure.ac | 56 +++-
 1 file changed, 47 insertions(+), 9 deletions(-)

diff --git a/configure.ac b/configure.ac
index 1b5c56b1a9..f23faf8b8f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -322,17 +322,55 @@ AC_CHECK_SIZEOF([long])
 
 dnl Availability of various common functions (non-fatal if missing),
 dnl and various less common threadsafe functions
-AC_CHECK_FUNCS_ONCE([cfmakeraw fallocate geteuid getgid getgrnam_r \
-  getmntent_r getpwuid_r getrlimit getuid if_indextoname kill mmap \
-  newlocale posix_fallocate posix_memalign prlimit regexec \
-  sched_getaffinity setgroups setns setrlimit symlink sysctlbyname \
-  getifaddrs sched_setscheduler unshare])
+AC_CHECK_FUNCS_ONCE([\
+  cfmakeraw \
+  fallocate \
+  geteuid \
+  getgid \
+  getgrnam_r \
+  getmntent_r \
+  getpwuid_r \
+  getrlimit \
+  getuid \
+  if_indextoname \
+  kill \
+  mmap \
+  newlocale \
+  posix_fallocate \
+  posix_memalign \
+  prlimit \
+  regexec \
+  sched_getaffinity \
+  setgroups \
+  setns \
+  setrlimit \
+  symlink \
+  sysctlbyname \
+  getifaddrs \
+  sched_setscheduler \
+  unshare \
+  ])
 
 dnl Availability of various common headers (non-fatal if missing).
-AC_CHECK_HEADERS([pwd.h regex.h sys/un.h \
-  sys/poll.h syslog.h mntent.h net/ethernet.h linux/magic.h \
-  sys/un.h sys/syscall.h sys/sysctl.h netinet/tcp.h ifaddrs.h \
-  libtasn1.h sys/ucred.h sys/mount.h stdarg.h])
+AC_CHECK_HEADERS([\
+  pwd.h \
+  regex.h \
+  sys/un.h \
+  sys/poll.h \
+  syslog.h \
+  mntent.h \
+  net/ethernet.h \
+  linux/magic.h \
+  sys/un.h \
+  sys/syscall.h \
+  sys/sysctl.h \
+  netinet/tcp.h \
+  ifaddrs.h \
+  libtasn1.h \
+  sys/ucred.h \
+  sys/mount.h \
+  stdarg.h \
+  ])
 dnl Check whether endian provides handy macros.
 AC_CHECK_DECLS([htole64], [], [], [[#include ]])
 AC_CHECK_FUNCS([stat stat64 __xstat __xstat64 lstat lstat64 __lxstat 
__lxstat64])
-- 
2.16.4

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

[libvirt] [PATCH 0/7] configure: remove some unused checks

2018-09-25 Thread Ján Tomko
Ján Tomko (7):
  configure: split common header and function checks
  configure: sort AC_CHECK_HEADERS argument list
  configure: remove duplicit check for sys/un.h
  configure: remove check for regex.h
  configure: remove check for poll.h
  configure: do not check for kill
  configure: remove regexec and getgrnam_r checks

 configure.ac | 50 +-
 1 file changed, 41 insertions(+), 9 deletions(-)

-- 
2.16.4

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

Re: [libvirt] [PATCH] lxc_monitor: Avoid AB / BA lock race

2018-09-25 Thread Michal Privoznik
On 09/25/2018 04:31 PM, Mark Asselstine wrote:
> On Tuesday, September 25, 2018 5:20:26 AM EDT Michal Privoznik wrote:
>> On 09/24/2018 05:11 PM, Mark Asselstine wrote:

> 
> Here is my read, again I have to caveat that this code is rather new to me.
> 
> The object poisoning in virObjectUnref() would prevent a double free as the 
> second call would fail VIR_OBJECT_NOTVALID(). However, you are still correct 
> as the mon object would potentially be destroyed and cleaned up before the 
> close callback is run and thus when the the close callback is eventually run 
> it would be operating on an old, no longer valid object.
> 
> So good catch on your part, thanks.

Yep, makes sense. I've ACKed and pushed this.

Congratulations on your first libvirt contribution. Usually it is just a
typo fix but you fixed a deadlock. Impressive.

Michal

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


Re: [libvirt] [PATCH v2 5/5] qemu: Avoid duplicate resume events and state changes

2018-09-25 Thread John Ferlan



On 9/25/18 9:19 AM, Jiri Denemark wrote:
> The only place where VIR_DOMAIN_EVENT_RESUMED should be generated is the
> RESUME event handler to make sure we don't generate duplicate events or
> state changes. In the worse case the duplicity can revert or cover
> changes done by other event handlers.
> 
> For example, after QEMU sent RESUME, BLOCK_IO_ERROR, and STOP events
> we could happily mark the domain as running and report
> VIR_DOMAIN_EVENT_RESUMED to registered clients.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1612943
> 
> Signed-off-by: Jiri Denemark 
> ---
> 
> Notes:
> Version 2:
> - keep VIR_DOMAIN_EVENT_RESUMED_MIGRATED event at the end of post-copy
>   migration
> 
>  src/qemu/qemu_driver.c| 13 ---
>  src/qemu/qemu_migration.c | 49 ---
>  src/qemu/qemu_process.c   | 10 
>  3 files changed, 24 insertions(+), 48 deletions(-)
> 

This resolves the issue I noted, so

Reviewed-by: John Ferlan 

John

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


Re: [libvirt] [PATCH] lxc_monitor: Avoid AB / BA lock race

2018-09-25 Thread Mark Asselstine
On Tuesday, September 25, 2018 5:20:26 AM EDT Michal Privoznik wrote:
> On 09/24/2018 05:11 PM, Mark Asselstine wrote:
> > A deadlock situation can occur when autostarting a LXC domain 'guest'
> > due to two threads attempting to take opposing locks while holding
> > opposing locks (AB BA problem). Thread A takes and holds the 'vm' lock
> > while attempting to take the 'client' lock, meanwhile, thread B takes
> > and holds the 'client' lock while attempting to take the 'vm' lock.
> > 
> > The potential for this can be seen as follows:
> > 
> > Thread A:
> > virLXCProcessAutostartDomain (takes vm lock)
> > 
> >  --> virLXCProcessStart
> >  
> >   --> virLXCProcessConnectMonitor
> >   
> >--> virLXCMonitorNew
> >
> > --> virNetClientSetCloseCallback (wants client lock)
> > 
> > Thread B:
> > virNetClientIncomingEvent (takes client lock)
> > 
> >  --> virNetClientIOHandleInput
> >  
> >   --> virNetClientCallDispatch
> >   
> >--> virNetClientCallDispatchMessage
> >
> > --> virNetClientProgramDispatch
> > 
> >  --> virLXCMonitorHandleEventInit
> >  
> >   --> virLXCProcessMonitorInitNotify (wants vm lock)
> > 
> > Since these threads are scheduled independently and are preemptible it
> > is possible for the deadlock scenario to occur where each thread locks
> > their first lock but both will fail to get their second lock and just
> > spin forever. You get something like:
> > 
> > virLXCProcessAutostartDomain (takes vm lock)
> > 
> >  --> virLXCProcessStart
> >  
> >   --> virLXCProcessConnectMonitor
> >   
> >--> virLXCMonitorNew
> > 
> > <...>
> > virNetClientIncomingEvent (takes client lock)
> > 
> >  --> virNetClientIOHandleInput
> >  
> >   --> virNetClientCallDispatch
> >   
> >--> virNetClientCallDispatchMessage
> >
> > --> virNetClientProgramDispatch
> > 
> >  --> virLXCMonitorHandleEventInit
> >  
> >   --> virLXCProcessMonitorInitNotify (wants vm lock but spins)
> > 
> > <...>
> > 
> > --> virNetClientSetCloseCallback (wants client lock but spins)
> > 
> > Neither thread ever gets the lock it needs to be able to continue
> > while holding the lock that the other thread needs.
> > 
> > The actual window for preemption which can cause this deadlock is
> > rather small, between the calls to virNetClientProgramNew() and
> > execution of virNetClientSetCloseCallback(), both in
> > virLXCMonitorNew(). But it can be seen in real world use that this
> > small window is enough.
> > 
> > By moving the call to virNetClientSetCloseCallback() ahead of
> > virNetClientProgramNew() we can close any possible chance of the
> > deadlock taking place. There should be no other implications to the
> > move since the close callback (in the unlikely event was called) will
> > spin on the vm lock. The remaining work that takes place between the
> > old call location of virNetClientSetCloseCallback() and the new
> > location is unaffected by the move.
> > 
> > Signed-off-by: Mark Asselstine 
> > ---
> > 
> > NOTE I am not intimately familiar with the libvirt code, so although I
> > am confident of the cause of the issue I am not sure this is the best
> > solution. So I am really curious what people who do know this code
> > think and as such I am definitely open to discussions and suggestions
> > on how to rework things to properly address the issue.
> > 
> >  src/lxc/lxc_monitor.c | 6 --
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/lxc/lxc_monitor.c b/src/lxc/lxc_monitor.c
> > index e765c16..218f426 100644
> > --- a/src/lxc/lxc_monitor.c
> > +++ b/src/lxc/lxc_monitor.c
> > @@ -161,6 +161,10 @@ virLXCMonitorPtr virLXCMonitorNew(virDomainObjPtr vm,
> > 
> >  if (virNetClientRegisterAsyncIO(mon->client) < 0)
> >  
> >  goto error;
> > 
> > +/* avoid deadlock by making this call before assigning
> > virLXCMonitorEvents */ +virNetClientSetCloseCallback(mon->client,
> > virLXCMonitorEOFNotify, mon, +
> > virLXCMonitorCloseFreeCallback); +
> > 
> >  if (!(mon->program = virNetClientProgramNew(VIR_LXC_MONITOR_PROGRAM,
> >  
> >  VIR_LXC_MONITOR_PROGRAM_V
> >  ERSION,
> >  virLXCMonitorEvents,
> > 
> > @@ -176,8 +180,6 @@ virLXCMonitorPtr virLXCMonitorNew(virDomainObjPtr vm,
> > 
> >  memcpy(>cb, cb, sizeof(mon->cb));
> >  
> >  virObjectRef(mon);
> 
> This ref ^^
> 
> > -virNetClientSetCloseCallback(mon->client, virLXCMonitorEOFNotify,
> > mon,
> > - virLXCMonitorCloseFreeCallback);
> > 
> >   cleanup:
> >  VIR_FREE(sockpath);
> 
> isn't it needed because of this SetCloseCallback, or actually
> virLXCMonitorCloseFreeCallback()? The extra reference is held around so
> that virLXCMonitorEOFNotify can use it and it's then released in
> 

Re: [libvirt] [PATCH] vircgroupv1: add ifdef around cgroup code

2018-09-25 Thread Ján Tomko

On Tue, Sep 25, 2018 at 03:35:11PM +0200, Pavel Hrdina wrote:

Cgroups are supported only on linux.

Signed-off-by: Pavel Hrdina 
---
src/util/vircgroupv1.c | 14 ++
1 file changed, 14 insertions(+)

diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c
index 62a6e5c448..3d05fbd745 100644
--- a/src/util/vircgroupv1.c
+++ b/src/util/vircgroupv1.c
@@ -55,6 +55,8 @@ VIR_ENUM_IMPL(virCgroupV1Controller, 
VIR_CGROUP_CONTROLLER_LAST,
  "name=systemd");


+#ifdef VIR_CGROUP_SUPPORTED
+
/* We're looking for at least one 'cgroup' fs mount,
 * which is *not* a named mount. */
static bool
@@ -2099,3 +2101,15 @@ virCgroupV1Register(void)
{
virCgroupBackendRegister();
}
+
+#else /* !VIR_CGROUP_SUPPORTED */
+
+void
+virCgroupV1Register(void)
+{
+virReportSystemError(ENOSYS, "%s",
+ _("Control groups not supported on this platform"));
+return -1;


util/vircgroupv1.c:2112:5: error: void function 'virCgroupV1Register' should 
not return a value [-Wreturn-type]
   return -1;
   ^  ~~
1 error generated.

Also, vircgrouptest.c is only guarded by #ifdef __linux__. I don't know
whether it was quietly broken on Linuxes that do not satisfy
VIR_CGROUP_SUPPORTED and nobody cared, or the dependency was introduced
by:
commit 8b62008d2bc5442f7755e579ea754ffd5e3f9691
   vircgrouptest: call virCgroupNewSelf instead virCgroupDetectMounts

Jano


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

Re: [libvirt] [PATCH] vircgroupv1: add ifdef around cgroup code

2018-09-25 Thread Erik Skultety
On Tue, Sep 25, 2018 at 03:35:11PM +0200, Pavel Hrdina wrote:
> Cgroups are supported only on linux.
>
> Signed-off-by: Pavel Hrdina 
> ---
>  src/util/vircgroupv1.c | 14 ++
>  1 file changed, 14 insertions(+)
>
> diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c
> index 62a6e5c448..3d05fbd745 100644
> --- a/src/util/vircgroupv1.c
> +++ b/src/util/vircgroupv1.c
> @@ -55,6 +55,8 @@ VIR_ENUM_IMPL(virCgroupV1Controller, 
> VIR_CGROUP_CONTROLLER_LAST,
>"name=systemd");
>
>
> +#ifdef VIR_CGROUP_SUPPORTED

this is defined in vircgroup.c, I don't think this works as expected in the
current form, I'd suggested moving the definition of the macro itself to a
header.

Erik

> +
>  /* We're looking for at least one 'cgroup' fs mount,
>   * which is *not* a named mount. */
>  static bool
> @@ -2099,3 +2101,15 @@ virCgroupV1Register(void)
>  {
>  virCgroupBackendRegister();
>  }
> +
> +#else /* !VIR_CGROUP_SUPPORTED */
> +
> +void
> +virCgroupV1Register(void)
> +{
> +virReportSystemError(ENOSYS, "%s",
> + _("Control groups not supported on this platform"));
> +return -1;
> +}
> +
> +#endif /* !VIR_CGROUP_SUPPORTED */
> --
> 2.17.1
>
> --
> 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] Plans for next release

2018-09-25 Thread Daniel Veillard
On Tue, Sep 25, 2018 at 09:42:20AM +0200, Michal Privoznik wrote:
> On 09/24/2018 03:42 PM, Daniel Veillard wrote:
> >   that's next week ! If we want to release around Oct 1st, I would
> > suggest to enter freeze this Wed, probably in the morning europe time,
> > then plan for an RC2 friday morning and if everything goes well, have
> > the final release next monday.
> > 
> >   I hope that works for everybody !
> 
> Well, I introduced metadata locking in this release and as usual there
> are some follow up patches waiting for review:
> 
> https://www.redhat.com/archives/libvir-list/2018-September/msg01134.html
> 
> Also, Bjoern reported some issues with the feature (/me wonders if it's
> s390 specific) and he said he'll debug this week. Anyway, those patches
> and any patch that he'll write can be viewed as a bug fix and therefore
> can be merged during freeze.

  Okay, let's proceed as expected, on Friday we can assess if Monday is still
right time for release, or if needed just postpone after Monday.

  thanks,

Daniel

-- 
Daniel Veillard  | Red Hat Developers Tools http://developer.redhat.com/
veill...@redhat.com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | virtualization library  http://libvirt.org/

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


[libvirt] [PATCH] vircgroupv1: add ifdef around cgroup code

2018-09-25 Thread Pavel Hrdina
Cgroups are supported only on linux.

Signed-off-by: Pavel Hrdina 
---
 src/util/vircgroupv1.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c
index 62a6e5c448..3d05fbd745 100644
--- a/src/util/vircgroupv1.c
+++ b/src/util/vircgroupv1.c
@@ -55,6 +55,8 @@ VIR_ENUM_IMPL(virCgroupV1Controller, 
VIR_CGROUP_CONTROLLER_LAST,
   "name=systemd");
 
 
+#ifdef VIR_CGROUP_SUPPORTED
+
 /* We're looking for at least one 'cgroup' fs mount,
  * which is *not* a named mount. */
 static bool
@@ -2099,3 +2101,15 @@ virCgroupV1Register(void)
 {
 virCgroupBackendRegister();
 }
+
+#else /* !VIR_CGROUP_SUPPORTED */
+
+void
+virCgroupV1Register(void)
+{
+virReportSystemError(ENOSYS, "%s",
+ _("Control groups not supported on this platform"));
+return -1;
+}
+
+#endif /* !VIR_CGROUP_SUPPORTED */
-- 
2.17.1

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


[libvirt] [PATCH v2 4/5] qemu: Map running reason to resume event detail

2018-09-25 Thread Jiri Denemark
Thanks to the previous commit the RESUME event handler knows what reason
should be used when changing the domain state to VIR_DOMAIN_RUNNING, but
the emitted VIR_DOMAIN_EVENT_RESUMED event still uses a generic
VIR_DOMAIN_EVENT_RESUMED_UNPAUSED detail. Luckily, the event detail can
be easily deduced from the running reason, which saves us from having to
pass one more value to the handler.

Signed-off-by: Jiri Denemark 
Reviewed-by: John Ferlan 
---

Notes:
Version 2:
- no change

 src/qemu/qemu_domain.c  | 29 +
 src/qemu/qemu_domain.h  |  3 +++
 src/qemu/qemu_process.c | 11 +++
 3 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index b20d43059e..939b2a3da2 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -13505,3 +13505,32 @@ qemuDomainStorageIdReset(qemuDomainObjPrivatePtr priv)
 {
 priv->nodenameindex = 0;
 }
+
+
+virDomainEventResumedDetailType
+qemuDomainRunningReasonToResumeEvent(virDomainRunningReason reason)
+{
+switch (reason) {
+case VIR_DOMAIN_RUNNING_RESTORED:
+case VIR_DOMAIN_RUNNING_FROM_SNAPSHOT:
+return VIR_DOMAIN_EVENT_RESUMED_FROM_SNAPSHOT;
+
+case VIR_DOMAIN_RUNNING_MIGRATED:
+case VIR_DOMAIN_RUNNING_MIGRATION_CANCELED:
+return VIR_DOMAIN_EVENT_RESUMED_MIGRATED;
+
+case VIR_DOMAIN_RUNNING_POSTCOPY:
+return VIR_DOMAIN_EVENT_RESUMED_POSTCOPY;
+
+case VIR_DOMAIN_RUNNING_UNKNOWN:
+case VIR_DOMAIN_RUNNING_SAVE_CANCELED:
+case VIR_DOMAIN_RUNNING_BOOTED:
+case VIR_DOMAIN_RUNNING_UNPAUSED:
+case VIR_DOMAIN_RUNNING_WAKEUP:
+case VIR_DOMAIN_RUNNING_CRASHED:
+case VIR_DOMAIN_RUNNING_LAST:
+break;
+}
+
+return VIR_DOMAIN_EVENT_RESUMED_UNPAUSED;
+}
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 461859e37f..2f8a1bf03c 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -1082,4 +1082,7 @@ char * 
qemuDomainGetManagedPRSocketPath(qemuDomainObjPrivatePtr priv);
 unsigned int qemuDomainStorageIdNew(qemuDomainObjPrivatePtr priv);
 void qemuDomainStorageIdReset(qemuDomainObjPrivatePtr priv);
 
+virDomainEventResumedDetailType
+qemuDomainRunningReasonToResumeEvent(virDomainRunningReason reason);
+
 #endif /* __QEMU_DOMAIN_H__ */
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index f79db1d4cf..a7d71962da 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -714,6 +714,7 @@ qemuProcessHandleResume(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
 virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 qemuDomainObjPrivatePtr priv;
 virDomainRunningReason reason = VIR_DOMAIN_RUNNING_UNPAUSED;
+virDomainEventResumedDetailType eventDetail;
 
 virObjectLock(vm);
 
@@ -729,14 +730,16 @@ qemuProcessHandleResume(qemuMonitorPtr mon 
ATTRIBUTE_UNUSED,
 goto unlock;
 }
 
+eventDetail = qemuDomainRunningReasonToResumeEvent(reason);
 VIR_DEBUG("Transitioned guest %s out of paused into resumed state, "
-  "reason '%s'",
-  vm->def->name, virDomainRunningReasonTypeToString(reason));
+  "reason '%s', event detail %d",
+  vm->def->name, virDomainRunningReasonTypeToString(reason),
+  eventDetail);
 
 virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, reason);
 event = virDomainEventLifecycleNewFromObj(vm,
- VIR_DOMAIN_EVENT_RESUMED,
- VIR_DOMAIN_EVENT_RESUMED_UNPAUSED);
+  VIR_DOMAIN_EVENT_RESUMED,
+  eventDetail);
 
 if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, 
driver->caps) < 0) {
 VIR_WARN("Unable to save status on vm %s after state change",
-- 
2.19.0

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


Re: [libvirt] [PATCH 5/4] security: Don't try to lock NULL paths

2018-09-25 Thread Erik Skultety
On Tue, Sep 25, 2018 at 01:01:59PM +0200, Michal Privoznik wrote:
> On 09/25/2018 12:47 PM, Erik Skultety wrote:
> > On Tue, Sep 25, 2018 at 09:34:50AM +0200, Michal Privoznik wrote:
> >> It may happen that in the list of paths/disk sources to relabel
> >> there is a disk source. If that is the case, the path is NULL. In
> >
> > Is there a way to prevent to such a source would not make it into the list 
> > in
> > the first place?
>
> Not at all. Disk source may consists of variety of strings. A path is
> just one of the options. The other is host FQN for iscsi, rbd, ceph and
> all sorts of other beasts. And yet, we might want to relabel them.

Fair enough.
Reviewed-by: Erik Skultety 

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


[libvirt] [PATCH v2 2/5] qemu: Report more appropriate running reasons

2018-09-25 Thread Jiri Denemark
This patch replaces some rather generic VIR_DOMAIN_RUNNING_UNPAUSED
reasons when changing domain state to running with more specific ones.
All of them are done when libvirtd reconnects to an existing domain
after being restarted and sees an unfinished migration or save.

Signed-off-by: Jiri Denemark 
Reviewed-by: John Ferlan 
---

Notes:
Version 2:
- modified commit message

 src/qemu/qemu_process.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index a471a1e839..417e47ec29 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3292,7 +3292,7 @@ qemuProcessRecoverMigrationIn(virQEMUDriverPtr driver,
 VIR_DEBUG("Incoming migration finished, resuming domain %s",
   vm->def->name);
 if (qemuProcessStartCPUs(driver, vm,
- VIR_DOMAIN_RUNNING_UNPAUSED,
+ VIR_DOMAIN_RUNNING_MIGRATED,
  QEMU_ASYNC_JOB_NONE) < 0) {
 VIR_WARN("Could not resume domain %s", vm->def->name);
 }
@@ -3399,7 +3399,7 @@ qemuProcessRecoverMigrationOut(virQEMUDriverPtr driver,
 (reason == VIR_DOMAIN_PAUSED_MIGRATION ||
  reason == VIR_DOMAIN_PAUSED_UNKNOWN)) {
 if (qemuProcessStartCPUs(driver, vm,
- VIR_DOMAIN_RUNNING_UNPAUSED,
+ VIR_DOMAIN_RUNNING_MIGRATION_CANCELED,
  QEMU_ASYNC_JOB_NONE) < 0) {
 VIR_WARN("Could not resume domain %s", vm->def->name);
 }
@@ -3457,7 +3457,7 @@ qemuProcessRecoverJob(virQEMUDriverPtr driver,
 reason == VIR_DOMAIN_PAUSED_MIGRATION)) ||
   reason == VIR_DOMAIN_PAUSED_UNKNOWN)) {
  if (qemuProcessStartCPUs(driver, vm,
-  VIR_DOMAIN_RUNNING_UNPAUSED,
+  VIR_DOMAIN_RUNNING_SAVE_CANCELED,
   QEMU_ASYNC_JOB_NONE) < 0) {
  VIR_WARN("Could not resume domain '%s' after migration to 
file",
   vm->def->name);
-- 
2.19.0

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


[libvirt] [PATCH v2 5/5] qemu: Avoid duplicate resume events and state changes

2018-09-25 Thread Jiri Denemark
The only place where VIR_DOMAIN_EVENT_RESUMED should be generated is the
RESUME event handler to make sure we don't generate duplicate events or
state changes. In the worse case the duplicity can revert or cover
changes done by other event handlers.

For example, after QEMU sent RESUME, BLOCK_IO_ERROR, and STOP events
we could happily mark the domain as running and report
VIR_DOMAIN_EVENT_RESUMED to registered clients.

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

Signed-off-by: Jiri Denemark 
---

Notes:
Version 2:
- keep VIR_DOMAIN_EVENT_RESUMED_MIGRATED event at the end of post-copy
  migration

 src/qemu/qemu_driver.c| 13 ---
 src/qemu/qemu_migration.c | 49 ---
 src/qemu/qemu_process.c   | 10 
 3 files changed, 24 insertions(+), 48 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 0c1afbfd4e..126c783a0f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1861,7 +1861,6 @@ static int qemuDomainResume(virDomainPtr dom)
 virQEMUDriverPtr driver = dom->conn->privateData;
 virDomainObjPtr vm;
 int ret = -1;
-virObjectEventPtr event = NULL;
 int state;
 int reason;
 virQEMUDriverConfigPtr cfg = NULL;
@@ -1900,9 +1899,6 @@ static int qemuDomainResume(virDomainPtr dom)
"%s", _("resume operation failed"));
 goto endjob;
 }
-event = virDomainEventLifecycleNewFromObj(vm,
- VIR_DOMAIN_EVENT_RESUMED,
- VIR_DOMAIN_EVENT_RESUMED_UNPAUSED);
 }
 if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 
0)
 goto endjob;
@@ -1913,7 +1909,6 @@ static int qemuDomainResume(virDomainPtr dom)
 
  cleanup:
 virDomainObjEndAPI();
-virObjectEventStateQueue(driver->domainEventState, event);
 virObjectUnref(cfg);
 return ret;
 }
@@ -15983,7 +15978,6 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr 
snapshot,
 virDomainDefPtr config = NULL;
 virQEMUDriverConfigPtr cfg = NULL;
 virCapsPtr caps = NULL;
-bool was_running = false;
 bool was_stopped = false;
 qemuDomainSaveCookiePtr cookie;
 virCPUDefPtr origCPU = NULL;
@@ -16174,7 +16168,6 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr 
snapshot,
 priv = vm->privateData;
 if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
 /* Transitions 5, 6 */
-was_running = true;
 if (qemuProcessStopCPUs(driver, vm,
 VIR_DOMAIN_PAUSED_FROM_SNAPSHOT,
 QEMU_ASYNC_JOB_START) < 0)
@@ -16271,12 +16264,6 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr 
snapshot,
 event = virDomainEventLifecycleNewFromObj(vm,
  VIR_DOMAIN_EVENT_STARTED,
  detail);
-} else if (!was_running) {
-/* Transition 8 */
-detail = VIR_DOMAIN_EVENT_RESUMED_FROM_SNAPSHOT;
-event = virDomainEventLifecycleNewFromObj(vm,
- VIR_DOMAIN_EVENT_RESUMED,
- detail);
 }
 }
 break;
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 825a9d399b..67940330aa 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2982,14 +2982,10 @@ qemuMigrationSrcConfirmPhase(virQEMUDriverPtr driver,
 virFreeError(orig_err);
 
 if (virDomainObjGetState(vm, ) == VIR_DOMAIN_PAUSED &&
-reason == VIR_DOMAIN_PAUSED_POSTCOPY) {
+reason == VIR_DOMAIN_PAUSED_POSTCOPY)
 qemuMigrationAnyPostcopyFailed(driver, vm);
-} else if (qemuMigrationSrcRestoreDomainState(driver, vm)) {
-event = virDomainEventLifecycleNewFromObj(vm,
-  VIR_DOMAIN_EVENT_RESUMED,
-  
VIR_DOMAIN_EVENT_RESUMED_MIGRATED);
-virObjectEventStateQueue(driver->domainEventState, event);
-}
+else
+qemuMigrationSrcRestoreDomainState(driver, vm);
 
 qemuMigrationParamsReset(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT,
  priv->job.migParams, priv->job.apiFlags);
@@ -4624,11 +4620,7 @@ qemuMigrationSrcPerformJob(virQEMUDriverPtr driver,
 qemuMigrationParamsReset(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT,
  priv->job.migParams, priv->job.apiFlags);
 
-if (qemuMigrationSrcRestoreDomainState(driver, vm)) {
-event = virDomainEventLifecycleNewFromObj(vm,
- VIR_DOMAIN_EVENT_RESUMED,
- 

[libvirt] [PATCH v2 1/5] qemu: Properly report VIR_DOMAIN_EVENT_RESUMED_FROM_SNAPSHOT

2018-09-25 Thread Jiri Denemark
VIR_DOMAIN_EVENT_RESUMED_FROM_SNAPSHOT was defined but not used anywhere
in our event generation code. This fixes qemuDomainRevertToSnapshot to
properly report why the domain was resumed.

Signed-off-by: Jiri Denemark 
Reviewed-by: John Ferlan 
---

Notes:
Version 2:
- no change

 src/qemu/qemu_driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 10d6bca186..0c1afbfd4e 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16273,7 +16273,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr 
snapshot,
  detail);
 } else if (!was_running) {
 /* Transition 8 */
-detail = VIR_DOMAIN_EVENT_RESUMED;
+detail = VIR_DOMAIN_EVENT_RESUMED_FROM_SNAPSHOT;
 event = virDomainEventLifecycleNewFromObj(vm,
  VIR_DOMAIN_EVENT_RESUMED,
  detail);
-- 
2.19.0

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


[libvirt] [PATCH v2 0/5] qemu: Change the way we generate VIR_DOMAIN_EVENT_RESUMED

2018-09-25 Thread Jiri Denemark
https://bugzilla.redhat.com/show_bug.cgi?id=1612943

Version 2:
- keep VIR_DOMAIN_EVENT_RESUMED_MIGRATED event at the end of post-copy
  migration

Jiri Denemark (5):
  qemu: Properly report VIR_DOMAIN_EVENT_RESUMED_FROM_SNAPSHOT
  qemu: Report more appropriate running reasons
  qemu: Pass running reason to RESUME event handler
  qemu: Map running reason to resume event detail
  qemu: Avoid duplicate resume events and state changes

 src/qemu/qemu_domain.c| 29 +++
 src/qemu/qemu_domain.h|  7 ++
 src/qemu/qemu_driver.c| 13 ---
 src/qemu/qemu_migration.c | 49 ---
 src/qemu/qemu_process.c   | 46 ++--
 5 files changed, 85 insertions(+), 59 deletions(-)

-- 
2.19.0

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


[libvirt] [PATCH v2 3/5] qemu: Pass running reason to RESUME event handler

2018-09-25 Thread Jiri Denemark
Whenever we get the RESUME event from QEMU, we change the state of the
affected domain to VIR_DOMAIN_RUNNING with VIR_DOMAIN_RUNNING_UNPAUSED
reason. This is fine if the domain is resumed unexpectedly, but when we
sent "cont" to QEMU we usually have a better reason for the state
change. The better reason is used in qemuProcessStartCPUs which also
sets the domain state to running if qemuMonitorStartCPUs reports
success. Thus we may end up with two state updates in a row, but the
final reason is correct.

This patch is a preparation for dropping the state change done in
qemuMonitorStartCPUs for which we need to pass the actual running reason
to the RESUME event handler and use it there instead of
VIR_DOMAIN_RUNNING_UNPAUSED.

Signed-off-by: Jiri Denemark 
Reviewed-by: John Ferlan 
---

Notes:
Version 2:
- no change

 src/qemu/qemu_domain.h  |  4 
 src/qemu/qemu_process.c | 23 +--
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index ecbe2e8533..461859e37f 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -366,6 +366,10 @@ struct _qemuDomainObjPrivate {
 
 /* counter for generating node names for qemu disks */
 unsigned long long nodenameindex;
+
+/* qemuProcessStartCPUs stores the reason for starting vCPUs here for the
+ * RESUME event handler to use it */
+virDomainRunningReason runningReason;
 };
 
 # define QEMU_DOMAIN_PRIVATE(vm) \
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 417e47ec29..f79db1d4cf 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -712,21 +712,28 @@ qemuProcessHandleResume(qemuMonitorPtr mon 
ATTRIBUTE_UNUSED,
 virQEMUDriverPtr driver = opaque;
 virObjectEventPtr event = NULL;
 virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+qemuDomainObjPrivatePtr priv;
+virDomainRunningReason reason = VIR_DOMAIN_RUNNING_UNPAUSED;
 
 virObjectLock(vm);
-if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) {
-qemuDomainObjPrivatePtr priv = vm->privateData;
 
+priv = vm->privateData;
+if (priv->runningReason != VIR_DOMAIN_RUNNING_UNKNOWN) {
+reason = priv->runningReason;
+priv->runningReason = VIR_DOMAIN_RUNNING_UNKNOWN;
+}
+
+if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) {
 if (priv->gotShutdown) {
 VIR_DEBUG("Ignoring RESUME event after SHUTDOWN");
 goto unlock;
 }
 
-VIR_DEBUG("Transitioned guest %s out of paused into resumed state",
-  vm->def->name);
+VIR_DEBUG("Transitioned guest %s out of paused into resumed state, "
+  "reason '%s'",
+  vm->def->name, virDomainRunningReasonTypeToString(reason));
 
-virDomainObjSetState(vm, VIR_DOMAIN_RUNNING,
- VIR_DOMAIN_RUNNING_UNPAUSED);
+virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, reason);
 event = virDomainEventLifecycleNewFromObj(vm,
  VIR_DOMAIN_EVENT_RESUMED,
  VIR_DOMAIN_EVENT_RESUMED_UNPAUSED);
@@ -3087,6 +3094,8 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver, 
virDomainObjPtr vm,
 }
 VIR_FREE(priv->lockState);
 
+priv->runningReason = reason;
+
 if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
 goto release;
 
@@ -3104,6 +3113,7 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver, 
virDomainObjPtr vm,
 return ret;
 
  release:
+priv->runningReason = VIR_DOMAIN_RUNNING_UNKNOWN;
 if (virDomainLockProcessPause(driver->lockManager, vm, >lockState) < 
0)
 VIR_WARN("Unable to release lease on %s", vm->def->name);
 VIR_DEBUG("Preserving lock state '%s'", NULLSTR(priv->lockState));
@@ -5987,6 +5997,7 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver,
 priv->monError = false;
 priv->monStart = 0;
 priv->gotShutdown = false;
+priv->runningReason = VIR_DOMAIN_RUNNING_UNKNOWN;
 
 VIR_DEBUG("Updating guest CPU definition");
 if (qemuProcessUpdateGuestCPU(vm->def, priv->qemuCaps, caps, flags) < 0)
-- 
2.19.0

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


[libvirt] [PATCH] tests: libxl: skip tests with nested_hvm

2018-09-25 Thread Ján Tomko
Commit 95d19cd unconditionally adjusted the tests to account for
the conditional move of the nested_hvm setting location.

Run the affected tests only for the new setup (witnessed by
LIBXL_HAVE_BUILDINFO_NESTED_HVM).

Signed-off-by: Ján Tomko 
---
Technically a build breaker fix, but I'd love to hear any ideas
that would easily let us test both versions.

Also, my app-emulation/xen-tools-4.9.2-r1 on Gentoo
does not seem to have LIBXL_HAVE_BUILDINFO_NESTED_HVM in libxl.h,
even though it was introduced in 4.9.0-rc7-831-g3c44f8ed44

 tests/libxlxml2domconfigtest.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c
index 0eba5814b1..6b1f9826d0 100644
--- a/tests/libxlxml2domconfigtest.c
+++ b/tests/libxlxml2domconfigtest.c
@@ -207,9 +207,16 @@ mymain(void)
 DO_TEST("cpu-shares-hvm");
 DO_TEST("variable-clock-hvm");
 DO_TEST("moredevs-hvm");
+
+# ifdef LIBXL_HAVE_BUILDINFO_NESTED_HVM
 DO_TEST("vnuma-hvm");
+# endif
+
 DO_TEST("multiple-ip");
+
+# ifdef LIBXL_HAVE_BUILDINFO_NESTED_HVM
 DO_TEST("fullvirt-cpuid");
+# endif
 
 unlink("libxl-driver.log");
 
-- 
2.16.4

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

Re: [libvirt] [RFC PATCH auto partition NUMA guest domains v1 0/2] auto partition guests providing the host NUMA topology

2018-09-25 Thread Jiri Denemark
On Tue, Sep 25, 2018 at 12:02:40 +0200, Wim Ten Have wrote:
> From: Wim ten Have 
> 
> This patch extends the guest domain administration adding support
> to automatically advertise the host NUMA node capabilities obtained
> architecture under a guest by creating a vNUMA copy.

I'm pretty sure someone would find this useful and such configuration is
perfectly valid in libvirt. But I don't think there is a compelling
reason to add some magic into the domain XML which would automatically
expand to such configuration. It's basically a NUMA placement policy and
libvirt generally tries to avoid including any kind of policies and
rather just provide all the mechanisms and knobs which can be used by
applications to implement any policy they like.

> The mechanism is enabled by setting the check='numa' attribute under
> the CPU 'host-passthrough' topology:
>   

Anyway, this is definitely not the right place for such option. The
'check' attribute is described as

"Since 3.2.0, an optional check attribute can be used to request a
specific way of checking whether the virtual CPU matches the
specification."

and the new 'numa' value does not fit in there in any way.

Moreover the code does the automatic NUMA placement at the moment
libvirt parses the domain XML, which is not the right place since it
would break migration, snapshots, and save/restore features.

We have existing placement attributes for vcpu and numatune/memory
elements which would have been much better place for implementing such
feature. And event cpu/numa element could have been enhanced to support
similar configuration.

Jirka

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


Re: [libvirt] [PATCH 47/47] vircgroup: rename controllers to legacy

2018-09-25 Thread Pavel Hrdina
On Tue, Sep 25, 2018 at 12:49:46PM +0200, Ján Tomko wrote:
> On Tue, Sep 18, 2018 at 05:46:08PM +0200, Pavel Hrdina wrote:
> > Signed-off-by: Pavel Hrdina 
> > ---
> > src/util/vircgroup.c |   6 +-
> > src/util/vircgrouppriv.h |   2 +-
> > src/util/vircgroupv1.c   | 140 +++
> > tests/vircgrouptest.c|  14 ++--
> > 4 files changed, 81 insertions(+), 81 deletions(-)
> > 
> 
> Reviewed-by: Ján Tomko 
> 
> Jano

Thanks for review, pushed now with all the issues fixed.

Pavel


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

Re: [libvirt] [PATCH 0/6] syntax-check: Introduce a new rule to check misaligned stuff and fix it (batch I)

2018-09-25 Thread Michal Privoznik
On 09/25/2018 02:03 PM, Ján Tomko wrote:
> On Mon, Sep 24, 2018 at 03:30:02PM +0200, Michal Privoznik wrote:
>> On 09/24/2018 03:14 PM, Ján Tomko wrote:
>>> Please no.
>>>
>>> Despite my comments on the first spacing rule [0], this series slows
>>> down the spacing check by 100 % (from 1.6s to 3.1s in my case).
>>
>> I don't think that syntax-check run time is our goal.
> 
> Well that's just sad.

Why is that? I spend more time trying to figure our what the code does
than running syntax-check over it. Therefore in my view code cleanliness
is more important. But if you provide some argument otherwise we can
certainly discuss it.

Michal

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


Re: [libvirt] [PATCH 0/6] syntax-check: Introduce a new rule to check misaligned stuff and fix it (batch I)

2018-09-25 Thread Ján Tomko

On Mon, Sep 24, 2018 at 03:30:02PM +0200, Michal Privoznik wrote:

On 09/24/2018 03:14 PM, Ján Tomko wrote:

Please no.

Despite my comments on the first spacing rule [0], this series slows
down the spacing check by 100 % (from 1.6s to 3.1s in my case).


I don't think that syntax-check run time is our goal.


Well that's just sad.

Jano


I think it's the
code cleanliness.


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

Re: [libvirt] [PATCH 5/4] security: Don't try to lock NULL paths

2018-09-25 Thread Michal Privoznik
On 09/25/2018 12:47 PM, Erik Skultety wrote:
> On Tue, Sep 25, 2018 at 09:34:50AM +0200, Michal Privoznik wrote:
>> It may happen that in the list of paths/disk sources to relabel
>> there is a disk source. If that is the case, the path is NULL. In
> 
> Is there a way to prevent to such a source would not make it into the list in
> the first place?

Not at all. Disk source may consists of variety of strings. A path is
just one of the options. The other is host FQN for iscsi, rbd, ceph and
all sorts of other beasts. And yet, we might want to relabel them.

> 
>> that case, we shouldn't try to lock the path. It's likely a
>> network disk anyway and therefore there is nothing to lock.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/security/security_dac.c | 3 ++-
>>  src/security/security_selinux.c | 3 ++-
>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
>> index 876cca0f2f..04168feb3d 100644
>> --- a/src/security/security_dac.c
>> +++ b/src/security/security_dac.c
>> @@ -216,7 +216,8 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
>>  for (i = 0; i < list->nItems; i++) {
>>  const char *p = list->items[i]->path;
>>
>> -if (virFileIsDir(p))
>> +if (!p ||
>> +virFileIsDir(p))
> 
> ^this easily fits on the same line...

Yep, I just wanted to separate these conditions as they both serve
different purposes. But I can move it onto a single line if desired.

Michal

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


Re: [libvirt] [PATCH 47/47] vircgroup: rename controllers to legacy

2018-09-25 Thread Ján Tomko

On Tue, Sep 18, 2018 at 05:46:08PM +0200, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
src/util/vircgroup.c |   6 +-
src/util/vircgrouppriv.h |   2 +-
src/util/vircgroupv1.c   | 140 +++
tests/vircgrouptest.c|  14 ++--
4 files changed, 81 insertions(+), 81 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 46/47] vircgroup: rename virCgroupController into virCgroupV1Controller

2018-09-25 Thread Ján Tomko

On Tue, Sep 18, 2018 at 05:46:07PM +0200, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
src/util/vircgrouppriv.h | 8 
src/util/vircgroupv1.c   | 4 ++--
2 files changed, 6 insertions(+), 6 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 45/47] vircgroup: extract virCgroupV1(Set|Get)CpusetCpus

2018-09-25 Thread Ján Tomko

On Tue, Sep 18, 2018 at 05:46:06PM +0200, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
src/util/vircgroup.c| 10 ++
src/util/vircgroupbackend.h | 10 ++
src/util/vircgroupv1.c  | 24 
3 files changed, 36 insertions(+), 8 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 5/4] security: Don't try to lock NULL paths

2018-09-25 Thread Erik Skultety
On Tue, Sep 25, 2018 at 09:34:50AM +0200, Michal Privoznik wrote:
> It may happen that in the list of paths/disk sources to relabel
> there is a disk source. If that is the case, the path is NULL. In

Is there a way to prevent to such a source would not make it into the list in
the first place?

> that case, we shouldn't try to lock the path. It's likely a
> network disk anyway and therefore there is nothing to lock.
>
> Signed-off-by: Michal Privoznik 
> ---
>  src/security/security_dac.c | 3 ++-
>  src/security/security_selinux.c | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 876cca0f2f..04168feb3d 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -216,7 +216,8 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
>  for (i = 0; i < list->nItems; i++) {
>  const char *p = list->items[i]->path;
>
> -if (virFileIsDir(p))
> +if (!p ||
> +virFileIsDir(p))

^this easily fits on the same line...

Erik

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


Re: [libvirt] [PATCH 44/47] vircgroup: extract virCgroupV1(Set|Get)CpusetMemoryMigrate

2018-09-25 Thread Ján Tomko

On Tue, Sep 18, 2018 at 05:46:05PM +0200, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
src/util/vircgroup.c| 13 ++---
src/util/vircgroupbackend.h | 10 ++
src/util/vircgroupv1.c  | 27 +++
3 files changed, 39 insertions(+), 11 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 43/47] vircgroup: extract virCgroupV1(Set|Get)CpusetMems

2018-09-25 Thread Ján Tomko

On Tue, Sep 18, 2018 at 05:46:04PM +0200, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
src/util/vircgroup.c| 10 ++
src/util/vircgroupbackend.h | 11 +++
src/util/vircgroupv1.c  | 25 +
3 files changed, 38 insertions(+), 8 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 42/47] vircgroup: extract virCgroupV1(Set|Get)FreezerState

2018-09-25 Thread Ján Tomko

On Tue, Sep 18, 2018 at 05:46:03PM +0200, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
src/util/vircgroup.c|  8 ++--
src/util/vircgroupbackend.h | 11 +++
src/util/vircgroupv1.c  | 23 +++
3 files changed, 36 insertions(+), 6 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 41/47] vircgroup: extract virCgroupV1GetCpuacctStat

2018-09-25 Thread Ján Tomko

On Tue, Sep 18, 2018 at 05:46:02PM +0200, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
src/util/vircgroup.c| 39 +-
src/util/vircgroupbackend.h |  6 +
src/util/vircgroupv1.c  | 47 +
3 files changed, 54 insertions(+), 38 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 40/47] vircgroup: extract virCgroupV1GetCpuacct*Usage

2018-09-25 Thread Ján Tomko

On Tue, Sep 18, 2018 at 05:46:01PM +0200, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
src/util/vircgroup.c|  7 ++-
src/util/vircgroupbackend.h | 11 +++
src/util/vircgroupv1.c  | 22 ++
3 files changed, 35 insertions(+), 5 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 39/47] vircgroup: extract virCgroupV1SupportsCpuBW

2018-09-25 Thread Ján Tomko

On Tue, Sep 18, 2018 at 05:46:00PM +0200, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
src/util/vircgroup.c| 13 +
src/util/vircgroupbackend.h |  4 
src/util/vircgroupv1.c  | 19 +++
3 files changed, 24 insertions(+), 12 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 37/47] vircgroup: extract virCgroupV1(Set|Get)CpuCfsPeriod

2018-09-25 Thread Ján Tomko

On Tue, Sep 18, 2018 at 05:45:58PM +0200, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
src/util/vircgroup.c| 18 ++
src/util/vircgroupbackend.h | 10 ++
src/util/vircgroupv1.c  | 32 
3 files changed, 44 insertions(+), 16 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 38/47] vircgroup: extract virCgroupV1(Set|Get)CpuCfsQuota

2018-09-25 Thread Ján Tomko

On Tue, Sep 18, 2018 at 05:45:59PM +0200, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
src/util/vircgroup.c| 20 +++-
src/util/vircgroupbackend.h | 10 ++
src/util/vircgrouppriv.h|  5 +
src/util/vircgroupv1.c  | 32 
4 files changed, 50 insertions(+), 17 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 36/47] vircgroup: extract virCgroupV1(Set|Get)CpuShares

2018-09-25 Thread Ján Tomko

On Tue, Sep 18, 2018 at 05:45:57PM +0200, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
src/util/vircgroup.c|  8 ++--
src/util/vircgroupbackend.h | 11 +++
src/util/vircgroupv1.c  | 23 +++
3 files changed, 36 insertions(+), 6 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 35/47] vircgroup: extract virCgroupV1(Allow|Deny)AllDevices

2018-09-25 Thread Ján Tomko

On Tue, Sep 18, 2018 at 05:45:56PM +0200, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
src/util/vircgroup.c| 18 ++
src/util/vircgroupbackend.h |  9 +
src/util/vircgroupv1.c  | 31 +++
3 files changed, 42 insertions(+), 16 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v2 0/9] cgroup cleanups and preparation for v2

2018-09-25 Thread Pavel Hrdina
On Thu, Sep 20, 2018 at 09:41:28PM +0200, Fabiano Fidêncio wrote:
> On Thu, Sep 20, 2018 at 10:54 AM, Pavel Hrdina  wrote:

Thanks for the review, pushed now.

Pavel


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

Re: [libvirt] [PATCH 34/47] vircgroup: extract virCgroupV1(Allow|Deny)Device

2018-09-25 Thread Ján Tomko

On Tue, Sep 18, 2018 at 05:45:55PM +0200, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
src/util/vircgroup.c| 68 +++-
src/util/vircgroupbackend.h | 17 +
src/util/vircgroupv1.c  | 69 +
3 files changed, 98 insertions(+), 56 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 33/47] vircgroup: extract virCgroupV1GetMemSwapUsage

2018-09-25 Thread Ján Tomko

On Tue, Sep 18, 2018 at 05:45:54PM +0200, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
src/util/vircgroup.c|  9 +
src/util/vircgroupbackend.h |  5 +
src/util/vircgroupv1.c  | 16 
3 files changed, 22 insertions(+), 8 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 32/47] vircgroup: extract virCgroupV1(Set|Get)Memory*Limit

2018-09-25 Thread Ján Tomko

On Tue, Sep 18, 2018 at 05:45:53PM +0200, Pavel Hrdina wrote:

They all need virCgroupV1GetMemoryUnlimitedKB() so it's easier to
move them in one commit.

Signed-off-by: Pavel Hrdina 
---
src/util/vircgroup.c| 128 ++-
src/util/vircgroupbackend.h |  30 +++
src/util/vircgrouppriv.h|   6 ++
src/util/vircgroupv1.c  | 168 
4 files changed, 211 insertions(+), 121 deletions(-)

diff --git a/src/util/vircgroupbackend.h b/src/util/vircgroupbackend.h
index 672fb082c1..5e75e495c3 100644
--- a/src/util/vircgroupbackend.h
+++ b/src/util/vircgroupbackend.h
@@ -219,6 +219,30 @@ typedef int
(*virCgroupGetMemoryUsageCB)(virCgroupPtr group,
 unsigned long *kb);

+typedef int
+(*virCgroupSetMemoryHardLimitCB)(virCgroupPtr group,
+ unsigned long long kb);
+
+typedef int
+(*virCgroupGetMemoryHardLimitCB)(virCgroupPtr group,
+ unsigned long long *kb);
+
+typedef int
+(*virCgroupSetMemorySoftLimitCB)(virCgroupPtr group,
+ unsigned long long kb);
+
+typedef int
+(*virCgroupGetMemorySoftLimitCB)(virCgroupPtr group,
+ unsigned long long *kb);
+
+typedef int
+(*virCgroupSetMemSwapHardLimitCB)(virCgroupPtr group,
+  unsigned long long kb);
+
+typedef int
+(*virCgroupGetMemSwapHardLimitCB)(virCgroupPtr group,
+  unsigned long long *kb);
+
struct _virCgroupBackend {
virCgroupBackendType type;



There are just two different types here.

Regardless:
Reviewed-by: Ján Tomko 

Jano


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

[libvirt] [RFC PATCH auto partition NUMA guest domains v1 2/2] qemuxml2argv: add tests that exercise vNUMA auto partition topology

2018-09-25 Thread Wim Ten Have
From: Wim ten Have 

Add tests to ensure that the virDomainNumaAutoconfig() routine to auto-
partition vNUMA topology generates correct KVM/QEMU cmdline arguments
under applicable  setup.

Signed-off-by: Wim ten Have 
---
 .../cpu-host-passthrough-nonuma.args  | 25 
 .../cpu-host-passthrough-nonuma.xml   | 18 
 .../cpu-host-passthrough-numa.args| 29 +++
 .../cpu-host-passthrough-numa.xml | 18 
 tests/qemuxml2argvtest.c  |  2 ++
 5 files changed, 92 insertions(+)
 create mode 100644 tests/qemuxml2argvdata/cpu-host-passthrough-nonuma.args
 create mode 100644 tests/qemuxml2argvdata/cpu-host-passthrough-nonuma.xml
 create mode 100644 tests/qemuxml2argvdata/cpu-host-passthrough-numa.args
 create mode 100644 tests/qemuxml2argvdata/cpu-host-passthrough-numa.xml

diff --git a/tests/qemuxml2argvdata/cpu-host-passthrough-nonuma.args 
b/tests/qemuxml2argvdata/cpu-host-passthrough-nonuma.args
new file mode 100644
index ..4599cdfcc159
--- /dev/null
+++ b/tests/qemuxml2argvdata/cpu-host-passthrough-nonuma.args
@@ -0,0 +1,25 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-x86_64 \
+-name QEMUGuest1 \
+-S \
+-machine pc,accel=kvm,usb=off,dump-guest-core=off \
+-cpu host \
+-m 214 \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev 
socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
+server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-usb \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/cpu-host-passthrough-nonuma.xml 
b/tests/qemuxml2argvdata/cpu-host-passthrough-nonuma.xml
new file mode 100644
index ..d8daa8c9a43a
--- /dev/null
+++ b/tests/qemuxml2argvdata/cpu-host-passthrough-nonuma.xml
@@ -0,0 +1,18 @@
+
+  QEMUGuest1
+  c7a5fdbd-edaf-9455-926a-d65c16db1809
+  219100
+  219100
+  1
+  
+hvm
+  
+  
+  
+  destroy
+  restart
+  destroy
+  
+/usr/bin/qemu-system-x86_64
+  
+
diff --git a/tests/qemuxml2argvdata/cpu-host-passthrough-numa.args 
b/tests/qemuxml2argvdata/cpu-host-passthrough-numa.args
new file mode 100644
index ..5b7b357cc0fe
--- /dev/null
+++ b/tests/qemuxml2argvdata/cpu-host-passthrough-numa.args
@@ -0,0 +1,29 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-x86_64 \
+-name QEMUGuest1 \
+-S \
+-machine pc,accel=kvm,usb=off,dump-guest-core=off \
+-cpu host \
+-m 216 \
+-smp 6,sockets=6,cores=1,threads=1 \
+-numa node,nodeid=0,cpus=0,cpus=4,mem=54 \
+-numa node,nodeid=1,cpus=1,cpus=5,mem=54 \
+-numa node,nodeid=2,cpus=2,mem=54 \
+-numa node,nodeid=3,cpus=3,mem=54 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev 
socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
+server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-usb \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/cpu-host-passthrough-numa.xml 
b/tests/qemuxml2argvdata/cpu-host-passthrough-numa.xml
new file mode 100644
index ..39488521b27d
--- /dev/null
+++ b/tests/qemuxml2argvdata/cpu-host-passthrough-numa.xml
@@ -0,0 +1,18 @@
+
+  QEMUGuest1
+  c7a5fdbd-edaf-9455-926a-d65c16db1809
+  219100
+  219100
+  6
+  
+hvm
+  
+  
+  
+  destroy
+  restart
+  destroy
+  
+/usr/bin/qemu-system-x86_64
+  
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index a7cde3ed7e74..4b6436b32ac7 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1745,6 +1745,8 @@ mymain(void)
  FLAG_SKIP_LEGACY_CPUS | FLAG_EXPECT_FAILURE,
  0, GIC_NONE, NONE);
 DO_TEST("cpu-host-passthrough", QEMU_CAPS_KVM);
+DO_TEST("cpu-host-passthrough-numa", QEMU_CAPS_NUMA);
+DO_TEST("cpu-host-passthrough-nonuma", QEMU_CAPS_NUMA);
 DO_TEST_FAILURE("cpu-qemu-host-passthrough", QEMU_CAPS_KVM);
 
 qemuTestSetHostArch(driver.caps, VIR_ARCH_S390X);
-- 
2.17.1

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


[libvirt] [RFC PATCH auto partition NUMA guest domains v1 1/2] domain: auto partition guests providing the host NUMA topology

2018-09-25 Thread Wim Ten Have
From: Wim ten Have 

Add a mechanism to auto partition the host NUMA topology under the
guest domain.

This patch adds a framework to automatically partition the host into a
small vNUMA subset defined by the guest XML given  and 
description when  are in
effect and the hypervisor indicates per the host capabilities that a
physical NUMA topology is in effect.

The mechanism automatically renders the host capabilities provided NUMA
architecture, evenly balances the guest reserved vcpu and memory amongst
its vNUMA composed cells and have the cell allocated vcpus pinned towards
the host NUMA node physical cpusets.  This in such way that the host NUMA
topology is still in effect under the partitioned guest vNUMA domain.

Signed-off-by: Wim ten Have 
---
 docs/formatdomain.html.in |   7 ++
 docs/schemas/cputypes.rng |   1 +
 src/conf/cpu_conf.c   |   3 +-
 src/conf/cpu_conf.h   |   1 +
 src/conf/domain_conf.c| 166 ++
 5 files changed, 177 insertions(+), 1 deletion(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 1f12ab5b4214..ba073d952545 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1500,6 +1500,13 @@
   The virtual CPU created by the hypervisor will be checked
 against the CPU specification and the domain will not be started
 unless the two CPUs match.
+
+  numa
+  When the CPU mode='host-passthrough' check='numa' option
+combination is set, libvirt auto partitions the guest domain
+by rendering the host NUMA architecture.  Here the virtual
+CPUs and memory are evenly balanced across the defined NUMA
+nodes. The vCPUs are also pinned to their physical CPUs.
 
 
 Since 0.9.10, an optional mode
diff --git a/docs/schemas/cputypes.rng b/docs/schemas/cputypes.rng
index 1f1e0e36d59b..d384d161ee7e 100644
--- a/docs/schemas/cputypes.rng
+++ b/docs/schemas/cputypes.rng
@@ -29,6 +29,7 @@
 none
 partial
 full
+numa
   
 
   
diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
index 863413e75eaa..0d52f6aa4813 100644
--- a/src/conf/cpu_conf.c
+++ b/src/conf/cpu_conf.c
@@ -52,7 +52,8 @@ VIR_ENUM_IMPL(virCPUCheck, VIR_CPU_CHECK_LAST,
   "default",
   "none",
   "partial",
-  "full")
+  "full",
+  "numa")
 
 VIR_ENUM_IMPL(virCPUFallback, VIR_CPU_FALLBACK_LAST,
   "allow",
diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h
index 9f2e7ee2649d..f2e2f0bef3ae 100644
--- a/src/conf/cpu_conf.h
+++ b/src/conf/cpu_conf.h
@@ -68,6 +68,7 @@ typedef enum {
 VIR_CPU_CHECK_NONE,
 VIR_CPU_CHECK_PARTIAL,
 VIR_CPU_CHECK_FULL,
+VIR_CPU_CHECK_NUMA,
 
 VIR_CPU_CHECK_LAST
 } virCPUCheck;
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9911d56130a9..c2f9398cfe85 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1759,6 +1759,168 @@ virDomainDefGetVcpusTopology(const virDomainDef *def,
 }
 
 
+/**
+ * virDomainNumaAutoconfig: auto partition guest vNUMA XML definitions
+ * taking the machine NUMA topology creating a small guest copy instance.
+ * @def: domain definition
+ * @caps: host capabilities
+ *
+ * Auto partitioning vNUMA guests is requested under XML configuration
+ * .  Here libvirt takes the
+ * host NUMA topology, including maxvcpus, online vcpus, memory and
+ * pinning node cpu's where it renders the guest domain vNUMA topology
+ * building an architectural copy of the host.
+ *
+ * Returns 0 on success and -1 on error.
+ */
+static int
+virDomainNumaAutoconfig(virDomainDefPtr def,
+virCapsPtr caps)
+{
+int ret = -1;
+
+if (caps && def->cpu &&
+def->cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH &&
+def->cpu->check == VIR_CPU_CHECK_NUMA) {
+
+size_t i, cell;
+size_t nvcpus = 0;
+size_t nnumaCell = 0;
+unsigned long long memsizeCell = 0;
+virBitmapPtr vnumask = NULL;
+virCapsHostPtr host = >host;
+
+nnumaCell = host->nnumaCell;
+if (!nnumaCell)
+goto cleanup;
+
+/* Compute the online vcpus */
+for (i = 0; i < def->maxvcpus; i++)
+if (def->vcpus[i]->online)
+nvcpus++;
+
+if (nvcpus < nnumaCell) {
+VIR_WARN("vNUMA disabled: %ld vcpus is insufficient "
+ "to arrange a vNUMA topology for %ld nodes.",
+  nvcpus, nnumaCell);
+goto cleanup;
+}
+
+/* Compute the memory size (memsizeCell) per arranged nnumaCell
+ */
+if ((memsizeCell = def->mem.total_memory / nnumaCell) == 0)
+goto cleanup;
+
+/* Correct vNUMA can only be accomplished if the number of maxvcpus
+ * is a multiple of the number of physical nodes.  If this is not
+ * possible we set 

[libvirt] [RFC PATCH auto partition NUMA guest domains v1 0/2] auto partition guests providing the host NUMA topology

2018-09-25 Thread Wim Ten Have
From: Wim ten Have 

This patch extends the guest domain administration adding support
to automatically advertise the host NUMA node capabilities obtained
architecture under a guest by creating a vNUMA copy.

The mechanism is enabled by setting the check='numa' attribute under
the CPU 'host-passthrough' topology:
   

When enabled the mechanism automatically renders the host capabilities
provided NUMA architecture, evenly balances the guest reserved vcpu
and memory amongst its vNUMA composed cells and have the cell allocated
vcpus pinned towards the host NUMA node physical cpusets.  This in such
way that the host NUMA topology is still in effect under the partitioned
guest domain.

Below example auto partitions the host 'lscpu' listed physical NUMA detail
under a guest domain vNUMA description.

[root@host ]# lscpu 
Architecture:  x86_64
CPU op-mode(s):32-bit, 64-bit
Byte Order:Little Endian
CPU(s):240
On-line CPU(s) list:   0-239
Thread(s) per core:2
Core(s) per socket:15
Socket(s): 8
NUMA node(s):  8
Vendor ID: GenuineIntel
CPU family:6
Model: 62
Model name:Intel(R) Xeon(R) CPU E7-8895 v2 @ 2.80GHz
Stepping:  7
CPU MHz:   3449.555
CPU max MHz:   3600.
CPU min MHz:   1200.
BogoMIPS:  5586.28
Virtualization:VT-x
L1d cache: 32K
L1i cache: 32K
L2 cache:  256K
L3 cache:  38400K
NUMA node0 CPU(s): 0-14,120-134
NUMA node1 CPU(s): 15-29,135-149
NUMA node2 CPU(s): 30-44,150-164
NUMA node3 CPU(s): 45-59,165-179
NUMA node4 CPU(s): 60-74,180-194
NUMA node5 CPU(s): 75-89,195-209
NUMA node6 CPU(s): 90-104,210-224
NUMA node7 CPU(s): 105-119,225-239
Flags: ...

The guest 'anuma' without the auto partition rendering enabled
reads;   ""


  anuma
  3f439f5f-1156-4d48-9491-945a2c0abc6d
  67108864
  67108864
  16
  
hvm

  
  



  
  
  



  
  destroy
  restart
  destroy
  


  
  
/usr/bin/qemu-system-x86_64

  
  

Enabling the auto partitioning the guest 'anuma' XML is rewritten
as listed below;   ""


  anuma
  3f439f5f-1156-4d48-9491-945a2c0abc6d
  67108864
  67108864
  16
  
















  
  
hvm

  
  



  
  


  

  
  
  
  
  
  
  
  

  
  

  
  
  
  
  
  
  
  

  
  

  
  
  
  
  
  
  
  

  
  

  
  
  
  
  
  
  
  

  
  

  
  
  
  
  
  
  
  

  
  

  
  
  
  
  
  
  
  

  
  

  
  
  
  
  
  
  
  

  
  

  
  
  
  
  
  
  
  

  

  
  



  
  destroy
  restart
  destroy
  


  
  
/usr/bin/qemu-system-x86_64

  
  

Finally the auto partitioned guest anuma 'lscpu' listed virtual vNUMA detail.

[root@anuma ~]# lscpu
Architecture:x86_64
CPU op-mode(s):  32-bit, 64-bit
Byte Order:  Little Endian
CPU(s):  16
On-line CPU(s) list: 0-15
Thread(s) per core:  2
Core(s) per socket:  1
Socket(s):   8
NUMA node(s):8
Vendor ID:   GenuineIntel
CPU family:  6
Model:   62
Model name:  Intel(R) Xeon(R) CPU E7-8895 v2 @ 2.80GHz
Stepping: 

Re: [libvirt] [PATCH 31/47] vircgroup: extract virCgroupV1GetMemoryUsage

2018-09-25 Thread Ján Tomko

On Tue, Sep 18, 2018 at 05:45:52PM +0200, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
src/util/vircgroup.c|  9 +
src/util/vircgroupbackend.h |  5 +
src/util/vircgroupv1.c  | 16 
3 files changed, 22 insertions(+), 8 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 30/47] vircgroup: extract virCgroupV1GetMemoryStat

2018-09-25 Thread Ján Tomko

On Tue, Sep 18, 2018 at 05:45:51PM +0200, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
src/util/vircgroup.c| 67 +++---
src/util/vircgroupbackend.h | 10 ++
src/util/vircgroupv1.c  | 71 +
3 files changed, 85 insertions(+), 63 deletions(-)

@@ -246,6 +255,7 @@ struct _virCgroupBackend {
virCgroupGetBlkioDeviceWriteBpsCB getBlkioDeviceWriteBps;

virCgroupSetMemoryCB setMemory;
+virCgroupGetMemoryStatCB getMemoryStat;
};
typedef struct _virCgroupBackend virCgroupBackend;
typedef virCgroupBackend *virCgroupBackendPtr;
diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c
index 3235ef3d2d..cfe29649fb 100644
--- a/src/util/vircgroupv1.c
+++ b/src/util/vircgroupv1.c
@@ -1403,6 +1403,76 @@ virCgroupV1SetMemory(virCgroupPtr group,
}


+static int
+virCgroupV1GetMemoryStat(virCgroupPtr group,
+ unsigned long long *cache,
+ unsigned long long *activeAnon,
+ unsigned long long *inactiveAnon,
+ unsigned long long *activeFile,
+ unsigned long long *inactiveFile,
+ unsigned long long *unevictable)
+{


Original:

-int ret = -1;
-char *stat = NULL;

New:

+VIR_AUTOFREE(char *) stat = NULL;



+char *line = NULL;
+unsigned long long cacheVal = 0;
+unsigned long long activeAnonVal = 0;
+unsigned long long inactiveAnonVal = 0;
+unsigned long long activeFileVal = 0;
+unsigned long long inactiveFileVal = 0;
+unsigned long long unevictableVal = 0;
+


With the VIR_AUTOFREE conversion separated:
Reviewed-by: Ján Tomko 

Jano


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

[libvirt] [PATCH v3] qemu: Remove network type limitation for qemuARPGetInterfaces

2018-09-25 Thread Lin Ma
Let's ignore the checking of interface type when we call the function
qemuARPGetInterfaces to get IP from host's arp table.

Based on suggestion from Laine.
https://www.redhat.com/archives/libvir-list/2018-September/msg00684.html

Signed-off-by: Lin Ma 
---
 src/qemu/qemu_driver.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 10d6bca186..3110e74e0e 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -20945,9 +20945,6 @@ qemuARPGetInterfaces(virDomainObjPtr vm,
 goto cleanup;
 
 for (i = 0; i < vm->def->nnets; i++) {
-if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK)
-continue;
-
 virMacAddrFormat(&(vm->def->nets[i]->mac), macaddr);
 for (j = 0; j < table->n; j++) {
 virArpTableEntry entry = table->t[j];
-- 
2.19.0

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


Re: [libvirt] [PATCH v5 07/13] conf: Introduce parser, formatter for uid and fid

2018-09-25 Thread Andrea Bolognani
On Tue, 2018-09-25 at 17:16 +0800, Yi Min Zhao wrote:
> > > So do you mean we use this kind of API in next version for review?
> > 
> > Yes, please :)
> 
> I'm not sure how big the change is. So I might take more time
> to prepare the new version. So that we can't catch up with
> the new release. Thanks for your kindness and comments!

I think it's going to be a fairly sizeable change, but as we're
getting awfully close to freeze anyway I would have advised against
merging zPCI support this late in the cycle anyway, so timing-wise
I'd say it probably won't change much.

-- 
Andrea Bolognani / Red Hat / Virtualization

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


Re: [libvirt] [PATCH] lxc_monitor: Avoid AB / BA lock race

2018-09-25 Thread Michal Privoznik
On 09/24/2018 05:11 PM, Mark Asselstine wrote:
> A deadlock situation can occur when autostarting a LXC domain 'guest'
> due to two threads attempting to take opposing locks while holding
> opposing locks (AB BA problem). Thread A takes and holds the 'vm' lock
> while attempting to take the 'client' lock, meanwhile, thread B takes
> and holds the 'client' lock while attempting to take the 'vm' lock.
> 
> The potential for this can be seen as follows:
> 
> Thread A:
> virLXCProcessAutostartDomain (takes vm lock)
>  --> virLXCProcessStart
>   --> virLXCProcessConnectMonitor
>--> virLXCMonitorNew
> --> virNetClientSetCloseCallback (wants client lock)
> 
> Thread B:
> virNetClientIncomingEvent (takes client lock)
>  --> virNetClientIOHandleInput
>   --> virNetClientCallDispatch
>--> virNetClientCallDispatchMessage
> --> virNetClientProgramDispatch
>  --> virLXCMonitorHandleEventInit
>   --> virLXCProcessMonitorInitNotify (wants vm lock)
> 
> Since these threads are scheduled independently and are preemptible it
> is possible for the deadlock scenario to occur where each thread locks
> their first lock but both will fail to get their second lock and just
> spin forever. You get something like:
> 
> virLXCProcessAutostartDomain (takes vm lock)
>  --> virLXCProcessStart
>   --> virLXCProcessConnectMonitor
>--> virLXCMonitorNew
> <...>
> virNetClientIncomingEvent (takes client lock)
>  --> virNetClientIOHandleInput
>   --> virNetClientCallDispatch
>--> virNetClientCallDispatchMessage
> --> virNetClientProgramDispatch
>  --> virLXCMonitorHandleEventInit
>   --> virLXCProcessMonitorInitNotify (wants vm lock but spins)
> <...>
> --> virNetClientSetCloseCallback (wants client lock but spins)
> 
> Neither thread ever gets the lock it needs to be able to continue
> while holding the lock that the other thread needs.
> 
> The actual window for preemption which can cause this deadlock is
> rather small, between the calls to virNetClientProgramNew() and
> execution of virNetClientSetCloseCallback(), both in
> virLXCMonitorNew(). But it can be seen in real world use that this
> small window is enough.
> 
> By moving the call to virNetClientSetCloseCallback() ahead of
> virNetClientProgramNew() we can close any possible chance of the
> deadlock taking place. There should be no other implications to the
> move since the close callback (in the unlikely event was called) will
> spin on the vm lock. The remaining work that takes place between the
> old call location of virNetClientSetCloseCallback() and the new
> location is unaffected by the move.
> 
> Signed-off-by: Mark Asselstine 
> ---
> 
> NOTE I am not intimately familiar with the libvirt code, so although I
> am confident of the cause of the issue I am not sure this is the best
> solution. So I am really curious what people who do know this code
> think and as such I am definitely open to discussions and suggestions
> on how to rework things to properly address the issue.
> 
>  src/lxc/lxc_monitor.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/lxc/lxc_monitor.c b/src/lxc/lxc_monitor.c
> index e765c16..218f426 100644
> --- a/src/lxc/lxc_monitor.c
> +++ b/src/lxc/lxc_monitor.c
> @@ -161,6 +161,10 @@ virLXCMonitorPtr virLXCMonitorNew(virDomainObjPtr vm,
>  if (virNetClientRegisterAsyncIO(mon->client) < 0)
>  goto error;
>  
> +/* avoid deadlock by making this call before assigning 
> virLXCMonitorEvents */
> +virNetClientSetCloseCallback(mon->client, virLXCMonitorEOFNotify, mon,
> + virLXCMonitorCloseFreeCallback);
> +
>  if (!(mon->program = virNetClientProgramNew(VIR_LXC_MONITOR_PROGRAM,
>  
> VIR_LXC_MONITOR_PROGRAM_VERSION,
>  virLXCMonitorEvents,
> @@ -176,8 +180,6 @@ virLXCMonitorPtr virLXCMonitorNew(virDomainObjPtr vm,
>  memcpy(>cb, cb, sizeof(mon->cb));
>  
>  virObjectRef(mon);

This ref ^^

> -virNetClientSetCloseCallback(mon->client, virLXCMonitorEOFNotify, mon,
> - virLXCMonitorCloseFreeCallback);
>  
>   cleanup:
>  VIR_FREE(sockpath);
> 

isn't it needed because of this SetCloseCallback, or actually
virLXCMonitorCloseFreeCallback()? The extra reference is held around so
that virLXCMonitorEOFNotify can use it and it's then released in
virLXCMonitorCloseFreeCallback(). If you move the SetCloseCallback()
call and for instance virNetClientProgramNew() fails, then I think we
are facing a double free: one free() is right under error label, the
other is when the connection closes.

Therefore I think we have to move the ref call too. No need to post v2,
I am just checking with you. I can fix it before pushing.

Michal

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


Re: [libvirt] [PATCH v5 07/13] conf: Introduce parser, formatter for uid and fid

2018-09-25 Thread Yi Min Zhao



在 2018/9/25 下午4:02, Andrea Bolognani 写道:

On Tue, 2018-09-25 at 13:15 +0800, Yi Min Zhao wrote:

  From the high level point of view, code outside of conf/ should,
for the most part, not need to concern itself with zPCI at all: it
would eg. ask for a PCI address to be allocated, and if the device
in question can be a zPCI device then a zPCI extension address will
be allocated for it as part of the same function call; the only
part of qemu/ that should care about the zPCI address is the one
generating the relevant command line arguments.

Can you try and see whether this kind of API would work?

I did a simple test. It worked. Do you prefer this way?

Yes please, I'd very much like to see what that looks like and
whether it addresses the problems caused by the ambiguity of the
API we've used until now.

So do you mean we use this kind of API in next version for review?

Yes, please :)


I'm not sure how big the change is. So I might take more time
to prepare the new version. So that we can't catch up with
the new release. Thanks for your kindness and comments!

--
Yi Min

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

Re: [libvirt] [PATCH 29/47] vircgroup: extract virCgroupV1SetMemory

2018-09-25 Thread Ján Tomko

On Tue, Sep 18, 2018 at 05:45:50PM +0200, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
src/util/vircgroup.c| 20 +---
src/util/vircgroupbackend.h |  6 ++
src/util/vircgroupv1.c  | 28 
3 files changed, 35 insertions(+), 19 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 28/47] vircgroup: extract virCgroupV1(Set|Get)BlkioDeviceWriteBps

2018-09-25 Thread Ján Tomko

On Tue, Sep 18, 2018 at 05:45:49PM +0200, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
src/util/vircgroup.c| 34 ++---
src/util/vircgroupbackend.h | 12 +
src/util/vircgroupv1.c  | 50 +
3 files changed, 64 insertions(+), 32 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 27/47] vircgroup: extract virCgroupV1(Set|Get)BlkioDeviceReadBps

2018-09-25 Thread Ján Tomko

On Tue, Sep 18, 2018 at 05:45:48PM +0200, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
src/util/vircgroup.c| 34 ++---
src/util/vircgroupbackend.h | 12 +
src/util/vircgroupv1.c  | 50 +
3 files changed, 64 insertions(+), 32 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 26/47] vircgroup: extract virCgroupV1(Set|Get)BlkioDeviceWriteIops

2018-09-25 Thread Ján Tomko

On Tue, Sep 18, 2018 at 05:45:47PM +0200, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
src/util/vircgroup.c| 34 ++---
src/util/vircgroupbackend.h | 12 +
src/util/vircgroupv1.c  | 50 +
3 files changed, 64 insertions(+), 32 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 25/47] vircgroup: extract virCgroupV1(Set|Get)BlkioDeviceReadIops

2018-09-25 Thread Ján Tomko

On Tue, Sep 18, 2018 at 05:45:46PM +0200, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
src/util/vircgroup.c| 34 ++---
src/util/vircgroupbackend.h | 12 +
src/util/vircgroupv1.c  | 50 +
3 files changed, 64 insertions(+), 32 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 24/47] vircgroup: extract virCgroupV1(Set|Get)BlkioDeviceWeight

2018-09-25 Thread Ján Tomko

On Tue, Sep 18, 2018 at 05:45:45PM +0200, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
src/util/vircgroup.c| 36 +++---
src/util/vircgroupbackend.h | 12 +
src/util/vircgrouppriv.h|  6 +
src/util/vircgroupv1.c  | 50 +
4 files changed, 71 insertions(+), 33 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 23/47] vircgroup: extract virCgroupV1GetBlkioIoDeviceServiced

2018-09-25 Thread Ján Tomko

On Tue, Sep 18, 2018 at 05:45:44PM +0200, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
src/util/vircgroup.c| 83 ++
src/util/vircgroupbackend.h |  9 
src/util/vircgrouppriv.h|  2 +
src/util/vircgroupv1.c  | 89 +
4 files changed, 104 insertions(+), 79 deletions(-)




diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c
index 4b9c03830c..206318d436 100644
--- a/src/util/vircgroupv1.c
+++ b/src/util/vircgroupv1.c
@@ -1049,6 +1049,94 @@ virCgroupV1GetBlkioIoServiced(virCgroupPtr group,
}


+static int
+virCgroupV1GetBlkioIoDeviceServiced(virCgroupPtr group,
+const char *path,
+long long *bytes_read,
+long long *bytes_write,
+long long *requests_read,
+long long *requests_write)
+{
+VIR_AUTOFREE(char *) str1 = NULL;
+VIR_AUTOFREE(char *) str2 = NULL;
+VIR_AUTOFREE(char *) str3 = NULL;


Just like in the last patch. Instead of:

+char *p1, *p2;


use:


-char *p1 = NULL;
-char *p2 = NULL;



+size_t i;
+
+const char *value_names[] = {
+"Read ",
+"Write "
+};


Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 22/47] vircgroup: extract virCgroupV1GetBlkioIoServiced

2018-09-25 Thread Ján Tomko

On Tue, Sep 18, 2018 at 05:45:43PM +0200, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
src/util/vircgroup.c| 87 ++
src/util/vircgroupbackend.h |  8 
src/util/vircgroupv1.c  | 94 +
3 files changed, 105 insertions(+), 84 deletions(-)

diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c
index 380a310589..4b9c03830c 100644
--- a/src/util/vircgroupv1.c
+++ b/src/util/vircgroupv1.c
@@ -956,6 +956,99 @@ virCgroupV1GetBlkioWeight(virCgroupPtr group,
}


+static int
+virCgroupV1GetBlkioIoServiced(virCgroupPtr group,
+  long long *bytes_read,
+  long long *bytes_write,
+  long long *requests_read,
+  long long *requests_write)
+{
+long long stats_val;
+VIR_AUTOFREE(char *) str1 = NULL;
+VIR_AUTOFREE(char *) str2 = NULL;



+char *p1, *p2;


These were written as:

-char *p1 = NULL;
-char *p2 = NULL;

in the original function.
Perhaps incorrect conflict resolution with commit 94f1855f
   util: cgroup: use VIR_AUTOFREE instead of VIR_FREE for scalar types


+size_t i;
+
+const char *value_names[] = {
+"Read ",
+"Write "
+};


With that addressed:
Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v5 07/13] conf: Introduce parser, formatter for uid and fid

2018-09-25 Thread Andrea Bolognani
On Tue, 2018-09-25 at 13:15 +0800, Yi Min Zhao wrote:
> > > >  From the high level point of view, code outside of conf/ should,
> > > > for the most part, not need to concern itself with zPCI at all: it
> > > > would eg. ask for a PCI address to be allocated, and if the device
> > > > in question can be a zPCI device then a zPCI extension address will
> > > > be allocated for it as part of the same function call; the only
> > > > part of qemu/ that should care about the zPCI address is the one
> > > > generating the relevant command line arguments.
> > > > 
> > > > Can you try and see whether this kind of API would work?
> > > 
> > > I did a simple test. It worked. Do you prefer this way?
> > 
> > Yes please, I'd very much like to see what that looks like and
> > whether it addresses the problems caused by the ambiguity of the
> > API we've used until now.
> 
> So do you mean we use this kind of API in next version for review?

Yes, please :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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


Re: [libvirt] [PATCH 21/47] vircgroup: extract virCgroupV1(Set|Get)BlkioWeight

2018-09-25 Thread Ján Tomko

On Tue, Sep 18, 2018 at 05:45:42PM +0200, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
src/util/vircgroup.c| 14 ++
src/util/vircgroupbackend.h | 20 
src/util/vircgroupv1.c  | 29 +
3 files changed, 51 insertions(+), 12 deletions(-)

@@ -161,4 +173,12 @@ virCgroupBackendRegister(virCgroupBackendPtr backend);
virCgroupBackendPtr *
virCgroupBackendGetAll(void);

+# define VIR_CGROUP_BACKEND_CALL(group, func, ret, ...) \
+if (!group->backend->func) { \
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", \
+   _("operation '" #func "' not supported")); \


_("operation '%s' not supported"), #func)

I don't think the function names are translatable. And having a separate
translatable string for each unlikely error message is a waste of
translators' time.


+return ret; \
+} \
+return group->backend->func(group, ##__VA_ARGS__);
+
#endif /* __VIR_CGROUP_BACKEND_H__ */


Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] Plans for next release

2018-09-25 Thread Michal Privoznik
On 09/24/2018 03:42 PM, Daniel Veillard wrote:
>   that's next week ! If we want to release around Oct 1st, I would
> suggest to enter freeze this Wed, probably in the morning europe time,
> then plan for an RC2 friday morning and if everything goes well, have
> the final release next monday.
> 
>   I hope that works for everybody !

Well, I introduced metadata locking in this release and as usual there
are some follow up patches waiting for review:

https://www.redhat.com/archives/libvir-list/2018-September/msg01134.html

Also, Bjoern reported some issues with the feature (/me wonders if it's
s390 specific) and he said he'll debug this week. Anyway, those patches
and any patch that he'll write can be viewed as a bug fix and therefore
can be merged during freeze.

Michal

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


[libvirt] [PATCH 5/4] security: Don't try to lock NULL paths

2018-09-25 Thread Michal Privoznik
It may happen that in the list of paths/disk sources to relabel
there is a disk source. If that is the case, the path is NULL. In
that case, we shouldn't try to lock the path. It's likely a
network disk anyway and therefore there is nothing to lock.

Signed-off-by: Michal Privoznik 
---
 src/security/security_dac.c | 3 ++-
 src/security/security_selinux.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 876cca0f2f..04168feb3d 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -216,7 +216,8 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
 for (i = 0; i < list->nItems; i++) {
 const char *p = list->items[i]->path;
 
-if (virFileIsDir(p))
+if (!p ||
+virFileIsDir(p))
 continue;
 
 VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p);
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 3c847d8dcb..3adbeada14 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -227,7 +227,8 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
 for (i = 0; i < list->nItems; i++) {
 const char *p = list->items[i]->path;
 
-if (virFileIsDir(p))
+if (!p ||
+virFileIsDir(p))
 continue;
 
 VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p);
-- 
2.16.4

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