[PATCH] qemu: tpm: Enable creation of certs for TPM 1.2 in non-privileged mode

[Stefan Berger]

When 'swtpm_setup --print-capabilities' shows the 'tpm12-not-need-root'
flag, then it is possible to create certificates for the TPM 1.2 also
in non-privileged mode since swtpm_setup doesn't need tcsd anymore.
Check for this flag and create the certificates if this flag is found.

Signed-off-by: Stefan Berger 
---
 src/qemu/qemu_tpm.c | 5 -
 src/util/virtpm.c   | 1 +
 src/util/virtpm.h   | 1 +
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index e1b08a66c5..91e21ae646 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -463,11 +463,14 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
 if (!swtpm_setup)
 return -1;
 
-if (!privileged && tpmversion == VIR_DOMAIN_TPM_VERSION_1_2)
+if (!privileged && tpmversion == VIR_DOMAIN_TPM_VERSION_1_2 &&
+!virTPMSwtpmSetupCapsGet(
+VIR_TPM_SWTPM_SETUP_FEATURE_TPM12_NOT_NEED_ROOT)) {
 return virFileWriteStr(logfile,
_("Did not create EK and certificates since "
  "this requires privileged mode for a "
  "TPM 1.2\n"), 0600);
+}
 
 if (!privileged && qemuTPMCreateConfigFiles(swtpm_setup) < 0)
 return -1;
diff --git a/src/util/virtpm.c b/src/util/virtpm.c
index 0f50de866c..40d9272e66 100644
--- a/src/util/virtpm.c
+++ b/src/util/virtpm.c
@@ -46,6 +46,7 @@ VIR_ENUM_IMPL(virTPMSwtpmSetupFeature,
   VIR_TPM_SWTPM_SETUP_FEATURE_LAST,
   "cmdarg-pwdfile-fd",
   "cmdarg-create-config-files",
+  "tpm12-not-need-root",
 );
 
 /**
diff --git a/src/util/virtpm.h b/src/util/virtpm.h
index 3bb03b3b33..b75eb84f31 100644
--- a/src/util/virtpm.h
+++ b/src/util/virtpm.h
@@ -39,6 +39,7 @@ typedef enum {
 typedef enum {
 VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_PWDFILE_FD,
 VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_CREATE_CONFIG_FILES,
+VIR_TPM_SWTPM_SETUP_FEATURE_TPM12_NOT_NEED_ROOT,
 
 VIR_TPM_SWTPM_SETUP_FEATURE_LAST
 } virTPMSwtpmSetupFeature;
-- 
2.31.1

Re: [PATCH] qemu: Extend qemu.conf with PCR banks to activate during 'TPM manufacturing'

[Stefan Berger]

On 10/28/21 14:16, Daniel P. Berrangé wrote:

On Thu, Oct 28, 2021 at 01:51:33PM -0400, Stefan Berger wrote:


On the libvirt side, I think we could have a domain XML config option
for PCR banks, to allow the built-in default or admin local default to
be override per-VM.

Is there an example of an attribute that can only be set once in the domain
XML and cannot be modified after? The choice of active PCR banks is limited
to 'TPM manufacturing' time, which means swtpm_setup runs once only when the
swtpm's state directory does not exist because later it would overwrite the
entire state and erase all keys etc.. Later manipulations of the PCR banks
would have to be done using the firmware menu, which exist in EDK2, SeaBIOS
and SLOF.

Yeah, it is a little unusual, but then I guess we have the similarish
with other firmware selection, where setting "secure=yes|no" determines
which OVMF binary we pick to use.



I will probably add a new feature (for swtpm v0.7) to be able to 
reconfigure the active pcr banks. The availability of this feature can 
be detected by libvirt via the JSON that swtpm_setup 
--print-capabilities returns (as usual). Now the problems are:


- What to do when an older version of swtpm package is installed 
regarding the contents of the XML? Reject the pcr banks one can declare 
in the domain XML? The other option would be to allow the XML but not to 
react to it at all and document that one needs swptm v0.7 or later which 
will probably be the case in most setups sooner or later.


- How would one track changes to the XML versus the state of the swtpm? 
At the moment I would run the reconfigure script ever time if a set of 
active PCR banks was given in the XML and it would log like shown below. 
Should we just turn off the logging (no --log  option) for 
when doing the '--reconfigure'? Or still log it? Or could we assume the 
user will remove the active PCR banks description from the XML to avoid 
the running of swtpm_setup every time to reconfigure (probably not)?


$ swtpm_setup --tpmstate ./ --tpm2 --reconfigure --pcr-banks sha1
Starting vTPM reconfiguration as stefanb:stefanb @ Fri 29 Oct 2021 
03:23:59 PM EDT

TPM is listening on Unix socket.
Successfully activated PCR banks sha1 among sha1,sha256,sha384,sha512.
Successfully authored TPM state.
Ending vTPM manufacturing @ Fri 29 Oct 2021 03:23:59 PM EDT

The only concern is a log full of these messages.


The alternative is to configuring the active PCR banks on the 
swtpm_setup level via swtpm_setup.conf and default compile-time options 
and leave the reconfiguration to using the firmware...


  Stefan

[libvirt PATCH v2 0/3] PCI VPD: Handle More Edge Cases

[Dmitrii Shcherbakov]

This patch set improves edge case testing:

* The parser is now more strict about checking boundary conditions when
  parsing fields: an invalid field length is a possibility which is
now being accounted for;
* The parser will now make sure that RV and RW fields are the last
  in their section by making sure that no more data is left to read
after those;
* The RW field in the read-write section is not considered a VPD format
  violation even though it is a violation of the spec since it does not
  prevent Libvirt from parsing valid data for presenting it to a user.
  This is a policy decision made by Libvirt in favor of usability with
  hardware that does not strictly follow the PCI/PCIe VPD spec.

Invalid field values are now skipped instead of halting further parsing
completely.

Some vendors use 0xFF as placeholders in VPD-W since those values do not
correspond to printable ASCII characters, they will be discarded,
however, parsing will continue beyond that point.

Also, it turns out that some vendors use printable ASCII characters not
present in the alphanumeric range. Following a mailing list discussion
Libvirt will accept printable ASCII characters to avoid cases where
useful data is discarded.

https://listman.redhat.com/archives/libvir-list/2021-October/msg01043.html

Higher-level software needs to account for this character set and act
accordingly.

For example, the outcome of this is that one may get "N/A" as a value
for a serial number that is supposed to be unique, however, there is
no way for Libvirt to validate serial number uniqueness anyway even if
it was a different character sequence.

https://gitlab.com/dmitriis/libvirt/-/pipelines/398517951
(x86_64 only, have not set up arch-specific runners yet and over the
limit of what gitlab provides)

Dmitrii Shcherbakov (3):
  PCI VPD: handle additional edge cases
  PCI VPD: Skip fields with invalid values
  PCI VPD: Fix a wrong return code in a test case

 src/util/virpcivpd.c  |  63 +++---
 tests/virpcivpdtest.c | 263 --
 2 files changed, 296 insertions(+), 30 deletions(-)

-- 
2.32.0

[libvirt PATCH v2 2/3] PCI VPD: Skip fields with invalid values

[Dmitrii Shcherbakov]

While invalid values need to be ignored when presenting VPD data to the
user, it would be good to attempt to parse a valid portion of the VPD
instead of marking it invalid as a whole.

Based on a mailing list discussion, the set of accepted characters is
extended to the set of printable ASCII characters.

https://listman.redhat.com/archives/libvir-list/2021-October/msg01043.html

The particular example encountered on real hardware was multi-faceted:

* "N/A" strings present in read-only fields. This would not be a useful
  valid value for a field (especially if a unique serial number is
  expected), however, it was decided to delegate handling of those kinds
  of values to higher-level software;
* "4W/1W PCIeG2x4" - looks like some vendors use even more printable
  characters in the ASCII range than we currently allow. Since the
  PCI/PCIe VPD specs mention alphanumeric characters without specifying
  the full character set, it looks like this is ambiguous for vendors
  and they tend to use printable ASCII characters;
* 0xFF bytes present in VPD-W field values. Those bytes do not map to
  printable ASCII code points and were probably used by the vendor as
  placeholders. Ignoring the whole VPD because of that would be too
  strict.

Signed-off-by: Dmitrii Shcherbakov 
---
 src/util/virpcivpd.c  |  22 +---
 tests/virpcivpdtest.c | 125 ++
 2 files changed, 127 insertions(+), 20 deletions(-)

diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c
index 4c96bc1a06..d8f2a43cde 100644
--- a/src/util/virpcivpd.c
+++ b/src/util/virpcivpd.c
@@ -161,8 +161,6 @@ virPCIVPDResourceGetFieldValueFormat(const char *keyword)
 return format;
 }
 
-#define ACCEPTED_CHARS 
"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789 -_,.:;="
-
 /**
  * virPCIVPDResourceIsValidTextValue:
  * @value: A NULL-terminated string to assess.
@@ -175,6 +173,7 @@ virPCIVPDResourceGetFieldValueFormat(const char *keyword)
 bool
 virPCIVPDResourceIsValidTextValue(const char *value)
 {
+size_t i = 0;
 /*
  * The PCI(e) specs mention alphanumeric characters when talking about 
text fields
  * and the string resource but also include spaces and dashes in the 
provided example.
@@ -191,10 +190,12 @@ virPCIVPDResourceIsValidTextValue(const char *value)
 if (STREQ(value, ""))
 return true;
 
-if (strspn(value, ACCEPTED_CHARS) != strlen(value)) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("The provided value contains invalid characters: 
%s"), value);
-return false;
+while (i < strlen(value)) {
+if (!g_ascii_isprint(value[i])) {
+VIR_DEBUG("The provided value contains non-ASCII printable 
characters: %s", value);
+return false;
+}
+++i;
 }
 return true;
 }
@@ -544,9 +545,12 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, 
uint16_t resPos, uint16_t re
  */
 fieldValue = g_strstrip(g_strndup((char *)buf, fieldDataLen));
 if (!virPCIVPDResourceIsValidTextValue(fieldValue)) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Field value contains invalid characters"));
