Re: [libvirt] [PATCH 00/10] VFIO fixes for PCI devices

[Alex Williamson]

On Wed, 2015-12-09 at 13:51 +0100, Andrea Bolognani wrote:
> On Wed, 2015-12-02 at 18:17 +0100, Andrea Bolognani wrote:
> > This series is my attempt at fixing
> > 
> >   https://bugzilla.redhat.com/show_bug.cgi?id=1272300
> > 
> [...]
> > 
> > The problem being solved is that, when using VFIO, IOMMU group
> > ownership can't be shared, eg. two devices that are in the
> > same IOMMU group can't be assigned to different guests, or to
> > the host and a guest. If that happens, the host will probably
> > crash.
> > 
> > The series deals with this issue by making sure safety
> > conditions are met before detaching devices from the host or
> > reattaching them to the host. In praticular, when we're asked
> > to reattach a device to the host but doing so would lead to
> > sharing IOMMU group ownership, we delay the operation until
> > we can guarantee this will not cause problems. As a nice side
> > effect of the changes we check for this when starting a guest
> > too, instead of assuming it will work and having QEMU error
> > out immediately afterwards.
> 
> Shivaprasad raised a concern on IRC, which I'm sharing here
> for wider discussion. I'm CC'ing Laine and Alex, hopefully
> they don't mind - let me know otherwise.
> 
> Assume we have a PCI device with two functions. With this
> series applied, when reattaching both functions to the host
> this would happen:
> 
>   f0 remove from guest
>   f1 remove from guest
> f0 unbind from vfio-pci
>   f0 trigger host driver reprobe
> f1 unbind from vfio-pci
>   f1 trigger host driver reprobe
> 
> Shivaprasad is concerned this is not actually safe, and the
> proper sequence would rather be:
> 
>   f0 remove from guest
>   f1 remove from guest
> f0 unbind from vfio-pci
> f1 unbind from vfio-pci
>   f0 trigger host driver reprobe
>   f1 trigger host driver reprobe
> 
> Doing so would AFAICT mean basically duplicating the delay
> logic this series adds to virHostdev into virPCI, to ensure
> that devices are unbound from vfio-pci only once the same
> operation has been requested for all devices in the IOMMU
> group, and reprobe is triggered only after all devices have
> been unbound from vfio-pci.
> 
> I was under the impression that what the current series
> does, eg. sharing devices in the same IOMMU group between
> the host driver and vfio-pci is safe as long as no guest is
> using them at the same time, and that devices could be
> safely "moved" between the host driver (eg. in use) and
> vfio-pci (eg. idle, waiting to be assigned to a guest) as
> many times as desired without ill consequences.
> 
> Is my understanding wrong? Do I need to rework the series
> so that unbinds and reprobes are always executed across the
> IOMMU group?
> 
> Any suggestion or pointers to relevant documentation will
> be very much appreciated.

Hi Andrea,

Your understanding is correct, so I think it just comes down to how sure
you are that the vfio group is idle/unused.  If there's any risk that a
device is still in use by QEMU, then we haven't solved the original
problem.  Unbinding isn't the only way to have good confidence of this
though.  You could track the QEMU pid, you could use fuser to make sure
the group is not in use, you could try to open the group yourself to
make sure it's not in use, and of course you can unbind as proposed in
the second option.  So long as you know the group is idle, somehow, it
shouldn't matter what order you unbind and reprobe.  Thanks,

Alex

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

[libvirt] [PATCH 8/8] virsh: extend domstats command

[Qiaowei Ren]

This patch extend domstats command to match extended
virDomainListGetStats API in previous patch.

Signed-off-by: Qiaowei Ren 
---
 tools/virsh-domain-monitor.c | 7 +++
 tools/virsh.pod  | 7 +--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index 7dcf833..645936c 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -2017,6 +2017,10 @@ static const vshCmdOptDef opts_domstats[] = {
  .type = VSH_OT_BOOL,
  .help = N_("report domain block device statistics"),
 },
