[libvirt] [PATCH 1/1] qemu: host NUMA hugepage policy without guest NUMA

[Sam Bobroff]

At the moment, guests that are backed by hugepages in the host are
only able to use policy to control the placement of those hugepages
on a per-(guest-)CPU basis. Policy applied globally is ignored.

Such guests would use  and
a  block with  but no  elements.

This patch corrects this by, in this specific case, changing the QEMU
command line from "-mem-prealloc -mem-path=..." (which cannot
specify NUMA policy) to "-object memory-backend-file ..." (which can).

Note: This is not visible to the guest and does not appear to create
a migration incompatibility.

Signed-off-by: Sam Bobroff 
---
There was some discussion leading up to this patch, here:

https://www.redhat.com/archives/libvir-list/2016-October/msg00033.html

During that discussion it seemed that fixing this issue would cause
migration incompatibility but after some careful testing, it appears
that it only does when a memory-backend object is attached to a guest
NUMA node and that is not the case here. If only one is created, and
used globally (not attached via mem=), the migration data does not
seem to be changed and so it seems reasonable to try something like
this patch.

This patch does work for my test cases but I don't claim a deep
understanding of the libvirt code so this is at least partly a RFC.
Comments welcome :-)

Cheers,
Sam.

 src/qemu/qemu_command.c | 55 -
 src/qemu/qemu_command.h |  2 +-
 2 files changed, 46 insertions(+), 11 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 0804133..c28c8f2 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3143,7 +3143,7 @@ qemuBuildMemoryBackendStr(unsigned long long size,
   int guestNode,
   virBitmapPtr userNodeset,
   virBitmapPtr autoNodeset,
-  virDomainDefPtr def,
+  const virDomainDefPtr def,
   virQEMUCapsPtr qemuCaps,
   virQEMUDriverConfigPtr cfg,
   const char **backendType,
@@ -7129,12 +7129,18 @@ qemuBuildSmpCommandLine(virCommandPtr cmd,
 
 static int
 qemuBuildMemPathStr(virQEMUDriverConfigPtr cfg,
-const virDomainDef *def,
+const virDomainDefPtr def,
 virQEMUCapsPtr qemuCaps,
-virCommandPtr cmd)
+virCommandPtr cmd,
+virBitmapPtr auto_nodeset)
 {
 const long system_page_size = virGetSystemPageSizeKB();
 char *mem_path = NULL;
+virBitmapPtr nodemask = NULL;
+const char *backendType = NULL;
+char *backendStr = NULL;
+virJSONValuePtr props;
+int rv = -1;
 
 /*
  *  No-op if hugepages were not requested.
@@ -7159,18 +7165,47 @@ qemuBuildMemPathStr(virQEMUDriverConfigPtr cfg,
 if (qemuGetHupageMemPath(cfg, def->mem.hugepages[0].size, _path) < 0)
 return -1;
 
-virCommandAddArgList(cmd, "-mem-prealloc", "-mem-path", mem_path, NULL);
+if (virDomainNumatuneMaybeGetNodeset(def->numa, auto_nodeset,
+ , -1) < 0)
+return -1;
+if (nodemask && virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE)) {
+props = virJSONValueNewObject();
+if (qemuBuildMemoryBackendStr(virDomainDefGetMemoryInitial(def),
+  0, -1, NULL, auto_nodeset,
+  def, qemuCaps, cfg, ,
+  , false) < 0)
+goto cleanup;
+if (!(backendStr = virQEMUBuildObjectCommandlineFromJSON(backendType,
+ "mem",
+ props)))
+goto cleanup;
+virCommandAddArgList(cmd, "-object", backendStr, NULL);
+rv = 0;
+cleanup:
+VIR_FREE(backendStr);
+VIR_FREE(props);
+}
+else {
+if (nodemask || 1)
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("Memory file backend objects are "
+ "unsupported by QEMU binary. Global NUMA "
+ "hugepage policy will be ignored."));
+virCommandAddArgList(cmd, "-mem-prealloc", "-mem-path", mem_path, 
NULL);
+rv = 0;
+}
 VIR_FREE(mem_path);
 
-return 0;
+return rv;
 }
 
 
 static int
 qemuBuildMemCommandLine(virCommandPtr cmd,
 virQEMUDriverConfigPtr cfg,
-const virDomainDef *def,
-virQEMUCapsPtr qemuCaps)
+const virDomainDefPtr def,
+virQEMUCapsPtr qemuCaps,
+virBitmapPtr auto_nodeset)
 {
 if (qemuDomainDefValidateMemoryHotplug(def, qemuCaps, NULL) < 0)
 return -1;
@@ -7194,7 +7229,7 @@ 

Re: [libvirt] [PATCH] sheepdog: allow snapshot

[Michal Privoznik]

On 11.10.2016 19:30, Vasiliy Tolstov wrote:
> 2016-09-29 16:00 GMT+03:00 Vasiliy Tolstov :
>> partially revert f7c1410b0ee5b878e81f2eddf86c609947a9b27c because
>> sheepdog allow to store vm state inside vdi
>>
> 
> Sorry, can somebody check this?
> 
>> Signed-off-by: Vasiliy Tolstov 
>> ---
>>  src/qemu/qemu_driver.c | 6 ++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index db99c414d458..816514d2d909 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -13887,6 +13887,12 @@ qemuDomainSnapshotPrepare(virConnectPtr conn,
>>active) < 0)
>>  goto cleanup;
>>
>> +/* sheepdog allow to store memory inside the vdi */
>> +if (vm->def->disks[i]->src->type == VIR_STORAGE_TYPE_NETWORK &&
>> +(vm->def->disks[i]->src->protocol == 
>> VIR_STORAGE_NET_PROTOCOL_SHEEPDOG)) {
>> +break;
>> +}
>> +

Unfortunately, I don't have a setup to try this out. The code looks
okay, however the commit you are referring to says that there might be
some problems with storing memory into RBD image. So is that the case?
Because if it is, we might not want to allow this for full snapshots
with guest memory.

Michal

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

Re: [libvirt] [PATCH 3/3] qemu: add support for simpler UEFI config

[John Ferlan]

On 10/03/2016 11:49 AM, Daniel P. Berrange wrote:
> This wires up the domain XML post-parse callback so that
> it can fill in the loader/nvram paths, when seeing the
> "firmware" attribute set.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  src/qemu/qemu_command.c|  6 ++-
>  src/qemu/qemu_conf.c   | 12 ++---
>  src/qemu/qemu_conf.h   |  7 +++
>  src/qemu/qemu_domain.c | 60 
> +++---
>  .../qemuxml2argv-bios-firmware.args| 26 ++
>  .../qemuxml2argv-bios-firmware.xml | 41 +++
>  tests/qemuxml2argvtest.c   |  1 +
>  .../qemuxml2xmlout-bios-firmware.xml   | 48 +
>  tests/qemuxml2xmltest.c|  1 +
>  tests/testutilsqemu.c  | 30 ++-
>  10 files changed, 214 insertions(+), 18 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-bios-firmware.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-bios-firmware.xml
>  create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-bios-firmware.xml
> 

The xml2xml tests could go in patch2 I suppose, but they're fine here
too - at least they're provided.

Could add that empty path for bios/rom type test too if you felt so
compelled.

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 7e52963..a7529bf 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -8927,8 +8927,10 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
>  
>  switch ((virDomainLoader) loader->type) {
>  case VIR_DOMAIN_LOADER_TYPE_ROM:
> -virCommandAddArg(cmd, "-bios");
> -virCommandAddArg(cmd, loader->path);
> +if (loader->path) {
> +virCommandAddArg(cmd, "-bios");
> +virCommandAddArg(cmd, loader->path);

If !loader->path then -bios has no path? Is that OK from QEMU?

> +}
>  break;
>  
>  case VIR_DOMAIN_LOADER_TYPE_PFLASH:
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 635fa27..879e2aa 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -124,13 +124,6 @@ void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr 
> def)
>  }
>  
>  
> -#define VIR_QEMU_OVMF_LOADER_PATH "/usr/share/OVMF/OVMF_CODE.fd"
> -#define VIR_QEMU_OVMF_NVRAM_PATH "/usr/share/OVMF/OVMF_VARS.fd"
> -#define VIR_QEMU_OVMF_SEC_LOADER_PATH "/usr/share/OVMF/OVMF_CODE.secboot.fd"
> -#define VIR_QEMU_OVMF_SEC_NVRAM_PATH "/usr/share/OVMF/OVMF_VARS.fd"
> -#define VIR_QEMU_AAVMF_LOADER_PATH "/usr/share/AAVMF/AAVMF_CODE.fd"
> -#define VIR_QEMU_AAVMF_NVRAM_PATH "/usr/share/AAVMF/AAVMF_VARS.fd"
> -
>  virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
>  {
>  virQEMUDriverConfigPtr cfg;
> @@ -334,6 +327,11 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool 
> privileged)
>  VIR_STRDUP(cfg->firmwares[2]->name, VIR_QEMU_OVMF_SEC_LOADER_PATH) < 
> 0 ||
>  VIR_STRDUP(cfg->firmwares[2]->nvram, VIR_QEMU_OVMF_SEC_NVRAM_PATH) < 
> 0)
>  goto error;
> +
> +cfg->firmwares[0]->arch = VIR_ARCH_AARCH64;
> +cfg->firmwares[1]->arch = VIR_ARCH_X86_64;
> +cfg->firmwares[2]->arch = VIR_ARCH_X86_64;
> +cfg->firmwares[2]->secboot = true;
>  #endif
>  
>  return cfg;
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index d32689a..2f0c91f 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -66,6 +66,13 @@ typedef virQEMUDriver *virQEMUDriverPtr;
>  typedef struct _virQEMUDriverConfig virQEMUDriverConfig;
>  typedef virQEMUDriverConfig *virQEMUDriverConfigPtr;
>  
> +# define VIR_QEMU_OVMF_LOADER_PATH "/usr/share/OVMF/OVMF_CODE.fd"
> +# define VIR_QEMU_OVMF_NVRAM_PATH "/usr/share/OVMF/OVMF_VARS.fd"
> +# define VIR_QEMU_OVMF_SEC_LOADER_PATH "/usr/share/OVMF/OVMF_CODE.secboot.fd"
> +# define VIR_QEMU_OVMF_SEC_NVRAM_PATH "/usr/share/OVMF/OVMF_VARS.fd"
> +# define VIR_QEMU_AAVMF_LOADER_PATH "/usr/share/AAVMF/AAVMF_CODE.fd"
> +# define VIR_QEMU_AAVMF_NVRAM_PATH "/usr/share/AAVMF/AAVMF_VARS.fd"
> +
>  /* Main driver config. The data in these object
>   * instances is immutable, so can be accessed
>   * without locking. Threads must, however, hold
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 9b1a32e..3badd38 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2346,13 +2346,59 @@ qemuDomainDefPostParse(virDomainDefPtr def,
>  goto cleanup;
>  }
>  
> -if (def->os.loader &&
> -def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH &&
> -def->os.loader->readonly == VIR_TRISTATE_SWITCH_ON &&
> -!def->os.loader->nvram) {
> -if (virAsprintf(>os.loader->nvram, "%s/%s_VARS.fd",
> -cfg->nvramDir, def->name) < 0)
> -goto cleanup;
> +if (def->os.loader) {
> +

Re: [libvirt] [PATCH 2/3] conf: add support for choosing firmware type

[John Ferlan]

On 10/03/2016 11:49 AM, Daniel P. Berrange wrote:
> Currently if you want to enable UEFI firmware for a guest
> you need to know about the hypervisor platform specific
> firmware path. This does not even work for all platforms,
> as hypervisors like VMWare don't expose UEFI as a path,
> they just have a direct config option for turning it on
> or off.
> 
> This adds ability to use the much simpler:
> 
>   
> 
> to choose the different firmware types.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  docs/formatdomain.html.in |  9 +-
>  docs/schemas/domaincommon.rng | 12 +++-
>  src/conf/domain_conf.c| 70 
> ++-
>  src/conf/domain_conf.h| 11 +++
>  4 files changed, 93 insertions(+), 9 deletions(-)
> 

The xml2xml tests from patch 3 could move here, but at least it exists..

> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 7008005..b8e9315 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -143,7 +143,14 @@
>  pflash. Moreover, some firmwares may
>  implement the Secure boot feature. Attribute
>  secure can be used then to control it.
> -Since 2.1.0
> +Since 2.1.0. The firmware
> +attribute can be used to request a specific type of firmware image
> +based on its common name, accepting the values uefi
> +and bios. Since 2.4.0. When
> +firmware is set, it is not neccessary to provide any

necessary

> +path for the loader, nor set the type attribute or
> +nvram elements, as they will be automatically set
> +to the hypervisor specific default values.

So, if the firmware attribute isn't set, then is path is required? It
seems to only ever be defaulted in patch 3 in the post parse.

>nvram
>Some UEFI firmwares may want to use a non-volatile memory to store
>  some variables. In the host, this is represented as a file and the
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 6eeb4e9..197b542 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -276,7 +276,17 @@
>  
>
>  
> -
> +
> +  
> +
> +  bios
> +  uefi
> +
> +  
> +
> +
> +  
> +
>
>  
>  
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 7972a4e..054be94 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -835,6 +835,12 @@ VIR_ENUM_IMPL(virDomainLoader,
>"rom",
>"pflash")
>  
> +VIR_ENUM_IMPL(virDomainLoaderFirmware,
> +  VIR_DOMAIN_LOADER_FIRMWARE_LAST,
> +  "default",
> +  "bios",
> +  "uefi")
> +
>  /* Internal mapping: subset of block job types that can be present in
>   *  XML (remaining types are not two-phase). */
>  VIR_ENUM_DECL(virDomainBlockJob)
> @@ -15539,17 +15545,36 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
>  char *readonly_str = NULL;
>  char *secure_str = NULL;
>  char *type_str = NULL;
> +char *firmware_str = NULL;
>  
>  readonly_str = virXMLPropString(node, "readonly");
>  secure_str = virXMLPropString(node, "secure");
>  type_str = virXMLPropString(node, "type");
> +firmware_str = virXMLPropString(node, "firmware");
>  loader->path = (char *) xmlNodeGetContent(node);
>  
> -if (readonly_str &&
> -(loader->readonly = virTristateBoolTypeFromString(readonly_str)) <= 
> 0) {
> -virReportError(VIR_ERR_XML_DETAIL,
> -   _("unknown readonly value: %s"), readonly_str);
> -goto cleanup;
> +if (loader->path && STREQ(loader->path, ""))
> +VIR_FREE(loader->path);

STREQ_NULLABLE works too.

Is it 'undocumented' feature of having "" for path mean some sort of
default?  It becomes more important in the next patch. But this does
follow the schema w/r/t path being optional I suppose.

ACK in principle - your call on the loader->path question.

John

> +
> +if (firmware_str) {
> +int firmware;
> +if ((firmware = virDomainLoaderFirmwareTypeFromString(firmware_str)) 
> < 0) {
> +virReportError(VIR_ERR_XML_DETAIL,
> +   _("unknown firmware value: %s"), firmware_str);
> +goto cleanup;
> +}
> +loader->firmware = firmware;
> +}
> +
> +if (readonly_str) {
> +if ((loader->readonly = virTristateBoolTypeFromString(readonly_str)) 
> <= 0) {
> +virReportError(VIR_ERR_XML_DETAIL,
> +   _("unknown readonly value: %s"), readonly_str);
> +goto cleanup;
> +}
> +} else {
> +if (loader->firmware == 

Re: [libvirt] [PATCH 1/3] firmware: include arch and features in firmware file list

[John Ferlan]

On 10/03/2016 11:49 AM, Daniel P. Berrange wrote:
> Currently qemu.conf contains a nvram parameter which
> lists firmware code files and the corresponding nvram
> file path. We need to know which architecture and
> features are associated with each firmware file for
> future enhancement. This extends the syntax in a
> backwards compatible manner to record this info.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  src/libvirt_private.syms   |  1 +
>  src/qemu/qemu.conf | 14 --
>  src/qemu/test_libvirtd_qemu.aug.in |  6 +--
>  src/util/virfirmware.c | 94 
> ++
>  src/util/virfirmware.h |  7 +++
>  5 files changed, 105 insertions(+), 17 deletions(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 68d0ea9..a2f0b83 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1628,6 +1628,7 @@ virFirewallStartTransaction;
>  
>  
>  # util/virfirmware.h
> +virFirmwareFind;
>  virFirmwareFreeList;
>  virFirmwareParse;
>  virFirmwareParseList;
> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> index e4c2aae..56a0e55 100644
> --- a/src/qemu/qemu.conf
> +++ b/src/qemu/qemu.conf
> @@ -595,16 +595,22 @@
>  # using this master file as image. Each UEFI firmware can,
>  # however, have different variables store. Therefore the nvram is
>  # a list of strings when a single item is in form of:
> -#   ${PATH_TO_UEFI_FW}:${PATH_TO_UEFI_VARS}.
> +#
> +#   ${PATH_TO_UEFI_FW}:${PATH_TO_UEFI_VARS}:${ARCH}[:${FEATURE}:...].
> +#
> +# Current valid features include:
> +#
> +#   'secboot' - the firmware has secure boot enabled
> +#
>  # Later, when libvirt creates per domain variable store, this list is
>  # searched for the master image. The UEFI firmware can be called
>  # differently for different guest architectures. For instance, it's OVMF
>  # for x86_64 and i686, but it's AAVMF for aarch64. The libvirt default
>  # follows this scheme.
>  #nvram = [
> -#   "/usr/share/OVMF/OVMF_CODE.fd:/usr/share/OVMF/OVMF_VARS.fd",
> -#   "/usr/share/OVMF/OVMF_CODE.secboot.fd:/usr/share/OVMF/OVMF_VARS.fd",
> -#   "/usr/share/AAVMF/AAVMF_CODE.fd:/usr/share/AAVMF/AAVMF_VARS.fd"
> +#   "/usr/share/OVMF/OVMF_CODE.fd:/usr/share/OVMF/OVMF_VARS.fd:x86_64",
> +#   
> "/usr/share/OVMF/OVMF_CODE.secboot.fd:/usr/share/OVMF/OVMF_VARS.fd:x86_64:secboot",
> +#   "/usr/share/AAVMF/AAVMF_CODE.fd:/usr/share/AAVMF/AAVMF_VARS.fd:aarch64"
>  #]
>  
>  # The backend to use for handling stdout/stderr output from
> diff --git a/src/qemu/test_libvirtd_qemu.aug.in 
> b/src/qemu/test_libvirtd_qemu.aug.in
> index cd162ae..b78a0b3 100644
> --- a/src/qemu/test_libvirtd_qemu.aug.in
> +++ b/src/qemu/test_libvirtd_qemu.aug.in
> @@ -83,8 +83,8 @@ module Test_libvirtd_qemu =
>  { "migration_port_max" = "49215" }
>  { "log_timestamp" = "0" }
>  { "nvram"
> -{ "1" = "/usr/share/OVMF/OVMF_CODE.fd:/usr/share/OVMF/OVMF_VARS.fd" }
> -{ "2" = 
> "/usr/share/OVMF/OVMF_CODE.secboot.fd:/usr/share/OVMF/OVMF_VARS.fd" }
> -{ "3" = "/usr/share/AAVMF/AAVMF_CODE.fd:/usr/share/AAVMF/AAVMF_VARS.fd" }
> +{ "1" = 
> "/usr/share/OVMF/OVMF_CODE.fd:/usr/share/OVMF/OVMF_VARS.fd:x86_64" }
> +{ "2" = 
> "/usr/share/OVMF/OVMF_CODE.secboot.fd:/usr/share/OVMF/OVMF_VARS.fd:x86_64:secboot"
>  }
> +{ "3" = 
> "/usr/share/AAVMF/AAVMF_CODE.fd:/usr/share/AAVMF/AAVMF_VARS.fd:aarch64" }
>  }
>  { "stdio_handler" = "logd" }
> diff --git a/src/util/virfirmware.c b/src/util/virfirmware.c
> index 6b20c06..117e8ef 100644
> --- a/src/util/virfirmware.c
> +++ b/src/util/virfirmware.c
> @@ -61,30 +61,83 @@ int
>  virFirmwareParse(const char *str, virFirmwarePtr firmware)
>  {
>  int ret = -1;
> -char **token;
> +char **token, **tmp;
> +size_t ntoken = 0;
> +int arch;
>  
> -if (!(token = virStringSplit(str, ":", 0)))
> +if (!(tmp = token = virStringSplit(str, ":", 0)))

virStringSplitCount would return the number allow the usage of a for
loop below rather than counting ntoken...

>  goto cleanup;
>  
> -if (token[0]) {
> -virSkipSpaces((const char **) [0]);
> -if (token[1])
> -virSkipSpaces((const char **) [1]);
> +while (tmp && *tmp) {
> +virSkipSpaces((const char **) );
> +if (STREQ(*tmp, "")) {
> +virReportError(VIR_ERR_CONF_SYNTAX,
> +   _("Invalid nvram format for '%s', token "
> + "must not be the empty string"),
> +   str);
> +goto cleanup;
> +}
> +ntoken++;
> +tmp++;
>  }
>  
> -/* Exactly two tokens are expected */
> -if (!token[0] || !token[1] || token[2] ||
> -STREQ(token[0], "") || STREQ(token[1], "")) {
> +if (ntoken < 2 || ntoken > 4) {
>  virReportError(VIR_ERR_CONF_SYNTAX,
> -   _("Invalid nvram format: '%s'"),
> +   _("Invalid nvram 

[libvirt] [PATCH] util: Alter return value of virReadFCHost and fix mem leak

[John Ferlan]

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

Rather than return a 0 or -1 and the *result string, return just the result
string to the caller.  Alter all the callers to handle the different return.

As a side effect or result of this, it's much clearer that we cannot just
assign the returned string into the scsi_host wwnn, wwpn, and fabric_wwn
fields - rather we should fetch a temporary string, then as long as our
fetch was good, VIR_FREE what may have been there, and STEAL what we just got.
This fixes a memory leak in the virNodeDeviceCreateXML code path through
find_new_device and nodeDeviceLookupSCSIHostByWWN which will continually
call nodeDeviceSysfsGetSCSIHostCaps until the expected wwnn/wwpn is found
in the device object capabilities.

Signed-off-by: John Ferlan 
---

 I suppose I could have made two patches out of this, but it felt like
 overkill once I realized what the issue was. OK a third one line patch
 could have been added to change the virGetFCHostNameByWWN comment as well,
 but that'd really be overkill.

 src/node_device/node_device_linux_sysfs.c | 55 ---
 src/util/virutil.c| 34 ---
 src/util/virutil.h|  9 +++--
 tests/fchosttest.c| 30 ++---
 4 files changed, 49 insertions(+), 79 deletions(-)

diff --git a/src/node_device/node_device_linux_sysfs.c 
b/src/node_device/node_device_linux_sysfs.c
index 549d32c..be99c41 100644
--- a/src/node_device/node_device_linux_sysfs.c
+++ b/src/node_device/node_device_linux_sysfs.c
@@ -44,8 +44,7 @@ VIR_LOG_INIT("node_device.node_device_linux_sysfs");
 int
 nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapDataPtr d)
 {
-char *max_vports = NULL;
-char *vports = NULL;
+char *tmp = NULL;
 int ret = -1;
 
 if (virReadSCSIUniqueId(NULL, d->scsi_host.host,
@@ -59,64 +58,53 @@ nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapDataPtr d)
 if (virIsCapableFCHost(NULL, d->scsi_host.host)) {
 d->scsi_host.flags |= VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST;
 
-if (virReadFCHost(NULL,
-  d->scsi_host.host,
-  "port_name",
-  >scsi_host.wwpn) < 0) {
+if (!(tmp = virReadFCHost(NULL, d->scsi_host.host, "port_name"))) {
 VIR_WARN("Failed to read WWPN for host%d", d->scsi_host.host);
 goto cleanup;
 }
+VIR_FREE(d->scsi_host.wwpn);
+VIR_STEAL_PTR(d->scsi_host.wwpn, tmp);
 
-if (virReadFCHost(NULL,
-  d->scsi_host.host,
-  "node_name",
-  >scsi_host.wwnn) < 0) {
+if (!(tmp = virReadFCHost(NULL, d->scsi_host.host, "node_name"))) {
 VIR_WARN("Failed to read WWNN for host%d", d->scsi_host.host);
 goto cleanup;
 }
+VIR_FREE(d->scsi_host.wwnn);
+VIR_STEAL_PTR(d->scsi_host.wwnn, tmp);
 
-if (virReadFCHost(NULL,
-  d->scsi_host.host,
-  "fabric_name",
-  >scsi_host.fabric_wwn) < 0) {
+if (!(tmp = virReadFCHost(NULL, d->scsi_host.host, "fabric_name"))) {
 VIR_WARN("Failed to read fabric WWN for host%d",
  d->scsi_host.host);
 goto cleanup;
 }
+VIR_FREE(d->scsi_host.fabric_wwn);
+VIR_STEAL_PTR(d->scsi_host.fabric_wwn, tmp);
 }
 
 if (virIsCapableVport(NULL, d->scsi_host.host)) {
 d->scsi_host.flags |= VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS;
 
-if (virReadFCHost(NULL,
-  d->scsi_host.host,
-  "max_npiv_vports",
-  _vports) < 0) {
+if (!(tmp = virReadFCHost(NULL, d->scsi_host.host,
+  "max_npiv_vports"))) {
 VIR_WARN("Failed to read max_npiv_vports for host%d",
  d->scsi_host.host);
 goto cleanup;
 }
 
- if (virReadFCHost(NULL,
-  d->scsi_host.host,
-  "npiv_vports_inuse",
-  ) < 0) {
-VIR_WARN("Failed to read npiv_vports_inuse for host%d",
- d->scsi_host.host);
+if (virStrToLong_i(tmp, NULL, 10, >scsi_host.max_vports) < 0) {
+VIR_WARN("Failed to parse value of max_npiv_vports '%s'", tmp);
 goto cleanup;
 }
 
-if (virStrToLong_i(max_vports, NULL, 10,
-   >scsi_host.max_vports) < 0) {
-VIR_WARN("Failed to parse value of max_npiv_vports '%s'",
-  max_vports);
+ if (!(tmp = virReadFCHost(NULL, d->scsi_host.host,
+   "npiv_vports_inuse"))) {
+VIR_WARN("Failed to read npiv_vports_inuse for host%d",
+ d->scsi_host.host);
 goto 

[libvirt] [PATCH] virsh: vol-resize: be explicit that this is an offline operation.

[Ivan Baldo]

No time to read manpages at home heheh (qemu-img check -r all repaired it very 
well though).
---
 tools/virsh-volume.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c
index f302f95..46c36c2 100644
--- a/tools/virsh-volume.c
+++ b/tools/virsh-volume.c
@@ -1050,7 +1050,7 @@ static const vshCmdInfo info_vol_resize[] = {
  .data = N_("resize a vol")
 },
 {.name = "desc",
- .data = N_("Resizes a storage volume.")
+ .data = N_("Resizes a storage volume offline (only safe if not in use).")
 },
 {.name = NULL}
 };
-- 
2.1.4

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

Re: [libvirt] [PATCH 3/4] vbox: change API (un)initialization logic.

[Dawid Zamirski]

On Tue, 2016-10-11 at 10:58 -0400, Dawid Zamirski wrote:
> On Tue, 2016-10-11 at 16:22 +0200, Martin Kletzander wrote:
> > On Wed, Sep 28, 2016 at 01:41:35PM -0400, Dawid Zamirski wrote:
> > 
> 
> > I don't understand how vbox works, but if initializing
> > g_pVBoxGlobalData
> > does not make any connection, and ISession does (which would make
> > sense), we could keep the global data around and add ISession for
> > each connection.  I guess that's something you're talking about
> > below.
> 
> 
> Neither I'm super familiar with vbox internals, but your suggestion
> sounds reasonable, so I'll dive into that code in vbox source to find
> out.
> 

So after a quick glance at vbox initalization code, it seems that both
IVirtualBox and ISession are reference counted global instances
(g_VirtualBox and g_Session [1]) and both are allocated and released in
pfnComInitialize [2] and pfnComUninitialize [3]  implementation calls.
As such I won't be able to separate out ISession instances to be per-
connection while keeping global IVirtualBox reference.


[1] https://github.com/mdaniel/virtualbox-org-svn-vbox-trunk/blob/maste
r/src/VBox/Main/cbinding/VBoxCAPI.cpp#L46-L49
[2] https://github.com/mdaniel/virtualbox-org-svn-vbox-trunk/blob/maste
r/src/VBox/Main/cbinding/VBoxCAPI.cpp#L480-L481
[3] https://github.com/mdaniel/virtualbox-org-svn-vbox-trunk/blob/maste
r/src/VBox/Main/cbinding/VBoxCAPI.cpp#L493-L502

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

[libvirt] libvirt API / balloon last-update

[Marko Myllynen]

Hi,

I see that recently the last-update field has been added to the balloon
statistics, this is how it looks like from Python:

[(, {'balloon.rss': 960048L,
'balloon.swap_in': 0L, 'balloon.usable': 1676424L, 'balloon.unused':
1376188L, 'balloon.major_fault': 816L, 'balloon.swap_out': 0L,
'balloon.current': 2097152L, 'balloon.maximum': 2097152L,
'balloon.available': 2048364L, 'balloon.minor_fault': 460160L,
'balloon.last-update': 1476213030L})]

As you can see, there's an inconsistency as all the other members use
underscore (_), not dash (-). This actually matters with the PCP plugin
since PCP metric names can contain _ but not -.

Now that last-update is part of the API in 2.3 I guess it's too late to
change this but it would nice if in the future the member names could be
kept consistent and underscore used for new members, as it has been so far.

Thanks,

-- 
Marko Myllynen

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

Re: [libvirt] [PATCH v3 09/18] qemu: set/use info->pciConnectFlags during qemuDomainAssignDevicePCISlots

[Laine Stump]

On 10/04/2016 10:11 AM, Andrea Bolognani wrote:

On Tue, 2016-09-20 at 15:14 -0400, Laine Stump wrote:

Set pciConnectFlags in each device's DeviceInfo prior to assigning PCI
addresses, and then use those flags later when actually assigning the
addresses with qemuDomainPCIAddressReserveNextAddr() (rather than
scattering the logic about which devices need which type of slot all
over the place).
---
   src/qemu/qemu_domain_address.c | 234 
++---
   1 file changed, 104 insertions(+), 130 deletions(-)

[...]

@@ -686,9 +685,6 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def 
ATTRIBUTE_UNUSED,
   int ret = -1;
   virPCIDeviceAddressPtr addr = >addr.pci;
   bool entireSlot;
-/* flags may be changed from default below */
-virDomainPCIConnectFlags flags = (VIR_PCI_CONNECT_HOTPLUGGABLE |
-  VIR_PCI_CONNECT_TYPE_PCI_DEVICE);

This hunk you're removing seems to support my comment in
patch 4, the one arguing against adding HOTPLUGGABLE at the
end of the function.


   if (!virDeviceInfoPCIAddressPresent(info) ||
   ((device->type == VIR_DOMAIN_DEVICE_HOSTDEV) &&
@@ -700,69 +696,25 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def 
ATTRIBUTE_UNUSED,
   return 0;
   }
   
-/* Change flags according to differing requirements of different

- * devices.
+/* If we get to here, the device has a PCI address assigned in the
+ * config and we should mark it as in-use. But if the
+ * pciConnectFlags are 0, then this device shouldn't have a PCI
+ * address associated with it. *BUT* since there are cases in the
+ * past where we've apparently allowed that, we need to pretend
+ * for now that it's okay, otherwise an existing domain could
+ * "disappear" from the list of domains due to a parse failure. We
+ * can fix this by just forcing the pciConnectFlags to be
+ * PCI_DEVICE (and then relying on validation functions to report
+ * inappropriate address types.
*/
-switch (device->type) {
-case VIR_DOMAIN_DEVICE_CONTROLLER:
-switch (device->data.controller->type) {
-case  VIR_DOMAIN_CONTROLLER_TYPE_PCI:
-   flags = 
virDomainPCIControllerModelToConnectType(device->data.controller->model);
-break;
-
-case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
-/* SATA controllers aren't hot-plugged, and can be put in
- * either a PCI or PCIe slot
- */
-flags = (VIR_PCI_CONNECT_TYPE_PCI_DEVICE
- | VIR_PCI_CONNECT_TYPE_PCIE_DEVICE);

The new code doesn't seem to have any special handling of
CONTROLLER_TYPE_SATA, which means it will end up as
PCI_DEVICE|HOTPLUGGABLE instead of PCI_DEVICE|PCIE_DEVICE.


All of the flag settings in this function are essentially pointless, and 
*not* what is used when actually assigning an address to a device (it 
took me a while to realize this). There is a fuller explanation below, 
but in short, when we are examining device addresses that were manually 
set (or auto-assigned at some earlier time), any combination of the 
following flags is considered equivalent:


  VIR_PCI_CONNECT_TYPE_PCI_DEVICE
  VIR_PCI_CONNECT_TYPE_PCIE_DEVICE
  VIR_PCI_CONNECT_HOTPLUGGABLE

so all of the flags settings in this function are equivalent to 
PCI_DEVICE | HOTPLUGGABLE.





-break;
-
-case VIR_DOMAIN_CONTROLLER_TYPE_USB:
-/* allow UHCI and EHCI controllers to be manually placed on
- * the PCIe bus (but don't put them there automatically)
- */
-switch (device->data.controller->model) {
-case VIR_DOMAIN_CONTROLLER_MODEL_USB_EHCI:
-case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1:
-case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1:
-case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2:
-case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3:
-case VIR_DOMAIN_CONTROLLER_MODEL_USB_VT82C686B_UHCI:
-flags = VIR_PCI_CONNECT_TYPE_PCI_DEVICE;

All these will be HOTPLUGGABLE.


^^ e.g. this




-break;
-case VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI:
-/* should this be PCIE-only? Or do we need to allow PCI
- * for backward compatibility?
- */
-flags = (VIR_PCI_CONNECT_TYPE_PCI_DEVICE
- | VIR_PCI_CONNECT_TYPE_PCIE_DEVICE);

This will be PCI_DEVICE|HOTPLUGGABLE instead of
PCI_DEVICE|PCIE_DEVICE.


^^ and this. etc.




-break;
-case VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI:
-case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI:
-case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX4_UHCI:
-/* Allow these for PCI only */
-break;
-}
-}
-break;
-
-case VIR_DOMAIN_DEVICE_SOUND:
-switch 

[libvirt] [PATCH 5/6] virsh: Extract fallback handling in cmdVcpuinfo

[Peter Krempa]

Put it into a separate function so that more fallback handling can be
added without making a mess.
---
 tools/virsh-domain.c | 75 +---
 1 file changed, 54 insertions(+), 21 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 8e1b9ed..84a4854 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -6311,6 +6311,49 @@ virshVcpuinfoPrintAffinity(vshControl *ctl,


 static bool
+virshVcpuinfoInactive(vshControl *ctl,
+  virDomainPtr dom,
+  int nvcpus,
+  int maxcpu,
+  bool pretty)
+{
+unsigned char *cpumaps = NULL;
+size_t cpumaplen;
+int ncpus;
+size_t i;
+bool ret = false;
+
+cpumaplen = VIR_CPU_MAPLEN(maxcpu);
+cpumaps = vshMalloc(ctl, nvcpus * cpumaplen);
+
+if ((ncpus = virDomainGetVcpuPinInfo(dom, nvcpus,
+ cpumaps, cpumaplen,
+ VIR_DOMAIN_AFFECT_CONFIG)) < 0)
+goto cleanup;
+
+for (i = 0; i < ncpus; i++) {
+if (i != 0)
+vshPrint(ctl, "\n");
+
+vshPrint(ctl, "%-15s %zu\n", _("VCPU:"), i);
+vshPrint(ctl, "%-15s %s\n", _("CPU:"), _("N/A"));
+vshPrint(ctl, "%-15s %s\n", _("State:"), _("N/A"));
+vshPrint(ctl, "%-15s %s\n", _("CPU time"), _("N/A"));
+
+if (virshVcpuinfoPrintAffinity(ctl, VIR_GET_CPUMAP(cpumaps, cpumaplen, 
i),
+   maxcpu, pretty) < 0)
+goto cleanup;
+}
+
+ret = true;
+
+ cleanup:
+VIR_FREE(cpumaps);
+return ret;
+}
+
+
+static bool
 cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd)
 {
 virDomainInfo info;
@@ -6343,32 +6386,22 @@ cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd)
 if (info.state != VIR_DOMAIN_SHUTOFF)
 goto cleanup;

-/* fall back to virDomainGetVcpuPinInfo and free cpuinfo to mark this 
*/
-VIR_FREE(cpuinfo);
-if ((ncpus = virDomainGetVcpuPinInfo(dom, info.nrVirtCpu,
- cpumaps, cpumaplen,
- VIR_DOMAIN_AFFECT_CONFIG)) < 0)
-goto cleanup;
+/* for offline VMs we can return pinning information */
+ret = virshVcpuinfoInactive(ctl, dom, info.nrVirtCpu, maxcpu, pretty);
+goto cleanup;
 }

 for (n = 0; n < ncpus; n++) {
-if (cpuinfo) {
-vshPrint(ctl, "%-15s %d\n", _("VCPU:"), cpuinfo[n].number);
-vshPrint(ctl, "%-15s %d\n", _("CPU:"), cpuinfo[n].cpu);
-vshPrint(ctl, "%-15s %s\n", _("State:"),
- virshDomainVcpuStateToString(cpuinfo[n].state));
-if (cpuinfo[n].cpuTime != 0) {
-double cpuUsed = cpuinfo[n].cpuTime;
+vshPrint(ctl, "%-15s %d\n", _("VCPU:"), cpuinfo[n].number);
+vshPrint(ctl, "%-15s %d\n", _("CPU:"), cpuinfo[n].cpu);
+vshPrint(ctl, "%-15s %s\n", _("State:"),
+ virshDomainVcpuStateToString(cpuinfo[n].state));
+if (cpuinfo[n].cpuTime != 0) {
+double cpuUsed = cpuinfo[n].cpuTime;

-cpuUsed /= 10.0;
+cpuUsed /= 10.0;

-vshPrint(ctl, "%-15s %.1lfs\n", _("CPU time:"), cpuUsed);
-}
-} else {
-vshPrint(ctl, "%-15s %d\n", _("VCPU:"), n);
-vshPrint(ctl, "%-15s %s\n", _("CPU:"), _("N/A"));
-vshPrint(ctl, "%-15s %s\n", _("State:"), _("N/A"));
-vshPrint(ctl, "%-15s %s\n", _("CPU time"), _("N/A"));
+vshPrint(ctl, "%-15s %.1lfs\n", _("CPU time:"), cpuUsed);
 }

 if (virshVcpuinfoPrintAffinity(ctl, VIR_GET_CPUMAP(cpumaps, cpumaplen, 
n),
-- 
2.10.0

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

[libvirt] [PATCH 2/6] virsh: domain: Fix broken indentation in virshCPUCountCollect

[Peter Krempa]

I managed to space most of the code by 5 spaces instead of 4 when
orignally implementing this function.
---
 tools/virsh-domain.c | 68 ++--
 1 file changed, 34 insertions(+), 34 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index b19f499..03bf032 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -6125,50 +6125,50 @@ virshCPUCountCollect(vshControl *ctl,
 return count;

 /* fallback code */
- if (!(last_error->code == VIR_ERR_NO_SUPPORT ||
-   last_error->code == VIR_ERR_INVALID_ARG))
- goto cleanup;
+if (!(last_error->code == VIR_ERR_NO_SUPPORT ||
+  last_error->code == VIR_ERR_INVALID_ARG))
+goto cleanup;

- if (flags & VIR_DOMAIN_VCPU_GUEST) {
- vshError(ctl, "%s", _("Failed to retrieve vCPU count from the 
guest"));
- goto cleanup;
- }
+if (flags & VIR_DOMAIN_VCPU_GUEST) {
+vshError(ctl, "%s", _("Failed to retrieve vCPU count from the guest"));
+goto cleanup;
+}

- if (!(flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) &&
- virDomainIsActive(dom) == 1)
- flags |= VIR_DOMAIN_AFFECT_LIVE;
+if (!(flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) &&
+virDomainIsActive(dom) == 1)
+flags |= VIR_DOMAIN_AFFECT_LIVE;

- vshResetLibvirtError();
+vshResetLibvirtError();

- if (flags & VIR_DOMAIN_AFFECT_LIVE) {
- if (flags & VIR_DOMAIN_VCPU_MAXIMUM) {
+if (flags & VIR_DOMAIN_AFFECT_LIVE) {
+if (flags & VIR_DOMAIN_VCPU_MAXIMUM) {
 count = virDomainGetMaxVcpus(dom);
- } else {
-if (virDomainGetInfo(dom, ) < 0)
-goto cleanup;
+} else {
+   if (virDomainGetInfo(dom, ) < 0)
+   goto cleanup;

-count = info.nrVirtCpu;
- }
- } else {
- if (!(def = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_INACTIVE)))
- goto cleanup;
+   count = info.nrVirtCpu;
+}
+} else {
+if (!(def = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_INACTIVE)))
+goto cleanup;

 if (!(xml = virXMLParseStringCtxt(def, _("(domain_definition)"), 
)))
 goto cleanup;

- if (flags & VIR_DOMAIN_VCPU_MAXIMUM) {
- if (virXPathInt("string(/domain/vcpus)", ctxt, ) < 0) {
- vshError(ctl, "%s", _("Failed to retrieve maximum vcpu 
count"));
- goto cleanup;
- }
- } else {
- if (virXPathInt("string(/domain/vcpus/@current)",
- ctxt, ) < 0) {
- vshError(ctl, "%s", _("Failed to retrieve current vcpu 
count"));
- goto cleanup;
- }
- }
- }
+if (flags & VIR_DOMAIN_VCPU_MAXIMUM) {
+if (virXPathInt("string(/domain/vcpus)", ctxt, ) < 0) {
+vshError(ctl, "%s", _("Failed to retrieve maximum vcpu 
count"));
+goto cleanup;
+}
+} else {
+if (virXPathInt("string(/domain/vcpus/@current)",
+ctxt, ) < 0) {
+vshError(ctl, "%s", _("Failed to retrieve current vcpu 
count"));
+goto cleanup;
+}
+}
+}

 ret = count;
  cleanup:
-- 
2.10.0

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

[libvirt] [PATCH 4/6] virsh: Extract cpumap formatting in cmdVcpuinfo

[Peter Krempa]

cmdVcpuinfo will be split in upcomming patches thus extract the common
code that formats pinning cpumaps for the vcpus.
---
 tools/virsh-domain.c | 52 +++-
 1 file changed, 35 insertions(+), 17 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 5fdad1b..8e1b9ed 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -6280,6 +6280,36 @@ static const vshCmdOptDef opts_vcpuinfo[] = {
 {.name = NULL}
 };

+
+static int
+virshVcpuinfoPrintAffinity(vshControl *ctl,
+   const unsigned char *cpumap,
+   int maxcpu,
+   bool pretty)
+{
+char *str = NULL;
+size_t i;
+int ret = -1;
+
+vshPrint(ctl, "%-15s ", _("CPU Affinity:"));
+if (pretty) {
+if (!(str = virBitmapDataToString(cpumap, VIR_CPU_MAPLEN(maxcpu
+goto cleanup;
+vshPrint(ctl, _("%s (out of %d)"), str, maxcpu);
+} else {
+for (i = 0; i < maxcpu; i++)
+vshPrint(ctl, "%c", VIR_CPU_USED(cpumap, i) ? 'y' : '-');
+}
+vshPrint(ctl, "\n");
+
+ret = 0;
+
+ cleanup:
+VIR_FREE(str);
+return ret;
+}
+
+
 static bool
 cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd)
 {
@@ -6291,7 +6321,7 @@ cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd)
 size_t cpumaplen;
 bool ret = false;
 bool pretty = vshCommandOptBool(cmd, "pretty");
-int n, m;
+int n;
 virshControlPtr priv = ctl->privData;

 if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
@@ -6340,23 +6370,11 @@ cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd)
 vshPrint(ctl, "%-15s %s\n", _("State:"), _("N/A"));
 vshPrint(ctl, "%-15s %s\n", _("CPU time"), _("N/A"));
 }
-vshPrint(ctl, "%-15s ", _("CPU Affinity:"));
-if (pretty) {
-char *str;

-str = virBitmapDataToString(VIR_GET_CPUMAP(cpumaps, cpumaplen, n),
-cpumaplen);
-if (!str)
-goto cleanup;
-vshPrint(ctl, _("%s (out of %d)"), str, maxcpu);
-VIR_FREE(str);
-} else {
-for (m = 0; m < maxcpu; m++) {
-vshPrint(ctl, "%c",
- VIR_CPU_USABLE(cpumaps, cpumaplen, n, m) ? 'y' : '-');
-}
-}
-vshPrint(ctl, "\n");
+if (virshVcpuinfoPrintAffinity(ctl, VIR_GET_CPUMAP(cpumaps, cpumaplen, 
n),
+   maxcpu, pretty) < 0)
+goto cleanup;
+
 if (n < (ncpus - 1))
 vshPrint(ctl, "\n");
 }
-- 
2.10.0

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

[libvirt] [PATCH 3/6] virsh: Fix xpath queries for retrieving vcpu count

[Peter Krempa]

The fallback code used if virDomainGetVcpusFlags is not supported used
wrong XPath queries and basically did not work at all. Fix them to point
to the   element instead of  which was not present
until lately.
---
 tools/virsh-domain.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 03bf032..5fdad1b 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -6157,13 +6157,12 @@ virshCPUCountCollect(vshControl *ctl,
 goto cleanup;

 if (flags & VIR_DOMAIN_VCPU_MAXIMUM) {
-if (virXPathInt("string(/domain/vcpus)", ctxt, ) < 0) {
+if (virXPathInt("string(/domain/vcpu)", ctxt, ) < 0) {
 vshError(ctl, "%s", _("Failed to retrieve maximum vcpu 
count"));
 goto cleanup;
 }
 } else {
-if (virXPathInt("string(/domain/vcpus/@current)",
-ctxt, ) < 0) {
+if (virXPathInt("string(/domain/vcpu/@current)", ctxt, ) < 
0) {
 vshError(ctl, "%s", _("Failed to retrieve current vcpu 
count"));
 goto cleanup;
 }
-- 
2.10.0

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

[libvirt] [PATCH 0/6] virsh: Fix 'vcpuinfo' on inactive VMs with new hotplug vcpus

[Peter Krempa]

See patch 6/6.


Peter Krempa (6):
  util: bitmap: Make bitmaps const in virBitmapNewData and
virBitmapDataToString
  virsh: domain: Fix broken indentation in virshCPUCountCollect
  virsh: Fix xpath queries for retrieving vcpu count
  virsh: Extract cpumap formatting in cmdVcpuinfo
  virsh: Extract fallback handling in cmdVcpuinfo
  virsh: vcpuinfo: Report proper vcpu numbers and data for offline VMs

 src/util/virbitmap.c |   6 +-
 src/util/virbitmap.h |   4 +-
 tools/virsh-domain.c | 287 ++-
 3 files changed, 219 insertions(+), 78 deletions(-)

-- 
2.10.0

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

[libvirt] [PATCH 6/6] virsh: vcpuinfo: Report proper vcpu numbers and data for offline VMs

[Peter Krempa]

If the VM is offline virsh attempted to at least report the pinning
information for the VM. This would not work properly now that the vcpus
can be sparse. Fix it by getting the vcpu states from the XML.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1375920
---
 tools/virsh-domain.c | 109 ++-
 1 file changed, 100 insertions(+), 9 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 84a4854..050e7fb 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -6310,37 +6310,125 @@ virshVcpuinfoPrintAffinity(vshControl *ctl,
 }


+static virBitmapPtr
+virshDomainGetVcpuBitmap(vshControl *ctl,
+ virDomainPtr dom,
+ bool inactive)
+{
+unsigned int flags = 0;
+char *def = NULL;
+virBitmapPtr ret = NULL;
+xmlDocPtr xml = NULL;
+xmlXPathContextPtr ctxt = NULL;
+xmlNodePtr *nodes = NULL;
+xmlNodePtr old;
+int nnodes;
+size_t i;
+unsigned int curvcpus = 0;
+unsigned int maxvcpus = 0;
+unsigned int vcpuid;
+char *online = NULL;
+
+if (inactive)
+flags |= VIR_DOMAIN_XML_INACTIVE;
+
+if (!(def = virDomainGetXMLDesc(dom, flags)))
+goto cleanup;
+
+if (!(xml = virXMLParseStringCtxt(def, _("(domain_definition)"), )))
+goto cleanup;
+
+if (virXPathUInt("string(/domain/vcpu)", ctxt, ) < 0) {
+vshError(ctl, "%s", _("Failed to retrieve maximum vcpu count"));
+goto cleanup;
+}
+
+ignore_value(virXPathUInt("string(/domain/vcpu/@current)", ctxt, 
));
+
+if (curvcpus == 0)
+curvcpus = maxvcpus;
+
+if (!(ret = virBitmapNew(maxvcpus)))
+goto cleanup;
+
+if ((nnodes = virXPathNodeSet("/domain/vcpus/vcpu", ctxt, )) <= 0) {
+/* if the specific vcpu state is missing provide a fallback */
+for (i = 0; i < curvcpus; i++)
+ignore_value(virBitmapSetBit(ret, i));
+
+goto cleanup;
+}
+
+old = ctxt->node;
+
+for (i = 0; i < nnodes; i++) {
+ctxt->node = nodes[i];
+
+if (virXPathUInt("string(@id)", ctxt, ) < 0 ||
+!(online = virXPathString("string(@enabled)", ctxt)))
+continue;
+
+if (STREQ(online, "yes"))
+ignore_value(virBitmapSetBit(ret, vcpuid));
+
+VIR_FREE(online);
+}
+
+ctxt->node = old;
+
+if (virBitmapCountBits(ret) != curvcpus) {
+vshError(ctl, "%s", _("Failed to retrieve vcpu state bitmap"));
+virBitmapFree(ret);
+ret = NULL;
+}
+
+ cleanup:
+VIR_FREE(online);
+VIR_FREE(nodes);
+xmlXPathFreeContext(ctxt);
+xmlFreeDoc(xml);
+VIR_FREE(def);
+return ret;
+}
+
+
 static bool
 virshVcpuinfoInactive(vshControl *ctl,
   virDomainPtr dom,
-  int nvcpus,
   int maxcpu,
   bool pretty)
 {
 unsigned char *cpumaps = NULL;
 size_t cpumaplen;
 int ncpus;
-size_t i;
+virBitmapPtr vcpus = NULL;
+ssize_t nextvcpu = -1;
 bool ret = false;
+bool first = true;
+
+if (!(vcpus = virshDomainGetVcpuBitmap(ctl, dom, true)))
+goto cleanup;

 cpumaplen = VIR_CPU_MAPLEN(maxcpu);
-cpumaps = vshMalloc(ctl, nvcpus * cpumaplen);
+cpumaps = vshMalloc(ctl, virBitmapSize(vcpus) * cpumaplen);

-if ((ncpus = virDomainGetVcpuPinInfo(dom, nvcpus,
+if ((ncpus = virDomainGetVcpuPinInfo(dom, virBitmapSize(vcpus),
  cpumaps, cpumaplen,
  VIR_DOMAIN_AFFECT_CONFIG)) < 0)
 goto cleanup;

-for (i = 0; i < ncpus; i++) {
-if (i != 0)
+while ((nextvcpu = virBitmapNextSetBit(vcpus, nextvcpu)) >= 0) {
+if (!first)
 vshPrint(ctl, "\n");
+first = false;

-vshPrint(ctl, "%-15s %zu\n", _("VCPU:"), i);
+vshPrint(ctl, "%-15s %zd\n", _("VCPU:"), nextvcpu);
 vshPrint(ctl, "%-15s %s\n", _("CPU:"), _("N/A"));
 vshPrint(ctl, "%-15s %s\n", _("State:"), _("N/A"));
 vshPrint(ctl, "%-15s %s\n", _("CPU time"), _("N/A"));

-if (virshVcpuinfoPrintAffinity(ctl, VIR_GET_CPUMAP(cpumaps, cpumaplen, 
i),
+if (virshVcpuinfoPrintAffinity(ctl,
+   VIR_GET_CPUMAP(cpumaps, cpumaplen, 
nextvcpu),
maxcpu, pretty) < 0)
 goto cleanup;
 }
@@ -6348,6 +6436,7 @@ virshVcpuinfoInactive(vshControl *ctl,
 ret = true;

  cleanup:
+virBitmapFree(vcpus);
 VIR_FREE(cpumaps);
 return ret;
 }
@@ -6386,8 +6475,10 @@ cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd)
 if (info.state != VIR_DOMAIN_SHUTOFF)
 goto cleanup;

+vshResetLibvirtError();
+
 /* for offline VMs we can return pinning information */
-ret = virshVcpuinfoInactive(ctl, dom, info.nrVirtCpu, maxcpu, pretty);
+ret = 

[libvirt] [PATCH 1/6] util: bitmap: Make bitmaps const in virBitmapNewData and virBitmapDataToString

[Peter Krempa]

The functions just read the passed pointer so it can be marked as const.
---
 src/util/virbitmap.c | 6 +++---
 src/util/virbitmap.h | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
index 3b85c16..0c04f1a 100644
--- a/src/util/virbitmap.c
+++ b/src/util/virbitmap.c
@@ -689,12 +689,12 @@ virBitmapPtr virBitmapNewCopy(virBitmapPtr src)
  * Returns a pointer to the allocated bitmap or NULL if
  * memory cannot be allocated.
  */
-virBitmapPtr virBitmapNewData(void *data, int len)
+virBitmapPtr virBitmapNewData(const void *data, int len)
 {
 virBitmapPtr bitmap;
 size_t i, j;
 unsigned long *p;
-unsigned char *bytes = data;
+const unsigned char *bytes = data;

 bitmap = virBitmapNew(len * CHAR_BIT);
 if (!bitmap)
@@ -1058,7 +1058,7 @@ virBitmapCountBits(virBitmapPtr bitmap)
  * Returns: a string representation of the data, or NULL on error
  */
 char *
-virBitmapDataToString(void *data,
+virBitmapDataToString(const void *data,
   int len)
 {
 virBitmapPtr map = NULL;
diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h
index 5984b80..58e0ee6 100644
--- a/src/util/virbitmap.h
+++ b/src/util/virbitmap.h
@@ -101,7 +101,7 @@ virBitmapParseUnlimited(const char *str,

 virBitmapPtr virBitmapNewCopy(virBitmapPtr src) ATTRIBUTE_NONNULL(1);

-virBitmapPtr virBitmapNewData(void *data, int len) ATTRIBUTE_NONNULL(1);
+virBitmapPtr virBitmapNewData(const void *data, int len) ATTRIBUTE_NONNULL(1);

 int virBitmapToData(virBitmapPtr bitmap, unsigned char **data, int *dataLen)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
@@ -138,7 +138,7 @@ ssize_t virBitmapNextClearBit(virBitmapPtr bitmap, ssize_t 
pos)
 size_t virBitmapCountBits(virBitmapPtr bitmap)
 ATTRIBUTE_NONNULL(1);

-char *virBitmapDataToString(void *data,
+char *virBitmapDataToString(const void *data,
 int len)
 ATTRIBUTE_NONNULL(1);
 bool virBitmapOverlaps(virBitmapPtr b1,
-- 
2.10.0

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

Re: [libvirt] [PATCH 3/4] vbox: change API (un)initialization logic.

[Martin Kletzander]

On Tue, Oct 11, 2016 at 11:23:13AM -0400, Dawid Zamirski wrote:

On Tue, 2016-10-11 at 16:22 +0200, Martin Kletzander wrote:

On Wed, Sep 28, 2016 at 01:41:35PM -0400, Dawid Zamirski wrote:
>
> * _pfnUnitilalize decrements connectionCount in g_pVBoxGlobalData
> and
>  once it is down to 0, it calls pfnComUnitialize and
>  g_pVBoxGlobalData if free'd. This ensures that pfnComInitialize
> and
>  pfnComUnitialize pair is called only once, even when multiple
>  concurrent connections are in use.

That's not true if there is a connection being made while it is being
free()d or it's being allocated in two threads, etc.  Unless I missed
something, that is.


On a second thought, seeing how both _pfnInitialize and _pfnUnintialize
(each called in virConnectOpen/Close respectively) obtain a mutex on
g_pVBoxGlobalData, I think my original statement holds true, that is,
ignoring the unsafe allocation of the global as you pointed out
earlier.



They can't take mutex on g_pVBoxGlobalData if it is not initialized
(allocated) yet.  Or is there another one?


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


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

Re: [libvirt] [PATCH 3/4] vbox: change API (un)initialization logic.

[Martin Kletzander]

On Tue, Oct 11, 2016 at 10:58:47AM -0400, Dawid Zamirski wrote:

On Tue, 2016-10-11 at 16:22 +0200, Martin Kletzander wrote:

On Wed, Sep 28, 2016 at 01:41:35PM -0400, Dawid Zamirski wrote:

The allocation is not guarded by any lock, so there's still a
race.  I
think there should be a global struct that has only some lock in it
and
whatever global data you need, the struct will be initialized on the
first call to any function (check out VIR_ONCE_GLOBAL_INIT) and then
the
connection (or global data or how it's called) would be reference
counted (just like you have).  It's just that having the reference
count
in the object you will be reallocating over and over again for each
connection is not really good.



Thanks, I see, I'll address this in v2


I don't understand how vbox works, but if initializing
g_pVBoxGlobalData
does not make any connection, and ISession does (which would make
sense), we could keep the global data around and add ISession for
each connection.  I guess that's something you're talking about
below.


Neither I'm super familiar with vbox internals, but your suggestion
sounds reasonable, so I'll dive into that code in vbox source to find
out.



>
> * _pfnInitialize sets up the virConnectPtr->privateData (aka
>  vboxPrivateData) for each connection by copying references to
>  ISession and IVirtualBox from g_pVBoxGlobalData so that the rest
> of
>  the driver API can use it without referencing the global. Each
> time
>  this happens, a conntionCount is incremented on g_pVBoxGlobalData
> to
>  keep track of when it's safe to call pfnComUnitialize. One of the
>  reasons for existence of per-connection vboxPrivateData rather
> than
>  completely relying on vboxGlobalData, is that more modern VBOX
> APIs
>  provide pfnClientInitialize (since 4.2.20 and 4.3.4) and
>  pfnClientThreadInitialize (5.0+) that allow to create multiple
>  instances of ISession so if the code ever gets ported to support
>  those newer APIs it should create much less diff noise as all API
>  implementations are already updated to assume per-connection
>  ISession/IVirutalBox instances.
>
> * _pfnUnitilalize decrements connectionCount in g_pVBoxGlobalData
> and
>  once it is down to 0, it calls pfnComUnitialize and
>  g_pVBoxGlobalData if free'd. This ensures that pfnComInitialize
> and
>  pfnComUnitialize pair is called only once, even when multiple
>  concurrent connections are in use.

That's not true if there is a connection being made while it is being
free()d or it's being allocated in two threads, etc.  Unless I missed
something, that is.


Understood, though I figure that your initial suggestion to guard
allocation of g_pVBoxGlobalData should take care of this?


Yes, that would do.


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

Re: [libvirt] [PATCH 3/4] vbox: change API (un)initialization logic.

[Dawid Zamirski]

On Tue, 2016-10-11 at 16:22 +0200, Martin Kletzander wrote:
> On Wed, Sep 28, 2016 at 01:41:35PM -0400, Dawid Zamirski wrote:
> > 
> > * _pfnUnitilalize decrements connectionCount in g_pVBoxGlobalData
> > and
> >  once it is down to 0, it calls pfnComUnitialize and
> >  g_pVBoxGlobalData if free'd. This ensures that pfnComInitialize
> > and
> >  pfnComUnitialize pair is called only once, even when multiple
> >  concurrent connections are in use.
> 
> That's not true if there is a connection being made while it is being
> free()d or it's being allocated in two threads, etc.  Unless I missed
> something, that is.

On a second thought, seeing how both _pfnInitialize and _pfnUnintialize
(each called in virConnectOpen/Close respectively) obtain a mutex on
g_pVBoxGlobalData, I think my original statement holds true, that is,
ignoring the unsafe allocation of the global as you pointed out
earlier.

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

Re: [libvirt] [PATCH 4/8] conf, qemu: Add newer shmem models

[Martin Kletzander]

On Fri, Oct 07, 2016 at 11:09:54AM -0400, John Ferlan wrote:



On 09/27/2016 08:24 AM, Martin Kletzander wrote:

The old ivshmem is deprecated in QEMU, so let's use the better
ivshmem-{plain,doorbell} variants instead.



Perhaps explained a bit differently (my understanding ;-))... Older
versions of qemu had one type (ivshmem - the current default), but newer
versions of qemu will force selection of a specific type using "plain"
or "doorbell" as the model type depending on which "features" are
desired (??) [fill in the details]



Old ivshmem could've been optionally used with or without "server"
option (it would connect to a UNIX socket and talk to the server and so
on.  That got broken up to two separate new devices.  One without server
(-plain) and one with the server (-doorbell).  As mentioned in one of
the previous series, I could've add model named something like
'ivshmem-newer' and under the covers choose -plain or -doorbell, but 1)
it would be really stupid and 2) the differentiation could change in the
future and being specific is better than being bitten in the butt later
on.


I guess the nagging question is - what happens if someone "chooses" to
have  in this newer world?  Does the deprecation
from qemu kick in and the domain cannot start?  We should note that
somehow/somewhere.



They get a warning in the log saying "ivshmem is deprecated, please use
ivshmem-plain or ivshmem-doorbell instead".  And if QEMU stops
supporting that, we'll know about it thanks to the capabilities and tell
the user that the model is not available (like for _almost_ all other
devices).


Signed-off-by: Martin Kletzander 
---
 docs/schemas/domaincommon.rng | 2 ++
 src/conf/domain_conf.c| 4 +++-
 src/conf/domain_conf.h| 2 ++
 src/qemu/qemu_command.c   | 7 +++
 4 files changed, 14 insertions(+), 1 deletion(-)



docs/formatdomain.html.in needs to change here too

In particular what constitutes each setting - that is are there XML
elements that would or wouldn't make sense for the type of shmem device
(I'm thinking of Daniel's table here).

BTW: When using your "Since" tags, it'll need to be clear which version
of qemu no longer supports "type='ivshmem'" and forces someone to choose
-plain or -doorbell.



I'd rather remove the Since (at least for QEMU, maybe leave one for
libvirt), the version doesn't mean poopsicle.  I can see the nitpicky
people saying: "look, I have this QEMU and it says it shouldn't be
supported and it is, isn't this a bug?" because some distribution chose
to backport one patch.  I don't think the versions make sense, everyone
can lookup when it was introduced.  I believe it's not that hard.


ACK w/ the doc change


John




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

Re: [libvirt] [PATCH 3/4] vbox: change API (un)initialization logic.

[Dawid Zamirski]

On Tue, 2016-10-11 at 16:22 +0200, Martin Kletzander wrote:
> On Wed, Sep 28, 2016 at 01:41:35PM -0400, Dawid Zamirski wrote:
> 
> The allocation is not guarded by any lock, so there's still a
> race.  I
> think there should be a global struct that has only some lock in it
> and
> whatever global data you need, the struct will be initialized on the
> first call to any function (check out VIR_ONCE_GLOBAL_INIT) and then
> the
> connection (or global data or how it's called) would be reference
> counted (just like you have).  It's just that having the reference
> count
> in the object you will be reallocating over and over again for each
> connection is not really good.
> 

Thanks, I see, I'll address this in v2

> I don't understand how vbox works, but if initializing
> g_pVBoxGlobalData
> does not make any connection, and ISession does (which would make
> sense), we could keep the global data around and add ISession for
> each connection.  I guess that's something you're talking about
> below.

Neither I'm super familiar with vbox internals, but your suggestion
sounds reasonable, so I'll dive into that code in vbox source to find
out.

> 
> > 
> > * _pfnInitialize sets up the virConnectPtr->privateData (aka
> >  vboxPrivateData) for each connection by copying references to
> >  ISession and IVirtualBox from g_pVBoxGlobalData so that the rest
> > of
> >  the driver API can use it without referencing the global. Each
> > time
> >  this happens, a conntionCount is incremented on g_pVBoxGlobalData
> > to
> >  keep track of when it's safe to call pfnComUnitialize. One of the
> >  reasons for existence of per-connection vboxPrivateData rather
> > than
> >  completely relying on vboxGlobalData, is that more modern VBOX
> > APIs
> >  provide pfnClientInitialize (since 4.2.20 and 4.3.4) and
> >  pfnClientThreadInitialize (5.0+) that allow to create multiple
> >  instances of ISession so if the code ever gets ported to support
> >  those newer APIs it should create much less diff noise as all API
> >  implementations are already updated to assume per-connection
> >  ISession/IVirutalBox instances.
> > 
> > * _pfnUnitilalize decrements connectionCount in g_pVBoxGlobalData
> > and
> >  once it is down to 0, it calls pfnComUnitialize and
> >  g_pVBoxGlobalData if free'd. This ensures that pfnComInitialize
> > and
> >  pfnComUnitialize pair is called only once, even when multiple
> >  concurrent connections are in use.
> 
> That's not true if there is a connection being made while it is being
> free()d or it's being allocated in two threads, etc.  Unless I missed
> something, that is.

Understood, though I figure that your initial suggestion to guard
allocation of g_pVBoxGlobalData should take care of this?

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

Re: [libvirt] [PATCH 5/8] qemu: Add capabilities for ivshmem-{plain, doorbell}

[Martin Kletzander]

On Fri, Oct 07, 2016 at 11:11:53AM -0400, John Ferlan wrote:



On 09/27/2016 08:24 AM, Martin Kletzander wrote:

Signed-off-by: Martin Kletzander 
---
 src/qemu/qemu_capabilities.c| 4 
 src/qemu/qemu_capabilities.h| 2 ++
 tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml | 2 ++
 tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml | 2 ++
 tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml   | 2 ++
 tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml| 2 ++
 tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml| 2 ++
 7 files changed, 16 insertions(+)



re-ACK (this is ACK'd previously somewhere IIRC)



Well, yeah, this is just being reposted because pushing it doesn't make
sense until it's used (we would end up with the flag and had to support
it even if we didn't use it).


John

The question becomes who gets to push first and who gets to merge their
capabilities changes ;-)



This is *always* the case.  Of course.





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

Re: [libvirt] [PATCH 3/8] conf, qemu: Add support for shmem model

[Martin Kletzander]

On Fri, Oct 07, 2016 at 03:55:44PM +0100, Daniel P. Berrange wrote:

On Fri, Oct 07, 2016 at 10:53:48AM -0400, John Ferlan wrote:



On 09/27/2016 08:24 AM, Martin Kletzander wrote:
> Just the default one now, new ones will be added in following commits.
>
> Signed-off-by: Martin Kletzander 
> ---
>  docs/schemas/domaincommon.rng |  9 +
>  src/conf/domain_conf.c| 44 
+--
>  src/conf/domain_conf.h|  8 +
>  src/libvirt_private.syms  |  2 ++
>  src/qemu/qemu_command.c   | 11 +-
>  tests/qemuxml2argvdata/qemuxml2argv-shmem.xml |  2 ++
>  tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml |  8 +
>  7 files changed, 72 insertions(+), 12 deletions(-)
>

docs/formatdomain.html.in ??

Just so I'm sure I understand ;-)... This is the existing 'model type'
for 'shmem' being implemented as the "default" (e.g. 0 entry) because we
know at some short time in the future we're going to be adding a new
type (or two).



This is the 'ivshmem' being implemented as default if you don't pick any
due to the fact that there are some older domains that already have
 without  and we're sure they are using 'ivshmem'.  So
we don't want to change that.  We could add a MODEL_DEFAULT and
MODEL_IVSHMEM, but we would have to set it to _IVSHMEM in the PostParse
anyway because _DEFAULT doesn't make sense here (in contrast to other
places/options).  That is because libvirt *has* to choose the model,
there is no way to let the hypervisor (QEMU) choose.


Since the  is now going to be the default on
output, we should explain in some way... and encourage choosing
something else because "at some point in the future" ;-) we'll deprecate
this one (whenever that dire time exists who knows).

Making sure #2 - we don't have to care about save files, true?  Since
the default will be to now to ShmemDefFormat the  - a save file
read on an older libvirt will have a failure, but that'd be true for any
XML change I suppose.


It won't, it would just *not* be parsed.  We're not iterating over all
children elements, we're just picking the ones we want.  Have you ever
edited the XML and lost the stuff you added because of a typo?  Well
that's exactly why.  And also why we added RNG schema validation, I
believe.



IIRC ivshmem is non-migratable, so its impossible to have any existing
save files.



It's not that black-n-white, there are some grey areas.  But we disabled
the migration in previous commit due to the fact that it was never safe
or that it would work for sure.  Plus it doesn't make much sense, so
IMNSHO we don't have to have a problem wrt migration.



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


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

[libvirt] [Fwd: Re: [PATCH 2/4] vbox: replace vboxGlobalData with vboxPrivate.]

[Dawid Zamirski]

Sorry, forgot to CC the ML in my last reply.

 Forwarded Message 
From: Dawid Zamirski 
To: Martin Kletzander 
Subject: Re: [libvirt] [PATCH 2/4] vbox: replace vboxGlobalData with
vboxPrivate.
Date: Tue, 11 Oct 2016 10:43:23 -0400

On Tue, 2016-10-11 at 15:47 +0200, Martin Kletzander wrote:
> 
> 
> and this (and others) you *just* cast it to different struct.  That's
> not good.  I'm guessing this is still just a differently separated
> commit.

Yes, this was to just make simple changes all over the code base but
keep the commit in "compilable" state for bisecting reasons. Same goes
for the first patch in the series.

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

Re: [libvirt] [PATCH] libvirt-guests: Weaken dependency on libvirtd

[Martin Kletzander]

On Tue, Oct 11, 2016 at 03:17:07PM +0200, Andrea Bolognani wrote:

On Mon, 2016-10-10 at 15:59 +0200, Martin Kletzander wrote:

On Fri, Oct 07, 2016 at 10:19:53AM +0200, Andrea Bolognani wrote:
> 
> The Requires relationship is very strong, in that it prevents
> a unit from running unless all the units it Requires are
> running as well.
> 
> This turns out to be a problem because we want to be able to
> restart libvirtd at any time without having libvirt-guests
> suspend or shutdown running guests.
> 
> Turn the Requires relationship into a Wants relationship:
> this way starting libvirt-guests will cause systemd to (attempt
> to) start libvirtd as well, but stopping or restarting libvirtd
> will not alter libvirt-guests' running state.
 
I can't figure out how exactly this works, even when looking at the
systemd.unit documentation.  You are saying that Wants= means that if
you issue 'service libvirtd stop'


You mean

  systemctl stop libvirtd

Unless distributions have converted service(8) into a wrapper
for systemctl(1)... Oh, it looks like at least Fedora and
Debian have. Makes sense :)


it will not save your guests after
this patch.


Correct. If B Requires=A and A becomes unavailable, B will be
stopped as well; the same doesn't happen if B merely Wants=A.


What if you stop the service and then shutdown the machine?
I would agree that it's your fault if it doesn't save your guests.


It would be super neat if we could stop libvirt-guests when
stopping libvirtd, so that guests are saved properly, but not
stop it when libvirtd is merely being restarted.

Unfortunately systemd doesn't allow that level of granularity,
so we're basically forced to choose between two partially
unsatisfying solutions :(



Sometimes you don't even want that.  Maybe systemd could add yet another
new thing that would mean this service needs this other service when it
is being stopped or started, but not in between.  And also this service
should change state when starting or shutting down only... and bunch of
others.  Until that we'll always have bunch of workarounds anyway.


From
what I am reading, it makes sense, so ACK from me.  Basically because I
can't come up with scenarios that could be broken by this change =)


I will wait a while before pushing it, so other people can
have a chance to weigh in.

In the meantime, thanks for the review! :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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


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

Re: [libvirt] [PATCH 3/4] vbox: change API (un)initialization logic.

[Martin Kletzander]

On Wed, Sep 28, 2016 at 01:41:35PM -0400, Dawid Zamirski wrote:

Since VBOX API initialization method (pfnComInitialize) is not
thread-safe and must be called from the primary thread, calling it in
every vboxConnectOpen (as we used to do) leads to segmentation
faults when multiple connections are in use. Therefore the initalization
and unitialization logic has been changed as the following:

* _registerGlobalData allocates g_pVBoxGlobalData (only when not
 already allocated) and calls VBOX's pfnComInitialize to grab
 references to ISession and IVirtualBox objects. This ensures that
 pfnComInitialize is called when the first connection is established.



The allocation is not guarded by any lock, so there's still a race.  I
think there should be a global struct that has only some lock in it and
whatever global data you need, the struct will be initialized on the
first call to any function (check out VIR_ONCE_GLOBAL_INIT) and then the
connection (or global data or how it's called) would be reference
counted (just like you have).  It's just that having the reference count
in the object you will be reallocating over and over again for each
connection is not really good.

I don't understand how vbox works, but if initializing g_pVBoxGlobalData
does not make any connection, and ISession does (which would make
sense), we could keep the global data around and add ISession for each
connection.  I guess that's something you're talking about below.


* _pfnInitialize sets up the virConnectPtr->privateData (aka
 vboxPrivateData) for each connection by copying references to
 ISession and IVirtualBox from g_pVBoxGlobalData so that the rest of
 the driver API can use it without referencing the global. Each time
 this happens, a conntionCount is incremented on g_pVBoxGlobalData to
 keep track of when it's safe to call pfnComUnitialize. One of the
 reasons for existence of per-connection vboxPrivateData rather than
 completely relying on vboxGlobalData, is that more modern VBOX APIs
 provide pfnClientInitialize (since 4.2.20 and 4.3.4) and
 pfnClientThreadInitialize (5.0+) that allow to create multiple
 instances of ISession so if the code ever gets ported to support
 those newer APIs it should create much less diff noise as all API
 implementations are already updated to assume per-connection
 ISession/IVirutalBox instances.

* _pfnUnitilalize decrements connectionCount in g_pVBoxGlobalData and
 once it is down to 0, it calls pfnComUnitialize and
 g_pVBoxGlobalData if free'd. This ensures that pfnComInitialize and
 pfnComUnitialize pair is called only once, even when multiple
 concurrent connections are in use.


That's not true if there is a connection being made while it is being
free()d or it's being allocated in two threads, etc.  Unless I missed
something, that is.


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

Re: [libvirt] [Qemu-devel] [PATCH v6 3/3] target-i386: Return runnability information on query-cpu-definitions

[Igor Mammedov]

On Fri,  7 Oct 2016 17:29:02 -0300
Eduardo Habkost  wrote:

> Fill the "unavailable-features" field on the x86 implementation
> of query-cpu-definitions.
> 
> Cc: Jiri Denemark 
> Cc: libvir-list@redhat.com
> Signed-off-by: Eduardo Habkost 
Reviewed-by: Igor Mammedov 

> ---
> Changes v5 -> v6:
> * Call x86_cpu_filter_features(), now that x86_cpu_load_features()
>   won't run it automatically
> 
> Changes v4 -> v5:
> * (none)
> 
> Changes v3 -> v4:
> * Handle missing XSAVE components cleanly, but looking up
>   the original feature that required it
> * Use x86_cpu_load_features() function
> 
> Changes v2 -> v3:
> * Create a x86_cpu_feature_name() function, to
>   isolate the code that returns the property name
> 
> Changes v1 -> v2:
> * Updated to the new schema: no @runnable field, and
>   always report @unavailable-features as present
> ---
>  target-i386/cpu.c | 76 
> +++
>  1 file changed, 76 insertions(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 23cc19b..63330ce 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1945,6 +1945,27 @@ static inline void feat2prop(char *s)
>  }
>  }
>  
> +/* Return the feature property name for a feature flag bit */
> +static const char *x86_cpu_feature_name(FeatureWord w, int bitnr)
> +{
> +/* XSAVE components are automatically enabled by other features,
> + * so return the original feature name instead
> + */
> +if (w == FEAT_XSAVE_COMP_LO || w == FEAT_XSAVE_COMP_HI) {
> +int comp = (w == FEAT_XSAVE_COMP_HI) ? bitnr + 32 : bitnr;
> +
> +if (comp < ARRAY_SIZE(x86_ext_save_areas) &&
> +x86_ext_save_areas[comp].bits) {
> +w = x86_ext_save_areas[comp].feature;
> +bitnr = ctz32(x86_ext_save_areas[comp].bits);
> +}
> +}
> +
> +assert(bitnr < 32);
> +assert(w < FEATURE_WORDS);
> +return feature_word_info[w].feat_names[bitnr];
> +}
> +
>  /* Compatibily hack to maintain legacy +-feat semantic,
>   * where +-feat overwrites any feature set by
>   * feat=on|feat even if the later is parsed after +-feat
> @@ -2030,6 +2051,59 @@ static void x86_cpu_parse_featurestr(const char 
> *typename, char *features,
>  }
>  }
>  
> +static void x86_cpu_load_features(X86CPU *cpu, Error **errp);
> +static int x86_cpu_filter_features(X86CPU *cpu);
> +
> +/* Check for missing features that may prevent the CPU class from
> + * running using the current machine and accelerator.
> + */
> +static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,
> + strList **missing_feats)
> +{
> +X86CPU *xc;
> +FeatureWord w;
> +Error *err = NULL;
> +strList **next = missing_feats;
> +
> +if (xcc->kvm_required && !kvm_enabled()) {
> +strList *new = g_new0(strList, 1);
> +new->value = g_strdup("kvm");;
> +*missing_feats = new;
> +return;
> +}
> +
> +xc = X86_CPU(object_new(object_class_get_name(OBJECT_CLASS(xcc;
> +
> +x86_cpu_load_features(xc, );
> +if (err) {
> +/* Errors at x86_cpu_load_features should never happen,
> + * but in case it does, just report the model as not
> + * runnable at all using the "type" property.
> + */
> +strList *new = g_new0(strList, 1);
> +new->value = g_strdup("type");
> +*next = new;
> +next = >next;
> +}
> +
> +x86_cpu_filter_features(xc);
> +
> +for (w = 0; w < FEATURE_WORDS; w++) {
> +uint32_t filtered = xc->filtered_features[w];
> +int i;
> +for (i = 0; i < 32; i++) {
> +if (filtered & (1UL << i)) {
> +strList *new = g_new0(strList, 1);
> +new->value = g_strdup(x86_cpu_feature_name(w, i));
> +*next = new;
> +next = >next;
> +}
> +}
> +}
> +
> +object_unref(OBJECT(xc));
> +}
> +
>  /* Print all cpuid feature names in featureset
>   */
>  static void listflags(FILE *f, fprintf_function print, const char 
> **featureset)
> @@ -2122,6 +2196,8 @@ static void x86_cpu_definition_entry(gpointer data, 
> gpointer user_data)
>  
>  info = g_malloc0(sizeof(*info));
>  info->name = x86_cpu_class_get_model_name(cc);
> +x86_cpu_class_check_missing_features(cc, >unavailable_features);
> +info->has_unavailable_features = true;
>  
>  entry = g_malloc0(sizeof(*entry));
>  entry->value = info;

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

Re: [libvirt] [PATCH 0/4] systemd-related fixes and improvements

[Andrea Bolognani]

On Tue, 2016-10-11 at 14:59 +0200, Boris Fiuczynski wrote:
> > I tried upgrading from 2.2.0 to 2.3.0 a bunch of times, but
> > I haven't been able to reproduce the failure you're
> > reporting: libvirt-guests is never started automatically.
> > 
> > Can you provide more information? What distribution are
> > you using? Are you building from source, or using your
> > distribution's packages? Are you sure libvirt-guests was
> > not running even before upgrade?
> 
> I used fc20 and built the libvirt rpms from source. libvirt-guests was 
> not running when updating from 2.2.0 to libvirt 2.3.0. I switched 
> systems and retried on fc23 after rebuilding libvirt 2.2.0 and 2.3.0 
> from source. On this system I cannot reproduce the behaviour.
> You are correct that the behaviour I got does not match with the 
> requires directive. So my guess is that my old fc20 might still have 
> some systemd problem which has been fixed in newer fc versions.

That's great to hear! Thanks for providing feedback :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v6 3/3] target-i386: Return runnability information on query-cpu-definitions

[Igor Mammedov]

On Tue, 11 Oct 2016 10:24:38 -0300
Eduardo Habkost  wrote:

> On Tue, Oct 11, 2016 at 03:21:05PM +0200, Igor Mammedov wrote:
> > On Tue, 11 Oct 2016 08:58:02 -0300
> > Eduardo Habkost  wrote:
> >   
> > > On Tue, Oct 11, 2016 at 01:45:21PM +0200, Igor Mammedov wrote:  
> > > > On Mon, 10 Oct 2016 14:01:10 -0300
> > > > Eduardo Habkost  wrote:
> > > > > On Mon, Oct 10, 2016 at 02:27:49PM +0200, Igor Mammedov wrote:
> > > > > > On Fri,  7 Oct 2016 17:29:02 -0300
> > > > > > Eduardo Habkost  wrote:
> > > [...]  
> > > > > > > +static void x86_cpu_class_check_missing_features(X86CPUClass 
> > > > > > > *xcc,
> > > > > > > + strList 
> > > > > > > **missing_feats)
> > > > > > > +{
> > > > > > > +X86CPU *xc;
> > > > > > > +FeatureWord w;
> > > > > > > +Error *err = NULL;
> > > > > > > +strList **next = missing_feats;
> > > > > > > +
> > > > > > > +if (xcc->kvm_required && !kvm_enabled()) {
> > > > > > > +strList *new = g_new0(strList, 1);
> > > > > > > +new->value = g_strdup("kvm");;
> > > > > > > +*missing_feats = new;
> > > > > > > +return;
> > > > > > > +}
> > > > > > > +
> > > > > > > +xc = 
> > > > > > > X86_CPU(object_new(object_class_get_name(OBJECT_CLASS(xcc;
> > > > > > > +
> > > > > > > +x86_cpu_load_features(xc, );
> > > > > > > +if (err) {
> > > > > > > +/* Errors at x86_cpu_load_features should never happen,
> > > > > > > + * but in case it does, just report the model as not
> > > > > > > + * runnable at all using the "type" property.
> > > > > > > + */
> > > > > > > +strList *new = g_new0(strList, 1);
> > > > > > > +new->value = g_strdup("type");
> > > > > > > +*next = new;
> > > > > > > +next = >next;
> > > > > > > +}
> > > > > > > +
> > > > > > > +x86_cpu_filter_features(xc);
> > > > > > > +
> > > > > > > +for (w = 0; w < FEATURE_WORDS; w++) {
> > > > > > > +uint32_t filtered = xc->filtered_features[w];
> > > > > > > +int i;
> > > > > > > +for (i = 0; i < 32; i++) {
> > > > > > > +if (filtered & (1UL << i)) {
> > > > > > > +strList *new = g_new0(strList, 1);
> > > > > > > +new->value = g_strdup(x86_cpu_feature_name(w, 
> > > > > > > i));
> > > > > > > +*next = new;
> > > > > > > +next = >next;
> > > > > > > +}
> > > > > > > +}
> > > > > > > +}  
> > > > > > Shouldn't you add 
> > > > > >if (IS_AMD_CPU(env)) { 
> > > > > > fixup here, that realize does right after calling 
> > > > > > x86_cpu_filter_features()  
> > > > > 
> > > > > What would it be useful for? The IS_AMD_CPU fixup runs after
> > > > > x86_cpu_filter_features() (so it doesn't affect filtered_features
> > > > > at all), and filtered_features is the only field used as input to
> > > > > build missing_feats.
> > > > For completeness of features returned by query-cpu-definitions, I'd 
> > > > guess.
> > > > So that returned cpu definitions would match actually created cpus.
> > > 
> > > For completeness of which query-cpu-definitions field, exactly?
> > > There's no field in the return value of query-cpu-definitions
> > > that would be affected by the AMD aliases. The AMD aliases don't
> > > affect runnability of the CPU model because they aren't included
> > > in the GET_SUPPORTED_CPUID check[1].
> > > 
> > > You would be right if we did return a copy of the low-level CPUID
> > > data that's seen by the guest, or if the AMD aliases were set up
> > > before x86_cpu_filter_features() (so they could affect
> > > filtered_features/unavailable-features), but that's not the case.
> > > 
> > > [1] They aren't included in the GET_SUPPORTED_CPUID check because
> > > the existence of the AMD aliases depend only on the
> > > configured guest vendor ID, not on the host CPU.
> > >   
> > Got it.
> > 
> > I've tried to build with this patch but build fails with
> > 
> > make -j32
> > CHK version_gen.h
> >   CC  i386-linux-user/target-i386/cpu.o
> >   target-i386/cpu.c: In function ‘x86_cpu_definition_entry’:
> >   target-i386/cpu.c:2199:51: error: ‘CpuDefinitionInfo’ has no 
> > member named ‘unavailable_features’
> >  x86_cpu_class_check_missing_features(cc, >unavailable_features);
> >^
> >   target-i386/cpu.c:2200:9: error: ‘CpuDefinitionInfo’ has no 
> > member named ‘has_unavailable_features’
> >  info->has_unavailable_features = true;
> > 
> > Probably series misses a patch that adds it.  
> 
> See git URLs on cover letter. Series is based on my x86-next branch.
> 
> ] This series can be seen in the git branch at:
> ]   https://github.com/ehabkost/qemu-hacks.git 
> work/query-cpu-definitions-runnable-info
> ]
> ] 

Re: [libvirt] [OSSTEST PATCH 1/2] libvirt: Check migration capabilities using proper XML parser

[Ian Jackson]

Julien Grall writes ("Re: [OSSTEST PATCH 1/2] libvirt: Check migration 
capabilities using proper XML parser"):
> live migration is not currently the supported but will be in the future. 
> FWIW, there is a patch series on xen-devel to support dead migration 
> (first step for live migration) [1].
> 
> I hope this answer to your question.

Yes, thanks.

Thanks everyone, the patches (as discessed etc.) are now in the
osstest self-push-gate.

Ian.

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

Re: [libvirt] [PATCH 2/4] vbox: replace vboxGlobalData with vboxPrivate.

[Martin Kletzander]

On Wed, Sep 28, 2016 at 01:41:34PM -0400, Dawid Zamirski wrote:

Since those stucts are identical at the moment, this patch basically
does s/vboxGlobalData \*data/vboxPrivate *data/ with type casts in
vboxDriverLock/Unlock calls to keep the code working and take care of
unavoidable diff noise to set the stage for further commits that
actually address the issue for the patch series.
---
src/vbox/vbox_common.c| 164 +-
src/vbox/vbox_network.c   |  24 +++
src/vbox/vbox_storage.c   |  20 +++---
src/vbox/vbox_tmpl.c  | 146 ++---
src/vbox/vbox_uniformed_api.h |  54 +++---
5 files changed, 204 insertions(+), 204 deletions(-)

diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index 23f63f9..13869eb 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -206,12 +206,12 @@ static vboxGlobalData *g_pVBoxGlobalData;


So the g_pVBoxGlobalData is still pointer to vboxGlobalData, but ...


@@ -1312,7 +1312,7 @@ _vboxDomainSnapshotRestore(virDomainPtr dom,
  IMachine *machine,
  ISnapshot *snapshot)
{
-vboxGlobalData *data = dom->conn->privateData;
+vboxPrivate *data = dom->conn->privateData;


in all cases you change here, like this...


# if VBOX_API_VERSION < 500
IConsole *console = NULL;
# endif /*VBOX_API_VERSION < 500*/
@@ -1422,7 +1422,7 @@ vboxCallbackOnMachineStateChange(IVirtualBoxCallback 
*pThis ATTRIBUTE_UNUSED,
int event = 0;
int detail = 0;

-vboxDriverLock(g_pVBoxGlobalData);
+vboxDriverLock((vboxPrivate *) g_pVBoxGlobalData);



and this (and others) you *just* cast it to different struct.  That's
not good.  I'm guessing this is still just a differently separated
commit.


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

Re: [libvirt] [OSSTEST PATCH 1/2] libvirt: Check migration capabilities using proper XML parser

[Julien Grall]

Hi Ian,

On 06/10/16 11:00, Ian Jackson wrote:

Julien Grall writes ("Re: [OSSTEST PATCH 1/2] libvirt: Check migration capabilities 
using proper XML parser"):

On 04/10/2016 10:05, Ian Jackson wrote:

Missing _.  I didn't test this again before sending it.  I'd still
like a review from libvirt (and Xen/ARM) folks.


I am not sure what kind of input you would need from Xen/ARM. This patch
set looks very libvirt specific.


It is.  Sorry to spam you with the whole thread.  The only thing I
need from Xen/ARM people is a sanity check that my understanding of
the current and likely future supported features is correct.


live migration is not currently the supported but will be in the future. 
FWIW, there is a patch series on xen-devel to support dead migration 
(first step for live migration) [1].


I hope this answer to your question.

Cheers,

[1] 
https://lists.xenproject.org/archives/html/xen-devel/2015-12/msg01053.html



--
Julien Grall

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

Re: [libvirt] [PATCH 1/4] vbox: add vboxPrivate struct.

[Martin Kletzander]

On Wed, Sep 28, 2016 at 01:41:33PM -0400, Dawid Zamirski wrote:

To be passed as per-connection context info instead of using
vboxGlobalData that it will eventually replace in most cases.
---
src/vbox/vbox_uniformed_api.h | 30 ++
1 file changed, 30 insertions(+)

diff --git a/src/vbox/vbox_uniformed_api.h b/src/vbox/vbox_uniformed_api.h
index 74e9ac0..6ec5245 100644
--- a/src/vbox/vbox_uniformed_api.h
+++ b/src/vbox/vbox_uniformed_api.h
@@ -110,6 +110,36 @@ typedef struct {
PCVBOXXPCOM pFuncs;

/* The next is used for domainEvent */
+/* Async event handling */
+virObjectEventStatePtr domainEvents;
+int fdWatch;
+int volatile vboxCallBackRefCount;
+# if defined(VBOX_API_VERSION) && VBOX_API_VERSION > 2002000 && VBOX_API_VERSION 
< 400
+IVirtualBoxCallback *vboxCallback;
+nsIEventQueue *vboxQueue;
+# else /* VBOX_API_VERSION <= 2002000 || VBOX_API_VERSION >= 400 || 
VBOX_API_VERSION undefined */
+void *vboxCallback;
+void *vboxQueue;
+# endif /* VBOX_API_VERSION <= 2002000 || VBOX_API_VERSION >= 400 || 
VBOX_API_VERSION undefined */
+
+/* pointer back to the connection */
+virConnectPtr conn;
+} vboxPrivate;
+
+typedef struct {
+virMutex lock;
+unsigned long version;
+
+virCapsPtr caps;
+virDomainXMLOptionPtr xmlopt;
+
+IVirtualBox *vboxObj;
+ISession *vboxSession;
+
+/** Our version specific API table pointer. */
+PCVBOXXPCOM pFuncs;
+
+/* The next is used for domainEvent */
# if defined(VBOX_API_VERSION) && VBOX_API_VERSION > 2002000 && VBOX_API_VERSION 
< 400

/* Async event handling */


So after this patch the structure vboxGlobalData is changed, but none of
the code is adjusted to that.  I think that would cause some errors, but
I don't have vbox installed to try that out.  I'm quite sure, though.
We are trying to separate comics in a way that you can compile and use
the code after any commit so that bisecting works and we can clearly
find out which particular commit causes problems.


--
2.7.4

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


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

Re: [libvirt] Recommendations for Libvirt LTS release

[Cole Robinson]

On 10/01/2016 05:15 AM, Guido Günther wrote:
> Hi,
> Debian is about to enter it's freeze for the Stretch release in November
> and we will support the libvirt version that is in Stretch for at least
> 5 years.
> 
> The upstream version of libvirt at this point will likely be 2.4.0.  Are
> any other distros about to pick a version around that time so we can
> possibly join forces? Are there any recommendations to rather pick
> another version due to planned features/fixes/refactorings that haven't
> made it to the list yet?
> 
> As with previous versions I will feed back patches to the -maint branch
> and hope to cut point releases as were currently doing with 0.9.12 but
> if other distros (apart from Debians downstreams) would use the same
> version this would be a plus.

Unfortunately libvirt 2.4.0 came in too late in Fedora 25 cycle, so we will be
sticking with libvirt 2.3.0

- Cole

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

Re: [libvirt] [PATCH v6 3/3] target-i386: Return runnability information on query-cpu-definitions

[Eduardo Habkost]

On Tue, Oct 11, 2016 at 03:21:05PM +0200, Igor Mammedov wrote:
> On Tue, 11 Oct 2016 08:58:02 -0300
> Eduardo Habkost  wrote:
> 
> > On Tue, Oct 11, 2016 at 01:45:21PM +0200, Igor Mammedov wrote:
> > > On Mon, 10 Oct 2016 14:01:10 -0300
> > > Eduardo Habkost  wrote:  
> > > > On Mon, Oct 10, 2016 at 02:27:49PM +0200, Igor Mammedov wrote:  
> > > > > On Fri,  7 Oct 2016 17:29:02 -0300
> > > > > Eduardo Habkost  wrote:  
> > [...]
> > > > > > +static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,
> > > > > > + strList 
> > > > > > **missing_feats)
> > > > > > +{
> > > > > > +X86CPU *xc;
> > > > > > +FeatureWord w;
> > > > > > +Error *err = NULL;
> > > > > > +strList **next = missing_feats;
> > > > > > +
> > > > > > +if (xcc->kvm_required && !kvm_enabled()) {
> > > > > > +strList *new = g_new0(strList, 1);
> > > > > > +new->value = g_strdup("kvm");;
> > > > > > +*missing_feats = new;
> > > > > > +return;
> > > > > > +}
> > > > > > +
> > > > > > +xc = 
> > > > > > X86_CPU(object_new(object_class_get_name(OBJECT_CLASS(xcc;
> > > > > > +
> > > > > > +x86_cpu_load_features(xc, );
> > > > > > +if (err) {
> > > > > > +/* Errors at x86_cpu_load_features should never happen,
> > > > > > + * but in case it does, just report the model as not
> > > > > > + * runnable at all using the "type" property.
> > > > > > + */
> > > > > > +strList *new = g_new0(strList, 1);
> > > > > > +new->value = g_strdup("type");
> > > > > > +*next = new;
> > > > > > +next = >next;
> > > > > > +}
> > > > > > +
> > > > > > +x86_cpu_filter_features(xc);
> > > > > > +
> > > > > > +for (w = 0; w < FEATURE_WORDS; w++) {
> > > > > > +uint32_t filtered = xc->filtered_features[w];
> > > > > > +int i;
> > > > > > +for (i = 0; i < 32; i++) {
> > > > > > +if (filtered & (1UL << i)) {
> > > > > > +strList *new = g_new0(strList, 1);
> > > > > > +new->value = g_strdup(x86_cpu_feature_name(w, i));
> > > > > > +*next = new;
> > > > > > +next = >next;
> > > > > > +}
> > > > > > +}
> > > > > > +}
> > > > > Shouldn't you add 
> > > > >if (IS_AMD_CPU(env)) { 
> > > > > fixup here, that realize does right after calling 
> > > > > x86_cpu_filter_features()
> > > > 
> > > > What would it be useful for? The IS_AMD_CPU fixup runs after
> > > > x86_cpu_filter_features() (so it doesn't affect filtered_features
> > > > at all), and filtered_features is the only field used as input to
> > > > build missing_feats.  
> > > For completeness of features returned by query-cpu-definitions, I'd guess.
> > > So that returned cpu definitions would match actually created cpus.  
> > 
> > For completeness of which query-cpu-definitions field, exactly?
> > There's no field in the return value of query-cpu-definitions
> > that would be affected by the AMD aliases. The AMD aliases don't
> > affect runnability of the CPU model because they aren't included
> > in the GET_SUPPORTED_CPUID check[1].
> > 
> > You would be right if we did return a copy of the low-level CPUID
> > data that's seen by the guest, or if the AMD aliases were set up
> > before x86_cpu_filter_features() (so they could affect
> > filtered_features/unavailable-features), but that's not the case.
> > 
> > [1] They aren't included in the GET_SUPPORTED_CPUID check because
> > the existence of the AMD aliases depend only on the
> > configured guest vendor ID, not on the host CPU.
> > 
> Got it.
> 
> I've tried to build with this patch but build fails with
> 
> make -j32
>   CHK version_gen.h
>   CC  i386-linux-user/target-i386/cpu.o
>   target-i386/cpu.c: In function ‘x86_cpu_definition_entry’:
>   target-i386/cpu.c:2199:51: error: ‘CpuDefinitionInfo’ has no member 
> named ‘unavailable_features’
>  x86_cpu_class_check_missing_features(cc, >unavailable_features);
>^
>   target-i386/cpu.c:2200:9: error: ‘CpuDefinitionInfo’ has no member 
> named ‘has_unavailable_features’
>  info->has_unavailable_features = true;
> 
> Probably series misses a patch that adds it.

See git URLs on cover letter. Series is based on my x86-next branch.

] This series can be seen in the git branch at:
]   https://github.com/ehabkost/qemu-hacks.git 
work/query-cpu-definitions-runnable-info
]
] The series is based on my x86-next branch:
]   https://github.com/ehabkost/qemu.git x86-next

-- 
Eduardo

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

Re: [libvirt] [PATCH 0/3] Make UEFI firmware config simpler

[Daniel P. Berrange]

ping

On Mon, Oct 03, 2016 at 04:49:46PM +0100, Daniel P. Berrange wrote:
> This series lets apps enabled UEFI for a guest by
> simply doing
> 
>   
> 
> with the other (existing) attributes being auto-filled
> with correct QEMU specific defaults.
> 
> Daniel P. Berrange (3):
>   firmware: include arch and features in firmware file list
>   conf: add support for choosing firmware type
>   qemu: add support for simpler UEFI config
> 
>  docs/formatdomain.html.in  |  9 ++-
>  docs/schemas/domaincommon.rng  | 12 ++-
>  src/conf/domain_conf.c | 70 ++--
>  src/conf/domain_conf.h | 11 +++
>  src/libvirt_private.syms   |  1 +
>  src/qemu/qemu.conf | 14 +++-
>  src/qemu/qemu_command.c|  6 +-
>  src/qemu/qemu_conf.c   | 12 ++-
>  src/qemu/qemu_conf.h   |  7 ++
>  src/qemu/qemu_domain.c | 60 --
>  src/qemu/test_libvirtd_qemu.aug.in |  6 +-
>  src/util/virfirmware.c | 94 
> +++---
>  src/util/virfirmware.h |  7 ++
>  .../qemuxml2argv-bios-firmware.args| 26 ++
>  .../qemuxml2argv-bios-firmware.xml | 41 ++
>  tests/qemuxml2argvtest.c   |  1 +
>  .../qemuxml2xmlout-bios-firmware.xml   | 48 +++
>  tests/qemuxml2xmltest.c|  1 +
>  tests/testutilsqemu.c  | 30 ++-
>  19 files changed, 412 insertions(+), 44 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-bios-firmware.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-bios-firmware.xml
>  create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-bios-firmware.xml
> 
> -- 
> 2.7.4
> 

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

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

Re: [libvirt] [PATCH v6 3/3] target-i386: Return runnability information on query-cpu-definitions

[Igor Mammedov]

On Tue, 11 Oct 2016 08:58:02 -0300
Eduardo Habkost  wrote:

> On Tue, Oct 11, 2016 at 01:45:21PM +0200, Igor Mammedov wrote:
> > On Mon, 10 Oct 2016 14:01:10 -0300
> > Eduardo Habkost  wrote:  
> > > On Mon, Oct 10, 2016 at 02:27:49PM +0200, Igor Mammedov wrote:  
> > > > On Fri,  7 Oct 2016 17:29:02 -0300
> > > > Eduardo Habkost  wrote:  
> [...]
> > > > > +static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,
> > > > > + strList 
> > > > > **missing_feats)
> > > > > +{
> > > > > +X86CPU *xc;
> > > > > +FeatureWord w;
> > > > > +Error *err = NULL;
> > > > > +strList **next = missing_feats;
> > > > > +
> > > > > +if (xcc->kvm_required && !kvm_enabled()) {
> > > > > +strList *new = g_new0(strList, 1);
> > > > > +new->value = g_strdup("kvm");;
> > > > > +*missing_feats = new;
> > > > > +return;
> > > > > +}
> > > > > +
> > > > > +xc = 
> > > > > X86_CPU(object_new(object_class_get_name(OBJECT_CLASS(xcc;
> > > > > +
> > > > > +x86_cpu_load_features(xc, );
> > > > > +if (err) {
> > > > > +/* Errors at x86_cpu_load_features should never happen,
> > > > > + * but in case it does, just report the model as not
> > > > > + * runnable at all using the "type" property.
> > > > > + */
> > > > > +strList *new = g_new0(strList, 1);
> > > > > +new->value = g_strdup("type");
> > > > > +*next = new;
> > > > > +next = >next;
> > > > > +}
> > > > > +
> > > > > +x86_cpu_filter_features(xc);
> > > > > +
> > > > > +for (w = 0; w < FEATURE_WORDS; w++) {
> > > > > +uint32_t filtered = xc->filtered_features[w];
> > > > > +int i;
> > > > > +for (i = 0; i < 32; i++) {
> > > > > +if (filtered & (1UL << i)) {
> > > > > +strList *new = g_new0(strList, 1);
> > > > > +new->value = g_strdup(x86_cpu_feature_name(w, i));
> > > > > +*next = new;
> > > > > +next = >next;
> > > > > +}
> > > > > +}
> > > > > +}
> > > > Shouldn't you add 
> > > >if (IS_AMD_CPU(env)) { 
> > > > fixup here, that realize does right after calling 
> > > > x86_cpu_filter_features()
> > > 
> > > What would it be useful for? The IS_AMD_CPU fixup runs after
> > > x86_cpu_filter_features() (so it doesn't affect filtered_features
> > > at all), and filtered_features is the only field used as input to
> > > build missing_feats.  
> > For completeness of features returned by query-cpu-definitions, I'd guess.
> > So that returned cpu definitions would match actually created cpus.  
> 
> For completeness of which query-cpu-definitions field, exactly?
> There's no field in the return value of query-cpu-definitions
> that would be affected by the AMD aliases. The AMD aliases don't
> affect runnability of the CPU model because they aren't included
> in the GET_SUPPORTED_CPUID check[1].
> 
> You would be right if we did return a copy of the low-level CPUID
> data that's seen by the guest, or if the AMD aliases were set up
> before x86_cpu_filter_features() (so they could affect
> filtered_features/unavailable-features), but that's not the case.
> 
> [1] They aren't included in the GET_SUPPORTED_CPUID check because
> the existence of the AMD aliases depend only on the
> configured guest vendor ID, not on the host CPU.
> 
Got it.

I've tried to build with this patch but build fails with

make -j32
CHK version_gen.h
  CC  i386-linux-user/target-i386/cpu.o
  target-i386/cpu.c: In function ‘x86_cpu_definition_entry’:
  target-i386/cpu.c:2199:51: error: ‘CpuDefinitionInfo’ has no member 
named ‘unavailable_features’
 x86_cpu_class_check_missing_features(cc, >unavailable_features);
   ^
  target-i386/cpu.c:2200:9: error: ‘CpuDefinitionInfo’ has no member 
named ‘has_unavailable_features’
 info->has_unavailable_features = true;

Probably series misses a patch that adds it.

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

Re: [libvirt] [PATCH] libvirt-guests: Weaken dependency on libvirtd

[Andrea Bolognani]

On Mon, 2016-10-10 at 15:59 +0200, Martin Kletzander wrote:
> On Fri, Oct 07, 2016 at 10:19:53AM +0200, Andrea Bolognani wrote:
> > 
> > The Requires relationship is very strong, in that it prevents
> > a unit from running unless all the units it Requires are
> > running as well.
> > 
> > This turns out to be a problem because we want to be able to
> > restart libvirtd at any time without having libvirt-guests
> > suspend or shutdown running guests.
> > 
> > Turn the Requires relationship into a Wants relationship:
> > this way starting libvirt-guests will cause systemd to (attempt
> > to) start libvirtd as well, but stopping or restarting libvirtd
> > will not alter libvirt-guests' running state.
> 
> I can't figure out how exactly this works, even when looking at the
> systemd.unit documentation.  You are saying that Wants= means that if
> you issue 'service libvirtd stop'

You mean

  systemctl stop libvirtd

Unless distributions have converted service(8) into a wrapper
for systemctl(1)... Oh, it looks like at least Fedora and
Debian have. Makes sense :)

> it will not save your guests after
> this patch.

Correct. If B Requires=A and A becomes unavailable, B will be
stopped as well; the same doesn't happen if B merely Wants=A.

> What if you stop the service and then shutdown the machine?
> I would agree that it's your fault if it doesn't save your guests.

It would be super neat if we could stop libvirt-guests when
stopping libvirtd, so that guests are saved properly, but not
stop it when libvirtd is merely being restarted.

Unfortunately systemd doesn't allow that level of granularity,
so we're basically forced to choose between two partially
unsatisfying solutions :(

> From
> what I am reading, it makes sense, so ACK from me.  Basically because I
> can't come up with scenarios that could be broken by this change =)

I will wait a while before pushing it, so other people can
have a chance to weigh in.

In the meantime, thanks for the review! :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 0/4] systemd-related fixes and improvements

[Boris Fiuczynski]

On 10/07/2016 05:04 PM, Andrea Bolognani wrote:

On Fri, 2016-10-07 at 11:42 +0200, Andrea Bolognani wrote:

On Fri, 2016-10-07 at 11:19 +0200, Boris Fiuczynski wrote:

Andrea,
there is another "side effect" of the Requires directive.
libvirt-guests gets automatically started when libvirt is updated to
v2.3.0. This has some rather nasty implications for the end users.


Mh, I don't see why that would happen.

The Requires relationship goes in the opposite direction,
so if libvirt-guests was not running before the upgrade
there should be no reason for it to be started, whether
that relationship is there or not.

I'll try to reproduce this on my machine and get back to
you in a while.


I tried upgrading from 2.2.0 to 2.3.0 a bunch of times, but
I haven't been able to reproduce the failure you're
reporting: libvirt-guests is never started automatically.

Can you provide more information? What distribution are
you using? Are you building from source, or using your
distribution's packages? Are you sure libvirt-guests was
not running even before upgrade?

--
Andrea Bolognani / Red Hat / Virtualization

I used fc20 and built the libvirt rpms from source. libvirt-guests was 
not running when updating from 2.2.0 to libvirt 2.3.0. I switched 
systems and retried on fc23 after rebuilding libvirt 2.2.0 and 2.3.0 
from source. On this system I cannot reproduce the behaviour.
You are correct that the behaviour I got does not match with the 
requires directive. So my guess is that my old fc20 might still have 
some systemd problem which has been fixed in newer fc versions.


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

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

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

[libvirt] [PATCH 5/5] Clean timer in virObjectEventStateFlush

[Martin Kletzander]

If the last event callback is unregistered while the event loop is
dispatching, it is only marked as deleted, but not removed.  The number
of callbacks is more than zero in that case, so the timer is not
removed.  Because it can be removed in this function now (but also
accessed afterwards so that we set 'isDispatching = false' and have it
locked), we need to temporarily increase the reference counter of the
state for the duration of this function.

Signed-off-by: Martin Kletzander 
---
 src/conf/object_event.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/src/conf/object_event.c b/src/conf/object_event.c
index b71960f3399d..1bde434a3af3 100644
--- a/src/conf/object_event.c
+++ b/src/conf/object_event.c
@@ -807,6 +807,9 @@ virObjectEventStateFlush(virObjectEventStatePtr state)
 {
 virObjectEventQueue tempQueue;

+/* We need to lock as well as ref due to the fact that we might
+ * unref the state we're working on in this very function */
+virObjectRef(state);
 virObjectLock(state);
 state->isDispatching = true;

@@ -826,8 +829,13 @@ virObjectEventStateFlush(virObjectEventStatePtr state)
 /* Purge any deleted callbacks */
 virObjectEventCallbackListPurgeMarked(state->callbacks);

+/* If we purged all callbacks, we need to remove the timeout as
+ * well like virObjectEventStateDeregisterID() would do. */
+virObjectEventStateCleanupTimer(state, true);
+
 state->isDispatching = false;
 virObjectUnlock(state);
+virObjectUnref(state);
 }


--
2.10.1

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

[libvirt] [PATCH 4/5] Add virObjectEventStateRegisterID to symsfile

[Martin Kletzander]

To go with the Deregister version as well.

Signed-off-by: Martin Kletzander 
---
 src/libvirt_private.syms | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d92d3d865307..fd63f99b568c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -820,6 +820,7 @@ virObjectEventStateDeregisterID;
 virObjectEventStateEventID;
 virObjectEventStateNew;
 virObjectEventStateQueue;
+virObjectEventStateRegisterID;


 # conf/secret_conf.h
--
2.10.1

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

[libvirt] [PATCH 2/5] Reference state when using it as opaque

[Martin Kletzander]

There should be one more reference because it is being kept in the list
of callbacks as an opaque.  We also unref it properly using
virObjectFreeCallback.

Signed-off-by: Martin Kletzander 
---
 src/conf/object_event.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/conf/object_event.c b/src/conf/object_event.c
index b859835b47a1..5994c2574d6f 100644
--- a/src/conf/object_event.c
+++ b/src/conf/object_event.c
@@ -870,12 +870,14 @@ virObjectEventStateRegisterID(virConnectPtr conn,
 (state->timer = virEventAddTimeout(-1,
virObjectEventTimer,
state,
-   NULL)) < 0) {
+   virObjectFreeCallback)) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("could not initialize domain event timer"));
 goto cleanup;
 }

+virObjectRef(state);
+
 ret = virObjectEventCallbackListAddID(conn, state->callbacks,
   key, filter, filter_opaque,
   klass, eventID,
--
2.10.1

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

[libvirt] [PATCH 1/5] Change virDomainEventState to virObjectLockable

[Martin Kletzander]

This way we get reference counting and we can get rid of locking
function.

Signed-off-by: Martin Kletzander 
---
 src/bhyve/bhyve_driver.c   |  2 +-
 src/conf/object_event.c| 99 +++---
 src/conf/object_event.h|  1 -
 src/libvirt_private.syms   |  1 -
 src/libxl/libxl_driver.c   |  2 +-
 src/lxc/lxc_driver.c   |  2 +-
 src/network/bridge_driver.c|  2 +-
 src/node_device/node_device_udev.c |  2 +-
 src/qemu/qemu_driver.c |  2 +-
 src/remote/remote_driver.c |  2 +-
 src/storage/storage_driver.c   |  2 +-
 src/test/test_driver.c |  2 +-
 src/uml/uml_driver.c   |  2 +-
 src/vbox/vbox_common.c |  2 +-
 src/vz/vz_driver.c |  2 +-
 src/xen/xen_driver.c   |  2 +-
 16 files changed, 52 insertions(+), 75 deletions(-)

diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index 49b9e1a60626..04be78b675f7 100644
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -1198,7 +1198,7 @@ bhyveStateCleanup(void)
 virObjectUnref(bhyve_driver->xmlopt);
 virSysinfoDefFree(bhyve_driver->hostsysinfo);
 virObjectUnref(bhyve_driver->closeCallbacks);
-virObjectEventStateFree(bhyve_driver->domainEventState);
+virObjectUnref(bhyve_driver->domainEventState);

 virMutexDestroy(_driver->lock);
 VIR_FREE(bhyve_driver);
diff --git a/src/conf/object_event.c b/src/conf/object_event.c
index e5af4be68a7e..b859835b47a1 100644
--- a/src/conf/object_event.c
+++ b/src/conf/object_event.c
@@ -32,6 +32,7 @@
 #include "datatypes.h"
 #include "viralloc.h"
 #include "virerror.h"
+#include "virobject.h"
 #include "virstring.h"

 #define VIR_FROM_THIS VIR_FROM_NONE
@@ -71,6 +72,7 @@ typedef struct _virObjectEventQueue virObjectEventQueue;
 typedef virObjectEventQueue *virObjectEventQueuePtr;

 struct _virObjectEventState {
+virObjectLockable parent;
 /* The list of domain event callbacks */
 virObjectEventCallbackListPtr callbacks;
 /* The queue of object events */
@@ -79,22 +81,31 @@ struct _virObjectEventState {
 int timer;
 /* Flag if we're in process of dispatching */
 bool isDispatching;
-virMutex lock;
 };

 static virClassPtr virObjectEventClass;
+static virClassPtr virObjectEventStateClass;

 static void virObjectEventDispose(void *obj);
+static void virObjectEventStateDispose(void *obj);

 static int
 virObjectEventOnceInit(void)
 {
+if (!(virObjectEventStateClass =
+  virClassNew(virClassForObjectLockable(),
+  "virObjectEventState",
+  sizeof(virObjectEventState),
+  virObjectEventStateDispose)))
+return -1;
+
 if (!(virObjectEventClass =
   virClassNew(virClassForObject(),
   "virObjectEvent",
   sizeof(virObjectEvent),
   virObjectEventDispose)))
 return -1;
+
 return 0;
 }

@@ -504,51 +515,23 @@ virObjectEventQueueNew(void)


 /**
- * virObjectEventStateLock:
- * @state: the event state object
- *
- * Lock event state before calling functions from object_event_private.h.
- */
-static void
-virObjectEventStateLock(virObjectEventStatePtr state)
-{
-virMutexLock(>lock);
-}
-
-
-/**
- * virObjectEventStateUnlock:
- * @state: the event state object
- *
- * Unlock event state after calling functions from object_event_private.h.
- */
-static void
-virObjectEventStateUnlock(virObjectEventStatePtr state)
-{
-virMutexUnlock(>lock);
-}
-
-
-/**
- * virObjectEventStateFree:
+ * virObjectEventStateDispose:
  * @list: virObjectEventStatePtr to free
  *
  * Free a virObjectEventStatePtr and its members, and unregister the timer.
  */
-void
-virObjectEventStateFree(virObjectEventStatePtr state)
+static void
+virObjectEventStateDispose(void *obj)
 {
-if (!state)
-return;
+virObjectEventStatePtr state = obj;
+
+VIR_DEBUG("obj=%p", state);

 virObjectEventCallbackListFree(state->callbacks);
 virObjectEventQueueFree(state->queue);

 if (state->timer != -1)
 virEventRemoveTimeout(state->timer);
-
-virMutexDestroy(>lock);
-VIR_FREE(state);
 }


@@ -583,15 +566,11 @@ virObjectEventStateNew(void)
 {
 virObjectEventStatePtr state = NULL;

-if (VIR_ALLOC(state) < 0)
-goto error;
+if (virObjectEventInitialize() < 0)
+return NULL;

-if (virMutexInit(>lock) < 0) {
-virReportSystemError(errno, "%s",
- _("unable to initialize state mutex"));
-VIR_FREE(state);
-goto error;
-}
+if (!(state = virObjectLockableNew(virObjectEventStateClass)))
+return NULL;

 if (VIR_ALLOC(state->callbacks) < 0)
 goto error;
@@ -604,7 +583,7 @@ virObjectEventStateNew(void)
 return state;

  error:
-virObjectEventStateFree(state);
+virObjectUnref(state);
 return 

[libvirt] [PATCH 3/5] De-duplicate code into virObjectEventStateCleanupTimer()

[Martin Kletzander]

There is a repeating pattern of code that removes the timer if it's not
needed.  So let's move it to a new function.  We'll also use it later.

Signed-off-by: Martin Kletzander 
---
 src/conf/object_event.c | 34 ++
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/src/conf/object_event.c b/src/conf/object_event.c
index 5994c2574d6f..b71960f3399d 100644
--- a/src/conf/object_event.c
+++ b/src/conf/object_event.c
@@ -784,6 +784,25 @@ virObjectEventStateQueue(virObjectEventStatePtr state,


 static void
+virObjectEventStateCleanupTimer(virObjectEventStatePtr state, bool clear_queue)
+{
+/* There are still some callbacks, keep the timer. */
+if (state->callbacks->count)
+return;
+
+/* The timer is not registered, nothing to do. */
+if (state->timer == -1)
+return;
+
+virEventRemoveTimeout(state->timer);
+state->timer = -1;
+
+if (clear_queue)
+virObjectEventQueueClear(state->queue);
+}
+
+
+static void
 virObjectEventStateFlush(virObjectEventStatePtr state)
 {
 virObjectEventQueue tempQueue;
@@ -884,12 +903,8 @@ virObjectEventStateRegisterID(virConnectPtr conn,
   cb, opaque, freecb,
   legacy, callbackID, serverFilter);

-if (ret == -1 &&
-state->callbacks->count == 0 &&
-state->timer != -1) {
-virEventRemoveTimeout(state->timer);
-state->timer = -1;
-}
+if (ret < 0)
+virObjectEventStateCleanupTimer(state, false);

  cleanup:
 virObjectUnlock(state);
@@ -924,12 +939,7 @@ virObjectEventStateDeregisterID(virConnectPtr conn,
 ret = virObjectEventCallbackListRemoveID(conn,
  state->callbacks, callbackID);

-if (state->callbacks->count == 0 &&
-state->timer != -1) {
-virEventRemoveTimeout(state->timer);
-state->timer = -1;
-virObjectEventQueueClear(state->queue);
-}
+virObjectEventStateCleanupTimer(state, true);

 virObjectUnlock(state);
 return ret;
--
2.10.1

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

[libvirt] [PATCH 0/5] Various virObjectEventState fixes

[Martin Kletzander]

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


Martin Kletzander (5):
  Change virDomainEventState to virObjectLockable
  Reference state when using it as opaque
  De-duplicate code into virObjectEventStateCleanupTimer()
  Add virObjectEventStateRegisterID to symsfile
  Clean timer in virObjectEventStateFlush

 src/bhyve/bhyve_driver.c   |   2 +-
 src/conf/object_event.c| 145 ++---
 src/conf/object_event.h|   1 -
 src/libvirt_private.syms   |   2 +-
 src/libxl/libxl_driver.c   |   2 +-
 src/lxc/lxc_driver.c   |   2 +-
 src/network/bridge_driver.c|   2 +-
 src/node_device/node_device_udev.c |   2 +-
 src/qemu/qemu_driver.c |   2 +-
 src/remote/remote_driver.c |   2 +-
 src/storage/storage_driver.c   |   2 +-
 src/test/test_driver.c |   2 +-
 src/uml/uml_driver.c   |   2 +-
 src/vbox/vbox_common.c |   2 +-
 src/vz/vz_driver.c |   2 +-
 src/xen/xen_driver.c   |   2 +-
 16 files changed, 86 insertions(+), 88 deletions(-)

--
2.10.1

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

Re: [libvirt] [PATCH] audit: add descriptions about audit log

[Peter Krempa]

On Tue, Oct 11, 2016 at 19:43:19 +0800, Chen Hanxiao wrote:
> From: Chen Hanxiao 
> 
> This patch add some descriptions on what libvirt
> audit will record in introduction section.
> 
> Signed-off-by: Chen Hanxiao 
> ---
>  docs/auditlog.html.in | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/auditlog.html.in b/docs/auditlog.html.in
> index 45464af..d1d0f6e 100644
> --- a/docs/auditlog.html.in
> +++ b/docs/auditlog.html.in
> @@ -14,7 +14,9 @@
>subsystem. This provides administrators / auditors with a canonical 
> historical
>record of changes to virtual machines' / containers' lifecycle states 
> and
>their configuration. On hosts which are running the Linux audit daemon,
> -  the logs will usually end up in /var/log/audit/audit.log
> +  the logs will usually end up in /var/log/audit/audit.log.
> +  If QEMU try to use some resources on the host, libvirt will record them
> +  in audit log.
>  

This is documented as the VIRT_RESOURCE section. Adding it here doesn't
make much sense.

NACK



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

Re: [libvirt] [PATCH v6 3/3] target-i386: Return runnability information on query-cpu-definitions

[Eduardo Habkost]

On Tue, Oct 11, 2016 at 01:45:21PM +0200, Igor Mammedov wrote:
> On Mon, 10 Oct 2016 14:01:10 -0300
> Eduardo Habkost  wrote:
> > On Mon, Oct 10, 2016 at 02:27:49PM +0200, Igor Mammedov wrote:
> > > On Fri,  7 Oct 2016 17:29:02 -0300
> > > Eduardo Habkost  wrote:
[...]
> > > > +static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,
> > > > + strList 
> > > > **missing_feats)
> > > > +{
> > > > +X86CPU *xc;
> > > > +FeatureWord w;
> > > > +Error *err = NULL;
> > > > +strList **next = missing_feats;
> > > > +
> > > > +if (xcc->kvm_required && !kvm_enabled()) {
> > > > +strList *new = g_new0(strList, 1);
> > > > +new->value = g_strdup("kvm");;
> > > > +*missing_feats = new;
> > > > +return;
> > > > +}
> > > > +
> > > > +xc = X86_CPU(object_new(object_class_get_name(OBJECT_CLASS(xcc;
> > > > +
> > > > +x86_cpu_load_features(xc, );
> > > > +if (err) {
> > > > +/* Errors at x86_cpu_load_features should never happen,
> > > > + * but in case it does, just report the model as not
> > > > + * runnable at all using the "type" property.
> > > > + */
> > > > +strList *new = g_new0(strList, 1);
> > > > +new->value = g_strdup("type");
> > > > +*next = new;
> > > > +next = >next;
> > > > +}
> > > > +
> > > > +x86_cpu_filter_features(xc);
> > > > +
> > > > +for (w = 0; w < FEATURE_WORDS; w++) {
> > > > +uint32_t filtered = xc->filtered_features[w];
> > > > +int i;
> > > > +for (i = 0; i < 32; i++) {
> > > > +if (filtered & (1UL << i)) {
> > > > +strList *new = g_new0(strList, 1);
> > > > +new->value = g_strdup(x86_cpu_feature_name(w, i));
> > > > +*next = new;
> > > > +next = >next;
> > > > +}
> > > > +}
> > > > +}  
> > > Shouldn't you add 
> > >if (IS_AMD_CPU(env)) { 
> > > fixup here, that realize does right after calling 
> > > x86_cpu_filter_features()  
> > 
> > What would it be useful for? The IS_AMD_CPU fixup runs after
> > x86_cpu_filter_features() (so it doesn't affect filtered_features
> > at all), and filtered_features is the only field used as input to
> > build missing_feats.
> For completeness of features returned by query-cpu-definitions, I'd guess.
> So that returned cpu definitions would match actually created cpus.

For completeness of which query-cpu-definitions field, exactly?
There's no field in the return value of query-cpu-definitions
that would be affected by the AMD aliases. The AMD aliases don't
affect runnability of the CPU model because they aren't included
in the GET_SUPPORTED_CPUID check[1].

You would be right if we did return a copy of the low-level CPUID
data that's seen by the guest, or if the AMD aliases were set up
before x86_cpu_filter_features() (so they could affect
filtered_features/unavailable-features), but that's not the case.

[1] They aren't included in the GET_SUPPORTED_CPUID check because
the existence of the AMD aliases depend only on the
configured guest vendor ID, not on the host CPU.

-- 
Eduardo

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

Re: [libvirt] [PATCH v6 3/3] target-i386: Return runnability information on query-cpu-definitions

[Igor Mammedov]

On Mon, 10 Oct 2016 14:01:10 -0300
Eduardo Habkost  wrote:

> On Mon, Oct 10, 2016 at 02:27:49PM +0200, Igor Mammedov wrote:
> > On Fri,  7 Oct 2016 17:29:02 -0300
> > Eduardo Habkost  wrote:
> >   
> > > Fill the "unavailable-features" field on the x86 implementation
> > > of query-cpu-definitions.
> > > 
> > > Cc: Jiri Denemark 
> > > Cc: libvir-list@redhat.com
> > > Signed-off-by: Eduardo Habkost 
> > > ---
> > > Changes v5 -> v6:
> > > * Call x86_cpu_filter_features(), now that x86_cpu_load_features()
> > >   won't run it automatically
> > > 
> > > Changes v4 -> v5:
> > > * (none)
> > > 
> > > Changes v3 -> v4:
> > > * Handle missing XSAVE components cleanly, but looking up
> > >   the original feature that required it
> > > * Use x86_cpu_load_features() function
> > > 
> > > Changes v2 -> v3:
> > > * Create a x86_cpu_feature_name() function, to
> > >   isolate the code that returns the property name
> > > 
> > > Changes v1 -> v2:
> > > * Updated to the new schema: no @runnable field, and
> > >   always report @unavailable-features as present
> > > ---
> > >  target-i386/cpu.c | 76 
> > > +++
> > >  1 file changed, 76 insertions(+)
> > > 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index 23cc19b..63330ce 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -1945,6 +1945,27 @@ static inline void feat2prop(char *s)
> > >  }
> > >  }
> > >  
> > > +/* Return the feature property name for a feature flag bit */
> > > +static const char *x86_cpu_feature_name(FeatureWord w, int bitnr)
> > > +{
> > > +/* XSAVE components are automatically enabled by other features,
> > > + * so return the original feature name instead
> > > + */
> > > +if (w == FEAT_XSAVE_COMP_LO || w == FEAT_XSAVE_COMP_HI) {
> > > +int comp = (w == FEAT_XSAVE_COMP_HI) ? bitnr + 32 : bitnr;
> > > +
> > > +if (comp < ARRAY_SIZE(x86_ext_save_areas) &&
> > > +x86_ext_save_areas[comp].bits) {
> > > +w = x86_ext_save_areas[comp].feature;
> > > +bitnr = ctz32(x86_ext_save_areas[comp].bits);
> > > +}
> > > +}
> > > +
> > > +assert(bitnr < 32);
> > > +assert(w < FEATURE_WORDS);
> > > +return feature_word_info[w].feat_names[bitnr];
> > > +}
> > > +
> > >  /* Compatibily hack to maintain legacy +-feat semantic,
> > >   * where +-feat overwrites any feature set by
> > >   * feat=on|feat even if the later is parsed after +-feat
> > > @@ -2030,6 +2051,59 @@ static void x86_cpu_parse_featurestr(const char 
> > > *typename, char *features,
> > >  }
> > >  }
> > >  
> > > +static void x86_cpu_load_features(X86CPU *cpu, Error **errp);
> > > +static int x86_cpu_filter_features(X86CPU *cpu);
> > > +
> > > +/* Check for missing features that may prevent the CPU class from
> > > + * running using the current machine and accelerator.
> > > + */
> > > +static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,
> > > + strList **missing_feats)
> > > +{
> > > +X86CPU *xc;
> > > +FeatureWord w;
> > > +Error *err = NULL;
> > > +strList **next = missing_feats;
> > > +
> > > +if (xcc->kvm_required && !kvm_enabled()) {
> > > +strList *new = g_new0(strList, 1);
> > > +new->value = g_strdup("kvm");;
> > > +*missing_feats = new;
> > > +return;
> > > +}
> > > +
> > > +xc = X86_CPU(object_new(object_class_get_name(OBJECT_CLASS(xcc;
> > > +
> > > +x86_cpu_load_features(xc, );
> > > +if (err) {
> > > +/* Errors at x86_cpu_load_features should never happen,
> > > + * but in case it does, just report the model as not
> > > + * runnable at all using the "type" property.
> > > + */
> > > +strList *new = g_new0(strList, 1);
> > > +new->value = g_strdup("type");
> > > +*next = new;
> > > +next = >next;
> > > +}
> > > +
> > > +x86_cpu_filter_features(xc);
> > > +
> > > +for (w = 0; w < FEATURE_WORDS; w++) {
> > > +uint32_t filtered = xc->filtered_features[w];
> > > +int i;
> > > +for (i = 0; i < 32; i++) {
> > > +if (filtered & (1UL << i)) {
> > > +strList *new = g_new0(strList, 1);
> > > +new->value = g_strdup(x86_cpu_feature_name(w, i));
> > > +*next = new;
> > > +next = >next;
> > > +}
> > > +}
> > > +}  
> > Shouldn't you add 
> >if (IS_AMD_CPU(env)) { 
> > fixup here, that realize does right after calling x86_cpu_filter_features() 
> >  
> 
> What would it be useful for? The IS_AMD_CPU fixup runs after
> x86_cpu_filter_features() (so it doesn't affect filtered_features
> at all), and filtered_features is the only field used as input to
> build missing_feats.
For completeness of 

[libvirt] [PATCH] audit: add descriptions about audit log

[Chen Hanxiao]

From: Chen Hanxiao 

This patch add some descriptions on what libvirt
audit will record in introduction section.

Signed-off-by: Chen Hanxiao 
---
 docs/auditlog.html.in | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/docs/auditlog.html.in b/docs/auditlog.html.in
index 45464af..d1d0f6e 100644
--- a/docs/auditlog.html.in
+++ b/docs/auditlog.html.in
@@ -14,7 +14,9 @@
   subsystem. This provides administrators / auditors with a canonical 
historical
   record of changes to virtual machines' / containers' lifecycle states and
   their configuration. On hosts which are running the Linux audit daemon,
-  the logs will usually end up in /var/log/audit/audit.log
+  the logs will usually end up in /var/log/audit/audit.log.
+  If QEMU try to use some resources on the host, libvirt will record them
+  in audit log.
 
 
 Configuration
-- 
1.8.3.1


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

Re: [libvirt] [Qemu-devel] [PATCH] fixup! target-i386: x86_cpu_load_features() function

[Igor Mammedov]

On Mon, 10 Oct 2016 13:58:25 -0300
Eduardo Habkost  wrote:

> On Mon, Oct 10, 2016 at 02:25:32PM +0200, Igor Mammedov wrote:
> > On Fri,  7 Oct 2016 17:29:01 -0300
> > Eduardo Habkost  wrote:
> >   
> > > When probing for CPU model information, we need to reuse the code
> > > that initializes CPUID fields, but not the remaining side-effects
> > > of x86_cpu_realizefn(). Move that code to a separate function
> > > that can be reused later.
> > > 
> > > Signed-off-by: Eduardo Habkost 
> > > ---
> > > Changes v5 -> v6:
> > > * Move x86_cpu_filter_features() outside x86_cpu_load_features(),
> > >   as the CPU model querying API won't run
> > >   x86_cpu_filter_features() on most cases  
> >   
> > > 
> > > Changes v4 -> v5:
> > > * Fix typo on x86_cpu_load_features() comment
> > >   Reported-by: Paolo Bonzini 
> > > 
> > > Changes series v3 -> v4:
> > > * New patch added to series
> > > ---
> > >  target-i386/cpu.c | 67 
> > > +++
> > >  1 file changed, 43 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index 1e8127b..23cc19b 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -2993,34 +2993,13 @@ static void 
> > > x86_cpu_enable_xsave_components(X86CPU *cpu)
> > >  env->features[FEAT_XSAVE_COMP_HI] = mask >> 32;
> > >  }
> > >  
> > > -#define IS_INTEL_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 
> > > && \
> > > -   (env)->cpuid_vendor2 == CPUID_VENDOR_INTEL_2 
> > > && \
> > > -   (env)->cpuid_vendor3 == CPUID_VENDOR_INTEL_3)
> > > -#define IS_AMD_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && \
> > > - (env)->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && \
> > > - (env)->cpuid_vendor3 == CPUID_VENDOR_AMD_3)
> > > -static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> > > +/* Load CPUID data based on configured features */
> > > +static void x86_cpu_load_features(X86CPU *cpu, Error **errp)
> > >  {
> > > -CPUState *cs = CPU(dev);
> > > -X86CPU *cpu = X86_CPU(dev);
> > > -X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
> > >  CPUX86State *env = >env;
> > > -Error *local_err = NULL;
> > > -static bool ht_warned;
> > >  FeatureWord w;
> > >  GList *l;
> > > -
> > > -if (xcc->kvm_required && !kvm_enabled()) {
> > > -char *name = x86_cpu_class_get_model_name(xcc);
> > > -error_setg(_err, "CPU model '%s' requires KVM", name);
> > > -g_free(name);
> > > -goto out;
> > > -}
> > > -
> > > -if (cpu->apic_id == UNASSIGNED_APIC_ID) {
> > > -error_setg(errp, "apic-id property was not initialized 
> > > properly");
> > > -return;
> > > -}
> > > +Error *local_err = NULL;
> > >  
> > >  /*TODO: cpu->host_features incorrectly overwrites features
> > >   * set using "feat=on|off". Once we fix this, we can convert
> > > @@ -3086,6 +3065,46 @@ static void x86_cpu_realizefn(DeviceState *dev, 
> > > Error **errp)
> > >  env->cpuid_xlevel2 = env->cpuid_min_xlevel2;
> > >  }
> > >  
> > > +out:
> > > +if (local_err != NULL) {
> > > +error_propagate(errp, local_err);
> > > +}
> > > +}
> > > +
> > > +#define IS_INTEL_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 
> > > && \
> > > +   (env)->cpuid_vendor2 == CPUID_VENDOR_INTEL_2 
> > > && \
> > > +   (env)->cpuid_vendor3 == CPUID_VENDOR_INTEL_3)
> > > +#define IS_AMD_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && \
> > > + (env)->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && \
> > > + (env)->cpuid_vendor3 == CPUID_VENDOR_AMD_3)
> > > +static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> > > +{
> > > +CPUState *cs = CPU(dev);
> > > +X86CPU *cpu = X86_CPU(dev);
> > > +X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
> > > +CPUX86State *env = >env;
> > > +Error *local_err = NULL;
> > > +static bool ht_warned;
> > > +
> > > +if (xcc->kvm_required && !kvm_enabled()) {
> > > +char *name = x86_cpu_class_get_model_name(xcc);
> > > +error_setg(_err, "CPU model '%s' requires KVM", name);
> > > +g_free(name);
> > > +goto out;
> > > +}
> > > +
> > > +if (cpu->apic_id == UNASSIGNED_APIC_ID) {
> > > +error_setg(errp, "apic-id property was not initialized 
> > > properly");
> > > +return;
> > > +}
> > > +
> > > +x86_cpu_load_features(cpu, _err);
> > > +if (local_err) {
> > > +goto out;
> > > +}
> > > +
> > > +x86_cpu_filter_features(cpu);  
> > that makes 2 invocations of ^^ inside realize,
> > see followup line   
> 
> Oops, leftover from v5. Thanks for spotting that! Fixup below.
> 
> Signed-off-by: Eduardo 

Re: [libvirt] [PATCH] sheepdog: allow snapshot

[Vasiliy Tolstov]

2016-09-29 16:00 GMT+03:00 Vasiliy Tolstov :
> partially revert f7c1410b0ee5b878e81f2eddf86c609947a9b27c because
> sheepdog allow to store vm state inside vdi
>

Sorry, can somebody check this?

> Signed-off-by: Vasiliy Tolstov 
> ---
>  src/qemu/qemu_driver.c | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index db99c414d458..816514d2d909 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -13887,6 +13887,12 @@ qemuDomainSnapshotPrepare(virConnectPtr conn,
>active) < 0)
>  goto cleanup;
>
> +/* sheepdog allow to store memory inside the vdi */
> +if (vm->def->disks[i]->src->type == VIR_STORAGE_TYPE_NETWORK &&
> +(vm->def->disks[i]->src->protocol == 
> VIR_STORAGE_NET_PROTOCOL_SHEEPDOG)) {
> +break;
> +}
> +
>  if (vm->def->disks[i]->src->format > 0 &&
>  vm->def->disks[i]->src->format != VIR_STORAGE_FILE_QCOW2) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> --
> 2.7.4
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list



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

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

Re: [libvirt] [PATCH 0/2] Sanitize vcpu topology checking

[Michal Privoznik]

On 10.10.2016 22:01, Peter Krempa wrote:
> See patch 1.
> 
> Peter Krempa (2):
>   conf: Sanitize cpu topology numbers
>   qemu: Reuse virDomainDeGetVcpusTopology to calculate total vcpu count
> 
>  src/conf/domain_conf.c   | 39 +++
>  src/conf/domain_conf.h   |  2 ++
>  src/libvirt_private.syms |  1 +
>  src/qemu/qemu_domain.c   | 18 --
>  src/qemu/qemu_driver.c   | 14 ++
>  5 files changed, 56 insertions(+), 18 deletions(-)
> 

ACK to both.

Michal

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

Re: [libvirt] [PATCH 2/2] Forbid new-line char in name of networks

[Michal Privoznik]

On 11.10.2016 04:15, Sławek Kapłoński wrote:
> New line character in name of network is now forbidden because it
> mess virsh output and can be confusing for users.
> 
> Closes-Bug: https://bugzilla.redhat.com/show_bug.cgi?id=818064
> ---
>  src/conf/network_conf.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index aa39776..a7f802b 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -2117,11 +2117,8 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
>  goto error;
>  }
>  
> -if (strchr(def->name, '/')) {
> -virReportError(VIR_ERR_XML_ERROR,
> -   _("name %s cannot contain '/'"), def->name);
> +if (virXMLCheckString("name", def->name, "/\n"))
>  goto error;
> -}
>  
>  /* Extract network uuid */
>  tmp = virXPathString("string(./uuid[1])", ctxt);
> 

Almost. As Peter pointed out, failure to parse XML (and this is causing
failure) results in libvirt not knowing anything about the object in the
XML. For instance, if somebody already has a network with '\n' in its
name, after this is applied libvirt stops reporting the network and
leaves it behind. With no chance for users to adjust their definition.

What we should do here is to:
1) still deny '/' in network name (as this was never allowed),
2) deny networks with any other forbidden characters (like '\n' for
instance) in network startup process.

Michal

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

Re: [libvirt] [PATCH 0/2] Forbid new-line char in name of networks

[Michal Privoznik]

On 11.10.2016 04:15, Sławek Kapłoński wrote:
> *** BLURB HERE ***
> 
> Sławek Kapłoński (2):
>   util: Add function to check if string contains some chars
>   Forbid new-line char in name of networks
> 
>  src/conf/network_conf.c |  5 +
>  src/util/virxml.c   | 18 ++
>  src/util/virxml.h   |  4 
>  3 files changed, 23 insertions(+), 4 deletions(-)
> 

Usually, we prefix new versions of the patches with 'PATCH v2 ...' for
version 2, 'PATCH v3 ...' for version 3, and so on. And on the top of
that, we describe what are the changes compared to previous version(s).
This is a good example:

https://www.redhat.com/archives/libvir-list/2016-October/msg00306.html

Michal

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

Re: [libvirt] [PATCH 1/2] util: Add function to check if string contains some chars

[Michal Privoznik]

On 11.10.2016 04:15, Sławek Kapłoński wrote:
> This new function can be used to check if e.g. name of XML node don't
> contains forbidden chars like "/" or new-line.
> ---
>  src/util/virxml.c | 18 ++
>  src/util/virxml.h |  4 
>  2 files changed, 22 insertions(+)
> 
> diff --git a/src/util/virxml.c b/src/util/virxml.c
> index 03bd784..450487e 100644
> --- a/src/util/virxml.c
> +++ b/src/util/virxml.c
> @@ -890,6 +890,24 @@ virXMLChildElementCount(xmlNodePtr node)
>  return ret;
>  }
>  
> +/**
> + * virXMLCheckString: checks if string contains at least one of
> + * forbidden characters
> + *
> + * Returns: 0 if string don't contains any of given characters, -1 otherwise
> + */
> +int virXMLCheckString(const char *nodeName, char *str,
> +  const char *forbiddenChars)
> +{
> +char *c;
> +c = strpbrk(str, forbiddenChars);
> +if (c) {
> +virReportError(VIR_ERR_XML_DETAIL,
> +_("invalid char in %s: %c"), nodeName, *c);
> +return -1;
> +}
> +return 0;
> +}
>  
>  /**
>   * virXMLNodeToString: convert an XML node ptr to an XML string
> diff --git a/src/util/virxml.h b/src/util/virxml.h
> index 7a0a1da..676bf5e 100644
> --- a/src/util/virxml.h
> +++ b/src/util/virxml.h
> @@ -75,6 +75,10 @@ char *  virXMLPropString(xmlNodePtr node,
>   const char *name);
>  long virXMLChildElementCount(xmlNodePtr node);
>  
> +intvirXMLCheckString(const char *nodeName,
> + char *str,
> + const char *forbiddenChars);
> +
>  /* Internal function; prefer the macros below.  */
>  xmlDocPtr  virXMLParseHelper(int domcode,
>   const char *filename,
> 

Looking good, however you forgot to add the symbol to
src/libvirt_private.syms. Without it, other parts of the code might not
be able to use this function if linker decides to optimize the symbol out.

Michal

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

Re: [libvirt] [PATCH v3 15/18] qemu: auto-add pcie-root-port/dmi-to-pci-bridge controllers as needed

[Andrea Bolognani]

On Mon, 2016-10-10 at 15:43 -0400, Laine Stump wrote:
> > > 3) Although it is strongly discouraged, it is legal for a pci-bridge
> > >  to be directly plugged into pcie-root, and we don't want to
> > >  auto-add a dmi-to-pci-bridge if there is already a pci-bridge
> > >  that's been forced directly into pcie-root. Finally, although I
> > >  fail to see the utility of it, it is legal to have something like
> > >  this in the xml:
> > >   
> > >
> > >
> > >   
> > >  and that will lead to an automatically added dmi-to-pci-bridge at
> > >  index=1 (to give the unaddressed pci-bridge a proper place to plug
> > >  in):
> > >   
> > >
> > >   
> > >  (for example, see the existing test case
> > >  "usb-controller-default-q35"). This is handled in
> > >  qemuDomainPCIAddressSetCreate() when it's adding in controllers to
> > >  fill holes in the indexes.
> > 
> > I wonder how this "feature" came to be... It seems to be the
> > reason for quite a bit of convoluted code down below, which
> > we could avoid if this had never worked at all. As is so
> > often the case, too late for that :(
> 
> Maybe not. The only place I ever saw that was in the above test case, 
> and another named "usb-controller-explicit-q35", and the author of both 
> of those tests was (wait for it!), no, not Albert Einstein.  Andrea 
> Bolognani!

Oh, *that* guy! It's always *that* guy, isn't it? :P

> The only reason it worked in the past was because we always 
> automatically added the dmi-to-pci-bridge very early in the post-parse 
> processing. This implies that any saved config anywhere will already 
> have the necessary dmi-to-pci-bridge at index='1', so we only need to 
> preserve the behavior for new domain definitions that have a pci-bridge 
> at index='2' but nothing at index='1'.
> 
> Since you're the only person documented to have ever created a config 
> like that, and it would only be problematic if someone tried to create 
> another new config, maybe we should just stop accidentally supporting it 
> and count it as a bug that's being fixed. What's your opinion?

Given the evidence you're presenting, I'm all for getting
rid of it, especially since it will make the code below
much simpler and hence more maintainable.

Considering how critical that part of libvirt is, anything
we can do to make it leaner and meaner is going to be a huge
win in the long run.

> > > @@ -82,6 +82,30 @@ 
> > > virDomainPCIControllerModelToConnectType(virDomainControllerModelPCI 
> > > model)
> > >return 0;
> > >}
> > >
> > > +
> > > +static int
> > s/int/virDomainControllerModelPCI/
> 
> But then we can't return -1 when there isn't a perfect match (that's why 
> I made it int)

Right. Disregard my comments then :)

> > Alternatively you could turn it into
> > 
> >if ((flags & VIR_PCI_CONNECT_PCIE_DEVICE) ||
> >(flags & VIR_PCI_CONNECT_PCIE_SWITCH_UPSTREAM_PORT))
> > 
> > which is more obviously correct and also nicer to look at :)
> 
> but takes two operations instead of one.

I hardly think this would turn out to be a bottleneck, but
feel free to stick to the original implementation - after
fixing it, of course :)

> > > +if (lowestDMIToPCIBridge > idx)
> > > +lowestDMIToPCIBridge = idx;
> > > +} else if (cont->model == 
> > > VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE) {
> > > +if (virDeviceInfoPCIAddressWanted(>info)) {
> > > +if (lowestUnaddressedPCIBridge > idx)
> > > +lowestUnaddressedPCIBridge = idx;
> > > +} else {
> > > +if (lowestAddressedPCIBridge > idx)
> > > +lowestAddressedPCIBridge = idx;
> > > +}
> > >}
> > > +}
> > > +
> > > +if (nbuses > 0 && !addrs->buses[0].model) {
> > > +if (virDomainPCIAddressBusSetModel(>buses[0],
> > > +   
> > > VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) < 0)
> > > +goto error;
> > > +}
> > 
> > Shouldn't we use either PCI_ROOT or PCIE_ROOT based on the
> > machine type here? And set hasPCIeRoot *after* doing so?
> > 
> > Sorry for the questions, I guess this is the point in the
> > patch where I got a bit lost :(

I'm afraid you missed this question :)

> > > +else if (lowestUnaddressedPCIBridge < MIN(lowestAddressedPCIBridge,
> > > +  lowestDMIToPCIBridge))
> > > +defaultModel = VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE;
> > 
> > Again, a bit lost here, sorry :(
> > 
> > Basically if we've found a PCI bridge without address that
> > can't be plugged into any existing PCI bridge or DMI-to-PCI
> > bridge, we want to add a DMI-to-PCI bridge so that we have
> > somewhere to plug it in, right?

And I'm pretty sure I got it right here, but just to be on
the safe side, it would be great if you could confirm or
deny :)

> > > +else
> > 

Re: [libvirt] libvirt-guest.sh bug fixes

[Stefan Bader]

On 10.10.2016 20:43, Eric Blake wrote:
> On 10/10/2016 11:48 AM, Stefan Bader wrote:
> 
>>> I did not hear about that before. But revisiting things again I think what
>>> happened is that the Xen patch which I had done before (but at that time 
>>> forgot
>>> to submit upstream) was adding quotes there because without, the grep does 
>>> not
>>> work. Back then I did not realize this broke the shutdown as that also had
>>> quotes around the list. All the other users of the list are ok because they 
>>> wrap
>>> the processing into for ... loops and for those newline or space does not 
>>> matter.
>>
>> I think the surprising part is that the calls to virsh like:
>>
>>   list=$(virsh ... list)
>>
>> seem to retain the newline seperator despite having no quotes. Only when 
>> using
>> echo with the unquoted variable seems to do that. That is with dash as sh at 
>> least.
> 
> That's true for ALL shells.  Word splitting does NOT happen during
> variable assignment.  list=$() is _strictly_ equivalent to list="$()",
> no matter the shell.

Funny that with all those years working with shells I never memorized that as a
fact. :) Guess it was enough to adhere to the good habit and just use "$()" all
the time.

So just as a wrap up, the first patch ends up not being a fix in the strict
sense as before the second patch word splitting would happen in the list_guest
function. Luckily it neither breaks anything on its own as the result is the
same whether list was already split or not. Somehow I subconsciously reordered
it to come before the dom0 patch which I had done here before (without noticing
that the list file is then broken for more than one guest to shutdown). Somehow
the big part of adding the grep in the dom0 patch made me miss the additional
quotes around the variable. In hindsight maybe the better way would have been to
add a new line which filters the dom0 uid and reassigns the result to list and
keep the echo line unmodified. But then, the end result now should be working
and robust enough...

-Stefan
> 
> It's just that there are so many other places where $() and "$()" behave
> differently that it's (usually) a good habit to always use "", rather
> than learning the rules on when you NEED them, vs. telling the special
> cases when you DON'T want them apart from the cases where they are optional.
> 




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

Re: [libvirt] [PATCH] libvirt-storage.c:Lines too long, use 80 character columns.

[Nitesh Konkar]

Hello Daniel,

Thanks for the feedback . Actually I only line wrapped those lines
exceeding
80 column limit. Rest I left unchanged. I particularly sent this patch to
get
feedback on whether its a rule or not as HACKING file hints at it
and patch reviews point at it.

Anywaz it does not make any sense now as its clear that we do not apply it
strictly.

Would it be wise to document this in hacking.html saying we encourage but
not
enforce it?

Thanks,
Nitesh.

On Wed, Sep 28, 2016 at 2:34 PM, Daniel P. Berrange 
wrote:

> On Wed, Sep 28, 2016 at 12:27:21AM +0530, Nitesh Konkar wrote:
> > Signed-off-by: Nitesh Konkar 
> > ---
> >  src/libvirt-storage.c | 24 
> >  1 file changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/libvirt-storage.c b/src/libvirt-storage.c
> > index 48996ba..c4f2a03 100644
> > --- a/src/libvirt-storage.c
> > +++ b/src/libvirt-storage.c
> > @@ -233,7 +233,8 @@ virConnectNumOfDefinedStoragePools(virConnectPtr
> conn)
> >
> >  virCheckConnectReturn(conn, -1);
> >
> > -if (conn->storageDriver && conn->storageDriver->
> connectNumOfDefinedStoragePools) {
> > +if (conn->storageDriver &&
> > +conn->storageDriver->connectNumOfDefinedStoragePools) {
> >  int ret;
> >  ret = conn->storageDriver->connectNumOfDefinedStoragePool
> s(conn);
> >  if (ret < 0)
> > @@ -280,7 +281,8 @@ virConnectListDefinedStoragePools(virConnectPtr
> conn,
> >  virCheckNonNullArgGoto(names, error);
> >  virCheckNonNegativeArgGoto(maxnames, error);
> >
> > -if (conn->storageDriver && conn->storageDriver->
> connectListDefinedStoragePools) {
> > +if (conn->storageDriver &&
> > +conn->storageDriver->connectListDefinedStoragePools) {
> >  int ret;
> >  ret = conn->storageDriver->connectListDefinedStoragePools(conn,
> names, maxnames);
> >  if (ret < 0)
> > @@ -332,7 +334,8 @@ virConnectFindStoragePoolSources(virConnectPtr conn,
> >  virCheckNonNullArgGoto(type, error);
> >  virCheckReadOnlyGoto(conn->flags, error);
> >
> > -if (conn->storageDriver && 
> > conn->storageDriver->connectFindStoragePoolSources)
> {
> > +if (conn->storageDriver &&
> > +conn->storageDriver->connectFindStoragePoolSources) {
> >  char *ret;
> >  ret = conn->storageDriver->connectFindStoragePoolSources(conn,
> type, srcSpec, flags);
> >  if (!ret)
> > @@ -485,7 +488,8 @@ virStoragePoolLookupByVolume(virStorageVolPtr vol)
> >
> >  virCheckStorageVolReturn(vol, NULL);
> >
> > -if (vol->conn->storageDriver && 
> > vol->conn->storageDriver->storagePoolLookupByVolume)
> {
> > +if (vol->conn->storageDriver &&
> > +vol->conn->storageDriver->storagePoolLookupByVolume) {
> >  virStoragePoolPtr ret;
> >  ret = vol->conn->storageDriver->storagePoolLookupByVolume(vol);
> >  if (!ret)
> > @@ -1188,7 +1192,8 @@ virStoragePoolNumOfVolumes(virStoragePoolPtr pool)
> >
> >  virCheckStoragePoolReturn(pool, -1);
> >
> > -if (pool->conn->storageDriver && 
> > pool->conn->storageDriver->storagePoolNumOfVolumes)
> {
> > +if (pool->conn->storageDriver &&
> > +pool->conn->storageDriver->storagePoolNumOfVolumes) {
> >  int ret;
> >  ret = pool->conn->storageDriver->storagePoolNumOfVolumes(pool);
> >  if (ret < 0)
> > @@ -1230,7 +1235,8 @@ virStoragePoolListVolumes(virStoragePoolPtr pool,
> >  virCheckNonNullArgGoto(names, error);
> >  virCheckNonNegativeArgGoto(maxnames, error);
> >
> > -if (pool->conn->storageDriver && 
> > pool->conn->storageDriver->storagePoolListVolumes)
> {
> > +if (pool->conn->storageDriver &&
> > +pool->conn->storageDriver->storagePoolListVolumes) {
> >  int ret;
> >  ret = pool->conn->storageDriver->storagePoolListVolumes(pool,
> names, maxnames);
> >  if (ret < 0)
> > @@ -1297,7 +1303,8 @@ virStorageVolLookupByName(virStoragePoolPtr pool,
> >  virCheckStoragePoolReturn(pool, NULL);
> >  virCheckNonNullArgGoto(name, error);
> >
> > -if (pool->conn->storageDriver && 
> > pool->conn->storageDriver->storageVolLookupByName)
> {
> > +if (pool->conn->storageDriver &&
> > +pool->conn->storageDriver->storageVolLookupByName) {
> >  virStorageVolPtr ret;
> >  ret = pool->conn->storageDriver->storageVolLookupByName(pool,
> name);
> >  if (!ret)
> > @@ -1471,7 +1478,8 @@ virStorageVolCreateXML(virStoragePoolPtr pool,
> >  virCheckNonNullArgGoto(xmlDesc, error);
> >  virCheckReadOnlyGoto(pool->conn->flags, error);
> >
> > -if (pool->conn->storageDriver && 
> > pool->conn->storageDriver->storageVolCreateXML)
> {
> > +if (pool->conn->storageDriver &&
> > +pool->conn->storageDriver->storageVolCreateXML) {
> >  virStorageVolPtr ret;
> >  ret = pool->conn->storageDriver->storageVolCreateXML(pool,
> xmlDesc, flags);
> >