-return false;
+/* Skip fields with invalid values - this is safe assuming 
field length is
+ * correctly specified. */
+VIR_DEBUG("A value for field %s contains invalid characters", 
fieldKeyword);
+g_free(g_steal_pointer());
+g_free(g_steal_pointer());
+continue;
 }
 } else if (fieldFormat == 
VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RESVD) {
 if (*csum) {
diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c
index a99bde2b92..b7bc86d922 100644
--- a/tests/virpcivpdtest.c
+++ b/tests/virpcivpdtest.c
@@ -322,8 +322,10 @@ testPCIVPDIsValidTextValue(const void *data G_GNUC_UNUSED)
 {"under_score_example", true},
 {"", true},
 {";", true},
-{"\\42", false},
-{"/42", false},
+{"\\42", true},
+{"N/A", true},
+/* The first and last code points are outside ASCII (multi-byte in 
UTF-8). */
+{"гbl", false},
 };
 for (i = 0; i < sizeof(textValueCases) / sizeof(textValueCases[0]); ++i) {
 if (virPCIVPDResourceIsValidTextValue(textValueCases[i].keyword) !=
@@ -741,6 +743,113 @@ testVirPCIVPDParseFullVPDSkipInvalidKeywords(const void 
*opaque G_GNUC_UNUSED)
 return 0;
 }
 
+static int
+testVirPCIVPDParseFullVPDSkipInvalidValues(const void *opaque G_GNUC_UNUSED)
+{
+int fd = -1;
+size_t dataLen = 0;
+size_t i = 0;
+virPCIVPDResourceCustom *custom = NULL;
+
+g_autoptr(virPCIVPDResource) res = NULL;
+
+/* This example is based on real-world hardware which was programmed by 
the vendor with
+ * invalid field values in both the RO section and RW section. The RO 

[libvirt PATCH v2 3/3] PCI VPD: Fix a wrong return code in a test case

[Dmitrii Shcherbakov]

The test case should return -1, not 0 in case a valid resource could
not be parsed successfully but the ret value is initialized to 0.

Signed-off-by: Dmitrii Shcherbakov 
---
 tests/virpcivpdtest.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c
index b7bc86d922..a9405f9427 100644
--- a/tests/virpcivpdtest.c
+++ b/tests/virpcivpdtest.c
@@ -537,7 +537,6 @@ testVirPCIVPDParseFullVPD(const void *opaque G_GNUC_UNUSED)
 {
 int fd = -1;
 size_t dataLen = 0;
-int ret = 0;
 
 g_autoptr(virPCIVPDResource) res = NULL;
 /* Note: Custom fields are supposed to be freed by the resource cleanup 
code. */
@@ -558,7 +557,7 @@ testVirPCIVPDParseFullVPD(const void *opaque G_GNUC_UNUSED)
 if (!res) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
"The resource pointer is NULL after parsing which is 
unexpected");
-return ret;
+return -1;
 }
 
 if (!res->ro) {
@@ -596,7 +595,7 @@ testVirPCIVPDParseFullVPD(const void *opaque G_GNUC_UNUSED)
 return -1;
 
 custom = NULL;
-return ret;
+return 0;
 }
 
 static int
-- 
2.32.0

[libvirt PATCH v2 1/3] PCI VPD: handle additional edge cases

[Dmitrii Shcherbakov]

* RV and RW fields must be at the last position in their respective
  section (per the conditions in the spec). Therefore, the parser now
  stops iterating over fields as soon as it encounters one of those
  fields and checks whether the end of the resource has been reached;
* The lack of the RW field is not treated as a parsing error since we
  can still extract valid data even though this is a PCI/PCIe VPD spec
  violation;
* Individual fields must have a valid length - the parser needs to check
  for invalid length values that violate boundary conditions of the
  resource.
* A zero-length field may be the last one in the resource, however, the
  boundary check is currently too strict to allow that.

Signed-off-by: Dmitrii Shcherbakov 
---
 src/util/virpcivpd.c  |  41 ++---
 tests/virpcivpdtest.c | 137 ++
 2 files changed, 169 insertions(+), 9 deletions(-)

diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c
index 8856bca459..4c96bc1a06 100644
--- a/src/util/virpcivpd.c
+++ b/src/util/virpcivpd.c
@@ -466,8 +466,12 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, 
uint16_t resPos, uint16_t re
 
 bool hasChecksum = false;
 bool hasRW = false;
+bool endReached = false;
 
-while (fieldPos + 3 < resPos + resDataLen) {
+/* Note the equal sign - fields may have a zero length in which case they 
will
+ * just occupy 3 header bytes. In the in case of the RW field this may 
mean that
+ * no more space is left in the section. */
+while (fieldPos + 3 <= resPos + resDataLen) {
 /* Keyword resources consist of keywords (2 ASCII bytes per the spec) 
and 1-byte length. */
 if (virPCIVPDReadVPDBytes(vpdFileFd, buf, 3, fieldPos, csum) != 3) {
 /* Invalid field encountered which means the resource itself is 
invalid too. Report
@@ -518,6 +522,13 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, 
uint16_t resPos, uint16_t re
 return false;
 }
 
+if (resPos + resDataLen < fieldPos + fieldDataLen) {
+/* In this case the field cannot simply be skipped since the 
position of the
+ * next field is determined based on the length of a previous 
field. */
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("A field data length violates the resource length 
boundary."));
+return false;
+}
 if (virPCIVPDReadVPDBytes(vpdFileFd, buf, bytesToRead, fieldPos, csum) 
!= bytesToRead) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Could not parse a resource field data - VPD has 
invalid format"));
@@ -546,12 +557,13 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, 
uint16_t resPos, uint16_t re
 hasChecksum = true;
 g_free(g_steal_pointer());
 g_free(g_steal_pointer());
-continue;
+break;
 } else if (fieldFormat == 
VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RDWR) {
 /* Skip the read-write space since it is used for indication only. 
*/
 hasRW = true;
 g_free(g_steal_pointer());
 g_free(g_steal_pointer());
+break;
 } else if (fieldFormat == 
VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_LAST) {
 /* Skip unknown fields */
 g_free(g_steal_pointer());
@@ -579,14 +591,25 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, 
uint16_t resPos, uint16_t re
 g_free(g_steal_pointer());
 g_free(g_steal_pointer());
 }
-if (readOnly && !hasChecksum) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("VPD-R does not contain the mandatory RV field"));
-return false;
-} else if (!readOnly && !hasRW) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("VPD-W does not contain the mandatory RW field"));
+
+/* May have exited the loop prematurely in case RV or RW were encountered 
and
+ * they were not the last fields in the section. */
+endReached = (fieldPos >= resPos + resDataLen);
+if (readOnly && !(hasChecksum && endReached)) {
+VIR_DEBUG("VPD-R does not contain the mandatory RV field as the last 
field");
 return false;
+} else if (!readOnly && !endReached) {
+/* The lack of RW is allowed on purpose in the read-write section 
since some vendors
+ * violate the PCI/PCIe specs and do not include it, however, this 
does not prevent parsing
+ * of valid data. If the RW is present, however, we make sure it is 
the last field in
+ * the read-write section. */
+if (hasRW) {
+VIR_DEBUG("VPD-W section parsing ended prematurely (RW is not the 
last field).");
+return false;
+} else {
+VIR_DEBUG("VPD-W section parsing ended prematurely.");
+return false;
+}
 }
 
 return true;
diff --git 

Re: [PATCH v2 8/9] qapi: Factor out compat_policy_input_ok()

[Philippe Mathieu-Daudé]

On 10/29/21 18:55, Markus Armbruster wrote:
> Markus Armbruster  writes:
> 
>> The code to check policy for handling deprecated input is triplicated.
>> Factor it out into compat_policy_input_ok() before I mess with it in
>> the next commit.
>>
>> Signed-off-by: Markus Armbruster 
>> Reviewed-by: Philippe Mathieu-Daudé 
>> ---
>>  include/qapi/compat-policy.h |  7 +
>>  qapi/qapi-visit-core.c   | 18 +
>>  qapi/qmp-dispatch.c  | 51 +++-
>>  qapi/qobject-input-visitor.c | 19 +++---
>>  4 files changed, 55 insertions(+), 40 deletions(-)

>> +bool compat_policy_input_ok(unsigned special_features,
>> +const CompatPolicy *policy,
>> +ErrorClass error_class,
>> +const char *kind, const char *name,
>> +Error **errp)
>> +{
>> +if ((special_features & 1u << QAPI_DEPRECATED)
>> +&& !compat_policy_input_ok1("Deprecated",
>> +policy->deprecated_input,
>> +error_class, kind, name, errp)) {
>> +return false;
>> +}
>> +return true;
>> +}
>> +
>>  Visitor *qobject_input_visitor_new_qmp(QObject *obj)
>>  {
>>  Visitor *v = qobject_input_visitor_new(obj);
> 
> I'm moving the new functions along with @compat_policy to qapi-util.c,
> so the QAPI visitors survive linking without qmp-dispatch.o.

OK. I am waiting your series to get in before respining my QAPI
sysemu/user series which will re-scramble meson.build here.
Probably too late for this release. Not super important, it just
saves energy avoiding generating / building unused files.

Re: [PATCH v2 8/9] qapi: Factor out compat_policy_input_ok()

[Markus Armbruster]

Markus Armbruster  writes:

> The code to check policy for handling deprecated input is triplicated.
> Factor it out into compat_policy_input_ok() before I mess with it in
> the next commit.
>
> Signed-off-by: Markus Armbruster 
> Reviewed-by: Philippe Mathieu-Daudé 
> ---
>  include/qapi/compat-policy.h |  7 +
>  qapi/qapi-visit-core.c   | 18 +
>  qapi/qmp-dispatch.c  | 51 +++-
>  qapi/qobject-input-visitor.c | 19 +++---
>  4 files changed, 55 insertions(+), 40 deletions(-)
>
> diff --git a/include/qapi/compat-policy.h b/include/qapi/compat-policy.h
> index 1083f95122..8b7b25c0b5 100644
> --- a/include/qapi/compat-policy.h
> +++ b/include/qapi/compat-policy.h
> @@ -13,10 +13,17 @@
>  #ifndef QAPI_COMPAT_POLICY_H
>  #define QAPI_COMPAT_POLICY_H
>  
> +#include "qapi/error.h"
>  #include "qapi/qapi-types-compat.h"
>  
>  extern CompatPolicy compat_policy;
>  
> +bool compat_policy_input_ok(unsigned special_features,
> +const CompatPolicy *policy,
> +ErrorClass error_class,
> +const char *kind, const char *name,
> +Error **errp);
> +
>  /*
>   * Create a QObject input visitor for @obj for use with QMP
>   *
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 34c59286b2..6c13510a2b 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -13,6 +13,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qapi/compat-policy.h"
>  #include "qapi/error.h"
>  #include "qapi/qmp/qerror.h"
>  #include "qapi/visitor.h"
> @@ -409,18 +410,11 @@ static bool input_type_enum(Visitor *v, const char 
> *name, int *obj,
>  }
>  
>  if (lookup->special_features
> -&& (lookup->special_features[value] & QAPI_DEPRECATED)) {
> -switch (v->compat_policy.deprecated_input) {
> -case COMPAT_POLICY_INPUT_ACCEPT:
> -break;
> -case COMPAT_POLICY_INPUT_REJECT:
> -error_setg(errp, "Deprecated value '%s' disabled by policy",
> -   enum_str);
> -return false;
> -case COMPAT_POLICY_INPUT_CRASH:
> -default:
> -abort();
> -}
> +&& !compat_policy_input_ok(lookup->special_features[value],
> +   >compat_policy,
> +   ERROR_CLASS_GENERIC_ERROR,
> +   "value", enum_str, errp)) {
> +return false;
>  }
>  
>  *obj = value;
> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> index 8cca18c891..e29ade134c 100644
> --- a/qapi/qmp-dispatch.c
> +++ b/qapi/qmp-dispatch.c
> @@ -28,6 +28,40 @@
>  
>  CompatPolicy compat_policy;
>  
> +static bool compat_policy_input_ok1(const char *adjective,
> +CompatPolicyInput policy,
> +ErrorClass error_class,
> +const char *kind, const char *name,
> +Error **errp)
> +{
> +switch (policy) {
> +case COMPAT_POLICY_INPUT_ACCEPT:
> +return true;
> +case COMPAT_POLICY_INPUT_REJECT:
> +error_set(errp, error_class, "%s %s %s disabled by policy",
> +  adjective, kind, name);
> +return false;
> +case COMPAT_POLICY_INPUT_CRASH:
> +default:
> +abort();
> +}
> +}
> +
> +bool compat_policy_input_ok(unsigned special_features,
> +const CompatPolicy *policy,
> +ErrorClass error_class,
> +const char *kind, const char *name,
> +Error **errp)
> +{
> +if ((special_features & 1u << QAPI_DEPRECATED)
> +&& !compat_policy_input_ok1("Deprecated",
> +policy->deprecated_input,
> +error_class, kind, name, errp)) {
> +return false;
> +}
> +return true;
> +}
> +
>  Visitor *qobject_input_visitor_new_qmp(QObject *obj)
>  {
>  Visitor *v = qobject_input_visitor_new(obj);

I'm moving the new functions along with @compat_policy to qapi-util.c,
so the QAPI visitors survive linking without qmp-dispatch.o.

> @@ -176,19 +210,10 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject 
> *request,
>"The command %s has not been found", command);
>  goto out;
>  }
> -if (cmd->special_features & 1u << QAPI_DEPRECATED) {
> -switch (compat_policy.deprecated_input) {
> -case COMPAT_POLICY_INPUT_ACCEPT:
> -break;
> -case COMPAT_POLICY_INPUT_REJECT:
> -error_set(, ERROR_CLASS_COMMAND_NOT_FOUND,
> -  "Deprecated command %s disabled by policy",
> -  command);
> -goto out;
> -case COMPAT_POLICY_INPUT_CRASH:
> -default:
> -

Re: [PATCH v2 5/9] qapi: Generalize struct member policy checking

[Philippe Mathieu-Daudé]

On 10/29/21 17:34, Markus Armbruster wrote:
> Eric Blake  writes:
> 
>> On Thu, Oct 28, 2021 at 12:25:16PM +0200, Markus Armbruster wrote:
>>> The generated visitor functions call visit_deprecated_accept() and
>>> visit_deprecated() when visiting a struct member with special feature
>>> flag 'deprecated'.  This makes the feature flag visible to the actual
>>> visitors.  I want to make feature flag 'unstable' visible there as
>>> well, so I can add policy for it.
>>>
>>> To let me make it visible, replace these functions by
>>> visit_policy_reject() and visit_policy_skip(), which take the member's
>>> special features as an argument.  Note that the new functions have the
>>> opposite sense, i.e. the return value flips.
>>>
>>> Signed-off-by: Markus Armbruster 
>>> ---
>>
>>> +++ b/qapi/qapi-forward-visitor.c
>>> @@ -246,25 +246,27 @@ static void forward_field_optional(Visitor *v, const 
>>> char *name, bool *present)
>>>  visit_optional(ffv->target, name, present);
>>>  }
>>>  
>>> -static bool forward_field_deprecated_accept(Visitor *v, const char *name,
>>> -Error **errp)
>>> +static bool forward_field_policy_reject(Visitor *v, const char *name,
>>> +unsigned special_features,
>>> +Error **errp)
>>>  {
>>>  ForwardFieldVisitor *ffv = to_ffv(v);
>>>  
>>>  if (!forward_field_translate_name(ffv, , errp)) {
>>>  return false;
>>
>> Should this return value be flipped?
>>
>>>  }
>>> -return visit_deprecated_accept(ffv->target, name, errp);
>>> +return visit_policy_reject(ffv->target, name, special_features, errp);
>>>  }
>>>  
>>> -static bool forward_field_deprecated(Visitor *v, const char *name)
>>> +static bool forward_field_policy_skip(Visitor *v, const char *name,
>>> +  unsigned special_features)
>>>  {
>>>  ForwardFieldVisitor *ffv = to_ffv(v);
>>>  
>>>  if (!forward_field_translate_name(ffv, , NULL)) {
>>>  return false;
>>
>> and here too?
> 
> Good catch!

Ouch. I admit this patch logic is hard to review, but couldn't come
with a nice way to split it further.

> These functions are called indirectly like
> 
> if (visit_policy_reject(v, "values", 1u << QAPI_DEPRECATED, errp)) {
> return false;
> }
> if (!visit_policy_skip(v, "values", 1u << QAPI_DEPRECATED)) {
> if (!visit_type_strList(v, "values", >values, errp)) {
> return false;
> }
> }
> 
> visit_policy_reject() must return true when it sets an error, or else we
> call visit_policy_skip() with @errp pointing to non-null.
> 
> Same argument for visit_policy_skip().
> 
>>>  }
>>> -return visit_deprecated(ffv->target, name);
>>> +return visit_policy_skip(ffv->target, name, special_features);
>>>  }
>>>  
>>
>> Otherwise, the rest of the logic changes for flipped sense look right.
> 

Re: [PATCH v2 5/9] qapi: Generalize struct member policy checking

[Philippe Mathieu-Daudé]

On 10/29/21 16:01, Markus Armbruster wrote:
> Philippe Mathieu-Daudé  writes:
> 
>> On 10/28/21 12:25, Markus Armbruster wrote:
>>> The generated visitor functions call visit_deprecated_accept() and
>>> visit_deprecated() when visiting a struct member with special feature
>>> flag 'deprecated'.  This makes the feature flag visible to the actual
>>> visitors.  I want to make feature flag 'unstable' visible there as
>>> well, so I can add policy for it.
>>>
>>> To let me make it visible, replace these functions by
>>> visit_policy_reject() and visit_policy_skip(), which take the member's
>>> special features as an argument.  Note that the new functions have the
>>> opposite sense, i.e. the return value flips.
>>>
>>> Signed-off-by: Markus Armbruster 
>>> ---
>>>  include/qapi/visitor-impl.h   |  6 --
>>>  include/qapi/visitor.h| 17 +
>>>  qapi/qapi-forward-visitor.c   | 16 +---
>>>  qapi/qapi-visit-core.c| 22 --
>>>  qapi/qobject-input-visitor.c  | 15 ++-
>>>  qapi/qobject-output-visitor.c |  9 ++---
>>>  qapi/trace-events |  4 ++--
>>>  scripts/qapi/visit.py | 14 +++---
>>>  8 files changed, 63 insertions(+), 40 deletions(-)

>>>  case COMPAT_POLICY_INPUT_CRASH:
>>
>> Clearer as:
>>
>>abort();
>>default:
>>g_assert_not_reached();
> 
> Maybe, but making it so has nothing to do with this patch.  It could
> perhaps be done in PATCH 8, or in a followup patch.
> 
>> Otherwise,
>> Reviewed-by: Philippe Mathieu-Daudé 
> 
> Okay to tack your R-by to the unmodified patch?

Sure.

Re: [PATCH v2 6/9] qapi: Generalize command policy checking

[Philippe Mathieu-Daudé]

On 10/29/21 17:28, Eric Blake wrote:
> On Thu, Oct 28, 2021 at 12:25:17PM +0200, Markus Armbruster wrote:
>> The code to check command policy can see special feature flag
>> 'deprecated' as command flag QCO_DEPRECATED.  I want to make feature
>> flag 'unstable' visible there as well, so I can add policy for it.
>>
>> To let me make it visible, add member @special_features (a bitset of
>> QapiSpecialFeature) to QmpCommand, and adjust the generator to pass it
>> through qmp_register_command().  Then replace "QCO_DEPRECATED in
>> @flags" by QAPI_DEPRECATED in @special_features", and drop
>> QCO_DEPRECATED.
>>
>> Signed-off-by: Markus Armbruster 
>> Reviewed-by: Philippe Mathieu-Daudé 
>> Acked-by: John Snow 
>> ---
> 
>> +++ b/qapi/qmp-dispatch.c
>> @@ -176,7 +176,7 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject 
>> *request,
>>"The command %s has not been found", command);
>>  goto out;
>>  }
>> -if (cmd->options & QCO_DEPRECATED) {
>> +if (cmd->special_features & 1u << QAPI_DEPRECATED) {
> 
> I admit having to check the C operator precedence table when reading

This doesn't seem a good use of (y)our time. Using a pair of parenthesis
is simpler.

I expect in a not far future that compilers emit a warning for this.

> this (<< is higher than &); if writing it myself, I would probably
> have used explicit () to avoid reviewer confusion, but what you have
> is correct.  (After grepping for ' & 1.*<<' and ' & (1.*<<', it looks
> like authors using explicit precedence happens more often, but that
> there are other instances in the code base relying on implicit
> precedence.)

$ git grep -E ' & [0-9a-zA-Z_]+ <<'
hw/dma/pl330.c:997:if (~ch->parent->inten & ch->parent->ev_status &
1 << ev_id) {
hw/dma/xlnx-zynq-devcfg.c:198:if (s->regs[R_LOCK] & 1 << i) {
hw/intc/grlib_irqmp.c:144:if (s->broadcast & 1 << irq) {
hw/net/fsl_etsec/rings.c:491:if (etsec->regs[RSTAT].value & 1 << (23
- ring_nbr)) {
hw/net/virtio-net.c:748:(n->host_features & 1ULL <<
VIRTIO_NET_F_MTU)) {
hw/pci-host/mv64361.c:812:if ((ch & 0xff << i) && !(val
& 0xff << i)) {
hw/pci-host/mv64361.c:858:if (s->gpp_int_level && !(val & 0xff
<< b)) {
hw/ssi/xilinx_spi.c:123:qemu_set_irq(s->cs_lines[i],
!(~s->regs[R_SPISSR] & 1 << i));
hw/ssi/xilinx_spips.c:441:r[idx[!d]] |= x[idx[d]] & 1 <<
bit[d] ? 1 << bit[!d] : 0;
target/s390x/cpu_features.c:56:if (init[i] & 1ULL << j) {
tests/qtest/bios-tables-test.c:209:if (!(val & 1UL << 20 /*
HW_REDUCED_ACPI */)) {

Not that many.

Re: [PATCH v2 5/9] qapi: Generalize struct member policy checking

[Markus Armbruster]

Eric Blake  writes:

> On Thu, Oct 28, 2021 at 12:25:16PM +0200, Markus Armbruster wrote:
>> The generated visitor functions call visit_deprecated_accept() and
>> visit_deprecated() when visiting a struct member with special feature
>> flag 'deprecated'.  This makes the feature flag visible to the actual
>> visitors.  I want to make feature flag 'unstable' visible there as
>> well, so I can add policy for it.
>> 
>> To let me make it visible, replace these functions by
>> visit_policy_reject() and visit_policy_skip(), which take the member's
>> special features as an argument.  Note that the new functions have the
>> opposite sense, i.e. the return value flips.
>> 
>> Signed-off-by: Markus Armbruster 
>> ---
>
>> +++ b/qapi/qapi-forward-visitor.c
>> @@ -246,25 +246,27 @@ static void forward_field_optional(Visitor *v, const 
>> char *name, bool *present)
>>  visit_optional(ffv->target, name, present);
>>  }
>>  
>> -static bool forward_field_deprecated_accept(Visitor *v, const char *name,
>> -Error **errp)
>> +static bool forward_field_policy_reject(Visitor *v, const char *name,
>> +unsigned special_features,
>> +Error **errp)
>>  {
>>  ForwardFieldVisitor *ffv = to_ffv(v);
>>  
>>  if (!forward_field_translate_name(ffv, , errp)) {
>>  return false;
>
> Should this return value be flipped?
>
>>  }
>> -return visit_deprecated_accept(ffv->target, name, errp);
>> +return visit_policy_reject(ffv->target, name, special_features, errp);
>>  }
>>  
>> -static bool forward_field_deprecated(Visitor *v, const char *name)
>> +static bool forward_field_policy_skip(Visitor *v, const char *name,
>> +  unsigned special_features)
>>  {
>>  ForwardFieldVisitor *ffv = to_ffv(v);
>>  
>>  if (!forward_field_translate_name(ffv, , NULL)) {
>>  return false;
>
> and here too?

Good catch!

These functions are called indirectly like

if (visit_policy_reject(v, "values", 1u << QAPI_DEPRECATED, errp)) {
return false;
}
if (!visit_policy_skip(v, "values", 1u << QAPI_DEPRECATED)) {
if (!visit_type_strList(v, "values", >values, errp)) {
return false;
}
}

visit_policy_reject() must return true when it sets an error, or else we
call visit_policy_skip() with @errp pointing to non-null.

Same argument for visit_policy_skip().

>>  }
>> -return visit_deprecated(ffv->target, name);
>> +return visit_policy_skip(ffv->target, name, special_features);
>>  }
>>  
>
> Otherwise, the rest of the logic changes for flipped sense look right.

Re: [PATCH v2 6/9] qapi: Generalize command policy checking

[Eric Blake]

On Thu, Oct 28, 2021 at 12:25:17PM +0200, Markus Armbruster wrote:
> The code to check command policy can see special feature flag
> 'deprecated' as command flag QCO_DEPRECATED.  I want to make feature
> flag 'unstable' visible there as well, so I can add policy for it.
> 
> To let me make it visible, add member @special_features (a bitset of
> QapiSpecialFeature) to QmpCommand, and adjust the generator to pass it
> through qmp_register_command().  Then replace "QCO_DEPRECATED in
> @flags" by QAPI_DEPRECATED in @special_features", and drop
> QCO_DEPRECATED.
> 
> Signed-off-by: Markus Armbruster 
> Reviewed-by: Philippe Mathieu-Daudé 
> Acked-by: John Snow 
> ---

> +++ b/qapi/qmp-dispatch.c
> @@ -176,7 +176,7 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject 
> *request,
>"The command %s has not been found", command);
>  goto out;
>  }
> -if (cmd->options & QCO_DEPRECATED) {
> +if (cmd->special_features & 1u << QAPI_DEPRECATED) {

I admit having to check the C operator precedence table when reading
this (<< is higher than &); if writing it myself, I would probably
have used explicit () to avoid reviewer confusion, but what you have
is correct.  (After grepping for ' & 1.*<<' and ' & (1.*<<', it looks
like authors using explicit precedence happens more often, but that
there are other instances in the code base relying on implicit
precedence.)

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 5/9] qapi: Generalize struct member policy checking

[Eric Blake]

On Thu, Oct 28, 2021 at 12:25:16PM +0200, Markus Armbruster wrote:
> The generated visitor functions call visit_deprecated_accept() and
> visit_deprecated() when visiting a struct member with special feature
> flag 'deprecated'.  This makes the feature flag visible to the actual
> visitors.  I want to make feature flag 'unstable' visible there as
> well, so I can add policy for it.
> 
> To let me make it visible, replace these functions by
> visit_policy_reject() and visit_policy_skip(), which take the member's
> special features as an argument.  Note that the new functions have the
> opposite sense, i.e. the return value flips.
> 
> Signed-off-by: Markus Armbruster 
> ---

> +++ b/qapi/qapi-forward-visitor.c
> @@ -246,25 +246,27 @@ static void forward_field_optional(Visitor *v, const 
> char *name, bool *present)
>  visit_optional(ffv->target, name, present);
>  }
>  
> -static bool forward_field_deprecated_accept(Visitor *v, const char *name,
> -Error **errp)
> +static bool forward_field_policy_reject(Visitor *v, const char *name,
> +unsigned special_features,
> +Error **errp)
>  {
>  ForwardFieldVisitor *ffv = to_ffv(v);
>  
>  if (!forward_field_translate_name(ffv, , errp)) {
>  return false;

Should this return value be flipped?

>  }
> -return visit_deprecated_accept(ffv->target, name, errp);
> +return visit_policy_reject(ffv->target, name, special_features, errp);
>  }
>  
> -static bool forward_field_deprecated(Visitor *v, const char *name)
> +static bool forward_field_policy_skip(Visitor *v, const char *name,
> +  unsigned special_features)
>  {
>  ForwardFieldVisitor *ffv = to_ffv(v);
>  
>  if (!forward_field_translate_name(ffv, , NULL)) {
>  return false;

and here too?

>  }
> -return visit_deprecated(ffv->target, name);
> +return visit_policy_skip(ffv->target, name, special_features);
>  }
>  

Otherwise, the rest of the logic changes for flipped sense look right.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 9/9] qapi: Extend -compat to set policy for unstable interfaces

[Markus Armbruster]

Eric Blake  writes:

> On Thu, Oct 28, 2021 at 12:25:20PM +0200, Markus Armbruster wrote:
>> New option parameters unstable-input and unstable-output set policy
>> for unstable interfaces just like deprecated-input and
>> deprecated-output set policy for deprecated interfaces (see commit
>> 6dd75472d5 "qemu-options: New -compat to set policy for deprecated
>> interfaces").  This is intended for testing users of the management
>> interfaces.  It is experimental.
>> 
>> For now, this covers only syntactic aspects of QMP, i.e. stuff tagged
>> with feature 'unstable'.  We may want to extend it to cover semantic
>> aspects, or the command line.
>> 
>> Note that there is no good way for management application to detect
>> presence of these new option parameters: they are not visible output
>> of query-qmp-schema or query-command-line-options.  Tolerable, because
>> it's meant for testing.  If running with -compat fails, skip the test.
>
> Not to mention, once we finish QAPIfying the command line, we could
> make sure it is visible through introspection at that time (it may
> require tagging the command line option with a feature, if nothing
> else makes it pop through).
>
>> 
>> Signed-off-by: Markus Armbruster 
>> Acked-by: John Snow 
>> ---
>>  qapi/compat.json  |  6 +-
>>  include/qapi/util.h   |  1 +
>>  qapi/qmp-dispatch.c   |  6 ++
>>  qapi/qobject-output-visitor.c |  8 ++--
>>  qemu-options.hx   | 20 +++-
>>  scripts/qapi/events.py| 10 ++
>>  scripts/qapi/schema.py| 10 ++
>>  7 files changed, 49 insertions(+), 12 deletions(-)
>> 
>> diff --git a/qapi/compat.json b/qapi/compat.json
>> index 74a8493d3d..9bc9804abb 100644
>> --- a/qapi/compat.json
>> +++ b/qapi/compat.json
>> @@ -47,9 +47,13 @@
>>  #
>>  # @deprecated-input: how to handle deprecated input (default 'accept')
>>  # @deprecated-output: how to handle deprecated output (default 'accept')
>> +# @unstable-input: how to handle unstable input (default 'accept')
>> +# @unstable-output: how to handle unstable output (default 'accept')
>
> Missing '(since 6.2)' doc tags on the two new policies.

Fixing...

>>  #
>>  # Since: 6.0
>>  ##
>>  { 'struct': 'CompatPolicy',
>>'data': { '*deprecated-input': 'CompatPolicyInput',
>> -'*deprecated-output': 'CompatPolicyOutput' } }
>> +'*deprecated-output': 'CompatPolicyOutput',
>> +'*unstable-input': 'CompatPolicyInput',
>> +'*unstable-output': 'CompatPolicyOutput' } }
>> diff --git a/include/qapi/util.h b/include/qapi/util.h
>> index 0cc98db9f9..81a2b13a33 100644
>> --- a/include/qapi/util.h
>> +++ b/include/qapi/util.h
>> @@ -13,6 +13,7 @@
>>  
>>  typedef enum {
>>  QAPI_DEPRECATED,
>> +QAPI_UNSTABLE,
>>  } QapiSpecialFeature;
>
>> +++ b/qemu-options.hx
>> @@ -3641,7 +3641,9 @@ DEFHEADING(Debug/Expert options:)
>>  
>>  DEF("compat", HAS_ARG, QEMU_OPTION_compat,
>>  "-compat 
>> [deprecated-input=accept|reject|crash][,deprecated-output=accept|hide]\n"
>> -"Policy for handling deprecated management 
>> interfaces\n",
>> +"Policy for handling deprecated management interfaces\n"
>> +"-compat 
>> [unstable-input=accept|reject|crash][,unstable-output=accept|hide]\n"
>> +"Policy for handling unstable management interfaces\n",
>
> It may not be machine-introspectible, but at least we can grep --help
> output to see when the policy is usable for testing.
>
>>  QEMU_ARCH_ALL)
>>  SRST
>>  ``-compat 
>> [deprecated-input=@var{input-policy}][,deprecated-output=@var{output-policy}]``
>> @@ -3659,6 +3661,22 @@ SRST
>>  Suppress deprecated command results and events
>>  
>>  Limitation: covers only syntactic aspects of QMP.
>> +
>> +``-compat 
>> [unstable-input=@var{input-policy}][,unstable-output=@var{output-policy}]``
>> +Set policy for handling unstable management interfaces (experimental):
>
> Once we QAPIfy the command line, this says we would add the 'unstable'
> feature flag to '-compat unstable-input'.  How meta ;)

