Re: [libvirt] [PATCH v4 2/3] conf: Introduce TLS options for VxHS block device clients

[John Ferlan]

On 07/18/2017 11:11 PM, ashish mittal wrote:
> On Mon, Jul 17, 2017 at 4:50 PM, John Ferlan  wrote:
>>
>>
>> On 07/12/2017 06:09 PM, ashish mittal wrote:
>>> On Fri, Jun 30, 2017 at 1:29 PM, John Ferlan  wrote:


 On 06/29/2017 10:02 PM, Ashish Mittal wrote:
> From: Ashish Mittal 
>
> Add a new TLS X.509 certificate type - "vxhs". This will handle the
> creation of a TLS certificate capability for properly configured
> VxHS network block device clients.
>
> Signed-off-by: Ashish Mittal 
> ---
> Changelog:
>
> (1) Add two new options in /etc/libvirt/qemu.conf
> to control TLS behavior with VxHS block devices
> "vxhs_tls" and "vxhs_tls_x509_cert_dir".
> (2) Setting "vxhs_tls=1" in /etc/libvirt/qemu.conf will enable
> TLS for VxHS block devices.
> (3) "vxhs_tls_x509_cert_dir" can be set to the full path where the
> TLS certificates and keys are saved. If this value is missing,
> the "default_tls_x509_cert_dir" will be used instead.
>
>  src/qemu/libvirtd_qemu.aug |  4 
>  src/qemu/qemu.conf | 23 +++
>  src/qemu/qemu_conf.c   |  7 +++
>  src/qemu/qemu_conf.h   |  3 +++
>  src/qemu/test_libvirtd_qemu.aug.in |  2 ++
>  5 files changed, 39 insertions(+)
>

 FWIW: I have another patch upstream:

 https://www.redhat.com/archives/libvir-list/2017-June/msg01341.html

 Which is making some textual and code changes to qemu.conf and
 qemu_conf.c - just so you are aware.


> diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
> index e1983d1..c19bf3a 100644
> --- a/src/qemu/libvirtd_qemu.aug
> +++ b/src/qemu/libvirtd_qemu.aug
> @@ -115,6 +115,9 @@ module Libvirtd_qemu =
>
> let memory_entry = str_entry "memory_backing_dir"
>
> +   let vxhs_entry = bool_entry "vxhs_tls"
> + | str_entry "vxhs_tls_x509_cert_dir"
> +
> (* Each entry in the config is one of the following ... *)
> let entry = default_tls_entry
>   | vnc_entry
> @@ -133,6 +136,7 @@ module Libvirtd_qemu =
>   | nvram_entry
>   | gluster_debug_level_entry
>   | memory_entry
> + | vxhs_entry
>
> let comment = [ label "#comment" . del /#[ \t]*/ "# " .  store /([^ 
> \t\n][^\n]*)?/ . del /\n/ "\n" ]
> let empty = [ label "#empty" . eol ]
> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> index e6c0832..83c2377 100644
> --- a/src/qemu/qemu.conf
> +++ b/src/qemu/qemu.conf
> @@ -250,6 +250,29 @@
>  #chardev_tls_x509_secret_uuid = "----"
>
>
> +# Enable use of TLS encryption on the VxHS network block devices.
> +#
> +# When the VxHS network block device server is set up appropriately,
> +# x509 certificates are used for authentication between the clients
> +# (qemu processes) and the remote VxHS server.
> +#
> +# It is necessary to setup CA and issue client and server certificates
> +# before enabling this.
> +#
> +#vxhs_tls = 1
> +
> +
> +# In order to override the default TLS certificate location for VxHS
> +# device TCP certificates, supply a valid path to the certificate 
> directory.
> +# This is used to authenticate the VxHS block device clients to the VxHS
> +# server.
> +#
> +# If the provided path does not exist then the default_tls_x509_cert_dir
> +# path will be used.
> +#
> +#vxhs_tls_x509_cert_dir = "/etc/pki/libvirt-vxhs"
> +
> +
>  # In order to override the default TLS certificate location for migration
>  # certificates, supply a valid path to the certificate directory. If the
>  # provided path does not exist then the default_tls_x509_cert_dir path

 In patch 3, the call to qemuBuildTLSx509CommandLine has a @verifypeer
 parameter = true, thus it's always going to expect to verify the client
 rather than making that determination using *_tls_x509_verify. So that
 means you have to describe the environment as requiring the
 client-cert.pem and client-key.pem files (like the description for 
 default).

> 
> As you have rightly noted later, we are actually setting up the client
> TLS env here (libvirt/qemu = vxhs client). I hope the default
> libvirt/qemu TLS env can be used for setting up the client connection
> also! If not, then I guess I just have to mandate a dedicated vxhs TLS
> env.
> 
> vxhs server TLS env setup would happen separately. So the @verifypeer
> parameter = true in qemuBuildTLSx509CommandLine would apply to the
> client connection and be ignored anyway (is true by default for the
> client). If, on the vxhs server side, we were to set up the env with
> @verifypeer parameter = false, t

[libvirt] [PATCH] qemu: Fix bug assuming usage of default UUID for certificate passphrase

[John Ferlan]

If an environment specific _tls_x509_cert_dir is provided, then
do not VIR_STRDUP the defaultTLSx509secretUUID as that would be
for the "default" environment and not the vnc, spice, chardev, or
migrate environments. If the environment needs a secret to decode
it's certificate, then it must provide the secret. If the secrets
happen to be the same, then configuration would use the same UUID
as the default (but we cannot assume that nor can we assume that
the secret would be necessary).