+{.name = "perf",
+ .type = VSH_OT_BOOL,
+ .help = N_("report domain perf event statistics"),
+},
 {.name = "list-active",
  .type = VSH_OT_BOOL,
  .help = N_("list only active domains"),
@@ -2128,6 +2132,9 @@ cmdDomstats(vshControl *ctl, const vshCmd *cmd)
 if (vshCommandOptBool(cmd, "block"))
 stats |= VIR_DOMAIN_STATS_BLOCK;
 
+if (vshCommandOptBool(cmd, "perf"))
+stats |= VIR_DOMAIN_STATS_PERF;
+
 if (vshCommandOptBool(cmd, "list-active"))
 flags |= VIR_CONNECT_GET_ALL_DOMAINS_STATS_ACTIVE;
 
diff --git a/tools/virsh.pod b/tools/virsh.pod
index bfc7e63..89c5405 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -846,7 +846,7 @@ or unique source names printed by this command.
 
 =item B [I<--raw>] [I<--enforce>] [I<--backing>] [I<--state>]
 [I<--cpu-total>] [I<--balloon>] [I<--vcpu>] [I<--interface>] [I<--block>]
-[[I<--list-active>] [I<--list-inactive>] [I<--list-persistent>]
+[I<--perf>] [[I<--list-active>] [I<--list-inactive>] [I<--list-persistent>]
 [I<--list-transient>] [I<--list-running>] [I<--list-paused>]
 [I<--list-shutoff>] [I<--list-other>]] | [I ...]
 
@@ -864,7 +864,7 @@ behavior use the I<--raw> flag.
 The individual statistics groups are selectable via specific flags. By
 default all supported statistics groups are returned. Supported
 statistics groups flags are: I<--state>, I<--cpu-total>, I<--balloon>,
-I<--vcpu>, I<--interface>, I<--block>.
+I<--vcpu>, I<--interface>, I<--block>, I<--perf>.
 
 When selecting the I<--state> group the following fields are returned:
 "state.state" - state of the VM, returned as number from virDomainState enum,
@@ -899,6 +899,9 @@ I<--interface> returns:
 "net..tx.errs" - number of transmission errors,
 "net..tx.drop" - number of transmit packets dropped
 
+I<--perf> returns the statistics of all enabled perf events:
+"perf.cache" - the cache usage in Byte currently used
+
 I<--block> returns information about disks associated with each
 domain.  Using the I<--backing> flag extends this information to
 cover all resources in the backing chain, rather than the default
-- 
1.9.1

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

[libvirt] [PATCH LIBVIRT] libxl: Use libxentoollog in preference to libxenctrl if available.

[Ian Campbell]

Upstream Xen is in the process of splitting the (stable API) xtl_*
interfaces out from the (unstable API) libxenctrl library and into a
new (stable API) libxentoollog.

In order to be compatible with Xen both before and after this
transition check for xtl_createlogger_stdiostream in a libxentoollog
library and use it if present. If it is not present assume it is in
libxenctrl.

Compile tested on Xen 4.6 and a development tree with the split in
place.

Signed-off-by: Ian Campbell 
---
I'm waiting on applying the upstream change until downstreams are
prepared for this. The latest upstream patch is
http://lists.xen.org/archives/html/xen-devel/2015-12/msg00454.html
which had to be reverted because I had somehow not properly checked if
libvirt used this interface
http://lists.xen.org/archives/html/xen-devel/2015-12/msg01153.html

It might be nice to get this into 1.3.0 so that supports Xen 4.7 out
of the box? Not sure what the libvirt stable backport policy is but it
might also be good to eventually consider it for that?
---
 configure.ac | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 98cf210..b641cc7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -883,7 +883,6 @@ if test "$with_libxl" != "no" ; then
 PKG_CHECK_MODULES([LIBXL], [xenlight], [
  LIBXL_FIRMWARE_DIR=`$PKG_CONFIG --variable xenfirmwaredir xenlight`
  LIBXL_EXECBIN_DIR=`$PKG_CONFIG --variable libexec_bin xenlight`
- LIBXL_LIBS="$LIBXL_LIBS -lxenctrl"
  with_libxl=yes
 ], [LIBXL_FOUND=no])
 if test "$LIBXL_FOUND" = "no"; then
@@ -896,7 +895,7 @@ if test "$with_libxl" != "no" ; then
 LIBS="$LIBS $LIBXL_LIBS"
 AC_CHECK_LIB([xenlight], [libxl_ctx_alloc], [
 with_libxl=yes
-LIBXL_LIBS="$LIBXL_LIBS -lxenlight -lxenctrl"
+LIBXL_LIBS="$LIBXL_LIBS -lxenlight"
 ],[
 if test "$with_libxl" = "yes"; then
 fail=1
@@ -924,6 +923,14 @@ if test "$with_libxl" = "yes"; then
 if test "x$LIBXL_EXECBIN_DIR" != "x"; then
 AC_DEFINE_UNQUOTED([LIBXL_EXECBIN_DIR], ["$LIBXL_EXECBIN_DIR"], 
[directory containing Xen libexec binaries])
 fi
+dnl Check if the xtl_* infrastructure is in libxentoollog
+dnl (since Xen 4.7) if not then assume it is in libxenctrl
+dnl (as it was for 4.6 and earler)
+AC_CHECK_LIB([xentoollog], [xtl_createlogger_stdiostream], [
+LIBXL_LIBS="$LIBXL_LIBS -lxentoollog"
+],[
+LIBXL_LIBS="$LIBXL_LIBS -lxenctrl"
+])
 fi
 AM_CONDITIONAL([WITH_LIBXL], [test "$with_libxl" = "yes"])
 
-- 
2.1.4

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

[libvirt] [PATCH 6/8] perf: reenable perf events when libvirtd restart

[Qiaowei Ren]

When libvirtd daemon restart, this patch will reenable those perf
events previously enabled.

Signed-off-by: Qiaowei Ren 
---
 src/qemu/qemu_driver.c | 36 
 1 file changed, 36 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7d95364..09607ca 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -164,6 +164,9 @@ static int qemuDomainGetMaxVcpus(virDomainPtr dom);
 static int qemuDomainManagedSaveLoad(virDomainObjPtr vm,
  void *opaque);
 
+static int qemuDomainPerfRestart(virDomainObjPtr vm,
+ void *data);
+
 static int qemuOpenFile(virQEMUDriverPtr driver,
 virDomainObjPtr vm,
 const char *path, int oflags,
@@ -939,6 +942,10 @@ qemuStateInitialize(bool privileged,
 qemuDomainManagedSaveLoad,
 qemu_driver);
 
+virDomainObjListForEach(qemu_driver->domains,
+qemuDomainPerfRestart,
+NULL);
+
 qemuProcessReconnectAll(conn, qemu_driver);
 
 qemu_driver->workerPool = virThreadPoolNew(0, 1, 0, 
qemuProcessEventHandler, qemu_driver);
@@ -3460,6 +3467,35 @@ qemuDomainManagedSaveLoad(virDomainObjPtr vm,
 return ret;
 }
 
+static int
+qemuDomainPerfRestart(virDomainObjPtr vm,
+  void *data ATTRIBUTE_UNUSED)
+{
+size_t i;
+virDomainDefPtr def = vm->def;
+qemuDomainObjPrivatePtr priv = vm->privateData;
+
+virPerfFree(priv->perf);
+
+priv->perf = virPerfNew();
+if (!priv->perf)
+return -1;
+
+for (i = 0; i < VIR_PERF_EVENT_LAST; i++) {
+if (def->perf->events[i] &&
+def->perf->events[i] == VIR_TRISTATE_BOOL_YES) {
+if (virPerfEventEnable(priv->perf, i, vm->pid))
+goto cleanup;
+}
+}
+
+return 0;
+
+ cleanup:
+virPerfFree(priv->perf);
+return -1;
+}
+
 
 static int
 qemuDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags)
-- 
1.9.1

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

[libvirt] [PATCH 7/8] virsh: implement new command to support perf

[Qiaowei Ren]

This patch add new perf command to enable/disable perf event
for a guest domain.

Signed-off-by: Qiaowei Ren 
---
 tools/virsh-domain.c | 128 +++
 tools/virsh.pod  |  20 
 2 files changed, 148 insertions(+)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index b7e7606..cef68f5 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -,6 +,128 @@ cmdMemtune(vshControl *ctl, const vshCmd *cmd)
 }
 
 /*
+ * "perf" command
+ */
+static const vshCmdInfo info_perf[] = {
+{.name = "help",
+.data = N_("Get or set perf event")
+},
+{.name = "desc",
+.data = N_("Get or set the current perf events for a guest"
+   " domain.\n"
+   "To get the perf events list use following command: 
\n\n"
+   "virsh # perf ")
+},
+{.name = NULL}
+};
+
+static const vshCmdOptDef opts_perf[] = {
+{.name = "domain",
+ .type = VSH_OT_DATA,
+ .flags = VSH_OFLAG_REQ,
+ .help = N_("domain name, id or uuid")
+},
+{.name = "enable",
+ .type = VSH_OT_STRING,
+ .help = N_("perf events which will be enabled")
+},
+{.name = "disable",
+ .type = VSH_OT_STRING,
+ .help = N_("perf events which will be disabled")
+},
+{.name = NULL}
+};
+
+static int
+virshParseEventStr(vshControl *ctl,
+   const char *event,
+   bool state,
+   virTypedParameterPtr *params,
+   int *nparams,
+   int *maxparams)
+{
+char **tok = NULL;
+size_t i, ntok;
+int ret = -1;
+
+if (!(tok = virStringSplitCount(event, "|", 0, )))
+return -1;
+
+if (ntok > VIR_PERF_EVENT_LAST) {
+vshError(ctl, _("event string '%s' has too many fields"), event);
+goto cleanup;
+}
+
+for(i = 0; i < ntok; i++) {
+if ((*tok[i] != '\0') &&
+virTypedParamsAddBoolean(params, nparams,
+ maxparams, tok[i], state) < 0)
+goto cleanup;
+}
+
+ret = 0;
+ cleanup:
+virStringFreeList(tok);
+return ret;
+}
+
+static bool
+cmdPerf(vshControl *ctl, const vshCmd *cmd)
+{
+virDomainPtr dom;
+int nparams = 0;
+int maxparams = 0;
+size_t i;
+virTypedParameterPtr params = NULL;
+bool ret = false;
+const char *enable = NULL, *disable = NULL;
+
+if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
+return false;
+
+if (vshCommandOptStringReq(ctl, cmd, "enable", ) < 0 ||
+vshCommandOptStringReq(ctl, cmd, "disable", ) < 0)
+return false;
+
+if (enable && virshParseEventStr(ctl, enable, true,
+ , , ) < 0)
+goto cleanup;
+
+if (disable && virshParseEventStr(ctl, disable, false,
+  , , ) < 0)
+   goto cleanup;
+
+if (nparams == 0) {
+if (virDomainGetPerfEvents(dom, , ) != 0) {
+vshError(ctl, "%s", _("Unable to get perf events"));
+goto cleanup;
+}
+for (i = 0; i < nparams; i++) {
+if (params[i].type == VIR_TYPED_PARAM_BOOLEAN &&
+params[i].value.b) {
+vshPrint(ctl, "%-15s: %s\n", params[i].field, _("enabled"));
+} else {
+vshPrint(ctl, "%-15s: %s\n", params[i].field, _("disabled"));
+}
+}
+} else {
+if (virDomainSetPerfEvents(dom, params, nparams) != 0)
+goto error;
+}
+
+ret = true;
+ cleanup:
+virTypedParamsFree(params, nparams);
+virDomainFree(dom);
+return ret;
+
+ error:
+vshError(ctl, "%s", _("Unable to enable/disable perf events"));
+goto cleanup;
+}
+
+
+/*
  * "numatune" command
  */
 static const vshCmdInfo info_numatune[] = {
@@ -13406,6 +13528,12 @@ const vshCmdDef domManagementCmds[] = {
  .info = info_memtune,
  .flags = 0
 },
+{.name = "perf",
+ .handler = cmdPerf,
+ .opts = opts_perf,
+ .info = info_perf,
+ .flags = 0
+},
 {.name = "metadata",
  .handler = cmdMetadata,
  .opts = opts_metadata,
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 21ae4a3..bfc7e63 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -2113,6 +2113,26 @@ The guaranteed minimum memory allocation for the guest.
 
 Specifying -1 as a value for these limits is interpreted as unlimited.
 
+=item B I [I<--enable> B]
+[I<--disable> B]
+
+Get the current perf events setting or enable/disable specific perf
+event for a guest domain.
+
+Perf is a performance analyzing tool in Linux, and it can instrument
+CPU performance counters, tracepoints, kprobes, and uprobes (dynamic
+tracing). Perf supports a list of measurable events, and can measure
+events coming from different sources. For instance, some event are
+pure kernel counters, in this case they are called software events,
+including 

[libvirt] [PATCH 2/8] perf: implement the remote protocol for perf event

[Qiaowei Ren]

Add remote support for perf event.

Signed-off-by: Qiaowei Ren 
---
 daemon/remote.c  | 47 
 src/remote/remote_driver.c   | 39 
 src/remote/remote_protocol.x | 30 +++-
 src/remote_protocol-structs  | 18 +
 4 files changed, 133 insertions(+), 1 deletion(-)

diff --git a/daemon/remote.c b/daemon/remote.c
index 3a3eb09..9df29ef 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -2810,6 +2810,53 @@ remoteDispatchNodeGetMemoryStats(virNetServerPtr server 
ATTRIBUTE_UNUSED,
 }
 
 static int
+remoteDispatchDomainGetPerfEvents(virNetServerPtr server ATTRIBUTE_UNUSED,
+  virNetServerClientPtr client 
ATTRIBUTE_UNUSED,
+  virNetMessagePtr msg ATTRIBUTE_UNUSED,
+  virNetMessageErrorPtr rerr,
+  remote_domain_get_perf_events_args *args,
+  remote_domain_get_perf_events_ret *ret)
+{
+virDomainPtr dom = NULL;
+virTypedParameterPtr params = NULL;
+int nparams = 0;
+int rv = -1;
+struct daemonClientPrivate *priv =
+virNetServerClientGetPrivateData(client);
+
+if (!priv->conn) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
+goto cleanup;
+}
+
+if (!(dom = get_nonnull_domain(priv->conn, args->dom)))
+goto cleanup;
+
+if (virDomainGetPerfEvents(dom, , ) < 0)
+goto cleanup;
+
+if (nparams > REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large"));
+goto cleanup;
+}
+
+if (remoteSerializeTypedParameters(params, nparams,
+   >params.params_val,
+   >params.params_len,
+   0) < 0)
+goto cleanup;
+
+rv = 0;
+
+ cleanup:
+if (rv < 0)
+virNetMessageSaveError(rerr);
+virTypedParamsFree(params, nparams);
+virObjectUnref(dom);
+return rv;
+}
+
+static int
 remoteDispatchDomainGetBlockJobInfo(virNetServerPtr server ATTRIBUTE_UNUSED,
 virNetServerClientPtr client 
ATTRIBUTE_UNUSED,
 virNetMessagePtr msg ATTRIBUTE_UNUSED,
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index a1dd640..377f356 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -2028,6 +2028,43 @@ remoteDomainGetNumaParameters(virDomainPtr domain,
 }
 
 static int
+remoteDomainGetPerfEvents(virDomainPtr domain,
+  virTypedParameterPtr *params,
+  int *nparams)
+{
+int rv = -1;
+remote_domain_get_perf_events_args args;
+remote_domain_get_perf_events_ret ret;
+struct private_data *priv = domain->conn->privateData;
+
+remoteDriverLock(priv);
+
+make_nonnull_domain(, domain);
+
+memset(, 0, sizeof(ret));
+if (call(domain->conn, priv, 0, REMOTE_PROC_DOMAIN_GET_PERF_EVENTS,
+ (xdrproc_t) xdr_remote_domain_get_perf_events_args, (char *) 
,
+ (xdrproc_t) xdr_remote_domain_get_perf_events_ret, (char *) ) 
== -1)
+goto done;
+
+if (remoteDeserializeTypedParameters(ret.params.params_val,
+ ret.params.params_len,
+ REMOTE_DOMAIN_PERF_EVENTS_MAX,
+ params,
+ nparams) < 0)
+goto cleanup;
+
+rv = 0;
+
+ cleanup:
+xdr_free((xdrproc_t) xdr_remote_domain_get_perf_events_ret,
+ (char *) );
+ done:
+remoteDriverUnlock(priv);
+return rv;
+}
+
+static int
 remoteDomainGetBlkioParameters(virDomainPtr domain,
virTypedParameterPtr params, int *nparams,
unsigned int flags)
@@ -8253,6 +8290,8 @@ static virHypervisorDriver hypervisor_driver = {
 .domainGetMemoryParameters = remoteDomainGetMemoryParameters, /* 0.8.5 */
 .domainSetBlkioParameters = remoteDomainSetBlkioParameters, /* 0.9.0 */
 .domainGetBlkioParameters = remoteDomainGetBlkioParameters, /* 0.9.0 */
+.domainGetPerfEvents = remoteDomainGetPerfEvents, /* 1.3.1 */
+.domainSetPerfEvents = remoteDomainSetPerfEvents, /* 1.3.1 */
 .domainGetInfo = remoteDomainGetInfo, /* 0.3.0 */
 .domainGetState = remoteDomainGetState, /* 0.9.2 */
 .domainGetControlInfo = remoteDomainGetControlInfo, /* 0.9.3 */
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
index 80f4a8b..3039958 100644
--- a/src/remote/remote_protocol.x
+++ b/src/remote/remote_protocol.x
@@ -127,6 +127,9 @@ const REMOTE_DOMAIN_BLOCK_IO_TUNE_PARAMETERS_MAX = 16;
 /* Upper limit on list of numa parameters. */
 const 

[libvirt] [PATCH 3/8] perf: implement a set of util functions for perf event

[Qiaowei Ren]

This patch implement a set of interfaces for perf event. Based on
these interfaces, we can implement internal driver API for perf,
and get the results of perf conuter you care about.

Signed-off-by: Qiaowei Ren 
---
 include/libvirt/virterror.h |   1 +
 src/Makefile.am |   1 +
 src/libvirt_private.syms|  12 ++
 src/util/virerror.c |   1 +
 src/util/virperf.c  | 303 
 src/util/virperf.h  |  63 +
 6 files changed, 381 insertions(+)
 create mode 100644 src/util/virperf.c
 create mode 100644 src/util/virperf.h

diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
index 0539e48..7e87162 100644
--- a/include/libvirt/virterror.h
+++ b/include/libvirt/virterror.h
@@ -128,6 +128,7 @@ typedef enum {
 VIR_FROM_THREAD = 61,   /* Error from thread utils */
 VIR_FROM_ADMIN = 62,/* Error from admin backend */
 VIR_FROM_LOGGING = 63,  /* Error from log manager */
+VIR_FROM_PERF = 64, /* Error from perf */
 
 # ifdef VIR_ENUM_SENTINELS
 VIR_ERR_DOMAIN_LAST
diff --git a/src/Makefile.am b/src/Makefile.am
index 7219f7c..26ac935 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -98,6 +98,7 @@ UTIL_SOURCES =
\
util/virauthconfig.c util/virauthconfig.h   \
util/virbitmap.c util/virbitmap.h   \
util/virbuffer.c util/virbuffer.h   \
+   util/virperf.c util/virperf.h   \
util/vircgroup.c util/vircgroup.h util/vircgrouppriv.h  \
util/virclosecallbacks.c util/virclosecallbacks.h   
\
util/vircommand.c util/vircommand.h util/vircommandpriv.h \
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index dd085c3..057bd29 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2001,6 +2001,18 @@ virPCIGetVirtualFunctions;
 virPCIIsVirtualFunction;
 
 
+# util/virperf.h
+virPerfEventDisable;
+virPerfEventEnable;
+virPerfEventIsEnabled;
+virPerfEventTypeFromString;
+virPerfEventTypeToString;
+virPerfFree;
+virPerfGetEventFd;
+virPerfNew;
+virPerfReadEvent;
+
+
 # util/virpidfile.h
 virPidFileAcquire;
 virPidFileAcquirePath;
diff --git a/src/util/virerror.c b/src/util/virerror.c
index 098211a..7e6918d 100644
--- a/src/util/virerror.c
+++ b/src/util/virerror.c
@@ -135,6 +135,7 @@ VIR_ENUM_IMPL(virErrorDomain, VIR_ERR_DOMAIN_LAST,
   "Thread jobs",
   "Admin Interface",
   "Log Manager",
+  "Perf",
 )
 
 
diff --git a/src/util/virperf.c b/src/util/virperf.c
new file mode 100644
index 000..35d516d
--- /dev/null
+++ b/src/util/virperf.c
@@ -0,0 +1,303 @@
+/*
+ * virperf.c: methods for managing perf events
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ *
+ * Authors:
+ *  Ren Qiaowei 
+ */
+#include 
+
+#include 
+#if defined HAVE_SYS_SYSCALL_H
+# include 
+#endif
+
+#include "virperf.h"
+#include "viralloc.h"
+#include "virerror.h"
+#include "virlog.h"
+#include "virfile.h"
+#include "virstring.h"
+#include "virtypedparam.h"
+
+VIR_LOG_INIT("util.perf");
+
+#define VIR_FROM_THIS VIR_FROM_PERF
+
+VIR_ENUM_IMPL(virPerfEvent, VIR_PERF_EVENT_LAST,
+  "cmt");
+
+struct virPerfEvent {
+int type;
+int fd;
+bool enabled;
+union {
+/* cmt */
+struct {
+int scale;
+} cmt;
+} efields;
+};
+typedef struct virPerfEvent *virPerfEventPtr;
+
+struct virPerf {
+struct virPerfEvent events[VIR_PERF_EVENT_LAST];
+};
+
+virPerfPtr
+virPerfNew(void)
+{
+size_t i;
+virPerfPtr perf;
+
+if (VIR_ALLOC(perf) < 0)
+return NULL;
+
+for (i = 0; i < VIR_PERF_EVENT_LAST; i++) {
+perf->events[i].type = i;
+perf->events[i].fd = -1;
+perf->events[i].enabled = false;
+}
+
+return perf;
+}
+
+void
+virPerfFree(virPerfPtr perf)
+{
+size_t i;
+
+if (perf == NULL)
+return;
+
+for (i = 0; i < VIR_PERF_EVENT_LAST; i++) {
+if (perf->events[i].enabled)
+virPerfEventDisable(perf, i);
+}
+
+VIR_FREE(perf);
+}
+
+#if defined(__linux__) && defined(HAVE_SYS_SYSCALL_H)
+
+# include 
+

Re: [libvirt] [PATCH 7/7] qemu_domain: cleanup default input device code

[Erik Skultety]

On 04/12/15 20:30, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_domain.c | 28 
> ++
>  .../qemuxml2argvdata/qemuxml2argv-pseries-disk.xml |  2 +-
>  2 files changed, 13 insertions(+), 17 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 736624e..9cd3f4f 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1038,8 +1038,7 @@ qemuDomainDefPostParse(virDomainDefPtr def,
>  bool addPCIRoot = false;
>  bool addPCIeRoot = false;
>  bool addDefaultMemballoon = true;
> -bool addDefaultUSBKBD = false;
> -bool addDefaultUSBMouse = false;
> +bool addDefaultUSBInput = false;
>  bool addPanicDevice = false;
>  int ret = -1;
>  
> @@ -1096,8 +1095,7 @@ qemuDomainDefPostParse(virDomainDefPtr def,
>  case VIR_ARCH_PPC64:
>  case VIR_ARCH_PPC64LE:
>  addPCIRoot = true;
> -addDefaultUSBKBD = true;
> -addDefaultUSBMouse = true;
> +addDefaultUSBInput = true;
>  /* For pSeries guests, the firmware provides the same
>   * functionality as the pvpanic device, so automatically
>   * add the definition if not already present */
> @@ -1177,19 +1175,17 @@ qemuDomainDefPostParse(virDomainDefPtr def,
>  def->memballoon = memballoon;
>  }
>  
> -if (addDefaultUSBKBD &&
> -def->ngraphics > 0 &&
> -virDomainDefMaybeAddInput(def,
> -  VIR_DOMAIN_INPUT_TYPE_KBD,
> -  VIR_DOMAIN_INPUT_BUS_USB) < 0)
> -goto cleanup;
> +if (def->ngraphics > 0 && addDefaultUSBInput) {
> +if (virDomainDefMaybeAddInput(def,
> +  VIR_DOMAIN_INPUT_TYPE_MOUSE,
> +  VIR_DOMAIN_INPUT_BUS_USB) < 0)
> +goto cleanup;
>  
> -if (addDefaultUSBMouse &&
> -def->ngraphics > 0 &&
> -virDomainDefMaybeAddInput(def,
> -  VIR_DOMAIN_INPUT_TYPE_MOUSE,
> -  VIR_DOMAIN_INPUT_BUS_USB) < 0)
> -goto cleanup;
> +if (virDomainDefMaybeAddInput(def,
> +  VIR_DOMAIN_INPUT_TYPE_KBD,
> +  VIR_DOMAIN_INPUT_BUS_USB) < 0)
> +goto cleanup;
> +}
>  
>  if (addPanicDevice) {
>  size_t j;
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml 
> b/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml
> index 39f4a1f..6472af5 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml
> @@ -30,8 +30,8 @@
>  
>  
>  
> -
>  
> +
>  
>  
>
> 

I'm not convinced we want this, I discussed it with Jan to find some
corner cases that would break, there might be a slight problem with
saving and restoring an old domain on a new libvirt if user provided
some newxml to alter the host definition during restore which would have
those input devices removed, then migratable definition would be created
in qemuDomainSaveImageUpdateDef which would add these implicit devices
in reverse order if graphic device was present. The following  ABI
stability check would then fail.
Someone else could have at this as well and correct me, possibly NACKing
the patch. You could wait a while, maybe there will be a different review.

Erik
Erik

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

[libvirt] [PATCH 1/8] perf: add new public APIs for perf event

[Qiaowei Ren]

API agreed on in
https://www.redhat.com/archives/libvir-list/2015-October/msg00872.html

* include/libvirt/libvirt-domain.h (virDomainGetPerfEvents,
virDomainSetPerfEvents): New declarations.
* src/libvirt_public.syms: Export new symbols.
* src/driver-hypervisor.h (virDrvDomainGetPerfEvents,
virDrvDomainSetPerfEvents): New typedefs.
* src/libvirt-domain.c: Implement virDomainGetPerfEvents and
virDomainSetPerfEvents.

Signed-off-by: Qiaowei Ren 
---
 include/libvirt/libvirt-domain.h | 18 
 src/driver-hypervisor.h  | 12 ++
 src/libvirt-domain.c | 93 
 src/libvirt_public.syms  |  6 +++
 4 files changed, 129 insertions(+)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index a1ea6a5..cc1b29b 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -1802,6 +1802,24 @@ int virDomainListGetStats(virDomainPtr *doms,
 void virDomainStatsRecordListFree(virDomainStatsRecordPtr *stats);
 
 /*
+ * Perf Event API
+ */
+
+/**
+ * VIR_PERF_PARAM_CMT:
+ *
+ * Macro for typed parameter name that represents CMT perf event.
+ */
+# define VIR_PERF_PARAM_CMT "cmt"
+
+int virDomainGetPerfEvents(virDomainPtr dom,
+   virTypedParameterPtr *params,
+   int *nparams);
+int virDomainSetPerfEvents(virDomainPtr dom,
+   virTypedParameterPtr params,
+   int nparams);
+
+/*
  * BlockJob API
  */
 
diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
index ae2ec4d..aedbc83 100644
--- a/src/driver-hypervisor.h
+++ b/src/driver-hypervisor.h
@@ -958,6 +958,16 @@ typedef int
 unsigned int flags);
 
 typedef int
+(*virDrvDomainGetPerfEvents)(virDomainPtr dom,
+ virTypedParameterPtr *params,
+ int *nparams);
+
+typedef int
+(*virDrvDomainSetPerfEvents)(virDomainPtr dom,
+ virTypedParameterPtr params,
+ int nparams);
+
+typedef int
 (*virDrvDomainBlockJobAbort)(virDomainPtr dom,
  const char *path,
  unsigned int flags);
@@ -1413,6 +1423,8 @@ struct _virHypervisorDriver {
 virDrvConnectSetKeepAlive connectSetKeepAlive;
 virDrvConnectIsAlive connectIsAlive;
 virDrvNodeSuspendForDuration nodeSuspendForDuration;
+virDrvDomainGetPerfEvents domainGetPerfEvents;
+virDrvDomainSetPerfEvents domainSetPerfEvents;
 virDrvDomainSetBlockIoTune domainSetBlockIoTune;
 virDrvDomainGetBlockIoTune domainGetBlockIoTune;
 virDrvDomainGetCPUStats domainGetCPUStats;
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index b91388e..f105803 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -9564,6 +9564,99 @@ virDomainOpenChannel(virDomainPtr dom,
 
 
 /**
+ * virDomainGetPerfEvents:
+ * @domain: a domain object
+ * @params: where to store perf events setting
+ * @nparams: number of items in @params
+ *
+ * Get all perf events setting. Possible fields returned in @params are
+ * defined by VIR_DOMAIN_PERF_* macros and new fields will likely be
+ * introduced in the future.
+ *
+ * Returns -1 in case of failure, 0 in case of success.
+ */
+int virDomainGetPerfEvents(virDomainPtr domain,
+   virTypedParameterPtr *params,
+   int *nparams)
+{
+virConnectPtr conn;
+
+VIR_DOMAIN_DEBUG(domain, "params=%p, nparams=%p",
+ params, nparams);
+
+virResetLastError();
+
+virCheckDomainReturn(domain, -1);
+virCheckNonNullArgGoto(params, error);
+virCheckNonNullArgGoto(nparams, error);
+
+conn = domain->conn;
+
+if (conn->driver->domainGetPerfEvents) {
+int ret;
+ret = conn->driver->domainGetPerfEvents(domain, params, nparams);
+if (ret < 0)
+goto error;
+return ret;
+}
+virReportUnsupportedError();
+
+ error:
+virDispatchError(domain->conn);
+return -1;
+}
+
+
+/**
+ * virDomainSetPerfEvents:
+ * @domain: a domain object
+ * @params: pointer to perf events parameter object
+ * @nparams: number of perf event parameters (this value can be the same
+ *   less than the number of parameters supported)
+ *
+ * Enable or disable the particular list of perf events you care about.
+ *
+ * Returns -1 in case of error, 0 in case of success.
+ */
+int virDomainSetPerfEvents(virDomainPtr domain,
+   virTypedParameterPtr params,
+   int nparams)
+{
+virConnectPtr conn;
+
+VIR_DOMAIN_DEBUG(domain, "params=%p, nparams=%d",
+ params, nparams);
+VIR_TYPED_PARAMS_DEBUG(params, nparams);
+
+virResetLastError();
+
+virCheckDomainReturn(domain, -1);
+conn = domain->conn;
+
+virCheckReadOnlyGoto(conn->flags, error);
+

[libvirt] [PATCH] test: qemuxml2argv: Mock virMemoryMaxValue to remove 32/64 bit difference

[Peter Krempa]

Always return LLONG_MAX even on 32 bit systems. The limitation
originates from our use of "unsigned long" in several APIs. The internal
data type is unsigned long long. Make the test suite deterministic by
removing the architecture difference.

Flaw was introduced in 645881139b3d2c86acf9d644c3a1471520bc9e57 where
I've added a test that uses too large numbers.
---
I've tested this by changing the mock value to the number returned on 32-bit
systems and got the same error. I didn't test it on an actual 32 bit system
though.

 src/util/virutil.c   |  2 ++
 tests/qemuxml2argvmock.c | 10 ++
 2 files changed, 12 insertions(+)

diff --git a/src/util/virutil.c b/src/util/virutil.c
index fe65faf..9d56d66 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -2639,6 +2639,8 @@ virMemoryLimitIsSet(unsigned long long value)
  * @capped: whether the value must fit into unsigned long
  *   (long long is assumed otherwise)
  *
+ * Note: This function is mocked in tests/qemuxml2argvmock.c for test stability
+ *
  * Returns the maximum possible memory value in bytes.
  */
 unsigned long long
diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c
index e58b8ce..8426108 100644
--- a/tests/qemuxml2argvmock.c
+++ b/tests/qemuxml2argvmock.c
@@ -74,3 +74,13 @@ virTPMCreateCancelPath(const char *devpath)

 return path;
 }
+
+/**
+ * Large values for memory would fail on 32 bit systems, despite having
+ * variables that support it.
+ */
+unsigned long long
+virMemoryMaxValue(bool capped ATTRIBUTE_UNUSED)
+{
+return LLONG_MAX;
+}
-- 
2.6.2

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

[libvirt] [PATCH 5/8] perf: add new xml element

[Qiaowei Ren]

This patch adds new xml element, and so we can have the option of
also having perf events enabled immediately at startup.

Signed-off-by: Qiaowei Ren 
---
 docs/schemas/domaincommon.rng |  27 +++
 src/conf/domain_conf.c| 111 ++
 src/conf/domain_conf.h|  10 +++
 src/qemu/qemu_driver.c|  26 ++
 src/qemu/qemu_process.c   |   8 +-
 tests/domainschemadata/domain-perf-simple.xml |  20 +
 6 files changed, 200 insertions(+), 2 deletions(-)
 create mode 100644 tests/domainschemadata/domain-perf-simple.xml

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 4804c69..004fcf9 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -56,6 +56,9 @@
   
 
 
+  
+
+
   
 
 
@@ -393,6 +396,30 @@
   
 
   
+  
+
+  
+
+  
+
+  cmt
+
+  
+  
+
+  
+
+  
+
+  
+
+  

Re: [libvirt] [PATCHv2 4/4] qemu: Implement shared memory device hot-unplug

[John Ferlan]

On 11/26/2015 04:07 AM, Luyao Huang wrote:
> Signed-off-by: Luyao Huang 
> ---
>  src/qemu/qemu_driver.c  |  4 ++-
>  src/qemu/qemu_hotplug.c | 94 
> -
>  src/qemu/qemu_hotplug.h |  3 ++
>  3 files changed, 99 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 3c25c07..9e7429a 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7881,6 +7881,9 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
>  case VIR_DOMAIN_DEVICE_MEMORY:
>  ret = qemuDomainDetachMemoryDevice(driver, vm, dev->data.memory);
>  break;
> +case VIR_DOMAIN_DEVICE_SHMEM:
> +ret = qemuDomainDetachShmemDevice(driver, vm, dev->data.shmem);
> +break;
>  
>  case VIR_DOMAIN_DEVICE_FS:
>  case VIR_DOMAIN_DEVICE_INPUT:
> @@ -7892,7 +7895,6 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
>  case VIR_DOMAIN_DEVICE_SMARTCARD:
>  case VIR_DOMAIN_DEVICE_MEMBALLOON:
>  case VIR_DOMAIN_DEVICE_NVRAM:
> -case VIR_DOMAIN_DEVICE_SHMEM:
>  case VIR_DOMAIN_DEVICE_REDIRDEV:
>  case VIR_DOMAIN_DEVICE_NONE:
>  case VIR_DOMAIN_DEVICE_TPM:
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index c5e544d..4ab05f3 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -3347,6 +3347,45 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver,
>  }
>  
>  
> +static int
> +qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver,
> +virDomainObjPtr vm,
> +virDomainShmemDefPtr shmem)
> +{
> +virObjectEventPtr event;
> +char *charAlias = NULL;
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +ssize_t idx;
> +int rc = 0;
> +
> +VIR_DEBUG("Removing shared memory device %s from domain %p %s",
> +  shmem->info.alias, vm, vm->def->name);
> +
> +if (shmem->server.enabled) {
> +if (virAsprintf(, "char%s", shmem->info.alias) < 0)
> +return -1;
> +
> +qemuDomainObjEnterMonitor(driver, vm);
> +rc = qemuMonitorDetachCharDev(priv->mon, charAlias);
> +VIR_FREE(charAlias);
> +if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +return -1;
> +}
> +

Auditing code?

> +if (rc < 0)
> +return -1;
> +

I know this is a copy of the RemoveRNGDevice; however, this code doesn't
remove an 'obj'. In fact, if !shmem->server.enabled, then we don't enter
the monitor at all.

Thus the following event probably won't happen...

> +if ((event = virDomainEventDeviceRemovedNewFromObj(vm, 
> shmem->info.alias)))
> +qemuDomainEventQueue(driver, event);
> +
> +if ((idx = virDomainShmemFind(vm->def, shmem, true)) >= 0)
> +virDomainShmemRemove(vm->def, idx);
> +qemuDomainReleaseDeviceAddress(vm, >info, NULL);
> +virDomainShmemDefFree(shmem);
> +return 0;
> +}
> +
> +
>  int
>  qemuDomainRemoveDevice(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> @@ -3378,6 +3417,10 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver,
>  ret = qemuDomainRemoveMemoryDevice(driver, vm, dev->data.memory);
>  break;
>  
> +case VIR_DOMAIN_DEVICE_SHMEM:
> +ret = qemuDomainRemoveShmemDevice(driver, vm, dev->data.shmem);
> +break;
> +
>  case VIR_DOMAIN_DEVICE_NONE:
>  case VIR_DOMAIN_DEVICE_LEASE:
>  case VIR_DOMAIN_DEVICE_FS:
> @@ -3391,7 +3434,6 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver,
>  case VIR_DOMAIN_DEVICE_SMARTCARD:
>  case VIR_DOMAIN_DEVICE_MEMBALLOON:
>  case VIR_DOMAIN_DEVICE_NVRAM:
> -case VIR_DOMAIN_DEVICE_SHMEM:
>  case VIR_DOMAIN_DEVICE_TPM:
>  case VIR_DOMAIN_DEVICE_PANIC:
>  case VIR_DOMAIN_DEVICE_LAST:
> @@ -4378,3 +4420,53 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver,
>  qemuDomainResetDeviceRemoval(vm);
>  return ret;
>  }
> +
> +
> +int
> +qemuDomainDetachShmemDevice(virQEMUDriverPtr driver,
> +virDomainObjPtr vm,
> +virDomainShmemDefPtr shmem)
> +{
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +ssize_t idx;
> +virDomainShmemDefPtr tmpshmem;
> +int rc;
> +int ret = -1;
> +
> +if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
> +virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +   _("qemu does not support -device"));
> +return -1;
> +}

Well it's here, but not in AttachShmemDevice...

> +
> +if ((idx = virDomainShmemFind(vm->def, shmem, true)) < 0) {

Again we could use a 'flags' argument instead...  Of course there isn't
a "false" argument to any of the added callers to date, so not sure of
the need for it...  Could still use/add flags and keep it as "unused"
for now.

> +virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +   _("device not present in domain 

[libvirt] [PATCH v2 5/7] virNetDevMacVLanTapSetup: Allow enabling of IFF_MULTI_QUEUE

[Michal Privoznik]

Like we are doing for TUN/TAP devices, we should do the same for
macvtaps. Although, it's not as critical as in that case, we
should do it for the consistency.

Signed-off-by: Michal Privoznik 
---
 src/util/virnetdevmacvlan.c | 40 ++--
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index c4d0d53..76fd542 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -289,24 +289,26 @@ virNetDevMacVLanTapOpen(const char *ifname,
  * @tapfd: array of file descriptors of the macvtap tap
  * @tapfdSize: number of file descriptors in @tapfd
  * @vnet_hdr: whether to enable or disable IFF_VNET_HDR
+ * @multiqueue: whether to enable or disable IFF_MULTI_QUEUE
+ *
+ * Turn the IFF_VNET_HDR flag, if requested and available, make sure it's
+ * off in the other cases. Similarly, IFF_MULTI_QUEUE is enabled if
+ * requested. However, if requested and failed to set, it is considered a
+ * fatal error (as opposed to @vnet_hdr).
  *
- * Turn the IFF_VNET_HDR flag, if requested and available, make sure
- * it's off in the other cases.
  * A fatal error is defined as the VNET_HDR flag being set but it cannot
  * be turned off for some reason. This is reported with -1. Other fatal
  * error is not being able to read the interface flags. In that case the
  * macvtap device should not be used.
  *
- * Returns 0 on success, -1 in case of fatal error, error code otherwise.
+ * Returns 0 on success, -1 in case of fatal error.
  */
 static int
-virNetDevMacVLanTapSetup(int *tapfd, size_t tapfdSize, bool vnet_hdr)
+virNetDevMacVLanTapSetup(int *tapfd, size_t tapfdSize, bool vnet_hdr, bool 
multiqueue)
 {
 unsigned int features;
 struct ifreq ifreq;
 short new_flags = 0;
-int rc_on_fail = 0;
-const char *errmsg = NULL;
 size_t i;
 
 for (i = 0; i < tapfdSize; i++) {
@@ -320,27 +322,29 @@ virNetDevMacVLanTapSetup(int *tapfd, size_t tapfdSize, 
bool vnet_hdr)
 
 new_flags = ifreq.ifr_flags;
 
-if ((ifreq.ifr_flags & IFF_VNET_HDR) && !vnet_hdr) {
-new_flags = ifreq.ifr_flags & ~IFF_VNET_HDR;
-rc_on_fail = -1;
-errmsg = _("cannot clean IFF_VNET_HDR flag on macvtap tap");
-} else if ((ifreq.ifr_flags & IFF_VNET_HDR) == 0 && vnet_hdr) {
+if (vnet_hdr) {
 if (ioctl(tapfd[i], TUNGETFEATURES, ) < 0) {
 virReportSystemError(errno, "%s",
  _("cannot get feature flags on macvtap 
tap"));
 return -1;
 }
-if ((features & IFF_VNET_HDR)) {
-new_flags = ifreq.ifr_flags | IFF_VNET_HDR;
-errmsg = _("cannot set IFF_VNET_HDR flag on macvtap tap");
-}
+if (features & IFF_VNET_HDR)
+new_flags |= IFF_VNET_HDR;
+} else {
+new_flags &= ~IFF_VNET_HDR;
 }
 
+if (multiqueue)
+new_flags |= IFF_MULTI_QUEUE;
+else
+new_flags &= ~IFF_MULTI_QUEUE;
+
 if (new_flags != ifreq.ifr_flags) {
 ifreq.ifr_flags = new_flags;
 if (ioctl(tapfd[i], TUNSETIFF, ) < 0) {
-virReportSystemError(errno, "%s", errmsg);
-return rc_on_fail;
+virReportSystemError(errno, "%s",
+ _("unable to set vnet or multiqueue flags 
on macvtap"));
+return -1;
 }
 }
 }
@@ -852,7 +856,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const char 
*tgifname,
 if (virNetDevMacVLanTapOpen(cr_ifname, , 1, 10) < 0)
 goto disassociate_exit;
 
-if (virNetDevMacVLanTapSetup(, 1, vnet_hdr) < 0) {
+if (virNetDevMacVLanTapSetup(, 1, vnet_hdr, false) < 0) {
 VIR_FORCE_CLOSE(rc); /* sets rc to -1 */
 goto disassociate_exit;
 }
-- 
2.4.10

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

[libvirt] [PATCH v2 6/7] virNetDevMacVLanCreateWithVPortProfile: Rework to support multiple FDs

[Michal Privoznik]

For the multiqueue on macvtaps we are going to need to open
the device multiple times. Currently, this is not supported.
Rework the function, so that upper layers can be reworked too.

Signed-off-by: Michal Privoznik 
---
 src/lxc/lxc_process.c   |  1 +
 src/qemu/qemu_command.c | 21 -
 src/util/virnetdevmacvlan.c | 20 +++-
 src/util/virnetdevmacvlan.h |  4 +++-
 4 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 0ada6e4..f7e2b81 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -349,6 +349,7 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn,
 _ifname,
 VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
 cfg->stateDir,
+NULL, 0,
 macvlan_create_flags) < 0)
 goto cleanup;
 
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index b1febed..d856377 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -230,15 +230,18 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
 if (net->model && STREQ(net->model, "virtio"))
 macvlan_create_flags |= VIR_NETDEV_MACVLAN_VNET_HDR;
 
-rc = virNetDevMacVLanCreateWithVPortProfile(
-net->ifname, >mac,
-virDomainNetGetActualDirectDev(net),
-virDomainNetGetActualDirectMode(net),
-def->uuid,
-virDomainNetGetActualVirtPortProfile(net),
-_ifname,
-vmop, cfg->stateDir,
-macvlan_create_flags);
+if (virNetDevMacVLanCreateWithVPortProfile(net->ifname,
+   >mac,
+   
virDomainNetGetActualDirectDev(net),
+   
virDomainNetGetActualDirectMode(net),
+   def->uuid,
+   
virDomainNetGetActualVirtPortProfile(net),
+   _ifname,
+   vmop, cfg->stateDir,
+   , 1,
+   macvlan_create_flags) < 0)
+return -1;
+
 if (rc >= 0) {
 virDomainAuditNetDevice(def, net, res_ifname, true);
 VIR_FREE(net->ifname);
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index 76fd542..e62cbda 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -727,11 +727,15 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char 
*ifname,
  * @res_ifname: Pointer to a string pointer where the actual name of the
  * interface will be stored into if everything succeeded. It is up
  * to the caller to free the string.
+ * @tapfd: array of file descriptor return value for the new tap device
+ * @tapfdSize: number of file descriptors in @tapfd
  * @flags: OR of virNetDevMacVLanCreateFlags.
  *
- * Returns file descriptor of the tap device in case of success with
- * @flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP, otherwise returns 0; returns
- * -1 on error.
+ * Creates a macvlan device. Optionally, if flags &
+ * VIR_NETDEV_MACVLAN_CREATE_WITH_TAP is set, @tapfd is populated with FDs of
+ * tap devices up to @tapfdSize.
+ *
+ * Return 0 on success, -1 on error.
  */
 int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname,
const virMacAddr *macaddress,
@@ -742,6 +746,8 @@ int virNetDevMacVLanCreateWithVPortProfile(const char 
*tgifname,
char **res_ifname,
virNetDevVPortProfileOp vmOp,
char *stateDir,
+   int *tapfd,
+   size_t tapfdSize,
unsigned int flags)
 {
 const char *type = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ?
@@ -853,10 +859,10 @@ int virNetDevMacVLanCreateWithVPortProfile(const char 
*tgifname,
 }
 
 if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) {
-if (virNetDevMacVLanTapOpen(cr_ifname, , 1, 10) < 0)
+if (virNetDevMacVLanTapOpen(cr_ifname, tapfd, tapfdSize, 10) < 0)
 goto disassociate_exit;
 
-if (virNetDevMacVLanTapSetup(, 1, vnet_hdr, false) < 0) {
+if (virNetDevMacVLanTapSetup(tapfd, tapfdSize, vnet_hdr, tapfdSize > 
0) < 0) {
 VIR_FORCE_CLOSE(rc); /* sets rc to -1 */
 goto disassociate_exit;
 }
@@ -892,6 +898,8 @@ int virNetDevMacVLanCreateWithVPortProfile(const char 
*tgifname,
linkdev,
vf,
vmOp));
+while (tapfdSize--)
+VIR_FORCE_CLOSE(tapfd[tapfdSize]);
 
  link_del_exit:
 

[libvirt] [PATCHv2 1/3] util: cgroups do not implicitly add task to new machine cgroup

[Henning Schild]

virCgroupNewMachine used to add the pidleader to the newly created
machine cgroup. Do not do this implicit anymore.

Signed-off-by: Henning Schild 
---
 src/lxc/lxc_cgroup.c   | 11 +++
 src/qemu/qemu_cgroup.c | 11 +++
 src/util/vircgroup.c   | 22 --
 3 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c
index ad254e4..609e9ea 100644
--- a/src/lxc/lxc_cgroup.c
+++ b/src/lxc/lxc_cgroup.c
@@ -504,6 +504,17 @@ virCgroupPtr virLXCCgroupCreate(virDomainDefPtr def,
 ) < 0)
 goto cleanup;
 
+if (virCgroupAddTask(cgroup, initpid) < 0) {
+virErrorPtr saved = virSaveLastError();
+virCgroupRemove(cgroup);
+virCgroupFree();
+if (saved) {
+virSetError(saved);
+virFreeError(saved);
+}
+goto cleanup;
+}
+
 /* setup control group permissions for user namespace */
 if (def->idmap.uidmap) {
 if (virCgroupSetOwner(cgroup,
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 0da6c02..7320046 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -770,6 +770,17 @@ qemuInitCgroup(virQEMUDriverPtr driver,
 goto cleanup;
 }
 
+if (virCgroupAddTask(priv->cgroup, vm->pid) < 0) {
+virErrorPtr saved = virSaveLastError();
+virCgroupRemove(priv->cgroup);
+virCgroupFree(>cgroup);
+if (saved) {
+virSetError(saved);
+virFreeError(saved);
+}
+goto cleanup;
+}
+
  done:
 ret = 0;
  cleanup:
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 0379c2e..a07f3c2 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -1669,16 +1669,6 @@ virCgroupNewMachineSystemd(const char *name,
 }
 }
 
-if (virCgroupAddTask(*group, pidleader) < 0) {
-virErrorPtr saved = virSaveLastError();
-virCgroupRemove(*group);
-virCgroupFree(group);
-if (saved) {
-virSetError(saved);
-virFreeError(saved);
-}
-}
-
 ret = 0;
  cleanup:
 virCgroupFree();
@@ -1701,7 +1691,6 @@ int virCgroupTerminateMachine(const char *name,
 static int
 virCgroupNewMachineManual(const char *name,
   const char *drivername,
-  pid_t pidleader,
   const char *partition,
   int controllers,
   virCgroupPtr *group)
@@ -1727,16 +1716,6 @@ virCgroupNewMachineManual(const char *name,
 group) < 0)
 goto cleanup;
 
-if (virCgroupAddTask(*group, pidleader) < 0) {
-virErrorPtr saved = virSaveLastError();
-virCgroupRemove(*group);
-virCgroupFree(group);
-if (saved) {
-virSetError(saved);
-virFreeError(saved);
-}
-}
-
  done:
 ret = 0;
 
@@ -1783,7 +1762,6 @@ virCgroupNewMachine(const char *name,
 
 return virCgroupNewMachineManual(name,
  drivername,
- pidleader,
  partition,
  controllers,
  group);
-- 
2.4.10

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

[libvirt] [PATCH 2/3] Remove dead code from qemuDomainAttachControllerDevice

[Ján Tomko]

We only support hotplugging SCSI controllers,
USB and virtio-serial related code is useless here.
---
 src/conf/domain_addr.c   | 21 -
 src/conf/domain_addr.h   |  4 
 src/libvirt_private.syms |  1 -
 src/qemu/qemu_hotplug.c  | 18 --
 4 files changed, 44 deletions(-)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index ca5803e..c7eab0c 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -884,27 +884,6 @@ 
virDomainVirtioSerialAddrSetAddControllers(virDomainVirtioSerialAddrSetPtr addrs
 return 0;
 }
 
-/* virDomainVirtioSerialAddrSetRemoveController
- *
- * Removes a virtio serial controller from the address set.
- */
-void
-virDomainVirtioSerialAddrSetRemoveController(virDomainVirtioSerialAddrSetPtr 
addrs,
- virDomainControllerDefPtr cont)
-{
-ssize_t pos;
-
-if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL)
-return;
-
-VIR_DEBUG("Removing virtio serial controller index %u "
-  "from the address set", cont->idx);
-
-pos = virDomainVirtioSerialAddrFindController(addrs, cont->idx);
-
-if (pos >= 0)
-VIR_DELETE_ELEMENT(addrs->controllers, pos, addrs->ncontrollers);
-}
 
 void
 virDomainVirtioSerialAddrSetFree(virDomainVirtioSerialAddrSetPtr addrs)
diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
index 2220a79..74f414e 100644
--- a/src/conf/domain_addr.h
+++ b/src/conf/domain_addr.h
@@ -209,10 +209,6 @@ int
 virDomainVirtioSerialAddrSetAddController(virDomainVirtioSerialAddrSetPtr 
addrs,
   virDomainControllerDefPtr cont)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
-void
-virDomainVirtioSerialAddrSetRemoveController(virDomainVirtioSerialAddrSetPtr 
addrs,
- virDomainControllerDefPtr cont)
-ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
 int
 virDomainVirtioSerialAddrSetAddControllers(virDomainVirtioSerialAddrSetPtr 
addrs,
virDomainDefPtr def)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 63d8618..536acab 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -115,7 +115,6 @@ virDomainVirtioSerialAddrSetAddController;
 virDomainVirtioSerialAddrSetAddControllers;
 virDomainVirtioSerialAddrSetCreate;
 virDomainVirtioSerialAddrSetFree;
-virDomainVirtioSerialAddrSetRemoveController;
 
 
 # conf/domain_audit.h
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index eae5418..9bd4238 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -442,7 +442,6 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr 
driver,
 char *devstr = NULL;
 qemuDomainObjPrivatePtr priv = vm->privateData;
 bool releaseaddr = false;
-bool addedToAddrSet = false;
 
 if (controller->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
 virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
@@ -484,20 +483,6 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr 
driver,
 if (qemuAssignDeviceControllerAlias(vm->def, priv->qemuCaps, 
controller) < 0)
 goto cleanup;
 
-if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB &&
-controller->model == -1 &&
-!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("USB controller hotplug unsupported in this QEMU 
binary"));
-goto cleanup;
-}
-
-if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL &&
-virDomainVirtioSerialAddrSetAddController(priv->vioserialaddrs,
-  controller) < 0)
-goto cleanup;
-addedToAddrSet = true;
-
 if (!(devstr = qemuBuildControllerDevStr(vm->def, controller, 
priv->qemuCaps, NULL)))
 goto cleanup;
 }
@@ -526,9 +511,6 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr 
driver,
 }
 
  cleanup:
-if (ret != 0 && addedToAddrSet)
-virDomainVirtioSerialAddrSetRemoveController(priv->vioserialaddrs,
- controller);
 if (ret != 0 && releaseaddr)
 qemuDomainReleaseDeviceAddress(vm, >info, NULL);
 
-- 
2.4.6

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

Re: [libvirt] [PATCH 3/6] qemu: Add qemuDomainAdjustMaxMemLock()

[John Ferlan]

On 11/24/2015 08:56 AM, Andrea Bolognani wrote:
[...]

> +}
> +
> +/* Don't do anything unless we're actually setting a limit */
> +if (bytes) {
> +if (virProcessSetMaxMemLock(vm->pid, bytes) < 0)
> +goto out;
> +}


virProcessSetMaxMemLock aready checks:

   if (bytes == 0)
return 0;

So the "if (bytes)" is unnecessary


John

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

Re: [libvirt] [PATCH 0/7] tests: Always fake root directory

[Andrea Bolognani]

On Fri, 2015-12-04 at 17:07 +0100, Andrea Bolognani wrote:
> On Fri, 2015-12-04 at 16:31 +0100, Michal Privoznik wrote:
> > ACK, although I'd rather wait until after release. If you (or somebody
> > else) feels otherwise, do push it any time you want.
> 
> No need to rush, I'll push after release.
> 
> Thanks for the review!

Now pushed.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

[libvirt] [PATCH 08/10] tests.nwfilterebiptablestest: swap actual and expected

[Pavel Hrdina]

Those parameters should be in opposite order.

Signed-off-by: Pavel Hrdina 
---
 tests/nwfilterebiptablestest.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tests/nwfilterebiptablestest.c b/tests/nwfilterebiptablestest.c
index e1330ef..6bd6aad 100644
--- a/tests/nwfilterebiptablestest.c
+++ b/tests/nwfilterebiptablestest.c
@@ -116,7 +116,7 @@ testNWFilterEBIPTablesAllTeardown(const void *opaque 
ATTRIBUTE_UNUSED)
 virtTestClearCommandPath(actual);
 
 if (STRNEQ_NULLABLE(actual, expected)) {
-virtTestDifference(stderr, actual, expected);
+virtTestDifference(stderr, expected, actual);
 goto cleanup;
 }
 
@@ -187,7 +187,7 @@ testNWFilterEBIPTablesTearOldRules(const void *opaque 
ATTRIBUTE_UNUSED)
 virtTestClearCommandPath(actual);
 
 if (STRNEQ_NULLABLE(actual, expected)) {
-virtTestDifference(stderr, actual, expected);
+virtTestDifference(stderr, expected, actual);
 goto cleanup;
 }
 
@@ -236,7 +236,7 @@ testNWFilterEBIPTablesRemoveBasicRules(const void *opaque 
ATTRIBUTE_UNUSED)
 virtTestClearCommandPath(actual);
 
 if (STRNEQ_NULLABLE(actual, expected)) {
-virtTestDifference(stderr, actual, expected);
+virtTestDifference(stderr, expected, actual);
 goto cleanup;
 }
 
@@ -270,7 +270,7 @@ testNWFilterEBIPTablesTearNewRules(const void *opaque 
ATTRIBUTE_UNUSED)
 virtTestClearCommandPath(actual);
 
 if (STRNEQ_NULLABLE(actual, expected)) {
-virtTestDifference(stderr, actual, expected);
+virtTestDifference(stderr, expected, actual);
 goto cleanup;
 }
 
@@ -342,7 +342,7 @@ testNWFilterEBIPTablesApplyBasicRules(const void *opaque 
ATTRIBUTE_UNUSED)
 virtTestClearCommandPath(actual);
 
 if (STRNEQ_NULLABLE(actual, expected)) {
-virtTestDifference(stderr, actual, expected);
+virtTestDifference(stderr, expected, actual);
 goto cleanup;
 }
 
@@ -432,7 +432,7 @@ testNWFilterEBIPTablesApplyDHCPOnlyRules(const void *opaque 
ATTRIBUTE_UNUSED)
 virtTestClearCommandPath(actual);
 
 if (STRNEQ_NULLABLE(actual, expected)) {
-virtTestDifference(stderr, actual, expected);
+virtTestDifference(stderr, expected, actual);
 goto cleanup;
 }
 
@@ -505,7 +505,7 @@ testNWFilterEBIPTablesApplyDropAllRules(const void *opaque 
ATTRIBUTE_UNUSED)
 virtTestClearCommandPath(actual);
 
 if (STRNEQ_NULLABLE(actual, expected)) {
-virtTestDifference(stderr, actual, expected);
+virtTestDifference(stderr, expected, actual);
 goto cleanup;
 }
 
-- 
2.6.3

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

Re: [libvirt] [PATCH v2 03/10] virsh: Add support for list -reason

[Martin Kletzander]

On Thu, Dec 10, 2015 at 03:15:43PM -0500, John Ferlan wrote:



On 12/01/2015 12:35 PM, Martin Kletzander wrote:

Add new parameter for the list command, --reason.  This adds another
optional column in the table output of listed domains.

Signed-off-by: Martin Kletzander 
---
 tests/virshtest.c| 16 
 tools/virsh-domain-monitor.c | 19 ++-
 tools/virsh.pod  |  5 -
 3 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/tests/virshtest.c b/tests/virshtest.c
index 5983b5b190d6..ecec898c7264 100644
--- a/tests/virshtest.c
+++ b/tests/virshtest.c
@@ -117,6 +117,18 @@ static int testCompareListCustom(const void *data 
ATTRIBUTE_UNUSED)
   return testCompareOutputLit(exp, NULL, argv);
 }

+static int testCompareListReason(const void *data ATTRIBUTE_UNUSED)
+{
+const char *const argv[] = { VIRSH_CUSTOM, "list", "--reason", NULL };
+  const char *exp = "\
+ IdName   State  Reason\n\
+\n\
+ 1 fv0runningunknown   \n\
+ 2 fc4runningunknown   \n\
+\n";
+  return testCompareOutputLit(exp, NULL, argv);
+}
+


Nice to have this test... Would be better to have one with Table too...



You mean Title, not Table, right?  This is Table output (default).  With
Title I would find out that there is extra newline, I'll include that as well.


 static int testCompareNodeinfoDefault(const void *data ATTRIBUTE_UNUSED)
 {
   const char *const argv[] = { VIRSH_DEFAULT, "nodeinfo", NULL };
@@ -266,6 +278,10 @@ mymain(void)
 testCompareListCustom, NULL) != 0)
 ret = -1;

+if (virtTestRun("virsh list (with reason)",
+testCompareListReason, NULL) != 0)
+ret = -1;
+
 if (virtTestRun("virsh nodeinfo (default)",
 testCompareNodeinfoDefault, NULL) != 0)
 ret = -1;
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index 24b902905968..e2ef2c566f84 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -1848,6 +1848,10 @@ static const vshCmdOptDef opts_list[] = {
  .type = VSH_OT_BOOL,
  .help = N_("show domain title")
 },
+{.name = "reason",
+ .type = VSH_OT_BOOL,
+ .help = N_("show domain state reason")
+},
 {.name = NULL}
 };

@@ -1862,10 +1866,12 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
 bool optTable = vshCommandOptBool(cmd, "table");
 bool optUUID = vshCommandOptBool(cmd, "uuid");
 bool optName = vshCommandOptBool(cmd, "name");
+bool optReason = vshCommandOptBool(cmd, "reason");
 size_t i;
 char *title;
 char uuid[VIR_UUID_STRING_BUFLEN];
 int state;
+int state_reason;
 bool ret = false;
 virshDomainListPtr list = NULL;
 virDomainPtr dom;
@@ -1916,12 +1922,18 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
 if (optTable) {
 vshPrintExtra(ctl, " %-5s %-30s %-10s", _("Id"), _("Name"), 
_("State"));

+if (optReason)
+vshPrintExtra(ctl, " %-10s", _("Reason"));
+


Fairly easy to "dashcount += 10;"

or because the longest reason I can find is 19 characters, perhaps use
20... The only oddity is long state reason when title is also used. Your
call on this.


 if (optTitle)
 vshPrintExtra(ctl, " %-20s\n", _("Title"));

 vshPrintExtra(ctl, "\n%s",
   "-");

+if (optReason)
+vshPrintExtra(ctl, "%s", "---");
+


not needing this if use dashcount...


 if (optTitle)
 vshPrintExtra(ctl, "%s", "-");

@@ -1936,7 +1948,7 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
 else
 ignore_value(virStrcpyStatic(id_buf, "-"));

-state = virshDomainState(ctl, dom, NULL);
+state = virshDomainState(ctl, dom, _reason);

 /* Domain could've been removed in the meantime */
 if (state < 0)
@@ -1952,6 +1964,11 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
  state == -2 ? _("saved")
  : virshDomainStateToString(state));

+if (optReason) {
+vshPrint(ctl, " %-10s",
+ virshDomainStateReasonToString(state, state_reason));
+}
+
 if (optTitle) {
 if (!(title = virshGetDomainDescription(ctl, dom, true, 0)))
 goto cleanup;
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 21ae4a3e5b46..7fddfc65d48c 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -393,7 +393,7 @@ value will generate output for the specific machine.
 Inject NMI to the guest.

 =item B [I<--inactive> | I<--all>]
-  [I<--managed-save>] [I<--title>]
+  [I<--managed-save>] [I<--title>] [I<--reason>]
   { 

[libvirt] [PATCH] util: Fixup virnetdevmacvlan.h ATTRIBUTE_NONNULL's

[John Ferlan]

Commit id '56e2171c6' removed a variable from the argument list, but
neglected to update the ATTRIBUTE_NONNULL values, so when commit id
'08da97bfb' added a couple of arguments, the values were off.

Signed-off-by: John Ferlan 
---

Pushed as build breaker (coverity) and trivial.

 src/util/virnetdevmacvlan.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/util/virnetdevmacvlan.h b/src/util/virnetdevmacvlan.h
index 2844a44..70e3b01 100644
--- a/src/util/virnetdevmacvlan.h
+++ b/src/util/virnetdevmacvlan.h
@@ -74,8 +74,8 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *ifname,
int *tapfd,
size_t tapfdSize,
unsigned int flags)
-ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(6)
-ATTRIBUTE_NONNULL(8) ATTRIBUTE_NONNULL(12) ATTRIBUTE_RETURN_CHECK;
+ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(5)
+ATTRIBUTE_NONNULL(7) ATTRIBUTE_NONNULL(9) ATTRIBUTE_RETURN_CHECK;
 
 int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname,
const virMacAddr *macaddress,
-- 
2.5.0

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

Re: [libvirt] [PATCH v2 03/10] virsh: Add support for list -reason

[John Ferlan]

On 12/11/2015 05:40 AM, Martin Kletzander wrote:
[...]
>>> +static int testCompareListReason(const void *data ATTRIBUTE_UNUSED)
>>> +{
>>> +const char *const argv[] = { VIRSH_CUSTOM, "list", "--reason",
>>> NULL };
>>> +  const char *exp = "\
>>> + IdName   State  Reason\n\
>>> +\n\
>>> + 1 fv0runningunknown   \n\
>>> + 2 fc4runningunknown   \n\
>>> +\n";
>>> +  return testCompareOutputLit(exp, NULL, argv);
>>> +}
>>> +
>>
>> Nice to have this test... Would be better to have one with Table too...
>>
> 
> You mean Title, not Table, right?  This is Table output (default).  With
> Title I would find out that there is extra newline, I'll include that as
> well.
> 

Yeah Title not Table

John

[...]

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

Re: [libvirt] [PATCH v2 02/10] virsh: Refactor the table output of the list command

[Martin Kletzander]

On Thu, Dec 10, 2015 at 03:13:36PM -0500, John Ferlan wrote:



On 12/01/2015 12:35 PM, Martin Kletzander wrote:

Clean up some duplicate code so it is easier to add new parameters.  The
tests need to be adjusted because after this patch there will be
additional spaces at the end of lines in the output of virsh list, but
this affects only the table output that is meant to be read by users.

Signed-off-by: Martin Kletzander 
---
 tests/virshtest.c| 14 +++---
 tools/virsh-domain-monitor.c | 38 +++---
 2 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/tests/virshtest.c b/tests/virshtest.c
index 3fdae92b7834..5983b5b190d6 100644
--- a/tests/virshtest.c
+++ b/tests/virshtest.c
@@ -98,9 +98,9 @@ static int testCompareListDefault(const void *data 
ATTRIBUTE_UNUSED)
 {
   const char *const argv[] = { VIRSH_DEFAULT, "list", NULL };
   const char *exp = "\
- IdName   State\n\
-\n\
- 1 test   running\n\
+ IdName   State \n\
+-\n\
+ 1 test   running   \n\
 \n";
   return testCompareOutputLit(exp, NULL, argv);
 }
@@ -109,10 +109,10 @@ static int testCompareListCustom(const void *data 
ATTRIBUTE_UNUSED)
 {
   const char *const argv[] = { VIRSH_CUSTOM, "list", NULL };
   const char *exp = "\
- IdName   State\n\
-\n\
- 1 fv0running\n\
- 2 fc4running\n\
+ IdName   State \n\
+-\n\
+ 1 fv0running   \n\
+ 2 fc4running   \n\


[1]If only to make the output line up w/r/t \n...



I can do that.  I don't like this output either, but I imagined
increasing the complexity of the output generating would gain negative
comments from others.  It's not needed for this series, just a
nice-to-have feature once this series is in (if ever).


 \n";
   return testCompareOutputLit(exp, NULL, argv);
 }
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index db5bf4b2a566..24b902905968 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -1914,16 +1914,18 @@ cmdList(vshControl *ctl, const vshCmd *cmd)

 /* print table header in legacy mode */
 if (optTable) {
+vshPrintExtra(ctl, " %-5s %-30s %-10s", _("Id"), _("Name"), 
_("State"));
+
 if (optTitle)
-vshPrintExtra(ctl, " %-5s %-30s %-10s %-20s\n%s\n",
-  _("Id"), _("Name"), _("State"), _("Title"),
-  "-"
-  "-");
-else
-vshPrintExtra(ctl, " %-5s %-30s %s\n%s\n",
-  _("Id"), _("Name"), _("State"),
-  "-"
-  "---");
+vshPrintExtra(ctl, " %-20s\n", _("Title"));


^^
I believe if Title were added there'd be an extra line once the
following dashes are printed (end w/ \n, start with \n)



I'll fix that as a part of another round of rework.


+
+vshPrintExtra(ctl, "\n%s",
+  "-");
+
+if (optTitle)
+vshPrintExtra(ctl, "%s", "-");


It does seem in the expected virshtest.c output we're still off by 1.
That is there is 1 more "-" than extra space after the State column.

[1]
Instead of counting dashes...

   dashcount = 45;  /* Or some value - whatever it is */
...
   if (optTitle) {
...
   dashcount += 20;
  }
,,,
   for (i = 0; i < dashcount; i++)
   buf[i] = '-';
   vshPrintExtra(ctl, "%s\n", buf);
...

where of course buf is appropriately defined ;-)  A nit would be that
the old output was 3 dashes longer.



I'll generalize and unify it so that it looks nice most of the time.
For the time being I did not talk at all about what non-ASCII characters
do wit the output and I'm not going to deal with that, mainly because
it' s aproblem of (f)printf itself.



ACK with at least the fixup as a result of having Title on the line (and
it would be nice to add a test for it too)!

Whether you implement the '-' hack is up to you, but it may make future
adjustments more simple...



Not that I would see any adjustments coming, there wasn't mucha
happening in this part of the code for some time, but I agree that if it
can be done better I should try more ;)


John


+
+vshPrintExtra(ctl, "\n");
 }

 for (i = 0; i < list->ndomains; i++) {
@@ -1945,23 +1947,21 @@ 

Re: [libvirt] [PATCH v2 07/10] conf: Optionally keep domains with invalid XML, but don't allow starting them

[John Ferlan]

[...]
>>> +
>>> +if (virFileReadAll(configFile, 1024*1024*10, ) < 0)
>>
>> Any reason to not use MAX_CONFIG_FILE_SIZE?  Cannot imagine this
>> failing, but I mention for consistency
>>
> 
> This was a copy-paste from somewhere else, but I will add
> MAX_CONFIG_FILE_SIZE both here and to that copy-paste source as well (if
> I can find it ;)
> 

Dang it's in a .c file now...  I initially cscope searched on
virFileReadAll... That had some examples... Which is why I mentioned it.

Using 'find src -exec grep "1024.*\*1024.*\*10" {} \;' yields

src/util/virconf.c
src/security/security_apparmor.h

Couldn't say the magic words to cscope to take that string and find
anything though ;-)

John

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

Re: [libvirt] [PATCH v2 00/10] Make loading domains with invalid XML possible

[Martin Kletzander]

On Thu, Dec 10, 2015 at 03:20:20PM -0500, John Ferlan wrote:



On 12/01/2015 12:35 PM, Martin Kletzander wrote:

We always had to deal with new parsing errors in a weird way.  All of
them needed to go into functions starting the domains.  That messes up
the code, it's confusing to newcomers and so on.

I once had an idea that we can handle this stuff.  We know what
failed, we have the XML that failed parsing and if the problem is not
in the domain name nor UUID, we can create a domain object out of that
as well.  This way we are able to do something we weren't with this
series applied.  Example follows.

Assume "dummy" is a domain with invalid XML (I just modified the
file).  Now, just for the purpose of this silly example, let's say we
allowed domains with archtecture x86_*, but now we've realized that
x86_64 is the only one we want to allow, but there already is a domain
defined that has .  With the current master,
the domain would be lost, so we would need to modify the funstion
starting the domain (e.g. qemuProcessStart for qemu domain).  However,
with this series, it behaves like this:

  # virsh list --all --reason
 IdName   State  Reason
---
 - dummy  shut off   invalid XML

  # virsh domstate --reason dummy
shut off (invalid XML)

  # virsh start dummy
  error: Failed to start domain dummy
  error: XML error: domain 'dummy' was not loaded due to an XML error
  (unsupported configuration: Unknown architecture x86_256), please
  redefine it

  # VISUAL='sed -i s/x86_256/x86_64/' virsh edit dummy
  Domain dummy XML configuration edited.

  # virsh domstate --reason dummy
  shut off (unknown)

  # virsh start dummy
  Domain dummy started

This is a logical next step for us to clean and separate parsing and
starting, getting rid of some old code without sacrifying compatibility
and maybe generating parser code in the future.

v2:
 - rebased on top of current master (with virdomainobjlist.c)
 - only disallow starting domains with invalid definitions (change
   done in v1), but allow re-defining them
 - add support for "virsh list --reason" to better excercise the
   feature added in this patchset

v1:
 - rebase
 - don't allow starting domains with invalid state
 - https://www.redhat.com/archives/libvir-list/2015-November/msg01127.html

RFC: https://www.redhat.com/archives/libvir-list/2015-September/msg00698.html


Martin Kletzander (10):
  conf, virsh: Add new domain shutoff reason
  virsh: Refactor the table output of the list command
  virsh: Add support for list -reason
  qemu: Few whitespace cleanups
  conf: Extract name-parsing into its own function
  conf: Extract UUID parsing into its own function
  conf: Optionally keep domains with invalid XML, but don't allow
starting them
  qemu: Don't lookup invalid domains unless specified otherwise
  qemu: Prepare basic APIs to handle invalid defs
  qemu: Load domains with invalid XML on start

 include/libvirt/libvirt-domain.h |   2 +
 src/bhyve/bhyve_driver.c |   2 +
 src/conf/domain_conf.c   | 121 +++
 src/conf/domain_conf.h   |   7 +++
 src/conf/virdomainobjlist.c  |  71 +--
 src/conf/virdomainobjlist.h  |   1 +
 src/libvirt_private.syms |   1 +
 src/libxl/libxl_driver.c |   3 +
 src/lxc/lxc_driver.c |   3 +
 src/qemu/qemu_driver.c   |  64 +
 src/uml/uml_driver.c |   2 +
 tests/virshtest.c|  30 +++---
 tools/virsh-domain-monitor.c |  60 ---
 tools/virsh.pod  |   5 +-
 14 files changed, 303 insertions(+), 69 deletions(-)

--
2.6.3

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



Beyond the few noted spots changes look good to me. Implicit ACK for
those not specifically noted.



Thanks a lot, but there is still the security issue and a crash
mentioned by Luyao.  I know how to deal with only a part of it.  Anyway,
this will need another version, so I'll include all the nits pointed out
in there, but it'll take some time again because from my POV this is
just a nice-to-have feature and nobody is asking for this, so there are
other, more pressing, things in the priority queue and hence this one
will have to wait again.  Anyway, thanks again for checking this out.


John


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

Re: [libvirt] [PATCH v3 0/6] Add read error ignore to storage backend vol info APIs

[Ján Tomko]

On Fri, Dec 04, 2015 at 05:23:47PM -0500, John Ferlan wrote:
> v2 here:
> http://www.redhat.com/archives/libvir-list/2015-November/thread.html
> 
> Based on review and well just the amount of change I figured I'd
> generate a v3.  The patches were mostly ACK'd, but patch 3 required
> removal of some flags which resulted in patch 1 needing updates... 
> Then I found that what I'd done in patch 3 regarding returning errors
> needed to generate a patch 2.5... So I just bit the bullet and am
> posting the patches again.  Patches 2, 4, and 5 of this series are
> unchanged. While patch 1 removes 'readflags' from an API, patch 3
> is new, and patch 6 uses the new name.
> 
> John Ferlan (6):
>   storage: Add readflags for backend error processing
>   storage: Add comments for backend APIs
>   storage: Set ret = -1 on failures in
> virStorageBackendUpdateVolTargetInfo
>   storage: Handle readflags errors
>   storage: Add debug message
>   storage: Ignore block devices that fail format detection
> 
>  src/storage/storage_backend.c | 83 
> ++-
>  src/storage/storage_backend.h | 15 ++-
>  src/storage/storage_backend_disk.c|  5 ++-
>  src/storage/storage_backend_fs.c  |  4 +-
>  src/storage/storage_backend_logical.c |  2 +-
>  src/storage/storage_backend_mpath.c   |  2 +-
>  src/storage/storage_backend_scsi.c|  6 ++-
>  7 files changed, 96 insertions(+), 21 deletions(-)
> 

ACK series

Jan


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

Re: [libvirt] [PATCH 00/10] VFIO fixes for PCI devices

[Andrea Bolognani]

On Wed, 2015-12-02 at 18:17 +0100, Andrea Bolognani wrote:
> This series is my attempt at fixing
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=1272300
> 
[...]
> 
> The problem being solved is that, when using VFIO, IOMMU group
> ownership can't be shared, eg. two devices that are in the
> same IOMMU group can't be assigned to different guests, or to
> the host and a guest. If that happens, the host will probably
> crash.
> 
> The series deals with this issue by making sure safety
> conditions are met before detaching devices from the host or
> reattaching them to the host. In praticular, when we're asked
> to reattach a device to the host but doing so would lead to
> sharing IOMMU group ownership, we delay the operation until
> we can guarantee this will not cause problems. As a nice side
> effect of the changes we check for this when starting a guest
> too, instead of assuming it will work and having QEMU error
> out immediately afterwards.

Shivaprasad raised a concern on IRC, which I'm sharing here
for wider discussion. I'm CC'ing Laine and Alex, hopefully
they don't mind - let me know otherwise.

Assume we have a PCI device with two functions. With this
series applied, when reattaching both functions to the host
this would happen:

  f0 remove from guest
  f1 remove from guest
f0 unbind from vfio-pci
  f0 trigger host driver reprobe
f1 unbind from vfio-pci
  f1 trigger host driver reprobe

Shivaprasad is concerned this is not actually safe, and the
proper sequence would rather be:

  f0 remove from guest
  f1 remove from guest
f0 unbind from vfio-pci
f1 unbind from vfio-pci
  f0 trigger host driver reprobe
  f1 trigger host driver reprobe

Doing so would AFAICT mean basically duplicating the delay
logic this series adds to virHostdev into virPCI, to ensure
that devices are unbound from vfio-pci only once the same
operation has been requested for all devices in the IOMMU
group, and reprobe is triggered only after all devices have
been unbound from vfio-pci.

I was under the impression that what the current series
does, eg. sharing devices in the same IOMMU group between
the host driver and vfio-pci is safe as long as no guest is
using them at the same time, and that devices could be
safely "moved" between the host driver (eg. in use) and
vfio-pci (eg. idle, waiting to be assigned to a guest) as
many times as desired without ill consequences.

Is my understanding wrong? Do I need to rework the series
so that unbinds and reprobes are always executed across the
IOMMU group?

Any suggestion or pointers to relevant documentation will
be very much appreciated.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

Re: [libvirt] [PATCH 4/6] qemu: Use qemuDomainAdjustMaxMemLock()

[John Ferlan]

On 11/24/2015 08:56 AM, Andrea Bolognani wrote:
> Replace all uses of the qemuDomainRequiresMlock/virProcessSetMaxMemLock
> combination with the equivalent qemuDomainAdjustMaxMemLock() call.
> ---
>  src/qemu/qemu_hotplug.c | 40 +---
>  1 file changed, 13 insertions(+), 27 deletions(-)
> 

OK - so now I see how this is going to be used... patch 3's multi
purpose MaxMemLock makes a bit more sense; however, I'm still wondering
how we'd ever get to a point where the a last removal wouldn't have set
the lockbytes value to anything but the original (yes, patch 5 is
missing from the current removal)...

Seems like perhaps the original_memlock is fixing the bug that was shown
in patch 5 - that the hostdev removal didn't return our value properly.

IOW: If the original_memlock adjustment was taken out, is there a case
(after patch 5 is applied) where when memory locking is no longer
required that the value after the removal wouldn't have been what was
saved in original_memlock.

Perhaps would have been easier to have added the Adjust code without
original_memlock, then replace code as is done here, then add the
hostdev case, then add the original value. Just trying to separate new
functionality from code motion/creation of helper code.

> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 8804d3d..a5c134a 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1276,17 +1276,14 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver,
>  }
>  
>  /* Temporarily add the hostdev to the domain definition. This is needed
> - * because qemuDomainRequiresMlock() and qemuDomainGetMlockLimitBytes()
> - * require the hostdev to be already part of the domain definition, but
> - * other functions like qemuAssignDeviceHostdevAlias() used below expect
> - * it *not* to be there. A better way to handle this would be nice */
> + * because qemuDomainAdjustMaxMemLock() requires the hostdev to be 
> already
> + * part of the domain definition, but other functions like
> + * qemuAssignDeviceHostdevAlias() used below expect it *not* to be there.
> + * A better way to handle this would be nice */
>  vm->def->hostdevs[vm->def->nhostdevs++] = hostdev;
> -if (qemuDomainRequiresMlock(vm->def)) {
> -if (virProcessSetMaxMemLock(vm->pid,
> -qemuDomainGetMlockLimitBytes(vm->def)) < 
> 0) {
> -vm->def->hostdevs[--(vm->def->nhostdevs)] = NULL;
> -goto error;
> -}
> +if (qemuDomainAdjustMaxMemLock(vm) < 0) {
> +vm->def->hostdevs[--(vm->def->nhostdevs)] = NULL;
> +goto error;
>  }
>  vm->def->hostdevs[--(vm->def->nhostdevs)] = NULL;
>  
> @@ -1772,7 +1769,6 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
>  virJSONValuePtr props = NULL;
>  virObjectEventPtr event;
>  bool fix_balloon = false;
> -bool mlock = false;
>  int id;
>  int ret = -1;
>  
> @@ -1804,12 +1800,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
>  goto cleanup;
>  }
>  
> -mlock = qemuDomainRequiresMlock(vm->def);
> -
> -if (mlock &&
> -virProcessSetMaxMemLock(vm->pid,
> -qemuDomainGetMlockLimitBytes(vm->def)) < 0) {
> -mlock = false;
> +if (qemuDomainAdjustMaxMemLock(vm) < 0) {

The current code says don't run the "reset" code if the SetMaxMemLock
failed... With this change, in the removedef code you'll call the
AdjustMaxMemLock regardless of whether we failed to set.  That fail to
set (at this point in time) could be the "GetMaxMemLock" failed or the
"SetMaxMemLock" failed.

The point being at removedef, how do we know if the failure was because
we failed to Adjust or something else.  If we failed to Adjust, then
calling it again will of course more than likely fail.


John
>  virJSONValueFree(props);
>  goto removedef;
>  }
> @@ -1870,13 +1861,10 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
>  mem = NULL;
>  
>  /* reset the mlock limit */
> -if (mlock) {
> -virErrorPtr err = virSaveLastError();
> -ignore_value(virProcessSetMaxMemLock(vm->pid,
> - 
> qemuDomainGetMlockLimitBytes(vm->def)));
> -virSetError(err);
> -virFreeError(err);
> -}
> +virErrorPtr err = virSaveLastError();
> +ignore_value(qemuDomainAdjustMaxMemLock(vm));
> +virSetError(err);
> +virFreeError(err);
>  
>  goto audit;
>  }
> @@ -2970,9 +2958,7 @@ qemuDomainRemoveMemoryDevice(virQEMUDriverPtr driver,
>  virDomainMemoryDefFree(mem);
>  
>  /* decrease the mlock limit after memory unplug if necessary */
> -if (qemuDomainRequiresMlock(vm->def))
> -ignore_value(virProcessSetMaxMemLock(vm->pid,
> - 
> qemuDomainGetMlockLimitBytes(vm->def)));
> +

[libvirt] [PATCH 02/10] tests: introduce VIR_TEST_REGENERATE_OUTPUT flag

[Pavel Hrdina]

When this flag is specified, some of the expected output files will be
regenerated with the actual output data.  This is a helper for updating
test data.

Signed-off-by: Pavel Hrdina 
---
 tests/testutils.c | 14 +++---
 tests/testutils.h |  1 +
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/tests/testutils.c b/tests/testutils.c
index 857e819..2b0d3b6 100644
--- a/tests/testutils.c
+++ b/tests/testutils.c
@@ -67,6 +67,7 @@ VIR_LOG_INIT("tests.testutils");
 static unsigned int testDebug = -1;
 static unsigned int testVerbose = -1;
 static unsigned int testExpensive = -1;
+static unsigned int testRegenerate = -1;
 
 #ifdef TEST_OOM
 static unsigned int testOOM;
@@ -598,9 +599,8 @@ virtTestCompareToFile(const char *strcontent,
 int ret = -1;
 char *filecontent = NULL;
 char *fixedcontent = NULL;
-bool regenerate = !!virTestGetFlag("VIR_TEST_REGENERATE_OUTPUT");
 
-if (virtTestLoadFile(filename, ) < 0 && !regenerate)
+if (virtTestLoadFile(filename, ) < 0 && 
!virTestGetRegenerate())
 goto failure;
 
 if (filecontent &&
@@ -612,7 +612,7 @@ virtTestCompareToFile(const char *strcontent,
 
 if (STRNEQ_NULLABLE(fixedcontent ? fixedcontent : strcontent,
 filecontent)) {
-if (regenerate) {
+if (virTestGetRegenerate()) {
 if (virFileWriteStr(filename, strcontent, 0666) < 0)
 goto failure;
 goto out;
@@ -716,6 +716,14 @@ virTestGetExpensive(void)
 return testExpensive;
 }
 
+unsigned int
+virTestGetRegenerate(void)
+{
+if (testRegenerate == -1)
+testRegenerate = virTestGetFlag("VIR_TEST_REGENERATE_OUTPUT");
+return testRegenerate;
+}
+
 int virtTestMain(int argc,
  char **argv,
  int (*func)(void))
diff --git a/tests/testutils.h b/tests/testutils.h
index ccf1d29..8ef70e4 100644
--- a/tests/testutils.h
+++ b/tests/testutils.h
@@ -77,6 +77,7 @@ int virtTestCompareToFile(const char *strcontent,
 unsigned int virTestGetDebug(void);
 unsigned int virTestGetVerbose(void);
 unsigned int virTestGetExpensive(void);
+unsigned int virTestGetRegenerate(void);
 
 # define VIR_TEST_DEBUG(...)\
 do {\
-- 
2.6.3

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

[libvirt] [PATCH 00/10] various test fixes and input devices handling

[Pavel Hrdina]

This patch series is a follow up for [1].  The first patch fixes one missed path
for calling virDomainDefPostParse after parsing driver specific configuration.

Then there is a bunch of tests improvements to help developers handle large
updates of expected output files.  It helped me with the last patch, that
updates XML files after the input device fixes.

And there is a v2 of intput device cleanup patch, now it add implicit input
devices into config XML and uses driver's post parse functions to do that.

[1] https://www.redhat.com/archives/libvir-list/2015-December/msg00188.html

Pavel Hrdina (10):
  xen: move virDomainDefPostParse to xenParseSxpr
  tests: introduce VIR_TEST_REGENERATE_OUTPUT flag
  tests.testutils: use VIR_TEST_REGENERATE_OUTPUT for
virTestDifferenceFull
  tests.testutils: use virTestDifferenceFull in virtTestCompareToFile
  tests: use virtTestDifferenceFull in tests where we have output file
  tests: skip regeneration for problematic test
  tests: regenerate all outputs using VIR_TEST_REGENERATE_OUTPUT flag
  tests.nwfilterebiptablestest: swap actual and expected
  device: cleanup input device code
  tests: update test XML files to follow input changes

 src/Makefile.am|   4 +-
 src/conf/domain_conf.c |  77 +---
 src/libxl/libxl_domain.c   |   5 +
 src/qemu/qemu_domain.c |  22 
 src/xen/xen_driver.c   |   5 +
 src/xen/xend_internal.c|   6 +-
 src/xenapi/xenapi_driver.c |   5 +
 src/xenconfig/xen_common.c |  22 
 src/xenconfig/xen_common.h |   2 +
 src/xenconfig/xen_sxpr.c   |  22 ++--
 src/xenconfig/xen_sxpr.h   |   9 +-
 .../disk_snapshot_redefine.xml |   2 +
 .../external_vm_redefine.xml   |   2 +
 tests/domainsnapshotxml2xmlout/full_domain.xml |   2 +
 tests/domainsnapshotxml2xmlout/metadata.xml|   2 +
 tests/domainsnapshotxml2xmltest.c  |   2 +-
 tests/interfacexml2xmltest.c   |   2 +-
 tests/lxcconf2xmltest.c|   2 +-
 tests/nodedevxml2xmltest.c |   2 +-
 tests/nwfilterebiptablestest.c |  14 +--
 tests/qemuargv2xmltest.c   |  18 ++-
 tests/qemuhotplugtest.c|  11 +-
 .../qemuhotplug-hotplug-base+disk-scsi.xml |   2 +
 .../qemuhotplug-hotplug-base+disk-usb.xml  |   2 +
 .../qemuhotplug-hotplug-base+disk-virtio.xml   |   2 +
 tests/qemuxml2argvdata/qemuxml2argv-bios-nvram.xml |   2 +
 .../qemuxml2argvdata/qemuxml2argv-blkdeviotune.xml |   2 +
 .../qemuxml2argv-blkiotune-device.xml  |   2 +
 tests/qemuxml2argvdata/qemuxml2argv-blkiotune.xml  |   2 +
 tests/qemuxml2argvdata/qemuxml2argv-boot-cdrom.xml |   6 +-
 .../qemuxml2argvdata/qemuxml2argv-boot-floppy.xml  |   6 +-
 .../qemuxml2argv-boot-menu-disable.xml |   2 +
 .../qemuxml2argv-boot-menu-enable-with-timeout.xml |   2 +
 tests/qemuxml2argvdata/qemuxml2argv-boot-multi.xml |   2 +
 .../qemuxml2argvdata/qemuxml2argv-boot-network.xml |   6 +-
 tests/qemuxml2argvdata/qemuxml2argv-boot-order.xml |   2 +
 .../qemuxml2argv-channel-guestfwd.xml  |   2 +
 .../qemuxml2argv-channel-virtio.xml|   2 +
 .../qemuxml2argv-chardev-label.xml |   2 +
 .../qemuxml2argv-clock-catchup.xml |   2 +
 .../qemuxml2argv-clock-localtime.xml   |   6 +-
 .../qemuxml2argv-clock-timer-hyperv-rtc.xml|   2 +
 tests/qemuxml2argvdata/qemuxml2argv-clock-utc.xml  |   6 +-
 .../qemuxml2argv-console-compat.xml|   6 +-
 .../qemuxml2argv-console-virtio-many.xml   |   2 +
 .../qemuxml2argv-cpu-eoi-disabled.xml  |   2 +
 .../qemuxml2argv-cpu-eoi-enabled.xml   |   2 +
 .../qemuxml2argv-cpu-host-kvmclock.xml |   2 +
 .../qemuxml2argv-cpu-host-model-features.xml   |   2 +
 .../qemuxml2argv-cpu-host-passthrough-features.xml |   2 +
 .../qemuxml2argvdata/qemuxml2argv-cpu-kvmclock.xml |   2 +
 .../qemuxml2argv-cpu-numa-disjoint.xml |   2 +
 .../qemuxml2argv-cpu-numa-memshared.xml|   2 +
 ...xml2argv-cputune-iothreadsched-zeropriority.xml |   2 +
 .../qemuxml2argv-cputune-numatune.xml  |   2 +
 .../qemuxml2argv-cputune-zero-shares.xml   |   2 +
 tests/qemuxml2argvdata/qemuxml2argv-cputune.xml|   2 +
 .../qemuxml2argv-disk-active-commit.xml|   2 +
 tests/qemuxml2argvdata/qemuxml2argv-disk-aio.xml   |   2 +
 .../qemuxml2argv-disk-cdrom-empty.xml  |   6 +-
 tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom.xml |   6 +-
 .../qemuxml2argv-disk-copy_on_read.xml |   2 +
 

[libvirt] [PATCH 07/10] tests: regenerate all outputs using VIR_TEST_REGENERATE_OUTPUT flag

[Pavel Hrdina]

Since more tests supports regenerating its output, this patch updates
those expected output files.

Signed-off-by: Pavel Hrdina 
---
 tests/qemuxml2argvdata/qemuxml2argv-boot-cdrom.xml  | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-boot-floppy.xml | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-boot-network.xml| 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-clock-localtime.xml | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-clock-utc.xml   | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-console-compat.xml  | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-empty.xml| 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom.xml  | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-boot-cdrom.xml   | 5 +++--
 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-boot-disk.xml| 5 +++--
 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-none.xml| 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-policy.xml | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-sasl.xml   | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-socket.xml | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-tls.xml| 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-websocket.xml  | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc.xml| 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-hostdev-pci-address.xml | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-hyperv-panic.xml| 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-hyperv.xml  | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-kvm-features.xml| 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-kvmclock.xml| 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-machine-aeskeywrap-off-argv.xml | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-machine-aeskeywrap-on-argv.xml  | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-machine-core-off.xml| 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-machine-core-on.xml | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-machine-deakeywrap-off-argv.xml | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-machine-deakeywrap-on-argv.xml  | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-machine-keywrap-none-argv.xml   | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-misc-disable-s3.xml | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-misc-disable-suspends.xml   | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-misc-enable-s4.xml  | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-misc-no-reboot.xml  | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-net-eth-ifname.xml  | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-net-eth.xml | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-nosharepages.xml| 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-parallel-tcp.xml| 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml| 2 +-
 tests/qemuxml2argvdata/qemuxml2argv-reboot-timeout-disabled.xml | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-reboot-timeout-enabled.xml  | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-serial-dev.xml  | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-telnet.xml   | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp.xml  | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-serial-vc.xml   | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-watchdog.xml| 4 ++--
 45 files changed, 91 insertions(+), 89 deletions(-)

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-boot-cdrom.xml 
b/tests/qemuxml2argvdata/qemuxml2argv-boot-cdrom.xml
index b639821..d36e9a9 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-boot-cdrom.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-boot-cdrom.xml
@@ -1,8 +1,8 @@
 
   QEMUGuest1
   c7a5fdbd-edaf-9455-926a-d65c16db1809
-  219100
-  219100
+  219136
+  219136
   1
   
 hvm
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-boot-floppy.xml 
b/tests/qemuxml2argvdata/qemuxml2argv-boot-floppy.xml
index 610321f..e2dfa6d 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-boot-floppy.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-boot-floppy.xml
@@ -1,8 +1,8 @@
 
   QEMUGuest1
   c7a5fdbd-edaf-9455-926a-d65c16db1809
-  219100
-  219100
+  219136
+  219136
   1
   
 hvm
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-boot-network.xml 
b/tests/qemuxml2argvdata/qemuxml2argv-boot-network.xml
index f4ebc2e..58c98f1 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-boot-network.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-boot-network.xml
@@ -1,8 +1,8 @@
 
   QEMUGuest1
   c7a5fdbd-edaf-9455-926a-d65c16db1809
-  219100
-  219100
+  219136
+  219136
   1
   
 hvm
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-clock-localtime.xml 

Re: [libvirt] [Qemu-devel] FD passing for chardevs and chardev backend multiplexing

[Daniel P. Berrange]

On Tue, Dec 08, 2015 at 10:04:55AM -0700, Eric Blake wrote:
> On 12/08/2015 07:59 AM, Daniel P. Berrange wrote:
> 
> > So for this my plan is to stop using the QEMU 'file' backend for char
> > devs and instead pass across a pre-opened file descriptor, connected
> > to virtlogd. There is no "officially documented" way to pass in a
> > file descriptor to QEMU chardevs, but since QEMU uses qemu_open(),
> > we can make use of the fdset feature to achieve this. eg
> > 
> > eg, consider fd 33 is the write end of a pipe file descriptor
> > I can (in theory) do
> > 
> >   -add-fd set=2,fd=33 -chardev file,id=charserial0,path=/dev/fdset/2
> > 
> > Now in practice this doesn't work, because qmp_chardev_open_file()
> > passes the O_CREAT|O_TRUNC flags in, which means the qemu_open()
> > call will fail when using the pipe FD pased in via fdsets.
> 
> Is it just the O_TRUNC that is failing? If so, there is a recent patch
> to add an 'append':true flag that switches O_TRUNC off in favor of O_APPEND:
> https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg00762.html

Yes, it is the ftruncate() call in qemu_dup_flags, called from qemu_open
that is failing.

> Or is it that the pipe is one-way, but chardev insists on O_RDWR and
> fails because it is not two-way?

The chardev file: backend wants a O_RDONLY file - it won't accept
an O_RDWR file in fact, so we must use a pipe with it.

> > After more investigation I found it *is* possible to use a socketpair
> > and a pipe backend though...
> > 
> >   -add-fd set=2,fd=33 -chardev pipe,id=charserial0,path=/dev/fdset/2
> 
> Yes, a socketpair is bi-directional, so it supports O_RDWR opening.

Yep.

> > ..because for reasons I don't understand, if QEMU can't open $PATH.in
> > and $PATH.out, it'll fallback to just opening $PATH in read-write
> > mode even. AFAICT, this is pretty useless with pipes since they
> > are unidirectional, but, it works nicely with socketpairs, where
> > virtlogd has one of the socketpairs and QEMU gets passed the other
> > via fdset.
> 
> Is it something where we'd want to support two pipes, and open
> /dev/fdset/2 tied to char.in and /dev/fdset/3 tied to char.out, where
> uni-directional pipes are again okay?

In theory we could do, but it would need us to special case the
code, as just taking  '/dev/fdset/2' and appending '.in' obviously
doesn't work. I don't think this really matters though - using a
socketpair is just fine.

> > I can easily check this works for historical QEMU versions back
> > to when fdsets support was added to chardevs, but I'm working if
> > the QEMU maintainers consider this usage acceptable over the long
> > term, and if so, should we explicitly document it as supported ?
> 
> It seems like a bi-directional socketpair as the single endpoint for a
> chardev is useful enough to support and document, but I'm not the
> maintainer to give final say-so.
> 
> > 
> > If not, should we introduce a more explicit syntax for passing in
> > a pre-opened FD for chardevs ? eg
> > 
> >   -add-fd set=2,fd=33 -chardev fd,id=charserial0,path=/dev/fdset/2
> > 
> 
> Difference to the line you tried above:
> 
> >   -add-fd set=2,fd=33 -chardev file,id=charserial0,path=/dev/fdset/2
> 
> is 'fd' instead of 'file'.  But if we're going to add a new protocol, do
> we even need to go through the "/dev/fdset/..." name, or can we just
> pass the fd number directly?
> 
> > Or just make  -chardev file,id=charserial0,path=/dev/fdset/2 actually
> > work ?
> 
> I'd lean more to this case - the whole point of fdsets was that we don't
> have to add multiple fd protocols; that everyone that understood file
> syntax and uses qemu_open() magically gained fd support.

Yeah, that is a good point about not inventing multiple fd protocols.
>From that POV I'd be happy enough if we documented & supported that
'pipe' can be used with a socketpair, and 'file' can be used with
an pipe (once append=true support added)


> > eg should we make something like this work:
> > 
> >   -add-fd set=2,fd=33
> >   -chardev pipe,id=charserial0file,path=/dev/fdset/2
> >   -chardev 
> > socket,id=charserial0tcp,host=127.0.0.1,port=,telnet,server,nowait
> >   -chardev multiplex,id=charserial0,muxA=charserial0file,muxB=charserial1
> 
> wouldn't muxB be charserial0tcp (not charserial1)?

Yes, silly typo.

> 
> >   -serial isa-serial,chardev=charserial0,id=serial0
> 
> But the idea of a multiplex protocol that has multiple data sinks (guest
> output copied to all sinks) and a single source (at most one source can
> provide input to the guest) makes sense on the surface.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

--
libvir-list mailing list
libvir-list@redhat.com

[libvirt] [PATCH 0/5] virStorageBackendWipeLocal cleanups

[Ján Tomko]

The first patch fixes the return values of virStorageWipe on non-sparse local 
files
in the case of a partial wipe or a fdatasync error.

The rest reduces the number of parameters of 
virStorageBackendWipe{Extent,}Local.

Ján Tomko (5):
  storage: fix return values of virStorageBackendWipeExtentLocal
  storage: move buffer allocation inside
virStorageBackendWipeExtentLocal
  storage: drop 'Extent' from virStorageBackendWipeExtentLocal
  virStorageBackendWipeLocal: use unsigned long long instead of off_t
  virStorageBackendWipeLocal: remove bytes_wiped argument

 src/storage/storage_backend.c | 53 +--
 1 file changed, 21 insertions(+), 32 deletions(-)

-- 
2.4.6

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

Re: [libvirt] How should libvirt apps enable virtio-pci for aarch64?

[Pavel Fedin]

 Hello!

> 00:04:0 is VFIO passthrough, and i put everything to bus#0 by hands, for 
> MSI-X to work. I
> could also leave devices at bus#2, just in
> this case i don't get MSI-X, and this, for example, makes vhost-net unable to 
> use irqfds (or
> it cannot initialize at all, again, i
> already don't remember), because this requires per-event irqs.

 Just tested Q35 machine on x86-64, and retested the same on ARM64. Sorry, but 
please ignore what i said before about MSI-X.
Everything works fine, MSI-X is OK on bus#2 after the bridge, and this was 
either some old bug in old qemu, or i was simply doing
something crappy and wrong.
 So, i don't care anymore about this bridge complex on PCIe bus, it does not 
make my living any worse. If libvirt strictly
distinguishes between PCI and PCIe, let it just be there.

 And, to complete the testing, i've just migrated the Q35 machine and the 
bridge also breaks down. 'lspci -v' starts reporting
"Unknown header type: 7f" for the bridge and for all devices behind it (and all 
devices fail). But it just means it should be fixed
in qemu.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia


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

Re: [libvirt] [PATCH] hypervisor driver for Jailhouse

[Daniel P. Berrange]

On Wed, Nov 25, 2015 at 02:39:17PM +0100, Christian Loehle wrote:
> The suggestions in your review should all be implemented now.


NB, as mentioned on IRC, for future postings please send as
a top-level patch, rather than in-reply to, and use
--subject-prefix 'PATCH v3' to indicate the 3rd posting
for example.


> diff --git a/libvirt-jailhousev2.patch b/libvirt-jailhousev2.patch
> new file mode 100644
> index 000..e69de29

This file probably shouldn't have been added

> diff --git a/m4/virt-driver-jailhouse.m4 b/m4/virt-driver-jailhouse.m4
> new file mode 100644
> index 000..65d53f2
> --- /dev/null
> +++ b/m4/virt-driver-jailhouse.m4
> @@ -0,0 +1,31 @@
> +
> +AC_DEFUN([LIBVIRT_DRIVER_CHECK_JAILHOUSE],[
> +AC_ARG_WITH([jailhouse],
> +  [AS_HELP_STRING([--with-jailhouse],
> +[add Jailhouse support @<:@default=yes@:>@])])
> +m4_divert_text([DEFAULTS], [with_jailhouse=yes])
> +

You need

if test "$with_jailhouse" = "yes"; then
AC_DEFINE_UNQUOTED([WITH_JAILHOUSE], 1, [whether jailhouse driver is 
enabled])
fi

Without this, your code never gets enabled in libvirt.so,
so I'm not sure how you tested this patch, as the driver
is never loaded.

> +AM_CONDITIONAL([WITH_JAILHOUSE], [test "$with_jailhouse" = "yes"])
> +])
> +
> +AC_DEFUN([LIBVIRT_DRIVER_RESULT_JAILHOUSE],[
> +AC_MSG_NOTICE([Jailhouse: $with_jailhouse])
> +])

> diff --git a/src/jailhouse/README b/src/jailhouse/README
> new file mode 100644
> index 000..02ba87d
> --- /dev/null
> +++ b/src/jailhouse/README
> @@ -0,0 +1,14 @@
> +The jailhouse hypervisor driver for the libvirt project aims to provide
> +rudimentary support for managing jailhouse with the libvirt library.
> +The main advantage of this is the possibility to use virt-manager as a GUI to
> +manage Jailhouse cells. Thus the driver is mainly built around the API calls
> +that virt-manager uses and needs.
> +Due to the concept of Jailhouse a lot of libvirt functions can't be realized,
> +so this driver isn't as full-featured as upstream drivers of the
> +libvirt project.
> +Currently the driver relies on the Jailhouse binary, which has to be in $PATH
> +or passed when connecting a libvirt client to it
> +(e.g. virt-manager -c jailhouse:///path/to/jailhouse/tools/jailhouse).

As mentioned before, I really don't think we should be passing binary
names in the URI like this. For everything else in libvirt we just
rely on the binary being in $PATH. If someone has put jailhouse in
some unusual location, then can just pre-pend that location to $PATH

> +#define STATERUNNINGSTRING  "running "
> +#define STATERUNNINGLOCKED 1
> +#define STATERUNNINGLOCKEDSTRING"running/locked  "
> +#define STATESHUTDOWN 2
> +#define STATESHUTDOWNSTRING "shut down   "
> +#define STATEFAILED 3
> +#define STATEFAILEDSTRING   "failed  "

Looks

> +#define JAILHOUSEVERSIONOUTPUT  "Jailhouse management tool"
> +
> +struct virJailhouseCell {
> +int id;
> +char name[NAMELENGTH+1];
> +int state;
> +virBitmapPtr assignedCPUs;
> +virBitmapPtr failedCPUs; /* currently unused */
> +unsigned char uuid[VIR_UUID_BUFLEN];
> +};
> +typedef struct virJailhouseCell virJailhouseCell;
> +typedef virJailhouseCell *virJailhouseCellPtr;
> +
> +/*
> + *  Because virCommandRunRegex Callback gets called every line
> + */
> +struct virJailhouseCellCallbackData {
> +size_t ncells;
> +virJailhouseCellPtr cells;
> +};
> +typedef struct virJailhouseCellCallbackData virJailhouseCellCallbackData;
> +typedef virJailhouseCellCallbackData *virJailhouseCellCallbackDataPtr;
> +
> +/*
> + *  The driver requeries the cells on most calls, it stores the result of the
> + *  last query, so it can copy the UUIDs in the new query if the cell is the
> + *  same(otherwise it just generates a new one).
> + *  not preserving the UUID results in a lot of bugs in libvirts clients.
> + */
> +struct virJailhouseDriver {
> +char *binary;
> +size_t lastQueryCellsCount;
> +virJailhouseCellPtr lastQueryCells;
> +};
> +typedef struct virJailhouseDriver virJailhouseDriver;
> +typedef virJailhouseDriver *virJailhouseDriverPtr;
> +
> +static int virJailhouseParseListOutputCallback(char **const groups, void 
> *data)
> +{
> +virJailhouseCellCallbackDataPtr celldata = 
> (virJailhouseCellCallbackDataPtr) data;
> +virJailhouseCellPtr cells = celldata->cells;
> +size_t count = celldata->ncells;
> +char* endptr = groups[0] + strlen(groups[0]) - 1;
> +char* state = groups[2];
> +if (cells == NULL) {
> +if (VIR_ALLOC(cells))
> +return -1;
> +}
> +else if (VIR_EXPAND_N(cells, count, 1))
> +return -1;

You are not incrementing 'count' in the first patch of the
conditional, but you are incrementing it in the second.
As a result, as soon as you have more than one cell running
the following code will be a buffer overflow causing libvirt
to crash.  VIR_EXPAND_N copes just fine with 

[libvirt] [PATCH v2 7/7] qemu: Replace Mlock with MemLock in function names

[Andrea Bolognani]

MemLock is already used in other modules and, while still an
abbreviation, is not ambiguous.
---
 src/qemu/qemu_command.c |  4 ++--
 src/qemu/qemu_domain.c  | 10 +-
 src/qemu/qemu_domain.h  |  4 ++--
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index d2f37e4..692f088 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -11262,8 +11262,8 @@ qemuBuildCommandLine(virConnectPtr conn,
 
 /* In some situations, eg. VFIO passthrough, QEMU might need to lock a
  * significant amount of memory, so we need to set the limit accordingly */
-if (qemuDomainRequiresMlock(def))
-virCommandSetMaxMemLock(cmd, qemuDomainGetMlockLimitBytes(def));
+if (qemuDomainRequiresMemLock(def))
+virCommandSetMaxMemLock(cmd, qemuDomainGetMemLockLimitBytes(def));
 
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_MSG_TIMESTAMP) &&
 cfg->logTimestamp)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 1e1e57f..bb8d47f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3964,7 +3964,7 @@ qemuDomainUpdateCurrentMemorySize(virQEMUDriverPtr driver,
 
 
 /**
- * qemuDomainGetMlockLimitBytes:
+ * qemuDomainGetMemLockLimitBytes:
  *
  * @def: domain definition
  *
@@ -3974,7 +3974,7 @@ qemuDomainUpdateCurrentMemorySize(virQEMUDriverPtr driver,
  * value returned may depend upon the architecture or devices present.
  */
 unsigned long long
-qemuDomainGetMlockLimitBytes(virDomainDefPtr def)
+qemuDomainGetMemLockLimitBytes(virDomainDefPtr def)
 {
 unsigned long long memKB;
 
@@ -4095,7 +4095,7 @@ qemuDomainGetMlockLimitBytes(virDomainDefPtr def)
  * requirements.
  * */
 bool
-qemuDomainRequiresMlock(virDomainDefPtr def)
+qemuDomainRequiresMemLock(virDomainDefPtr def)
 {
 size_t i;
 
@@ -4138,7 +4138,7 @@ qemuDomainAdjustMaxMemLock(virDomainObjPtr vm)
 unsigned long long bytes = 0;
 int ret = -1;
 
-if (qemuDomainRequiresMlock(vm->def)) {
+if (qemuDomainRequiresMemLock(vm->def)) {
 /* If this is the first time adjusting the limit, save the current
  * value so that we can restore it once memory locking is no longer
  * required. Failing to obtain the current limit is not a critical
@@ -4147,7 +4147,7 @@ qemuDomainAdjustMaxMemLock(virDomainObjPtr vm)
 if (virProcessGetMaxMemLock(vm->pid, &(vm->original_memlock)) < 0)
 vm->original_memlock = 0;
 }
-bytes = qemuDomainGetMlockLimitBytes(vm->def);
+bytes = qemuDomainGetMemLockLimitBytes(vm->def);
 } else {
 /* Once memory locking is no longer required, we can restore the
  * original, usually very low, limit */
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 704351b..cff48d7 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -498,8 +498,8 @@ bool qemuDomainMachineHasBuiltinIDE(const virDomainDef 
*def);
 int qemuDomainUpdateCurrentMemorySize(virQEMUDriverPtr driver,
   virDomainObjPtr vm);
 
-unsigned long long qemuDomainGetMlockLimitBytes(virDomainDefPtr def);
-bool qemuDomainRequiresMlock(virDomainDefPtr def);
+unsigned long long qemuDomainGetMemLockLimitBytes(virDomainDefPtr def);
+bool qemuDomainRequiresMemLock(virDomainDefPtr def);
 int qemuDomainAdjustMaxMemLock(virDomainObjPtr vm);
 
 int qemuDomainDefValidateMemoryHotplug(const virDomainDef *def,
-- 
2.5.0

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

[libvirt] [PATCH v2 1/7] process: Allow virProcessPrLimit() to get current limit

[Andrea Bolognani]

The prlimit() function allows both getting and setting limits for
a process; expose the same functionality in our wrapper.

Add the const modifier for new_limit, in accordance with the
prototype for prlimit().
---
 src/util/virprocess.c | 16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 4b18903..9b38834 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -725,15 +725,19 @@ int virProcessSetNamespaces(size_t nfdlist,
 
 #if HAVE_PRLIMIT
 static int
-virProcessPrLimit(pid_t pid, int resource, struct rlimit *rlim)
+virProcessPrLimit(pid_t pid,
+  int resource,
+  const struct rlimit *new_limit,
+  struct rlimit *old_limit)
 {
-return prlimit(pid, resource, rlim, NULL);
+return prlimit(pid, resource, new_limit, old_limit);
 }
 #elif HAVE_SETRLIMIT
 static int
 virProcessPrLimit(pid_t pid ATTRIBUTE_UNUSED,
   int resource ATTRIBUTE_UNUSED,
-  struct rlimit *rlim ATTRIBUTE_UNUSED)
+  const struct rlimit *new_limit ATTRIBUTE_UNUSED,
+  struct rlimit *old_limit ATTRIBUTE_UNUSED)
 {
 errno = ENOSYS;
 return -1;
@@ -758,7 +762,7 @@ virProcessSetMaxMemLock(pid_t pid, unsigned long long bytes)
 return -1;
 }
 } else {
-if (virProcessPrLimit(pid, RLIMIT_MEMLOCK, ) < 0) {
+if (virProcessPrLimit(pid, RLIMIT_MEMLOCK, , NULL) < 0) {
 virReportSystemError(errno,
  _("cannot limit locked memory "
"of process %lld to %llu"),
@@ -803,7 +807,7 @@ virProcessSetMaxProcesses(pid_t pid, unsigned int procs)
 return -1;
 }
 } else {
-if (virProcessPrLimit(pid, RLIMIT_NPROC, ) < 0) {
+if (virProcessPrLimit(pid, RLIMIT_NPROC, , NULL) < 0) {
 virReportSystemError(errno,
  _("cannot limit number of subprocesses "
"of process %lld to %u"),
@@ -851,7 +855,7 @@ virProcessSetMaxFiles(pid_t pid, unsigned int files)
 return -1;
 }
 } else {
-if (virProcessPrLimit(pid, RLIMIT_NOFILE, ) < 0) {
+if (virProcessPrLimit(pid, RLIMIT_NOFILE, , NULL) < 0) {
 virReportSystemError(errno,
  _("cannot limit number of open files "
"of process %lld to %u"),
-- 
2.5.0

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

[libvirt] how to pass qemu drive option

[Vasiliy Tolstov]

I want to pass to my drive detect-zeros=on how can i do that in libvirt?
I'm use lvm with virtio-scsi for sda disk.

-- 
Vasiliy Tolstov,
e-mail: v.tols...@selfip.ru

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

[libvirt] [PATCH v2 6/7] qemu: Allow qemuDomainAdjustMaxMemLock() to restore previous value

[Andrea Bolognani]

When the function changes the memory lock limit for the first time,
it will retrieve the current value and store it inside the
virDomainObj for the domain.

When the function is called again, if memory locking is no longer
needed, it will be able to restore the memory locking limit to its
original value.
---
 src/conf/domain_conf.h |  3 +++
 src/qemu/qemu_domain.c | 21 +++--
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index cec681a..952d3cc 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2420,6 +2420,9 @@ struct _virDomainObj {
 void (*privateDataFreeFunc)(void *);
 
 int taint;
+
+unsigned long long original_memlock; /* Original RLIMIT_MEMLOCK, zero if no
+  * restore will be required later */
 };
 
 typedef bool (*virDomainObjListACLFilter)(virConnectPtr conn,
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 4b796ab..1e1e57f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4125,7 +4125,10 @@ qemuDomainRequiresMlock(virDomainDefPtr def)
  * Adjust the memory locking limit for the QEMU process associated to @vm, in
  * order to comply with VFIO or architecture requirements.
  *
- * The limit will not be changed unless doing so is needed.
+ * The limit will not be changed unless doing so is needed; the first time
+ * the limit is changed, the original (default) limit is stored in @vm and
+ * that value will be restored if qemuDomainAdjustMaxMemLock() is called once
+ * memory locking is no longer required.
  *
  * Returns: 0 on success, <0 on failure
  */
@@ -4135,8 +4138,22 @@ qemuDomainAdjustMaxMemLock(virDomainObjPtr vm)
 unsigned long long bytes = 0;
 int ret = -1;
 
-if (qemuDomainRequiresMlock(vm->def))
+if (qemuDomainRequiresMlock(vm->def)) {
+/* If this is the first time adjusting the limit, save the current
+ * value so that we can restore it once memory locking is no longer
+ * required. Failing to obtain the current limit is not a critical
+ * failure, it just means we'll be unable to lower it later */
+if (!vm->original_memlock) {
+if (virProcessGetMaxMemLock(vm->pid, &(vm->original_memlock)) < 0)
+vm->original_memlock = 0;
+}
 bytes = qemuDomainGetMlockLimitBytes(vm->def);
+} else {
+/* Once memory locking is no longer required, we can restore the
+ * original, usually very low, limit */
+bytes = vm->original_memlock;
+vm->original_memlock = 0;
+}
 
 /* Trying to set the memory locking limit to zero is a no-op */
 if (virProcessSetMaxMemLock(vm->pid, bytes) < 0)
-- 
2.5.0

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

[libvirt] [PATCH 3/5] storage: drop 'Extent' from virStorageBackendWipeExtentLocal

[Ján Tomko]

The only caller always passes 0 for the extent start.
Drop the 'extent_start' parameter, as well as the mention of extents
from the function name.
---
 src/storage/storage_backend.c | 32 +++-
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 120d654..d1276dd 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -1987,29 +1987,28 @@ 
virStorageBackendVolZeroSparseFileLocal(virStorageVolDefPtr vol,
 
 
 static int
-virStorageBackendWipeExtentLocal(virStorageVolDefPtr vol,
- int fd,
- off_t extent_start,
- off_t extent_length,
- size_t writebuf_length,
- size_t *bytes_wiped)
+virStorageBackendWipeLocal(virStorageVolDefPtr vol,
+   int fd,
+   off_t extent_length,
+   size_t writebuf_length,
+   size_t *bytes_wiped)
 {
 int ret = -1, written = 0;
 off_t remaining = 0;
 size_t write_size = 0;
 char *writebuf = NULL;
 
-VIR_DEBUG("extent logical start: %ju len: %ju",
-  (uintmax_t)extent_start, (uintmax_t)extent_length);
+VIR_DEBUG("extent logical start: 0 len: %ju",
+  (uintmax_t)extent_length);
 
 if (VIR_ALLOC_N(writebuf, writebuf_length) < 0)
 goto cleanup;
 
-if (lseek(fd, extent_start, SEEK_SET) < 0) {
+if (lseek(fd, 0, SEEK_SET) < 0) {
 virReportSystemError(errno,
- _("Failed to seek to position %ju in volume "
+ _("Failed to seek to the start in volume "
"with path '%s'"),
- (uintmax_t)extent_start, vol->target.path);
+ vol->target.path);
 goto cleanup;
 }
 
@@ -2126,12 +2125,11 @@ virStorageBackendVolWipeLocal(virConnectPtr conn 
ATTRIBUTE_UNUSED,
 if (S_ISREG(st.st_mode) && st.st_blocks < (st.st_size / DEV_BSIZE)) {
 ret = virStorageBackendVolZeroSparseFileLocal(vol, st.st_size, fd);
 } else {
-ret = virStorageBackendWipeExtentLocal(vol,
-   fd,
-   0,
-   vol->target.allocation,
-   st.st_blksize,
-   _wiped);
+ret = virStorageBackendWipeLocal(vol,
+ fd,
+ vol->target.allocation,
+ st.st_blksize,
+ _wiped);
 }
 }
 
-- 
2.4.6

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

[libvirt] [PATCH v2 2/7] process: Add virProcessGetMaxMemLock()

[Andrea Bolognani]

This function can be used to retrieve the current locked memory
limit for a process, so that the setting can be later restored.

Add a configure check for getrlimit(), which we now use.
---
 configure.ac |  2 +-
 src/libvirt_private.syms |  1 +
 src/util/virprocess.c| 45 +
 src/util/virprocess.h|  2 ++
 4 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 98cf210..25fdfe2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -279,7 +279,7 @@ 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 getuid kill mmap newlocale posix_fallocate \
+  getmntent_r getpwuid_r getrlimit getuid kill mmap newlocale posix_fallocate \
   posix_memalign prlimit regexec sched_getaffinity setgroups setns \
   setrlimit symlink sysctlbyname getifaddrs sched_setscheduler])
 
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 55822ae..0b5ddc1 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2040,6 +2040,7 @@ virPortAllocatorSetUsed;
 virProcessAbort;
 virProcessExitWithStatus;
 virProcessGetAffinity;
+virProcessGetMaxMemLock;
 virProcessGetNamespaces;
 virProcessGetPids;
 virProcessGetStartTime;
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 9b38834..277b3bc 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -788,6 +788,51 @@ virProcessSetMaxMemLock(pid_t pid ATTRIBUTE_UNUSED, 
unsigned long long bytes)
 }
 #endif /* ! (HAVE_SETRLIMIT && defined(RLIMIT_MEMLOCK)) */
 
+#if HAVE_GETRLIMIT && defined(RLIMIT_MEMLOCK)
+int
+virProcessGetMaxMemLock(pid_t pid,
+unsigned long long *bytes)
+{
+struct rlimit rlim;
+
+if (!bytes)
+return 0;
+
+if (pid == 0) {
+if (getrlimit(RLIMIT_MEMLOCK, ) < 0) {
+virReportSystemError(errno,
+ "%s",
+ _("cannot get locked memory limit"));
+return -1;
+}
+} else {
+if (virProcessPrLimit(pid, RLIMIT_MEMLOCK, NULL, ) < 0) {
+virReportSystemError(errno,
+ _("cannot get locked memory limit "
+   "of process %lld"),
+ (long long int) pid);
+return -1;
+}
+}
+
+/* virProcessSetMaxMemLock() sets both rlim_cur and rlim_max to the
+ * same value, so we can retrieve just rlim_max here */
+*bytes = rlim.rlim_max;
+
+return 0;
+}
+#else /* ! (HAVE_GETRLIMIT && defined(RLIMIT_MEMLOCK)) */
+int
+virProcessGetMaxMemLock(pid_t pid ATTRIBUTE_UNUSED,
+unsigned long long *bytes)
+{
+if (!bytes)
+return 0;
+
+virReportSystemError(ENOSYS, "%s", _("Not supported on this platform"));
+return -1;
+}
+#endif /* ! (HAVE_GETRLIMIT && defined(RLIMIT_MEMLOCK)) */
 
 #if HAVE_SETRLIMIT && defined(RLIMIT_NPROC)
 int
diff --git a/src/util/virprocess.h b/src/util/virprocess.h
index 1768009..a7a1fe9 100644
--- a/src/util/virprocess.h
+++ b/src/util/virprocess.h
@@ -76,6 +76,8 @@ int virProcessSetMaxMemLock(pid_t pid, unsigned long long 
bytes);
 int virProcessSetMaxProcesses(pid_t pid, unsigned int procs);
 int virProcessSetMaxFiles(pid_t pid, unsigned int files);
 
+int virProcessGetMaxMemLock(pid_t pid, unsigned long long *bytes);
+
 /* Callback to run code within the mount namespace tied to the given
  * pid.  This function must use only async-signal-safe functions, as
  * it gets run after a fork of a multi-threaded process.  The return
-- 
2.5.0

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

Re: [libvirt] [PATCH 5/6] qemu: Reduce memlock limit after detaching hostdev

[Andrea Bolognani]

On Thu, 2015-12-10 at 12:44 -0500, John Ferlan wrote:
> > Calling qemuDomainAdjustMaxMemLock() more than necessary shouldn't
> > really cause any harm, but adding a check for
> > 
> >   hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI
> > 
> > around the call is even better.
> > 
> > Would that address your concerns?
> 
> You mean of course before the pesky "virDomainHostdevDefFree(hostdev)"?
> 
> You could perhaps do it after the qemuDomainRemovePCIHostDevice in the
> switch or within that qemuDomainRemovePCIHostDevice code.

I moved it into the switch. Tomorrow I'll do a round of tests and
send out a v2.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

Re: [libvirt] [PATCH v2 7/8] virsh: implement new command to support perf

[Ren, Qiaowei]

> -Original Message-
> From: Daniel P. Berrange [mailto:berra...@redhat.com]
> Sent: Tuesday, December 8, 2015 7:07 PM
> To: Ren, Qiaowei
> Cc: libvir-list@redhat.com; Jiri Denemark
> Subject: Re: [PATCH v2 7/8] virsh: implement new command to support perf
> 
> On Mon, Dec 07, 2015 at 03:53:58PM +0800, Qiaowei Ren wrote:
> > This patch add new perf command to enable/disable perf event for a
> > guest domain.
> >
> > Signed-off-by: Qiaowei Ren 
> > ---
> >  tools/virsh-domain.c | 125
> +++
> >  tools/virsh.pod  |  18 
> >  2 files changed, 143 insertions(+)
> >
> > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index
> > b7e7606..a47ef9f 100644
> > --- a/tools/virsh-domain.c
> > +++ b/tools/virsh-domain.c
> > @@ -,6 +,125 @@ cmdMemtune(vshControl *ctl, const vshCmd
> *cmd)
> > }
> >
> >  /*
> > + * "perf" command
> > + */
> > +static const vshCmdInfo info_perf[] = {
> > +{.name = "help",
> > +.data = N_("Get or set perf event")
> > +},
> > +{.name = "desc",
> > +.data = N_("Get or set the current perf events for a guest"
> > +   " domain.\n"
> > +   "To get the perf events list use following command: 
> > \n\n"
> > +   "virsh # perf ")
> > +},
> > +{.name = NULL}
> > +};
> > +
> > +static const vshCmdOptDef opts_perf[] = {
> > +{.name = "domain",
> > + .type = VSH_OT_DATA,
> > + .flags = VSH_OFLAG_REQ,
> > + .help = N_("domain name, id or uuid")
> > +},
> > +{.name = "cmt",
> > + .type = VSH_OT_INT,
> > + .help = N_("CMT event, as integer (default 0, disable)")
> > +},
> 
> Imagine we add many more perf events - we don't want to be adding args to
> virsh for each one. Instead of --cmt, we should have '--event cmt'
> 
> 

Ok. I guess that this command will be the following style:

virsh perf  --enable  --disable <>

For instance: virsh perf guest01 --enable cmt

Daniel, do you think this is OK?

Thanks,
Qiaowei

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

Re: [libvirt] [PATCH 2/3] qemu: do not put a task into machine cgroup

[Henning Schild]

On Tue, 8 Dec 2015 12:23:19 -0500
John Ferlan  wrote:

> 
> 
> On 11/13/2015 11:57 AM, Henning Schild wrote:
> > The machine cgroup is a superset, a parent to the emulator and vcpuX
> > cgroups. The parent cgroup should never have any tasks directly in
> > it. In fact the parent cpuset might contain way more cpus than the
> > sum of emulatorpin and vcpupins. So putting tasks in the superset
> > will allow them to run outside of .
> > 
> > Signed-off-by: Henning Schild 
> > ---
> >  src/qemu/qemu_cgroup.c | 10 --
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> > index 28d2ca2..2c74a22 100644
> > --- a/src/qemu/qemu_cgroup.c
> > +++ b/src/qemu/qemu_cgroup.c
> > @@ -769,12 +769,6 @@ qemuInitCgroup(virQEMUDriverPtr driver,
> >  goto cleanup;
> >  }
> >  
> > -if (virCgroupAddTask(priv->cgroup, vm->pid) < 0) {
> > -virCgroupRemove(priv->cgroup);
> > -virCgroupFree(>cgroup);
> > -goto cleanup;
> > -}
> > -
> 
> Moving this to later would also seem to imply that the code after the
> qemuSetupCgroup (which calls qemuInitCgroup) from qemuProcessLaunch
> would need some movement too, e.g.:
> 
> /* This must be done after cgroup placement to avoid resetting CPU
>  * affinity */
> if (!vm->def->cputune.emulatorpin &&
> qemuProcessInitCpuAffinity(vm) < 0)
> goto cleanup;
>
> Theoretically that would then need to be between the following:
> 
>VIR_DEBUG("Setting cgroup for emulator (if required)");
> if (qemuSetupCgroupForEmulator(vm) < 0)
> goto cleanup;
> 
> <<<... right here, I believe  ...>>>

Good catch! That code is confusing. I will try and merge
qemuProcessInitCpuAffinity with qemuProcessSetEmulatorAffinity.

> VIR_DEBUG("Setting affinity of emulator threads");
> if (qemuProcessSetEmulatorAffinity(vm) < 0)
> goto cleanup;
> 
> 
> Again, weak ACK - hopefully Peter/Martin can take a look. In any case
> a v2 probably should be done.
> 
> John
> >   done:
> >  ret = 0;
> >   cleanup:
> > @@ -1145,6 +1139,10 @@ qemuSetupCgroupForEmulator(virDomainObjPtr
> > vm) goto cleanup;
> >  }
> >  
> > +/* consider the first thread an emulator-thread */
> > +if (virCgroupAddTask(cgroup_emulator, vm->pid) < 0)
> > +goto cleanup;
> > +
> >  virCgroupFree(_emulator);
> >  return 0;
> >  
> > 

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

[libvirt] [PATCHv2 0/3] security: misc cleanups

[Ján Tomko]

v1: https://www.redhat.com/archives/libvir-list/2015-November/msg00781.html
new in v2:
  * split by security driver
  * more functions renamed

Ján Tomko (3):
  security_dac: remove extra Security from function names
  security_selinux: remove extra Security from function names
  security_stack: : remove extra security from function names

 src/security/security_dac.c | 242 +-
 src/security/security_selinux.c | 375 
 src/security/security_stack.c   |  68 
 3 files changed, 338 insertions(+), 347 deletions(-)

-- 
2.4.6

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

Re: [libvirt] [PATCH 2/5] qemu: prefer 00:1D.x and 00:1A.x for USB2 controllers on Q35

[John Ferlan]

On 11/19/2015 01:24 PM, Laine Stump wrote:
> The real Q35 machine puts the first USB controller set (EHCI+(UHCIx4))
> on bus 0 slot 0x1D, and the 2nd USB controller set on bus 0 slot 0x1A,
> so let's attempt to make the virtual machine match that for
> controllers with auto-assigned addresses when possible.
> 
> Three test cases were added to assure that the proper addresses are
> assigned - one with a single set of unaddressed USB controllers, one
> with 3 (to grab both preferred slots plus one more), and one with the
> order of the controller definitions reordered, to assure that the
> auto-assignment isn't mixed up by order.
> ---
>  src/qemu/qemu_command.c| 52 -
>  .../qemuxml2argv-q35-usb2-multi.args   | 40 +
>  .../qemuxml2argv-q35-usb2-multi.xml| 47 +++
>  .../qemuxml2argv-q35-usb2-reorder.args | 40 +
>  .../qemuxml2argv-q35-usb2-reorder.xml  | 47 +++
>  tests/qemuxml2argvdata/qemuxml2argv-q35-usb2.args  | 30 ++
>  tests/qemuxml2argvdata/qemuxml2argv-q35-usb2.xml   | 39 +
>  tests/qemuxml2argvtest.c   | 21 +++
>  .../qemuxml2xmlout-q35-usb2-multi.xml  | 66 
> ++
>  .../qemuxml2xmlout-q35-usb2-reorder.xml| 66 
> ++
>  .../qemuxml2xmloutdata/qemuxml2xmlout-q35-usb2.xml | 46 +++
>  tests/qemuxml2xmltest.c|  3 +
>  12 files changed, 494 insertions(+), 3 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-usb2-multi.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-usb2-multi.xml
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-usb2-reorder.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-usb2-reorder.xml
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-usb2.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-usb2.xml
>  create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-usb2-multi.xml
>  create mode 100644 
> tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-usb2-reorder.xml
>  create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-usb2.xml
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 3c867de..06e3073 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2049,6 +2049,47 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr 
> def,
>  }
>  break;
>  
> +case VIR_DOMAIN_CONTROLLER_TYPE_USB:
> +if ((def->controllers[i]->model
> + == VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1) &&
> +(def->controllers[i]->info.type
> + == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)) {
> +/* Try to assign the first found USB2 controller to
> + * 00:1D.0 and 2nd to 00:1A.0 (because that is their
> + * standard location on real Q35 hardware) unless they
> + * are already taken, but don't insist on it.
> + *
> + * (NB: all other controllers at the same index will
> + * get assigned to the same slot as the UHCI1 when
> + * addresses are later assigned to all devices.)
> + */
> +bool assign = false;
> +
> +memset(_addr, 0, sizeof(tmp_addr));
> +tmp_addr.slot = 0x1D;
> +if (!virDomainPCIAddressSlotInUse(addrs, _addr)) {
> +assign = true;
> +} else {
> +tmp_addr.slot = 0x1A;
> +if (!virDomainPCIAddressSlotInUse(addrs, _addr))
> +assign = true;
> +}
> +if (assign) {
> +if (virDomainPCIAddressReserveAddr(addrs, _addr,
> +   flags, false, true) < 
> 0)

Should param5 be 'false' since we're running from qemu_command and not
fromConfig ?

> +goto cleanup;

So is this 'cleanup' case a condition where perhaps the bus was full?
IOW: Do we want to fail or just let code that would handle the case
where assign = false would then (I assume) later on assign an address on
some available slot?

So the result would be

   if (virDomainPCIAddressReserveAddr(...) == 0) {
   def->controllers
   ...
   }

?

> +def->controllers[i]->info.type
> += VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
> +def->controllers[i]->info.addr.pci.domain = 0;
> +def->controllers[i]->info.addr.pci.bus = 0;
> +def->controllers[i]->info.addr.pci.slot = tmp_addr.slot;
> +def->controllers[i]->info.addr.pci.function = 0;
> +def->controllers[i]->info.addr.pci.multi
> +  

Re: [libvirt] [PATCH 4/5] qemu: define virDomainDevAddUSBController()

[John Ferlan]

On 11/19/2015 01:25 PM, Laine Stump wrote:
> This new function will add a single controller of the given model,
> except the case of ich9-usb-ehci1 (the master controller for a USB2
> controller set) in which case a set of related controllers will be
> added (EHCI1, UHCI1, UHCI2, UHCI3). These controllers will not be
> given PCI addresses, but should be otherwise ready to use.
> 
> "-1" is allowed for controller model, and means "default for this
> machinetype". This matches the existing practice in
> qemuDomainDefPostParse(), which always adds the default controller
> with model = -1, and relies on the commandline builder to set a model
> (that is wrong, but will be fixed later).
> ---
>  src/conf/domain_conf.c   | 48 
> 
>  src/conf/domain_conf.h   |  2 ++
>  src/libvirt_private.syms |  1 +
>  tests/qemuxml2argvtest.c |  1 +
>  4 files changed, 52 insertions(+)
> 

Wasn't able to "git am -3" this patch - so I assume you have some merge
conflicts coming...  Safe to assume from your side that I wasn't able to
compile this - so I'll further assume this and the next patch will have
gone through check/check-syntax processing

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index ab05e7f..63888b1 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -14410,6 +14410,54 @@ virDomainDefAddController(virDomainDefPtr def, int 
> type, int idx, int model)
>  }
>  
>  
> +/*
> + * virDomainDefAddUSBController() - add a USB controller of the
> + * specified model. If model is ich9-usb-ehci, also add companion
> + * uhci1, uhci2, and uhci3 controllers at the same index. Note that a
> + * model of "-1" is allowed for backward compatibility, and means
> + * "default USB controller for this machinetype".
> + */

Nice! comments, but the format I've seen lately has been:

/* functionName
 * @arg1: description
 * @argn: description
 *
 * Function description
 *
 * Returns description
 */
> +int
> +virDomainDefAddUSBController(virDomainDefPtr def, int idx, int model)
> +{
> +virDomainControllerDefPtr cont; /* this is a *copy* of the DefPtr */
> +
> +cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB,
> + idx, model);
> +if (!cont)
> +return -1;
> +
> +if (model != VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1)
> +return 0;
> +
> +/* When the initial controller is ich9-usb-ehci, also add the
> + * companion controllers
> + */
> +
> +idx = cont->idx; /* in case original request was "-1" */
> +

And the operating assumption being that none of the following exist?
IOW: Would it be possible for someone to add these, but not the above?
Not that anyone (qa) would do that...

> +if (!(cont = virDomainDefAddController(def, 
> VIR_DOMAIN_CONTROLLER_TYPE_USB,
> +   idx, 
> VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1)))
> +return -1;
> +cont->info.mastertype = VIR_DOMAIN_CONTROLLER_MASTER_USB;
> +cont->info.master.usb.startport = 0;
> +
> +if (!(cont = virDomainDefAddController(def, 
> VIR_DOMAIN_CONTROLLER_TYPE_USB,
> +   idx, 
> VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2)))
> +return -1;
> +cont->info.mastertype = VIR_DOMAIN_CONTROLLER_MASTER_USB;
> +cont->info.master.usb.startport = 2;
> +
> +if (!(cont = virDomainDefAddController(def, 
> VIR_DOMAIN_CONTROLLER_TYPE_USB,
> +   idx, 
> VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3)))
> +return -1;
> +cont->info.mastertype = VIR_DOMAIN_CONTROLLER_MASTER_USB;
> +cont->info.master.usb.startport = 4;
> +
> +return 0;
> +}
> +
> +
>  int
>  virDomainDefMaybeAddController(virDomainDefPtr def,
> int type,
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 8d43ee6..c34bfd0 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -3169,6 +3169,8 @@ int virDomainObjListConvert(virDomainObjListPtr domlist,
>  bool skip_missing);
>  
>  int
> +virDomainDefAddUSBController(virDomainDefPtr def, int idx, int model);
> +int
>  virDomainDefMaybeAddController(virDomainDefPtr def,
> int type,
> int idx,
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 7e60d87..b7008e0 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -200,6 +200,7 @@ virDomainControllerTypeToString;
>  virDomainCpuPlacementModeTypeFromString;
>  virDomainCpuPlacementModeTypeToString;
>  virDomainDefAddImplicitControllers;
> +virDomainDefAddUSBController;
>  virDomainDefCheckABIStability;
>  virDomainDefCheckDuplicateDiskInfo;
>  virDomainDefCheckUnsupportedMemoryHotplug;
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 5fe52b0..25ffbea 100644

Re: [libvirt] [PATCH 00/10] VFIO fixes for PCI devices

[Alex Williamson]

On Thu, 2015-12-10 at 09:06 +0100, Andrea Bolognani wrote:
> On Wed, 2015-12-09 at 08:09 -0700, Alex Williamson wrote:
> > > I was under the impression that what the current series
> > > does, eg. sharing devices in the same IOMMU group between
> > > the host driver and vfio-pci is safe as long as no guest is
> > > using them at the same time, and that devices could be
> > > safely "moved" between the host driver (eg. in use) and
> > > vfio-pci (eg. idle, waiting to be assigned to a guest) as
> > > many times as desired without ill consequences.
> > > 
> > > Is my understanding wrong? Do I need to rework the series
> > > so that unbinds and reprobes are always executed across the
> > > IOMMU group?
> > 
> > Hi Andrea,
> > 
> > Your understanding is correct, so I think it just comes down to how sure
> > you are that the vfio group is idle/unused.  If there's any risk that a
> > device is still in use by QEMU, then we haven't solved the original
> > problem.  Unbinding isn't the only way to have good confidence of this
> > though.  You could track the QEMU pid, you could use fuser to make sure
> > the group is not in use, you could try to open the group yourself to
> > make sure it's not in use, and of course you can unbind as proposed in
> > the second option.  So long as you know the group is idle, somehow, it
> > shouldn't matter what order you unbind and reprobe.
> 
> Thanks for the confirmation, Alex.
> 
> What we're doing right now in libvirt (and my series extends on that)
> is keeping track of each device's state internally by recording
> information such as which guest, if any, is using it.
> 
> We're mostly assuming that the user will not mess with the configuration
> behind our back. I guess adding more checks to make sure that's actually
> the case would be good, but then again, we can hardly stop the user from
> writing stuff into /sys :)
> 
> At least now we know my series is not built upon a completely incorrect
> understanding of the constraints!
> 
> Quick follow-up question: all of this care with IOMMU group ownership is
> only really applied when dealing with VFIO passthrough - are legacy KVM
> assignment and Xen assignment really not affected at all? Is the IOMMU
> not involved in the process?

Both legacy KVM device assignment and (afaik) Xen device assignment
ignore the issues of device isolation, and typically even iommu
granularity, and allow any configuration imaginable, even when it
interferes with the operation of devices owned by other guests or even
host-owned devices.  Thanks,

Alex

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

Re: [libvirt] [PATCH] Revert "admin: Rename virAdmConnect to virAdmDaemon"

[Daniel P. Berrange]

On Thu, Dec 10, 2015 at 03:46:45PM +0100, Erik Skultety wrote:
> Commmit df8192aa introduced admin related rename and some minor
> (caused by automated approach, aka sed) and some more severe isues along with
> it. First reason to revert is the inconsistency with libvirt library.

We have release 1.3.0 with this header file, so IMHO we can't just
rename the public APIs now.

>  include/libvirt/libvirt-admin.h |  48 +-

> diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h
> index b81698e..ab9df96 100644
> --- a/include/libvirt/libvirt-admin.h
> +++ b/include/libvirt/libvirt-admin.h
> @@ -35,51 +35,53 @@ extern "C" {
>  # undef __VIR_ADMIN_H_INCLUDES__
>  
>  /**
> - * virAdmDaemon:
> + * virAdmConnect:
>   *
> - * a virAdmDaemon is a private structure representing a remote daemon.
> + * a virAdmConnect is a private structure representing a connection to
> + * libvirt daemon.
>   */
> -typedef struct _virAdmDaemon virAdmDaemon;
> +typedef struct _virAdmConnect virAdmConnect;
>  
>  /**
> - * virAdmDaemonPtr:
> + * virAdmConnectPtr:
>   *
> - * a virAdmDaemonPtr is pointer to a virAdmDaemon private structure,
> - * this is the type used to reference a daemon in the API.
> + * a virAdmConnectPtr is pointer to a virAdmConnect private structure,
> + * this is the type used to reference a connection to the daemon
> + * in the API.
>   */
> -typedef virAdmDaemon *virAdmDaemonPtr;
> +typedef virAdmConnect *virAdmConnectPtr;
>  
> -virAdmDaemonPtr virAdmDaemonOpen(const char *name, unsigned int flags);
> -int virAdmDaemonClose(virAdmDaemonPtr dmn);
> +virAdmConnectPtr virAdmConnectOpen(const char *name, unsigned int flags);
> +int virAdmConnectClose(virAdmConnectPtr conn);
>  
> -int virAdmDaemonRef(virAdmDaemonPtr dmn);
> -int virAdmDaemonIsAlive(virAdmDaemonPtr dmn);
> +int virAdmConnectRef(virAdmConnectPtr conn);
> +int virAdmConnectIsAlive(virAdmConnectPtr conn);
>  
>  int virAdmGetVersion(unsigned long long *libVer);
>  
> -char *virAdmDaemonGetURI(virAdmDaemonPtr dmn);
> +char *virAdmConnectGetURI(virAdmConnectPtr conn);
>  
> -int virAdmDaemonGetVersion(virAdmDaemonPtr dmn,
> -   unsigned long long *libVer);
> +int virAdmConnectGetLibVersion(virAdmConnectPtr conn,
> +   unsigned long long *libVer);
>  
>  /**
> - * virAdmDaemonCloseFunc:
> - * @dmn: virAdmDaemon connection
> + * virAdmConnectCloseFunc:
> + * @conn: virAdmConnect connection
>   * @reason: reason why the connection was closed (see virConnectCloseReason)
>   * @opaque: opaque client data
>   *
>   * A callback to be registered, in case a connection was closed.
>   */
> -typedef void (*virAdmDaemonCloseFunc)(virAdmDaemonPtr dmn,
> +typedef void (*virAdmConnectCloseFunc)(virAdmConnectPtr conn,
> int reason,
> void *opaque);
>  
> -int virAdmDaemonRegisterCloseCallback(virAdmDaemonPtr dmn,
> -  virAdmDaemonCloseFunc cb,
> -  void *opaque,
> -  virFreeCallback freecb);
> -int virAdmDaemonUnregisterCloseCallback(virAdmDaemonPtr dmn,
> -virAdmDaemonCloseFunc cb);
> +int virAdmConnectRegisterCloseCallback(virAdmConnectPtr conn,
> +   virAdmConnectCloseFunc cb,
> +   void *opaque,
> +   virFreeCallback freecb);
> +int virAdmConnectUnregisterCloseCallback(virAdmConnectPtr conn,
> + virAdmConnectCloseFunc cb);
>  
>  # ifdef __cplusplus
>  }

This is all public API


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

[libvirt] [PATCH RESEND 2/3] Remove dead code from qemuDomainAttachControllerDevice

[Ján Tomko]

We only support hotplugging SCSI controllers,
USB and virtio-serial related code is useless here.
---
 src/conf/domain_addr.c   | 21 -
 src/conf/domain_addr.h   |  4 
 src/libvirt_private.syms |  1 -
 src/qemu/qemu_hotplug.c  | 18 --
 4 files changed, 44 deletions(-)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index ca5803e..c7eab0c 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -884,27 +884,6 @@ 
virDomainVirtioSerialAddrSetAddControllers(virDomainVirtioSerialAddrSetPtr addrs
 return 0;
 }
 
-/* virDomainVirtioSerialAddrSetRemoveController
- *
- * Removes a virtio serial controller from the address set.
- */
-void
-virDomainVirtioSerialAddrSetRemoveController(virDomainVirtioSerialAddrSetPtr 
addrs,
- virDomainControllerDefPtr cont)
-{
-ssize_t pos;
-
-if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL)
-return;
-
-VIR_DEBUG("Removing virtio serial controller index %u "
-  "from the address set", cont->idx);
-
-pos = virDomainVirtioSerialAddrFindController(addrs, cont->idx);
-
-if (pos >= 0)
-VIR_DELETE_ELEMENT(addrs->controllers, pos, addrs->ncontrollers);
-}
 
 void
 virDomainVirtioSerialAddrSetFree(virDomainVirtioSerialAddrSetPtr addrs)
diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
index 2220a79..74f414e 100644
--- a/src/conf/domain_addr.h
+++ b/src/conf/domain_addr.h
@@ -209,10 +209,6 @@ int
 virDomainVirtioSerialAddrSetAddController(virDomainVirtioSerialAddrSetPtr 
addrs,
   virDomainControllerDefPtr cont)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
-void
-virDomainVirtioSerialAddrSetRemoveController(virDomainVirtioSerialAddrSetPtr 
addrs,
- virDomainControllerDefPtr cont)
-ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
 int
 virDomainVirtioSerialAddrSetAddControllers(virDomainVirtioSerialAddrSetPtr 
addrs,
virDomainDefPtr def)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 63d8618..536acab 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -115,7 +115,6 @@ virDomainVirtioSerialAddrSetAddController;
 virDomainVirtioSerialAddrSetAddControllers;
 virDomainVirtioSerialAddrSetCreate;
 virDomainVirtioSerialAddrSetFree;
-virDomainVirtioSerialAddrSetRemoveController;
 
 
 # conf/domain_audit.h
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index eae5418..9bd4238 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -442,7 +442,6 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr 
driver,
 char *devstr = NULL;
 qemuDomainObjPrivatePtr priv = vm->privateData;
 bool releaseaddr = false;
-bool addedToAddrSet = false;
 
 if (controller->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
 virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
@@ -484,20 +483,6 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr 
driver,
 if (qemuAssignDeviceControllerAlias(vm->def, priv->qemuCaps, 
controller) < 0)
 goto cleanup;
 
-if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB &&
-controller->model == -1 &&
-!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("USB controller hotplug unsupported in this QEMU 
binary"));
-goto cleanup;
-}
-
-if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL &&
-virDomainVirtioSerialAddrSetAddController(priv->vioserialaddrs,
-  controller) < 0)
-goto cleanup;
-addedToAddrSet = true;
-
 if (!(devstr = qemuBuildControllerDevStr(vm->def, controller, 
priv->qemuCaps, NULL)))
 goto cleanup;
 }
@@ -526,9 +511,6 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr 
driver,
 }
 
  cleanup:
-if (ret != 0 && addedToAddrSet)
-virDomainVirtioSerialAddrSetRemoveController(priv->vioserialaddrs,
- controller);
 if (ret != 0 && releaseaddr)
 qemuDomainReleaseDeviceAddress(vm, >info, NULL);
 
-- 
2.4.6

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

Re: [libvirt] [PATCH 5/6] qemu: Reduce memlock limit after detaching hostdev

[Andrea Bolognani]

On Wed, 2015-12-09 at 15:15 -0500, John Ferlan wrote:
> On 11/24/2015 08:56 AM, Andrea Bolognani wrote:
> > We increase the limit before plugging in a PCI hostdev or a memory
> > module because some memory might need to be locked due to eg. VFIO.
> > 
> > Of course we should do the opposite after unplugging a device: this
> > was already the case for memory modules, but not for hostdevs.
> > ---
> >  src/qemu/qemu_hotplug.c | 6 ++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> > index a5c134a..04baeff 100644
> > --- a/src/qemu/qemu_hotplug.c
> > +++ b/src/qemu/qemu_hotplug.c
> > @@ -3070,6 +3070,12 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
> >  networkReleaseActualDevice(vm->def, net);
> >  virDomainNetDefFree(net);
> >  }
> > +
> > +/* QEMU might no longer need to lock as much memory, eg. we just 
> > detached
> > + * a VFIO device, so adjust the limit here */
> > +if (qemuDomainAdjustMaxMemLock(vm) < 0)
> > +VIR_WARN("Failed to adjust locked memory limit");
> > +
> >  ret = 0;
> >  
> >   cleanup:
> > 
> 
> Since the add was in qemuDomainAttachHostPCIDevice, why is the remove
> not in qemuDomainDetachHostPCIDevice?  Doing it here would mean it's
> call for any host device removal - whether or not it was ever added.

Because qemuDomainDetachHostPCIDevice() is where we ask for the device
to be removed from the guest, but we have to wait for it to actually
be removed (and for the guest definition to be updated) before changing
the memory locking limit.

I guess I could move this code to qemuDomainDetachThisHostDevice(), but
keeping it close to where the guest definition is updated looks cleaner
to me.

I also fail to see how a device that was never added could be removed,
could you please elaborate?

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

[libvirt] [PATCH] qemu: add reporting of vCPU wait time

[Daniel P. Berrange]

The VIR_DOMAIN_STATS_VCPU flag to virDomainListGetStats
enables reporting of stats about vCPUs. Currently we
only report the cumulative CPU running time and the
execution state.

This adds reporting of the wait time - time the vCPU
wants to run, but the host schedular has something else
running ahead of it.

The data is reported per-vCPU eg

$ virsh domstats --vcpu demo
 Domain: 'demo'
   vcpu.current=4
   vcpu.maximum=4
   vcpu.0.state=1
   vcpu.0.time=142000
   vcpu.0.wait=18403928
   vcpu.1.state=1
   vcpu.1.time=13000
   vcpu.1.wait=10612111
   vcpu.2.state=1
   vcpu.2.time=11000
   vcpu.2.wait=12759501
   vcpu.3.state=1
   vcpu.3.time=9000
   vcpu.3.wait=21825087

In implementing this I notice our reporting of CPU execute
time has very poor granularity, since we are getting it
from /proc/$PID/stat. As a future enhancement we should
prefer to get CPU execute time from /proc/$PID/schedstat
or /proc/$PID/sched (if either exist on the running kernel)

Signed-off-by: Daniel P. Berrange 
---
 src/qemu/qemu_driver.c | 100 +++--
 1 file changed, 96 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 783a7cd..5293294 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1361,6 +1361,80 @@ static char *qemuConnectGetCapabilities(virConnectPtr 
conn) {
 
 
 static int
+qemuGetSchedInfo(unsigned long long *cpuWait,
+ pid_t pid, pid_t tid)
+{
+char *proc = NULL;
+char *data = NULL;
+char **lines = NULL;
+size_t i;
+int ret = -1;
+double val;
+
+*cpuWait = 0;
+
+/* In general, we cannot assume pid_t fits in int; but /proc parsing
+ * is specific to Linux where int works fine.  */
+if (tid)
+ret = virAsprintf(, "/proc/%d/task/%d/sched", (int)pid, (int)tid);
+else
+ret = virAsprintf(, "/proc/%d/sched", (int)pid);
+if (ret < 0)
+goto cleanup;
+
+/* The file is not guaranteed to exist (needs CONFIG_SCHED_DEBUG) */
+if (access(proc, R_OK) < 0)
+return 0;
+
+if (virFileReadAll(proc, (1<<16), ) < 0)
+goto cleanup;
+
+lines = virStringSplit(data, "\n", 0);
+if (!lines)
+goto cleanup;
+
+for (i = 0; lines[i] != NULL; i++) {
+const char *line = lines[i];
+
+/* Needs CONFIG_SCHEDSTATS. The second check
+ * is the old name the kernel used in past */
+if (STRPREFIX(line, "se.statistics.wait_sum") ||
+STRPREFIX(line, "se.wait_sum")) {
+line = strchr(line, ':');
+if (!line) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Missing separate in sched info '%s'"),
+   lines[i]);
+goto cleanup;
+}
+line++;
+while (*line == ' ') {
+line++;
+}
+
+if (virStrToDouble(line, NULL, ) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Unable to parse sched info value '%s'"),
+   line);
+goto cleanup;
+}
+
+*cpuWait = (unsigned long long)(val * 100);
+break;
+}
+}
+
+ret = 0;
+
+ cleanup:
+VIR_FREE(data);
+VIR_FREE(proc);
+VIR_FREE(lines);
+return ret;
+}
+
+
+static int
 qemuGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, long *vm_rss,
pid_t pid, int tid)
 {
@@ -1424,6 +1498,7 @@ qemuGetProcessInfo(unsigned long long *cpuTime, int 
*lastCpu, long *vm_rss,
 static int
 qemuDomainHelperGetVcpus(virDomainObjPtr vm,
  virVcpuInfoPtr info,
+ unsigned long long *cpuwait,
  int maxinfo,
  unsigned char *cpumaps,
  int maplen)
@@ -1476,6 +1551,11 @@ qemuDomainHelperGetVcpus(virDomainObjPtr vm,
 virBitmapFree(map);
 }
 
+if (cpuwait) {
+if (qemuGetSchedInfo(&(cpuwait[i]), vm->pid, vcpupid) < 0)
+return -1;
+}
+
 ncpuinfo++;
 }
 
@@ -5517,7 +5597,7 @@ qemuDomainGetVcpus(virDomainPtr dom,
 goto cleanup;
 }
 
-ret = qemuDomainHelperGetVcpus(vm, info, maxinfo, cpumaps, maplen);
+ret = qemuDomainHelperGetVcpus(vm, info, NULL, maxinfo, cpumaps, maplen);
 
  cleanup:
 virDomainObjEndAPI();
@@ -19089,6 +19169,7 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver 
ATTRIBUTE_UNUSED,
 int ret = -1;
 char param_name[VIR_TYPED_PARAM_FIELD_LENGTH];
 virVcpuInfoPtr cpuinfo = NULL;
+unsigned long long *cpuwait = NULL;
 
 if (virTypedParamsAddUInt(>params,
   >nparams,
@@ -19104,10 +19185,12 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver 
ATTRIBUTE_UNUSED,
   

[libvirt] [PATCH] bhyve: bhyveload: respect boot dev and boot order

[Roman Bogorodskiy]

Make bhyveload respect boot order as specified by os.boot section of the
domain XML or by "boot order" for specific devices. As bhyve does not
support a real boot order specification right now, it's just about
choosing a single device to boot from.
---
 src/bhyve/bhyve_command.c  | 49 --
 .../bhyvexml2argv-bhyveload-bootorder.args | 10 +
 .../bhyvexml2argv-bhyveload-bootorder.ldargs   |  3 ++
 .../bhyvexml2argv-bhyveload-bootorder.xml  | 30 +
 .../bhyvexml2argv-bhyveload-bootorder1.args| 10 +
 .../bhyvexml2argv-bhyveload-bootorder1.ldargs  |  3 ++
 .../bhyvexml2argv-bhyveload-bootorder1.xml | 29 +
 .../bhyvexml2argv-bhyveload-bootorder2.args|  9 
 .../bhyvexml2argv-bhyveload-bootorder2.ldargs  |  3 ++
 .../bhyvexml2argv-bhyveload-bootorder2.xml | 24 +++
 .../bhyvexml2argv-bhyveload-bootorder3.args| 10 +
 .../bhyvexml2argv-bhyveload-bootorder3.ldargs  |  3 ++
 .../bhyvexml2argv-bhyveload-bootorder3.xml | 30 +
 tests/bhyvexml2argvtest.c  |  4 ++
 14 files changed, 214 insertions(+), 3 deletions(-)
 create mode 100644 
tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-bootorder.args
 create mode 100644 
tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-bootorder.ldargs
 create mode 100644 
tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-bootorder.xml
 create mode 100644 
tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-bootorder1.args
 create mode 100644 
tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-bootorder1.ldargs
 create mode 100644 
tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-bootorder1.xml
 create mode 100644 
tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-bootorder2.args
 create mode 100644 
tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-bootorder2.ldargs
 create mode 100644 
tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-bootorder2.xml
 create mode 100644 
tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-bootorder3.args
 create mode 100644 
tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-bootorder3.ldargs
 create mode 100644 
tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-bootorder3.xml

diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index 6576029..1e5d4c7 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -522,6 +522,48 @@ virBhyveProcessBuildGrubbhyveCmd(virDomainDefPtr def,
 return cmd;
 }
 
+static virDomainDiskDefPtr
+virBhyveGetBootDisk(virConnectPtr conn, virDomainDefPtr def)
+{
+size_t i;
+virDomainDiskDefPtr match = NULL;
+int best_index = INT_MAX;
+int boot_cdrom = 0, boot_disk = 0;
+
+if (def->ndisks == 0)
+return NULL;
+
+for (i = 0; i < def->os.nBootDevs; i++) {
+switch (def->os.bootDevs[i]) {
+case VIR_DOMAIN_BOOT_CDROM:
+boot_cdrom = i;
+break;
+case VIR_DOMAIN_BOOT_DISK:
+boot_disk = i;
+break;
+}
+}
+
+for (i = 0; i < def->ndisks; i++) {
+int bootIndex;
+if (!virBhyveUsableDisk(conn, def->disks[i]))
+continue;
+
+bootIndex = def->disks[i]->info.bootIndex;
+if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_CDROM)
+bootIndex += boot_cdrom;
+else if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK)
+bootIndex += boot_disk;
+
+if (bootIndex < best_index) {
+best_index = bootIndex;
+match = def->disks[i];
+}
+}
+
+return match;
+}
+
 virCommandPtr
 virBhyveProcessBuildLoadCmd(virConnectPtr conn, virDomainDefPtr def,
 const char *devmap_file, char **devicesmap_out)
@@ -535,10 +577,11 @@ virBhyveProcessBuildLoadCmd(virConnectPtr conn, 
virDomainDefPtr def,
 }
 
 if (def->os.bootloader == NULL) {
-disk = def->disks[0];
+disk = virBhyveGetBootDisk(conn, def);
 
-if (!virBhyveUsableDisk(conn, disk))
-return NULL;
+if (disk == NULL)
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("no bootable disks found"));
 
 return virBhyveProcessBuildBhyveloadCmd(def, disk);
 } else if (strstr(def->os.bootloader, "grub-bhyve") != NULL) {
diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-bootorder.args 
b/tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-bootorder.args
new file mode 100644
index 000..01a0290
--- /dev/null
+++ b/tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-bootorder.args
@@ -0,0 +1,10 @@
+/usr/sbin/bhyve \
+-c 1 \
+-m 214 \
+-u \
+-H \
+-P \
+-s 0:0,hostbridge \
+-s 3:0,virtio-net,faketapdev,mac=52:54:00:00:00:00 \
+-s 2:0,ahci-hd,/tmp/freebsd.img \
+-s 4:0,ahci-cd,/tmp/cdrom.iso bhyve
diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-bootorder.ldargs 
b/tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-bootorder.ldargs
new file mode 

[libvirt] [PATCH v2] vz: implementation of domainCreateXML callback

[Mikhail Feoktistov]

---

 diff from v1.
 Remove call of vzDomainSuspend() in case of VIR_DOMAIN_START_PAUSED flag is 
set.
 Now we don't support to create instance in stopped state.

 src/vz/vz_driver.c | 24 
 1 file changed, 24 insertions(+)

diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index ea1090a..4498e01 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -979,6 +979,29 @@ vzDomainUndefine(virDomainPtr domain)
 return vzDomainUndefineFlags(domain, 0);
 }
 
+static virDomainPtr
+vzDomainCreateXML(virConnectPtr conn,
+  const char *xml,
+  unsigned int flags)
+{
+virDomainPtr domain;
+int ret;
+
+virCheckFlags(0, NULL);
+
+domain = vzDomainDefineXMLFlags(conn, xml, 0);
+if (domain == NULL)
+return domain;
+
+ret = vzDomainCreate(domain);
+if (ret != 0) {
+vzDomainUndefine(domain);
+return NULL;
+}
+
+return domain;
+}
+
 static int
 vzDomainHasManagedSaveImage(virDomainPtr domain, unsigned int flags)
 {
@@ -1493,6 +1516,7 @@ static virHypervisorDriver vzDriver = {
 .domainShutdown = vzDomainShutdown, /* 0.10.0 */
 .domainCreate = vzDomainCreate,/* 0.10.0 */
 .domainCreateWithFlags = vzDomainCreateWithFlags, /* 1.2.10 */
+.domainCreateXML = vzDomainCreateXML, /* 1.2.22 */
 .domainReboot = vzDomainReboot, /* 1.3.0 */
 .domainDefineXML = vzDomainDefineXML,  /* 0.10.0 */
 .domainDefineXMLFlags = vzDomainDefineXMLFlags, /* 1.2.12 */
-- 
1.8.3.1

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

[libvirt] vf configuration cleanup when VM is delete

[Moshe Levi]

Hi,

I have a setup with libvirt 1.3.0 and OpenStack trunk.
Before launched the VM ip link command show the following VF mac/vlan 
configuration [1]
When I launch a VM with  via openstack api (OpenStack 
direct port)
I can see that the VF get the mac/vlan according to libvrit xml [2] and ip link 
command  [3], but when I delete the VM the mac/vlan config are still shown as 
in [3] and not restored to [1]
Shouldn't  libvirt restore the mac/vlan to [1].

The same problem exists when using  (OpenStack macvtap 
port)  but just for the MAC configuration of the VF.



[1]  - 24: enp3s0f0:  mtu 1500 qdisc mq master 
ovs-system state UP mode DEFAULT group default qlen 1000
link/ether e4:1d:2d:a5:f1:22 brd ff:ff:ff:ff:ff:ff
vf 0 MAC 00:00:00:00:00:00, spoof checking off, link-state auto
vf 1 MAC 00:00:00:00:00:00, spoof checking off, link-state auto
vf 2 MAC 00:00:00:00:00:00, spoof checking off, link-state auto
vf 3 MAC 00:00:00:00:00:00, spoof checking off, link-state auto

[2] - 
  
  
  

  
  

  
  
  



[3] 24: enp3s0f0:  mtu 1500 qdisc mq master 
ovs-system state UP mode DEFAULT group default qlen 1000
link/ether e4:1d:2d:a5:f1:22 brd ff:ff:ff:ff:ff:ff
vf 0 MAC 00:00:00:00:00:00, spoof checking off, link-state auto
vf 1 MAC 00:00:00:00:00:00, spoof checking off, link-state auto
vf 2 MAC 00:00:00:00:00:00, spoof checking off, link-state auto
vf 3 MAC fa:16:3e:11:af:fe, vlan 190, spoof checking off, link-state enable

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

Re: [libvirt] [PATCH] qemu: add reporting of vCPU wait time

[Vasiliy Tolstov]

13 дек. 2015 г. 12:53 пользователь "Daniel P. Berrange" 
написал:
>
> The VIR_DOMAIN_STATS_VCPU flag to virDomainListGetStats
> enables reporting of stats about vCPUs. Currently we
> only report the cumulative CPU running time and the
> execution state.
>
> This adds reporting of the wait time - time the vCPU
> wants to run, but the host schedular has something else
> running ahead of it.
>

As i understand this is well known steal time analog that can be viewed
inside vm?

> The data is reported per-vCPU eg
>
> $ virsh domstats --vcpu demo
>  Domain: 'demo'
>vcpu.current=4
>vcpu.maximum=4
>vcpu.0.state=1
>vcpu.0.time=142000
>vcpu.0.wait=18403928
>vcpu.1.state=1
>vcpu.1.time=13000
>vcpu.1.wait=10612111
>vcpu.2.state=1
>vcpu.2.time=11000
>vcpu.2.wait=12759501
>vcpu.3.state=1
>vcpu.3.time=9000
>vcpu.3.wait=21825087
>
> In implementing this I notice our reporting of CPU execute
> time has very poor granularity, since we are getting it
> from /proc/$PID/stat. As a future enhancement we should
> prefer to get CPU execute time from /proc/$PID/schedstat
> or /proc/$PID/sched (if either exist on the running kernel)
>
> Signed-off-by: Daniel P. Berrange 
> ---
>  src/qemu/qemu_driver.c | 100
+++--
>  1 file changed, 96 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 783a7cd..5293294 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1361,6 +1361,80 @@ static char
*qemuConnectGetCapabilities(virConnectPtr conn) {
>
>
>  static int
> +qemuGetSchedInfo(unsigned long long *cpuWait,
> + pid_t pid, pid_t tid)
> +{
> +char *proc = NULL;
> +char *data = NULL;
> +char **lines = NULL;
> +size_t i;
> +int ret = -1;
> +double val;
> +
> +*cpuWait = 0;
> +
> +/* In general, we cannot assume pid_t fits in int; but /proc parsing
> + * is specific to Linux where int works fine.  */
> +if (tid)
> +ret = virAsprintf(, "/proc/%d/task/%d/sched", (int)pid,
(int)tid);
> +else
> +ret = virAsprintf(, "/proc/%d/sched", (int)pid);
> +if (ret < 0)
> +goto cleanup;
> +
> +/* The file is not guaranteed to exist (needs CONFIG_SCHED_DEBUG) */
> +if (access(proc, R_OK) < 0)
> +return 0;
> +
> +if (virFileReadAll(proc, (1<<16), ) < 0)
> +goto cleanup;
> +
> +lines = virStringSplit(data, "\n", 0);
> +if (!lines)
> +goto cleanup;
> +
> +for (i = 0; lines[i] != NULL; i++) {
> +const char *line = lines[i];
> +
> +/* Needs CONFIG_SCHEDSTATS. The second check
> + * is the old name the kernel used in past */
> +if (STRPREFIX(line, "se.statistics.wait_sum") ||
> +STRPREFIX(line, "se.wait_sum")) {
> +line = strchr(line, ':');
> +if (!line) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Missing separate in sched info '%s'"),
> +   lines[i]);
> +goto cleanup;
> +}
> +line++;
> +while (*line == ' ') {
> +line++;
> +}
> +
> +if (virStrToDouble(line, NULL, ) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Unable to parse sched info value
'%s'"),
> +   line);
> +goto cleanup;
> +}
> +
> +*cpuWait = (unsigned long long)(val * 100);
> +break;
> +}
> +}
> +
> +ret = 0;
> +
> + cleanup:
> +VIR_FREE(data);
> +VIR_FREE(proc);
> +VIR_FREE(lines);
> +return ret;
> +}
> +
> +
> +static int
>  qemuGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, long
*vm_rss,
> pid_t pid, int tid)
>  {
> @@ -1424,6 +1498,7 @@ qemuGetProcessInfo(unsigned long long *cpuTime, int
*lastCpu, long *vm_rss,
>  static int
>  qemuDomainHelperGetVcpus(virDomainObjPtr vm,
>   virVcpuInfoPtr info,
> + unsigned long long *cpuwait,
>   int maxinfo,
>   unsigned char *cpumaps,
>   int maplen)
> @@ -1476,6 +1551,11 @@ qemuDomainHelperGetVcpus(virDomainObjPtr vm,
>  virBitmapFree(map);
>  }
>
> +if (cpuwait) {
> +if (qemuGetSchedInfo(&(cpuwait[i]), vm->pid, vcpupid) < 0)
> +return -1;
> +}
> +
>  ncpuinfo++;
>  }
>
> @@ -5517,7 +5597,7 @@ qemuDomainGetVcpus(virDomainPtr dom,
>  goto cleanup;
>  }
>
> -ret = qemuDomainHelperGetVcpus(vm, info, maxinfo, cpumaps, maplen);
> +ret = qemuDomainHelperGetVcpus(vm, info, NULL, maxinfo, cpumaps,
maplen);
>
>   cleanup:
>  virDomainObjEndAPI();
> @@ 

Re: [libvirt] Release of libvirt-1.3.0

[Roman Bogorodskiy]

  Daniel Veillard wrote:

>   So as planned, I tagged the release in git and pushed signed tarball and
> rpms to the usual place:
> 
>ftp://libvirt.org/libvirt

It seems there is still no entry for 1.3.0 on the web site:
http://libvirt.org/news.html

Though I can see the corresponding commit to docs/news.html.in.

Roman Bogorodskiy

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