Yes :)

>And goes along
> with your comments earlier in the series about how we may use the
> 'unstable' feature even without the 'x-' naming prefix, once it is
> machine-detectible.
>
> With the doc tweak,
> Reviewed-by: Eric Blake 

Thanks!

Re: [PATCH v2 9/9] qapi: Extend -compat to set policy for unstable interfaces

[Eric Blake]

On Thu, Oct 28, 2021 at 12:25:20PM +0200, Markus Armbruster wrote:
> New option parameters unstable-input and unstable-output set policy
> for unstable interfaces just like deprecated-input and
> deprecated-output set policy for deprecated interfaces (see commit
> 6dd75472d5 "qemu-options: New -compat to set policy for deprecated
> interfaces").  This is intended for testing users of the management
> interfaces.  It is experimental.
> 
> For now, this covers only syntactic aspects of QMP, i.e. stuff tagged
> with feature 'unstable'.  We may want to extend it to cover semantic
> aspects, or the command line.
> 
> Note that there is no good way for management application to detect
> presence of these new option parameters: they are not visible output
> of query-qmp-schema or query-command-line-options.  Tolerable, because
> it's meant for testing.  If running with -compat fails, skip the test.

Not to mention, once we finish QAPIfying the command line, we could
make sure it is visible through introspection at that time (it may
require tagging the command line option with a feature, if nothing
else makes it pop through).

> 
> Signed-off-by: Markus Armbruster 
> Acked-by: John Snow 
> ---
>  qapi/compat.json  |  6 +-
>  include/qapi/util.h   |  1 +
>  qapi/qmp-dispatch.c   |  6 ++
>  qapi/qobject-output-visitor.c |  8 ++--
>  qemu-options.hx   | 20 +++-
>  scripts/qapi/events.py| 10 ++
>  scripts/qapi/schema.py| 10 ++
>  7 files changed, 49 insertions(+), 12 deletions(-)
> 
> diff --git a/qapi/compat.json b/qapi/compat.json
> index 74a8493d3d..9bc9804abb 100644
> --- a/qapi/compat.json
> +++ b/qapi/compat.json
> @@ -47,9 +47,13 @@
>  #
>  # @deprecated-input: how to handle deprecated input (default 'accept')
>  # @deprecated-output: how to handle deprecated output (default 'accept')
> +# @unstable-input: how to handle unstable input (default 'accept')
> +# @unstable-output: how to handle unstable output (default 'accept')

Missing '(since 6.2)' doc tags on the two new policies.

>  #
>  # Since: 6.0
>  ##
>  { 'struct': 'CompatPolicy',
>'data': { '*deprecated-input': 'CompatPolicyInput',
> -'*deprecated-output': 'CompatPolicyOutput' } }
> +'*deprecated-output': 'CompatPolicyOutput',
> +'*unstable-input': 'CompatPolicyInput',
> +'*unstable-output': 'CompatPolicyOutput' } }
> diff --git a/include/qapi/util.h b/include/qapi/util.h
> index 0cc98db9f9..81a2b13a33 100644
> --- a/include/qapi/util.h
> +++ b/include/qapi/util.h
> @@ -13,6 +13,7 @@
>  
>  typedef enum {
>  QAPI_DEPRECATED,
> +QAPI_UNSTABLE,
>  } QapiSpecialFeature;

> +++ b/qemu-options.hx
> @@ -3641,7 +3641,9 @@ DEFHEADING(Debug/Expert options:)
>  
>  DEF("compat", HAS_ARG, QEMU_OPTION_compat,
>  "-compat 
> [deprecated-input=accept|reject|crash][,deprecated-output=accept|hide]\n"
> -"Policy for handling deprecated management interfaces\n",
> +"Policy for handling deprecated management interfaces\n"
> +"-compat 
> [unstable-input=accept|reject|crash][,unstable-output=accept|hide]\n"
> +"Policy for handling unstable management interfaces\n",

It may not be machine-introspectible, but at least we can grep --help
output to see when the policy is usable for testing.

>  QEMU_ARCH_ALL)
>  SRST
>  ``-compat 
> [deprecated-input=@var{input-policy}][,deprecated-output=@var{output-policy}]``
> @@ -3659,6 +3661,22 @@ SRST
>  Suppress deprecated command results and events
>  
>  Limitation: covers only syntactic aspects of QMP.
> +
> +``-compat 
> [unstable-input=@var{input-policy}][,unstable-output=@var{output-policy}]``
> +Set policy for handling unstable management interfaces (experimental):

Once we QAPIfy the command line, this says we would add the 'unstable'
feature flag to '-compat unstable-input'.  How meta ;) And goes along
with your comments earlier in the series about how we may use the
'unstable' feature even without the 'x-' naming prefix, once it is
machine-detectible.

With the doc tweak,
Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 8/9] qapi: Factor out compat_policy_input_ok()

[Markus Armbruster]

Philippe Mathieu-Daudé  writes:

> On 10/26/21 11:46, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé  writes:
>> 
>>> On 10/25/21 07:25, Markus Armbruster wrote:
 The code to check policy for handling deprecated input is triplicated.
 Factor it out into compat_policy_input_ok() before I mess with it in
 the next commit.

 Signed-off-by: Markus Armbruster 
 ---
  include/qapi/compat-policy.h |  7 +
  qapi/qapi-visit-core.c   | 18 +
  qapi/qmp-dispatch.c  | 51 +++-
  qapi/qobject-input-visitor.c | 19 +++---
  4 files changed, 55 insertions(+), 40 deletions(-)