Signed-off-by: John Ferlan 
---

 While responding to a different patch today regarding Veritas and
 usage of a default environment w/ or w/o secrets I realized that
 the existing logic has a flaw in "assuming" that someone would want
 to use the default secret. What if they defined their own environment
 without a secret?  Then the code would create a secret object to pass
 to QEMU which would think it needs to use it to decode the server
 certificate (but it doesn't), so it would seemingly fail the start.
 I assume based on the lack of complaints about this that everyone just
 uses the default environment!

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

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index c4714ed..a7a2aaa 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -526,14 +526,18 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr 
cfg,
 goto cleanup;   \
 if (rv == 0)\
 cfg->val## TLSx509verify = cfg->defaultTLSx509verify;   \
-if (virConfGetValueString(conf, #val "_tls_x509_cert_dir",  \
-  &cfg->val## TLSx509certdir) < 0)  \
+if ((rv = virConfGetValueString(conf, #val "_tls_x509_cert_dir",\
+  &cfg->val## TLSx509certdir)) < 0) \
 goto cleanup;   \
 if (virConfGetValueString(conf, \
   #val "_tls_x509_secret_uuid", \
   &cfg->val## TLSx509secretUUID) < 0)   \
 goto cleanup;   \
-if (!cfg->val## TLSx509secretUUID &&\
+/* Only if a *tls_x509_cert_dir wasn't found (e.g. rv == 0), should \
+ * we copy the defaultTLSx509secretUUID. If this environment needs  \
+ * a passphrase to decode the certificate, then it should provide   \
+ * it's own secretUUID for that. */ \
+if (rv == 0 && !cfg->val## TLSx509secretUUID && \
 cfg->defaultTLSx509secretUUID) {\
 if (VIR_STRDUP(cfg->val## TLSx509secretUUID,\
cfg->defaultTLSx509secretUUID) < 0)  \
-- 
2.9.4

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

Re: [libvirt] [PATCH v3 2/2] virt-aa-helper-test: Add test for aarch32 UEFI image path

[John Ferlan]

On 07/20/2017 03:56 PM, dann frazier wrote:
> Signed-off-by: dann frazier 
> ---
>  tests/virt-aa-helper-test | 1 +
>  1 file changed, 1 insertion(+)
> 

Just in case it's not obvious - this one got pushed too.

Tks -

John

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

Re: [libvirt] [PATCH v3 1/2] qemu: Add AAVMF32 to the list of known UEFIs

[John Ferlan]

On 07/21/2017 06:52 AM, Andrea Bolognani wrote:
> On Thu, 2017-07-20 at 13:56 -0600, dann frazier wrote:
>> @@ -131,6 +131,8 @@ void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr 
>> def)
>>   #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"
>> +#define VIR_QEMU_AAVMF32_LOADER_PATH "/usr/share/AAVMF/AAVMF32_CODE.fd"
>> +#define VIR_QEMU_AAVMF32_NVRAM_PATH "/usr/share/AAVMF/AAVMF32_VARS.fd"
>>   
>>   virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
>>   {
>> @@ -335,11 +337,11 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool 
>> privileged)
>>   goto error;
>>   
>>   #else
>> -if (VIR_ALLOC_N(cfg->firmwares, 3) < 0)
>> +if (VIR_ALLOC_N(cfg->firmwares, 4) < 0)
>>   goto error;
>> -cfg->nfirmwares = 3;
>> +cfg->nfirmwares = 4;
>>   if (VIR_ALLOC(cfg->firmwares[0]) < 0 || VIR_ALLOC(cfg->firmwares[1]) < 
>> 0 ||
>> -VIR_ALLOC(cfg->firmwares[2]) < 0)
>> +VIR_ALLOC(cfg->firmwares[2]) < 0 || VIR_ALLOC(cfg->firmwares[3]) < 
>> 0)
>>   goto error;
>>   
>>   if (VIR_STRDUP(cfg->firmwares[0]->name, VIR_QEMU_AAVMF_LOADER_PATH) < 
>> 0 ||
>> @@ -347,7 +349,9 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool 
>> privileged)
>>   VIR_STRDUP(cfg->firmwares[1]->name, VIR_QEMU_OVMF_LOADER_PATH) < 0 
>> ||
>>   VIR_STRDUP(cfg->firmwares[1]->nvram, VIR_QEMU_OVMF_NVRAM_PATH) < 0 
>> ||
>>   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)
>> +VIR_STRDUP(cfg->firmwares[2]->nvram, VIR_QEMU_OVMF_SEC_NVRAM_PATH) 
>> < 0 ||
>> +VIR_STRDUP(cfg->firmwares[3]->name, VIR_QEMU_AAVMF32_LOADER_PATH) < 
>> 0 ||
>> +VIR_STRDUP(cfg->firmwares[3]->nvram, VIR_QEMU_AAVMF32_NVRAM_PATH) < 
>> 0)
>>   goto error;
>>   #endif
> 
> Not your fault of course, but the way this is done at the
> moment is *extremely* yucky.
> 
> I just posted a patch[1] that makes it saner, and although I
> realize that's a bit more work I'd ask you to wait for it to
> land in master and then respin on top of it.

IMO: Probably should have had Dann's patch go first, then do the
adjustment. Water under the bridge though...

So in order to make progress and since it was a simple modification, I
updated Dann's code to utilize the new model in qemu_conf.c from commit
id '6c2c04e75':

# define DEFAULT_LOADER_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/AAVMF/AAVMF32_CODE.fd:/usr/share/AAVMF/AAVMF32_VARS.fd"

Since there's another "looks good though" - I've pushed this to master
(and now pray that some CI build doesn't break ;-))

John

Congrats on your first libvirt patch

> 
> Everything else looks good though :)
> 
> 
> [1] https://www.redhat.com/archives/libvir-list/2017-July/msg00870.html
> -- 
> Andrea Bolognani / Red Hat / Virtualization
> 

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

[libvirt] [PATCH v3] qemu: Check for existence of provided *_tls_x509_cert_dir

[John Ferlan]

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

Introduce virQEMUDriverConfigTLSDirValidateResetDefault to validate
that if any of the *_tls_x509_cert_dir values were set properly and
reset the default value if the default_tls_x509_cert_dir changed.

Update the qemu.conf description for default to describe the consequences
if the default directory path does not exist.

Signed-off-by: John Ferlan 
---

v2: https://www.redhat.com/archives/libvir-list/2017-June/msg01341.html

Follow-ups in July though.

Changes since v2 - reduced verbosity in qemu.conf and adjusted the logic
to create/call virQEMUDriverConfigTLSDirValidateResetDefault after all
the values are read in order to validate the values and adjust the default
if necessary.

Tested by

 1. Having everything commented out, w/ no /etc/pki/qemu:
Works as expected

 2. Uncomment the default_tls_x509_cert_dir, w/ no /etc/pki/qemu:
Fails as expected

 3. Uncomment each of the other *_tls_x509_cert_dir's when directory not exist:
Fails as expected

 4. Use a directory that exists for other _*tls_x509_cert_dirs:
Works as expected

 5. Change the default_tls_x509_cert_dir to an existing directory:
Works as expected, each of the other uncommented *_tls_x509_cert_dirs
will get the new value (tested via debug code) while one that used an
existing default didn't change from it's default /etc/pki/libvirt-*

 src/qemu/qemu.conf |  8 +++
 src/qemu/qemu_conf.c   | 65 +-
 src/qemu/qemu_conf.h   |  4 
 src/qemu/qemu_driver.c |  3 +++
 4 files changed, 79 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index e6c0832..9526aed 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -13,6 +13,14 @@
 #
 #  dh-params.pem - the DH params configuration file
 #
+# If the directory does not exist or contain the necessary files, QEMU
+# domains will fail to start if they are configured to use TLS.
+#
+# In order to overwrite the default path alter the following. This path
+# definition will be used as the default path for other *_tls_x509_cert_dir
+# configuration settings if their default path does not exist or is not
+# specifically set.
+#
 #default_tls_x509_cert_dir = "/etc/pki/qemu"
 
 
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 6f44cbf..87d2c2d 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -451,8 +451,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
 if (!(conf = virConfReadFile(filename, 0)))
 goto cleanup;
 
-if (virConfGetValueString(conf, "default_tls_x509_cert_dir", 
&cfg->defaultTLSx509certdir) < 0)
+if ((rv = virConfGetValueString(conf, "default_tls_x509_cert_dir", 
&cfg->defaultTLSx509certdir)) < 0)
 goto cleanup;
+cfg->checkdefaultTLSx509certdir = (rv == 1);
 if (virConfGetValueBool(conf, "default_tls_x509_verify", 
&cfg->defaultTLSx509verify) < 0)
 goto cleanup;
 if (virConfGetValueString(conf, "default_tls_x509_secret_uuid",
@@ -872,6 +873,68 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
 return ret;
 }
 
+
+/**
+ * @cfg: Recently config values
+ *
+ * Validate the recently read *_tls_x509_cert_dir values and if necessary
+ * update the default value to match the default_tls_x509_cert_dir
+ *
+ * Returns 0 on success, -1 on failure
+ */
+int
+virQEMUDriverConfigTLSDirValidateResetDefault(virQEMUDriverConfigPtr cfg)
+{
+bool newDefault = false;
+
+/* If the default entry was uncommented, then validate existence */
+if (cfg->checkdefaultTLSx509certdir) {
+if (!virFileExists(cfg->defaultTLSx509certdir)) {
+virReportError(VIR_ERR_CONF_SYNTAX,
+   _("default_tls_x509_cert_dir directory '%s' "
+ "does not exist"),
+   cfg->defaultTLSx509certdir);
+return -1;
+}
+if (STRNEQ(cfg->defaultTLSx509certdir, SYSCONFDIR "/pki/qemu"))
+newDefault = true;
+}
+
+/* We know virQEMUDriverConfigNew set the particular value to either
+ * it's default or default_tls_x509_cert_dir's default. So, if not the
+ * default default and the directory doesn't exist, then the entry was
+ * set in the config file to something that doesn't exist, so error.
+ *
+ * Also, if the defaultTLSx509certdir value was changed from the default,
+ * then we need to update the default for each setting as well to match
+ * the default_tls_x509_cert_dir.
+ */
+#define VALIDATE_TLS_X509_CERT_DIR(val)   \
+do {  \
+if (STRNEQ(cfg->val ## TLSx509certdir, SYSCONFDIR "/pki/qemu") && \
+!virFileExists(cfg->val ## TLSx509certdir)) { \
+virReportError(VIR_ERR_CONF_SYNTAX,   \
+   _(#val"_tls_x5

[libvirt] [PATCH 0/3] properly handle '=' in the VNC socket path

[Pavel Hrdina]

Pavel Hrdina (3):
  qemu: capabilities: introduce QEMU_CAPS_VNC_MULTI_SERVERS
  qemu: properly handle '=' in the VNC socket path
  tests: add test case for VNC unix path with '='

 src/qemu/qemu_capabilities.c   |  4 ++
 src/qemu/qemu_capabilities.h   |  3 ++
 src/qemu/qemu_command.c|  5 ++-
 src/qemu/qemu_process.c| 51 --
 tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml   |  1 +
 tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml   |  1 +
 .../caps_2.6.0-gicv2.aarch64.xml   |  1 +
 .../caps_2.6.0-gicv3.aarch64.xml   |  1 +
 tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml  |  1 +
 tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml   |  1 +
 tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml|  1 +
 tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml   |  1 +
 tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml|  1 +
 tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml   |  1 +
 tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.xml  |  1 +
 tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml|  1 +
 tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml   |  1 +
 ...muxml2argv-graphics-vnc-socket-new-cmdline.args | 22 ++
 ...emuxml2argv-graphics-vnc-socket-new-cmdline.xml | 20 +
 ...argv-graphics-vnc-socket-path-not-supported.xml |  1 +
 .../qemuxml2argvdata/qemuxml2argv-name-escape.args |  2 +-
 tests/qemuxml2argvtest.c   |  5 +++
 22 files changed, 121 insertions(+), 5 deletions(-)
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-socket-new-cmdline.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-socket-new-cmdline.xml
 create mode 12 
tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-socket-path-not-supported.xml

-- 
2.13.3

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

[libvirt] [PATCH 2/3] qemu: properly handle '=' in the VNC socket path

[Pavel Hrdina]

If a domain name contains a '=' and the unix socket path is
auto-generated or socket path provided by user contains '=' QEMU
is unable to properly parse the command line.  In order to make it
work we need to use the new command line syntax for VNC or we will
fail to start/define such domain.

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

Signed-off-by: Pavel Hrdina 
---
 src/qemu/qemu_command.c|  5 ++-
 src/qemu/qemu_process.c| 51 --
 .../qemuxml2argvdata/qemuxml2argv-name-escape.args |  2 +-
 tests/qemuxml2argvtest.c   |  1 +
 4 files changed, 54 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 0ce5aa5906..a0394a00ae 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7802,7 +7802,10 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr 
cfg,
 
 switch (glisten->type) {
 case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET:
-virBufferAddLit(&opt, "unix:");
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNC_MULTI_SERVERS))
+virBufferAddLit(&opt, "vnc=unix:");
+else
+virBufferAddLit(&opt, "unix:");
 virQEMUBuildBufferEscapeComma(&opt, glisten->socket);
 break;
 
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 525521aaf0..4e93420955 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4146,6 +4146,34 @@ 
qemuProcessGraphicsSetupNetworkAddress(virDomainGraphicsListenDefPtr glisten,
 return 0;
 }
 
+static int
+qemuProcessVncValidateUnixSocket(const char *socket,
+ virQEMUCapsPtr qemuCaps)
+{
+if (!socket)
+return 0;
+
+/* The way how QEMU process options disallow using '=' in the unix socket
+ * path.  The reason is that the first option doesn't have to use its name
+ * and -vnc has it's first option without name.  However when the parser
+ * finds first '=' in the option it always split it into key=value pair
+ * which blocks having '=' in the socket path.  Since QEMU 2.3.0 it's 
possible
+ * to use "vnc" as a key for the first option so we can use a unix socket
+ * path with '='. */
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNC_MULTI_SERVERS))
+return 0;
+
+if (strchr(socket, '=')) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("QEMU does not support having '=' in the "
+ "VNC socket path '%s'."), socket);
+return -1;
+}
+
+return 0;
+}
+
+
 
 static int
 qemuProcessGraphicsSetupListen(virQEMUDriverPtr driver,
@@ -4192,6 +4220,9 @@ qemuProcessGraphicsSetupListen(virQEMUDriverPtr driver,
 if (virAsprintf(&glisten->socket, "%s/%s.sock",
 priv->libDir, type) < 0)
 goto cleanup;
+if (qemuProcessVncValidateUnixSocket(glisten->socket,
+ priv->qemuCaps) < 0)
+goto cleanup;
 glisten->fromConfig = true;
 glisten->type = VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET;
 } else if (listenAddr) {
@@ -4216,6 +4247,9 @@ qemuProcessGraphicsSetupListen(virQEMUDriverPtr driver,
 if (virAsprintf(&glisten->socket, "%s/%s.sock",
 priv->libDir, type) < 0)
 goto cleanup;
+if (qemuProcessVncValidateUnixSocket(glisten->socket,
+ priv->qemuCaps) < 0)
+goto cleanup;
 glisten->autoGenerated = true;
 }
 break;
@@ -4440,9 +4474,10 @@ qemuProcessStartWarnShmem(virDomainObjPtr vm)
 
 
 static int
-qemuProcessStartValidateGraphics(virDomainObjPtr vm)
+qemuProcessStartValidateGraphics(virDomainObjPtr vm,
+ virQEMUCapsPtr qemuCaps)
 {
-size_t i;
+size_t i, j;
 
 for (i = 0; i < vm->def->ngraphics; i++) {
 virDomainGraphicsDefPtr graphics = vm->def->graphics[i];
@@ -4464,6 +4499,16 @@ qemuProcessStartValidateGraphics(virDomainObjPtr vm)
 case VIR_DOMAIN_GRAPHICS_TYPE_LAST:
 break;
 }
+
+if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
+for (j = 0; j < graphics->nListens; j++) {
+virDomainGraphicsListenDefPtr glisten = &graphics->listens[j];
+if (glisten->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET &&
+qemuProcessVncValidateUnixSocket(glisten->socket, 
qemuCaps) < 0) {
+return -1;
+}
+}
+}
 }
 
 return 0;
@@ -4633,7 +4678,7 @@ qemuProcessStartValidate(virQEMUDriverPtr driver,
 if (qemuProcessStartValidateXML(driver, vm, qemuCaps, caps, flags) < 0)
 return 

[libvirt] [PATCH 1/3] qemu: capabilities: introduce QEMU_CAPS_VNC_MULTI_SERVERS

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina 
---
 src/qemu/qemu_capabilities.c| 4 
 src/qemu/qemu_capabilities.h| 3 +++
 tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml| 1 +
 tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml| 1 +
 tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml | 1 +
 tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml | 1 +
 tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml   | 1 +
 tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml| 1 +
 tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml | 1 +
 tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml| 1 +
 tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml | 1 +
 tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml| 1 +
 tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.xml   | 1 +
 tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 +
 tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml| 1 +
 15 files changed, 20 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index c3f49a9754..e393b4ade6 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -431,7 +431,10 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
   "virtio.ats",
   "loadparm",
   "spapr-pci-host-bridge",
+
+  /* 265 */
   "spapr-pci-host-bridge.numa_node",
+  "vnc-multi-servers"
 );
 
 
@@ -3221,6 +3224,7 @@ static struct virQEMUCapsCommandLineProps 
virQEMUCapsCommandLine[] = {
 { "spice", "rendernode", QEMU_CAPS_SPICE_RENDERNODE },
 { "machine", "kernel_irqchip", QEMU_CAPS_MACHINE_KERNEL_IRQCHIP },
 { "machine", "loadparm", QEMU_CAPS_LOADPARM },
+{ "vnc", "vnc", QEMU_CAPS_VNC_MULTI_SERVERS },
 };
 
 static int
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 19f59a6d41..196ea3af87 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -417,7 +417,10 @@ typedef enum {
 QEMU_CAPS_VIRTIO_PCI_ATS, /* virtio-*-pci.ats */
 QEMU_CAPS_LOADPARM, /* -machine loadparm */
 QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, /* -device spapr-pci-host-bridge */
+
+/* 265 */
 QEMU_CAPS_SPAPR_PCI_HOST_BRIDGE_NUMA_NODE, /* 
spapr-pci-host-bridge.numa_node= */
+QEMU_CAPS_VNC_MULTI_SERVERS, /* -vnc vnc=unix:/path */
 
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml
index b7ba2240aa..11c8765738 100644
--- a/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml
@@ -185,6 +185,7 @@
   
   
   
+  
   2004000
   0
   
diff --git a/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml
index f3289df306..eee1ab24d9 100644
--- a/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml
@@ -191,6 +191,7 @@
   
   
   
+  
   2005000
   0
   
diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml 
b/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml
index af60bf6c6e..f8746f4c1e 100644
--- a/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml
@@ -169,6 +169,7 @@
   
   
   
+  
   2006000
   0
   
diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml 
b/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml
index d4eb19102c..0ce1c8a333 100644
--- a/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml
@@ -169,6 +169,7 @@
   
   
   
+  
   2006000
   0
   
diff --git a/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml 
b/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml
index cd3c0b5aa6..92dba13b06 100644
--- a/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml
+++ b/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml
@@ -164,6 +164,7 @@
   
   
   
+  
   2006000
   0
   
diff --git a/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml
index e4615c02ab..1937dc9c11 100644
--- a/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml
@@ -201,6 +201,7 @@
   
   
   
+  
   2006000
   0
   
diff --git a/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml 
b/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml
index ec79115cf1..1c1aab8dec 100644
--- a/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml
+++ b/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml
@@ -131,6 +131,7 @@
   
   
   
+  
   2007000
   0
   
diff --git a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml
index 5f98db8d1a..b484411314 100644
--- a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.7.0.x86_

[libvirt] [PATCH 3/3] tests: add test case for VNC unix path with '='

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina 
---
 ...muxml2argv-graphics-vnc-socket-new-cmdline.args | 22 ++
 ...emuxml2argv-graphics-vnc-socket-new-cmdline.xml | 20 
 ...argv-graphics-vnc-socket-path-not-supported.xml |  1 +
 tests/qemuxml2argvtest.c   |  4 
 4 files changed, 47 insertions(+)
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-socket-new-cmdline.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-socket-new-cmdline.xml
 create mode 12 
tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-socket-path-not-supported.xml

diff --git 
a/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-socket-new-cmdline.args 
b/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-socket-new-cmdline.args
new file mode 100644
index 00..862e96e066
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-socket-new-cmdline.args
@@ -0,0 +1,22 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-i686 \
+-name QEMUGuest1 \
+-S \
+-M pc \
+-m 214 \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-nodefaults \
+-chardev 
socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
+server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=readline \
+-no-acpi \
+-boot c \
+-usb \
+-vnc vnc=unix:/tmp/foo=bar.sock \
+-vga cirrus
diff --git 
a/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-socket-new-cmdline.xml 
b/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-socket-new-cmdline.xml
new file mode 100644
index 00..2b9758fecb
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-socket-new-cmdline.xml
@@ -0,0 +1,20 @@
+
+  QEMUGuest1
+  c7a5fdbd-edaf-9455-926a-d65c16db1809
+  219100
+  219100
+  1
+  
+hvm
+  
+  
+/usr/bin/qemu-system-i686
+
+  
+
+
+  
+
+
+  
+
diff --git 
a/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-socket-path-not-supported.xml
 
b/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-socket-path-not-supported.xml
new file mode 12
index 00..0102f50a76
--- /dev/null
+++ 
b/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-socket-path-not-supported.xml
@@ -0,0 +1 @@
+qemuxml2argv-graphics-vnc-socket-new-cmdline.xml
\ No newline at end of file
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 7dc7e52d86..dfe23876d1 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1053,6 +1053,10 @@ mymain(void)
 DO_TEST("graphics-vnc-auto-socket", QEMU_CAPS_VNC,
 QEMU_CAPS_DEVICE_CIRRUS_VGA);
 DO_TEST("graphics-vnc-none", QEMU_CAPS_VNC, QEMU_CAPS_DEVICE_CIRRUS_VGA);
+DO_TEST("graphics-vnc-socket-new-cmdline", QEMU_CAPS_VNC,
+QEMU_CAPS_DEVICE_CIRRUS_VGA, QEMU_CAPS_VNC_MULTI_SERVERS);
+DO_TEST_FAILURE("graphics-vnc-socket-path-not-supported", QEMU_CAPS_VNC,
+QEMU_CAPS_DEVICE_CIRRUS_VGA);
 
 driver.config->vncSASL = 1;
 VIR_FREE(driver.config->vncSASLdir);
-- 
2.13.3

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

[libvirt] [PATCH 1/3] migrate-getmaxdowntime command qemu side

[seg]

From: Scott Garfinkle 

JSON-only implementation to retrieve downtime-limit from Qemu - aka
GetMaxDowntime.

Signed-off-by: Scott Garfinkle 

---
 src/qemu/qemu_driver.c   | 45 
 src/qemu/qemu_monitor.c  | 12 
 src/qemu/qemu_monitor.h  |  3 +++
 src/qemu/qemu_monitor_json.c | 40 +++
 src/qemu/qemu_monitor_json.h |  3 +++
 5 files changed, 103 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8d261b7..72b4d8c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -13152,6 +13152,50 @@ qemuDomainMigrateSetMaxDowntime(virDomainPtr dom,
 return ret;
 }
 
+
+static int
+qemuDomainMigrateGetMaxDowntime(virDomainPtr dom,
+unsigned long long *downtime,
+unsigned int flags)
+{
+virQEMUDriverPtr driver = dom->conn->privateData;
+virDomainObjPtr vm;
+qemuDomainObjPrivatePtr priv;
+int ret = -1;
+
+virCheckFlags(0, -1);
+
+if (!(vm = qemuDomObjFromDomain(dom)))
+goto cleanup;
+
+if (virDomainMigrateGetMaxDowntimeEnsureACL(dom->conn, vm->def) < 0)
+goto cleanup;
+
+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
+goto cleanup;
+
+if (!virDomainObjIsActive(vm)) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+   "%s", _("domain is not running"));
+goto endjob;
+}
+
+priv = vm->privateData;
+qemuDomainObjEnterMonitor(driver, vm);
+
+ret = qemuMonitorGetMigrationDowntime(priv->mon, downtime);
+
+if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ret = -1;
+
+ endjob:
+qemuDomainObjEndJob(driver, vm);
+
+ cleanup:
+virDomainObjEndAPI(&vm);
+return ret;
+}
+
 static int
 qemuDomainMigrateGetCompressionCache(virDomainPtr dom,
  unsigned long long *cacheSize,
@@ -20796,6 +20840,7 @@ static virHypervisorDriver qemuHypervisorDriver = {
 .domainGetJobInfo = qemuDomainGetJobInfo, /* 0.7.7 */
 .domainGetJobStats = qemuDomainGetJobStats, /* 1.0.3 */
 .domainAbortJob = qemuDomainAbortJob, /* 0.7.7 */
+.domainMigrateGetMaxDowntime = qemuDomainMigrateGetMaxDowntime, /* 3.6.0 */
 .domainMigrateSetMaxDowntime = qemuDomainMigrateSetMaxDowntime, /* 0.8.0 */
 .domainMigrateGetCompressionCache = qemuDomainMigrateGetCompressionCache, 
/* 1.0.3 */
 .domainMigrateSetCompressionCache = qemuDomainMigrateSetCompressionCache, 
/* 1.0.3 */
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 2b0afcc..b17a60b 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2552,6 +2552,18 @@ qemuMonitorSetMigrationDowntime(qemuMonitorPtr mon,
 
 
 int
+qemuMonitorGetMigrationDowntime(qemuMonitorPtr mon,
+unsigned long long *downtime)
+{
+VIR_DEBUG("downtime=%p", downtime);
+
+QEMU_CHECK_MONITOR_JSON(mon);
+
+return qemuMonitorJSONGetMigrationDowntime(mon, downtime);
+}
+
+
+int
 qemuMonitorGetMigrationCacheSize(qemuMonitorPtr mon,
  unsigned long long *cacheSize)
 {
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 1697db5..d094d09 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -595,6 +595,9 @@ int qemuMonitorSavePhysicalMemory(qemuMonitorPtr mon,
 int qemuMonitorSetMigrationSpeed(qemuMonitorPtr mon,
  unsigned long bandwidth);
 
+int qemuMonitorGetMigrationDowntime(qemuMonitorPtr mon,
+unsigned long long *downtime);
+
 int qemuMonitorSetMigrationDowntime(qemuMonitorPtr mon,
 unsigned long long downtime);
 
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 65b1fbb..8fc11eb 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -2450,6 +2450,46 @@ int qemuMonitorJSONEjectMedia(qemuMonitorPtr mon,
 virJSONValueFree(cmd);
 virJSONValueFree(reply);
 return ret;
+} 
+
+
+int qemuMonitorJSONGetMigrationDowntime(qemuMonitorPtr mon,
+unsigned long long *downtime)
+{
+int ret = -1;
+virJSONValuePtr cmd;
+virJSONValuePtr reply = NULL;
+virJSONValuePtr result;
+unsigned int value = 0;
+
+if (!(cmd = qemuMonitorJSONMakeCommand("query-migrate-parameters", NULL)))
+return -1;
+
+if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
+goto cleanup;
+
+if (qemuMonitorJSONHasError(reply, "CommandNotFound"))
+goto cleanup;
+
+if (qemuMonitorJSONCheckError(cmd, reply) < 0)
+goto cleanup;
+
+if(!(result = virJSONValueObjectGet(reply, "return")))
+goto cleanup;
+
+if (virJSONValueObjectGetNumberUint(result, "downtime-limit", &value) < 0) 
{
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("missing migratio

[libvirt] [PATCH 0/3] implement migrate-getmaxdowntime command

[seg]

Currently, the maximum tolerable downtime for a domain being migrated is
write-only. This patch implements a way to query that value nondestructively.


Signed-off-by: Scott Garfinkle 

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

[libvirt] [PATCH 2/3] migrate-getmaxdowntime public symbols and table indices

[seg]

From: Scott Garfinkle 

virsh migrate-getmaxdowntime command public symbols, indices, and dependencies.

Signed-off-by: Scott Garfinkle 

---
 src/libvirt_public.syms  |  4 
 src/remote/remote_protocol.x | 16 +++-
 src/remote_protocol-structs  |  8 
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
index fac77fb..da5692a 100644
--- a/src/libvirt_public.syms
+++ b/src/libvirt_public.syms
@@ -768,4 +768,8 @@ LIBVIRT_3.4.0 {
 virStreamSparseSendAll;
 } LIBVIRT_3.1.0;
 
+LIBVIRT_3.6.0 {
+global:
+virDomainMigrateGetMaxDowntime;
+} LIBVIRT_3.4.0;
 #  define new API here using predicted next version number 
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
index aa0aa38..e1f4e27 100644
--- a/src/remote/remote_protocol.x
+++ b/src/remote/remote_protocol.x
@@ -2326,6 +2326,15 @@ struct remote_domain_abort_job_args {
 };
 
 
+struct remote_domain_migrate_get_max_downtime_args {
+remote_nonnull_domain dom;
+unsigned int flags;
+};
+
+struct remote_domain_migrate_get_max_downtime_ret {
+ unsigned hyper downtime; /* insert@1 */
+};
+
 struct remote_domain_migrate_set_max_downtime_args {
 remote_nonnull_domain dom;
 unsigned hyper downtime;
@@ -6064,7 +6073,12 @@ enum remote_procedure {
  * @generate: both
  * @acl: domain:write
  */
-REMOTE_PROC_DOMAIN_SET_BLOCK_THRESHOLD = 386
+REMOTE_PROC_DOMAIN_SET_BLOCK_THRESHOLD = 386,
 
+/**
+ * @generate: both
+ * @acl: domain:migrate
+ */
+REMOTE_PROC_DOMAIN_MIGRATE_GET_MAX_DOWNTIME = 387
 
 };
diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
index a46fe37..5804e60 100644
--- a/src/remote_protocol-structs
+++ b/src/remote_protocol-structs
@@ -1778,6 +1778,13 @@ struct remote_domain_migrate_set_max_downtime_args {
 uint64_t   downtime;
 u_int  flags;
 };
+struct remote_domain_migrate_get_max_downtime_args {
+remote_nonnull_domain  dom;
+u_int  flags;
+};
+struct remote_domain_migrate_get_max_downtime_ret {
+uint64_t   downtime;
+};
 struct remote_domain_migrate_get_compression_cache_args {
 remote_nonnull_domain  dom;
 u_int  flags;
@@ -3233,4 +3240,5 @@ enum remote_procedure {
 REMOTE_PROC_DOMAIN_SET_VCPU = 384,
 REMOTE_PROC_DOMAIN_EVENT_BLOCK_THRESHOLD = 385,
 REMOTE_PROC_DOMAIN_SET_BLOCK_THRESHOLD = 386,
+REMOTE_PROC_DOMAIN_GET_MAX_DOWNTIME = 387
 };
-- 
1.8.3.1

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

Re: [libvirt] 答复: Re: [PATCH v2] network: allow to specify buffer size fornetlink socket

[Laine Stump]

On 07/20/2017 11:08 PM, lu.zhip...@zte.com.cn wrote:
> i  will try to update libnl3 to 3.2.29  ,now  libnl3 is 3.2.8 in my host

Do you really mean 3.2.8 and not 3.2.28? 3.2.8 was released in April
2012, so a lot has changed since then; 3.2.28 was released in July 2016,
so much more recent.

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

[libvirt] [PATCH 3/3] migrate-getmaxdowntime local/libvirt bits

[seg]

From: Scott Garfinkle 

virsh migrate-getmaxdowntime command libvirt and virsh side, including help

Signed-off-by: Scott Garfinkle 

---
 include/libvirt/libvirt-domain.h |  4 
 src/driver-hypervisor.h  |  6 ++
 src/libvirt-domain.c | 43 +
 src/remote/remote_driver.c   |  1 +
 tools/virsh-domain.c | 46 
 tools/virsh.pod  | 18 
 6 files changed, 118 insertions(+)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 45f939a..ed3c461 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -1043,6 +1043,10 @@ int virDomainMigrateSetMaxDowntime (virDomainPtr domain,
 unsigned long long downtime,
 unsigned int flags);
 
+int virDomainMigrateGetMaxDowntime (virDomainPtr domain,
+unsigned long long *downtime,
+unsigned int flags);
+
 int virDomainMigrateGetCompressionCache(virDomainPtr domain,
 unsigned long long *cacheSize,
 unsigned int flags);
diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
index 3053d7a..7d32cad 100644
--- a/src/driver-hypervisor.h
+++ b/src/driver-hypervisor.h
@@ -702,6 +702,11 @@ typedef int
  unsigned int flags);
 
 typedef int
+(*virDrvDomainMigrateGetMaxDowntime)(virDomainPtr domain,
+ unsigned long long *downtime,
+ unsigned int flags);
+
+typedef int
 (*virDrvDomainMigrateGetCompressionCache)(virDomainPtr domain,
   unsigned long long *cacheSize,
   unsigned int flags);
@@ -1412,6 +1417,7 @@ struct _virHypervisorDriver {
 virDrvDomainGetJobInfo domainGetJobInfo;
 virDrvDomainGetJobStats domainGetJobStats;
 virDrvDomainAbortJob domainAbortJob;
+virDrvDomainMigrateGetMaxDowntime domainMigrateGetMaxDowntime;
 virDrvDomainMigrateSetMaxDowntime domainMigrateSetMaxDowntime;
 virDrvDomainMigrateGetCompressionCache domainMigrateGetCompressionCache;
 virDrvDomainMigrateSetCompressionCache domainMigrateSetCompressionCache;
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 4033ae8..12417e2 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -8781,6 +8781,49 @@ virDomainMigrateSetMaxDowntime(virDomainPtr domain,
 
 
 /**
+ * virDomainMigrateGetMaxDowntime:
+ * @domain: a domain object
+ * @downtime: return value of the maximum tolerable downtime for live 
migration, in milliseconds
+ * @flags: extra flags; not used yet, so callers should always pass 0
+ *
+ * Gets current maximum tolerable time for which the domain is allowed to be 
paused
+ * at the end of live migration. It's supposed to be called while the domain is
+ * being live-migrated as a reaction to migration progress.
+ *
+ * Returns 0 in case of success, -1 otherwise.
+ */
+int
+virDomainMigrateGetMaxDowntime(virDomainPtr domain,
+   unsigned long long *downtime,
+   unsigned int flags)
+{
+virConnectPtr conn;
+
+VIR_DOMAIN_DEBUG(domain, "downtime = %p, flags=%x", downtime, flags);
+
+virResetLastError();
+
+virCheckDomainReturn(domain, -1);
+conn = domain->conn;
+
+virCheckNonNullArgGoto(downtime, error);
+
+//unlike SetMaxDowntime, it's okay for the connection to be readonly
+
+if (conn->driver->domainMigrateGetMaxDowntime) {
+if (conn->driver->domainMigrateGetMaxDowntime(domain, downtime, flags) 
< 0)
+goto error;
+return 0;
+}
+
+virReportUnsupportedError();
+ error:
+virDispatchError(conn);
+return -1;
+}
+
+
+/**
  * virDomainMigrateGetCompressionCache:
  * @domain: a domain object
  * @cacheSize: return value of current size of the cache (in bytes)
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index a57d25f..aa8d8a1 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -8400,6 +8400,7 @@ static virHypervisorDriver hypervisor_driver = {
 .domainGetJobInfo = remoteDomainGetJobInfo, /* 0.7.7 */
 .domainGetJobStats = remoteDomainGetJobStats, /* 1.0.3 */
 .domainAbortJob = remoteDomainAbortJob, /* 0.7.7 */
+.domainMigrateGetMaxDowntime = remoteDomainMigrateGetMaxDowntime, /* 3.6.0 
*/
 .domainMigrateSetMaxDowntime = remoteDomainMigrateSetMaxDowntime, /* 0.8.0 
*/
 .domainMigrateGetCompressionCache = 
remoteDomainMigrateGetCompressionCache, /* 1.0.3 */
 .domainMigrateSetCompressionCache = 
remoteDomainMigrateSetCompressionCache, /* 1.0.3 */
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 0684979..1b524a3 100644
--- a/tools/virsh-doma

Re: [libvirt] [PATCH v2] qemu: Remove duplicated code in qemuBuildSerialChrDeviceStr()

[Andrea Bolognani]

On Thu, 2017-06-22 at 16:03 -0400, Laine Stump wrote:
> > The call to qemuBuildDeviceAddressStr() happens no matter
> > what, so we can move it to the outer possible scope inside
> > the function.
> > 
> > We can also move the call to virBufferAsprintf() after all
> > the checks have been performed, where it makes more sense.
> > 
> > Signed-off-by: Andrea Bolognani 
> > ---
> >  src/qemu/qemu_command.c | 22 +++---
> >  1 file changed, 7 insertions(+), 15 deletions(-)

Picking up old stuff...

> I haven't looked at the caller of qemuBuildSerialChrDeviceStr() with a
> fine-toothed comb, but this patch does change the code a bit. In
> particular, previously if qemuDomainIsPSeries(def) was true, but either
> one of these was false:
> 
>  serial->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL
>  serial->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO
> 
> then it would return an empty string in *deviceStr. But now it will
> instead return an address string, without the
> "spapr-vty,chardev=charBLAH" at the beginning. So can we guarantee that
> the above two conditions are always true for PSeries?

The check on serial->deviceType is pointless, because we're
only calling this function for TYPE_SERIAL. I'm pretty sure
the latter can't be false because...

> I guess in both the "before" and "after" cases, the result would be a
> failure (with before, we would end up with a "-device" with nothing
> following it, and now we would have "-device" with an address string but
> none of the preceding stuff describing the address).

... otherwise we would have run into this. But I'll check.

> (Similarly, before this patch, if serial->targetType wasn't one of
> VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE(USB|ISA|PCI) then *deviceStr would
> contain "(null),chardev=charBLAH,id=BLAH" with no address, but now it
> will have an address as well as the nonsensical preamble. (yes, I'm
> being ridiculous in this case :-P))

The only other case would be TYPE_LAST, which hopefully
will never happen. I can add a check for it though.

> In the end, ACK to your patch  since you haven't made the behavior any
> worse (and have eliminated a bunch of duplicated code, but I do think
> that either the checks on deviceType and info.type should be removed as
> pointless, or that they should trigger a failure directly rather than
> just creating a bad commandline.

I think we can go further and unify the way things are
handled so that pSeries doesn't need to be special-cased.

I've pushed the patch as it is for the moment, based on
your ACK, and will post a follow-up shortly.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH] libvirt-domain.h: Fix enum description placement

[Daniel P. Berrange]

On Fri, Jul 21, 2017 at 04:33:06PM +0200, Jiri Denemark wrote:
> On Fri, Jul 21, 2017 at 14:42:05 +0100, Daniel P. Berrange wrote:
> > On Fri, Jul 21, 2017 at 03:00:20PM +0200, Martin Kletzander wrote:
> 
> > > struct meh {
> > >   /*# This is comment for the following member foo */
> > >   unsigned int foo;
> > >   int bar; /*< This is for member bar that's on the same line */
> > > }
> > > 
> > > and so on.  If that doesn't help either and it never worked,
> > > then... it's a pity :-/
> > 
> > That is ambiguous - without seeing whitespace, the parser cannot
> > distinguish between these two scenarios:
> > 
> >  struct meh {
> >unsigned int foo; /*# This is comment for the following member foo */
> >int bar;
> >  }
> 
> Martin suggested < to be used instead of # for this type of comments to
> remove the ambiguity.
> 
> >  struct meh {
> >unsigned int foo;
> >
> > /*# This is comment for the following member foo */
> >int bar;
> >  }
> 
> However, I think we could just simply force using only comments above
> each member in our public header files and fix the generator to properly
> work with them. This should be a lot easier than fixing it to handle
> both options.

I'd probably go for doing that. In many cases where we put the comment
at the end of the line, we're forced to split it across many lines
anyway, lest the line get too long. Putting commments before the
item means we won't hit the line length so easily, so more comments
will fit on one line.

Oh and this issue affects struct field members too IIUC

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

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

Re: [libvirt] [PATCH] libvirt-domain.h: Fix enum description placement

[Martin Kletzander]

On Fri, Jul 21, 2017 at 02:42:05PM +0100, Daniel P. Berrange wrote:

On Fri, Jul 21, 2017 at 03:00:20PM +0200, Martin Kletzander wrote:

On Fri, Jul 21, 2017 at 11:58:45AM +0100, Daniel P. Berrange wrote:
> On Fri, Jul 21, 2017 at 12:52:06PM +0200, Michal Privoznik wrote:
> > There are only two acceptable places for describing enum values.
> > It's either:
> >
> > typedef enum {
> > /* Some long description. Therefore it's placed before
> >  * the value. */
> > VIR_ENUM_A_VAL = 1,
> > } virEnumA;
> >
> > or:
> >
> > typedef enum {
> > VIR_ENUM_B_VAL = 1, /* Some short description */
> > } virEnumB;
> >
> > However, during review of a patch sent upstream I realized that
> > is not always the case. I went through all the public header
> > files and identified all the offenders. Luckily there were just
> > two of them.
> >
> > Yes, this makes our HTML generated documentation broken, but
> > that's bug of the generator. Our header files shouldn't be forced
> > to use something we don't want to.
>
> FWIW, the problem is in the parseEnumBLock() method in apibuild.py
>
> Once it has completed parsing an enum item, it delays adding that
> enum to the list until it sees the next item, so that it can capture
> the trailing comment.
>
> The only way we can distinguish between a comment that comes before
> the enum vs a comment after the enum, on the same line, is by detecting
> whitespace (newline) before the trailing comment. Unfortunately I don't
> thing this is exposed by the lexer right, so its not entirely easy
> to solve.
>

I was under the impression that this worked, we only broke it by some
recent commit.  I looked at the code and got pretty confused by it, but
shouldn't it be pretty easy (from a big picture view)?  You read until
you have both comment and a member of the struct.

If it's really harder than I think, then we can start using some helper
characters for the comments (at least for now) so that we can properly
identify them, e.g.:

struct meh {
  /*# This is comment for the following member foo */
  unsigned int foo;
  int bar; /*< This is for member bar that's on the same line */
}

and so on.  If that doesn't help either and it never worked,
then... it's a pity :-/


That is ambiguous - without seeing whitespace, the parser cannot
distinguish between these two scenarios:



Oh, you're right, so the only other easy workaround option that I can
think of is to just document every single thing, then the parser can
always just first two things (comment and member) together.

Or, as Jirka suggested, just make it mandatory to put the comments
before and fix the script to accept just that order.


struct meh {
  unsigned int foo; /*# This is comment for the following member foo */
  int bar;
}

struct meh {
  unsigned int foo;

   /*# This is comment for the following member foo */
  int bar;
}




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

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


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

Re: [libvirt] [PATCH] libvirt-domain.h: Fix enum description placement

[Jiri Denemark]

On Fri, Jul 21, 2017 at 14:42:05 +0100, Daniel P. Berrange wrote:
> On Fri, Jul 21, 2017 at 03:00:20PM +0200, Martin Kletzander wrote:

> > struct meh {
> >   /*# This is comment for the following member foo */
> >   unsigned int foo;
> >   int bar; /*< This is for member bar that's on the same line */
> > }
> > 
> > and so on.  If that doesn't help either and it never worked,
> > then... it's a pity :-/
> 
> That is ambiguous - without seeing whitespace, the parser cannot
> distinguish between these two scenarios:
> 
>  struct meh {
>unsigned int foo; /*# This is comment for the following member foo */
>int bar;
>  }

Martin suggested < to be used instead of # for this type of comments to
remove the ambiguity.

>  struct meh {
>unsigned int foo;
>
> /*# This is comment for the following member foo */
>int bar;
>  }

However, I think we could just simply force using only comments above
each member in our public header files and fix the generator to properly
work with them. This should be a lot easier than fixing it to handle
both options.

Jirka

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

Re: [libvirt] [PATCH 2/3] qemu: command: rework adding of default cpu model

[Ján Tomko]

On Fri, Jul 14, 2017 at 07:43:05PM -0400, Cole Robinson wrote:

Certain XML features that aren't in the  block map to -cpu
flags on the qemu cli. If one of these is specified but the user
didn't explicitly pass an XML  model, we need to format a
default model on the command line.

The current code handles this by sprinkling this default cpu handling
among all the different flag string formatting. Instead, switch it
to do this just once.

This alters some test output slightly: the previous code would
write the default -cpu in some cases when no flags were actually
added, so the output was redundant.

Signed-off-by: Cole Robinson 
---
src/qemu/qemu_command.c| 72 +++---
.../qemuxml2argvdata/qemuxml2argv-hyperv-off.args  |  1 -
.../qemuxml2argv-kvm-features-off.args |  1 -
3 files changed, 22 insertions(+), 52 deletions(-)



ACK

Jan


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

Re: [libvirt] [PATCH 3/3] qemu: command: explicitly error for non-x86 default CPU

[Ján Tomko]

On Fri, Jul 14, 2017 at 07:43:06PM -0400, Cole Robinson wrote:

The code only currently handles writing an x86 default -cpu
argument, and doesn't know anything about other architectures.
Let's make this explicit rather than leaving ex. qemu ppc64 to
throw an error about -cpu qemu64

Signed-off-by: Cole Robinson 
---
src/qemu/qemu_command.c | 25 ++---
1 file changed, 18 insertions(+), 7 deletions(-)



ACK

Jan


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

Re: [libvirt] [PATCH] libvirt-domain.h: Fix enum description placement

[Daniel P. Berrange]

On Fri, Jul 21, 2017 at 03:00:20PM +0200, Martin Kletzander wrote:
> On Fri, Jul 21, 2017 at 11:58:45AM +0100, Daniel P. Berrange wrote:
> > On Fri, Jul 21, 2017 at 12:52:06PM +0200, Michal Privoznik wrote:
> > > There are only two acceptable places for describing enum values.
> > > It's either:
> > > 
> > > typedef enum {
> > > /* Some long description. Therefore it's placed before
> > >  * the value. */
> > > VIR_ENUM_A_VAL = 1,
> > > } virEnumA;
> > > 
> > > or:
> > > 
> > > typedef enum {
> > > VIR_ENUM_B_VAL = 1, /* Some short description */
> > > } virEnumB;
> > > 
> > > However, during review of a patch sent upstream I realized that
> > > is not always the case. I went through all the public header
> > > files and identified all the offenders. Luckily there were just
> > > two of them.
> > > 
> > > Yes, this makes our HTML generated documentation broken, but
> > > that's bug of the generator. Our header files shouldn't be forced
> > > to use something we don't want to.
> > 
> > FWIW, the problem is in the parseEnumBLock() method in apibuild.py
> > 
> > Once it has completed parsing an enum item, it delays adding that
> > enum to the list until it sees the next item, so that it can capture
> > the trailing comment.
> > 
> > The only way we can distinguish between a comment that comes before
> > the enum vs a comment after the enum, on the same line, is by detecting
> > whitespace (newline) before the trailing comment. Unfortunately I don't
> > thing this is exposed by the lexer right, so its not entirely easy
> > to solve.
> > 
> 
> I was under the impression that this worked, we only broke it by some
> recent commit.  I looked at the code and got pretty confused by it, but
> shouldn't it be pretty easy (from a big picture view)?  You read until
> you have both comment and a member of the struct.
> 
> If it's really harder than I think, then we can start using some helper
> characters for the comments (at least for now) so that we can properly
> identify them, e.g.:
> 
> struct meh {
>   /*# This is comment for the following member foo */
>   unsigned int foo;
>   int bar; /*< This is for member bar that's on the same line */
> }
> 
> and so on.  If that doesn't help either and it never worked,
> then... it's a pity :-/

That is ambiguous - without seeing whitespace, the parser cannot
distinguish between these two scenarios:

 struct meh {
   unsigned int foo; /*# This is comment for the following member foo */
   int bar;
 }

 struct meh {
   unsigned int foo;
   
/*# This is comment for the following member foo */
   int bar;
 }




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

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

[libvirt] [PATCH v2 7/7] qemu: privatize _virQEMUCapsCachePriv struct

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina 
---

Notes:
Changes in v2:
- the body of structure was changed

 src/qemu/qemu_capabilities.c | 10 ++
 src/qemu/qemu_capspriv.h | 12 
 2 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 9f327bccb4..3e6b503de5 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -3766,6 +3766,16 @@ virQEMUCapsLoadCPUModels(virQEMUCapsPtr qemuCaps,
 }
 
 
+struct _virQEMUCapsCachePriv {
+char *libDir;
+uid_t runUid;
+gid_t runGid;
+virArch hostArch;
+};
+typedef struct _virQEMUCapsCachePriv virQEMUCapsCachePriv;
+typedef virQEMUCapsCachePriv *virQEMUCapsCachePrivPtr;
+
+
 static void
 virQEMUCapsCachePrivFree(void *privData)
 {
diff --git a/src/qemu/qemu_capspriv.h b/src/qemu/qemu_capspriv.h
index 78f7e4ab39..d05256bd35 100644
--- a/src/qemu/qemu_capspriv.h
+++ b/src/qemu/qemu_capspriv.h
@@ -28,18 +28,6 @@
 #ifndef __QEMU_CAPSPRIV_H__
 # define __QEMU_CAPSPRIV_H__
 
-# include "virarch.h"
-
-struct _virQEMUCapsCachePriv {
-char *libDir;
-uid_t runUid;
-gid_t runGid;
-virArch hostArch;
-};
-typedef struct _virQEMUCapsCachePriv virQEMUCapsCachePriv;
-typedef virQEMUCapsCachePriv *virQEMUCapsCachePrivPtr;
-
-
 virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps);
 
 virQEMUCapsPtr
-- 
2.13.3

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

[libvirt] [PATCH v2 0/7] introduce virFileCache and use it for qemu capabilities

[Pavel Hrdina]

Pavel Hrdina (7):
  util: introduce virFileCache
  tests: add virfilecachetest
  qemu: introduce struct _virQEMUCapsCachePriv
  tests: rewrite host CPU mocking
  qemu: pass only host arch instead of the whole virCaps
  qemu: switch QEMU capabilities to use virFileCache
  qemu: privatize _virQEMUCapsCachePriv struct

 po/POTFILES.in |   1 +
 src/Makefile.am|   1 +
 src/libvirt_private.syms   |   9 +
 src/qemu/qemu_capabilities.c   | 372 ++---
 src/qemu/qemu_capabilities.h   |  20 +-
 src/qemu/qemu_capspriv.h   |  17 +-
 src/qemu/qemu_conf.h   |   3 +-
 src/qemu/qemu_domain.c |  17 +-
 src/qemu/qemu_driver.c |  10 +-
 src/qemu/qemu_process.c|   9 +-
 src/util/virfilecache.c| 439 +
 src/util/virfilecache.h| 137 +++
 tests/Makefile.am  |  12 +
 tests/domaincapstest.c |  55 +--
 tests/qemucapsprobe.c  |   3 +-
 tests/qemucpumock.c|  14 +-
 tests/qemuxml2argvtest.c   |   9 +-
 tests/testutilshostcpus.h  | 148 +++
 tests/testutilsqemu.c  | 107 +
 tests/testutilsqemu.h  |   3 +-
 tests/testutilsxen.c   |  29 +-
 ...a15b1658aa16923e46497dd8deeb6be287ddb0ca0.cache |   1 +
 ...ae4bc3a28e75bc3e262757001e8b953580f5e75ef.cache |   1 +
 tests/virfilecachemock.c   |  31 ++
 tests/virfilecachetest.c   | 238 +++
 25 files changed, 1200 insertions(+), 486 deletions(-)
 create mode 100644 src/util/virfilecache.c
 create mode 100644 src/util/virfilecache.h
 create mode 100644 tests/testutilshostcpus.h
 create mode 100644 
tests/virfilecachedata/5f3154560c130108b282a2aa15b1658aa16923e46497dd8deeb6be287ddb0ca0.cache
 create mode 12 
tests/virfilecachedata/9ca150bf3119b75dcac8e8bae4bc3a28e75bc3e262757001e8b953580f5e75ef.cache
 create mode 100644 tests/virfilecachemock.c
 create mode 100644 tests/virfilecachetest.c

-- 
2.13.3

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

[libvirt] [PATCH v2 3/7] qemu: introduce struct _virQEMUCapsCachePriv

[Pavel Hrdina]

This will store private data that will be used by following patches
when switching to virFileCache.

Signed-off-by: Pavel Hrdina 
Reviewed-by: Jiri Denemark 
---
 src/qemu/qemu_capabilities.c | 51 ++--
 src/qemu/qemu_capspriv.h | 12 ---
 2 files changed, 39 insertions(+), 24 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index b7351322bc..c310e97f61 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -3764,6 +3764,14 @@ virQEMUCapsLoadCPUModels(virQEMUCapsPtr qemuCaps,
 }
 
 
+static void
+virQEMUCapsCachePrivFree(virQEMUCapsCachePrivPtr priv)
+{
+VIR_FREE(priv->libDir);
+VIR_FREE(priv);
+}
+
+
 /*
  * Parsing a doc that looks like
  *
@@ -4252,8 +4260,7 @@ virQEMUCapsRememberCached(virQEMUCapsPtr qemuCaps, const 
char *cacheDir)
 
 static bool
 virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps,
-   uid_t runUid,
-   gid_t runGid)
+   virQEMUCapsCachePrivPtr priv)
 {
 bool kvmUsable;
 struct stat sb;
@@ -4290,7 +4297,7 @@ virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps,
 }
 
 kvmUsable = virFileAccessibleAs("/dev/kvm", R_OK | W_OK,
-runUid, runGid) == 0;
+priv->runUid, priv->runGid) == 0;
 
 if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) &&
 virQEMUCapsGet(qemuCaps, QEMU_CAPS_ENABLE_KVM) &&
@@ -4318,8 +4325,7 @@ virQEMUCapsInitCached(virCapsPtr caps,
   virQEMUCapsPtr *qemuCaps,
   const char *binary,
   const char *cacheDir,
-  uid_t runUid,
-  gid_t runGid)
+  virQEMUCapsCachePrivPtr priv)
 {
 char *capsdir = NULL;
 char *capsfile = NULL;
@@ -4370,7 +4376,7 @@ virQEMUCapsInitCached(virCapsPtr caps,
 goto discard;
 }
 
-if (!virQEMUCapsIsValid(qemuCapsNew, runUid, runGid))
+if (!virQEMUCapsIsValid(qemuCapsNew, priv))
 goto discard;
 
 VIR_DEBUG("Loaded '%s' for '%s' ctime %lld usedQMP=%d",
@@ -5275,22 +5281,21 @@ virQEMUCapsNewForBinaryInternal(virCapsPtr caps,
 static virQEMUCapsPtr
 virQEMUCapsNewForBinary(virCapsPtr caps,
 const char *binary,
-const char *libDir,
 const char *cacheDir,
-uid_t runUid,
-gid_t runGid)
+virQEMUCapsCachePrivPtr priv)
 {
 int rv;
 virQEMUCapsPtr qemuCaps = NULL;
 
-if ((rv = virQEMUCapsInitCached(caps, &qemuCaps, binary, cacheDir,
-runUid, runGid)) < 0)
+if ((rv = virQEMUCapsInitCached(caps, &qemuCaps, binary, cacheDir, priv)) 
< 0)
 goto error;
 
 if (rv == 0) {
 if (!(qemuCaps = virQEMUCapsNewForBinaryInternal(caps, binary,
- libDir, runUid,
- runGid, false))) {
+ priv->libDir,
+ priv->runUid,
+ priv->runGid,
+ false))) {
 goto error;
 }
 
@@ -5364,13 +5369,17 @@ virQEMUCapsCacheNew(const char *libDir,
 
 if (!(cache->binaries = virHashCreate(10, virObjectFreeHashData)))
 goto error;
-if (VIR_STRDUP(cache->libDir, libDir) < 0)
-goto error;
 if (VIR_STRDUP(cache->cacheDir, cacheDir) < 0)
 goto error;
 
-cache->runUid = runUid;
-cache->runGid = runGid;
+if (VIR_ALLOC(cache->priv) < 0)
+goto error;
+
+if (VIR_STRDUP(cache->priv->libDir, libDir) < 0)
+goto error;
+
+cache->priv->runUid = runUid;
+cache->priv->runGid = runGid;
 
 return cache;
 
@@ -5387,7 +5396,7 @@ virQEMUCapsCacheValidate(virQEMUCapsCachePtr cache,
  virQEMUCapsPtr *qemuCaps)
 {
 if (*qemuCaps &&
-!virQEMUCapsIsValid(*qemuCaps, cache->runUid, cache->runGid)) {
+!virQEMUCapsIsValid(*qemuCaps, cache->priv)) {
 VIR_DEBUG("Cached capabilities %p no longer valid for %s",
   *qemuCaps, binary);
 virHashRemoveEntry(cache->binaries, binary);
@@ -5397,8 +5406,8 @@ virQEMUCapsCacheValidate(virQEMUCapsCachePtr cache,
 if (!*qemuCaps) {
 VIR_DEBUG("Creating capabilities for %s", binary);
 *qemuCaps = virQEMUCapsNewForBinary(caps, binary,
-cache->libDir, cache->cacheDir,
-cache->runUid, cache->runGid);
+cache->cacheDir,
+cache->priv);
 if (*qemuCaps) {
 VIR_DEBUG("Caching capabilit

[libvirt] [PATCH v2 5/7] qemu: pass only host arch instead of the whole virCaps

[Pavel Hrdina]

This is a preparation for following patches where we switch to
virFileCache for QEMU capabilities cache

The host arch will always remain the same but virCaps may change.  Now
the host arch is stored while creating new qemu capabilities cache.
It removes the need to pass virCaps into virQEMUCapsCache*() functions.

Signed-off-by: Pavel Hrdina 
---

Notes:
New in v2

 src/qemu/qemu_capabilities.c | 61 +---
 src/qemu/qemu_capabilities.h |  9 +++
 src/qemu/qemu_capspriv.h | 11 +---
 src/qemu/qemu_domain.c   | 17 +---
 src/qemu/qemu_driver.c   |  8 +++---
 src/qemu/qemu_process.c  |  9 +++
 tests/qemucapsprobe.c|  3 ++-
 tests/qemucpumock.c  |  3 ++-
 tests/qemuxml2argvtest.c |  9 +--
 tests/testutilsqemu.c|  5 +++-
 10 files changed, 67 insertions(+), 68 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index c310e97f61..603ce18ac3 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -972,7 +972,7 @@ virQEMUCapsInitGuest(virCapsPtr caps,
 
 /* Ignore binary if extracting version info fails */
 if (binary) {
-if (!(qemubinCaps = virQEMUCapsCacheLookup(caps, cache, binary))) {
+if (!(qemubinCaps = virQEMUCapsCacheLookup(cache, binary))) {
 virResetLastError();
 VIR_FREE(binary);
 }
@@ -1012,7 +1012,7 @@ virQEMUCapsInitGuest(virCapsPtr caps,
 if (!kvmbin)
 continue;
 
-if (!(kvmbinCaps = virQEMUCapsCacheLookup(caps, cache, kvmbin))) {
+if (!(kvmbinCaps = virQEMUCapsCacheLookup(cache, kvmbin))) {
 virResetLastError();
 VIR_FREE(kvmbin);
 continue;
@@ -1152,7 +1152,7 @@ virQEMUCapsInitGuestFromBinary(virCapsPtr caps,
 
 
 virCPUDefPtr
-virQEMUCapsProbeHostCPUForEmulator(virCapsPtr caps,
+virQEMUCapsProbeHostCPUForEmulator(virArch hostArch,
virQEMUCapsPtr qemuCaps,
virDomainVirtType type)
 {
@@ -1163,7 +1163,7 @@ virQEMUCapsProbeHostCPUForEmulator(virCapsPtr caps,
 if (virQEMUCapsGetCPUDefinitions(qemuCaps, type, &models, &nmodels) < 0)
 return NULL;
 
-cpu = virCPUGetHost(caps->host.arch, VIR_CPU_TYPE_GUEST, NULL,
+cpu = virCPUGetHost(hostArch, VIR_CPU_TYPE_GUEST, NULL,
 (const char **) models, nmodels);
 
 virStringListFreeCount(models, nmodels);
@@ -2182,7 +2182,7 @@ int virQEMUCapsGetDefaultVersion(virCapsPtr caps,
 return -1;
 }
 
-qemucaps = virQEMUCapsCacheLookup(caps, capsCache, capsdata->emulator);
+qemucaps = virQEMUCapsCacheLookup(capsCache, capsdata->emulator);
 VIR_FREE(capsdata);
 if (!qemucaps)
 return -1;
@@ -3484,7 +3484,7 @@ virQEMUCapsNewHostCPUModel(void)
 
 void
 virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps,
-virCapsPtr caps,
+virArch hostArch,
 virDomainVirtType type)
 {
 virCPUDefPtr cpu = NULL;
@@ -3494,7 +3494,7 @@ virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps,
 size_t i;
 int rc;
 
-if (!caps || !virQEMUCapsGuestIsNative(caps->host.arch, qemuCaps->arch))
+if (!virQEMUCapsGuestIsNative(hostArch, qemuCaps->arch))
 return;
 
 if (!(cpu = virQEMUCapsNewHostCPUModel()))
@@ -3505,7 +3505,7 @@ virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps,
 } else if (rc == 1) {
 VIR_DEBUG("No host CPU model info from QEMU; probing host CPU 
directly");
 
-hostCPU = virQEMUCapsProbeHostCPUForEmulator(caps, qemuCaps, type);
+hostCPU = virQEMUCapsProbeHostCPUForEmulator(hostArch, qemuCaps, type);
 if (!hostCPU ||
 virCPUDefCopyModelFilter(cpu, hostCPU, true,
  virQEMUCapsCPUFilterFeatures,
@@ -3790,7 +3790,7 @@ virQEMUCapsCachePrivFree(virQEMUCapsCachePrivPtr priv)
  * 
  */
 int
-virQEMUCapsLoadCache(virCapsPtr caps,
+virQEMUCapsLoadCache(virArch hostArch,
  virQEMUCapsPtr qemuCaps,
  const char *filename)
 {
@@ -4007,8 +4007,8 @@ virQEMUCapsLoadCache(virCapsPtr caps,
 }
 VIR_FREE(nodes);
 
-virQEMUCapsInitHostCPUModel(qemuCaps, caps, VIR_DOMAIN_VIRT_KVM);
-virQEMUCapsInitHostCPUModel(qemuCaps, caps, VIR_DOMAIN_VIRT_QEMU);
+virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_KVM);
+virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_QEMU);
 
 ret = 0;
  cleanup:
@@ -4321,8 +4321,7 @@ virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps,
 
 
 static int
-virQEMUCapsInitCached(virCapsPtr caps,
-  virQEMUCapsPtr *qemuCaps,
+virQEMUCapsInitCached(virQEMUCapsPtr *qemuCaps,
   const char *binary,
   const char *cacheDir,
   virQEMUCapsCachePrivPtr priv)
@@ -4369,7 +43

[libvirt] [PATCH v2 6/7] qemu: switch QEMU capabilities to use virFileCache

[Pavel Hrdina]

The switch contains considerable amount of changes:

  virQEMUCapsRememberCached() is removed because this is now handled
  by virFileCacheSave().

  virQEMUCapsInitCached() is removed because this is now handled by
  virFileCacheLoad().

  virQEMUCapsNewForBinary() is split into two functions,
  virQEMUCapsNewData() which creates new data if there is nothing
  cached and virQEMUCapsLoadFile() which loads the cached data.
  This is now handled by virFileCacheNewData().

  virQEMUCapsCacheValidate() is removed because this is now handled by
  virFileCacheValidate().

  virQEMUCapsCacheFree() is removed because it's no longer required.

  Add virCapsPtr into virQEMUCapsCachePriv because for each call of
  virFileCacheLookup*() we need to use current virCapsPtr.

Signed-off-by: Pavel Hrdina 
---

Notes:
Changes in v2:
- removed the need to set/unset virCaps before calling 
virFileCacheLookup*

 src/qemu/qemu_capabilities.c | 316 +++
 src/qemu/qemu_capabilities.h |  17 +--
 src/qemu/qemu_capspriv.h |   6 -
 src/qemu/qemu_conf.h |   3 +-
 src/qemu/qemu_driver.c   |   2 +-
 tests/testutilsqemu.c|   9 +-
 tests/testutilsqemu.h|   3 +-
 7 files changed, 96 insertions(+), 260 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 603ce18ac3..9f327bccb4 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -29,6 +29,7 @@
 #include "virlog.h"
 #include "virerror.h"
 #include "virfile.h"
+#include "virfilecache.h"
 #include "virpidfile.h"
 #include "virprocess.h"
 #include "cpu/cpu.h"
@@ -954,7 +955,7 @@ virQEMUCapsFindBinaryForArch(virArch hostarch,
 
 static int
 virQEMUCapsInitGuest(virCapsPtr caps,
- virQEMUCapsCachePtr cache,
+ virFileCachePtr cache,
  virArch hostarch,
  virArch guestarch)
 {
@@ -1171,7 +1172,8 @@ virQEMUCapsProbeHostCPUForEmulator(virArch hostArch,
 }
 
 
-virCapsPtr virQEMUCapsInit(virQEMUCapsCachePtr cache)
+virCapsPtr
+virQEMUCapsInit(virFileCachePtr cache)
 {
 virCapsPtr caps;
 size_t i;
@@ -2162,7 +2164,7 @@ virQEMUCapsExtractDeviceStr(const char *qemu,
 
 
 int virQEMUCapsGetDefaultVersion(virCapsPtr caps,
- virQEMUCapsCachePtr capsCache,
+ virFileCachePtr capsCache,
  unsigned int *version)
 {
 virQEMUCapsPtr qemucaps;
@@ -3765,8 +3767,10 @@ virQEMUCapsLoadCPUModels(virQEMUCapsPtr qemuCaps,
 
 
 static void
-virQEMUCapsCachePrivFree(virQEMUCapsCachePrivPtr priv)
+virQEMUCapsCachePrivFree(void *privData)
 {
+virQEMUCapsCachePrivPtr priv = privData;
+
 VIR_FREE(priv->libDir);
 VIR_FREE(priv);
 }
@@ -4195,8 +4199,11 @@ virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps)
 
 
 static int
-virQEMUCapsSaveCache(virQEMUCapsPtr qemuCaps, const char *filename)
+virQEMUCapsSaveFile(void *data,
+const char *filename,
+void *privData ATTRIBUTE_UNUSED)
 {
+virQEMUCapsPtr qemuCaps = data;
 char *xml = NULL;
 int ret = -1;
 
@@ -4220,48 +4227,13 @@ virQEMUCapsSaveCache(virQEMUCapsPtr qemuCaps, const 
char *filename)
 return ret;
 }
 
-static int
-virQEMUCapsRememberCached(virQEMUCapsPtr qemuCaps, const char *cacheDir)
-{
-char *capsdir = NULL;
-char *capsfile = NULL;
-int ret = -1;
-char *binaryhash = NULL;
-
-if (virAsprintf(&capsdir, "%s/capabilities", cacheDir) < 0)
-goto cleanup;
-
-if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256,
-qemuCaps->binary,
-&binaryhash) < 0)
-goto cleanup;
-
-if (virAsprintf(&capsfile, "%s/%s.xml", capsdir, binaryhash) < 0)
-goto cleanup;
-
-if (virFileMakePath(capsdir) < 0) {
-virReportSystemError(errno,
- _("Unable to create directory '%s'"),
- capsdir);
-goto cleanup;
-}
-
-if (virQEMUCapsSaveCache(qemuCaps, capsfile) < 0)
-goto cleanup;
-
-ret = 0;
- cleanup:
-VIR_FREE(binaryhash);
-VIR_FREE(capsfile);
-VIR_FREE(capsdir);
-return ret;
-}
-
 
 static bool
-virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps,
-   virQEMUCapsCachePrivPtr priv)
+virQEMUCapsIsValid(void *data,
+   void *privData)
 {
+virQEMUCapsPtr qemuCaps = data;
+virQEMUCapsCachePrivPtr priv = privData;
 bool kvmUsable;
 struct stat sb;
 
@@ -4320,86 +4292,6 @@ virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps,
 }
 
 
-static int
-virQEMUCapsInitCached(virQEMUCapsPtr *qemuCaps,
-  const char *binary,
-  const char *cacheDir,
-  virQEMUCapsCachePrivPtr priv)
-{
-char *capsdir = NULL;
-char *capsfile = NULL;
-int ret = -1;
-char *binaryhash = NULL;
-struct stat sb;
-virQEMUCapsPtr qemu

Re: [libvirt] [PATCH v3 3/3] qemu: Enable NUMA node tag in pci-root for PPC64

[Andrea Bolognani]

On Fri, 2017-07-21 at 13:33 +0530, Shivaprasad G Bhat wrote:
[...]
> +++ b/docs/formatdomain.html.in
> @@ -3778,7 +3778,9 @@
>
>node
>
> -pci-expander-bus controllers can have an
> +Some PCI controllers (pci-expander-bus for the pc machine
> +type, pcie-expander-bus for the q35 machine type and
> +pci-root for the pseries machine type) can have an

Controller names could have been wrapped in  tags
for nicer formatting. A "Since" notice would also have
been nice.

[...]
> +++ b/src/conf/domain_conf.c
> @@ -9457,8 +9457,15 @@ virDomainControllerDefParseXML(xmlNodePtr node,
>  goto error;
>  }
>  }
> -if (numaNode >= 0)
> +if (numaNode >= 0) {
>  def->opts.pciopts.numaNode = numaNode;
> +if (def->idx == 0) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> +   _("The PCI controller with index=0 can't "
> + "be associated with a NUMA node."));
> +goto error;
> +}
> +}

The check should be *before* setting the value. Not that
it matters from a functional point of view, since you're
going to jump either way, but it looks nicer ;)

[...]
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-phb-numa-node.xml
> @@ -0,0 +1,41 @@
> +
> +  QEMUGuest1
> +  87eedafe-eedc-4336-8130-ed9fe5dc90c8
> +  2097152
> +  8

The number of vCPUs here...

> +  
> +
> +
> +  
> +  
> +

... doesn't match the topology here.

The test case won't fail because you don't enable the
QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS capability, but it's
confusing to have it there.

I'll fix up these small issues, add

  Reviewed-by: Andrea Bolognani 

and push.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[libvirt] [PATCH v2 4/7] tests: rewrite host CPU mocking

[Pavel Hrdina]

Move all the host CPU data into a separate file and rewrite qemucpumock
to not use passed @caps.  This is preparation for following patch which
will replace virCaps argument with virArch.

Signed-off-by: Pavel Hrdina 
---

Notes:
New in v2

 tests/domaincapstest.c|  55 +
 tests/qemucpumock.c   |  13 ++--
 tests/testutilshostcpus.h | 148 ++
 tests/testutilsqemu.c |  93 +++--
 tests/testutilsxen.c  |  29 +
 5 files changed, 167 insertions(+), 171 deletions(-)
 create mode 100644 tests/testutilshostcpus.h

diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c
index 5a36fcf29d..e17aa8ade3 100644
--- a/tests/domaincapstest.c
+++ b/tests/domaincapstest.c
@@ -117,38 +117,7 @@ fillAllCaps(virDomainCapsPtr domCaps)
 
 #if WITH_QEMU
 # include "testutilsqemu.h"
-
-static virCPUDef aarch64Cpu = {
-.sockets = 1,
-.cores = 1,
-.threads = 1,
-};
-
-static virCPUDef ppc64leCpu = {
-.type = VIR_CPU_TYPE_HOST,
-.arch = VIR_ARCH_PPC64LE,
-.model = (char *) "POWER8",
-.sockets = 1,
-.cores = 1,
-.threads = 1,
-};
-
-static virCPUDef x86Cpu = {
-.type = VIR_CPU_TYPE_HOST,
-.arch = VIR_ARCH_X86_64,
-.model = (char *) "Broadwell",
-.sockets = 1,
-.cores = 1,
-.threads = 1,
-};
-
-static virCPUDef s390Cpu = {
-.type = VIR_CPU_TYPE_HOST,
-.arch = VIR_ARCH_S390X,
-.sockets = 1,
-.cores = 1,
-.threads = 1,
-};
+# include "testutilshostcpus.h"
 
 static int
 fakeHostCPU(virCapsPtr caps,
@@ -156,32 +125,14 @@ fakeHostCPU(virCapsPtr caps,
 {
 virCPUDefPtr cpu;
 
-switch (arch) {
-case VIR_ARCH_AARCH64:
-cpu = &aarch64Cpu;
-break;
-
-case VIR_ARCH_PPC64LE:
-cpu = &ppc64leCpu;
-break;
-
-case VIR_ARCH_X86_64:
-cpu = &x86Cpu;
-break;
-
-case VIR_ARCH_S390X:
-cpu = &s390Cpu;
-break;
-
-default:
+if (!(cpu = testUtilsHostCpusGetDefForArch(arch))) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
"cannot fake host CPU for arch %s",
virArchToString(arch));
 return -1;
 }
 
-if (!(caps->host.cpu = virCPUDefCopy(cpu)))
-return -1;
+qemuTestSetHostCPU(caps, cpu);
 
 return 0;
 }
diff --git a/tests/qemucpumock.c b/tests/qemucpumock.c
index f0a113ffce..38827584ea 100644
--- a/tests/qemucpumock.c
+++ b/tests/qemucpumock.c
@@ -16,20 +16,23 @@
 
 #include 
 
-#include "internal.h"
+#include 
+
+#include "conf/cpu_conf.h"
+#include "cpu/cpu.h"
 #include "qemu/qemu_capabilities.h"
 #define __QEMU_CAPSPRIV_H_ALLOW__
 #include "qemu/qemu_capspriv.h"
 #undef __QEMU_CAPSPRIV_H_ALLOW__
+#include "testutilshostcpus.h"
 
 
 virCPUDefPtr
-virQEMUCapsProbeHostCPUForEmulator(virCapsPtr caps,
+virQEMUCapsProbeHostCPUForEmulator(virCapsPtr caps ATTRIBUTE_UNUSED,
virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED,
virDomainVirtType type ATTRIBUTE_UNUSED)
 {
-if (!caps || !caps->host.cpu || !caps->host.cpu->model)
-return NULL;
+const char *model = getenv("VIR_TEST_MOCK_FAKE_HOST_CPU");
 
-return virCPUDefCopy(caps->host.cpu);
+return testUtilsHostCpusGetDefForModel(model);
 }
diff --git a/tests/testutilshostcpus.h b/tests/testutilshostcpus.h
new file mode 100644
index 00..f0ab23b962
--- /dev/null
+++ b/tests/testutilshostcpus.h
@@ -0,0 +1,148 @@
+#include "conf/cpu_conf.h"
+#include "internal.h"
+#include "util/virarch.h"
+
+static virCPUFeatureDef cpuDefaultFeatures[] = {
+{ (char *) "ds",-1 },
+{ (char *) "acpi",  -1 },
+{ (char *) "ss",-1 },
+{ (char *) "ht",-1 },
+{ (char *) "tm",-1 },
+{ (char *) "pbe",   -1 },
+{ (char *) "ds_cpl",-1 },
+{ (char *) "vmx",   -1 },
+{ (char *) "est",   -1 },
+{ (char *) "tm2",   -1 },
+{ (char *) "cx16",  -1 },
+{ (char *) "xtpr",  -1 },
+{ (char *) "lahf_lm",   -1 },
+};
+static virCPUDef cpuDefaultData = {
+.type = VIR_CPU_TYPE_HOST,
+.arch = VIR_ARCH_X86_64,
+.model = (char *) "core2duo",
+.vendor = (char *) "Intel",
+.sockets = 1,
+.cores = 2,
+.threads = 1,
+.nfeatures = ARRAY_CARDINALITY(cpuDefaultFeatures),
+.nfeatures_max = ARRAY_CARDINALITY(cpuDefaultFeatures),
+.features = cpuDefaultFeatures,
+};
+
+static virCPUFeatureDef cpuHaswellFeatures[] = {
+{ (char *) "vme",   -1 },
+{ (char *) "ds",-1 },
+{ (char *) "acpi",  -1 },
+{ (char *) "ss",-1 },
+{ (char *) "ht",-1 },
+{ (char *) "tm",-1 },
+{ (char *) "pbe",   -1 },
+{ (char *) "dtes64",-1 },
+{ (char *) "monitor",   -1 },
+{ (char *) "ds_cpl",-1 },
+{ (char *) "vmx",   -1 },
+{ (char *) "smx",   -1 },
+{ (char *) "est",   -1 },
+{ (ch

[libvirt] [PATCH v2 2/7] tests: add virfilecachetest

[Pavel Hrdina]

Implements 3 test cases that covers how the cache is used.

We have to mock unlink() function because the caching code unlinks
files that are no longer valid and we don't want to do it in our tests.

Signed-off-by: Pavel Hrdina 
---

Notes:
Changes in v2:
- added tests/virfilecachemock.c
- specify suffix for virFileCacheNew()

 tests/Makefile.am  |  12 ++
 ...a15b1658aa16923e46497dd8deeb6be287ddb0ca0.cache |   1 +
 ...ae4bc3a28e75bc3e262757001e8b953580f5e75ef.cache |   1 +
 tests/virfilecachemock.c   |  31 +++
 tests/virfilecachetest.c   | 238 +
 5 files changed, 283 insertions(+)
 create mode 100644 
tests/virfilecachedata/5f3154560c130108b282a2aa15b1658aa16923e46497dd8deeb6be287ddb0ca0.cache
 create mode 12 
tests/virfilecachedata/9ca150bf3119b75dcac8e8bae4bc3a28e75bc3e262757001e8b953580f5e75ef.cache
 create mode 100644 tests/virfilecachemock.c
 create mode 100644 tests/virfilecachetest.c

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 5be81d2213..562c1c7717 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -182,6 +182,7 @@ test_programs = virshtest sockettest \
virpcitest \
virendiantest \
virfiletest \
+   virfilecachetest \
virfirewalltest \
viriscsitest \
virkeycodetest \
@@ -211,6 +212,7 @@ test_libraries = libshunload.la \
virrandommock.la \
virhostcpumock.la \
domaincapsmock.la \
+   virfilecachemock.la \
$(NULL)
 
 if WITH_REMOTE
@@ -1137,6 +1139,12 @@ virhostcpumock_la_CFLAGS = $(AM_CFLAGS)
 virhostcpumock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
 virhostcpumock_la_LIBADD = $(MOCKLIBS_LIBS)
 
+virfilecachemock_la_SOURCES = \
+   virfilecachemock.c
+virfilecachemock_la_CFLAGS = $(AM_CFLAGS)
+virfilecachemock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
+virfilecachemock_la_LIBADD = $(MOCKLIBS_LIBS)
+
 if WITH_LINUX
 vircaps2xmltest_SOURCES = \
vircaps2xmltest.c testutils.h testutils.c virfilewrapper.h 
virfilewrapper.c
@@ -1369,6 +1377,10 @@ virfiletest_SOURCES = \
virfiletest.c testutils.h testutils.c
 virfiletest_LDADD = $(LDADDS)
 
+virfilecachetest_SOURCES = \
+   virfilecachetest.c testutils.h testutils.c
+virfilecachetest_LDADD = $(LDADDS)
+
 virfirewalltest_SOURCES = \
virfirewalltest.c testutils.h testutils.c
 virfirewalltest_LDADD = $(LDADDS) $(DBUS_LIBS)
diff --git 
a/tests/virfilecachedata/5f3154560c130108b282a2aa15b1658aa16923e46497dd8deeb6be287ddb0ca0.cache
 
b/tests/virfilecachedata/5f3154560c130108b282a2aa15b1658aa16923e46497dd8deeb6be287ddb0ca0.cache
new file mode 100644
index 00..72943a16fb
--- /dev/null
+++ 
b/tests/virfilecachedata/5f3154560c130108b282a2aa15b1658aa16923e46497dd8deeb6be287ddb0ca0.cache
@@ -0,0 +1 @@
+aaa
diff --git 
a/tests/virfilecachedata/9ca150bf3119b75dcac8e8bae4bc3a28e75bc3e262757001e8b953580f5e75ef.cache
 
b/tests/virfilecachedata/9ca150bf3119b75dcac8e8bae4bc3a28e75bc3e262757001e8b953580f5e75ef.cache
new file mode 12
index 00..94df26de63
--- /dev/null
+++ 
b/tests/virfilecachedata/9ca150bf3119b75dcac8e8bae4bc3a28e75bc3e262757001e8b953580f5e75ef.cache
@@ -0,0 +1 @@
+5f3154560c130108b282a2aa15b1658aa16923e46497dd8deeb6be287ddb0ca0.cache
\ No newline at end of file
diff --git a/tests/virfilecachemock.c b/tests/virfilecachemock.c
new file mode 100644
index 00..a7476bce2e
--- /dev/null
+++ b/tests/virfilecachemock.c
@@ -0,0 +1,31 @@
+/*
+ * Copyright (C) 2017 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ *
+ */
+
+#include 
+
+#include 
+
+#include "internal.h"
+
+
+int
+unlink(const char *path ATTRIBUTE_UNUSED)
+{
+return 0;
+}
diff --git a/tests/virfilecachetest.c b/tests/virfilecachetest.c
new file mode 100644
index 00..1a6ff62109
--- /dev/null
+++ b/tests/virfilecachetest.c
@@ -0,0 +1,238 @@
+/*
+ * Copyright (C) 2017 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ 

[libvirt] [PATCH v2 1/7] util: introduce virFileCache

[Pavel Hrdina]

The new virFileCache will nicely handle the caching logic for any data
that we would like to cache.  For each type of data we will just need
to implement few handlers that will take care of creating, validating,
loading and saving the cached data.

The cached data must be an instance of virObject.

Currently we cache QEMU capabilities which will start using
virFileCache.

Signed-off-by: Pavel Hrdina 
---

Notes:
Changes in v2:
- added suffix argument into virFileCacheNew()
- fixed locking for virFileCache APIs
- moved virObjectUnref() into virFileCacheLoad()

 po/POTFILES.in   |   1 +
 src/Makefile.am  |   1 +
 src/libvirt_private.syms |   9 +
 src/util/virfilecache.c  | 439 +++
 src/util/virfilecache.h  | 137 +++
 5 files changed, 587 insertions(+)
 create mode 100644 src/util/virfilecache.c
 create mode 100644 src/util/virfilecache.h

diff --git a/po/POTFILES.in b/po/POTFILES.in
index b3891b5877..c1fa23427e 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -211,6 +211,7 @@ src/util/vireventpoll.c
 src/util/virfcp.c
 src/util/virfdstream.c
 src/util/virfile.c
+src/util/virfilecache.c
 src/util/virfirewall.c
 src/util/virfirmware.c
 src/util/virhash.c
diff --git a/src/Makefile.am b/src/Makefile.am
index e637dfd910..d86b282519 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -194,6 +194,7 @@ UTIL_SOURCES =  
\
util/virxdrdefs.h   \
util/virxml.c util/virxml.h \
util/virmdev.c util/virmdev.h   \
+   util/virfilecache.c util/virfilecache.h \
$(NULL)
 
 EXTRA_DIST += \
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a35ad0f859..28dad7149b 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1711,6 +1711,15 @@ virFileWriteStr;
 virFindFileInPath;
 
 
+# util/virfilecache.h
+virFileCacheGetPriv;
+virFileCacheInsertData;
+virFileCacheLookup;
+virFileCacheLookupByFunc;
+virFileCacheNew;
+virFileCacheSetPriv;
+
+
 # util/virfirewall.h
 virFirewallAddRuleFull;
 virFirewallApply;
diff --git a/src/util/virfilecache.c b/src/util/virfilecache.c
new file mode 100644
index 00..0401253146
--- /dev/null
+++ b/src/util/virfilecache.c
@@ -0,0 +1,439 @@
+/*
+ * virfilecache.c: file caching for data
+ *
+ * Copyright (C) 2017 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ *
+ */
+
+
+#include 
+
+#include "internal.h"
+
+#include "viralloc.h"
+#include "virbuffer.h"
+#include "vircrypto.h"
+#include "virerror.h"
+#include "virfile.h"
+#include "virfilecache.h"
+#include "virhash.h"
+#include "virlog.h"
+#include "virobject.h"
+#include "virstring.h"
+
+#include 
+#include 
+#include 
+
+#define VIR_FROM_THIS VIR_FROM_NONE
+
+VIR_LOG_INIT("util.filecache")
+
+
+struct _virFileCache {
+virObjectLockable object;
+
+virHashTablePtr table;
+
+char *dir;
+char *suffix;
+
+void *priv;
+
+virFileCacheHandlers handlers;
+};
+
+
+static virClassPtr virFileCacheClass;
+
+
+static void
+virFileCachePrivFree(virFileCachePtr cache)
+{
+if (cache->priv && cache->handlers.privFree)
+cache->handlers.privFree(cache->priv);
+}
+
+
+static void
+virFileCacheDispose(void *obj)
+{
+virFileCachePtr cache = obj;
+
+VIR_FREE(cache->dir);
+VIR_FREE(cache->suffix);
+
+virHashFree(cache->table);
+
+virFileCachePrivFree(cache);
+}
+
+
+static int
+virFileCacheOnceInit(void)
+{
+if (!(virFileCacheClass = virClassNew(virClassForObjectLockable(),
+  "virFileCache",
+  sizeof(virFileCache),
+  virFileCacheDispose)))
+return -1;
+
+return 0;
+}
+
+
+VIR_ONCE_GLOBAL_INIT(virFileCache)
+
+
+static char *
+virFileCacheGetFileName(virFileCachePtr cache,
+const char *name)
+{
+char *file = NULL;
+char *namehash = NULL;
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+
+if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, name, &namehash) < 0)
+goto cleanup;
+
+if (virFileMakePath(cache->dir) < 0) {
+virReportSystemError(errno,
+  

Re: [libvirt] 答复: Re: [PATCH] rpc : fix a access for null pointer

[Michal Privoznik]

On 07/21/2017 08:20 AM, liu.y...@zte.com.cn wrote:
> Hi Michal,
> 
> This problem is triggerred by libvirt python's example event-test.py. the 
> original examples has resouce leak issue
> 
> at the remove_handle and remove_timer. 
> 
> with "python -u event-test.py" run this example and "systemctl restart 
> libvirtd.service" will trigger resource leak problem.
> 
> with lsof -p  can see socket handler's number increased , 
> after restart libvirtd.serivce each time.

This is interesting. When I try this out, the python script just gets
disconnected and never connects back. So I don't see any number (FD)
getting increased.

> 
> the reason is remove_handle and remove_timer do not return the remove 
> handle information to libvirt-python's framework. 
> 
> little patch was apply to this example, to fix this problem.
> 
>Now, run this example again and restart libvirtd.service , call sequence 
> virNetSocketRemoveIOCallback->virNetSocketEventFree 
> 
> can be observed , the no-recursive mutex, lock with recursive issue can be 
> seen. 

Recursive mutexes are usually sign of bad design. Anyway in this case,
one lock should be held by thread doing virNetSocketRemoveIOCallback().
The other (event loop) should be trying to lock the socket lock from
virNetSocketEventFree(). Since these are two different threads, each one
of them is modifying the state of the socket we have to have them use
the lock.

> 
> you can check the detail stack trace and our comments about the lock's 
> issue in function virNetSocketEventFree  in the following.
> 
> 
> 
> 
>     
> 
>  def add_timer(self, interval, cb, opaque):
> 
> timerID = self.nextTimerID + 1
> 
> self.nextTimerID = self.nextTimerID + 1
> 
> 
> 
> 
> h = self.virEventLoopPureTimer(timerID, interval, cb, opaque)
> 
> self.timers.append(h)
> 
> -   self.timers_opaque[timerID] = opaque
> 
> self.interrupt()
> 
> 
> 
> 
> debug("Add timer %d interval %d" % (timerID, interval))
> 
> 
> 
> 
> return timerID
> 
> 
> 
> 
> 
> 
> 
>  def remove_handle(self, handleID):
> 
>  handles = []
> 
> -opaque = None
> 
>  for h in self.handles:
> 
>  if h.get_id() == handleID:
> 
>  self.poll.unregister(h.get_fd())
> 
> -opaque = self.opaques[handleID]
> 
> -del self.opaques[handleID]
> 
>  debug("Remove handle %d fd %d" % (handleID, h.get_fd()))
> 
>  else:
> 
>  handles.append(h)
> 
>  self.handles = handles
> 
>  self.interrupt()
> 
> -return opaque
> 
> 
> 
> 
>  # Stop firing the periodic timer
> 
>  def remove_timer(self, timerID):
> 
>  timers = []
> 
> -opaque = None
> 
>  for h in self.timers:
> 
>  if h.get_id() != timerID:
> 
>  timers.append(h)
> 
> -else:
> 
> -opaque = self.timers_opaque[timerID]
> 
>  debug("Remove timer %d" % timerID)
> 
>  self.timers = timers
> 
>  self.interrupt()
> 
> -return opaque


I don't see this code anywhere and thus cannot perform the changes
you've suggested. Sorry. Is this current git HEAD?

> 
> 
> 
> 
> 
> 
> 
>>> On 07/15/2017 05:00 PM, Peng Hao wrote:
> 
>>> virNetSocketRemoveIOCallback get sock's ObjectLock and will call
> 
>>> virNetSocketEventFree. virNetSocketEventFree may be free sock
> 
>>> object and virNetSocketRemoveIOCallback will access a null pointer
> 
>>> in release sock's ObjectLock.
> 
>>>
> 
>>> Signed-off-by: Liu Yun 
> 
>>> Signed-off-by: Peng Hao 
> 
>>> ---
> 
>>>  src/rpc/virnetsocket.c | 6 +++---
> 
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
>>>
> 
>>
> 
>> I don't think this can work.
> 
>>
> 
>>> diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
> 
>>> index d228c8a..8b550e8 100644
> 
>>> --- a/src/rpc/virnetsocket.c
> 
>>> +++ b/src/rpc/virnetsocket.c
> 
>>> @@ -2140,14 +2140,12 @@ static void virNetSocketEventFree(void *opaque)
> 
>>>  virFreeCallback ff
> 
>>>  void *eopaque
> 
>>>  
> 
>>> -virObjectLock(sock)
> 
>>>  ff = sock->ff
> 
>>>  eopaque = sock->opaque
> 
>>>  sock->func = NULL
> 
>>>  sock->ff = NULL
> 
>>>  sock->opaque = NULL
> 
>>> -virObjectUnlock(sock)
> 
>>
> 
>> I think we need the lock here. This function is called from the event
> 
>> loop thread. So even if virNetSocketUpdateIOCallback() locks the @socket
> 
>> this code can see it unlocked. Or locked. But the crucial part is it's
> 
>> modifying the object and thus should have lock held.
> 
>>
> 
>I have check the code , in default implementation of eventPoll, 
> virEventPollRunOnce always dispatch and clear in one thread loop,
> 
> so, the lock in the virNetSocketEv

Re: [libvirt] [PATCH] libvirt-domain.h: Fix enum description placement

[Martin Kletzander]

On Fri, Jul 21, 2017 at 11:58:45AM +0100, Daniel P. Berrange wrote:

On Fri, Jul 21, 2017 at 12:52:06PM +0200, Michal Privoznik wrote:

There are only two acceptable places for describing enum values.
It's either:

typedef enum {
/* Some long description. Therefore it's placed before
 * the value. */
VIR_ENUM_A_VAL = 1,
} virEnumA;

or:

typedef enum {
VIR_ENUM_B_VAL = 1, /* Some short description */
} virEnumB;

However, during review of a patch sent upstream I realized that
is not always the case. I went through all the public header
files and identified all the offenders. Luckily there were just
two of them.

Yes, this makes our HTML generated documentation broken, but
that's bug of the generator. Our header files shouldn't be forced
to use something we don't want to.


FWIW, the problem is in the parseEnumBLock() method in apibuild.py

Once it has completed parsing an enum item, it delays adding that
enum to the list until it sees the next item, so that it can capture
the trailing comment.

The only way we can distinguish between a comment that comes before
the enum vs a comment after the enum, on the same line, is by detecting
whitespace (newline) before the trailing comment. Unfortunately I don't
thing this is exposed by the lexer right, so its not entirely easy
to solve.



I was under the impression that this worked, we only broke it by some
recent commit.  I looked at the code and got pretty confused by it, but
shouldn't it be pretty easy (from a big picture view)?  You read until
you have both comment and a member of the struct.

If it's really harder than I think, then we can start using some helper
characters for the comments (at least for now) so that we can properly
identify them, e.g.:

struct meh {
  /*# This is comment for the following member foo */
  unsigned int foo;
  int bar; /*< This is for member bar that's on the same line */
}

and so on.  If that doesn't help either and it never worked,
then... it's a pity :-/




Signed-off-by: Michal Privoznik 
---
 include/libvirt/libvirt-domain.h | 17 +
 1 file changed, 9 insertions(+), 8 deletions(-)


Reviewed-by: Daniel P. Berrange 



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

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


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

[libvirt] 答复: Re: [PATCH] rpc : fix a access for null pointer

[liu.yunh]

Hi Michal,

This problem is triggerred by libvirt python's example event-test.py. the 
original examples has resouce leak issue

at the remove_handle and remove_timer. 

with "python -u event-test.py" run this example and "systemctl restart 
libvirtd.service" will trigger resource leak problem.

with lsof -p  can see socket handler's number increased , after 
restart libvirtd.serivce each time.

the reason is remove_handle and remove_timer do not return the remove 
handle information to libvirt-python's framework. 

little patch was apply to this example, to fix this problem.

   Now, run this example again and restart libvirtd.service , call sequence 
virNetSocketRemoveIOCallback->virNetSocketEventFree 

can be observed , the no-recursive mutex, lock with recursive issue can be 
seen. 

you can check the detail stack trace and our comments about the lock's 
issue in function virNetSocketEventFree  in the following.




    

 def add_timer(self, interval, cb, opaque):

timerID = self.nextTimerID + 1

self.nextTimerID = self.nextTimerID + 1




h = self.virEventLoopPureTimer(timerID, interval, cb, opaque)

self.timers.append(h)

-   self.timers_opaque[timerID] = opaque

self.interrupt()




debug("Add timer %d interval %d" % (timerID, interval))




return timerID







 def remove_handle(self, handleID):

 handles = []

-opaque = None

 for h in self.handles:

 if h.get_id() == handleID:

 self.poll.unregister(h.get_fd())

-opaque = self.opaques[handleID]

-del self.opaques[handleID]

 debug("Remove handle %d fd %d" % (handleID, h.get_fd()))

 else:

 handles.append(h)

 self.handles = handles

 self.interrupt()

-return opaque




 # Stop firing the periodic timer

 def remove_timer(self, timerID):

 timers = []

-opaque = None

 for h in self.timers:

 if h.get_id() != timerID:

 timers.append(h)

-else:

-opaque = self.timers_opaque[timerID]

 debug("Remove timer %d" % timerID)

 self.timers = timers

 self.interrupt()

-return opaque







>>On 07/15/2017 05:00 PM, Peng Hao wrote:

>> virNetSocketRemoveIOCallback get sock's ObjectLock and will call

>> virNetSocketEventFree. virNetSocketEventFree may be free sock

>> object and virNetSocketRemoveIOCallback will access a null pointer

>> in release sock's ObjectLock.

>> 

>> Signed-off-by: Liu Yun 

>> Signed-off-by: Peng Hao 

>> ---

>>  src/rpc/virnetsocket.c | 6 +++---

>>  1 file changed, 3 insertions(+), 3 deletions(-)

>> 

>

>I don't think this can work.

>

>> diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c

>> index d228c8a..8b550e8 100644

>> --- a/src/rpc/virnetsocket.c

>> +++ b/src/rpc/virnetsocket.c

>> @@ -2140,14 +2140,12 @@ static void virNetSocketEventFree(void *opaque)

>>  virFreeCallback ff

>>  void *eopaque

>>  

>> -virObjectLock(sock)

>>  ff = sock->ff

>>  eopaque = sock->opaque

>>  sock->func = NULL

>>  sock->ff = NULL

>>  sock->opaque = NULL

>> -virObjectUnlock(sock)

>

>I think we need the lock here. This function is called from the event

>loop thread. So even if virNetSocketUpdateIOCallback() locks the @socket

>this code can see it unlocked. Or locked. But the crucial part is it's

>modifying the object and thus should have lock held.

>

   I have check the code , in default implementation of eventPoll, 
virEventPollRunOnce always dispatch and clear in one thread loop,

so, the lock in the virNetSocketEventFree may be unnessary.




>> -

>> +  

>>  if (ff)

>>  ff(eopaque)

>>  

>> @@ -2207,6 +2205,7 @@ void virNetSocketUpdateIOCallback(virNetSocketPtr sock,

>>  

>>  void virNetSocketRemoveIOCallback(virNetSocketPtr sock)

>>  {

>> +virObjectRef(sock)




This should be mistake when generate the patch. The correct one is 

 +virObjectUnref(sock)

>>  virObjectLock(sock)

>

>I think this is what actually fixes your problem. However, I also think

>it introduces uneven ratio of ref:unref calls.

>

>>  

>>  if (sock->watch < 0) {

>> @@ -2220,6 +2219,7 @@ void virNetSocketRemoveIOCallback(virNetSocketPtr sock)

>>  sock->watch = -1

>>  

>>  virObjectUnlock(sock)

>> +virObjectRef(sock)





This should be mistake when generate the patch. The correct one is 


 +virObjectUnref(sock)

>

>It definitely does so because you ref twice. Anyway, do you perhaps have

>a backtrace to share?



#0  __lll_lock_wait ()

at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135

#1  0x7fde6207cd02

Re: [libvirt] [PATCH v3 2/3] qemu: capabilitity: Introduce QEMU_CAPS_SPAPR_PCI_HOST_BRIDGE_NUMA_NODE

[Andrea Bolognani]

On Fri, 2017-07-21 at 13:32 +0530, Shivaprasad G Bhat wrote:
> @@ -1899,7 +1904,10 @@ static struct virQEMUCapsObjectTypeProps 
> virQEMUCapsObjectProps[] = {
>-1 },
>  { "intel-iommu", virQEMUCapsObjectPropsIntelIOMMU,
>ARRAY_CARDINALITY(virQEMUCapsObjectPropsIntelIOMMU),
> -  QEMU_CAPS_DEVICE_INTEL_IOMMU},
> +  QEMU_CAPS_DEVICE_INTEL_IOMMU },

Tweaking that is entirely out of scope for your patch.
I'll drop the hunk before pushing.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH] docs: fix documentation of enum constants

[Tomáš Golembiovský]

On Fri, 21 Jul 2017 13:26:14 +0200
Michal Privoznik  wrote:

> On 07/21/2017 01:17 PM, Tomáš Golembiovský wrote:
> > On Fri, 21 Jul 2017 12:58:55 +0200
> > Michal Privoznik  wrote:
> >   
> >> On 07/21/2017 12:25 PM, Tomáš Golembiovský wrote:  
> >>> On Fri, 21 Jul 2017 10:12:38 +0200
> >>> Michal Privoznik  wrote:
> >>> 
>  On 07/20/2017 08:21 PM, Tomáš Golembiovský wrote:
> > The documentation string has to follow the definition of a constant in
> > the enum. Otherwise, the HTML documentation will be generated
> > incorrectly.
> >
> > Signed-off-by: Tomáš Golembiovský 
> > ---
> >  include/libvirt/libvirt-domain.h | 62 
> > 
> >  1 file changed, 31 insertions(+), 31 deletions(-)
> >
> > diff --git a/include/libvirt/libvirt-domain.h 
> > b/include/libvirt/libvirt-domain.h
> > index 45f939a8c..2f3162d0f 100644
> > --- a/include/libvirt/libvirt-domain.h
> > +++ b/include/libvirt/libvirt-domain.h
> > @@ -583,56 +583,56 @@ typedef virDomainInterfaceStatsStruct 
> > *virDomainInterfaceStatsPtr;
> >   * Memory Statistics Tags:
> >   */
> >  typedef enum {
> > -/* The total amount of data read from swap space (in kB). */
> >  VIR_DOMAIN_MEMORY_STAT_SWAP_IN = 0,
> > -/* The total amount of memory written out to swap space (in kB). */
> > +/* The total amount of data read from swap space (in kB). */
> >  VIR_DOMAIN_MEMORY_STAT_SWAP_OUT= 1,
> > +/* The total amount of memory written out to swap space (in kB). 
> > */  
> 
>  While this fixes generated HTML, it messes up the header file. So if
>  somebody is looking directly into header file they might get confused.
>  The problem is in our doc generator.
> >>>
> >>> I agree that it's not ideal solution. But since it's already used in the
> >>> header, e.g. in virDomainBlockJobType and
> >>> virDomainEventShutdownDetailType, 
> >>
> >> This one actually looks okay. Did you perhaps mean
> >> virConnectDomainEventDiskChangeReason?  
> > 
> > No, I really meant virDomainEventShutdownDetailType.  
> 
> Are we looking at the same code? Here's what mine looks like:

Damn! We're not. I've been looking at my patched version and
virDomainEventShutdownDetailType was changed by me. Sorry for confusion.

Tomas

> 
> typedef enum {
> /* Guest finished shutdown sequence */
> VIR_DOMAIN_EVENT_SHUTDOWN_FINISHED = 0,
> 
> /* Domain finished shutting down after request from the guest itself
>  * (e.g. hardware-specific action) */
> VIR_DOMAIN_EVENT_SHUTDOWN_GUEST = 1,
> 
> /* Domain finished shutting down after request from the host (e.g. killed 
> by
>  * a signal) */
> VIR_DOMAIN_EVENT_SHUTDOWN_HOST = 2,
> 
> # ifdef VIR_ENUM_SENTINELS
> VIR_DOMAIN_EVENT_SHUTDOWN_LAST
> # endif
> } virDomainEventShutdownDetailType;
> 
> 
> I see nothing wrong about it. Do you?
> 
> Michal


-- 
Tomáš Golembiovský 

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

Re: [libvirt] [PATCH v2 1/6] qemu: command: Rename and move qemuNetworkDriveGetPort

[John Ferlan]

[...]

Before it's too late...

> +
> +/**
> + * virStringParsePort:
> + * @str: port number to parse
> + * @port: pointer to parse port into
> + *
> + * Parses a string representation of a network port and validates it. Returns
> + * 0 on success and -1 on error.
> + */
> +int
> +virStringParsePort(const char *str,
> +   int *port)
> +{
> +unsigned int p = 0;
> +
> +*port = 0;
> +
> +if (!str)
> +return 0;
> +
> +if (virStrToLong_uip(str, NULL, 10, &p) < 0) {
> +virReportError(VIR_ERR_INVALID_ARG,
> +   _("failed to parse port number '%s'"), str);
> +return -1;
> +}
> +
> +if (p > 65535) {

Should this be UINT16_MAX ?


> +virReportError(VIR_ERR_INVALID_ARG,
> +   _("port '%s' out of range"), str);
> +return -1;
> +}
> +
> +*port = p;
> +
> +return 0;
> +}

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

Re: [libvirt] [PATCH 1/3] tests: add qemu x86 kvm 32-on-64 test

[Pavel Hrdina]

On Fri, Jul 14, 2017 at 07:43:04PM -0400, Cole Robinson wrote:
> There's some specific logic in qemuBuildCpuCommandLine to support
> auto adding -cpu qemu 32 for arch=i686 with an x86_64 qemu-kvm binary.
> Add a test case for it
> 
> Signed-off-by: Cole Robinson 
> ---
>  .../qemuxml2argv-x86-kvm-32-on-64.args  | 21 
> +
>  .../qemuxml2argv-x86-kvm-32-on-64.xml   | 13 +
>  tests/qemuxml2argvtest.c|  1 +
>  tests/testutilsqemu.c   | 18 --
>  4 files changed, 51 insertions(+), 2 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.xml
> 
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.args 
> b/tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.args
> new file mode 100644
> index 0..5fdeaf843
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.args
> @@ -0,0 +1,21 @@
> +LC_ALL=C \
> +PATH=/bin \
> +HOME=/home/test \
> +USER=test \
> +LOGNAME=test \
> +QEMU_AUDIO_DRV=none \
> +/usr/bin/qemu-kvm \
> +-name kvm \
> +-S \
> +-machine pc,accel=kvm \
> +-cpu qemu32 \
> +-m 4096 \
> +-smp 1,sockets=1,cores=1,threads=1 \
> +-uuid d091ea82-29e6-2e34-3005-f02617b36e87 \
> +-nographic \
> +-nodefaults \
> +-chardev 
> socket,id=charmonitor,path=/tmp/lib/domain--1-kvm/monitor.sock,server,\
> +nowait \
> +-mon chardev=charmonitor,id=monitor,mode=readline \
> +-no-acpi \
> +-boot c
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.xml 
> b/tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.xml
> new file mode 100644
> index 0..2939cec15
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.xml
> @@ -0,0 +1,13 @@
> +
> +  kvm
> +  d091ea82-29e6-2e34-3005-f02617b36e87
> +  4194304
> +  
> +hvm
> +  
> +  
> +/usr/bin/qemu-kvm
> +
> +
> +  
> +
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 302c9c892..ef5a9b0dc 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -690,6 +690,7 @@ mymain(void)
>  DO_TEST("kvm", QEMU_CAPS_MACHINE_OPT);
>  DO_TEST("default-kvm-host-arch", QEMU_CAPS_MACHINE_OPT);
>  DO_TEST("default-qemu-host-arch", QEMU_CAPS_MACHINE_OPT);
> +DO_TEST("x86-kvm-32-on-64", QEMU_CAPS_MACHINE_OPT);
>  DO_TEST("boot-cdrom", NONE);
>  DO_TEST("boot-network", NONE);
>  DO_TEST("boot-floppy", NONE);
> diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
> index ee4853841..d1290fdde 100644
> --- a/tests/testutilsqemu.c
> +++ b/tests/testutilsqemu.c
> @@ -111,7 +111,8 @@ typedef enum {
>  TEST_UTILS_QEMU_BIN_ARM,
>  TEST_UTILS_QEMU_BIN_PPC64,
>  TEST_UTILS_QEMU_BIN_PPC,
> -TEST_UTILS_QEMU_BIN_S390X
> +TEST_UTILS_QEMU_BIN_S390X,
> +TEST_UTILS_QEMU_BIN_KVM,
>  } QEMUBinType;
>  
>  static const char *QEMUBinList[] = {
> @@ -121,7 +122,8 @@ static const char *QEMUBinList[] = {
>  "/usr/bin/qemu-system-arm",
>  "/usr/bin/qemu-system-ppc64",
>  "/usr/bin/qemu-system-ppc",
> -"/usr/bin/qemu-system-s390x"
> +"/usr/bin/qemu-system-s390x",
> +"/usr/bin/qemu-kvm",

I don't like this patch because it adds a binary that is specific to
downstream releases.  The whole point of my previous patches was to
remove all downstream binaries from our test suite.

This test can be easily done with the current x86_64 binary
TEST_UTILS_QEMU_BIN_X86_64.

Pavel

>  };
>  
>  
> @@ -215,6 +217,18 @@ testQemuAddI686Guest(virCapsPtr caps)
> machines))
>  goto error;
>  
> +machines = NULL;
> +if (!(machines = testQemuAllocMachines(&nmachines)))
> +goto error;
> +
> +if (!virCapabilitiesAddGuestDomain(guest,
> +   VIR_DOMAIN_VIRT_KVM,
> +   QEMUBinList[TEST_UTILS_QEMU_BIN_KVM],
> +   NULL,
> +   nmachines,
> +   machines))
> +goto error;
> +
>  return 0;
>  
>   error:
> -- 
> 2.13.3
> 
> --
> 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] docs: fix documentation of enum constants

[Michal Privoznik]

On 07/21/2017 01:17 PM, Tomáš Golembiovský wrote:
> On Fri, 21 Jul 2017 12:58:55 +0200
> Michal Privoznik  wrote:
> 
>> On 07/21/2017 12:25 PM, Tomáš Golembiovský wrote:
>>> On Fri, 21 Jul 2017 10:12:38 +0200
>>> Michal Privoznik  wrote:
>>>   
 On 07/20/2017 08:21 PM, Tomáš Golembiovský wrote:  
> The documentation string has to follow the definition of a constant in
> the enum. Otherwise, the HTML documentation will be generated
> incorrectly.
>
> Signed-off-by: Tomáš Golembiovský 
> ---
>  include/libvirt/libvirt-domain.h | 62 
> 
>  1 file changed, 31 insertions(+), 31 deletions(-)
>
> diff --git a/include/libvirt/libvirt-domain.h 
> b/include/libvirt/libvirt-domain.h
> index 45f939a8c..2f3162d0f 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -583,56 +583,56 @@ typedef virDomainInterfaceStatsStruct 
> *virDomainInterfaceStatsPtr;
>   * Memory Statistics Tags:
>   */
>  typedef enum {
> -/* The total amount of data read from swap space (in kB). */
>  VIR_DOMAIN_MEMORY_STAT_SWAP_IN = 0,
> -/* The total amount of memory written out to swap space (in kB). */
> +/* The total amount of data read from swap space (in kB). */
>  VIR_DOMAIN_MEMORY_STAT_SWAP_OUT= 1,
> +/* The total amount of memory written out to swap space (in kB). */  
>   

 While this fixes generated HTML, it messes up the header file. So if
 somebody is looking directly into header file they might get confused.
 The problem is in our doc generator.  
>>>
>>> I agree that it's not ideal solution. But since it's already used in the
>>> header, e.g. in virDomainBlockJobType and
>>> virDomainEventShutdownDetailType,   
>>
>> This one actually looks okay. Did you perhaps mean
>> virConnectDomainEventDiskChangeReason?
> 
> No, I really meant virDomainEventShutdownDetailType.

Are we looking at the same code? Here's what mine looks like:

typedef enum {
/* Guest finished shutdown sequence */
VIR_DOMAIN_EVENT_SHUTDOWN_FINISHED = 0,

/* Domain finished shutting down after request from the guest itself
 * (e.g. hardware-specific action) */
VIR_DOMAIN_EVENT_SHUTDOWN_GUEST = 1,

/* Domain finished shutting down after request from the host (e.g. killed by
 * a signal) */
VIR_DOMAIN_EVENT_SHUTDOWN_HOST = 2,

# ifdef VIR_ENUM_SENTINELS
VIR_DOMAIN_EVENT_SHUTDOWN_LAST
# endif
} virDomainEventShutdownDetailType;


I see nothing wrong about it. Do you?

Michal

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

Re: [libvirt] [PATCH] qemu: Clean up firmware list initialization

[Michal Privoznik]

On 07/21/2017 12:44 PM, Andrea Bolognani wrote:
> Instead of going through two completely different code paths,
> one of which repeats the same hardcoded bit of information
> three times in rapid succession, depending on whether or not
> a firmware list has been provided at configure time, just
> provide a reasonable default value and remove the extra code.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_conf.c | 31 +++
>  1 file changed, 7 insertions(+), 24 deletions(-)

ACK. I wonder who introduced the code in the first place O:-)

Michal

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

Re: [libvirt] [PATCH] docs: fix documentation of enum constants

[Tomáš Golembiovský]

On Fri, 21 Jul 2017 12:58:55 +0200
Michal Privoznik  wrote:

> On 07/21/2017 12:25 PM, Tomáš Golembiovský wrote:
> > On Fri, 21 Jul 2017 10:12:38 +0200
> > Michal Privoznik  wrote:
> >   
> >> On 07/20/2017 08:21 PM, Tomáš Golembiovský wrote:  
> >>> The documentation string has to follow the definition of a constant in
> >>> the enum. Otherwise, the HTML documentation will be generated
> >>> incorrectly.
> >>>
> >>> Signed-off-by: Tomáš Golembiovský 
> >>> ---
> >>>  include/libvirt/libvirt-domain.h | 62 
> >>> 
> >>>  1 file changed, 31 insertions(+), 31 deletions(-)
> >>>
> >>> diff --git a/include/libvirt/libvirt-domain.h 
> >>> b/include/libvirt/libvirt-domain.h
> >>> index 45f939a8c..2f3162d0f 100644
> >>> --- a/include/libvirt/libvirt-domain.h
> >>> +++ b/include/libvirt/libvirt-domain.h
> >>> @@ -583,56 +583,56 @@ typedef virDomainInterfaceStatsStruct 
> >>> *virDomainInterfaceStatsPtr;
> >>>   * Memory Statistics Tags:
> >>>   */
> >>>  typedef enum {
> >>> -/* The total amount of data read from swap space (in kB). */
> >>>  VIR_DOMAIN_MEMORY_STAT_SWAP_IN = 0,
> >>> -/* The total amount of memory written out to swap space (in kB). */
> >>> +/* The total amount of data read from swap space (in kB). */
> >>>  VIR_DOMAIN_MEMORY_STAT_SWAP_OUT= 1,
> >>> +/* The total amount of memory written out to swap space (in kB). */  
> >>>   
> >>
> >> While this fixes generated HTML, it messes up the header file. So if
> >> somebody is looking directly into header file they might get confused.
> >> The problem is in our doc generator.  
> > 
> > I agree that it's not ideal solution. But since it's already used in the
> > header, e.g. in virDomainBlockJobType and
> > virDomainEventShutdownDetailType,   
> 
> This one actually looks okay. Did you perhaps mean
> virConnectDomainEventDiskChangeReason?

No, I really meant virDomainEventShutdownDetailType. But you're right
about virConnectDomainEventDiskChangeReason too.

I didn't look at other headers, but as far as libvirt-domain.h is
concerned those three seem to be all. There are also several (5?)
misplaced comments for the sentinels if you care about those too.


> > I assumed it's acceptable. And the
> > overall readability is not that bad because the constant+doc pairs are
> > separated with newline from one another.  
> 
> Nope. It's not. I've sent a patch that fixes those two places (I've went
> through all of our public headers and found just those two though):
> 
> https://www.redhat.com/archives/libvir-list/2017-July/msg00871.html
> 
> > 
> > 
> >   
> >> I recall this being discussed
> >> somewhere recently (probably on the list?). The proper fix IMO is to fix
> >> the generator so that it accepts both:  
> > 
> > That would be the best solution, but I'm too scared to look at the
> > generator. That would be a job for somebody else.  
> 
> Yeah. Our generator is not that great. I wish that we'd switch to
> something already out there so that we don't have to maintain our own
> generator.
> 
> Michal


-- 
Tomáš Golembiovský 

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

Re: [libvirt] [PATCH 10/19] storage: Create accessor API's for virStoragePoolObj

[John Ferlan]

On 07/21/2017 04:22 AM, Pavel Hrdina wrote:
> On Thu, Jul 20, 2017 at 05:38:53PM -0400, John Ferlan wrote:
>> On 07/14/2017 11:37 AM, Pavel Hrdina wrote:
>>> On Tue, May 09, 2017 at 11:30:17AM -0400, John Ferlan wrote:
 In preparation for making a private object, create accessor API's for
 consumer storage functions to use:

 virStoragePoolObjGetDef
 virStoragePoolObjGetNewDef
 virStoragePoolObjStealNewDef
 virStoragePoolObjGetConfigFile
 virStoragePoolObjSetConfigFile
 virStoragePoolObjIsActive
 virStoragePoolObjSetActive
 virStoragePoolObjGetAsyncjobs
 virStoragePoolObjIncrAsyncjobs
 virStoragePoolObjDecrAsyncjobs

 Signed-off-by: John Ferlan 
 ---
  src/conf/virstorageobj.c | 74 
 
  src/conf/virstorageobj.h | 36 +++
  src/libvirt_private.syms | 10 +++
  3 files changed, 115 insertions(+), 5 deletions(-)

 diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
 index 23346f3..7d6b311 100644
 --- a/src/conf/virstorageobj.c
 +++ b/src/conf/virstorageobj.c
 @@ -37,6 +37,80 @@
  VIR_LOG_INIT("conf.virstorageobj");
>>>
>>> [...]
>>>
 +void
 +virStoragePoolObjStealNewDef(virStoragePoolObjPtr obj)
 +{
 +virStoragePoolDefFree(obj->def);
 +obj->def = obj->newDef;
 +obj->newDef = NULL;
 +}
>>>
>>> I didn't notice this until the usage in following patches, the "Steal"
>>> part of the name is confusing.  We have a macro "VIR_STEAL_PTR" which
>>> returns pointer and sets the original one to NULL.  This function
>>> doesn't return the pointer, it replaces @def with @newDef.
>>>
>>> How about virStoragePoolObjUseNewDef() or
>>> virStoragePoolObjDefUseNewDef() or feel free to come up with another
>>> name which would be better than "Steel".
>>>
>>> Pavel
>>>
>>
>> It's theft by deception.
>>
>> How about virStoragePoolObjDefSwapNewDef
> 
> "Swap" is not a good choice as well, it replaces and discards the
> original @def by @newDef, "Swap" can mean that the @def will be @newDef.
> 
> Pavel
> 

I'll go with virStoragePoolObjDefUseNewDef

Although it's more like :

  virStoragePoolObjDefFreeAndAssignNewDefToDef

at some point the name of the function can be too descriptive and too
long;-). Not sure Use is the best either, but I did waste some time
looking at synonyms with nothing that made me think - yeah *that's* it
although Transmogrify came close (google transmogrify+calvin+hobbes -
when I worked at HP that's what we called the layer that "converted" or
"virtualized" privileged machine instructions into instructions that the
guest could run).

John

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

Re: [libvirt] [PATCH] docs: fix documentation of enum constants

[Daniel P. Berrange]

On Fri, Jul 21, 2017 at 01:03:13PM +0200, Michal Privoznik wrote:
> On 07/21/2017 01:01 PM, Daniel P. Berrange wrote:
> > On Fri, Jul 21, 2017 at 12:58:55PM +0200, Michal Privoznik wrote:
> >> 
> >> Yeah. Our generator is not that great. I wish that we'd switch to
> >> something already out there so that we don't have to maintain our own
> >> generator.
> > 
> > There's a good few options for docs generators. Unfortunately our code
> > is also used for generating the XML API description, that's in turn
> > used to generate some language bindings, and I don't know of any tools
> > that could replace that part. So it looks like we're stuck as long as
> > we want to support auto-generated bindings from the XML description
> 
> Well we can use proper doc generator for generating doc and then our own
> generator for generating the XML API description. BTW: what good options
> do you have in mind?

Doxygen, Sphinx, GTK-DOC, though I would not be in favour of Doxygen as
IMHO the docs it generates have awful navigation. QEMU is switching to
use Sphinx IIRC.

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

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

Re: [libvirt] [PATCH] docs: fix documentation of enum constants

[Michal Privoznik]

On 07/21/2017 01:01 PM, Daniel P. Berrange wrote:
> On Fri, Jul 21, 2017 at 12:58:55PM +0200, Michal Privoznik wrote:
>> 
>> Yeah. Our generator is not that great. I wish that we'd switch to
>> something already out there so that we don't have to maintain our own
>> generator.
> 
> There's a good few options for docs generators. Unfortunately our code
> is also used for generating the XML API description, that's in turn
> used to generate some language bindings, and I don't know of any tools
> that could replace that part. So it looks like we're stuck as long as
> we want to support auto-generated bindings from the XML description

Well we can use proper doc generator for generating doc and then our own
generator for generating the XML API description. BTW: what good options
do you have in mind?

Michal

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

Re: [libvirt] [PATCH] docs: fix documentation of enum constants

[Daniel P. Berrange]

On Fri, Jul 21, 2017 at 12:58:55PM +0200, Michal Privoznik wrote:
> On 07/21/2017 12:25 PM, Tomáš Golembiovský wrote:
> > On Fri, 21 Jul 2017 10:12:38 +0200
> > Michal Privoznik  wrote:
> > 
> >> On 07/20/2017 08:21 PM, Tomáš Golembiovský wrote:
> >>> The documentation string has to follow the definition of a constant in
> >>> the enum. Otherwise, the HTML documentation will be generated
> >>> incorrectly.
> >>>
> >>> Signed-off-by: Tomáš Golembiovský 
> >>> ---
> >>>  include/libvirt/libvirt-domain.h | 62 
> >>> 
> >>>  1 file changed, 31 insertions(+), 31 deletions(-)
> >>>
> >>> diff --git a/include/libvirt/libvirt-domain.h 
> >>> b/include/libvirt/libvirt-domain.h
> >>> index 45f939a8c..2f3162d0f 100644
> >>> --- a/include/libvirt/libvirt-domain.h
> >>> +++ b/include/libvirt/libvirt-domain.h
> >>> @@ -583,56 +583,56 @@ typedef virDomainInterfaceStatsStruct 
> >>> *virDomainInterfaceStatsPtr;
> >>>   * Memory Statistics Tags:
> >>>   */
> >>>  typedef enum {
> >>> -/* The total amount of data read from swap space (in kB). */
> >>>  VIR_DOMAIN_MEMORY_STAT_SWAP_IN = 0,
> >>> -/* The total amount of memory written out to swap space (in kB). */
> >>> +/* The total amount of data read from swap space (in kB). */
> >>>  VIR_DOMAIN_MEMORY_STAT_SWAP_OUT= 1,
> >>> +/* The total amount of memory written out to swap space (in kB). */  
> >>
> >> While this fixes generated HTML, it messes up the header file. So if
> >> somebody is looking directly into header file they might get confused.
> >> The problem is in our doc generator.
> > 
> > I agree that it's not ideal solution. But since it's already used in the
> > header, e.g. in virDomainBlockJobType and
> > virDomainEventShutdownDetailType, 
> 
> This one actually looks okay. Did you perhaps mean
> virConnectDomainEventDiskChangeReason?
> 
> > I assumed it's acceptable. And the
> > overall readability is not that bad because the constant+doc pairs are
> > separated with newline from one another.
> 
> Nope. It's not. I've sent a patch that fixes those two places (I've went
> through all of our public headers and found just those two though):
> 
> https://www.redhat.com/archives/libvir-list/2017-July/msg00871.html
> 
> > 
> > 
> > 
> >> I recall this being discussed
> >> somewhere recently (probably on the list?). The proper fix IMO is to fix
> >> the generator so that it accepts both:
> > 
> > That would be the best solution, but I'm too scared to look at the
> > generator. That would be a job for somebody else.
> 
> Yeah. Our generator is not that great. I wish that we'd switch to
> something already out there so that we don't have to maintain our own
> generator.

There's a good few options for docs generators. Unfortunately our code
is also used for generating the XML API description, that's in turn
used to generate some language bindings, and I don't know of any tools
that could replace that part. So it looks like we're stuck as long as
we want to support auto-generated bindings from the XML description


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

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

Re: [libvirt] [PATCH] libvirt-domain.h: Fix enum description placement

[Daniel P. Berrange]

On Fri, Jul 21, 2017 at 12:52:06PM +0200, Michal Privoznik wrote:
> There are only two acceptable places for describing enum values.
> It's either:
> 
> typedef enum {
> /* Some long description. Therefore it's placed before
>  * the value. */
> VIR_ENUM_A_VAL = 1,
> } virEnumA;
> 
> or:
> 
> typedef enum {
> VIR_ENUM_B_VAL = 1, /* Some short description */
> } virEnumB;
> 
> However, during review of a patch sent upstream I realized that
> is not always the case. I went through all the public header
> files and identified all the offenders. Luckily there were just
> two of them.
> 
> Yes, this makes our HTML generated documentation broken, but
> that's bug of the generator. Our header files shouldn't be forced
> to use something we don't want to.

FWIW, the problem is in the parseEnumBLock() method in apibuild.py

Once it has completed parsing an enum item, it delays adding that
enum to the list until it sees the next item, so that it can capture
the trailing comment.

The only way we can distinguish between a comment that comes before
the enum vs a comment after the enum, on the same line, is by detecting
whitespace (newline) before the trailing comment. Unfortunately I don't
thing this is exposed by the lexer right, so its not entirely easy
to solve.


> Signed-off-by: Michal Privoznik 
> ---
>  include/libvirt/libvirt-domain.h | 17 +
>  1 file changed, 9 insertions(+), 8 deletions(-)

Reviewed-by: Daniel P. Berrange 



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

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

Re: [libvirt] [PATCH] docs: fix documentation of enum constants

[Michal Privoznik]

On 07/21/2017 12:25 PM, Tomáš Golembiovský wrote:
> On Fri, 21 Jul 2017 10:12:38 +0200
> Michal Privoznik  wrote:
> 
>> On 07/20/2017 08:21 PM, Tomáš Golembiovský wrote:
>>> The documentation string has to follow the definition of a constant in
>>> the enum. Otherwise, the HTML documentation will be generated
>>> incorrectly.
>>>
>>> Signed-off-by: Tomáš Golembiovský 
>>> ---
>>>  include/libvirt/libvirt-domain.h | 62 
>>> 
>>>  1 file changed, 31 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/include/libvirt/libvirt-domain.h 
>>> b/include/libvirt/libvirt-domain.h
>>> index 45f939a8c..2f3162d0f 100644
>>> --- a/include/libvirt/libvirt-domain.h
>>> +++ b/include/libvirt/libvirt-domain.h
>>> @@ -583,56 +583,56 @@ typedef virDomainInterfaceStatsStruct 
>>> *virDomainInterfaceStatsPtr;
>>>   * Memory Statistics Tags:
>>>   */
>>>  typedef enum {
>>> -/* The total amount of data read from swap space (in kB). */
>>>  VIR_DOMAIN_MEMORY_STAT_SWAP_IN = 0,
>>> -/* The total amount of memory written out to swap space (in kB). */
>>> +/* The total amount of data read from swap space (in kB). */
>>>  VIR_DOMAIN_MEMORY_STAT_SWAP_OUT= 1,
>>> +/* The total amount of memory written out to swap space (in kB). */  
>>
>> While this fixes generated HTML, it messes up the header file. So if
>> somebody is looking directly into header file they might get confused.
>> The problem is in our doc generator.
> 
> I agree that it's not ideal solution. But since it's already used in the
> header, e.g. in virDomainBlockJobType and
> virDomainEventShutdownDetailType, 

This one actually looks okay. Did you perhaps mean
virConnectDomainEventDiskChangeReason?

> I assumed it's acceptable. And the
> overall readability is not that bad because the constant+doc pairs are
> separated with newline from one another.

Nope. It's not. I've sent a patch that fixes those two places (I've went
through all of our public headers and found just those two though):

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

> 
> 
> 
>> I recall this being discussed
>> somewhere recently (probably on the list?). The proper fix IMO is to fix
>> the generator so that it accepts both:
> 
> That would be the best solution, but I'm too scared to look at the
> generator. That would be a job for somebody else.

Yeah. Our generator is not that great. I wish that we'd switch to
something already out there so that we don't have to maintain our own
generator.

Michal

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

[libvirt] [PATCH] libvirt-domain.h: Fix enum description placement

[Michal Privoznik]

There are only two acceptable places for describing enum values.
It's either:

typedef enum {
/* Some long description. Therefore it's placed before
 * the value. */
VIR_ENUM_A_VAL = 1,
} virEnumA;

or:

typedef enum {
VIR_ENUM_B_VAL = 1, /* Some short description */
} virEnumB;

However, during review of a patch sent upstream I realized that
is not always the case. I went through all the public header
files and identified all the offenders. Luckily there were just
two of them.

Yes, this makes our HTML generated documentation broken, but
that's bug of the generator. Our header files shouldn't be forced
to use something we don't want to.

Signed-off-by: Michal Privoznik 
---
 include/libvirt/libvirt-domain.h | 17 +
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 45f939a8c..a3bb9cbe9 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -2309,21 +2309,21 @@ int virDomainSetPerfEvents(virDomainPtr dom,
 typedef enum {
 VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN = 0, /* Placeholder */
 
-VIR_DOMAIN_BLOCK_JOB_TYPE_PULL = 1,
 /* Block Pull (virDomainBlockPull, or virDomainBlockRebase without
  * flags), job ends on completion */
+VIR_DOMAIN_BLOCK_JOB_TYPE_PULL = 1,
 
-VIR_DOMAIN_BLOCK_JOB_TYPE_COPY = 2,
 /* Block Copy (virDomainBlockCopy, or virDomainBlockRebase with
  * flags), job exists as long as mirroring is active */
+VIR_DOMAIN_BLOCK_JOB_TYPE_COPY = 2,
 
-VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT = 3,
 /* Block Commit (virDomainBlockCommit without flags), job ends on
  * completion */
+VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT = 3,
 
-VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT = 4,
 /* Active Block Commit (virDomainBlockCommit with flags), job
  * exists as long as sync is active */
+VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT = 4,
 
 # ifdef VIR_ENUM_SENTINELS
 VIR_DOMAIN_BLOCK_JOB_TYPE_LAST
@@ -3712,12 +3712,13 @@ typedef void 
(*virConnectDomainEventBlockJobCallback)(virConnectPtr conn,
  * The reason describing why this callback is called
  */
 typedef enum {
-VIR_DOMAIN_EVENT_DISK_CHANGE_MISSING_ON_START = 0,
-/* removable media changed to empty according to startup policy as source
+/* Removable media changed to empty according to startup policy as source
  * was missing. oldSrcPath is set, newSrcPath is NULL */
-VIR_DOMAIN_EVENT_DISK_DROP_MISSING_ON_START = 1,
-/* disk was dropped from domain as source file was missing.
+VIR_DOMAIN_EVENT_DISK_CHANGE_MISSING_ON_START = 0,
+
+/* Disk was dropped from domain as source file was missing.
  * oldSrcPath is set, newSrcPath is NULL */
+VIR_DOMAIN_EVENT_DISK_DROP_MISSING_ON_START = 1,
 
 # ifdef VIR_ENUM_SENTINELS
 VIR_DOMAIN_EVENT_DISK_CHANGE_LAST
-- 
2.13.0

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

Re: [libvirt] [PATCH v3 1/2] qemu: Add AAVMF32 to the list of known UEFIs

[Andrea Bolognani]

On Thu, 2017-07-20 at 13:56 -0600, dann frazier wrote:
> @@ -131,6 +131,8 @@ void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr def)
>  #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"
> +#define VIR_QEMU_AAVMF32_LOADER_PATH "/usr/share/AAVMF/AAVMF32_CODE.fd"
> +#define VIR_QEMU_AAVMF32_NVRAM_PATH "/usr/share/AAVMF/AAVMF32_VARS.fd"
>  
>  virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
>  {
> @@ -335,11 +337,11 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool 
> privileged)
>  goto error;
>  
>  #else
> -if (VIR_ALLOC_N(cfg->firmwares, 3) < 0)
> +if (VIR_ALLOC_N(cfg->firmwares, 4) < 0)
>  goto error;
> -cfg->nfirmwares = 3;
> +cfg->nfirmwares = 4;
>  if (VIR_ALLOC(cfg->firmwares[0]) < 0 || VIR_ALLOC(cfg->firmwares[1]) < 0 
>||
> -VIR_ALLOC(cfg->firmwares[2]) < 0)
> +VIR_ALLOC(cfg->firmwares[2]) < 0 || VIR_ALLOC(cfg->firmwares[3]) < 0)
>  goto error;
>  
>  if (VIR_STRDUP(cfg->firmwares[0]->name, VIR_QEMU_AAVMF_LOADER_PATH) < 0 
>||
> @@ -347,7 +349,9 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool 
> privileged)
>  VIR_STRDUP(cfg->firmwares[1]->name, VIR_QEMU_OVMF_LOADER_PATH) < 0 ||
>  VIR_STRDUP(cfg->firmwares[1]->nvram, VIR_QEMU_OVMF_NVRAM_PATH) < 0 ||
>  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)
> +VIR_STRDUP(cfg->firmwares[2]->nvram, VIR_QEMU_OVMF_SEC_NVRAM_PATH) < 
> 0 ||
> +VIR_STRDUP(cfg->firmwares[3]->name, VIR_QEMU_AAVMF32_LOADER_PATH) < 
> 0 ||
> +VIR_STRDUP(cfg->firmwares[3]->nvram, VIR_QEMU_AAVMF32_NVRAM_PATH) < 
> 0)
>  goto error;
>  #endif

Not your fault of course, but the way this is done at the
moment is *extremely* yucky.

I just posted a patch[1] that makes it saner, and although I
realize that's a bit more work I'd ask you to wait for it to
land in master and then respin on top of it.

Everything else looks good though :)


[1] https://www.redhat.com/archives/libvir-list/2017-July/msg00870.html
-- 
Andrea Bolognani / Red Hat / Virtualization

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

[libvirt] [PATCH] qemu: Clean up firmware list initialization

[Andrea Bolognani]

Instead of going through two completely different code paths,
one of which repeats the same hardcoded bit of information
three times in rapid succession, depending on whether or not
a firmware list has been provided at configure time, just
provide a reasonable default value and remove the extra code.

Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_conf.c | 31 +++
 1 file changed, 7 insertions(+), 24 deletions(-)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index a65c92a..6f44cbf 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -125,12 +125,13 @@ 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"
+#ifndef DEFAULT_LOADER_NVRAM
+# define DEFAULT_LOADER_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"
+#endif
+
 
 virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
 {
@@ -328,29 +329,11 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool 
privileged)
 virBitmapSetBit(cfg->namespaces, QEMU_DOMAIN_NS_MOUNT) < 0)
 goto error;
 
-#ifdef DEFAULT_LOADER_NVRAM
 if (virFirmwareParseList(DEFAULT_LOADER_NVRAM,
  &cfg->firmwares,
  &cfg->nfirmwares) < 0)
 goto error;
 
-#else
-if (VIR_ALLOC_N(cfg->firmwares, 3) < 0)
-goto error;
-cfg->nfirmwares = 3;
-if (VIR_ALLOC(cfg->firmwares[0]) < 0 || VIR_ALLOC(cfg->firmwares[1]) < 0 ||
-VIR_ALLOC(cfg->firmwares[2]) < 0)
-goto error;
-
-if (VIR_STRDUP(cfg->firmwares[0]->name, VIR_QEMU_AAVMF_LOADER_PATH) < 0 ||
-VIR_STRDUP(cfg->firmwares[0]->nvram, VIR_QEMU_AAVMF_NVRAM_PATH) < 0  ||
-VIR_STRDUP(cfg->firmwares[1]->name, VIR_QEMU_OVMF_LOADER_PATH) < 0 ||
-VIR_STRDUP(cfg->firmwares[1]->nvram, VIR_QEMU_OVMF_NVRAM_PATH) < 0 ||
-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;
-#endif
-
 return cfg;
 
  error:
-- 
2.7.5

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

Re: [libvirt] [PATCH] docs: fix documentation of enum constants

[Daniel P. Berrange]

On Fri, Jul 21, 2017 at 12:25:02PM +0200, Tomáš Golembiovský wrote:
> On Fri, 21 Jul 2017 10:12:38 +0200
> Michal Privoznik  wrote:
> 
> > On 07/20/2017 08:21 PM, Tomáš Golembiovský wrote:
> > > The documentation string has to follow the definition of a constant in
> > > the enum. Otherwise, the HTML documentation will be generated
> > > incorrectly.
> > > 
> > > Signed-off-by: Tomáš Golembiovský 
> > > ---
> > >  include/libvirt/libvirt-domain.h | 62 
> > > 
> > >  1 file changed, 31 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/include/libvirt/libvirt-domain.h 
> > > b/include/libvirt/libvirt-domain.h
> > > index 45f939a8c..2f3162d0f 100644
> > > --- a/include/libvirt/libvirt-domain.h
> > > +++ b/include/libvirt/libvirt-domain.h
> > > @@ -583,56 +583,56 @@ typedef virDomainInterfaceStatsStruct 
> > > *virDomainInterfaceStatsPtr;
> > >   * Memory Statistics Tags:
> > >   */
> > >  typedef enum {
> > > -/* The total amount of data read from swap space (in kB). */
> > >  VIR_DOMAIN_MEMORY_STAT_SWAP_IN = 0,
> > > -/* The total amount of memory written out to swap space (in kB). */
> > > +/* The total amount of data read from swap space (in kB). */
> > >  VIR_DOMAIN_MEMORY_STAT_SWAP_OUT= 1,
> > > +/* The total amount of memory written out to swap space (in kB). */  
> > 
> > While this fixes generated HTML, it messes up the header file. So if
> > somebody is looking directly into header file they might get confused.
> > The problem is in our doc generator.
> 
> I agree that it's not ideal solution. But since it's already used in the
> header, e.g. in virDomainBlockJobType and
> virDomainEventShutdownDetailType, I assumed it's acceptable. And the
> overall readability is not that bad because the constant+doc pairs are
> separated with newline from one another.

I'm afraid, those examples are bad too and should never have been committed
as they are, so not a good guide to follow.

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

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

Re: [libvirt] [PATCH] docs: fix documentation of enum constants

[Daniel P. Berrange]

On Fri, Jul 21, 2017 at 10:12:38AM +0200, Michal Privoznik wrote:
> On 07/20/2017 08:21 PM, Tomáš Golembiovský wrote:
> > The documentation string has to follow the definition of a constant in
> > the enum. Otherwise, the HTML documentation will be generated
> > incorrectly.
> > 
> > Signed-off-by: Tomáš Golembiovský 
> > ---
> >  include/libvirt/libvirt-domain.h | 62 
> > 
> >  1 file changed, 31 insertions(+), 31 deletions(-)
> > 
> > diff --git a/include/libvirt/libvirt-domain.h 
> > b/include/libvirt/libvirt-domain.h
> > index 45f939a8c..2f3162d0f 100644
> > --- a/include/libvirt/libvirt-domain.h
> > +++ b/include/libvirt/libvirt-domain.h
> > @@ -583,56 +583,56 @@ typedef virDomainInterfaceStatsStruct 
> > *virDomainInterfaceStatsPtr;
> >   * Memory Statistics Tags:
> >   */
> >  typedef enum {
> > -/* The total amount of data read from swap space (in kB). */
> >  VIR_DOMAIN_MEMORY_STAT_SWAP_IN = 0,
> > -/* The total amount of memory written out to swap space (in kB). */
> > +/* The total amount of data read from swap space (in kB). */
> >  VIR_DOMAIN_MEMORY_STAT_SWAP_OUT= 1,
> > +/* The total amount of memory written out to swap space (in kB). */
> 
> While this fixes generated HTML, it messes up the header file. So if
> somebody is looking directly into header file they might get confused.

Yeah, absolutely NACK to this approach. Putting comments after the
constant name is awful for people reading the code.

> The problem is in our doc generator. I recall this being discussed
> somewhere recently (probably on the list?). The proper fix IMO is to fix
> the generator so that it accepts both:
> 
> enum {
>   /* Some very long description - therefore it's before the value. */
>   VIR_ENUM_A_VAL = 0,
> } virEnumA;
> 
> enum {
>   VIR_ENUM_B_VAL = 0, /* Short description */
> } virEnumB;
> 

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

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

Re: [libvirt] [PATCH] docs: fix documentation of enum constants

[Tomáš Golembiovský]

On Fri, 21 Jul 2017 10:12:38 +0200
Michal Privoznik  wrote:

> On 07/20/2017 08:21 PM, Tomáš Golembiovský wrote:
> > The documentation string has to follow the definition of a constant in
> > the enum. Otherwise, the HTML documentation will be generated
> > incorrectly.
> > 
> > Signed-off-by: Tomáš Golembiovský 
> > ---
> >  include/libvirt/libvirt-domain.h | 62 
> > 
> >  1 file changed, 31 insertions(+), 31 deletions(-)
> > 
> > diff --git a/include/libvirt/libvirt-domain.h 
> > b/include/libvirt/libvirt-domain.h
> > index 45f939a8c..2f3162d0f 100644
> > --- a/include/libvirt/libvirt-domain.h
> > +++ b/include/libvirt/libvirt-domain.h
> > @@ -583,56 +583,56 @@ typedef virDomainInterfaceStatsStruct 
> > *virDomainInterfaceStatsPtr;
> >   * Memory Statistics Tags:
> >   */
> >  typedef enum {
> > -/* The total amount of data read from swap space (in kB). */
> >  VIR_DOMAIN_MEMORY_STAT_SWAP_IN = 0,
> > -/* The total amount of memory written out to swap space (in kB). */
> > +/* The total amount of data read from swap space (in kB). */
> >  VIR_DOMAIN_MEMORY_STAT_SWAP_OUT= 1,
> > +/* The total amount of memory written out to swap space (in kB). */  
> 
> While this fixes generated HTML, it messes up the header file. So if
> somebody is looking directly into header file they might get confused.
> The problem is in our doc generator.

I agree that it's not ideal solution. But since it's already used in the
header, e.g. in virDomainBlockJobType and
virDomainEventShutdownDetailType, I assumed it's acceptable. And the
overall readability is not that bad because the constant+doc pairs are
separated with newline from one another.



> I recall this being discussed
> somewhere recently (probably on the list?). The proper fix IMO is to fix
> the generator so that it accepts both:

That would be the best solution, but I'm too scared to look at the
generator. That would be a job for somebody else.

Tomas

> 
> enum {
>   /* Some very long description - therefore it's before the value. */
>   VIR_ENUM_A_VAL = 0,
> } virEnumA;
> 
> enum {
>   VIR_ENUM_B_VAL = 0, /* Short description */
> } virEnumB;
> 
> 
> Michal


-- 
Tomáš Golembiovský 

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

Re: [libvirt] [PATCH v2 0/6] Convert port to unsigned integer

[Ján Tomko]

On Thu, Jul 20, 2017 at 02:29:57PM +0200, Peter Krempa wrote:

Since Michal did not want to finis this I did. I also found a few bugs in
my previous patches which are addressed here.



Thank you for taking this up, even though you did not finish the word
'finish'.


Peter Krempa (6):
 qemu: command: Rename and move qemuNetworkDriveGetPort
 util: uri: Convert port number to unsigned integer
 qemu: command: Remove condition to use default sheepdog port
 util: storage: fill in default ports when parsing backing chain
 conf: domain: Split up virDomainStorageHostParse and rename it
 virStorageNetHostDef: Turn @port into integer

src/conf/domain_conf.c| 133 +++---
src/libvirt_private.syms  |   1 +
src/libxl/libxl_conf.c|   2 +-
src/qemu/qemu_block.c |   7 +-
src/qemu/qemu_command.c   |  30 ++--
src/qemu/qemu_parse_command.c |  13 ++--
src/storage/storage_backend_gluster.c |  17 +
src/storage/storage_driver.c  |   7 +-
src/util/virstoragefile.c |  68 +++--
src/util/virstoragefile.h |   4 +-
src/util/virstring.c  |  37 ++
src/util/virstring.h  |   4 +
src/util/viruri.h |   2 +-
src/xenconfig/xen_xl.c|   2 +-
tests/virstoragetest.c|  14 ++--
15 files changed, 182 insertions(+), 159 deletions(-)


ACK series.

Jan


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

Re: [libvirt] [PATCH v2 2/3] qemu: capabilitity: Introduce QEMU_CAPS_SPAPR_PCI_HOST_BRIDGE_NUMA_NODE

[Shivaprasad G Bhat]

On 07/19/2017 07:08 PM, Andrea Bolognani wrote:

On Mon, 2017-07-17 at 21:28 +0530, Shivaprasad G Bhat wrote:
[...]

@@ -1900,6 +1905,9 @@ static struct virQEMUCapsObjectTypeProps 
virQEMUCapsObjectProps[] = {
   { "intel-iommu", virQEMUCapsObjectPropsIntelIOMMU,
 ARRAY_CARDINALITY(virQEMUCapsObjectPropsIntelIOMMU),
 QEMU_CAPS_DEVICE_INTEL_IOMMU},
+{ "spapr-pci-host-bridge", virQEMUCapsObjectPropsSpaprPCIHostBridge,
+  ARRAY_CARDINALITY(virQEMUCapsObjectPropsSpaprPCIHostBridge),
+  QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE},

There should be a space before the closing curly brace.

Thanks.. I was just following the previous pattern at  " 
QEMU_CAPS_DEVICE_INTEL_IOMMU},"


Will fix that along. :)


Reviewed-by: Andrea Bolognani 

--
Andrea Bolognani / Red Hat / Virtualization



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

Re: [libvirt] [PATCH RESEND] qemu: Remove inactive vm when failedtostart it

[wang.yi59]

>On 07/12/2017 07:34 AM, wang yi59 zte com cn wrote:

>> Hi John,

>>

>> Thanks for your review!

>>

>

>Somehow your response is out of synch with the rest of the series -

>things like this get lost very quickly.

>

>>> This seems to be a strange sequence of operations, but the claim is that

>>

>>> by adding this logic to CreateWithFlags, then the problem you're facing

>>

>>> is resolved. However, is adding this to the Create logic the right thing

>>

>>> to do?

>>

>>> IIUC:

>>

>> This condition is rare, so I designed some operations which can help us

>> to understand what happened:

>>

>> # virsh define win7 -> successful

>>

>> # virsh start win7 & sleep 0.2 virsh undefine win7 -> start failed and

>> undefine successful

>>

>>

>>

>> and we may see libvirtd output such logs like this:

>>

>> qemuDomainDefineXMLFlags:7386 : Creating domain win7

>>

>> qemuProcessStart:6086 : conn=

>>

>> qemuDomainUndefineFlags:7501 : Undefining domain win7

>>

>> error : qemuMonitorOpenUnix:379 : failed to connect to monitor socket

>>

>>

>>

>> Libvirtd unlocked vm during qemuProcessStart, so qemuDomainUndefineFlags

>> can get the lock and undefine vm successfully.

>

>Can you be a bit more specific. Do you mean during qemuConnectMonitor

>processing during ProcessLaunch? Or perhaps Agent processing?




It failed in processing during ProcessLaunch, on which period qemu exited

unexpectedly.




>

>In any case, perhaps then the better fix is to prohibit undefine whilst

>unlocked for Monitor or Agent start. The perhaps interesting question is

>where to set flag. Setting/clearing just around Monitor/Agent startup

>would be a seemingly short window and perhaps where you're seeing the

>virObjectUnlock however, I wonder if perhaps all of qemuProcessLaunch

>should be "protected" as there's some really critical things happening

>in there. Thus at the top one sets a boolean "obj->starting = true" and

>at cleanup "obj->starting = false".

>

>In qemuDomainUndefineFlags, there'd need to be a similar check to the if

>(!persistent) along the lines of "if (starting)" then an error message

>indicating that the domain is starting up and cannot be undefined.

>Perhaps similarly for Destroy just to be safe.




Yes, your advice looks better. So it seems like that we should protect all

of qemuProcessStart, 'cause some functions such like qemuProcessFinishStartup

may also invoke Monitor.

If this works, I will send the V2 patch.




>

>I didn't think all that long about it, but hopefully it's enough to

>perhaps have to generate more patches...  I don't believe with that it'd

>be possible to have the need to qemuDomainRemoveInactive in the

>CreateWithFlags, but I could be wrong.




---

Best wishes

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

Re: [libvirt] [PATCH] conf: Fix backwards migration of pSeries guests

[Andrea Bolognani]

On Fri, 2017-07-21 at 10:07 +0200, Andrea Bolognani wrote:
> > That is the main reason to fill all the values in right away. Since
> > there apparently was a period, where a default would be used, but not
> > recorded, it needs some trickery unfortunately.
> >  
> > In such case you basically standardize, that the now-filled-in default
> > model (which can't ever change) if it was not provided by the user is to
> > be dropped from the migratable XML.
> >  
> > Once you start assign a new default model, that then needs to be
> > explicitly sent over, so that the migration will not be successufl
> > unless the destination is able to use the new model.
> >  
> > This means basically, that a missing model name becomes assigned to a
> > particular value.
> 
> That makes sense.
> 
> Doesn't it also mean that we don't really need to record
> whether the user set the model name explicitly or not? We
> can just skip formatting it if it's spapr-pci-host-bridge
> and all versions of libvirt, past or future, will handle
> that correctly.

Wait, that's exactly how this patch behaves, except it's
not immediately apparent because the check for the model
name happens inside virDomainControllerIsPCIHostBridge()
rather than being explicit.

So maybe I'm still missing something?

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 10/19] storage: Create accessor API's for virStoragePoolObj

[Pavel Hrdina]

On Thu, Jul 20, 2017 at 05:38:53PM -0400, John Ferlan wrote:
> On 07/14/2017 11:37 AM, Pavel Hrdina wrote:
> > On Tue, May 09, 2017 at 11:30:17AM -0400, John Ferlan wrote:
> >> In preparation for making a private object, create accessor API's for
> >> consumer storage functions to use:
> >>
> >> virStoragePoolObjGetDef
> >> virStoragePoolObjGetNewDef
> >> virStoragePoolObjStealNewDef
> >> virStoragePoolObjGetConfigFile
> >> virStoragePoolObjSetConfigFile
> >> virStoragePoolObjIsActive
> >> virStoragePoolObjSetActive
> >> virStoragePoolObjGetAsyncjobs
> >> virStoragePoolObjIncrAsyncjobs
> >> virStoragePoolObjDecrAsyncjobs
> >>
> >> Signed-off-by: John Ferlan 
> >> ---
> >>  src/conf/virstorageobj.c | 74 
> >> 
> >>  src/conf/virstorageobj.h | 36 +++
> >>  src/libvirt_private.syms | 10 +++
> >>  3 files changed, 115 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
> >> index 23346f3..7d6b311 100644
> >> --- a/src/conf/virstorageobj.c
> >> +++ b/src/conf/virstorageobj.c
> >> @@ -37,6 +37,80 @@
> >>  VIR_LOG_INIT("conf.virstorageobj");
> > 
> > [...]
> > 
> >> +void
> >> +virStoragePoolObjStealNewDef(virStoragePoolObjPtr obj)
> >> +{
> >> +virStoragePoolDefFree(obj->def);
> >> +obj->def = obj->newDef;
> >> +obj->newDef = NULL;
> >> +}
> > 
> > I didn't notice this until the usage in following patches, the "Steal"
> > part of the name is confusing.  We have a macro "VIR_STEAL_PTR" which
> > returns pointer and sets the original one to NULL.  This function
> > doesn't return the pointer, it replaces @def with @newDef.
> > 
> > How about virStoragePoolObjUseNewDef() or
> > virStoragePoolObjDefUseNewDef() or feel free to come up with another
> > name which would be better than "Steel".
> > 
> > Pavel
> > 
> 
> It's theft by deception.
> 
> How about virStoragePoolObjDefSwapNewDef

"Swap" is not a good choice as well, it replaces and discards the
original @def by @newDef, "Swap" can mean that the @def will be @newDef.

Pavel


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

Re: [libvirt] [PATCH 11/19] storage: Introduce virStoragePoolObjNew

[Pavel Hrdina]

On Thu, Jul 20, 2017 at 06:00:05PM -0400, John Ferlan wrote:
> 
> 
> On 07/11/2017 11:17 AM, Pavel Hrdina wrote:
> > On Tue, May 09, 2017 at 11:30:18AM -0400, John Ferlan wrote:
> >> Create/use a helper to perform object allocation.
> >>
> >> Signed-off-by: John Ferlan 
> >> ---
> >>  src/conf/virstorageobj.c | 34 +++---
> >>  1 file changed, 23 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
> >> index 7d6b311..0079472 100644
> >> --- a/src/conf/virstorageobj.c
> >> +++ b/src/conf/virstorageobj.c
> >> @@ -37,6 +37,27 @@
> >>  VIR_LOG_INIT("conf.virstorageobj");
> >>  
> >>  
> >> +static virStoragePoolObjPtr
> >> +virStoragePoolObjNew(virStoragePoolDefPtr def)
> >> +{
> >> +virStoragePoolObjPtr obj;
> >> +
> >> +if (VIR_ALLOC(obj) < 0)
> >> +return NULL;
> >> +
> >> +if (virMutexInit(&obj->lock) < 0) {
> >> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >> +   _("cannot initialize mutex"));
> >> +VIR_FREE(obj);
> >> +return NULL;
> >> +}
> >> +virStoragePoolObjLock(obj);
> >> +obj->active = 0;
> >> +obj->def = def;
> >> +return obj;
> >> +}
> >> +
> >> +
> >>  virStoragePoolDefPtr
> >>  virStoragePoolObjGetDef(virStoragePoolObjPtr obj)
> >>  {
> >> @@ -386,24 +407,15 @@ virStoragePoolObjAssignDef(virStoragePoolObjListPtr 
> >> pools,
> >>  return obj;
> >>  }
> >>  
> >> -if (VIR_ALLOC(obj) < 0)
> >> +if (!(obj = virStoragePoolObjNew(def)))
> >>  return NULL;
> > 
> > The new virStoragePoolObjNew() doesn't have to take @def and ...
> > 
> >>  
> >> -if (virMutexInit(&obj->lock) < 0) {
> >> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >> -   _("cannot initialize mutex"));
> >> -VIR_FREE(obj);
> >> -return NULL;
> >> -}
> >> -virStoragePoolObjLock(obj);
> >> -obj->active = 0;
> >> -
> >>  if (VIR_APPEND_ELEMENT_COPY(pools->objs, pools->count, obj) < 0) {
> >> +obj->def = NULL;
> > 
> > ... need to set the @obj->def to NULL and ...
> > 
> >>  virStoragePoolObjUnlock(obj);
> >>  virStoragePoolObjFree(obj);
> >>  return NULL;
> >>  }
> >> -obj->def = def;
> > 
> > ... just keep this line as it is.
> > 
> 
> These are all changed in my local branch.
> 
> I figure as long as you're good with my most recent comments to 8/19
> that since I have ACKs now through 9/19 - I'll push those, then post a
> new version for the rest (eventually, but not right away).

That sound good to me, patches 01-09 with the fixes can be pushed.

> Hopefully that works.  I think I want to work through the NodeDev,
> Secrets, NWFilter, and Interface first. Then jump back into Storage and
> Network before perhaps one day considering domain.

Sure, the Storage and Network objects are more complex so it makes
perfect sense.

> Thanks for the reviews here though - I certainly appreciate it.

You're welcome :)

Pavel


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

Re: [libvirt] [PATCH] conf: Fix backwards migration of pSeries guests

[Andrea Bolognani]

On Thu, 2017-07-20 at 09:55 +0200, Peter Krempa wrote:
> > In any case, that made me realize that not sending the model,
> > even if automatically filled in, could cause issues in the
> > future if a new model is added and becomes the default, as
> > the guest ABI would not be preserved during migration then.
> 
> That is the main reason to fill all the values in right away. Since
> there apparently was a period, where a default would be used, but not
> recorded, it needs some trickery unfortunately.
> 
> In such case you basically standardize, that the now-filled-in default
> model (which can't ever change) if it was not provided by the user is to
> be dropped from the migratable XML.
> 
> Once you start assign a new default model, that then needs to be
> explicitly sent over, so that the migration will not be successufl
> unless the destination is able to use the new model.
> 
> This means basically, that a missing model name becomes assigned to a
> particular value.

That makes sense.

Doesn't it also mean that we don't really need to record
whether the user set the model name explicitly or not? We
can just skip formatting it if it's spapr-pci-host-bridge
and all versions of libvirt, past or future, will handle
that correctly.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v2 3/3] qemu: Enable NUMA node tag in pci-root for PPC64

[Shivaprasad G Bhat]

Hi Andrea,


On 07/20/2017 09:16 PM, Andrea Bolognani wrote:

On Mon, 2017-07-17 at 21:29 +0530, Shivaprasad G Bhat wrote:

@@ -3786,6 +3786,11 @@
   part of the specified NUMA node (it is up to the user of the
   libvirt API to attach host devices to the correct
   pci-expander-bus when assigning them to the domain).
+On PPC64, the PCI devices can be specified to be part of a NUMA
+node using only the pci-root controller with an optional
+ subelement within the
+ subelement. The PCI devices on the
+given pci-root controller will be part of the specified NUMA node.

Instead of adding an entire new sentence here, it would make
more sense to rephrase what's already present, something like:

   Some PCI controllers (pci-expander-bus for the pc machine
   type, pcie-expander-bus for the q35 machine type and
   pci-root for the pseries machine type) can have an optional
subelement [...]

@@ -9457,6 +9457,12 @@ virDomainControllerDefParseXML(xmlNodePtr node,
   goto error;
   }
   }
+if (def->idx == 0 && numaNode >= 0) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("Only the PCI controller with index != 0 can "
+ "have NUMA node property specified"));
+goto error;
+}
   if (numaNode >= 0)
   def->opts.pciopts.numaNode = numaNode;

The check you're adding can be merged with the one below it.

The error message should also be reworded, something like:

   The PCI controller with index=0 can't be associated with
   a NUMA node.

@@ -3458,9 +3458,14 @@ 
qemuDomainControllerDefPostParse(virDomainControllerDefPtr cont,
* that NUMA node is configured in the guest 
* array. NUMA cell id's in this array are numbered
* from 0 .. size-1.
+ *
+ * On PSeries, the NUMA node is set at the PHB.

As above, reworking the existing comment would work better
than merely appending to it.


*/
-if ((cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS ||
- cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS) &&
+if (((qemuDomainIsPSeries(def) &&
+  cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
+  cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) ||
+ (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS ||
+  cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS)) &&
   (int) virDomainNumaGetNodeCount(def->numa)
   <= cont->opts.pciopts.numaNode) {

I actually don't see any point in the condition being this
complex: it could just be

   if (cont->opts.pciopts.numaNode >= 0 &&
   cont->opts.pciopts.numaNode >=
   (int) virDomainNumaGetNodeCount(def->numa))

because we've already made sure, while parsing, that numaNode
is only set for controllers that allow it.


+++ b/tests/qemuxml2argvdata/qemuxml2argv-spapr-pci-host-bridge-numa-node.xml
@@ -0,0 +1,54 @@
+
+  QEMUGuest1
+  87eedafe-eedc-4336-8130-ed9fe5dc90c8
+  2097152
+  2048

You don't need 


+  8
+  
+
+  
+  
+
+
+  
+  
+
+  
+  
+hvm
+

 is unnecessary.


+  
+  
+  destroy
+  restart
+  destroy

 and  can be removed.


+  
+/usr/bin/qemu-system-ppc64
+
+  
+  
+  
+  
+

 is irrelevant for the test case at hand.


+

The model should be 'none'.


+

The SCSI controller is also not useful here.


+
+  
+
+
+  
+1
+  
+
+
+  
+
+
+  
+0
+  
+
+
+

You can omit .


+++ b/tests/qemuxml2argvtest.c
@@ -2739,6 +2739,9 @@ mymain(void)
   DO_TEST_PARSE_ERROR("cpu-cache-emulate-l2", QEMU_CAPS_KVM);
   DO_TEST_PARSE_ERROR("cpu-cache-passthrough3", QEMU_CAPS_KVM);
   DO_TEST_PARSE_ERROR("cpu-cache-passthrough-l3", QEMU_CAPS_KVM);
+DO_TEST("spapr-pci-host-bridge-numa-node", QEMU_CAPS_NUMA,
+QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE,
+QEMU_CAPS_SPAPR_PCI_HOST_BRIDGE_NUMA_NODE);

This should be moved up to be close to the pserie-phb* test
cases, and renamed to something like 'pseries-phb-numa-node'.

There should be one capability per line.

You also need to have an identical test case in qemuxml2xml,
and at least one negative test to show that trying to
assign the default PHB to a NUMA node will result in failure.

Agree to all your comments. Fixing them all.

As I relook at my test case, I realize the numatune should be for the 
memnode
instead of memory for this use case. Fixing that as well. For 
pci-expander-bus

test cases I see no numatune in the xml, will fix them later.



--
Andrea Bolognani / Red Hat / Virtualization



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

Re: [libvirt] [PATCH] docs: fix documentation of enum constants

[Michal Privoznik]

On 07/20/2017 08:21 PM, Tomáš Golembiovský wrote:
> The documentation string has to follow the definition of a constant in
> the enum. Otherwise, the HTML documentation will be generated
> incorrectly.
> 
> Signed-off-by: Tomáš Golembiovský 
> ---
>  include/libvirt/libvirt-domain.h | 62 
> 
>  1 file changed, 31 insertions(+), 31 deletions(-)
> 
> diff --git a/include/libvirt/libvirt-domain.h 
> b/include/libvirt/libvirt-domain.h
> index 45f939a8c..2f3162d0f 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -583,56 +583,56 @@ typedef virDomainInterfaceStatsStruct 
> *virDomainInterfaceStatsPtr;
>   * Memory Statistics Tags:
>   */
>  typedef enum {
> -/* The total amount of data read from swap space (in kB). */
>  VIR_DOMAIN_MEMORY_STAT_SWAP_IN = 0,
> -/* The total amount of memory written out to swap space (in kB). */
> +/* The total amount of data read from swap space (in kB). */
>  VIR_DOMAIN_MEMORY_STAT_SWAP_OUT= 1,
> +/* The total amount of memory written out to swap space (in kB). */

While this fixes generated HTML, it messes up the header file. So if
somebody is looking directly into header file they might get confused.
The problem is in our doc generator. I recall this being discussed
somewhere recently (probably on the list?). The proper fix IMO is to fix
the generator so that it accepts both:

enum {
  /* Some very long description - therefore it's before the value. */
  VIR_ENUM_A_VAL = 0,
} virEnumA;

enum {
  VIR_ENUM_B_VAL = 0, /* Short description */
} virEnumB;


Michal

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

[libvirt] [PATCH v3 3/3] qemu: Enable NUMA node tag in pci-root for PPC64

[Shivaprasad G Bhat]

This patch addresses the same aspects on PPC the bug 1103314 addressed
on x86.

PCI expander bus creates multiple primary PCI busses, where each of these
busses can be assigned a specific NUMA affinity, which, on x86 is
advertised through ACPI on a per-bus basis.

For SPAPR, a PHB's NUMA affinities are assigned on a per-PHB basis, and
there is no mechanism for advertising NUMA affinities to a guest on a
per-bus basis. So, even if qemu-ppc manages to get some sort of multi-bus
topology working using PXB, there is no way to expose the affinities
of these busses to the guest. It can only be exposed on a per-PHB/per-domain
basis.

So patch enables NUMA node tag in pci-root controller on PPC.

The way to set the NUMA node is through the numa_node option of
spapr-pci-host-bridge device. However for the implicit PHB, the only way
to set the numa_node is from the -global option. The -global option applies
to all the PHBs unless explicitly specified with the option on the
respective PHB of CLI. The default PHB has the emulated devices only, so
the patch prevents setting the NUMA node for the default PHB.

Signed-off-by: Shivaprasad G Bhat 
---
 docs/formatdomain.html.in  |4 +-
 src/conf/domain_conf.c |9 +++
 src/qemu/qemu_command.c|   10 
 src/qemu/qemu_domain.c |   13 ++---
 .../qemuxml2argv-pseries-default-phb-numa-node.xml |   29 +++
 .../qemuxml2argv-pseries-phb-numa-node.args|   28 +++
 .../qemuxml2argv-pseries-phb-numa-node.xml |   41 
 tests/qemuxml2argvtest.c   |6 ++
 .../qemuxml2xmlout-pseries-phb-numa-node.xml   |   52 
 tests/qemuxml2xmltest.c|4 ++
 10 files changed, 187 insertions(+), 9 deletions(-)
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-pseries-default-phb-numa-node.xml
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-pseries-phb-numa-node.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-pseries-phb-numa-node.xml
 create mode 100644 
tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-phb-numa-node.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index c12efcf..04f31aa 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -3778,7 +3778,9 @@
   
   node
   
-pci-expander-bus controllers can have an
+Some PCI controllers (pci-expander-bus for the pc machine
+type, pcie-expander-bus for the q35 machine type and
+pci-root for the pseries machine type) can have an
 optional  subelement within
 the  subelement, which is used to
 set the NUMA node reported to the guest OS for that bus - the
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3feeccb..8c4133c 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9457,8 +9457,15 @@ virDomainControllerDefParseXML(xmlNodePtr node,
 goto error;
 }
 }
-if (numaNode >= 0)
+if (numaNode >= 0) {
 def->opts.pciopts.numaNode = numaNode;
+if (def->idx == 0) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("The PCI controller with index=0 can't "
+ "be associated with a NUMA node."));
+goto error;
+}
+}
 break;
 
 default:
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6ac26af..83b277b 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3038,6 +3038,16 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
 virBufferAsprintf(&buf, "%s,index=%d,id=%s",
   modelName, def->opts.pciopts.targetIndex,
   def->info.alias);
+
+if (def->opts.pciopts.numaNode != -1) {
+if (!virQEMUCapsGet(qemuCaps, 
QEMU_CAPS_SPAPR_PCI_HOST_BRIDGE_NUMA_NODE)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("the spapr-pci-host-bridge controller "
+ "doesn't support numa_node on this QEMU 
binary"));
+goto error;
+}
+virBufferAsprintf(&buf, ",numa_node=%d", 
def->opts.pciopts.numaNode);
+}
 break;
 case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
 case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 464d3a1..8609e6c 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3454,15 +3454,14 @@ 
qemuDomainControllerDefPostParse(virDomainControllerDefPtr cont,
 return -1;
 }
 
-/* if a PCI expander bus has a NUMA node set, make sure
- * that NUMA node

[libvirt] [PATCH v3 2/3] qemu: capabilitity: Introduce QEMU_CAPS_SPAPR_PCI_HOST_BRIDGE_NUMA_NODE

[Shivaprasad G Bhat]

The patch adds a capability for spapr-pci-host-bridge.numa_node.

Signed-off-by: Shivaprasad G Bhat 
Reviewed-by: Andrea Bolognani 
---
 src/qemu/qemu_capabilities.c   |   10 ++
 src/qemu/qemu_capabilities.h   |1 
 .../caps_2.6.0.ppc64le.replies |   60 +++-
 .../caps_2.9.0.ppc64le.replies |  100 +++-
 tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.xml  |1 
 5 files changed, 157 insertions(+), 15 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 04aa8d5..a43bc46 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -431,6 +431,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
   "virtio.ats",
   "loadparm",
   "spapr-pci-host-bridge",
+  "spapr-pci-host-bridge.numa_node",
 );
 
 
@@ -1700,6 +1701,10 @@ static struct virQEMUCapsStringFlags 
virQEMUCapsObjectPropsVirtioNet[] = {
 { "host_mtu", QEMU_CAPS_VIRTIO_NET_HOST_MTU },
 };
 
+static struct virQEMUCapsStringFlags 
virQEMUCapsObjectPropsSpaprPCIHostBridge[] = {
+{ "numa_node", QEMU_CAPS_SPAPR_PCI_HOST_BRIDGE_NUMA_NODE },
+};
+
 static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioSCSI[] = {
 { "iothread", QEMU_CAPS_VIRTIO_SCSI_IOTHREAD },
 };
@@ -1899,7 +1904,10 @@ static struct virQEMUCapsObjectTypeProps 
virQEMUCapsObjectProps[] = {
   -1 },
 { "intel-iommu", virQEMUCapsObjectPropsIntelIOMMU,
   ARRAY_CARDINALITY(virQEMUCapsObjectPropsIntelIOMMU),
-  QEMU_CAPS_DEVICE_INTEL_IOMMU},
+  QEMU_CAPS_DEVICE_INTEL_IOMMU },
+{ "spapr-pci-host-bridge", virQEMUCapsObjectPropsSpaprPCIHostBridge,
+  ARRAY_CARDINALITY(virQEMUCapsObjectPropsSpaprPCIHostBridge),
+  QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE },
 };
 
 struct virQEMUCapsPropTypeObjects {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 8250b57..65bc350 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -417,6 +417,7 @@ typedef enum {
 QEMU_CAPS_VIRTIO_PCI_ATS, /* virtio-*-pci.ats */
 QEMU_CAPS_LOADPARM, /* -machine loadparm */
 QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, /* -device spapr-pci-host-bridge */
+QEMU_CAPS_SPAPR_PCI_HOST_BRIDGE_NUMA_NODE, /* 
spapr-pci-host-bridge.numa_node= */
 
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.replies 
b/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.replies
index 6350269..2e4fa72 100644
--- a/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.replies
+++ b/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.replies
@@ -3812,6 +3812,52 @@
 {
   "return": [
 {
+  "name": "dynamic-reconfiguration",
+  "type": "bool"
+},
+{
+  "name": "dma_win_size",
+  "type": "uint64"
+},
+{
+  "name": "dma_win_addr",
+  "type": "uint64"
+},
+{
+  "name": "io_win_size",
+  "type": "uint64"
+},
+{
+  "name": "mem_win_size",
+  "type": "uint64"
+},
+{
+  "name": "buid",
+  "type": "uint64"
+},
+{
+  "name": "io_win_addr",
+  "type": "uint64"
+},
+{
+  "name": "liobn",
+  "type": "uint32"
+},
+{
+  "name": "mem_win_addr",
+  "type": "uint64"
+},
+{
+  "name": "index",
+  "type": "uint32"
+}
+  ],
+  "id": "libvirt-41"
+}
+
+{
+  "return": [
+{
   "name": "ref405ep",
   "cpu-max": 1
 },
@@ -3878,7 +3924,7 @@
   "cpu-max": 255
 }
   ],
-  "id": "libvirt-41"
+  "id": "libvirt-42"
 }
 
 {
@@ -5180,19 +5226,19 @@
   "name": "MPC8541E_v11"
 }
   ],
-  "id": "libvirt-42"
+  "id": "libvirt-43"
 }
 
 {
   "return": [
   ],
-  "id": "libvirt-43"
+  "id": "libvirt-44"
 }
 
 {
   "return": [
   ],
-  "id": "libvirt-44"
+  "id": "libvirt-45"
 }
 
 {
@@ -6159,7 +6205,7 @@
   "option": "drive"
 }
   ],
-  "id": "libvirt-45"
+  "id": "libvirt-46"
 }
 
 {
@@ -6193,7 +6239,7 @@
   "capability": "postcopy-ram"
 }
   ],
-  "id": "libvirt-46"
+  "id": "libvirt-47"
 }
 
 {
@@ -14198,7 +14244,7 @@
   "meta-type": "array"
 }
   ],
-  "id": "libvirt-47"
+  "id": "libvirt-48"
 }
 
 {
diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.replies 
b/tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.replies
index 0c85f9e..0b286dc 100644
--- a/tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.replies
+++ b/tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.replies
@@ -4274,6 +4274,92 @@
 {
   "return": [
 {
+  "name": "dynamic-reconfiguration",
+  "type": "bool"
+},
+{
+  "name": "ddw",
+  "type": "bool"
+},
+{
+  "name": "dma_win_size",
+  "type": "uint64"
+},
+{
+  "name": "numa_node",
+  "type": "uint32"
+},
+{
+  "name": "pre-2.8-migration",
+  "type": "bool"
+},
+{
+  "name": "mem64_win_si