>>>
 diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
 index 8cca18c891..e29ade134c 100644
 --- a/qapi/qmp-dispatch.c
 +++ b/qapi/qmp-dispatch.c
 @@ -28,6 +28,40 @@
  
  CompatPolicy compat_policy;
  
 +static bool compat_policy_input_ok1(const char *adjective,
 +CompatPolicyInput policy,
 +ErrorClass error_class,
 +const char *kind, const char *name,
 +Error **errp)
 +{
 +switch (policy) {
 +case COMPAT_POLICY_INPUT_ACCEPT:
 +return true;
 +case COMPAT_POLICY_INPUT_REJECT:
 +error_set(errp, error_class, "%s %s %s disabled by policy",
 +  adjective, kind, name);
 +return false;
 +case COMPAT_POLICY_INPUT_CRASH:
 +default:
 +abort();
>>>
>>> g_assert_not_reached() provides a nicer user experience.
>> 
>> I find it hard to care for making the experience of a crash that should
>> never ever happen nicer :)
>
> Well COMPAT_POLICY_INPUT_CRASH can happen... What about:

Point.

>case COMPAT_POLICY_INPUT_CRASH:
>error_printf("%s %s %s disabled by policy",
> adjective, kind, name);
>abort();
>default:
>g_assert_not_reached();

Separate patch.  I'd prefer to delay it a bit, to avoid rocking the boat
so close to the soft freeze.

Re: [PATCH v2 5/9] qapi: Generalize struct member policy checking

[Markus Armbruster]

Philippe Mathieu-Daudé  writes:

> On 10/28/21 12:25, Markus Armbruster wrote:
>> The generated visitor functions call visit_deprecated_accept() and
>> visit_deprecated() when visiting a struct member with special feature
>> flag 'deprecated'.  This makes the feature flag visible to the actual
>> visitors.  I want to make feature flag 'unstable' visible there as
>> well, so I can add policy for it.
>> 
>> To let me make it visible, replace these functions by
>> visit_policy_reject() and visit_policy_skip(), which take the member's
>> special features as an argument.  Note that the new functions have the
>> opposite sense, i.e. the return value flips.
>> 
>> Signed-off-by: Markus Armbruster 
>> ---
>>  include/qapi/visitor-impl.h   |  6 --
>>  include/qapi/visitor.h| 17 +
>>  qapi/qapi-forward-visitor.c   | 16 +---
>>  qapi/qapi-visit-core.c| 22 --
>>  qapi/qobject-input-visitor.c  | 15 ++-
>>  qapi/qobject-output-visitor.c |  9 ++---
>>  qapi/trace-events |  4 ++--
>>  scripts/qapi/visit.py | 14 +++---
>>  8 files changed, 63 insertions(+), 40 deletions(-)
>
>> -static bool qobject_input_deprecated_accept(Visitor *v, const char *name,
>> -Error **errp)
>> +static bool qobject_input_policy_reject(Visitor *v, const char *name,
>> +unsigned special_features,
>> +Error **errp)
>>  {
>> +if (!(special_features & 1u << QAPI_DEPRECATED)) {
>> +return false;
>> +}
>> +
>>  switch (v->compat_policy.deprecated_input) {
>>  case COMPAT_POLICY_INPUT_ACCEPT:
>> -return true;
>> +return false;
>>  case COMPAT_POLICY_INPUT_REJECT:
>>  error_setg(errp, "Deprecated parameter '%s' disabled by policy",
>> name);
>> -return false;
>> +return true;
>>  case COMPAT_POLICY_INPUT_CRASH:
>
> Clearer as:
>
>abort();
>default:
>g_assert_not_reached();

Maybe, but making it so has nothing to do with this patch.  It could
perhaps be done in PATCH 8, or in a followup patch.

> Otherwise,
> Reviewed-by: Philippe Mathieu-Daudé 

Okay to tack your R-by to the unmodified patch?

Thanks!

>
>>  default:
>>  abort();

Re: [PATCH 0/7] qemu: Various monitor cleanups and removal of legacy cpu hotplug

[Ján Tomko]

On a Friday in 2021, Peter Krempa wrote:

Peter Krempa (7):
 qemuMonitorJSONQueryBlock: Reformat function header
 qemuMonitorJSONBlockInfoAdd: Refactor hash table addition
 qemuhotplugtest: Remove tests for legacy cpu hotplug on x86
 qemuDomainHotplugAddVcpu: Remove legacy hotplug branch
 qemu: monitor: Remove unused qemuMonitorSetCPU
 qemuMonitorEjectMedia: Remove stale comment
 qemuMonitorJSONSave[Physical|Virtual]Memory: Reformat function headers

src/qemu/qemu_hotplug.c   |  22 +-
src/qemu/qemu_monitor.c   |  14 --
src/qemu/qemu_monitor.h   |   6 -
src/qemu/qemu_monitor_json.c  |  77 +++
src/qemu/qemu_monitor_json.h  |   1 -
tests/qemuhotplugtest.c   |   1 -
.../x86-old-bulk-domain.xml   |  21 --
.../x86-old-bulk-monitor.json | 193 --
.../x86-old-bulk-result-conf.xml  |  31 ---
.../x86-old-bulk-result-live.xml  |  39 
tests/qemumonitorjsontest.c   |   2 -
11 files changed, 35 insertions(+), 372 deletions(-)
delete mode 100644 tests/qemuhotplugtestcpus/x86-old-bulk-domain.xml
delete mode 100644 tests/qemuhotplugtestcpus/x86-old-bulk-monitor.json
delete mode 100644 tests/qemuhotplugtestcpus/x86-old-bulk-result-conf.xml
delete mode 100644 tests/qemuhotplugtestcpus/x86-old-bulk-result-live.xml



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 4/7] qemuDomainHotplugAddVcpu: Remove legacy hotplug branch

[Ján Tomko]

On a Friday in 2021, Peter Krempa wrote:

Report an error if the new hotplug is not supported and remove the
alternate code paths.

The modern cpu-hotplug code was introduced in qemu-2.7. We keep the
capability so that proper errors are reported in case a platform doesn't
support hotplug.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_hotplug.c | 22 ++
1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index a7b432b6f5..51c5b517b4 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -6488,25 +6488,24 @@ qemuDomainHotplugAddVcpu(virQEMUDriver *driver,
virDomainVcpuDef *vcpuinfo = virDomainDefGetVcpu(vm->def, vcpu);
qemuDomainVcpuPrivate *vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpuinfo);
unsigned int nvcpus = vcpupriv->vcpus;
-bool newhotplug = qemuDomainSupportsNewVcpuHotplug(vm);
int rc;
int oldvcpus = virDomainDefGetVcpus(vm->def);
size_t i;

-if (newhotplug) {
-vcpupriv->alias = g_strdup_printf("vcpu%u", vcpu);
-
-if (!(vcpuprops = qemuBuildHotpluggableCPUProps(vcpuinfo)))
+if (!qemuDomainSupportsNewVcpuHotplug(vm)) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+   _("cpu hotplug is not supported"));
return -1;


Request: reindent return


signature.asc
Description: PGP signature

Re: [PATCH v2 5/9] qapi: Generalize struct member policy checking

[Philippe Mathieu-Daudé]

On 10/28/21 12:25, Markus Armbruster wrote:
> The generated visitor functions call visit_deprecated_accept() and
> visit_deprecated() when visiting a struct member with special feature
> flag 'deprecated'.  This makes the feature flag visible to the actual
> visitors.  I want to make feature flag 'unstable' visible there as
> well, so I can add policy for it.
> 
> To let me make it visible, replace these functions by
> visit_policy_reject() and visit_policy_skip(), which take the member's
> special features as an argument.  Note that the new functions have the
> opposite sense, i.e. the return value flips.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  include/qapi/visitor-impl.h   |  6 --
>  include/qapi/visitor.h| 17 +
>  qapi/qapi-forward-visitor.c   | 16 +---
>  qapi/qapi-visit-core.c| 22 --
>  qapi/qobject-input-visitor.c  | 15 ++-
>  qapi/qobject-output-visitor.c |  9 ++---
>  qapi/trace-events |  4 ++--
>  scripts/qapi/visit.py | 14 +++---
>  8 files changed, 63 insertions(+), 40 deletions(-)

> -static bool qobject_input_deprecated_accept(Visitor *v, const char *name,
> -Error **errp)
> +static bool qobject_input_policy_reject(Visitor *v, const char *name,
> +unsigned special_features,
> +Error **errp)
>  {
> +if (!(special_features & 1u << QAPI_DEPRECATED)) {
> +return false;
> +}
> +
>  switch (v->compat_policy.deprecated_input) {
>  case COMPAT_POLICY_INPUT_ACCEPT:
> -return true;
> +return false;
>  case COMPAT_POLICY_INPUT_REJECT:
>  error_setg(errp, "Deprecated parameter '%s' disabled by policy",
> name);
> -return false;
> +return true;
>  case COMPAT_POLICY_INPUT_CRASH:

Clearer as:

   abort();
   default:
   g_assert_not_reached();

Otherwise,
Reviewed-by: Philippe Mathieu-Daudé 

>  default:
>  abort();

Re: [PATCH v2 5/9] qapi: Generalize struct member policy checking

[Markus Armbruster]

Juan Quintela  writes:

> Markus Armbruster  wrote:
>> The generated visitor functions call visit_deprecated_accept() and
>> visit_deprecated() when visiting a struct member with special feature
>> flag 'deprecated'.  This makes the feature flag visible to the actual
>> visitors.  I want to make feature flag 'unstable' visible there as
>> well, so I can add policy for it.
>>
>> To let me make it visible, replace these functions by
>> visit_policy_reject() and visit_policy_skip(), which take the member's
>> special features as an argument.  Note that the new functions have the
>> opposite sense, i.e. the return value flips.
>>
>> Signed-off-by: Markus Armbruster 
>
> Reviewed-by: Juan Quintela 
>
> Reversing accept/reject make things "interesting" for a review point of view.

Sorry about that.

>> + * @special_features is the member's special features encoded as a
>> + * bitset of QapiSpecialFeature.
>
> Just to nitty pick, if you rename the variable to features, does the
> sentece is clearer?

Not to me, I'm afraid...

Thanks!

[PATCH 2/7] qemuMonitorJSONBlockInfoAdd: Refactor hash table addition

[Peter Krempa]

Open code virHashAddEntry so that the error code path can be avoided.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_monitor_json.c | 24 +---
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 19af6219aa..962876b43a 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -2220,27 +2220,21 @@ qemuMonitorJSONBlockInfoAdd(GHashTable *table,
 const char *entryname)
 {
 struct qemuDomainDiskInfo *tmp = NULL;
-int ret = -1;
+
+if (g_hash_table_contains(table, entryname)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Duplicate block info for '%s'"), entryname);
+return -1;
+}

 tmp = g_new0(struct qemuDomainDiskInfo, 1);

 *tmp = *info;
-tmp->nodename = NULL;
-
-if (info->nodename)
-tmp->nodename = g_strdup(info->nodename);
+tmp->nodename = g_strdup(info->nodename);

-if (virHashAddEntry(table, entryname, tmp) < 0)
-goto cleanup;
-
-tmp = NULL;
-ret = 0;
+g_hash_table_insert(table, g_strdup(entryname), tmp);

- cleanup:
-if (tmp)
-VIR_FREE(tmp->nodename);
-VIR_FREE(tmp);
-return ret;
+return 0;
 }


-- 
2.31.1

[PATCH 7/7] qemuMonitorJSONSave[Physical|Virtual]Memory: Reformat function headers

[Peter Krempa]

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_monitor_json.c | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 2ba8da454b..7d21e23800 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -2981,18 +2981,21 @@ static int qemuMonitorJSONSaveMemory(qemuMonitor *mon,
 }


-int qemuMonitorJSONSaveVirtualMemory(qemuMonitor *mon,
- unsigned long long offset,
- unsigned long long length,
- const char *path)
+int
+qemuMonitorJSONSaveVirtualMemory(qemuMonitor *mon,
+ unsigned long long offset,
+ unsigned long long length,
+ const char *path)
 {
 return qemuMonitorJSONSaveMemory(mon, "memsave", offset, length, path);
 }

-int qemuMonitorJSONSavePhysicalMemory(qemuMonitor *mon,
-  unsigned long long offset,
-  unsigned long long length,
-  const char *path)
+
+int
+qemuMonitorJSONSavePhysicalMemory(qemuMonitor *mon,
+  unsigned long long offset,
+  unsigned long long length,
+  const char *path)
 {
 return qemuMonitorJSONSaveMemory(mon, "pmemsave", offset, length, path);
 }
-- 
2.31.1

[PATCH 6/7] qemuMonitorEjectMedia: Remove stale comment

[Peter Krempa]

The QMP implementation didn't use any new approach. The command itself
is now only used with legacy qemu versions.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_monitor.h | 4 
 1 file changed, 4 deletions(-)

diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 5c5df04130..cd1c1c4291 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -795,10 +795,6 @@ int qemuMonitorExpirePassword(qemuMonitor *mon,
 int qemuMonitorSetBalloon(qemuMonitor *mon,
   unsigned long long newmem);

-/* XXX should we pass the virDomainDiskDef *instead
- * and hide dev_name details inside monitor. Reconsider
- * this when doing the QMP implementation
- */
 int qemuMonitorEjectMedia(qemuMonitor *mon,
   const char *dev_name,
   bool force);
-- 
2.31.1

[PATCH 5/7] qemu: monitor: Remove unused qemuMonitorSetCPU

[Peter Krempa]

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_monitor.c  | 14 --
 src/qemu/qemu_monitor.h  |  2 --
 src/qemu/qemu_monitor_json.c | 25 -
 src/qemu/qemu_monitor_json.h |  1 -
 tests/qemumonitorjsontest.c  |  2 --
 5 files changed, 44 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 908ee0d302..a63915b6d7 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -,20 +,6 @@ qemuMonitorSetBalloon(qemuMonitor *mon,
 }


-/*
- * Returns: 0 if CPU modification was successful or -1 on failure
- */
-int
-qemuMonitorSetCPU(qemuMonitor *mon, int cpu, bool online)
-{
-VIR_DEBUG("cpu=%d online=%d", cpu, online);
-
-QEMU_CHECK_MONITOR(mon);
-
-return qemuMonitorJSONSetCPU(mon, cpu, online);
-}
-
-
 int
 qemuMonitorEjectMedia(qemuMonitor *mon,
   const char *dev_name,
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index b54c1cf87a..5c5df04130 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -794,8 +794,6 @@ int qemuMonitorExpirePassword(qemuMonitor *mon,
   const char *expire_time);
 int qemuMonitorSetBalloon(qemuMonitor *mon,
   unsigned long long newmem);
-int qemuMonitorSetCPU(qemuMonitor *mon, int cpu, bool online);
-

 /* XXX should we pass the virDomainDiskDef *instead
  * and hide dev_name details inside monitor. Reconsider
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 962876b43a..2ba8da454b 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -2898,31 +2898,6 @@ qemuMonitorJSONSetBalloon(qemuMonitor *mon,
 }


-int qemuMonitorJSONSetCPU(qemuMonitor *mon,
-  int cpu, bool online)
-{
-g_autoptr(virJSONValue) cmd = NULL;
-g_autoptr(virJSONValue) reply = NULL;
-
-if (online) {
-cmd = qemuMonitorJSONMakeCommand("cpu-add",
- "i:id", cpu,
- NULL);
-} else {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("vCPU unplug is not supported by this QEMU"));
-return -1;
-}
-if (!cmd)
-return -1;
-
-if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
-return -1;
-
-return qemuMonitorJSONCheckError(cmd, reply);
-}
-
-
 /**
  * Run QMP command to eject a media from ejectable device.
  *
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index f9e01e5bf5..d91c5928e3 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -104,7 +104,6 @@ int qemuMonitorJSONExpirePassword(qemuMonitor *mon,
   const char *expire_time);
 int qemuMonitorJSONSetBalloon(qemuMonitor *mon,
   unsigned long long newmem);
-int qemuMonitorJSONSetCPU(qemuMonitor *mon, int cpu, bool online);

 int qemuMonitorJSONEjectMedia(qemuMonitor *mon,
   const char *dev_name,
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index e5ba39cd2f..0e6e0fe284 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -1168,7 +1168,6 @@ GEN_TEST_FUNC(qemuMonitorJSONBlockResize, "vda", "asdf", 
123456)
 GEN_TEST_FUNC(qemuMonitorJSONSetPassword, "spice", "secret_password", 
"disconnect")
 GEN_TEST_FUNC(qemuMonitorJSONExpirePassword, "spice", "123456")
 GEN_TEST_FUNC(qemuMonitorJSONSetBalloon, 1024)
-GEN_TEST_FUNC(qemuMonitorJSONSetCPU, 1, true)
 GEN_TEST_FUNC(qemuMonitorJSONEjectMedia, "hdc", true)
 GEN_TEST_FUNC(qemuMonitorJSONChangeMedia, "hdc", "/foo/bar", "formatstr")
 GEN_TEST_FUNC(qemuMonitorJSONSaveVirtualMemory, 0, 1024, "/foo/bar")
@@ -3011,7 +3010,6 @@ mymain(void)
 DO_TEST_GEN(qemuMonitorJSONSetPassword);
 DO_TEST_GEN(qemuMonitorJSONExpirePassword);
 DO_TEST_GEN(qemuMonitorJSONSetBalloon);
-DO_TEST_GEN_DEPRECATED(qemuMonitorJSONSetCPU, true);
 DO_TEST_GEN(qemuMonitorJSONEjectMedia);
 DO_TEST_GEN_DEPRECATED(qemuMonitorJSONChangeMedia, true);
 DO_TEST_GEN(qemuMonitorJSONSaveVirtualMemory);
-- 
2.31.1

[PATCH 3/7] qemuhotplugtest: Remove tests for legacy cpu hotplug on x86

[Peter Krempa]

Modern cpu hotplug was introduced in qemu-2.7, thus all qemu versions
actually support it. Remove the tests for the legacy hotplug.

Signed-off-by: Peter Krempa 
---
 tests/qemuhotplugtest.c   |   1 -
 .../x86-old-bulk-domain.xml   |  21 --
 .../x86-old-bulk-monitor.json | 193 --
 .../x86-old-bulk-result-conf.xml  |  31 ---
 .../x86-old-bulk-result-live.xml  |  39 
 5 files changed, 285 deletions(-)
 delete mode 100644 tests/qemuhotplugtestcpus/x86-old-bulk-domain.xml
 delete mode 100644 tests/qemuhotplugtestcpus/x86-old-bulk-monitor.json
 delete mode 100644 tests/qemuhotplugtestcpus/x86-old-bulk-result-conf.xml
 delete mode 100644 tests/qemuhotplugtestcpus/x86-old-bulk-result-live.xml

diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index ec448da09e..3484041cc8 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -874,7 +874,6 @@ mymain(void)
 } while (0)

 DO_TEST_CPU_GROUP("x86-modern-bulk", 7, true, false);
-DO_TEST_CPU_GROUP("x86-old-bulk", 7, false, false);
 DO_TEST_CPU_GROUP("ppc64-modern-bulk", 24, true, false);
 DO_TEST_CPU_GROUP("ppc64-modern-bulk", 15, true, true);
 DO_TEST_CPU_GROUP("ppc64-modern-bulk", 23, true, true);
diff --git a/tests/qemuhotplugtestcpus/x86-old-bulk-domain.xml 
b/tests/qemuhotplugtestcpus/x86-old-bulk-domain.xml
deleted file mode 100644
index ef0e39397b..00
--- a/tests/qemuhotplugtestcpus/x86-old-bulk-domain.xml
+++ /dev/null
@@ -1,21 +0,0 @@
-
-  QEMUGuest1
-  c7a5fdbd-edaf-9455-926a-d65c16db1809
-  219100
-  219100
-  8
-  
-hvm
-
-  
-  
-
-  
-  
-  destroy
-  restart
-  destroy
-  
-  /usr/bin/qemu-system-x86_64
-  
-
diff --git a/tests/qemuhotplugtestcpus/x86-old-bulk-monitor.json 
b/tests/qemuhotplugtestcpus/x86-old-bulk-monitor.json
deleted file mode 100644
index 626c0a5699..00
--- a/tests/qemuhotplugtestcpus/x86-old-bulk-monitor.json
+++ /dev/null
@@ -1,193 +0,0 @@
-{"execute":"query-cpus-fast","id":"libvirt-1"}
-
-{
-  "return": [
-{
-  "target": "x86",
-  "current": true,
-  "cpu-index": 0,
-  "qom-path": "/machine/unattached/device[0]",
-  "pc": -2130415978,
-  "halted": true,
-  "thread-id": 518291
-},
-{
-  "target": "x86",
-  "current": false,
-  "cpu-index": 1,
-  "qom-path": "/machine/unattached/device[2]",
-  "pc": -2130415978,
-  "halted": true,
-  "thread-id": 518292
-},
-{
-  "target": "x86",
-  "current": false,
-  "cpu-index": 2,
-  "qom-path": "/machine/unattached/device[3]",
-  "pc": -2130415978,
-  "halted": true,
-  "thread-id": 518294
-},
-{
-  "target": "x86",
-  "current": false,
-  "cpu-index": 3,
-  "qom-path": "/machine/unattached/device[4]",
-  "pc": -2130415978,
-  "halted": true,
-  "thread-id": 518295
-},
-{
-  "target": "x86",
-  "current": false,
-  "cpu-index": 4,
-  "qom-path": "/machine/unattached/device[5]",
-  "pc": -2130415978,
-  "halted": true,
-  "thread-id": 518296
-}
-  ],
-  "id": "libvirt-22"
-}
-
-{"execute":"cpu-add","arguments":{"id":5},"id":"libvirt-2"}
-
-{"return": {}}
-
-{"execute":"query-cpus-fast","id":"libvirt-3"}
-
-{
-  "return": [
-{
-  "target": "x86",
-  "current": true,
-  "cpu-index": 0,
-  "qom-path": "/machine/unattached/device[0]",
-  "pc": -2130415978,
-  "halted": true,
-  "thread-id": 518291
-},
-{
-  "target": "x86",
-  "current": false,
-  "cpu-index": 1,
-  "qom-path": "/machine/unattached/device[2]",
-  "pc": -2130415978,
-  "halted": true,
-  "thread-id": 518292
-},
-{
-  "target": "x86",
-  "current": false,
-  "cpu-index": 2,
-  "qom-path": "/machine/unattached/device[3]",
-  "pc": -2130415978,
-  "halted": true,
-  "thread-id": 518294
-},
-{
-  "target": "x86",
-  "current": false,
-  "cpu-index": 3,
-  "qom-path": "/machine/unattached/device[4]",
-  "pc": -2130415978,
-  "halted": true,
-  "thread-id": 518295
-},
-{
-  "target": "x86",
-  "current": false,
-  "cpu-index": 4,
-  "qom-path": "/machine/unattached/device[5]",
-  "pc": -2130415978,
-  "halted": true,
-  "thread-id": 518296
-},
-{
-  "target": "x86",
-  "current": false,
-  "cpu-index": 5,
-  "qom-path": "/machine/peripheral/vcpu5",
-  "pc": -2130415978,
-  "halted": true,
-  "thread-id": 518297
-}
-  ],
-  "id": "libvirt-22"
-}
-
-{"execute":"cpu-add","arguments":{"id":6},"id":"libvirt-4"}
-
-{"return": {}}
-
-{"execute":"query-cpus-fast","id":"libvirt-5"}
-
-{
-  "return": [
-{
-  "target": "x86",
-  "current": true,
-  "cpu-index": 0,
-  "qom-path": "/machine/unattached/device[0]",
-  "pc": -2130415978,
-  "halted": true,
-  

[PATCH 4/7] qemuDomainHotplugAddVcpu: Remove legacy hotplug branch

[Peter Krempa]

Report an error if the new hotplug is not supported and remove the
alternate code paths.

The modern cpu-hotplug code was introduced in qemu-2.7. We keep the
capability so that proper errors are reported in case a platform doesn't
support hotplug.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_hotplug.c | 22 ++
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index a7b432b6f5..51c5b517b4 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -6488,25 +6488,24 @@ qemuDomainHotplugAddVcpu(virQEMUDriver *driver,
 virDomainVcpuDef *vcpuinfo = virDomainDefGetVcpu(vm->def, vcpu);
 qemuDomainVcpuPrivate *vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpuinfo);
 unsigned int nvcpus = vcpupriv->vcpus;
-bool newhotplug = qemuDomainSupportsNewVcpuHotplug(vm);
 int rc;
 int oldvcpus = virDomainDefGetVcpus(vm->def);
 size_t i;

-if (newhotplug) {
-vcpupriv->alias = g_strdup_printf("vcpu%u", vcpu);
-
-if (!(vcpuprops = qemuBuildHotpluggableCPUProps(vcpuinfo)))
+if (!qemuDomainSupportsNewVcpuHotplug(vm)) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+   _("cpu hotplug is not supported"));
 return -1;
 }

+vcpupriv->alias = g_strdup_printf("vcpu%u", vcpu);
+
+if (!(vcpuprops = qemuBuildHotpluggableCPUProps(vcpuinfo)))
+return -1;
+
 qemuDomainObjEnterMonitor(driver, vm);

-if (newhotplug) {
-rc = qemuMonitorAddDeviceProps(qemuDomainGetMonitor(vm), );
-} else {
-rc = qemuMonitorSetCPU(qemuDomainGetMonitor(vm), vcpu, true);
-}
+rc = qemuMonitorAddDeviceProps(qemuDomainGetMonitor(vm), );

 if (qemuDomainObjExitMonitor(driver, vm) < 0)
 return -1;
@@ -6517,8 +6516,7 @@ qemuDomainHotplugAddVcpu(virQEMUDriver *driver,
 return -1;

 /* start outputting of the new XML element to allow keeping unpluggability 
*/
-if (newhotplug)
-vm->def->individualvcpus = true;
+vm->def->individualvcpus = true;

 if (qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE, false) < 0)
 return -1;
-- 
2.31.1

[PATCH 1/7] qemuMonitorJSONQueryBlock: Reformat function header

[Peter Krempa]

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_monitor_json.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index e9be9bdabd..19af6219aa 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -2036,10 +2036,11 @@ qemuMonitorJSONGetBalloonInfo(qemuMonitor *mon,
 }


-int qemuMonitorJSONGetMemoryStats(qemuMonitor *mon,
-  char *balloonpath,
-  virDomainMemoryStatPtr stats,
-  unsigned int nr_stats)
+int
+qemuMonitorJSONGetMemoryStats(qemuMonitor *mon,
+  char *balloonpath,
+  virDomainMemoryStatPtr stats,
+  unsigned int nr_stats)
 {
 int ret = -1;
 g_autoptr(virJSONValue) cmd = NULL;
-- 
2.31.1

Re: [PATCH for 7.9.0] NEWS: Document my bugfixes for v7.9.0

[Jiri Denemark]

On Fri, Oct 29, 2021 at 15:02:44 +0200, Michal Privoznik wrote:
> There are two bugs I fixed worth mentioning in the 7.9.0 release
> notes.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  NEWS.rst | 15 +++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/NEWS.rst b/NEWS.rst
> index 1d2d31430f..1bb6897abd 100644
> --- a/NEWS.rst
> +++ b/NEWS.rst
> @@ -86,6 +86,21 @@ v7.9.0 (unreleased)
>  qemu-5.2.0, regardless of qemu version and failed in qemu-5.1.0. This
>  release fixes the bug.
>  
> + * Don't enter endless loop when unable to accept new clients
> +
> +   If libvirtd (or any other daemon) hit the ulimit for maximum number of 
> open
> +   files but there are still client connections pending then libvirtd (or
> +   corresponding split daemon) would enter an endless loop from which it 
> would
> +   never recover. This behaviour is now fixed.
> +
> + * qemu: Run secondary driver hooks in split daemon mode
> +
> +   Because of a bug in implementation it may happen that hooks from secondary
> +   drivers were not called in all cases, for instance a network hook wasn't
> +   called upon removal of interface after domain shut off itself. With this
> +   release the bug is fixed.
> +
> +
>  v7.8.0 (2021-10-01)
>  ===

Reviewed-by: Jiri Denemark 

[PATCH 0/7] qemu: Various monitor cleanups and removal of legacy cpu hotplug

[Peter Krempa]

Peter Krempa (7):
  qemuMonitorJSONQueryBlock: Reformat function header
  qemuMonitorJSONBlockInfoAdd: Refactor hash table addition
  qemuhotplugtest: Remove tests for legacy cpu hotplug on x86
  qemuDomainHotplugAddVcpu: Remove legacy hotplug branch
  qemu: monitor: Remove unused qemuMonitorSetCPU
  qemuMonitorEjectMedia: Remove stale comment
  qemuMonitorJSONSave[Physical|Virtual]Memory: Reformat function headers

 src/qemu/qemu_hotplug.c   |  22 +-
 src/qemu/qemu_monitor.c   |  14 --
 src/qemu/qemu_monitor.h   |   6 -
 src/qemu/qemu_monitor_json.c  |  77 +++
 src/qemu/qemu_monitor_json.h  |   1 -
 tests/qemuhotplugtest.c   |   1 -
 .../x86-old-bulk-domain.xml   |  21 --
 .../x86-old-bulk-monitor.json | 193 --
 .../x86-old-bulk-result-conf.xml  |  31 ---
 .../x86-old-bulk-result-live.xml  |  39 
 tests/qemumonitorjsontest.c   |   2 -
 11 files changed, 35 insertions(+), 372 deletions(-)
 delete mode 100644 tests/qemuhotplugtestcpus/x86-old-bulk-domain.xml
 delete mode 100644 tests/qemuhotplugtestcpus/x86-old-bulk-monitor.json
 delete mode 100644 tests/qemuhotplugtestcpus/x86-old-bulk-result-conf.xml
 delete mode 100644 tests/qemuhotplugtestcpus/x86-old-bulk-result-live.xml

-- 
2.31.1

Re: [PATCH 2/9] qapi: Mark unstable QMP parts with feature 'unstable'

[Eric Blake]

On Mon, Oct 25, 2021 at 07:25:25AM +0200, Markus Armbruster wrote:
> Add special feature 'unstable' everywhere the name starts with 'x-',
> except for InputBarrierProperties member x-origin and
> MemoryBackendProperties member x-use-canonical-path-for-ramblock-id,
> because these two are actually stable.
> 
> Signed-off-by: Markus Armbruster 
> ---
> @@ -2495,27 +2508,57 @@
>  #
>  # Properties for throttle-group objects.
>  #
> -# The options starting with x- are aliases for the same key without x- in
> -# the @limits object. As indicated by the x- prefix, this is not a stable
> -# interface and may be removed or changed incompatibly in the future. Use
> -# @limits for a supported stable interface.
> -#
>  # @limits: limits to apply for this throttle group
>  #
> +# Features:
> +# @unstable: All members starting with x- are aliases for the same key
> +#without x- in the @limits object.  This is not a stable
> +#interface and may be removed or changed incompatibly in
> +#the future.  Use @limits for a supported stable
> +#interface.
> +#
>  # Since: 2.11
>  ##
>  { 'struct': 'ThrottleGroupProperties',
>'data': { '*limits': 'ThrottleLimits',
> -'*x-iops-total' : 'int', '*x-iops-total-max' : 'int',

> +'*x-iops-total': { 'type': 'int',
> +   'features': [ 'unstable' ] },

This struct has been around since 381bd74 (v6.0); but was not listed
as deprecated at the time.  Do we still need it in 6.2, or have we
gone enough release cycles with the saner naming without x- that we
could drop this?  But that is a question independent of this patch.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[PATCH for 7.9.0] NEWS: Document my bugfixes for v7.9.0

[Michal Privoznik]

There are two bugs I fixed worth mentioning in the 7.9.0 release
notes.

Signed-off-by: Michal Privoznik 
---
 NEWS.rst | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/NEWS.rst b/NEWS.rst
index 1d2d31430f..1bb6897abd 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -86,6 +86,21 @@ v7.9.0 (unreleased)
 qemu-5.2.0, regardless of qemu version and failed in qemu-5.1.0. This
 release fixes the bug.
 
+ * Don't enter endless loop when unable to accept new clients
+
+   If libvirtd (or any other daemon) hit the ulimit for maximum number of open
+   files but there are still client connections pending then libvirtd (or
+   corresponding split daemon) would enter an endless loop from which it would
+   never recover. This behaviour is now fixed.
+
+ * qemu: Run secondary driver hooks in split daemon mode
+
+   Because of a bug in implementation it may happen that hooks from secondary
+   drivers were not called in all cases, for instance a network hook wasn't
+   called upon removal of interface after domain shut off itself. With this
+   release the bug is fixed.
+
+
 v7.8.0 (2021-10-01)
 ===
 
-- 
2.32.0

Re: [libvirt PATCH] spec: Depend on qemu-kvm-block-driver-curl in RHEL-9

[Daniel P . Berrangé]

On Fri, Oct 29, 2021 at 02:53:09PM +0200, Jiri Denemark wrote:
> On Wed, Oct 27, 2021 at 15:38:22 +0100, Daniel P. Berrangé wrote:
> > On Wed, Oct 27, 2021 at 03:58:29PM +0200, Jiri Denemark wrote:
> > > The curl block driver is no longer a requirement for qemu-kvm package in
> > > RHEL-9, which breaks support for https network disks. Let's depend on it
> > > explicitly (in libvirt-daemon-kvm meta package) until we possibly
> > > reimplement the https disk support using nbdkit (tracked in bug 2016527).
> > > 
> > > https://bugzilla.redhat.com/show_bug.cgi?id=2014229
> > > 
> > > Signed-off-by: Jiri Denemark 
> > > ---
> > >  libvirt.spec.in | 4 
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/libvirt.spec.in b/libvirt.spec.in
> > > index 4ecb28114c..b71888653d 100644
> > > --- a/libvirt.spec.in
> > > +++ b/libvirt.spec.in
> > > @@ -791,6 +791,10 @@ Requires: libvirt-daemon-driver-nwfilter = 
> > > %{version}-%{release}
> > >  Requires: libvirt-daemon-driver-secret = %{version}-%{release}
> > >  Requires: libvirt-daemon-driver-storage = %{version}-%{release}
> > >  Requires: qemu-kvm
> > > +# qemu-kvm no longer depends on curl block driver in RHEL-9
> > > +%if 0%{?rhel} > 8
> > > +Requires: qemu-kvm-block-driver-curl
> > > +%endif
> > 
> > it feels pretty dubious to me todo this in libvirt.
> > 
> > If curl driver is still required by valid use cases in RHEL-9, then
> > 'qemu-kvm' should still depend on it, until we have a replacement
> > ready.
> 
> I would agree with you if this was either permanent or the replacement
> was to be implemented in QEMU. However, this is a temporary dependency
> and once we have replacement ready, we can just drop this without having
> to synchronize with other components and without worrying about possible
> issues when QEMU and libvirt are not upgraded in a single step.

This only fixes the problem in the specific case that someone
depends on 'libvirt-daemon-kvm', while apps may well just depend
on  "Requires: libvirt-daemon-driver-qemu qemu-kvm". We need to
fix this in qemu-kvm.spec, as that's where the problem was
caused.


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 :|

Re: [PATCH 0/4] Some news and docs update for v7.9.0

[Michal Prívozník]

On 10/28/21 9:07 AM, Han Han wrote:
> 
> Han Han (4):
>   docs: Fix a typo of page_per_vq
>   news: News for the new virtio attribute page_per_vq
>   docs: Make the version requirement more clear for rbd encryption
>   news: Add support for librbd encryption
> 
>  NEWS.rst | 11 +++
>  docs/formatdomain.rst|  2 +-
>  docs/formatstorageencryption.html.in |  5 +++--
>  3 files changed, 15 insertions(+), 3 deletions(-)
> 

Reviewed-by: Michal Privoznik 

and pushed.

Michal

Re: [libvirt PATCH] spec: Depend on qemu-kvm-block-driver-curl in RHEL-9

[Jiri Denemark]

On Wed, Oct 27, 2021 at 15:38:22 +0100, Daniel P. Berrangé wrote:
> On Wed, Oct 27, 2021 at 03:58:29PM +0200, Jiri Denemark wrote:
> > The curl block driver is no longer a requirement for qemu-kvm package in
> > RHEL-9, which breaks support for https network disks. Let's depend on it
> > explicitly (in libvirt-daemon-kvm meta package) until we possibly
> > reimplement the https disk support using nbdkit (tracked in bug 2016527).
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=2014229
> > 
> > Signed-off-by: Jiri Denemark 
> > ---
> >  libvirt.spec.in | 4 
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/libvirt.spec.in b/libvirt.spec.in
> > index 4ecb28114c..b71888653d 100644
> > --- a/libvirt.spec.in
> > +++ b/libvirt.spec.in
> > @@ -791,6 +791,10 @@ Requires: libvirt-daemon-driver-nwfilter = 
> > %{version}-%{release}
> >  Requires: libvirt-daemon-driver-secret = %{version}-%{release}
> >  Requires: libvirt-daemon-driver-storage = %{version}-%{release}
> >  Requires: qemu-kvm
> > +# qemu-kvm no longer depends on curl block driver in RHEL-9
> > +%if 0%{?rhel} > 8
> > +Requires: qemu-kvm-block-driver-curl
> > +%endif
> 
> it feels pretty dubious to me todo this in libvirt.
> 
> If curl driver is still required by valid use cases in RHEL-9, then
> 'qemu-kvm' should still depend on it, until we have a replacement
> ready.

I would agree with you if this was either permanent or the replacement
was to be implemented in QEMU. However, this is a temporary dependency
and once we have replacement ready, we can just drop this without having
to synchronize with other components and without worrying about possible
issues when QEMU and libvirt are not upgraded in a single step.

Jirka

[PATCH v2 8/8] testQEMUSchemaValidateEnum: Validate deprecated members

[Peter Krempa]

Starting from QEMU-6.2 enum members can be deprecated. Add support to
the validator.

Signed-off-by: Peter Krempa 
Reviewed-by: Michal Privoznik 
---
 tests/testutilsqemuschema.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/tests/testutilsqemuschema.c b/tests/testutilsqemuschema.c
index aaa0fdaea9..4a0fb8d944 100644
--- a/tests/testutilsqemuschema.c
+++ b/tests/testutilsqemuschema.c
@@ -381,6 +381,12 @@ testQEMUSchemaValidateEnum(virJSONValue *obj,
 virJSONValue *member = virJSONValueArrayGet(members, i);

 if (STREQ_NULLABLE(objstr, virJSONValueObjectGetString(member, 
"name"))) {
+int rc;
+
+/* the new 'members' array allows us to check deprecations */
+if ((rc = testQEMUSchemaValidateDeprecated(member, objstr, 
ctxt)) < 0)
+return rc;
+
 virBufferAsprintf(ctxt->debug, "'%s' OK", NULLSTR(objstr));
 return 0;
 }
-- 
2.31.1

[PATCH v2 7/8] testQEMUSchemaValidateDeprecated: Move to the top

[Peter Krempa]

Move the function to the top of the file so other functions placed
towards the top will be able to reuse it.

Signed-off-by: Peter Krempa 
Reviewed-by: Michal Privoznik 
---
 tests/testutilsqemuschema.c | 82 ++---
 1 file changed, 41 insertions(+), 41 deletions(-)

diff --git a/tests/testutilsqemuschema.c b/tests/testutilsqemuschema.c
index dfb1add90b..aaa0fdaea9 100644
--- a/tests/testutilsqemuschema.c
+++ b/tests/testutilsqemuschema.c
@@ -29,6 +29,47 @@ struct testQEMUSchemaValidateCtxt {
 };


+static int
+testQEMUSchemaValidateDeprecated(virJSONValue *root,
+ const char *name,
+ struct testQEMUSchemaValidateCtxt *ctxt)
+{
+virJSONValue *features = virJSONValueObjectGetArray(root, "features");
+size_t nfeatures;
+size_t i;
+
+if (!features)
+return 0;
+
+nfeatures = virJSONValueArraySize(features);
+
+for (i = 0; i < nfeatures; i++) {
+virJSONValue *cur = virJSONValueArrayGet(features, i);
+const char *curstr;
+
+if (!cur ||
+!(curstr = virJSONValueGetString(cur))) {
+virBufferAsprintf(ctxt->debug, "ERROR: features of '%s' are 
malformed", name);
+return -2;
+}
+
+if (STREQ(curstr, "deprecated")) {
+if (ctxt->allowDeprecated) {
+virBufferAsprintf(ctxt->debug, "WARNING: '%s' is deprecated", 
name);
+if (virTestGetVerbose())
+g_fprintf(stderr, "\nWARNING: '%s' is deprecated\n", name);
+return 0;
+} else {
+virBufferAsprintf(ctxt->debug, "ERROR: '%s' is deprecated", 
name);
+return -1;
+}
+}
+}
+
+return 0;
+}
+
+
 static int
 testQEMUSchemaValidateRecurse(virJSONValue *obj,
   virJSONValue *root,
@@ -466,47 +507,6 @@ testQEMUSchemaValidateAlternate(virJSONValue *obj,
 }


-static int
-testQEMUSchemaValidateDeprecated(virJSONValue *root,
- const char *name,
- struct testQEMUSchemaValidateCtxt *ctxt)
-{
-virJSONValue *features = virJSONValueObjectGetArray(root, "features");
-size_t nfeatures;
-size_t i;
-
-if (!features)
-return 0;
-
-nfeatures = virJSONValueArraySize(features);
-
-for (i = 0; i < nfeatures; i++) {
-virJSONValue *cur = virJSONValueArrayGet(features, i);
-const char *curstr;
-
-if (!cur ||
-!(curstr = virJSONValueGetString(cur))) {
-virBufferAsprintf(ctxt->debug, "ERROR: features of '%s' are 
malformed", name);
-return -2;
-}
-
-if (STREQ(curstr, "deprecated")) {
-if (ctxt->allowDeprecated) {
-virBufferAsprintf(ctxt->debug, "WARNING: '%s' is deprecated", 
name);
-if (virTestGetVerbose())
-g_fprintf(stderr, "\nWARNING: '%s' is deprecated\n", name);
-return 0;
-} else {
-virBufferAsprintf(ctxt->debug, "ERROR: '%s' is deprecated", 
name);
-return -1;
-}
-}
-}
-
-return 0;
-}
-
-
 static int
 testQEMUSchemaValidateRecurse(virJSONValue *obj,
   virJSONValue *root,
-- 
2.31.1

[PATCH v2 5/8] testQEMUSchemaValidateEnum: Refactor logic to simplify switching to new QMP schema format

[Peter Krempa]

QEMU-6.2 is reporting enum values in the new 'members' array which we'll
be switching to. Rewrite the logic so that adding the new checker is
more straightforward.

Signed-off-by: Peter Krempa 
Reviewed-by: Michal Privoznik 
---
 tests/testutilsqemuschema.c | 29 ++---
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/tests/testutilsqemuschema.c b/tests/testutilsqemuschema.c
index 4f9db98e07..82c5994604 100644
--- a/tests/testutilsqemuschema.c
+++ b/tests/testutilsqemuschema.c
@@ -324,7 +324,6 @@ testQEMUSchemaValidateEnum(virJSONValue *obj,
 {
 const char *objstr;
 virJSONValue *values = NULL;
-virJSONValue *value;
 size_t i;

 if (virJSONValueGetType(obj) != VIR_JSON_TYPE_STRING) {
@@ -334,24 +333,24 @@ testQEMUSchemaValidateEnum(virJSONValue *obj,

 objstr = virJSONValueGetString(obj);

-if (!(values = virJSONValueObjectGetArray(root, "values"))) {
-virBufferAsprintf(ctxt->debug, "ERROR: missing enum values in schema 
'%s'",
-  NULLSTR(virJSONValueObjectGetString(root, "name")));
-return -2;
-}
-
-for (i = 0; i < virJSONValueArraySize(values); i++) {
-value = virJSONValueArrayGet(values, i);
+if ((values = virJSONValueObjectGetArray(root, "values"))) {
+for (i = 0; i < virJSONValueArraySize(values); i++) {
+virJSONValue *value = virJSONValueArrayGet(values, i);

-if (STREQ_NULLABLE(objstr, virJSONValueGetString(value))) {
-virBufferAsprintf(ctxt->debug, "'%s' OK", NULLSTR(objstr));
-return 0;
+if (STREQ_NULLABLE(objstr, virJSONValueGetString(value))) {
+virBufferAsprintf(ctxt->debug, "'%s' OK", NULLSTR(objstr));
+return 0;
+}
 }
+
+virBufferAsprintf(ctxt->debug, "ERROR: enum value '%s' is not in 
schema",
+  NULLSTR(objstr));
+return -1;
 }

-virBufferAsprintf(ctxt->debug, "ERROR: enum value '%s' is not in schema",
-  NULLSTR(objstr));
-return -1;
+virBufferAsprintf(ctxt->debug, "ERROR: missing enum values in schema '%s'",
+  NULLSTR(virJSONValueObjectGetString(root, "name")));
+return -2;
 }


-- 
2.31.1

[PATCH v2 6/8] testQEMUSchemaValidateEnum: Use new 'members' for 'enum' meta type

[Peter Krempa]

Switch to the new more featured way to report enum members which will
also allow us to detect use of deprecated members.

Signed-off-by: Peter Krempa 
Reviewed-by: Michal Privoznik 
---
 tests/testutilsqemuschema.c | 17 +
 1 file changed, 17 insertions(+)

diff --git a/tests/testutilsqemuschema.c b/tests/testutilsqemuschema.c
index 82c5994604..dfb1add90b 100644
--- a/tests/testutilsqemuschema.c
+++ b/tests/testutilsqemuschema.c
@@ -324,6 +324,7 @@ testQEMUSchemaValidateEnum(virJSONValue *obj,
 {
 const char *objstr;
 virJSONValue *values = NULL;
+virJSONValue *members = NULL;
 size_t i;

 if (virJSONValueGetType(obj) != VIR_JSON_TYPE_STRING) {
@@ -333,6 +334,22 @@ testQEMUSchemaValidateEnum(virJSONValue *obj,

 objstr = virJSONValueGetString(obj);

+/* qemu-6.2 added a "members" array superseding "values" */
+if ((members = virJSONValueObjectGetArray(root, "members"))) {
+for (i = 0; i < virJSONValueArraySize(members); i++) {
+virJSONValue *member = virJSONValueArrayGet(members, i);
+
+if (STREQ_NULLABLE(objstr, virJSONValueObjectGetString(member, 
"name"))) {
+virBufferAsprintf(ctxt->debug, "'%s' OK", NULLSTR(objstr));
+return 0;
+}
+}
+
+virBufferAsprintf(ctxt->debug, "ERROR: enum value '%s' is not in 
schema",
+  NULLSTR(objstr));
+return -1;
+}
+
 if ((values = virJSONValueObjectGetArray(root, "values"))) {
 for (i = 0; i < virJSONValueArraySize(values); i++) {
 virJSONValue *value = virJSONValueArrayGet(values, i);
-- 
2.31.1

[PATCH v2 2/8] virQEMUQAPISchemaTraverseEnum: Move helper variables into loop

[Peter Krempa]

Move them closer to where they are actually used.

Signed-off-by: Peter Krempa 
Reviewed-by: Michal Privoznik 
---
 src/qemu/qemu_qapi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_qapi.c b/src/qemu/qemu_qapi.c
index 36b184b226..165ecf1180 100644
--- a/src/qemu/qemu_qapi.c
+++ b/src/qemu/qemu_qapi.c
@@ -243,8 +243,6 @@ virQEMUQAPISchemaTraverseEnum(virJSONValue *cur,
 {
 const char *query = virQEMUQAPISchemaTraverseContextNextQuery(ctxt);
 virJSONValue *values;
-virJSONValue *enumval;
-const char *value;
 size_t i;

 if (query[0] != '^')
@@ -259,6 +257,9 @@ virQEMUQAPISchemaTraverseEnum(virJSONValue *cur,
 return -2;

 for (i = 0; i < virJSONValueArraySize(values); i++) {
+virJSONValue *enumval;
+const char *value;
+
 if (!(enumval = virJSONValueArrayGet(values, i)) ||
 !(value = virJSONValueGetString(enumval)))
 continue;
-- 
2.31.1

[PATCH v2 4/8] virQEMUQAPISchemaTraverseEnum: Allow query of enume type features

[Peter Krempa]

QEMU-6.2 added feature flags for enum types. Add support for querying
them into our QMP schema query language.

Signed-off-by: Peter Krempa 
Reviewed-by: Michal Privoznik 
---
 src/qemu/qemu_qapi.c | 26 ++
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_qapi.c b/src/qemu/qemu_qapi.c
index 790f7c0fee..426db8d30d 100644
--- a/src/qemu/qemu_qapi.c
+++ b/src/qemu/qemu_qapi.c
@@ -242,6 +242,7 @@ virQEMUQAPISchemaTraverseEnum(virJSONValue *cur,
   struct virQEMUQAPISchemaTraverseContext *ctxt)
 {
 const char *query = virQEMUQAPISchemaTraverseContextNextQuery(ctxt);
+const char *featurequery = NULL;
 virJSONValue *values;
 virJSONValue *members;
 size_t i;
@@ -249,8 +250,16 @@ virQEMUQAPISchemaTraverseEnum(virJSONValue *cur,
 if (query[0] != '^')
 return 0;

-if (virQEMUQAPISchemaTraverseContextHasNextQuery(ctxt))
-return -3;
+if (virQEMUQAPISchemaTraverseContextHasNextQuery(ctxt)) {
+/* we might have a query for a feature flag of an enum value */
+featurequery = virQEMUQAPISchemaTraverseContextNextQuery(ctxt);
+
+if (*featurequery != '$' ||
+virQEMUQAPISchemaTraverseContextHasNextQuery(ctxt))
+return -3;
+
+featurequery++;
+}

 query++;

@@ -263,13 +272,21 @@ virQEMUQAPISchemaTraverseEnum(virJSONValue *cur,
 if (!member || !(name = virJSONValueObjectGetString(member, 
"name")))
 return -2;

-if (STREQ(name, query))
+if (STREQ(name, query)) {
+if (featurequery)
+return 
virQEMUQAPISchemaTraverseHasObjectFeature(featurequery, member);
+
 return 1;
+}
 }

 return 0;
 }

+/* old-style "values" array doesn't have feature flags so any query is 
necessarily false */
+if (featurequery)
+return 0;
+
 if (!(values = virJSONValueObjectGetArray(cur, "values")))
 return -2;

@@ -439,7 +456,8 @@ virQEMUQAPISchemaTraverse(const char *baseName,
  *
  * The above types can be chained arbitrarily using slashes to construct any
  * path into the schema tree, booleans must be always the last component as 
they
- * don't refer to a type.
+ * don't refer to a type. An exception is querying feature of an enum value
+ * (.../^enumval/$featurename) which is allowed.
  *
  * Returns 1 if @query was found in @schema filling @entry if non-NULL, 0 if
  * @query was not found in @schema and -1 on other errors along with an 
appropriate
-- 
2.31.1

[PATCH v2 3/8] virQEMUQAPISchemaTraverseEnum: Use the modern 'members' array

[Peter Krempa]

Starting from QEMU-6.2 enum members are reported as an array of objects
under new name "values" so that extra data can be reported for each
member.

Modify the code so that we prefer 'members' and skip 'values' completely
if we've used 'members'.

Signed-off-by: Peter Krempa 
Reviewed-by: Michal Privoznik 
---
 src/qemu/qemu_qapi.c | 17 +
 1 file changed, 17 insertions(+)

diff --git a/src/qemu/qemu_qapi.c b/src/qemu/qemu_qapi.c
index 165ecf1180..790f7c0fee 100644
--- a/src/qemu/qemu_qapi.c
+++ b/src/qemu/qemu_qapi.c
@@ -243,6 +243,7 @@ virQEMUQAPISchemaTraverseEnum(virJSONValue *cur,
 {
 const char *query = virQEMUQAPISchemaTraverseContextNextQuery(ctxt);
 virJSONValue *values;
+virJSONValue *members;
 size_t i;

 if (query[0] != '^')
@@ -253,6 +254,22 @@ virQEMUQAPISchemaTraverseEnum(virJSONValue *cur,

 query++;

+/* qemu-6.2 added a "members" array superseding "values" */
+if ((members = virJSONValueObjectGetArray(cur, "members"))) {
+for (i = 0; i < virJSONValueArraySize(members); i++) {
+virJSONValue *member = virJSONValueArrayGet(members, i);
+const char *name;
+
+if (!member || !(name = virJSONValueObjectGetString(member, 
"name")))
+return -2;
+
+if (STREQ(name, query))
+return 1;
+}
+
+return 0;
+}
+
 if (!(values = virJSONValueObjectGetArray(cur, "values")))
 return -2;

-- 
2.31.1

[PATCH v2 1/8] qemucapabilitiestest: Update capability probe for qemu-6.2 on x86_64

[Peter Krempa]

Update to v6.1.0-1735-gc52d69e7db which has Markus' patches for
improvements of enum probing.

Signed-off-by: Peter Krempa 
---
 .../caps_6.2.0.x86_64.replies | 3513 +++--
 .../caps_6.2.0.x86_64.xml |2 +-
 2 files changed, 3097 insertions(+), 418 deletions(-)

diff --git a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.replies 
b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.replies
index aa7a779a68..69d3b1b12a 100644
--- a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.replies
+++ b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.replies
@@ -21,7 +21,7 @@
   "minor": 1,
   "major": 6
 },
-"package": "v6.1.0-1510-gc148a05721"
+"package": "v6.1.0-1735-gc52d69e7db"
   },
   "id": "libvirt-2"
 }
@@ -4582,6 +4582,17 @@
 },
 {
   "name": "116",
+  "members": [
+{
+  "name": "tpm-tis"
+},
+{
+  "name": "tpm-crb"
+},
+{
+  "name": "tpm-spapr"
+}
+  ],
   "meta-type": "enum",
   "values": [
 "tpm-tis",
@@ -4596,6 +4607,14 @@
 },
 {
   "name": "117",
+  "members": [
+{
+  "name": "passthrough"
+},
+{
+  "name": "emulator"
+}
+  ],
   "meta-type": "enum",
   "values": [
 "passthrough",
@@ -7385,6 +7404,56 @@
 },
 {
   "name": "240",
+  "members": [
+{
+  "name": "debug"
+},
+{
+  "name": "inmigrate"
+},
+{
+  "name": "internal-error"
+},
+{
+  "name": "io-error"
+},
+{
+  "name": "paused"
+},
+{
+  "name": "postmigrate"
+},
+{
+  "name": "prelaunch"
+},
+{
+  "name": "finish-migrate"
+},
+{
+  "name": "restore-vm"
+},
+{
+  "name": "running"
+},
+{
+  "name": "save-vm"
+},
+{
+  "name": "shutdown"
+},
+{
+  "name": "suspended"
+},
+{
+  "name": "watchdog"
+},
+{
+  "name": "guest-panicked"
+},
+{
+  "name": "colo"
+}
+  ],
   "meta-type": "enum",
   "values": [
 "debug",
@@ -7407,6 +7476,38 @@
 },
 {
   "name": "241",
+  "members": [
+{
+  "name": "none"
+},
+{
+  "name": "host-error"
+},
+{
+  "name": "host-qmp-quit"
+},
+{
+  "name": "host-qmp-system-reset"
+},
+{
+  "name": "host-signal"
+},
+{
+  "name": "host-ui"
+},
+{
+  "name": "guest-shutdown"
+},
+{
+  "name": "guest-reset"
+},
+{
+  "name": "guest-panic"
+},
+{
+  "name": "subsystem-reset"
+}
+  ],
   "meta-type": "enum",
   "values": [
 "none",
@@ -7423,6 +7524,29 @@
 },
 {
   "name": "242",
+  "members": [
+{
+  "name": "reset"
+},
+{
+  "name": "shutdown"
+},
+{
+  "name": "poweroff"
+},
+{
+  "name": "pause"
+},
+{
+  "name": "debug"
+},
+{
+  "name": "none"
+},
+{
+  "name": "inject-nmi"
+}
+  ],
   "meta-type": "enum",
   "values": [
 "reset",
@@ -7436,6 +7560,14 @@
 },
 {
   "name": "243",
+  "members": [
+{
+  "name": "reset"
+},
+{
+  "name": "shutdown"
+}
+  ],
   "meta-type": "enum",
   "values": [
 "reset",
@@ -7444,6 +7576,14 @@
 },
 {
   "name": "244",
+  "members": [
+{
+  "name": "poweroff"
+},
+{
+  "name": "pause"
+}
+  ],
   "meta-type": "enum",
   "values": [
 "poweroff",
@@ -7452,6 +7592,17 @@
 },
 {
   "name": "245",
+  "members": [
+{
+  "name": "pause"
+},
+{
+  "name": "shutdown"
+},
+{
+  "name": "none"
+}
+  ],
   "meta-type": "enum",
   "values": [
 "pause",
@@ -7461,6 +7612,17 @@
 },
 {
   "name": "246",
+  "members": [
+{
+  "name": "pause"
+},
+{
+  "name": "poweroff"
+},
+{
+  "name": "run"
+}
+  ],
   "meta-type": "enum",
   "values": [
 "pause",
@@ -7491,6 +7653,14 @@
 },
 {
   "name": "248",
+  "members": [
+{
+  "name": "hypervisor"
+},
+{
+  "name": "guest"
+}
+  ],
   "meta-type": "enum",
   "values": [
 "hypervisor",
@@ 

[PATCH v2 0/8] qemu: QAPI-schema: Add support for enum value 'features'

[Peter Krempa]

Patches 2-8 were already reviewed by Michal. 1/8 is new as the qemu
functionality was just merged upstream.

Peter Krempa (8):
  qemucapabilitiestest: Update capability probe for qemu-6.2 on x86_64
  virQEMUQAPISchemaTraverseEnum: Move helper variables into loop
  virQEMUQAPISchemaTraverseEnum: Use the modern 'members' array
  virQEMUQAPISchemaTraverseEnum: Allow query of enume type features
  testQEMUSchemaValidateEnum: Refactor logic to simplify switching to
new QMP schema format
  testQEMUSchemaValidateEnum: Use new 'members' for 'enum' meta type
  testQEMUSchemaValidateDeprecated: Move to the top
  testQEMUSchemaValidateEnum: Validate deprecated members

 src/qemu/qemu_qapi.c  |   46 +-
 .../caps_6.2.0.x86_64.replies | 3513 +++--
 .../caps_6.2.0.x86_64.xml |2 +-
 tests/testutilsqemuschema.c   |  130 +-
 4 files changed, 3214 insertions(+), 477 deletions(-)

-- 
2.31.1

[PATCH] ch_driver: Drop needless fwd declaration

[Michal Privoznik]

In ch_driver.c there are two forward declarations that are not
needed. Drop them.

Signed-off-by: Michal Privoznik 
---
 src/ch/ch_driver.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
index 1824d2fd16..9eaf3ee499 100644
--- a/src/ch/ch_driver.c
+++ b/src/ch/ch_driver.c
@@ -47,11 +47,6 @@
 
 VIR_LOG_INIT("ch.ch_driver");
 
-static int chStateInitialize(bool privileged,
- const char *root,
- virStateInhibitCallback callback,
- void *opaque);
-static int chStateCleanup(void);
 virCHDriver *ch_driver = NULL;
 
 static virDomainObj *
-- 
2.32.0

Re: [libvirt PATCH 08/13] ch_cgroup: methods for cgroup mgmt in ch driver

[Michal Prívozník]

On 10/22/21 5:37 PM, Praveen K Paladugu wrote:
> From: Vineeth Pillai 
> 
> Signed-off-by: Vineeth Pillai 
> Signed-off-by: Praveen K Paladugu 
> ---
>  po/POTFILES.in  |   1 +
>  src/ch/ch_cgroup.c  | 457 
>  src/ch/ch_cgroup.h  |  45 +
>  src/ch/ch_conf.c|   2 +
>  src/ch/ch_conf.h|   4 +-
>  src/ch/ch_domain.c  |  33 
>  src/ch/ch_domain.h  |   3 +-
>  src/ch/ch_monitor.c | 125 ++--
>  src/ch/ch_monitor.h |  54 +-
>  src/ch/ch_process.c | 288 +++-
>  src/ch/ch_process.h |   3 +
>  src/ch/meson.build  |   2 +
>  12 files changed, 991 insertions(+), 26 deletions(-)
>  create mode 100644 src/ch/ch_cgroup.c
>  create mode 100644 src/ch/ch_cgroup.h
> 
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index b554cf08ca..3a8db501bc 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -19,6 +19,7 @@
>  @SRCDIR@src/bhyve/bhyve_parse_command.c
>  @SRCDIR@src/bhyve/bhyve_process.c
>  @SRCDIR@src/ch/ch_conf.c
> +@SRCDIR@src/ch/ch_cgroup.c
>  @SRCDIR@src/ch/ch_domain.c
>  @SRCDIR@src/ch/ch_driver.c
>  @SRCDIR@src/ch/ch_monitor.c
> diff --git a/src/ch/ch_cgroup.c b/src/ch/ch_cgroup.c
> new file mode 100644
> index 00..6be2184cf1
> --- /dev/null
> +++ b/src/ch/ch_cgroup.c
> @@ -0,0 +1,457 @@
> +/*
> + * ch_cgroup.c: CH cgroup management

This file is a verbatim copy of qemu_cgroup.c (except for some
formatting shenanigans). I wonder whether instead of copying code we can
move it under hypervisor agnostic location (src/hypervisor/) and then
only call respective functions from either of drivers. What do you think?

Michal

Re: [libvirt PATCH 07/13] ch_monitor: Get nicindexes in prep for cgroup mgmt

[Michal Prívozník]

On 10/22/21 5:37 PM, Praveen K Paladugu wrote:
> From: Vineeth Pillai 
> 
> Signed-off-by: Vineeth Pillai 
> Signed-off-by: Praveen Paladugu 
> Signed-off-by: Praveen K Paladugu 
> ---
>  src/ch/ch_conf.h|  5 +++
>  src/ch/ch_domain.c  | 26 +-
>  src/ch/ch_domain.h  |  4 +--
>  src/ch/ch_driver.c  |  4 ++-
>  src/ch/ch_monitor.c | 85 +
>  src/ch/ch_monitor.h | 14 +++-
>  src/ch/ch_process.c |  6 +++-
>  src/ch/meson.build  |  1 +
>  8 files changed, 132 insertions(+), 13 deletions(-)
> 
> diff --git a/src/ch/ch_conf.h b/src/ch/ch_conf.h
> index 37c36d9a09..49f286f97a 100644
> --- a/src/ch/ch_conf.h
> +++ b/src/ch/ch_conf.h
> @@ -44,6 +44,11 @@ struct _virCHDriver
>  {
>  virMutex lock;
>  
> +bool privileged;
> +
> +/* Embedded Mode: Not yet supported */
> +char *embeddedRoot;

I'd rather not introduce this member and pass NULL to
virDomainDriverGenerateMachineName() below directly. embeddedRoot is
completely different beast and it may create false beliefs upon seeing
this declaration.

Michal

Re: [libvirt PATCH 02/13] ch: Explicitly link to virt_util_lib

[Michal Prívozník]

On 10/22/21 5:37 PM, Praveen K Paladugu wrote:
> From: Vineeth Pillai 
> 
> link to virt_util_lib while building ch driver.
> 
> Signed-off-by: Vineeth Pillai 
> Signed-off-by: Praveen K Paladugu 
> ---
>  src/ch/meson.build | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/ch/meson.build b/src/ch/meson.build
> index e34974d56c..5c6cab2a9f 100644
> --- a/src/ch/meson.build
> +++ b/src/ch/meson.build
> @@ -30,6 +30,9 @@ if conf.has('WITH_CH')
>  include_directories: [
>conf_inc_dir,
>  ],
> +link_with: [
> +  virt_util_lib,
> +]
>)
>  
>virt_modules += {
> 

IIUC this is only needed because of missing libvirt_private.syms change
(pointed out in 1/13). After the suggested change is done this patch
shouldn't be needed.

Michal

Re: [libvirt PATCH 07/13] ch_monitor: Get nicindexes in prep for cgroup mgmt

[Michal Prívozník]

On 10/22/21 5:37 PM, Praveen K Paladugu wrote:
> From: Vineeth Pillai 
> 
> Signed-off-by: Vineeth Pillai 
> Signed-off-by: Praveen Paladugu 
> Signed-off-by: Praveen K Paladugu 
> ---
>  src/ch/ch_conf.h|  5 +++
>  src/ch/ch_domain.c  | 26 +-
>  src/ch/ch_domain.h  |  4 +--
>  src/ch/ch_driver.c  |  4 ++-
>  src/ch/ch_monitor.c | 85 +
>  src/ch/ch_monitor.h | 14 +++-
>  src/ch/ch_process.c |  6 +++-
>  src/ch/meson.build  |  1 +
>  8 files changed, 132 insertions(+), 13 deletions(-)
> 
> diff --git a/src/ch/ch_conf.h b/src/ch/ch_conf.h
> index 37c36d9a09..49f286f97a 100644
> --- a/src/ch/ch_conf.h
> +++ b/src/ch/ch_conf.h
> @@ -44,6 +44,11 @@ struct _virCHDriver
>  {
>  virMutex lock;
>  
> +bool privileged;
> +
> +/* Embedded Mode: Not yet supported */
> +char *embeddedRoot;
> +
>  /* Require lock to get a reference on the object,
>   * lockless access thereafter */
>  virCaps *caps;
> diff --git a/src/ch/ch_domain.c b/src/ch/ch_domain.c
> index c0b0b1005a..e1030800aa 100644
> --- a/src/ch/ch_domain.c
> +++ b/src/ch/ch_domain.c
> @@ -21,10 +21,12 @@
>  #include 
>  
>  #include "ch_domain.h"
> +#include "domain_driver.h"
>  #include "viralloc.h"
>  #include "virchrdev.h"
>  #include "virlog.h"
>  #include "virtime.h"
> +#include "virsystemd.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_CH
>  
> @@ -136,7 +138,7 @@ virCHDomainObjEndJob(virDomainObj *obj)
>  }
>  
>  static void *
> -virCHDomainObjPrivateAlloc(void *opaque G_GNUC_UNUSED)
> +virCHDomainObjPrivateAlloc(void *opaque)
>  {
>  virCHDomainObjPrivate *priv;
>  
> @@ -152,6 +154,7 @@ virCHDomainObjPrivateAlloc(void *opaque G_GNUC_UNUSED)
>  g_free(priv);
>  return NULL;
>  }
> +priv->driver = opaque;
>  
>  return priv;
>  }
> @@ -363,3 +366,24 @@ virCHDomainHasVcpuPids(virDomainObj *vm)
>  
>  return false;
>  }
> +
> +char *virCHDomainGetMachineName(virDomainObj *vm)
> +{
> +virCHDomainObjPrivate *priv = CH_DOMAIN_PRIVATE(vm);
> +virCHDriver *driver = priv->driver;
> +char *ret = NULL;
> +
> +if (vm->pid > 0) {
> +ret = virSystemdGetMachineNameByPID(vm->pid);
> +if (!ret)
> +virResetLastError();
> +}
> +
> +if (!ret)
> +ret = virDomainDriverGenerateMachineName("ch",
> + driver->embeddedRoot,
> + vm->def->id, vm->def->name,
> + driver->privileged);
> +
> +return ret;
> +}
> diff --git a/src/ch/ch_domain.h b/src/ch/ch_domain.h
> index e35777a9ec..3ac3421015 100644
> --- a/src/ch/ch_domain.h
> +++ b/src/ch/ch_domain.h
> @@ -54,9 +54,7 @@ struct _virCHDomainObjPrivate {
>  struct virCHDomainJobObj job;
>  
>  virChrdevs *chrdevs;
> -
>  virCgroup *cgroup;
> -

These extra spaces were introduced just a few patches back. Could you
change those so that you avoid removing them here?

>  virCHDriver *driver;
>  virCHMonitor *monitor;
>  char *machineName;
> @@ -94,3 +92,5 @@ virCHDomainObjEndJob(virDomainObj *obj);
>  int virCHDomainRefreshVcpuInfo(virDomainObj *vm);
>  pid_t virCHDomainGetVcpuPid(virDomainObj *vm, unsigned int vcpuid);
>  bool virCHDomainHasVcpuPids(virDomainObj *vm);
> +
> +char *virCHDomainGetMachineName(virDomainObj *vm);
> diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
> index 62ca6c1994..ca854da123 100644
> --- a/src/ch/ch_driver.c
> +++ b/src/ch/ch_driver.c
> @@ -944,7 +944,9 @@ static int chStateInitialize(bool privileged,
>  goto cleanup;
>  }
>  
> -ret = VIR_DRV_STATE_INIT_COMPLETE;
> +ch_driver->privileged = privileged;
> +
> +return VIR_DRV_STATE_INIT_COMPLETE;

Storing @privileged is okay, but changing ret to return doesn't feel
right. If you really want to do that then rename cleanup to error and
drop if() there since the condition will always be true (keep the body
thogh!). Or just don't change ret to return.

>  
>   cleanup:
>  if (ret != VIR_DRV_STATE_INIT_COMPLETE)
> diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
> index 691d1ce64b..c0ae031200 100644
> --- a/src/ch/ch_monitor.c
> +++ b/src/ch/ch_monitor.c
> @@ -228,7 +228,8 @@ virCHMonitorBuildDisksJson(virJSONValue *content, 
> virDomainDef *vmdef)
>  }
>  
>  static int
> -virCHMonitorBuildNetJson(virJSONValue *nets, virDomainNetDef *netdef)
> +virCHMonitorBuildNetJson(virJSONValue *nets, virDomainNetDef *netdef,
> +size_t *nnicindexes, int **nicindexes)
>  {
>  virDomainNetType netType = virDomainNetGetActualType(netdef);
>  char macaddr[VIR_MAC_STRING_BUFLEN];
> @@ -263,6 +264,18 @@ virCHMonitorBuildNetJson(virJSONValue *nets, 
> virDomainNetDef *netdef)
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("ethernet type supports a single guest ip"));
>  }
> +
> +/* network and 

Re: [libvirt PATCH 05/13] ch_driver, ch_domain: vcpu info getter callbacks

[Michal Prívozník]

On 10/22/21 5:37 PM, Praveen K Paladugu wrote:
> From: Vineeth Pillai 
> 
> Signed-off-by: Vineeth Pillai 
> Signed-off-by: Praveen K Paladugu 
> ---
>  src/ch/ch_domain.c |  25 
>  src/ch/ch_domain.h |   4 ++
>  src/ch/ch_driver.c | 138 +
>  3 files changed, 167 insertions(+)
> 
> diff --git a/src/ch/ch_domain.c b/src/ch/ch_domain.c
> index fedde4581b..c0b0b1005a 100644
> --- a/src/ch/ch_domain.c
> +++ b/src/ch/ch_domain.c
> @@ -338,3 +338,28 @@ virCHDomainGetMonitor(virDomainObj *vm)
>  {
>  return CH_DOMAIN_PRIVATE(vm)->monitor;
>  }
> +
> +pid_t
> +virCHDomainGetVcpuPid(virDomainObj *vm,
> + unsigned int vcpuid)
> +{
> +virDomainVcpuDef *vcpu = virDomainDefGetVcpu(vm->def, vcpuid);
> +return CH_DOMAIN_VCPU_PRIVATE(vcpu)->tid;
> +}
> +
> +bool
> +virCHDomainHasVcpuPids(virDomainObj *vm)
> +{
> +size_t i;
> +size_t maxvcpus = virDomainDefGetVcpusMax(vm->def);
> +virDomainVcpuDef *vcpu;
> +
> +for (i = 0; i < maxvcpus; i++) {
> +vcpu = virDomainDefGetVcpu(vm->def, i);
> +
> +if (CH_DOMAIN_VCPU_PRIVATE(vcpu)->tid > 0)
> +return true;
> +}
> +
> +return false;
> +}
> diff --git a/src/ch/ch_domain.h b/src/ch/ch_domain.h
> index 75b9933130..d9c9d34a19 100644
> --- a/src/ch/ch_domain.h
> +++ b/src/ch/ch_domain.h
> @@ -82,3 +82,7 @@ virCHDomainObjBeginJob(virDomainObj *obj, enum 
> virCHDomainJob job)
>  
>  void
>  virCHDomainObjEndJob(virDomainObj *obj);
> +
> +int virCHDomainRefreshVcpuInfo(virDomainObj *vm);
> +pid_t virCHDomainGetVcpuPid(virDomainObj *vm, unsigned int vcpuid);
> +bool virCHDomainHasVcpuPids(virDomainObj *vm);
> diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
> index 1824d2fd16..8ea5ce393d 100644
> --- a/src/ch/ch_driver.c
> +++ b/src/ch/ch_driver.c
> @@ -952,6 +952,141 @@ static int chStateInitialize(bool privileged,
>  return ret;
>  }
>  
> +static int
> +chDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags)
> +{
> +virDomainObj *vm;
> +virDomainDef *def;
> +int ret = -1;
> +
> +virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> +  VIR_DOMAIN_AFFECT_CONFIG |
> +  VIR_DOMAIN_VCPU_MAXIMUM |
> +  VIR_DOMAIN_VCPU_GUEST, -1);
> +
> +if (!(vm = chDomObjFromDomain(dom)))
> +return -1;
> +
> +if (virDomainGetVcpusFlagsEnsureACL(dom->conn, vm->def, flags) < 0)
> +goto cleanup;
> +
> +if (!(def = virDomainObjGetOneDef(vm, flags)))
> +goto cleanup;
> +
> +if (flags & VIR_DOMAIN_VCPU_MAXIMUM)
> +ret = virDomainDefGetVcpusMax(def);
> +else
> +ret = virDomainDefGetVcpus(def);
> +
> +
> +cleanup:
> +virDomainObjEndAPI();
> +return ret;
> +}
> +
> +static int
> +chDomainGetMaxVcpus(virDomainPtr dom)
> +{
> +return chDomainGetVcpusFlags(dom, (VIR_DOMAIN_AFFECT_LIVE | 
> VIR_DOMAIN_VCPU_MAXIMUM));
> +}
> +
> +static int
> +chDomainHelperGetVcpus(virDomainObj *vm,
> +   virVcpuInfoPtr info,
> +   unsigned long long *cpuwait,
> +   int maxinfo,
> +   unsigned char *cpumaps,
> +   int maplen)
> +{
> +size_t ncpuinfo = 0;
> +size_t i;
> +
> +if (maxinfo == 0)
> +return 0;
> +
> +if (!virCHDomainHasVcpuPids(vm)) {
> +virReportError(VIR_ERR_OPERATION_INVALID,
> +   "%s", _("cpu affinity is not supported"));
> +return -1;
> +}
> +
> +if (info)
> +memset(info, 0, sizeof(*info) * maxinfo);
> +
> +if (cpumaps)
> +memset(cpumaps, 0, sizeof(*cpumaps) * maxinfo);
> +
> +for (i = 0; i < virDomainDefGetVcpusMax(vm->def) && ncpuinfo < maxinfo; 
> i++) {
> +virDomainVcpuDef *vcpu = virDomainDefGetVcpu(vm->def, i);
> +pid_t vcpupid = virCHDomainGetVcpuPid(vm, i);
> +virVcpuInfoPtr vcpuinfo = info + ncpuinfo;
> +
> +if (!vcpu->online)
> +continue;
> +
> +if (info) {
> +vcpuinfo->number = i;
> +vcpuinfo->state = VIR_VCPU_RUNNING;
> +if (virProcessGetStatInfo(>cpuTime,
> +  >cpu, NULL,
> +  vm->pid, vcpupid) < 0) {
> +virReportSystemError(errno, "%s",
> + _("cannot get vCPU placement & pCPU 
> time"));
> +return -1;
> +}
> +}
> +
> +if (cpumaps) {
> +unsigned char *cpumap = VIR_GET_CPUMAP(cpumaps, maplen, 
> ncpuinfo);
> +virBitmap *map = NULL;

g_autoptr(virBitmap) ..

> +
> +if (!(map = virProcessGetAffinity(vcpupid)))
> +return -1;
> +
> +virBitmapToDataBuf(map, cpumap, maplen);
> +virBitmapFree(map);

.. and drop this explicit virBitmapFree().

> +}
> +
> +if (cpuwait) {
> +if 

Re: [libvirt PATCH 00/13] cgroup and thread management in ch driver.

[Michal Prívozník]

On 10/22/21 5:37 PM, Praveen K Paladugu wrote:
> This patchset adds support for cgroup management of ch threads. This version
> correctly manages cgroups for vcpu and emulator threads created by ch. cgroup
> management for iothreads is not yet supported.
> 
> Along with cgroup management, this patchset also enables support for pinning
> vcpu and emulator threads to selected host cpus.
> 
> Praveen K Paladugu (2):
>   ch_process: Setup emulator and iothread settings
>   ch_driver: emulator threadinfo & pinning callbacks
> 
> Vineeth Pillai (11):
>   util: Helper functions to get process info
>   ch: Explicitly link to virt_util_lib
>   ch_domain: add virCHDomainGetMonitor helper method
>   ch_domain: add methods to manage private vcpu data
>   ch_driver,ch_domain: vcpu info getter callbacks
>   ch_driver: domainGetVcpuPinInfo and nodeGetCPUMap
>   ch_monitor: Get nicindexes in prep for cgroup mgmt
>   ch_cgroup: methods for cgroup mgmt in ch driver
>   ch_driver,ch_domain: vcpupin callback in ch driver
>   ch_driver: enable typed param string for numatune
>   ch_driver: add numatune callbacks for CH driver
> 
>  po/POTFILES.in|   1 +
>  src/ch/ch_cgroup.c| 457 
>  src/ch/ch_cgroup.h|  45 +++
>  src/ch/ch_conf.c  |   2 +
>  src/ch/ch_conf.h  |   9 +-
>  src/ch/ch_domain.c| 170 -
>  src/ch/ch_domain.h|  32 +-
>  src/ch/ch_driver.c| 810 +-
>  src/ch/ch_monitor.c   | 254 -
>  src/ch/ch_monitor.h   |  60 +++-
>  src/ch/ch_process.c   | 368 ++-
>  src/ch/ch_process.h   |   3 +
>  src/ch/meson.build|   6 +
>  src/util/virprocess.c | 136 +++
>  src/util/virprocess.h |   5 +
>  15 files changed, 2329 insertions(+), 29 deletions(-)
>  create mode 100644 src/ch/ch_cgroup.c
>  create mode 100644 src/ch/ch_cgroup.h
> 

Firstly, apologies for reviewing only after the freeze.
Secondly, patches look more or less good. I've pointed out couple of
things I think are worth addressing in v2.

Michal

Re: [libvirt PATCH 01/13] util: Helper functions to get process info

[Michal Prívozník]

On 10/22/21 5:37 PM, Praveen K Paladugu wrote:
> From: Vineeth Pillai 
> 
> These helper methods are used to capture vcpu information
> in ch driver.
> 
> Signed-off-by: Vineeth Pillai 
> Signed-off-by: Praveen K Paladugu 
> ---
>  src/util/virprocess.c | 136 ++
>  src/util/virprocess.h |   5 ++
>  2 files changed, 141 insertions(+)
> 
> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> index 6de3f36f52..0164d70df6 100644
> --- a/src/util/virprocess.c
> +++ b/src/util/virprocess.c
> @@ -1721,3 +1721,139 @@ virProcessSetScheduler(pid_t pid G_GNUC_UNUSED,
>  }
>  
>  #endif /* !WITH_SCHED_SETSCHEDULER */
> +
> +/*
> +TODO: This method was cloned from qemuGetProcessInfo in 
> src/qemu/qemu_driver.c.
> +Need to refactor qemu driver to use this shared function.

There's no harm in doing that in this patch. In fact, it's desired. You
can move a qemu function into src/util, rename it and fix all places
where it is called, all in one patch.

Also, please don't forget to export this function in
src/libvirt_private.syms.

> +*/
> +int
> +virProcessGetStatInfo(unsigned long long *cpuTime, int *lastCpu, long 
> *vm_rss,
> +   pid_t pid, pid_t tid)

Indentation's off. I've noticed other patches in the series suffer from
mis-indentation too. Please fix that in another version.

> +{
> +g_autofree char *proc = NULL;
> +FILE *pidinfo;
> +unsigned long long usertime = 0, systime = 0;

I wonder whether we should take this opportunity and declare these two
variables at one line each.

> +long rss = 0;
> +int cpu = 0;

Michal

Re: [libvirt PATCH 09/13] ch_driver, ch_domain: vcpupin callback in ch driver

[Michal Prívozník]

On 10/22/21 5:37 PM, Praveen K Paladugu wrote:
> From: Vineeth Pillai 
> 
> Signed-off-by: Vineeth Pillai 
> Signed-off-by: Praveen K Paladugu 
> ---
>  src/ch/ch_domain.c |  30 +
>  src/ch/ch_domain.h |   1 +
>  src/ch/ch_driver.c | 151 +
>  3 files changed, 182 insertions(+)
> 
> diff --git a/src/ch/ch_domain.c b/src/ch/ch_domain.c
> index d0aaeed1f4..9ad00583aa 100644
> --- a/src/ch/ch_domain.c
> +++ b/src/ch/ch_domain.c
> @@ -20,6 +20,7 @@
>  
>  #include 
>  
> +#include "datatypes.h"
>  #include "ch_domain.h"
>  #include "domain_driver.h"
>  #include "viralloc.h"
> @@ -420,3 +421,32 @@ char *virCHDomainGetMachineName(virDomainObj *vm)
>  
>  return ret;
>  }
> +
> +/**
> + * virCHDomainObjFromDomain:
> + * @domain: Domain pointer that has to be looked up
> + *
> + * This function looks up @domain and returns the appropriate virDomainObjPtr
> + * that has to be released by calling virDomainObjEndAPI().
> + *
> + * Returns the domain object with incremented reference counter which is 
> locked
> + * on success, NULL otherwise.
> + */
> +virDomainObj *
> +virCHDomainObjFromDomain(virDomainPtr domain)
> +{
> +virDomainObj *vm;
> +virCHDriver *driver = domain->conn->privateData;
> +char uuidstr[VIR_UUID_STRING_BUFLEN];
> +
> +vm = virDomainObjListFindByUUID(driver->domains, domain->uuid);
> +if (!vm) {
> +virUUIDFormat(domain->uuid, uuidstr);
> +virReportError(VIR_ERR_NO_DOMAIN,
> +   _("no domain with matching uuid '%s' (%s)"),
> +   uuidstr, domain->name);
> +return NULL;
> +}
> +
> +return vm;
> +}
> diff --git a/src/ch/ch_domain.h b/src/ch/ch_domain.h
> index 2ce3e2cef3..db1451405b 100644
> --- a/src/ch/ch_domain.h
> +++ b/src/ch/ch_domain.h
> @@ -95,3 +95,4 @@ pid_t virCHDomainGetVcpuPid(virDomainObj *vm, unsigned int 
> vcpuid);
>  bool virCHDomainHasVcpuPids(virDomainObj *vm);
>  
>  char *virCHDomainGetMachineName(virDomainObj *vm);
> +virDomainObj *virCHDomainObjFromDomain(virDomain *domain);
> diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
> index ca854da123..7f3cf6dbef 100644
> --- a/src/ch/ch_driver.c
> +++ b/src/ch/ch_driver.c
> @@ -25,6 +25,7 @@
>  #include "ch_driver.h"
>  #include "ch_monitor.h"
>  #include "ch_process.h"
> +#include "ch_cgroup.h"
>  #include "datatypes.h"
>  #include "driver.h"
>  #include "viraccessapicheck.h"
> @@ -1141,6 +1142,154 @@ chDomainGetVcpus(virDomainPtr dom,
>  return ret;
>  }
>  
> +static int
> +chDomainPinVcpuLive(virDomainObj *vm,
> +virDomainDef *def,
> +int vcpu,
> +virCHDriver *driver,
> +virCHDriverConfig *cfg,
> +virBitmap *cpumap)
> +{
> +virBitmap *tmpmap = NULL;
> +virDomainVcpuDef *vcpuinfo;
> +virCHDomainObjPrivate *priv = vm->privateData;
> +virCgroup *cgroup_vcpu = NULL;
> +g_autofree char *str = NULL;
> +int ret = -1;
> +
> +if (!virCHDomainHasVcpuPids(vm)) {
> +virReportError(VIR_ERR_OPERATION_INVALID,
> +   "%s", _("cpu affinity is not supported"));
> +goto cleanup;
> +}
> +
> +if (!(vcpuinfo = virDomainDefGetVcpu(def, vcpu))) {
> +virReportError(VIR_ERR_INVALID_ARG,
> +   _("vcpu %d is out of range of live cpu count %d"),
> +   vcpu, virDomainDefGetVcpusMax(def));
> +goto cleanup;
> +}
> +
> +if (!(tmpmap = virBitmapNewCopy(cpumap)))
> +goto cleanup;
> +
> +if (!(str = virBitmapFormat(cpumap)))
> +goto cleanup;
> +
> +if (vcpuinfo->online) {
> +/* Configure the corresponding cpuset cgroup before set affinity. */
> +if (virCgroupHasController(priv->cgroup, 
> VIR_CGROUP_CONTROLLER_CPUSET)) {
> +if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_VCPU, 
> vcpu,
> +   false, _vcpu) < 0)
> +goto cleanup;
> +if (chSetupCgroupCpusetCpus(cgroup_vcpu, cpumap) < 0)
> +goto cleanup;
> +}
> +
> +if (virProcessSetAffinity(virCHDomainGetVcpuPid(vm, vcpu), cpumap, 
> false) < 0)
> +goto cleanup;
> +}
> +
> +virBitmapFree(vcpuinfo->cpumask);
> +vcpuinfo->cpumask = tmpmap;
> +tmpmap = NULL;
> +
> +if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0)
> +goto cleanup;
> +
> +ret = 0;
> +
> + cleanup:
> +virBitmapFree(tmpmap);
> +virCgroupFree(cgroup_vcpu);
> +return ret;
> +}
> +
> +
> +static int
> +chDomainPinVcpuFlags(virDomainPtr dom,
> + unsigned int vcpu,
> + unsigned char *cpumap,
> + int maplen,
> + unsigned int flags)
> +{
> +virCHDriver *driver = dom->conn->privateData;
> +virDomainObj *vm;
> +virDomainDef *def;
> +virDomainDef *persistentDef;
> +int 

Re: [PATCH v2 4/9] qapi: Tools for sets of special feature flags in generated code

[Philippe Mathieu-Daudé]

On 10/28/21 12:25, Markus Armbruster wrote:
> New enum QapiSpecialFeature enumerates the special feature flags.
> 
> New helper gen_special_features() returns code to represent a
> collection of special feature flags as a bitset.
> 
> The next few commits will put them to use.
> 
> Signed-off-by: Markus Armbruster 
> Reviewed-by: John Snow 
> ---
>  include/qapi/util.h| 4 
>  scripts/qapi/gen.py| 8 
>  scripts/qapi/schema.py | 3 +++
>  3 files changed, 15 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v2 7/9] qapi: Generalize enum member policy checking

[Philippe Mathieu-Daudé]

On 10/28/21 12:25, Markus Armbruster wrote:
> The code to check enumeration value policy can see special feature
> flag 'deprecated' in QEnumLookup member flags[value].  I want to make
> feature flag 'unstable' visible there as well, so I can add policy for
> it.
> 
> Instead of extending flags[], replace it by @special_features (a
> bitset of QapiSpecialFeature), because that's how special features get
> passed around elsewhere.
> 
> Signed-off-by: Markus Armbruster 
> Acked-by: John Snow 
> ---
>  include/qapi/util.h|  5 +
>  qapi/qapi-visit-core.c |  3 ++-
>  scripts/qapi/types.py  | 22 --
>  3 files changed, 15 insertions(+), 15 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v2 6/9] qapi: Generalize command policy checking

[Juan Quintela]

Markus Armbruster  wrote:
> The code to check command policy can see special feature flag
> 'deprecated' as command flag QCO_DEPRECATED.  I want to make feature
> flag 'unstable' visible there as well, so I can add policy for it.
>
> To let me make it visible, add member @special_features (a bitset of
> QapiSpecialFeature) to QmpCommand, and adjust the generator to pass it
> through qmp_register_command().  Then replace "QCO_DEPRECATED in
> @flags" by QAPI_DEPRECATED in @special_features", and drop
> QCO_DEPRECATED.
>
> Signed-off-by: Markus Armbruster 
> Reviewed-by: Philippe Mathieu-Daudé 
> Acked-by: John Snow 

Reviewed-by: Juan Quintela 

Re: [PATCH v2 5/9] qapi: Generalize struct member policy checking

[Juan Quintela]

Markus Armbruster  wrote:
> The generated visitor functions call visit_deprecated_accept() and
> visit_deprecated() when visiting a struct member with special feature
> flag 'deprecated'.  This makes the feature flag visible to the actual
> visitors.  I want to make feature flag 'unstable' visible there as
> well, so I can add policy for it.
>
> To let me make it visible, replace these functions by
> visit_policy_reject() and visit_policy_skip(), which take the member's
> special features as an argument.  Note that the new functions have the
> opposite sense, i.e. the return value flips.
>
> Signed-off-by: Markus Armbruster 

Reviewed-by: Juan Quintela 

Reversing accept/reject make things "interesting" for a review point of view.


> + * @special_features is the member's special features encoded as a
> + * bitset of QapiSpecialFeature.

Just to nitty pick, if you rename the variable to features, does the
sentece is clearer?

Later, Juan.

Re: [PATCH v2 4/9] qapi: Tools for sets of special feature flags in generated code

[Juan Quintela]

Markus Armbruster  wrote:
> New enum QapiSpecialFeature enumerates the special feature flags.
>
> New helper gen_special_features() returns code to represent a
> collection of special feature flags as a bitset.
>
> The next few commits will put them to use.
>
> Signed-off-by: Markus Armbruster 
> Reviewed-by: John Snow 

Reviewed-by: Juan Quintela 

libvirt-7.9.0 release candidate 2

[Jiri Denemark]

I have just tagged v7.9.0-rc2 in the repository and pushed signed
tarballs and source RPMs to https://libvirt.org/sources/

Please give the release candidate some testing and in case you find a
serious issue which should have a fix in the upcoming release, feel
free to reply to this thread to make sure the issue is more visible.

If you have not done so yet, please update NEWS.rst to document any
significant change you made since the last release.

Thanks,

Jirka

答复: [PATCH v3 0/4] Add qemu support setting qos via ovs on ovs interface

[张金生]

Hi Yalan,
 You are right about it. For other interface type, values in tc rules 
are calculated by multiply 8*1000 instead of 8*1024. I didn’t notice it. To 
make them uniform, I will fix it in next patch. Really thanks for your help.

Best Regards,
Jinsheng Zhang
发件人: Yalan Zhang [mailto:yalzh...@redhat.com]
发送时间: 2021年10月29日 16:52
收件人: Jinsheng Zhang (张金生)-云服务集团
抄送: libvir-list@redhat.com; Norman Shen(申嘉童)
主题: Re: [PATCH v3 0/4] Add qemu support setting qos via ovs on ovs interface

Hi Jinsheng,

I asked as I have mixed the "K" as 1000. Thank you for the explanation, I'm 
clear now.
And I found the "inbound.peak   : 200" was calculated to "ceil 1638Kbit", maybe 
1600Kbit is more reasonable? as I found for other interface type like nat, it 
was 1600Kbit from tc output. Please help to confirm, Thank you!

# virsh domiftune rhel vnet5

inbound.average: 100

inbound.peak   : 200

inbound.burst  : 256

...

#  tc -d class show  dev vnet5

class htb 1:1 parent 1:fffe prio 0 quantum 10240 rate 819200bit ceil 1638Kbit 
linklayer ethernet burst 256Kb/1 mpu 0b cburst 256Kb/1 mpu 0b level 0

class htb 1:fffe root rate 1638Kbit ceil 1638Kbit linklayer ethernet burst 
1499b/1 mpu 0b cburst 1499b/1 mpu 0b level 7


---
Best Regards,
Yalan Zhang
IRC: yalzhang


On Thu, Oct 28, 2021 at 4:20 PM Jinsheng Zhang (张金生)-云服务集团 
mailto:zhangj...@inspur.com>> wrote:
Hi Yalan,
It seems that there is no output error abount inbound settings from your 
statistics. 100KB is short for 100 kilobytes, and 1 byte is 8 bit, therefore 
100 kilobytes is 800 kilobit and is also 1024*800 bit which is 819200 bit or 
800 Kbit for short. Similarly, 200 KB is equal to 1600Kbit.

From your test results, inbound.average is set to 400 KB which is 400 * 1024 * 
8 bit(approximately 3.2*10^6 bits). outbound.average is set to 100 KB which is 
approximately 0.8*10^6 bits. Considering peek and burst is larger than average. 
The netperf test result is meaningful.

For the second bug mentioned, after create the ovs-net, tc rules are created. 
But when attach an interface to an instance, qos settings is not add to port 
neither in xml or tc . It is a bug, I think. I will think about fixing this.

---
Best Regards,
Jinsheng Zhang
发件人: Yalan Zhang [mailto:yalzh...@redhat.com]
发送时间: 2021年10月27日 18:35
收件人: Jinsheng Zhang (张金生)-云服务集团
抄送: libvir-list@redhat.com; Norman Shen(申嘉童)
主题: Re: [PATCH v3 0/4] Add qemu support setting qos via ovs on ovs interface


Hi Jinsheng,

Thank you for the explanation. From the statistics above, the tc outputs for 
outbound matches. But I'm confused about the inbound statistics:

# virsh domiftune rhel vnet5

inbound. approximately 3.2*10^6 bits: 100

inbound.peak   : 200

inbound.burst  : 256

...

#  tc -d class show  dev vnet5

class htb 1:1 parent 1:fffe prio 0 quantum 10240 rate 819200bit ceil 1638Kbit 
linklayer ethernet burst 256Kb/1 mpu 0b cburst 256Kb/1 mpu 0b level 0

class htb 1:fffe root rate 1638Kbit ceil 1638Kbit linklayer ethernet burst 
1499b/1 mpu 0b cburst 1499b/1 mpu 0b level 7



As the value in libvirt xml is KB, inbound.average: *100 KB* can not match with 
*"rate 819200bit"* in tc outputs, I supposed it should be 800Kbit. Please help 
to confirm.

And so does "ceil 1638Kbit" (may be it should be 1600Kbit as "inbound.peak   : 
200").



I have run netperf to test the actual rate, the result is pass. 2 vm connected 
to the same bridge, set one vm with Qos, see test results below:

# virsh domiftune rhel vnet0
inbound.average: 400
inbound.peak   : 500
inbound.burst  : 125
inbound.floor  : 0
outbound.average: 100
outbound.peak  : 200
outbound.burst : 256

Throughput for inbound:  3.92 * 10^6bits/sec

Throughput for outbound:  0.93 * 10^6bits/sec

These patches fixed the bug [1] which closed with deferred resolution. Thank 
you!
And this reminds me of another ovs Qos related bug [2], which was about network.
And I tried with the scenarios in [2], there are no changes(not fixed). Just 
for information. :-)

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1510237
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1826168


---
Best Regards,
Yalan Zhang
IRC: yalzhang


On Tue, Oct 26, 2021 at 3:23 PM Jinsheng Zhang (张金生)-云服务集团 
mailto:zhangj...@inspur.com>> wrote:
Hi Yalan,


1)   For inbound, we can use `ovs-vsctl list qos` and `ovs-vsctl list 
queue`  to check them from the openvswitch side. Values can be found in 
other_config.  Inbound is in kbyte when set qos with `virshdomiftune …`, 
well it is in bit in ovs, Therefore, when inbound.average is set to 100, the 
corresponding value will be  set to 819200 in ovs.

2)   For outbound, it is in kbyte in libvirt and ingress_policing_XX in ovs 
interface is in kbit.

3)   Ovs use tc to set qos, so we can see output from tc command.
This patch is to unify the qos control and query on ovs ports.
The conversion explanation is added in this patch: 

Re: [PATCH v3 0/4] Add qemu support setting qos via ovs on ovs interface

[Yalan Zhang]

Hi Jinsheng,

I asked as I have mixed the "K" as 1000. Thank you for the explanation, I'm
clear now.
And I found the "inbound.peak   : *200" *was calculated to "ceil
1638Kbit", maybe
1600Kbit is more reasonable? as I found for other interface type like nat,
it was 1600Kbit from tc output. Please help to confirm, Thank you!

# virsh domiftune rhel vnet5

inbound.average: 100

inbound.peak   : *200*

inbound.burst  : 256

...

#  tc -d class show  dev vnet5

class htb 1:1 parent 1:fffe prio 0 quantum 10240 rate 819200bit ceil
1638Kbit linklayer ethernet burst 256Kb/1 mpu 0b cburst 256Kb/1 mpu 0b
level 0

class htb 1:fffe root rate 1638Kbit ceil 1638Kbit linklayer ethernet burst
1499b/1 mpu 0b cburst 1499b/1 mpu 0b level 7


---
Best Regards,
Yalan Zhang
IRC: yalzhang


On Thu, Oct 28, 2021 at 4:20 PM Jinsheng Zhang (张金生)-云服务集团 <
zhangj...@inspur.com> wrote:

> Hi Yalan,
>
> It seems that there is no output error abount inbound settings from your
> statistics. 100KB is short for 100 kilobytes, and 1 byte is 8 bit,
> therefore 100 kilobytes is 800 kilobit and is also 1024*800 bit which is
> 819200 bit or 800 Kbit for short. Similarly, 200 KB is equal to 1600Kbit.
>
>
>
> From your test results, inbound.average is set to 400 KB which is 400 *
> 1024 * 8 bit(approximately 3.2*10^6 bits). outbound.average is set to 100
> KB which is approximately 0.8*10^6 bits. Considering peek and burst is
> larger than average. The netperf test result is meaningful.
>
>
>
> For the second bug mentioned, after create the ovs-net, tc rules are
> created. But when attach an interface to an instance, qos settings is not
> add to port neither in xml or tc . It is a bug, I think. I will think about
> fixing this.
>
>
>
> ---
>
> Best Regards,
>
> Jinsheng Zhang
>
> *发件人:* Yalan Zhang [mailto:yalzh...@redhat.com]
> *发送时间:* 2021年10月27日 18:35
> *收件人:* Jinsheng Zhang (张金生)-云服务集团
> *抄送:* libvir-list@redhat.com; Norman Shen(申嘉童)
> *主题:* Re: [PATCH v3 0/4] Add qemu support setting qos via ovs on ovs
> interface
>
>
>
>
>
> Hi Jinsheng,
>
>
>
> Thank you for the explanation. From the statistics above, the tc outputs
> for outbound matches. But I'm confused about the inbound statistics:
>
> # virsh domiftune rhel vnet5
>
> inbound. approximately 3.2*10^6 bits: *100*
>
> inbound.peak   : *200*
>
> inbound.burst  : 256
>
> ...
>
> #  tc -d class show  dev vnet5
>
> class htb 1:1 parent 1:fffe prio 0 quantum 10240 rate *819200bit* ceil
> *1638Kbit* linklayer ethernet burst 256Kb/1 mpu 0b cburst 256Kb/1 mpu 0b
> level 0
>
> class htb 1:fffe root rate 1638Kbit ceil 1638Kbit linklayer ethernet burst
> 1499b/1 mpu 0b cburst 1499b/1 mpu 0b level 7
>
>
>
> As the value in libvirt xml is KB, inbound.average: **100 KB** can not
> match with *"rate *819200bit"** in tc outputs*,* I supposed it should be 
> *800Kbit.
> * Please help to confirm.
>
> And so does "ceil *1638Kbit"* (may be it should be 1600Kbit as
> "inbound.peak   : 200").
>
>
>
> I have run netperf to test the actual rate, the result is pass. 2 vm
> connected to the same bridge, set one vm with Qos, see test results below:
>
> # virsh domiftune rhel vnet0
> inbound.average: 400
> inbound.peak   : 500
> inbound.burst  : 125
> inbound.floor  : 0
> outbound.average: 100
> outbound.peak  : 200
> outbound.burst : 256
>
> Throughput for inbound:  3.92 * 10^6bits/sec
>
> Throughput for outbound:  0.93 * 10^6bits/sec
>
>
>
> These patches fixed the bug [1] which closed with deferred resolution.
> Thank you!
>
> And this reminds me of another ovs Qos related bug [2], which was about
> network.
>
> And I tried with the scenarios in [2], there are no changes(not fixed).
> Just for information. :-)
>
>
>
> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1510237
>
> [2] https://bugzilla.redhat.com/show_bug.cgi?id=1826168
>
>
>
>
> ---
> Best Regards,
> Yalan Zhang
> IRC: yalzhang
>
>
>
>
>
> On Tue, Oct 26, 2021 at 3:23 PM Jinsheng Zhang (张金生)-云服务集团 <
> zhangj...@inspur.com> wrote:
>
> Hi Yalan,
>
>
>
> 1)   For inbound, we can use `ovs-vsctl list qos` and `ovs-vsctl list
> queue`  to check them from the openvswitch side. Values can be found in 
> other_config.
> Inbound is in kbyte when set qos with `virshdomiftune …`, well it is in
> bit in ovs, Therefore, when inbound.average is set to 100, the
> corresponding value will be  set to 819200 in ovs.
>
> 2)   For outbound, it is in kbyte in libvirt and ingress_policing_XX
> in ovs interface is in kbit.
>
> 3)   Ovs use tc to set qos, so we can see output from tc command.
>
> This patch is to unify the qos control and query on ovs ports.
>
> The conversion explanation is added in this patch:
> https://listman.redhat.com/archives/libvir-list/2021-August/msg00422.html
>
> And there are 6 following patches to fix some bugs. See
> https://listman.redhat.com/archives/libvir-list/2021-August/msg00423.html
>
>
>
> ---
>
> Best Regards,
>
> Jinsheng Zhang
>
>
>
> *发件人:* Yalan Zhang [mailto:yalzh...@redhat.com]
> *发送时间:* 2021年10月25日 

Re: [PATCH 1/3] docs: Mention QEMU version requirement for queue-size

[Peter Krempa]

On Fri, Oct 29, 2021 at 14:19:21 +0800, Han Han wrote:
> Signed-off-by: Han Han 
> ---
>  docs/formatdomain.rst | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index 58768f7e5e..dbda54dc93 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -3086,7 +3086,7 @@ paravirtualized driver is specified via the ``disk`` 
> element.
> -  The optional ``queues`` attribute specifies the number of virt queues 
> for
>virtio-blk. ( :since:`Since 3.9.0` )
> -  The optional ``queue_size`` attribute specifies the size of each virt
> -  queue for virtio-blk. ( :since:`Since 7.8.0` )
> +  queue for virtio-blk. ( :since:`Since 7.8.0, requires QEMU 2.12` )
> -  For virtio disks, `Virtio-specific options <#elementsVirtio>`__ can 
> also
>be set. ( :since:`Since 3.5.0` )
> -  The optional ``metadata_cache`` subelement controls aspects related to 
> the

Since qemu-2.11 is the oldest supported qemu and this is one release
after that, but introduced to libvirt just recently I don't think
anybody sane would use such a combination of versions. Thus it feels
pointless to add this to the docs.

[PATCH 3/4] qemu_agent: Drop destroy callback

[Michal Privoznik]

After previous cleanups this callback is unused. Remove it.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_agent.h   | 2 --
 src/qemu/qemu_process.c | 9 -
 2 files changed, 11 deletions(-)

diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
index 81b45b8e5d..b5954e76e7 100644
--- a/src/qemu/qemu_agent.h
+++ b/src/qemu/qemu_agent.h
@@ -28,8 +28,6 @@ typedef struct _qemuAgent qemuAgent;
 
 typedef struct _qemuAgentCallbacks qemuAgentCallbacks;
 struct _qemuAgentCallbacks {
-void (*destroy)(qemuAgent *mon,
-virDomainObj *vm);
 void (*eofNotify)(qemuAgent *mon,
   virDomainObj *vm);
 void (*errorNotify)(qemuAgent *mon,
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index b1fd72d269..ba9e9a5aa0 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -193,17 +193,8 @@ qemuProcessHandleAgentError(qemuAgent *agent G_GNUC_UNUSED,
 virObjectUnlock(vm);
 }
 
-static void qemuProcessHandleAgentDestroy(qemuAgent *agent,
-  virDomainObj *vm)
-{
-VIR_DEBUG("Received destroy agent=%p vm=%p", agent, vm);
-
-virObjectUnref(vm);
-}
-
 
 static qemuAgentCallbacks agentCallbacks = {
-.destroy = qemuProcessHandleAgentDestroy,
 .eofNotify = qemuProcessHandleAgentEOF,
 .errorNotify = qemuProcessHandleAgentError,
 };
-- 
2.32.0

[PATCH 4/4] qemuMonitorOpen: Rework domain object refcounting

[Michal Privoznik]

Similarly to one of previous commits, there's no need to
increment domain object refcounter before unlocking it. Any
number of lock and unlock calls over domain object has no effect
on the refcounter.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_monitor.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 908ee0d302..7559020001 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -769,10 +769,6 @@ qemuMonitorOpen(virDomainObj *vm,
 
 timeout += QEMU_DEFAULT_MONITOR_WAIT;
 
-/* Hold an extra reference because we can't allow 'vm' to be
- * deleted until the monitor gets its own reference. */
-virObjectRef(vm);
-
 if (config->type != VIR_DOMAIN_CHR_TYPE_UNIX) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("unable to handle monitor type: %s"),
@@ -798,7 +794,6 @@ qemuMonitorOpen(virDomainObj *vm,
  cleanup:
 if (!ret)
 VIR_FORCE_CLOSE(fd);
-virObjectUnref(vm);
 return ret;
 }
 
-- 
2.32.0

[PATCH 2/4] qemuAgentOpen: Rework domain object refcounting

[Michal Privoznik]

Currently, when opening an agent socket the qemuConnectAgent()
increments domain object refcounter and calls qemuAgentOpen()
where the domain object pointer is simply stored inside
_qemuAgent struct. If qemuAgentOpen() fails, then it clears @cb
member only to avoid qemuProcessHandleAgentDestroy() being called
(which decrements the domain object refcounter) and the domain
object refcounter is then decreased explicitly in
qemuConnectAgent().

The same result can be achieved with much cleaner code: increment
the refcounter inside qemuAgentOpen() and drop the dance around
@cb.

Also, the comment in qemuConnectAgent() about holding an extra
reference is not correct. The thread that called
qemuConnectAgent() already holds a reference to the domain
object. No matter how many time the object is locked and unlocked
the reference counter can't be decreased.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_agent.c   | 14 +-
 src/qemu/qemu_process.c |  7 ---
 2 files changed, 5 insertions(+), 16 deletions(-)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 166cfaf485..8bbaa127d4 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -171,9 +171,11 @@ qemuAgentEscapeNonPrintable(const char *text)
 static void qemuAgentDispose(void *obj)
 {
 qemuAgent *agent = obj;
+
 VIR_DEBUG("agent=%p", agent);
-if (agent->cb && agent->cb->destroy)
-(agent->cb->destroy)(agent, agent->vm);
+
+if (agent->vm)
+virObjectUnref(agent->vm);
 virCondDestroy(>notify);
 g_free(agent->buffer);
 g_main_context_unref(agent->context);
@@ -693,7 +695,7 @@ qemuAgentOpen(virDomainObj *vm,
 virObjectUnref(agent);
 return NULL;
 }
-agent->vm = vm;
+agent->vm = virObjectRef(vm);
 agent->cb = cb;
 agent->singleSync = singleSync;
 
@@ -729,12 +731,6 @@ qemuAgentOpen(virDomainObj *vm,
 return agent;
 
  cleanup:
-/* We don't want the 'destroy' callback invoked during
- * cleanup from construction failure, because that can
- * give a double-unref on virDomainObj *in the caller,
- * so kill the callbacks now.
- */
-agent->cb = NULL;
 qemuAgentClose(agent);
 return NULL;
 }
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index d2ea9b55fe..b1fd72d269 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -234,19 +234,12 @@ qemuConnectAgent(virQEMUDriver *driver, virDomainObj *vm)
 goto cleanup;
 }
 
-/* Hold an extra reference because we can't allow 'vm' to be
- * deleted while the agent is active */
-virObjectRef(vm);
-
 agent = qemuAgentOpen(vm,
   config->source,
   virEventThreadGetContext(priv->eventThread),
   ,
   virQEMUCapsGet(priv->qemuCaps, 
QEMU_CAPS_VSERPORT_CHANGE));
 
-if (agent == NULL)
-virObjectUnref(vm);
-
 if (!virDomainObjIsActive(vm)) {
 qemuAgentClose(agent);
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-- 
2.32.0

[PATCH 1/4] qemu_agent: Rework domain object locking when opening agent

[Michal Privoznik]

Just like qemuMonitorOpen(), hold the domain object locked
throughout the whole time of qemuConnectAgent() and unlock it
only for a brief time of actual connect() (because this is the
only part that has a potential of blocking).

The reason is that qemuAgentOpen() does access domain object
(well, its privateData) AND also at least one argument (@context)
depends on domain object. Accessing these without the lock is
potentially dangerous.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1845468#c12
Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_agent.c   | 3 +++
 src/qemu/qemu_process.c | 4 
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 5f421be6f6..166cfaf485 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -704,7 +704,10 @@ qemuAgentOpen(virDomainObj *vm,
 goto cleanup;
 }
 
+virObjectUnlock(vm);
 agent->fd = qemuAgentOpenUnix(config->data.nix.path);
+virObjectLock(vm);
+
 if (agent->fd == -1)
 goto cleanup;
 
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index d5f8a47ac2..d2ea9b55fe 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -238,16 +238,12 @@ qemuConnectAgent(virQEMUDriver *driver, virDomainObj *vm)
  * deleted while the agent is active */
 virObjectRef(vm);
 
-virObjectUnlock(vm);
-
 agent = qemuAgentOpen(vm,
   config->source,
   virEventThreadGetContext(priv->eventThread),
   ,
   virQEMUCapsGet(priv->qemuCaps, 
QEMU_CAPS_VSERPORT_CHANGE));
 
-virObjectLock(vm);
-
 if (agent == NULL)
 virObjectUnref(vm);
 
-- 
2.32.0

[PATCH 0/4] qemu_agent: Rework domain object handling in open

[Michal Privoznik]

The most important patch is the first one because it fixes a race
condition. The rest is just a cleanup I've noticed while looking at the
code.

Michal Prívozník (4):
  qemu_agent: Rework domain object locking when opening agent
  qemuAgentOpen: Rework domain object refcounting
  qemu_agent: Drop destroy callback
  qemuMonitorOpen: Rework domain object refcounting

 src/qemu/qemu_agent.c   | 17 -
 src/qemu/qemu_agent.h   |  2 --
 src/qemu/qemu_monitor.c |  5 -
 src/qemu/qemu_process.c | 20 
 4 files changed, 8 insertions(+), 36 deletions(-)

-- 
2.32.0

[PATCH] news: Support vhostuser in virsh attach-interface since v7.7.0

[Han Han]

Signed-off-by: Han Han 
---
 NEWS.rst | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/NEWS.rst b/NEWS.rst
index 5f1cf19940..694c506cad 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -162,6 +162,8 @@ v7.7.0 (2021-09-01)
   forbidden for older qemus which don't support the update API as the guest
   could still reboot and execute some instructions until it was terminated.
 
+  * virsh: Support vhostuser in attach-interface
+
 * **Bug fixes**
 
   * qemu: Open chardev logfile on behalf of QEMU
-- 
2.33.1

[PATCH 3/3] news: Add validation flags for creating of net, nwfilter-binding, net port

[Han Han]

Signed-off-by: Han Han 
---
 NEWS.rst | 12 
 1 file changed, 12 insertions(+)

diff --git a/NEWS.rst b/NEWS.rst
index cb1234e891..7815f1100f 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -96,6 +96,18 @@ v7.8.0 (2021-10-01)
 
 Implement the queue-size from QEMU to adjust the virtio disk queue size.
 
+* **Improvements**
+
+  * api: Add XML validation for creating of: networkport, nwfilter-binding,
+network
+
+* Add flag ``VIR_NETWORK_PORT_CREATE_VALIDATE`` to validate network port
+  input xml of network-port creating.
+* Add flag ``VIR_NETWORK_CREATE_VALIDATE`` to validate network input xml of
+  network creating.
+* Add flag ``VIR_NWFILTER_BINDING_CREATE_VALIDATE`` to validate
+  nwfilter-binding input xml of nwfilter-binding creating.
+
 v7.7.0 (2021-09-01)
 ===
 
-- 
2.33.1

[PATCH 2/3] news: Add queue_size option to virtio disk

[Han Han]

Signed-off-by: Han Han 
---
 NEWS.rst | 4 
 1 file changed, 4 insertions(+)

diff --git a/NEWS.rst b/NEWS.rst
index 5f1cf19940..cb1234e891 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -92,6 +92,10 @@ v7.8.0 (2021-10-01)
 active. This information can also be retrieved with the new virsh command
 ``nodedev-info``.
 
+  * qemu: Add attribute ``queue_size`` to set the queue size of virtio-blk
+
+Implement the queue-size from QEMU to adjust the virtio disk queue size.
+
 v7.7.0 (2021-09-01)
 ===
 
-- 
2.33.1

[PATCH 1/3] docs: Mention QEMU version requirement for queue-size

[Han Han]

Signed-off-by: Han Han 
---
 docs/formatdomain.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 58768f7e5e..dbda54dc93 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -3086,7 +3086,7 @@ paravirtualized driver is specified via the ``disk`` 
element.
-  The optional ``queues`` attribute specifies the number of virt queues for
   virtio-blk. ( :since:`Since 3.9.0` )
-  The optional ``queue_size`` attribute specifies the size of each virt
-  queue for virtio-blk. ( :since:`Since 7.8.0` )
+  queue for virtio-blk. ( :since:`Since 7.8.0, requires QEMU 2.12` )
-  For virtio disks, `Virtio-specific options <#elementsVirtio>`__ can also
   be set. ( :since:`Since 3.5.0` )
-  The optional ``metadata_cache`` subelement controls aspects related to 
the
-- 
2.33.1

[PATCH 0/3] Docs and news update for v7.8.0

[Han Han]

Han Han (3):
  docs: Mention QEMU version requirement for queue-size
  news: Add queue_size option to virtio disk
  news: Add validation flags for creating of net, nwfilter-binding, net
port

 NEWS.rst  | 16 
 docs/formatdomain.rst |  2 +-
 2 files changed, 17 insertions(+), 1 deletion(-)

-- 
2.33.1

Re: [PATCH v3 0/4] Add qemu support setting qos via ovs on ovs interface

[Yalan Zhang]

Hi Jinsheng,

Get it. Thank you for the explanation!

---
Best Regards,
Yalan Zhang
IRC: yalzhang


On Thu, Oct 28, 2021 at 4:20 PM Jinsheng Zhang (张金生)-云服务集团 <
zhangj...@inspur.com> wrote:

> Hi Yalan,
>
> It seems that there is no output error abount inbound settings from your
> statistics. 100KB is short for 100 kilobytes, and 1 byte is 8 bit,
> therefore 100 kilobytes is 800 kilobit and is also 1024*800 bit which is
> 819200 bit or 800 Kbit for short. Similarly, 200 KB is equal to 1600Kbit.
>
>
>
> From your test results, inbound.average is set to 400 KB which is 400 *
> 1024 * 8 bit(approximately 3.2*10^6 bits). outbound.average is set to 100
> KB which is approximately 0.8*10^6 bits. Considering peek and burst is
> larger than average. The netperf test result is meaningful.
>
>
>
> For the second bug mentioned, after create the ovs-net, tc rules are
> created. But when attach an interface to an instance, qos settings is not
> add to port neither in xml or tc . It is a bug, I think. I will think about
> fixing this.
>
>
>
> ---
>
> Best Regards,
>
> Jinsheng Zhang
>
> *发件人:* Yalan Zhang [mailto:yalzh...@redhat.com]
> *发送时间:* 2021年10月27日 18:35
> *收件人:* Jinsheng Zhang (张金生)-云服务集团
> *抄送:* libvir-list@redhat.com; Norman Shen(申嘉童)
> *主题:* Re: [PATCH v3 0/4] Add qemu support setting qos via ovs on ovs
> interface
>
>
>
>
>
> Hi Jinsheng,
>
>
>
> Thank you for the explanation. From the statistics above, the tc outputs
> for outbound matches. But I'm confused about the inbound statistics:
>
> # virsh domiftune rhel vnet5
>
> inbound. approximately 3.2*10^6 bits: *100*
>
> inbound.peak   : *200*
>
> inbound.burst  : 256
>
> ...
>
> #  tc -d class show  dev vnet5
>
> class htb 1:1 parent 1:fffe prio 0 quantum 10240 rate *819200bit* ceil
> *1638Kbit* linklayer ethernet burst 256Kb/1 mpu 0b cburst 256Kb/1 mpu 0b
> level 0
>
> class htb 1:fffe root rate 1638Kbit ceil 1638Kbit linklayer ethernet burst
> 1499b/1 mpu 0b cburst 1499b/1 mpu 0b level 7
>
>
>
> As the value in libvirt xml is KB, inbound.average: **100 KB** can not
> match with *"rate *819200bit"** in tc outputs*,* I supposed it should be 
> *800Kbit.
> * Please help to confirm.
>
> And so does "ceil *1638Kbit"* (may be it should be 1600Kbit as
> "inbound.peak   : 200").
>
>
>
> I have run netperf to test the actual rate, the result is pass. 2 vm
> connected to the same bridge, set one vm with Qos, see test results below:
>
> # virsh domiftune rhel vnet0
> inbound.average: 400
> inbound.peak   : 500
> inbound.burst  : 125
> inbound.floor  : 0
> outbound.average: 100
> outbound.peak  : 200
> outbound.burst : 256
>
> Throughput for inbound:  3.92 * 10^6bits/sec
>
> Throughput for outbound:  0.93 * 10^6bits/sec
>
>
>
> These patches fixed the bug [1] which closed with deferred resolution.
> Thank you!
>
> And this reminds me of another ovs Qos related bug [2], which was about
> network.
>
> And I tried with the scenarios in [2], there are no changes(not fixed).
> Just for information. :-)
>
>
>
> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1510237
>
> [2] https://bugzilla.redhat.com/show_bug.cgi?id=1826168
>
>
>
>
> ---
> Best Regards,
> Yalan Zhang
> IRC: yalzhang
>
>
>
>
>
> On Tue, Oct 26, 2021 at 3:23 PM Jinsheng Zhang (张金生)-云服务集团 <
> zhangj...@inspur.com> wrote:
>
> Hi Yalan,
>
>
>
> 1)   For inbound, we can use `ovs-vsctl list qos` and `ovs-vsctl list
> queue`  to check them from the openvswitch side. Values can be found in 
> other_config.
> Inbound is in kbyte when set qos with `virshdomiftune …`, well it is in
> bit in ovs, Therefore, when inbound.average is set to 100, the
> corresponding value will be  set to 819200 in ovs.
>
> 2)   For outbound, it is in kbyte in libvirt and ingress_policing_XX
> in ovs interface is in kbit.
>
> 3)   Ovs use tc to set qos, so we can see output from tc command.
>
> This patch is to unify the qos control and query on ovs ports.
>
> The conversion explanation is added in this patch:
> https://listman.redhat.com/archives/libvir-list/2021-August/msg00422.html
>
> And there are 6 following patches to fix some bugs. See
> https://listman.redhat.com/archives/libvir-list/2021-August/msg00423.html
>
>
>
> ---
>
> Best Regards,
>
> Jinsheng Zhang
>
>
>
> *发件人:* Yalan Zhang [mailto:yalzh...@redhat.com]
> *发送时间:* 2021年10月25日 17:54
> *收件人:* Michal Prívozník; Jinsheng Zhang (张金生)-云服务集团
> *抄送:* libvir-list@redhat.com; Norman Shen(申嘉童); zhangjl02
> *主题:* Re: [PATCH v3 0/4] Add qemu support setting qos via ovs on ovs
> interface
>
>
>
> Hi Jinsheng,
>
>
>
> I have tested the patch and have some questions, could you please help to
> confirm?
>
> 1) For inbound, how to check it from the openvswitch side? tc will still
> show the statistics, is that expected?
>
> 2) For outbound, the peak is ignored. I just can not understand the 
> "ingress_policing_burst:
> 2048", how can it come from the setting "outbound.burst : 256"?
>
> 3) Is the output from tc command expected?
>
>
>
> Test inbound:
>
> 1.