Libvirt-lxc: iptables not working in containers

[John Hurnett]

Hi,
I can't get iptables to work in libvirt-lxc containers. "iptables -L"
command shows empty chains. However I tested the same scenario with pure
lxc and iptables works as it should.
Has anyone experienced that? It seems like a bug, but maybe there is some
libvirt xml parameter I am missing?

BR

Re: [libvirt PATCH] ci: containers: Refresh the Dockerfiles

[Daniel P . Berrangé]

On Mon, Dec 14, 2020 at 08:30:56PM +0100, Erik Skultety wrote:
> Contains changes utilizing "nosync" and "eatmydata" for speedup as well
> as fixes for CentOS-8 repoid regression.
> ci-commit: b098ec6631a85880f818f2dd25c437d509e53680
> 
> Signed-off-by: Erik Skultety 
> ---
> Pipeline on my private fork: 
> https://gitlab.com/eskultety/libvirt/-/pipelines/229588144
> 
> If you want to ACK this patch, feel free to push this for me too as I won't
> respond much during my PTO and I'd like the pipeline to become green again.
> 
>  ci/containers/ci-centos-7.Dockerfile  | 13 ++--
>  ci/containers/ci-centos-8.Dockerfile  | 14 +---
>  ci/containers/ci-centos-stream.Dockerfile | 12 +--
>  .../ci-debian-10-cross-aarch64.Dockerfile | 30 ++---
>  .../ci-debian-10-cross-armv6l.Dockerfile  | 30 ++---
>  .../ci-debian-10-cross-armv7l.Dockerfile  | 30 ++---
>  .../ci-debian-10-cross-i686.Dockerfile| 30 ++---
>  .../ci-debian-10-cross-mips.Dockerfile| 30 ++---
>  .../ci-debian-10-cross-mips64el.Dockerfile| 30 ++---
>  .../ci-debian-10-cross-mipsel.Dockerfile  | 30 ++---
>  .../ci-debian-10-cross-ppc64le.Dockerfile | 30 ++---
>  .../ci-debian-10-cross-s390x.Dockerfile   | 30 ++---
>  ci/containers/ci-debian-10.Dockerfile | 17 +++---
>  .../ci-debian-sid-cross-aarch64.Dockerfile| 30 ++---
>  .../ci-debian-sid-cross-armv6l.Dockerfile | 30 ++---
>  .../ci-debian-sid-cross-armv7l.Dockerfile | 30 ++---
>  .../ci-debian-sid-cross-i686.Dockerfile   | 30 ++---
>  .../ci-debian-sid-cross-mips64el.Dockerfile   | 30 ++---
>  .../ci-debian-sid-cross-mipsel.Dockerfile | 30 ++---
>  .../ci-debian-sid-cross-ppc64le.Dockerfile| 30 ++---
>  .../ci-debian-sid-cross-s390x.Dockerfile  | 30 ++---
>  ci/containers/ci-debian-sid.Dockerfile| 17 +++---
>  ci/containers/ci-fedora-32.Dockerfile | 24 +++---
>  ci/containers/ci-fedora-33.Dockerfile | 24 +++---
>  ...ci-fedora-rawhide-cross-mingw32.Dockerfile | 33 +--
>  ...ci-fedora-rawhide-cross-mingw64.Dockerfile | 33 +--
>  ci/containers/ci-fedora-rawhide.Dockerfile| 26 ---
>  ci/containers/ci-opensuse-151.Dockerfile  |  6 
>  ci/containers/ci-ubuntu-1804.Dockerfile   | 15 ++---
>  ci/containers/ci-ubuntu-2004.Dockerfile   | 15 ++---
>  30 files changed, 495 insertions(+), 264 deletions(-)

Reviewed-by: Daniel P. Berrangé 

and i'll push as Erik requested

 

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] lxc: don't try to reserve macvtap name for LXC domains

[Daniel P . Berrangé]

On Mon, Dec 14, 2020 at 04:30:48PM -0500, Laine Stump wrote:
> Commit 729a06c41 added code to the LXC driver (patterned after similar
> code in the QEMU driver) that called
> virNetDevMacVlanReserveName(net->ifname) for all type='direct'
> interfaces during a libvirtd restart, to prevent other domains from
> attempting to use a macvtap device name that was already in use by a
> domain.
> 
> But, unlike a QEMU domain, when an LXC domain creates a macvtap
> device, that device is almost immediately moved into the namespace of
> the container (and it's then renamed, but that part isn't
> important). Because of this, the LXC driver doesn't keep track (in
> net->ifname) of the name used to create the device (as the QEMU driver
> does).
> 
> The result of this is that if libvirtd is restarted while there is an
> active LXC domain that has , libvirtd will
> segfault (since virNetDevMacVLanReserveName() doesn't check for a NULL
> pointer).
> 
> The fix is to just not call that function in the case of the LXC
> driver, since it is pointless anyway.
> 
> Signed-off-by: Laine Stump 
> ---
>  src/lxc/lxc_process.c | 7 ---
>  1 file changed, 7 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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: Libvirt-lxc: iptables not working in containers

[Michal Privoznik]

On 12/14/20 12:05 AM, John Hurnett wrote:

Hi,
I can't get iptables to work in libvirt-lxc containers. "iptables -L"
command shows empty chains. However I tested the same scenario with pure
lxc and iptables works as it should.
Has anyone experienced that? It seems like a bug, but maybe there is some
libvirt xml parameter I am missing?

BR



Libvirt will create a private network NS if:

1) you have an  defined for your container, or
2)  exists under 

This is documented here:

https://libvirt.org/drvlxc.html#securenetworking

And private network NS also means separate firewall and its tables.

Michal

[libvirt PATCH 0/2] Schema fixes for virsh [hypervisor-]cpu-compare

[Tim Wiederhake]

See individual commit messages for more details.

Tim Wiederhake (2):
  schemas: Deduplicate cpuTopology in cputypes.rng
  schema: Allow counter element in host cpu definition

 docs/schemas/cputypes.rng | 15 +--
 1 file changed, 9 insertions(+), 6 deletions(-)

-- 
2.26.2

[libvirt PATCH 2/2] schema: Allow counter element in host cpu definition

[Tim Wiederhake]

If the capabilities include a counter element, e.g.
  
the XML could not be validated:
  $ virsh capabilities > cap.xml
  $ virsh [hypervisor-]cpu-compare cap.xml --validate
  error: Failed to compare hypervisor CPU with cap.txt
  error: XML document failed to validate against schema: Unable to validate doc 
against /usr/share/libvirt/schemas/cpu.rng
  Did not expect element counter there

Signed-off-by: Tim Wiederhake 
---
 docs/schemas/cputypes.rng | 13 +
 1 file changed, 13 insertions(+)

diff --git a/docs/schemas/cputypes.rng b/docs/schemas/cputypes.rng
index c4e7621659..f66bc62ba7 100644
--- a/docs/schemas/cputypes.rng
+++ b/docs/schemas/cputypes.rng
@@ -336,6 +336,19 @@
 
   
 
+
+  
+
+  
+
+
+  
+
+
+  
+
+  
+
 
   
 
-- 
2.26.2

[libvirt PATCH 1/2] schemas: Deduplicate cpuTopology in cputypes.rng

[Tim Wiederhake]

The duplicate had the "dies" attribute missing, causing
  $ virsh capabilities > cap.xml
  $ virsh [hypervisor-]cpu-compare cap.xml --validate
to fail with
  error: Failed to compare hypervisor CPU with cap.xml
  error: XML document failed to validate against schema: Unable to validate doc 
against /usr/share/libvirt/schemas/cpu.rng
  Invalid attribute dies for element topology

Signed-off-by: Tim Wiederhake 
---
 docs/schemas/cputypes.rng | 12 +---
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/docs/schemas/cputypes.rng b/docs/schemas/cputypes.rng
index aea6ff0267..c4e7621659 100644
--- a/docs/schemas/cputypes.rng
+++ b/docs/schemas/cputypes.rng
@@ -337,17 +337,7 @@
   
 
 
-  
-
-  
-
-
-  
-
-
-  
-
-  
+  
 
 
   
-- 
2.26.2

[libvirt PATCH 06/29] cpu-gather: Allow overwriting cpuid binary location

[Tim Wiederhake]

This is useful if cpuid was compiled from source in a non-standard
location.

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-gather.py | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index 97655399c8..75cf290a28 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -18,10 +18,11 @@ def gather_name(args):
  "Use '--model' to set a model name.")
 
 
-def gather_cpuid_leaves():
+def gather_cpuid_leaves(args):
+cpuid = args.path_to_cpuid or "cpuid"
 try:
 output = subprocess.check_output(
-["cpuid", "-1r"],
+[cpuid, "-1r"],
 universal_newlines=True)
 except FileNotFoundError as e:
 exit("Error: '{}' not found.\n'cpuid' can be usually found in a "
@@ -43,13 +44,18 @@ def main():
 "--name",
 help="CPU model name. "
 "If unset, model name is read from '/proc/cpuinfo'.")
+parser.add_argument(
+"--path-to-cpuid",
+metavar="PATH",
+help="Path to 'cpuid' utility. "
+"If unset, the first executable 'cpuid' in $PATH is used.")
 
 args = parser.parse_args()
 
 name = gather_name(args)
 print("model name\t: {}".format(name))
 
-leaves = gather_cpuid_leaves()
+leaves = gather_cpuid_leaves(args)
 print("CPU:")
 for leave in leaves:
 print("   {}".format(leave))
-- 
2.26.2

[libvirt PATCH 05/29] cpu-gather: Move cpuid call to new script

[Tim Wiederhake]

Turn the comment on how to aquire cpuid into a runtime error message.
Use "http" instead of "https" in the URL, as the latter is broken.

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-gather.py | 25 +
 tests/cputestdata/cpu-gather.sh |  7 ---
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index 4e8c72e4f4..97655399c8 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -18,6 +18,25 @@ def gather_name(args):
  "Use '--model' to set a model name.")
 
 
+def gather_cpuid_leaves():
+try:
+output = subprocess.check_output(
+["cpuid", "-1r"],
+universal_newlines=True)
+except FileNotFoundError as e:
+exit("Error: '{}' not found.\n'cpuid' can be usually found in a "
+ "package named identically. If your distro does not provide such "
+ "package, you can find the sources or binary packages at "
+ "'http://www.etallen.com/cpuid.html'.".format(e.filename))
+
+for line in output.split("\n"):
+if not line:
+continue
+if line == "CPU:":
+continue
+yield line.strip()
+
+
 def main():
 parser = argparse.ArgumentParser(description="Gather cpu test data")
 parser.add_argument(
@@ -30,6 +49,12 @@ def main():
 name = gather_name(args)
 print("model name\t: {}".format(name))
 
+leaves = gather_cpuid_leaves()
+print("CPU:")
+for leave in leaves:
+print("   {}".format(leave))
+print()
+
 print(end="", flush=True)
 os.environ["CPU_GATHER_PY"] = "true"
 subprocess.check_call("./cpu-gather.sh")
diff --git a/tests/cputestdata/cpu-gather.sh b/tests/cputestdata/cpu-gather.sh
index b671f223a5..f84215e777 100755
--- a/tests/cputestdata/cpu-gather.sh
+++ b/tests/cputestdata/cpu-gather.sh
@@ -1,17 +1,10 @@
 #!/bin/bash
-#
-# The cpuid tool can be usually found in a package called "cpuid". If your
-# distro does not provide such package, you can find the sources or binary
-# packages at https://www.etallen.com/cpuid.html
 
 if [ -z "${CPU_GATHER_PY}" ]; then
 echo >&2 "Do not call this script directly. Use 'cpu-gather.py' instead."
 exit 1
 fi
 
-cpuid -1r
-echo
-
 python3 <

[libvirt PATCH 15/29] cpu-gather: Separate data input and output

[Tim Wiederhake]

This is a preparatory step to replace the output format with
something more readable.

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-gather.py | 50 +
 1 file changed, 32 insertions(+), 18 deletions(-)

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index 9c609aa6e9..005d1019b6 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -183,6 +183,36 @@ def gather_model(args):
 ])
 
 
+def gather(args):
+result = dict()
+result["name"] = gather_name(args)
+result["leaves"] = list(gather_cpuid_leaves(args))
+result["via"], result["msr"] = gather_msr()
+result["model"] = list(gather_model(args))
+return result
+
+
+def output_to_text(data):
+output = list()
+
+output.append("model name\t: {}".format(data["name"]))
+
+output.append("CPU:")
+for leave in data["leaves"]:
+output.append("   {}".format(leave))
+output.append("")
+
+if data["via"] is not None:
+output.append("MSR{}:".format(data["via"]))
+for key, value in sorted(data["msr"].items()):
+output.append("   0x{:x}: 0x{:016x}\n".format(int(key), value))
+
+for o in data["model"]:
+output.append(json.dumps(o))
+
+return "\n".join(output)
+
+
 def main():
 parser = argparse.ArgumentParser(description="Gather cpu test data")
 parser.add_argument(
@@ -213,24 +243,8 @@ def main():
 if os.path.isfile(f):
 args.path_to_qemu = f
 
-name = gather_name(args)
-print("model name\t: {}".format(name))
-
-leaves = gather_cpuid_leaves(args)
-print("CPU:")
-for leave in leaves:
-print("   {}".format(leave))
-print()
-
-via, msr = gather_msr()
-if via is not None:
-print("MSR{}:".format(via))
-for key, value in sorted(msr.items()):
-print("   0x{:x}: 0x{:016x}\n".format(int(key), value))
-
-model = gather_model(args)
-for o in model:
-print(json.dumps(o))
+data = gather(args)
+print(output_to_text(data))
 
 
 if __name__ == "__main__":
-- 
2.26.2

[libvirt PATCH 02/29] cpu-gather: Create python wrapper for shell script

[Tim Wiederhake]

This changes the invocation from
  ./cpu-gather.sh | ./cpu-parse.sh
to
  ./cpu-gather.py | ./cpu-parse.sh

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-gather.py | 13 +
 tests/cputestdata/cpu-gather.sh |  5 +
 2 files changed, 18 insertions(+)
 create mode 100755 tests/cputestdata/cpu-gather.py

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
new file mode 100755
index 00..f7030eb48b
--- /dev/null
+++ b/tests/cputestdata/cpu-gather.py
@@ -0,0 +1,13 @@
+#!/usr/bin/env python3
+
+import os
+import subprocess
+
+
+def main():
+os.environ["CPU_GATHER_PY"] = "true"
+subprocess.check_call("./cpu-gather.sh")
+
+
+if __name__ == "__main__":
+main()
diff --git a/tests/cputestdata/cpu-gather.sh b/tests/cputestdata/cpu-gather.sh
index 7574324d1c..cd65d74da5 100755
--- a/tests/cputestdata/cpu-gather.sh
+++ b/tests/cputestdata/cpu-gather.sh
@@ -4,6 +4,11 @@
 # distro does not provide such package, you can find the sources or binary
 # packages at https://www.etallen.com/cpuid.html
 
+if [ -z "${CPU_GATHER_PY}" ]; then
+echo >&2 "Do not call this script directly. Use 'cpu-gather.py' instead."
+exit 1
+fi
+
 grep 'model name' /proc/cpuinfo | head -n1
 
 cpuid -1r
-- 
2.26.2

[libvirt PATCH 11/29] cpu-gather: Move simple model extraction to new script

[Tim Wiederhake]

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-gather.py | 60 +
 tests/cputestdata/cpu-gather.sh | 13 ---
 2 files changed, 60 insertions(+), 13 deletions(-)

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index 0c8d8e93d0..5c03d226b6 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -115,6 +115,63 @@ def gather_static_model(args):
 return None
 
 
+def gather_full_model(args, static_model):
+if static_model:
+return []
+else:
+return call_qemu(args.path_to_qemu, [
+{
+"execute": "qom-get",
+"arguments":
+{
+"path": "/machine/unattached/device[0]",
+"property": "feature-words"
+},
+"id": "feature-words"
+},
+{
+"execute": "qom-get",
+"arguments":
+{
+"path": "/machine/unattached/device[0]",
+"property": "family"
+},
+"id": "family"
+},
+{
+"execute": "qom-get",
+"arguments":
+{
+"path": "/machine/unattached/device[0]",
+"property": "model"
+},
+"id": "model"
+},
+{
+"execute": "qom-get",
+"arguments":
+{
+"path": "/machine/unattached/device[0]",
+"property": "stepping"
+},
+"id": "stepping"
+},
+{
+"execute": "qom-get",
+"arguments":
+{
+"path": "/machine/unattached/device[0]",
+"property": "model-id"
+},
+"id": "model-id"
+},
+{
+"execute": "query-cpu-definitions",
+"id": "definitions"
+}
+])
+
+
 def main():
 parser = argparse.ArgumentParser(description="Gather cpu test data")
 parser.add_argument(
@@ -161,6 +218,9 @@ def main():
 print("   0x{:x}: 0x{:016x}\n".format(int(key), value))
 
 static_model = gather_static_model(args)
+model = gather_full_model(args, static_model)
+for o in model:
+print(json.dumps(o))
 
 print(end="", flush=True)
 os.environ["CPU_GATHER_PY"] = "true"
diff --git a/tests/cputestdata/cpu-gather.sh b/tests/cputestdata/cpu-gather.sh
index 726f013908..05faf14a96 100755
--- a/tests/cputestdata/cpu-gather.sh
+++ b/tests/cputestdata/cpu-gather.sh
@@ -5,13 +5,6 @@ if [ -z "${CPU_GATHER_PY}" ]; then
 exit 1
 fi
 
-qom_get()
-{
-path='/machine/unattached/device[0]'
-echo '{"execute":"qom-get","arguments":{"path":"'$path'",' \
- '"property":"'$1'"},"id":"'$1'"}'
-}
-
 model_expansion()
 {
 mode=$1
@@ -26,12 +19,6 @@ $qemu -machine accel=kvm -cpu host -nodefaults -nographic 
-qmp stdio <

[libvirt PATCH 19/29] cpu-parse: Move file name generation to new script

[Tim Wiederhake]

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-gather.py | 18 ++
 tests/cputestdata/cpu-parse.sh  | 13 -
 2 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index 0b1019456c..1a15cc1ff0 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -4,6 +4,7 @@ import argparse
 import fcntl
 import json
 import os
+import re
 import struct
 import subprocess
 import sys
@@ -192,11 +193,28 @@ def gather(args):
 return result
 
 
+def parse_filename(data):
+filename = data["name"].strip()
+filename = re.sub("[ -]+ +", " ", filename)
+filename = re.sub("\\(([Rr]|[Tt][Mm])\\)", "", filename)
+filename = re.sub(".*(Intel|AMD) ", "", filename)
+filename = re.sub(" (Duo|Quad|II X[0-9]+)", " ", filename)
+filename = re.sub(" (CPU|Processor)", "", filename)
+filename = re.sub(" @.*", "", filename)
+filename = re.sub(" APU .*", "", filename)
+filename = re.sub(" SE$", "", filename)
+filename = re.sub(" ", "-", filename)
+return "x86_64-cpuid-{}".format(filename)
+
+
 def parse(args):
 data = json.load(sys.stdin)
 
+filename = parse_filename(data)
+
 os.environ["CPU_GATHER_PY"] = "true"
 os.environ["model"] = data["name"]
+os.environ["fname"] = filename
 output = subprocess.check_output(
 "./cpu-parse.sh",
 input=output_to_text(data),
diff --git a/tests/cputestdata/cpu-parse.sh b/tests/cputestdata/cpu-parse.sh
index 5b8e57e427..2a41897538 100755
--- a/tests/cputestdata/cpu-parse.sh
+++ b/tests/cputestdata/cpu-parse.sh
@@ -7,19 +7,6 @@ fi
 
 data=`cat`
 
-fname=`sed -e 's/^ *//;
-   s/ *$//;
-   s/[ -]\+ \+/ /g;
-   s/(\([Rr]\|[Tt][Mm]\))//g;
-   s/.*\(Intel\|AMD\) //;
-   s/ \(Duo\|Quad\|II X[0-9]\+\) / /;
-   s/ \(CPU\|Processor\)\>//;
-   s/ @.*//;
-   s/ APU .*//;
-   s/ SE$//;
-   s/ /-/g' <<<"$model"`
-fname="x86_64-cpuid-$fname"
-
 xml()
 {
 hex='\(0x[0-9a-f]\+\)'
-- 
2.26.2

[libvirt PATCH 21/29] cpu-parse: Move json output to new script

[Tim Wiederhake]

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-gather.py   | 30 ++
 tests/cputestdata/cpu-parse.sh| 16 
 tests/cputestdata/cpu-reformat.py |  9 -
 3 files changed, 30 insertions(+), 25 deletions(-)
 delete mode 100755 tests/cputestdata/cpu-reformat.py

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index bc5c7dbb15..2c3f39ab35 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -244,13 +244,43 @@ def output_xml(data, filename):
 f.write("\n")
 
 
+def output_json(data, filename):
+replies = list()
+for reply in data["model"]:
+if "QMP" in reply:
+continue
+if "timestamp" in reply:
+continue
+if "return" in reply and not reply["return"]:
+continue
+replies.append(reply)
+
+if not replies:
+return
+
+if "model-expansion" not in [reply.get("id") for reply in replies]:
+exit(
+"Error: Missing query-cpu-model-expansion reply in "
+"{}".format(filename))
+
+print(filename)
+with open(filename, "wt") as f:
+for reply in replies:
+if reply is not replies[0]:
+f.write("\n")
+json.dump(reply, f, indent=2)
+f.write("\n")
+
+
 def parse(args):
 data = json.load(sys.stdin)
 
 filename = parse_filename(data)
 filename_xml = "{}.xml".format(filename)
+filename_json = "{}.json".format(filename)
 
 output_xml(data, filename_xml)
+output_json(data, filename_json)
 
 os.environ["CPU_GATHER_PY"] = "true"
 os.environ["model"] = data["name"]
diff --git a/tests/cputestdata/cpu-parse.sh b/tests/cputestdata/cpu-parse.sh
index 84d37d0df4..aeb6e4e07f 100755
--- a/tests/cputestdata/cpu-parse.sh
+++ b/tests/cputestdata/cpu-parse.sh
@@ -7,23 +7,7 @@ fi
 
 data=`cat`
 
-json()
-{
-first=true
-sed -ne '/{"QMP".*/d;
- /{"return": {}}/d;
- /{"timestamp":.*/d;
- /^{/p' <<<"$data" | \
-while read; do
-$first || echo
-first=false
-$(dirname $0)/cpu-reformat.py <<<"$REPLY"
-done
-}
-
-json <<<"$data" >$fname.json
 if [[ -s $fname.json ]]; then
-echo $fname.json
 if ! grep -q model-expansion $fname.json; then
 echo "Missing query-cpu-model-expansion reply in $name.json" >&2
 exit 1
diff --git a/tests/cputestdata/cpu-reformat.py 
b/tests/cputestdata/cpu-reformat.py
deleted file mode 100755
index fcc6b8ab41..00
--- a/tests/cputestdata/cpu-reformat.py
+++ /dev/null
@@ -1,9 +0,0 @@
-#!/usr/bin/env python3
-
-import sys
-import json
-
-dec = json.JSONDecoder()
-data, pos = dec.raw_decode(sys.stdin.read())
-json.dump(data, sys.stdout, indent=2, separators=(',', ': '))
-print("")
-- 
2.26.2

[libvirt PATCH 22/29] cpu-parse: Move call to cpu-cpuid.py to new script

[Tim Wiederhake]

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-gather.py | 10 ++
 tests/cputestdata/cpu-parse.sh  | 10 --
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index 2c3f39ab35..373f179c8d 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -282,6 +282,16 @@ def parse(args):
 output_xml(data, filename_xml)
 output_json(data, filename_json)
 
+if not os.path.isfile(filename_json):
+return
+if os.path.getsize(filename_json) == 0:
+return
+
+output = subprocess.check_output(
+["./cpu-cpuid.py", "diff", filename_json],
+universal_newlines=True)
+print(output)
+
 os.environ["CPU_GATHER_PY"] = "true"
 os.environ["model"] = data["name"]
 os.environ["fname"] = filename
diff --git a/tests/cputestdata/cpu-parse.sh b/tests/cputestdata/cpu-parse.sh
index aeb6e4e07f..eea15ded2e 100755
--- a/tests/cputestdata/cpu-parse.sh
+++ b/tests/cputestdata/cpu-parse.sh
@@ -6,13 +6,3 @@ if [ -z "${CPU_GATHER_PY}" ]; then
 fi
 
 data=`cat`
-
-if [[ -s $fname.json ]]; then
-if ! grep -q model-expansion $fname.json; then
-echo "Missing query-cpu-model-expansion reply in $name.json" >&2
-exit 1
-fi
-$(dirname $0)/cpu-cpuid.py diff $fname.json
-else
-rm $fname.json
-fi
-- 
2.26.2

[libvirt PATCH 12/29] cpu-gather: Move full model extraction to new script

[Tim Wiederhake]

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-gather.py | 16 +++-
 tests/cputestdata/cpu-gather.sh | 20 
 2 files changed, 15 insertions(+), 21 deletions(-)

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index 5c03d226b6..ea1d42f1ec 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -117,7 +117,21 @@ def gather_static_model(args):
 
 def gather_full_model(args, static_model):
 if static_model:
-return []
+return call_qemu(args.path_to_qemu, [
+{
+"execute": "query-cpu-model-expansion",
+"arguments":
+{
+"type": "full",
+"model": static_model
+},
+"id": "model-expansion"
+},
+{
+"execute": "query-cpu-definitions",
+"id": "definitions"
+}
+])
 else:
 return call_qemu(args.path_to_qemu, [
 {
diff --git a/tests/cputestdata/cpu-gather.sh b/tests/cputestdata/cpu-gather.sh
index 05faf14a96..5cef26c8e6 100755
--- a/tests/cputestdata/cpu-gather.sh
+++ b/tests/cputestdata/cpu-gather.sh
@@ -4,23 +4,3 @@ if [ -z "${CPU_GATHER_PY}" ]; then
 echo >&2 "Do not call this script directly. Use 'cpu-gather.py' instead."
 exit 1
 fi
-
-model_expansion()
-{
-mode=$1
-model=$2
-
-echo '{"execute":"query-cpu-model-expansion","arguments":' \
- '{"type":"'"$mode"'","model":'"$model"'},"id":"model-expansion"}'
-}
-
-$qemu -machine accel=kvm -cpu host -nodefaults -nographic -qmp stdio <

[libvirt PATCH 27/29] cpu-gather: Allow gathering and parsing data in one step.

[Tim Wiederhake]

Make
  ./cpu-gather.py --gather --parse
an alias of
  ./cpu-gather.py [--gather] | ./cpu-gather.py --parse

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-gather.py | 26 ++
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index fe660c486e..46997c8a48 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -282,9 +282,7 @@ def output_json(data, filename):
 f.write("\n")
 
 
-def parse(args):
-data = json.load(sys.stdin)
-
+def parse(data):
 filename = parse_filename(data)
 filename_xml = "{}.xml".format(filename)
 filename_json = "{}.json".format(filename)
@@ -320,16 +318,16 @@ def main():
 help="Path to qemu. "
 "If unset, will try '/usr/bin/qemu-system-x86_64', "
 "'/usr/bin/qemu-kvm', and '/usr/libexec/qemu-kvm'.")
-
-mode = parser.add_mutually_exclusive_group()
-mode.add_argument(
+parser.add_argument(
 "--gather",
 action="store_true",
-help="Acquire data on target system. This is the default.")
-mode.add_argument(
+help="Acquire data on target system. This is the default. "
+"If '--parse' is not set, outputs data on stdout.")
+parser.add_argument(
 "--parse",
 action="store_true",
-help="Parse data for libvirt use.")
+help="Parse data for libvirt use. "
+"If '--gather' is not set, expects input on stdin.")
 
 args = parser.parse_args()
 
@@ -348,9 +346,13 @@ def main():
 
 if args.gather:
 data = gather(args)
-json.dump(data, sys.stdout, indent=2)
-else:
-parse(args)
+if not args.parse:
+json.dump(data, sys.stdout, indent=2)
+
+if args.parse:
+if not args.gather:
+data = json.load(sys.stdin)
+parse(data)
 
 
 if __name__ == "__main__":
-- 
2.26.2

[libvirt PATCH 23/29] cpu-parse: Delete old script

[Tim Wiederhake]

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-gather.py | 31 ---
 tests/cputestdata/cpu-parse.sh  |  8 
 2 files changed, 39 deletions(-)
 delete mode 100755 tests/cputestdata/cpu-parse.sh

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index 373f179c8d..b6acae9eb7 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -292,37 +292,6 @@ def parse(args):
 universal_newlines=True)
 print(output)
 
-os.environ["CPU_GATHER_PY"] = "true"
-os.environ["model"] = data["name"]
-os.environ["fname"] = filename
-output = subprocess.check_output(
-"./cpu-parse.sh",
-input=output_to_text(data),
-stderr=subprocess.STDOUT,
-universal_newlines=True)
-print(output)
-
-
-def output_to_text(data):
-output = list()
-
-output.append("model name\t: {}".format(data["name"]))
-
-output.append("CPU:")
-for leave in data["leaves"]:
-output.append("   {}".format(leave))
-output.append("")
-
-if data["via"] is not None:
-output.append("MSR{}:".format(data["via"]))
-for key, value in sorted(data["msr"].items()):
-output.append("   0x{:x}: 0x{:016x}\n".format(int(key), value))
-
-for o in data["model"]:
-output.append(json.dumps(o))
-
-return "\n".join(output)
-
 
 def main():
 parser = argparse.ArgumentParser(description="Gather cpu test data")
diff --git a/tests/cputestdata/cpu-parse.sh b/tests/cputestdata/cpu-parse.sh
deleted file mode 100755
index eea15ded2e..00
--- a/tests/cputestdata/cpu-parse.sh
+++ /dev/null
@@ -1,8 +0,0 @@
-#!/bin/bash
-
-if [ -z "${CPU_GATHER_PY}" ]; then
-echo >&2 "Do not call this script directly. Use 'cpu-gather.py' instead."
-exit 1
-fi
-
-data=`cat`
-- 
2.26.2

[libvirt PATCH 13/29] cpu-gather: Merge model gathering logic

[Tim Wiederhake]

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-gather.py | 13 -
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index ea1d42f1ec..c639c433d2 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -96,7 +96,7 @@ def call_qemu(qemu, qmp_cmds):
 yield json.loads(line)
 
 
-def gather_static_model(args):
+def gather_model(args):
 output = call_qemu(args.path_to_qemu, [
 {
 "execute": "query-cpu-model-expansion",
@@ -108,14 +108,11 @@ def gather_static_model(args):
 "id": "model-expansion"
 }])
 
+static_model = None
 for o in output:
 if o.get("id") == "model-expansion":
-return o["return"]["model"]
+static_model = o["return"]["model"]
 
-return None
-
-
-def gather_full_model(args, static_model):
 if static_model:
 return call_qemu(args.path_to_qemu, [
 {
@@ -231,15 +228,13 @@ def main():
 for key, value in sorted(msr.items()):
 print("   0x{:x}: 0x{:016x}\n".format(int(key), value))
 
-static_model = gather_static_model(args)
-model = gather_full_model(args, static_model)
+model = gather_model(args)
 for o in model:
 print(json.dumps(o))
 
 print(end="", flush=True)
 os.environ["CPU_GATHER_PY"] = "true"
 os.environ["qemu"] = args.path_to_qemu
-os.environ["model"] = json.dumps(static_model) if static_model else ""
 subprocess.check_call("./cpu-gather.sh")
 
 
-- 
2.26.2

[libvirt PATCH 01/29] cpu-cpuid: Shorten overly long line

[Tim Wiederhake]

flake8 was complaining.

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-cpuid.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tests/cputestdata/cpu-cpuid.py b/tests/cputestdata/cpu-cpuid.py
index 37d00e3c97..dac43debb6 100755
--- a/tests/cputestdata/cpu-cpuid.py
+++ b/tests/cputestdata/cpu-cpuid.py
@@ -158,8 +158,9 @@ def parseMap():
 cpuMap = {}
 for feature in data["cpus"]["feature"]:
 for fType in ["cpuid", "msr"]:
-if fType in feature:
-cpuMap[feature["@name"]] = parseMapFeature(fType, 
feature[fType])
+if fType not in feature:
+continue
+cpuMap[feature["@name"]] = parseMapFeature(fType, feature[fType])
 
 return cpuMap
 
-- 
2.26.2

[libvirt PATCH 00/29] Refactor scripts in tests/cputestdata

[Tim Wiederhake]

This series refactors the various scripts found in tests/cputestdata and
adds support for CORE_CAPABILITY MSR, as found on e.g. SnowRidge.

Acquiring test data on a new system is a two step process. "cpu-gather.sh"
gathers information on the target machine and has as few dependencies as
possible. "cpu-parse.sh" processes this information but requires access to
a libvirt source tree and has more dependencies, e.g. "xmltodict".

This series merges three of the four involved scripts (cpu-gather.sh,
cpu-parse.sh and cpu-reformat.py) into a single python3 script. python3
already was a dependency for cpu-gather.sh and care has been taken to not
depend on modules that are not installed by default [1]. Merging the fourth
script, cpu-cpuid.py, will come in a seperate series.

Patches 1 to 14 transform cpu-gather into a python script, preserving the
format of the output (except for consistent "\n" line endings; previously
the tool would output a mix of "\n" and "\r\n").

Patches 15 to 23 merge cpu-parse into the script. In this process, the
format of the intermediary data is changed to json.

Patches 24 to 29 add support for "all in one" operation and extracting
IA32_CORE_CAPABILITY_MSR, which can be found on e.g. SnowRidge CPUs.

Old usage:
  ./cpu-gather.sh | ./cpu-parse.sh
New:
  ./cpu-gather.py [--gather] | ./cpu-gather.py --parse
Alternative on single machine:
  ./cpu-gather.py --gather --parse

[1] https://docs.python.org/3/py-modindex.html

Tim Wiederhake (29):
  cpu-cpuid: Shorten overly long line
  cpu-gather: Create python wrapper for shell script
  cpu-gather: Move model_name to new script
  cpu-gather: Allow overwriting model name
  cpu-gather: Move cpuid call to new script
  cpu-gather: Allow overwriting cpuid binary location
  cpu-gather: Move msr decoding to new script
  cpu-gather: Move qemu detection to new script
  cpu-gather: Move static model expansion to new script
  cpu-gather: Move static model extraction to new script
  cpu-gather: Move simple model extraction to new script
  cpu-gather: Move full model extraction to new script
  cpu-gather: Merge model gathering logic
  cpu-gather: Delete old script
  cpu-gather: Separate data input and output
  cpu-parse: Wrap with python script
  cpu-gather: Transport data as json
  cpu-parse: Move model name detection to new script
  cpu-parse: Move file name generation to new script
  cpu-parse: Move xml output to new script
  cpu-parse: Move json output to new script
  cpu-parse: Move call to cpu-cpuid.py to new script
  cpu-parse: Delete old script
  cpu-gather: Ignore empty responses from qemu
  cpu-gather: Ignore shutdown messages from qemu
  cpu-gather: Parse cpuid leaves early
  cpu-gather: Allow gathering and parsing data in one step.
  cpu-gather: Prepare gather_msr for reading multiple msr
  cpu-gather: Add IA32_CORE_CAPABILITY_MSR

 tests/cputestdata/cpu-cpuid.py|   5 +-
 tests/cputestdata/cpu-gather.py   | 365 ++
 tests/cputestdata/cpu-gather.sh   | 103 -
 tests/cputestdata/cpu-parse.sh|  65 --
 tests/cputestdata/cpu-reformat.py |   9 -
 5 files changed, 368 insertions(+), 179 deletions(-)
 create mode 100755 tests/cputestdata/cpu-gather.py
 delete mode 100755 tests/cputestdata/cpu-gather.sh
 delete mode 100755 tests/cputestdata/cpu-parse.sh
 delete mode 100755 tests/cputestdata/cpu-reformat.py

-- 
2.26.2

[libvirt PATCH 25/29] cpu-gather: Ignore shutdown messages from qemu

[Tim Wiederhake]

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-gather.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index 10dbeb87cf..49c72df320 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -98,6 +98,8 @@ def call_qemu(qemu, qmp_cmds):
 response = json.loads(line)
 if "return" in response and not response["return"]:
 continue
+if response.get("event") == "SHUTDOWN":
+continue
 yield response
 
 
-- 
2.26.2

[libvirt PATCH 26/29] cpu-gather: Parse cpuid leaves early

[Tim Wiederhake]

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-gather.py | 48 ++---
 1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index 49c72df320..fe660c486e 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -24,6 +24,15 @@ def gather_name(args):
 
 
 def gather_cpuid_leaves(args):
+leave_pattern = re.compile(
+"^\\s*"
+"(0x[0-9a-f]+)\\s*"
+"(0x[0-9a-f]+):\\s*"
+"eax=(0x[0-9a-f]+)\\s*"
+"ebx=(0x[0-9a-f]+)\\s*"
+"ecx=(0x[0-9a-f]+)\\s*"
+"edx=(0x[0-9a-f]+)\\s*$")
+
 cpuid = args.path_to_cpuid or "cpuid"
 try:
 output = subprocess.check_output(
@@ -36,11 +45,16 @@ def gather_cpuid_leaves(args):
  "'http://www.etallen.com/cpuid.html'.".format(e.filename))
 
 for line in output.split("\n"):
-if not line:
+match = leave_pattern.match(line)
+if not match:
 continue
-if line == "CPU:":
-continue
-yield line.strip()
+yield {
+"eax_in": int(match.group(1), 0),
+"ecx_in": int(match.group(2), 0),
+"eax": int(match.group(3), 0),
+"ebx": int(match.group(4), 0),
+"ecx": int(match.group(5), 0),
+"edx": int(match.group(6), 0)}
 
 
 def gather_msr():
@@ -214,23 +228,14 @@ def parse_filename(data):
 
 
 def output_xml(data, filename):
-leave_pattern = re.compile(
-"^\\s*"
-"(0x[0-9a-f]+)\\s*"
-"(0x[0-9a-f]+):\\s*"
-"eax=(0x[0-9a-f]+)\\s*"
-"ebx=(0x[0-9a-f]+)\\s*"
-"ecx=(0x[0-9a-f]+)\\s*"
-"edx=(0x[0-9a-f]+)\\s*$")
-
 leave_template = \
 "  \n"
 
 msr_template = "  \n"
@@ -239,9 +244,8 @@ def output_xml(data, filename):
 with open(filename, "wt") as f:
 f.write("\n".format(data["name"]))
 f.write("\n")
-for line in data["leaves"]:
-match = leave_pattern.match(line)
-f.write(leave_template.format(*match.groups()))
+for leave in data["leaves"]:
+f.write(leave_template.format(leave))
 for key, value in sorted(data["msr"].items()):
 f.write(msr_template.format(
 int(key),
-- 
2.26.2

[libvirt PATCH 08/29] cpu-gather: Move qemu detection to new script

[Tim Wiederhake]

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-gather.py | 17 +
 tests/cputestdata/cpu-gather.sh |  8 
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index db2348b460..7f9479f78d 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -77,9 +77,25 @@ def main():
 metavar="PATH",
 help="Path to 'cpuid' utility. "
 "If unset, the first executable 'cpuid' in $PATH is used.")
+parser.add_argument(
+"--path-to-qemu",
+metavar="PATH",
+help="Path to qemu. "
+"If unset, will try '/usr/bin/qemu-system-x86_64', "
+"'/usr/bin/qemu-kvm', and '/usr/libexec/qemu-kvm'.")
 
 args = parser.parse_args()
 
+if not args.path_to_qemu:
+args.path_to_qemu = "qemu-system-x86_64"
+search = [
+"/usr/bin/qemu-system-x86_64",
+"/usr/bin/qemu-kvm",
+"/usr/libexec/qemu-kvm"]
+for f in search:
+if os.path.isfile(f):
+args.path_to_qemu = f
+
 name = gather_name(args)
 print("model name\t: {}".format(name))
 
@@ -97,6 +113,7 @@ def main():
 
 print(end="", flush=True)
 os.environ["CPU_GATHER_PY"] = "true"
+os.environ["qemu"] = args.path_to_qemu
 subprocess.check_call("./cpu-gather.sh")
 
 
diff --git a/tests/cputestdata/cpu-gather.sh b/tests/cputestdata/cpu-gather.sh
index 427b81a64b..4b4ac1a47c 100755
--- a/tests/cputestdata/cpu-gather.sh
+++ b/tests/cputestdata/cpu-gather.sh
@@ -5,14 +5,6 @@ if [ -z "${CPU_GATHER_PY}" ]; then
 exit 1
 fi
 
-qemu=qemu-system-x86_64
-for cmd in /usr/bin/$qemu /usr/bin/qemu-kvm /usr/libexec/qemu-kvm; do
-if [[ -x $cmd ]]; then
-qemu=$cmd
-break
-fi
-done
-
 qom_get()
 {
 path='/machine/unattached/device[0]'
-- 
2.26.2

[libvirt PATCH 20/29] cpu-parse: Move xml output to new script

[Tim Wiederhake]

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-gather.py | 40 +
 tests/cputestdata/cpu-parse.sh  | 18 ---
 2 files changed, 40 insertions(+), 18 deletions(-)

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index 1a15cc1ff0..bc5c7dbb15 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -207,10 +207,50 @@ def parse_filename(data):
 return "x86_64-cpuid-{}".format(filename)
 
 
+def output_xml(data, filename):
+leave_pattern = re.compile(
+"^\\s*"
+"(0x[0-9a-f]+)\\s*"
+"(0x[0-9a-f]+):\\s*"
+"eax=(0x[0-9a-f]+)\\s*"
+"ebx=(0x[0-9a-f]+)\\s*"
+"ecx=(0x[0-9a-f]+)\\s*"
+"edx=(0x[0-9a-f]+)\\s*$")
+
+leave_template = \
+"  \n"
+
+msr_template = "  \n"
+
+print(filename)
+with open(filename, "wt") as f:
+f.write("\n".format(data["name"]))
+f.write("\n")
+for line in data["leaves"]:
+match = leave_pattern.match(line)
+f.write(leave_template.format(*match.groups()))
+for key, value in sorted(data["msr"].items()):
+f.write(msr_template.format(
+int(key),
+0x & (value >> 32),
+0x & (value >> 0)))
+f.write("\n")
+
+
 def parse(args):
 data = json.load(sys.stdin)
 
 filename = parse_filename(data)
+filename_xml = "{}.xml".format(filename)
+
+output_xml(data, filename_xml)
 
 os.environ["CPU_GATHER_PY"] = "true"
 os.environ["model"] = data["name"]
diff --git a/tests/cputestdata/cpu-parse.sh b/tests/cputestdata/cpu-parse.sh
index 2a41897538..84d37d0df4 100755
--- a/tests/cputestdata/cpu-parse.sh
+++ b/tests/cputestdata/cpu-parse.sh
@@ -7,21 +7,6 @@ fi
 
 data=`cat`
 
-xml()
-{
-hex='\(0x[0-9a-f]\+\)'
-matchCPUID="$hex $hex: eax=$hex ebx=$hex ecx=$hex edx=$hex"
-substCPUID=""
-
-matchMSR="$hex: $hex\(...[0-9a-f]\)"
-substMSR=""
-
-echo ""
-echo ""
-sed -ne "s/^ *$matchCPUID$/  $substCPUID/p; s/^ *$matchMSR$/  $substMSR/p"
-echo ""
-}
-
 json()
 {
 first=true
@@ -36,9 +21,6 @@ json()
 done
 }
 
-xml <<<"$data" >$fname.xml
-echo $fname.xml
-
 json <<<"$data" >$fname.json
 if [[ -s $fname.json ]]; then
 echo $fname.json
-- 
2.26.2

[libvirt PATCH 03/29] cpu-gather: Move model_name to new script

[Tim Wiederhake]

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-gather.py | 13 +
 tests/cputestdata/cpu-gather.sh |  2 --
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index f7030eb48b..1b02df6ec7 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -4,7 +4,20 @@ import os
 import subprocess
 
 
+def gather_name():
+with open("/proc/cpuinfo", "rt") as f:
+for line in f.readlines():
+if line.startswith("model name"):
+return line.split(":", 2)[1].strip()
+
+exit("Error: '/proc/cpuinfo' does not contain a model name.")
+
+
 def main():
+name = gather_name()
+print("model name\t: {}".format(name))
+
+print(end="", flush=True)
 os.environ["CPU_GATHER_PY"] = "true"
 subprocess.check_call("./cpu-gather.sh")
 
diff --git a/tests/cputestdata/cpu-gather.sh b/tests/cputestdata/cpu-gather.sh
index cd65d74da5..b671f223a5 100755
--- a/tests/cputestdata/cpu-gather.sh
+++ b/tests/cputestdata/cpu-gather.sh
@@ -9,8 +9,6 @@ if [ -z "${CPU_GATHER_PY}" ]; then
 exit 1
 fi
 
-grep 'model name' /proc/cpuinfo | head -n1
-
 cpuid -1r
 echo
 
-- 
2.26.2

[libvirt PATCH 04/29] cpu-gather: Allow overwriting model name

[Tim Wiederhake]

Some hardware, e.g. exotic platforms or pre-production hardware, may
report wrong or random data for the cpu model name. As the name of
the created files is derived from that name, this may lead to issues.

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-gather.py | 19 ---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index 1b02df6ec7..4e8c72e4f4 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -1,20 +1,33 @@
 #!/usr/bin/env python3
 
+import argparse
 import os
 import subprocess
 
 
-def gather_name():
+def gather_name(args):
+if args.name:
+return args.name
+
 with open("/proc/cpuinfo", "rt") as f:
 for line in f.readlines():
 if line.startswith("model name"):
 return line.split(":", 2)[1].strip()
 
-exit("Error: '/proc/cpuinfo' does not contain a model name.")
+exit("Error: '/proc/cpuinfo' does not contain a model name.\n"
+ "Use '--model' to set a model name.")
 
 
 def main():
-name = gather_name()
+parser = argparse.ArgumentParser(description="Gather cpu test data")
+parser.add_argument(
+"--name",
+help="CPU model name. "
+"If unset, model name is read from '/proc/cpuinfo'.")
+
+args = parser.parse_args()
+
+name = gather_name(args)
 print("model name\t: {}".format(name))
 
 print(end="", flush=True)
-- 
2.26.2

[libvirt PATCH 07/29] cpu-gather: Move msr decoding to new script

[Tim Wiederhake]

Fixes the leaking file descriptors. Does not silently ignore errors
(e.g. permission denied on /dev/cpu/0/msr if run as non-root) and
always attempt to read from /dev/kvm if /dev/cpu/0/msr failed.

'gather_msr()' returns a dictionary of values, as a later patch will
add more registers to be interrogated.

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-gather.py | 34 
 tests/cputestdata/cpu-gather.sh | 40 -
 2 files changed, 34 insertions(+), 40 deletions(-)

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index 75cf290a28..db2348b460 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -1,8 +1,11 @@
 #!/usr/bin/env python3
 
 import argparse
+import fcntl
 import os
+import struct
 import subprocess
+import sys
 
 
 def gather_name(args):
@@ -38,6 +41,31 @@ def gather_cpuid_leaves(args):
 yield line.strip()
 
 
+def gather_msr():
+IA32_ARCH_CAPABILITIES_MSR = 0x10a
+KVM_GET_MSRS = 0xc008ae88
+
+try:
+with open("/dev/cpu/0/msr", "rb") as f:
+f.seek(IA32_ARCH_CAPABILITIES_MSR)
+buf = f.read(8)
+msr = struct.unpack("=Q", buf)[0]
+return "", {IA32_ARCH_CAPABILITIES_MSR: msr}
+except IOError as e:
+print("Warning: {}".format(e), file=sys.stderr)
+
+try:
+bufIn = struct.pack("=Q", 1, 0, IA32_ARCH_CAPABILITIES_MSR, 0, 0)
+with open("/dev/kvm", "rb") as f:
+bufOut = fcntl.ioctl(f, KVM_GET_MSRS, bufIn)
+msr = struct.unpack("=Q", bufOut)[4]
+return " via KVM", {IA32_ARCH_CAPABILITIES_MSR: msr}
+except IOError as e:
+print("Warning: {}".format(e), file=sys.stderr)
+
+return None, {}
+
+
 def main():
 parser = argparse.ArgumentParser(description="Gather cpu test data")
 parser.add_argument(
@@ -61,6 +89,12 @@ def main():
 print("   {}".format(leave))
 print()
 
+via, msr = gather_msr()
+if via is not None:
+print("MSR{}:".format(via))
+for key, value in sorted(msr.items()):
+print("   0x{:x}: 0x{:016x}\n".format(int(key), value))
+
 print(end="", flush=True)
 os.environ["CPU_GATHER_PY"] = "true"
 subprocess.check_call("./cpu-gather.sh")
diff --git a/tests/cputestdata/cpu-gather.sh b/tests/cputestdata/cpu-gather.sh
index f84215e777..427b81a64b 100755
--- a/tests/cputestdata/cpu-gather.sh
+++ b/tests/cputestdata/cpu-gather.sh
@@ -5,46 +5,6 @@ if [ -z "${CPU_GATHER_PY}" ]; then
 exit 1
 fi
 
-python3 <

[libvirt PATCH 09/29] cpu-gather: Move static model expansion to new script

[Tim Wiederhake]

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-gather.py | 45 +
 tests/cputestdata/cpu-gather.sh |  7 -
 2 files changed, 45 insertions(+), 7 deletions(-)

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index 7f9479f78d..e4a82fc949 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -2,6 +2,7 @@
 
 import argparse
 import fcntl
+import json
 import os
 import struct
 import subprocess
@@ -66,6 +67,47 @@ def gather_msr():
 return None, {}
 
 
+def call_qemu(qemu, qmp_cmds):
+cmd = [
+qemu,
+"-machine", "accel=kvm",
+"-cpu", "host",
+"-nodefaults",
+"-nographic",
+"-qmp", "stdio"]
+
+stdin = list()
+stdin.append("{\"execute\": \"qmp_capabilities\"}")
+stdin.extend([json.dumps(o) for o in qmp_cmds])
+stdin.append("{\"execute\": \"quit\"}")
+
+try:
+output = subprocess.check_output(
+cmd,
+universal_newlines=True,
+input="\n".join(stdin))
+except subprocess.CalledProcessError:
+exit("Error: Non-zero exit code from '{}'.".format(qemu))
+except FileNotFoundError:
+exit("Error: File not found: '{}'.".format(qemu))
+
+return output
+
+
+def gather_static_model(args):
+output = call_qemu(args.path_to_qemu, [
+{
+"execute": "query-cpu-model-expansion",
+"arguments":
+{
+"type": "static",
+"model": {"name": "host"}
+},
+"id": "model-expansion"
+}])
+return output
+
+
 def main():
 parser = argparse.ArgumentParser(description="Gather cpu test data")
 parser.add_argument(
@@ -111,9 +153,12 @@ def main():
 for key, value in sorted(msr.items()):
 print("   0x{:x}: 0x{:016x}\n".format(int(key), value))
 
+static_model = gather_static_model(args)
+
 print(end="", flush=True)
 os.environ["CPU_GATHER_PY"] = "true"
 os.environ["qemu"] = args.path_to_qemu
+os.environ["model"] = static_model
 subprocess.check_call("./cpu-gather.sh")
 
 
diff --git a/tests/cputestdata/cpu-gather.sh b/tests/cputestdata/cpu-gather.sh
index 4b4ac1a47c..af0f40b61c 100755
--- a/tests/cputestdata/cpu-gather.sh
+++ b/tests/cputestdata/cpu-gather.sh
@@ -21,13 +21,6 @@ model_expansion()
  '{"type":"'"$mode"'","model":'"$model"'},"id":"model-expansion"}'
 }
 
-model=$(
-$qemu -machine accel=kvm -cpu host -nodefaults -nographic -qmp stdio <

[libvirt PATCH 10/29] cpu-gather: Move static model extraction to new script

[Tim Wiederhake]

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-gather.py | 13 ++---
 tests/cputestdata/cpu-gather.sh |  5 -
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index e4a82fc949..0c8d8e93d0 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -91,7 +91,9 @@ def call_qemu(qemu, qmp_cmds):
 except FileNotFoundError:
 exit("Error: File not found: '{}'.".format(qemu))
 
-return output
+for line in output.split("\n"):
+if line:
+yield json.loads(line)
 
 
 def gather_static_model(args):
@@ -105,7 +107,12 @@ def gather_static_model(args):
 },
 "id": "model-expansion"
 }])
-return output
+
+for o in output:
+if o.get("id") == "model-expansion":
+return o["return"]["model"]
+
+return None
 
 
 def main():
@@ -158,7 +165,7 @@ def main():
 print(end="", flush=True)
 os.environ["CPU_GATHER_PY"] = "true"
 os.environ["qemu"] = args.path_to_qemu
-os.environ["model"] = static_model
+os.environ["model"] = json.dumps(static_model) if static_model else ""
 subprocess.check_call("./cpu-gather.sh")
 
 
diff --git a/tests/cputestdata/cpu-gather.sh b/tests/cputestdata/cpu-gather.sh
index af0f40b61c..726f013908 100755
--- a/tests/cputestdata/cpu-gather.sh
+++ b/tests/cputestdata/cpu-gather.sh
@@ -21,11 +21,6 @@ model_expansion()
  '{"type":"'"$mode"'","model":'"$model"'},"id":"model-expansion"}'
 }
 
-model=$(
-echo "$model" | \
-sed -ne 's/^{"return": {"model": {\(.*{.*}\)}}, .*/{\1}/p'
-)
-
 $qemu -machine accel=kvm -cpu host -nodefaults -nographic -qmp stdio <

[libvirt PATCH 14/29] cpu-gather: Delete old script

[Tim Wiederhake]

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-gather.py | 5 -
 tests/cputestdata/cpu-gather.sh | 6 --
 2 files changed, 11 deletions(-)
 delete mode 100755 tests/cputestdata/cpu-gather.sh

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index c639c433d2..9c609aa6e9 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -232,11 +232,6 @@ def main():
 for o in model:
 print(json.dumps(o))
 
-print(end="", flush=True)
-os.environ["CPU_GATHER_PY"] = "true"
-os.environ["qemu"] = args.path_to_qemu
-subprocess.check_call("./cpu-gather.sh")
-
 
 if __name__ == "__main__":
 main()
diff --git a/tests/cputestdata/cpu-gather.sh b/tests/cputestdata/cpu-gather.sh
deleted file mode 100755
index 5cef26c8e6..00
--- a/tests/cputestdata/cpu-gather.sh
+++ /dev/null
@@ -1,6 +0,0 @@
-#!/bin/bash
-
-if [ -z "${CPU_GATHER_PY}" ]; then
-echo >&2 "Do not call this script directly. Use 'cpu-gather.py' instead."
-exit 1
-fi
-- 
2.26.2

[libvirt PATCH 16/29] cpu-parse: Wrap with python script

[Tim Wiederhake]

This changes the invocation from
  ./cpu-gather.py | ./cpu-parse.sh
to
  ./cpu-gather.py [--gather] | ./cpu-gather.py --parse

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-gather.py | 29 +++--
 tests/cputestdata/cpu-parse.sh  |  6 --
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index 005d1019b6..83f175d342 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -192,6 +192,15 @@ def gather(args):
 return result
 
 
+def parse(args):
+os.environ["CPU_GATHER_PY"] = "true"
+output = subprocess.check_output(
+"./cpu-parse.sh",
+stderr=subprocess.STDOUT,
+universal_newlines=True)
+print(output)
+
+
 def output_to_text(data):
 output = list()
 
@@ -231,8 +240,21 @@ def main():
 "If unset, will try '/usr/bin/qemu-system-x86_64', "
 "'/usr/bin/qemu-kvm', and '/usr/libexec/qemu-kvm'.")
 
+mode = parser.add_mutually_exclusive_group()
+mode.add_argument(
+"--gather",
+action="store_true",
+help="Acquire data on target system. This is the default.")
+mode.add_argument(
+"--parse",
+action="store_true",
+help="Parse data for libvirt use.")
+
 args = parser.parse_args()
 
+if not args.gather and not args.parse:
+args.gather = True
+
 if not args.path_to_qemu:
 args.path_to_qemu = "qemu-system-x86_64"
 search = [
@@ -243,8 +265,11 @@ def main():
 if os.path.isfile(f):
 args.path_to_qemu = f
 
-data = gather(args)
-print(output_to_text(data))
+if args.gather:
+data = gather(args)
+print(output_to_text(data))
+else:
+parse(args)
 
 
 if __name__ == "__main__":
diff --git a/tests/cputestdata/cpu-parse.sh b/tests/cputestdata/cpu-parse.sh
index 7501c57cba..fa4344b6ad 100755
--- a/tests/cputestdata/cpu-parse.sh
+++ b/tests/cputestdata/cpu-parse.sh
@@ -1,7 +1,9 @@
 #!/bin/bash
 
-# Usage:
-# ./cpu-gather.sh | ./cpu-parse.sh
+if [ -z "${CPU_GATHER_PY}" ]; then
+echo >&2 "Do not call this script directly. Use 'cpu-gather.py' instead."
+exit 1
+fi
 
 data=`cat`
 
-- 
2.26.2

[libvirt PATCH 17/29] cpu-gather: Transport data as json

[Tim Wiederhake]

More reliable, easier to parse, easier to edit.

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-gather.py | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index 83f175d342..7c51a47353 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -193,9 +193,12 @@ def gather(args):
 
 
 def parse(args):
+data = json.load(sys.stdin)
+
 os.environ["CPU_GATHER_PY"] = "true"
 output = subprocess.check_output(
 "./cpu-parse.sh",
+input=output_to_text(data),
 stderr=subprocess.STDOUT,
 universal_newlines=True)
 print(output)
@@ -267,7 +270,7 @@ def main():
 
 if args.gather:
 data = gather(args)
-print(output_to_text(data))
+json.dump(data, sys.stdout, indent=2)
 else:
 parse(args)
 
-- 
2.26.2

[libvirt PATCH 29/29] cpu-gather: Add IA32_CORE_CAPABILITY_MSR

[Tim Wiederhake]

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-gather.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index 9c9eec6d93..a01c615504 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -61,6 +61,7 @@ def gather_msr():
 msrs = dict()
 addresses = [
 0x10a,  # IA32_ARCH_CAPABILITIES_MSR
+0xcf,   # IA32_CORE_CAPABILITY_MSR
 ]
 KVM_GET_MSRS = 0xc008ae88
 
-- 
2.26.2

[libvirt PATCH 18/29] cpu-parse: Move model name detection to new script

[Tim Wiederhake]

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-gather.py | 1 +
 tests/cputestdata/cpu-parse.sh  | 2 --
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index 7c51a47353..0b1019456c 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -196,6 +196,7 @@ def parse(args):
 data = json.load(sys.stdin)
 
 os.environ["CPU_GATHER_PY"] = "true"
+os.environ["model"] = data["name"]
 output = subprocess.check_output(
 "./cpu-parse.sh",
 input=output_to_text(data),
diff --git a/tests/cputestdata/cpu-parse.sh b/tests/cputestdata/cpu-parse.sh
index fa4344b6ad..5b8e57e427 100755
--- a/tests/cputestdata/cpu-parse.sh
+++ b/tests/cputestdata/cpu-parse.sh
@@ -7,8 +7,6 @@ fi
 
 data=`cat`
 
-model=`sed -ne '/^model name[  ]*:/ {s/^[^:]*: \(.*\)/\1/p; q}' <<<"$data"`
-
 fname=`sed -e 's/^ *//;
s/ *$//;
s/[ -]\+ \+/ /g;
-- 
2.26.2

[libvirt PATCH 28/29] cpu-gather: Prepare gather_msr for reading multiple msr

[Tim Wiederhake]

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-gather.py | 23 ++-
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index 46997c8a48..9c9eec6d93 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -58,24 +58,29 @@ def gather_cpuid_leaves(args):
 
 
 def gather_msr():
-IA32_ARCH_CAPABILITIES_MSR = 0x10a
+msrs = dict()
+addresses = [
+0x10a,  # IA32_ARCH_CAPABILITIES_MSR
+]
 KVM_GET_MSRS = 0xc008ae88
 
 try:
 with open("/dev/cpu/0/msr", "rb") as f:
-f.seek(IA32_ARCH_CAPABILITIES_MSR)
-buf = f.read(8)
-msr = struct.unpack("=Q", buf)[0]
-return "", {IA32_ARCH_CAPABILITIES_MSR: msr}
+for addr in addresses:
+f.seek(addr)
+buf = f.read(8)
+msrs[addr] = struct.unpack("=Q", buf)[0]
+return "", msrs
 except IOError as e:
 print("Warning: {}".format(e), file=sys.stderr)
 
 try:
-bufIn = struct.pack("=Q", 1, 0, IA32_ARCH_CAPABILITIES_MSR, 0, 0)
 with open("/dev/kvm", "rb") as f:
-bufOut = fcntl.ioctl(f, KVM_GET_MSRS, bufIn)
-msr = struct.unpack("=Q", bufOut)[4]
-return " via KVM", {IA32_ARCH_CAPABILITIES_MSR: msr}
+for addr in addresses:
+bufIn = struct.pack("=Q", 1, 0, addr, 0, 0)
+bufOut = fcntl.ioctl(f, KVM_GET_MSRS, bufIn)
+msrs[addr] = struct.unpack("=Q", bufOut)[4]
+return " via KVM", msrs
 except IOError as e:
 print("Warning: {}".format(e), file=sys.stderr)
 
-- 
2.26.2

[libvirt PATCH 24/29] cpu-gather: Ignore empty responses from qemu

[Tim Wiederhake]

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-gather.py | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index b6acae9eb7..10dbeb87cf 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -93,8 +93,12 @@ def call_qemu(qemu, qmp_cmds):
 exit("Error: File not found: '{}'.".format(qemu))
 
 for line in output.split("\n"):
-if line:
-yield json.loads(line)
+if not line:
+continue
+response = json.loads(line)
+if "return" in response and not response["return"]:
+continue
+yield response
 
 
 def gather_model(args):
-- 
2.26.2

Re: [RFC PATCH 3/6] docs: added rng schema and formatdomain for NFS

[Laine Stump]

On 12/14/20 5:57 PM, Ryan Gahagan wrote:
On Thu, Dec 10, 2020 at 10:37 PM Han Han > wrote:


On Fri, Dec 11, 2020 at 4:00 AM Ryan Gahagan
mailto:rgaha...@cs.utexas.edu>> wrote:

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 512939679b..40a1a3c1e2 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -2601,6 +2601,7 @@ paravirtualized driver is specified via
the ``disk`` element.
       sheepdog one of the sheepdog servers (default is
localhost:7000) zero or one                           7000
       gluster  a server running glusterd daemon              
    one or more ( :since:`Since 2.1.0` ), just one prior to
that 24007
       vxhs     a server running Veritas HyperScale daemon   
          only one                            
+      nfs      a server running Network File System          
        only one                          2049

Mention the feature introduced version here.

What version would we specify? QEMU's version 2.9.0 patch where they 
introduce NFS support? The Libvirt current version 6.10.0 patch? A 
future patch number (e.g. Libvirt version 6.11.0 or similar)? We were 
unclear on what exact number goes here, but we were planning on 
putting it into the space parallel to the :since: in the gluster line.



You would put the version that libvirt will be at when the feature is in 
released code, so if it was pushed today, that would be 7.0.0. 
(Sometimes we also indicate a minimum qemu or kernel version that's 
required, which is more useful if a feature is just newly supported in 
qemu/kernel but over time it just becomes extra numbers to confuse 
people. In this case, my opinion would be that the qemu version is old 
enough that it's not necessary to mention it.)

Re: [RFC PATCH 1/6] compatibilities: added in flags for NFS support

[Laine Stump]

(answering for Peter since he is on "vacation", so it may be awhile 
before he gets around to this)


On 12/14/20 5:54 PM, Ryan Gahagan wrote:
On Fri, Dec 11, 2020 at 3:35 AM Peter Krempa > wrote:


In subject/summary. We don't have anything which we'd prefix with
'compatibilities:'.


Just to confirm, does this mean that we should not implement the 
feedback Han Han suggested about the NFS capability flags and instead 
leave the commit as-is (except for the summary)?



Yes. What Han Han was discussing is the QEMU capabilities flags, which 
indicate which of a large set of features are supported by a particular 
QEMU binary. Since, as you say below, any QEMU binary that is capable of 
using blockdev is by definition capable of using NFS, there is no need 
to add an extra flag for NFS.




We didn't provide an NFS CAPS flag because in a previous email you had 
suggested:

"- there's no need to add a specific capability for the NFS protocol as
it predates libvirt's use of -blockdev (QEMU_CAPS_BLOCKDEV). You have 
to add a check for it to qemuDomainValidateStorageSource based on the 
above capabapility."
We will provide this check in qemuDomainValidateStorageSource, but do 
we need to worry about CAPS flags?

[libvirt][PATCH v1 0/3] introduce 'restrictive' mode in numatune

[Luyao Zhong]

Before this patch set, numatune only has three memory modes:
static, interleave and prefered. These memory policies are
ultimately set by mbind() system call.

Memory policy could be 'hard coded' into the kernel, but none of
above policies fit our requirment under this case. mbind() support
default memory policy, but it requires a NULL nodemask. So obviously
setting allowed memory nodes is cgroups' mission under this case.
So we introduce a new option for mode in numatune named 'restrictive'.


   
   
   


The config above means we only use cgroups to restrict the allowed
memory nodes and not setting any specific memory policies explicitly.

RFC discussion:
https://www.redhat.com/archives/libvir-list/2020-November/msg01256.html

Regards,
Luyao

Luyao Zhong (3):
  docs: add docs for 'restrictive' option for mode in numatune
  schema: add 'restrictive' config option for mode in numatune
  qemu: add parser and formatter for 'restrictive' mode in numatune

 docs/formatdomain.rst |  7 +++-
 docs/schemas/domaincommon.rng |  2 +
 include/libvirt/libvirt-domain.h  |  1 +
 src/conf/numa_conf.c  |  9 +
 src/qemu/qemu_command.c   |  6 ++-
 src/qemu/qemu_process.c   | 27 +
 src/util/virnuma.c|  3 ++
 .../numatune-memnode-invalid-mode.err |  1 +
 .../numatune-memnode-invalid-mode.xml | 33 +++
 ...emnode-restrictive-mode.x86_64-latest.args | 40 +++
 .../numatune-memnode-restrictive-mode.xml | 33 +++
 tests/qemuxml2argvtest.c  |  2 +
 ...memnode-restrictive-mode.x86_64-latest.xml | 40 +++
 tests/qemuxml2xmltest.c   |  1 +
 14 files changed, 202 insertions(+), 3 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/numatune-memnode-invalid-mode.err
 create mode 100644 tests/qemuxml2argvdata/numatune-memnode-invalid-mode.xml
 create mode 100644 
tests/qemuxml2argvdata/numatune-memnode-restrictive-mode.x86_64-latest.args
 create mode 100644 tests/qemuxml2argvdata/numatune-memnode-restrictive-mode.xml
 create mode 100644 
tests/qemuxml2xmloutdata/numatune-memnode-restrictive-mode.x86_64-latest.xml

-- 
2.25.4

[libvirt][PATCH v1 1/3] docs: add docs for 'restrictive' option for mode in numatune

[Luyao Zhong]

When user would like use cgroups to restrict the allowed memory
nodes, and require not setting any specific memory policy, then
'restrictive' mode is useful.
---
 docs/formatdomain.rst | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index ff64996af2..90abe27d30 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -1086,8 +1086,11 @@ NUMA Node Tuning
 ``memory``
The optional ``memory`` element specifies how to allocate memory for the
domain process on a NUMA host. It contains several optional attributes.
-   Attribute ``mode`` is either 'interleave', 'strict', or 'preferred', 
defaults
-   to 'strict'. Attribute ``nodeset`` specifies the NUMA nodes, using the same
+   Attribute ``mode`` is either 'interleave', 'strict', 'preferred' or
+   'restrictive', defaults to 'strict'. The value 'restrictive' specifies
+   using system default policy and only cgroups is used to restrict the
+   memory nodes, and it requires setting mode to 'restrictive' in ``memnode``
+   elements. Attribute ``nodeset`` specifies the NUMA nodes, using the same
syntax as attribute ``cpuset`` of element ``vcpu``. Attribute ``placement`` 
(
:since:`since 0.9.12` ) can be used to indicate the memory placement mode 
for
domain process, its value can be either "static" or "auto", defaults to
-- 
2.25.4

[libvirt][PATCH v1 3/3] qemu: add parser and formatter for 'restrictive' mode in numatune

[Luyao Zhong]

---
 include/libvirt/libvirt-domain.h  |  1 +
 src/conf/numa_conf.c  |  9 +
 src/qemu/qemu_command.c   |  6 ++-
 src/qemu/qemu_process.c   | 27 +
 src/util/virnuma.c|  3 ++
 .../numatune-memnode-invalid-mode.err |  1 +
 .../numatune-memnode-invalid-mode.xml | 33 +++
 ...emnode-restrictive-mode.x86_64-latest.args | 40 +++
 .../numatune-memnode-restrictive-mode.xml | 33 +++
 tests/qemuxml2argvtest.c  |  2 +
 ...memnode-restrictive-mode.x86_64-latest.xml | 40 +++
 tests/qemuxml2xmltest.c   |  1 +
 12 files changed, 195 insertions(+), 1 deletion(-)
 create mode 100644 tests/qemuxml2argvdata/numatune-memnode-invalid-mode.err
 create mode 100644 tests/qemuxml2argvdata/numatune-memnode-invalid-mode.xml
 create mode 100644 
tests/qemuxml2argvdata/numatune-memnode-restrictive-mode.x86_64-latest.args
 create mode 100644 tests/qemuxml2argvdata/numatune-memnode-restrictive-mode.xml
 create mode 100644 
tests/qemuxml2xmloutdata/numatune-memnode-restrictive-mode.x86_64-latest.xml

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index b929877643..309040bf97 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -1527,6 +1527,7 @@ typedef enum {
 VIR_DOMAIN_NUMATUNE_MEM_STRICT  = 0,
 VIR_DOMAIN_NUMATUNE_MEM_PREFERRED   = 1,
 VIR_DOMAIN_NUMATUNE_MEM_INTERLEAVE  = 2,
+VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE = 3,
 
 # ifdef VIR_ENUM_SENTINELS
 VIR_DOMAIN_NUMATUNE_MEM_LAST /* This constant is subject to change */
diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
index f8a7a01ac9..df888a8dfb 100644
--- a/src/conf/numa_conf.c
+++ b/src/conf/numa_conf.c
@@ -43,6 +43,7 @@ VIR_ENUM_IMPL(virDomainNumatuneMemMode,
   "strict",
   "preferred",
   "interleave",
+  "restrictive",
 );
 
 VIR_ENUM_IMPL(virDomainNumatunePlacement,
@@ -234,6 +235,14 @@ virDomainNumatuneNodeParseXML(virDomainNumaPtr numa,
_("Invalid mode attribute in memnode element"));
 goto cleanup;
 }
+
+if (numa->memory.mode == VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE &&
+mode != VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("'restrictive' mode is required in memnode 
element "
+ "when mode is 'restrictive' in memory 
element"));
+goto cleanup;
+}
 VIR_FREE(tmp);
 mem_node->mode = mode;
 }
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 479bcc0b0c..c0eff6cb12 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -174,6 +174,7 @@ VIR_ENUM_IMPL(qemuNumaPolicy,
   "bind",
   "preferred",
   "interleave",
+  "restricted",
 );
 
 
@@ -3138,7 +3139,10 @@ qemuBuildMemoryBackendProps(virJSONValuePtr 
*backendProps,
 return -1;
 }
 
-if (nodemask) {
+/* If mode is "restrictive", we should only use cgroups setting allowed 
memory
+ * nodes, and skip passing the host-nodes and policy parameters to QEMU 
command
+ * line which means we will use system default memory policy. */
+if (nodemask && mode != VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE) {
 if (!virNumaNodesetIsAvailable(nodemask))
 return -1;
 if (virJSONValueObjectAdd(props,
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 579b3c3713..8c260883d9 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2709,6 +2709,7 @@ qemuProcessSetupPid(virDomainObjPtr vm,
 g_autoptr(virBitmap) hostcpumap = NULL;
 g_autofree char *mem_mask = NULL;
 int ret = -1;
+size_t i;
 
 if ((period || quota) &&
 !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
@@ -2749,6 +2750,32 @@ qemuProcessSetupPid(virDomainObjPtr vm,
 &mem_mask, -1) < 0)
 goto cleanup;
 
+/* For vCPU threads, mem_mask is different among cells and mem_mask
+ * is used to set cgroups cpuset.mems for vcpu threads. If we specify
+ * 'restrictive' mode, that means we will set system default memory
+ * policy and only use cgroups to restrict allowed memory nodes. */
+if (nameval == VIR_CGROUP_THREAD_VCPU) {
+virDomainNumaPtr numatune = vm->def->numa;
+virBitmapPtr numanode_cpumask = NULL;
+for (i = 0; i < virDomainNumaGetNodeCount(numatune); i++) {
+numanode_cpumask = virDomainNumaGetNodeCpumask(numatune, i);
+/* 'i' indicates the cell id, if the vCPU id is in this cell
+

[libvirt][PATCH v1 2/3] schema: add 'restrictive' config option for mode in numatune

[Luyao Zhong]

support 'restrictive' mode in memory element and memnode
element in numatune:
  
...

  
  
  

...
  
---
 docs/schemas/domaincommon.rng | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 795b654feb..bf1c406e37 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -1097,6 +1097,7 @@
   strict
   preferred
   interleave
+  restrictive
 
   
 
@@ -1129,6 +1130,7 @@
 strict
 preferred
 interleave
+restrictive
   
 
 
-- 
2.25.4

[PATCH 1/5] util: fix tap device name auto-generation for FreeBSD

[Laine Stump]

The Linux implementation of virNetDevCreate() no longer requires a
template ifname (e.g. "vnet%d") when it is called, but just generates
a new name if ifname is empty. The FreeBSD implementation requires
that the caller actually fill in a template ifname, and will fail if
ifname is empty. Since we want to eliminate all the special code in
callers that is setting the template name, we need to make the
behavior of the FreeBSD virNetDevCreate() match the behavior of the
Linux virNetDevCreate().

The simplest way to do this is to use the new virNetDevGenerateName()
function - if ifname is empty it generates a new name with the proper
prefix, and if it's not empty, it leaves it alone.

Signed-off-by: Laine Stump 
---
 src/util/virnetdevtap.c | 35 ++-
 1 file changed, 6 insertions(+), 29 deletions(-)

diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
index 88ad321627..cca2f614fe 100644
--- a/src/util/virnetdevtap.c
+++ b/src/util/virnetdevtap.c
@@ -308,7 +308,6 @@ int virNetDevTapCreate(char **ifname,
 int s;
 struct ifreq ifr;
 int ret = -1;
-char *newifname = NULL;
 
 if (tapfdSize > 1) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -316,6 +315,12 @@ int virNetDevTapCreate(char **ifname,
 goto cleanup;
 }
 
+/* auto-generate an unused name for the new device (this
+ * is NOP if a name has been provided)
+ */
+if (virNetDevGenerateName(ifname, VIR_NET_DEV_GEN_NAME_VNET) < 0)
+return -1;
+
 /* As FreeBSD determines interface type by name,
  * we have to create 'tap' interface first and
  * then rename it to 'vnet'
@@ -329,34 +334,6 @@ int virNetDevTapCreate(char **ifname,
 goto cleanup;
 }
 
-/* In case we were given exact interface name (e.g. 'vnetN'),
- * we just rename to it. If we have format string like
- * 'vnet%d', we need to find the first available name that
- * matches this pattern
- */
-if (strstr(*ifname, "%d") != NULL) {
-size_t i;
-for (i = 0; i <= IF_MAXUNIT; i++) {
-g_autofree char *newname = NULL;
-
-newname = g_strdup_printf(*ifname, i);
-
-if (virNetDevExists(newname) == 0) {
-newifname = g_steal_pointer(&newname);
-break;
-}
-}
-if (newifname) {
-VIR_FREE(*ifname);
-*ifname = newifname;
-} else {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("Failed to generate new name for interface %s"),
-   ifr.ifr_name);
-goto cleanup;
-}
-}
-
 if (tapfd) {
 g_autofree char *dev_path = NULL;
 dev_path = g_strdup_printf("/dev/%s", ifr.ifr_name);
-- 
2.28.0

[PATCH 4/5] util: simplify virNetDevMacVLanCreateWithVPortProfile()

[Laine Stump]

Since commit 282d135ddbb the parser for  has cleared out
any interface name from the input XML that used the macvtap/macvlan
name as a prefix. Along with that, the switch to use the new
virNetDevGenerateName() function for auto-generating macvtap/macvlan
device names (commit 9b5d741a9), has realized two facts:

1) virNetDevGenerateName() can be called with a name already filled
   in, and in that case it is an effective NOP.

2) because virNetDevGenerate() will always find an unused name, there
   is no need to retry device creation in a loop - if it fails the
   first time, it would fail any subsequent time as well.

that, combined with the aforementioned parser change allow us to
simplify virNetDevMacVLanCreateWithVPortProfile() - we no longer need
any extra code to determine if a template "AutoName" was requested,
and don't need a separate code path for creating the device in the
case that a specific name was given in the XML - all we need to do is
log any requested name, and then call exactly the same code as we
would if no name was given.

Signed-off-by: Laine Stump 
---
 src/util/virnetdevmacvlan.c | 64 ++---
 1 file changed, 10 insertions(+), 54 deletions(-)

diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index a4ad698335..2deefe6589 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -673,6 +673,7 @@ virNetDevMacVLanCreateWithVPortProfile(const char 
*ifnameRequested,
 uint32_t macvtapMode;
 int vf = -1;
 bool vnet_hdr = flags & VIR_NETDEV_MACVLAN_VNET_HDR;
+virNetDevGenNameType type;
 
 macvtapMode = modeMap[mode];
 
@@ -706,66 +707,21 @@ virNetDevMacVLanCreateWithVPortProfile(const char 
*ifnameRequested,
 }
 
 if (ifnameRequested) {
-int rc;
-bool isAutoName
-= (STRPREFIX(ifnameRequested, VIR_NET_GENERATED_MACVTAP_PREFIX) ||
-   STRPREFIX(ifnameRequested, VIR_NET_GENERATED_MACVLAN_PREFIX));
-
 VIR_INFO("Requested macvtap device name: %s", ifnameRequested);
-
-if ((rc = virNetDevExists(ifnameRequested)) < 0)
-return -1;
-
-if (rc) {
-/* ifnameRequested is already being used */
-
-if (!isAutoName) {
-virReportSystemError(EEXIST,
- _("Unable to create device '%s'"),
- ifnameRequested);
-return -1;
-}
-} else {
-
-/* ifnameRequested is available. try to open it */
-
-virNetDevReserveName(ifnameRequested);
-
-if (virNetDevMacVLanCreate(ifnameRequested, macaddress,
-   linkdev, macvtapMode, flags) == 0) {
-
-/* virNetDevMacVLanCreate() was successful - use this name */
-ifname = g_strdup(ifnameRequested);
-
-} else if (!isAutoName) {
-/* couldn't open ifnameRequested, but it wasn't an
- * autogenerated named, so there is nothing else to
- * try - fail and return.
- */
-return -1;
-}
-}
+ifname = g_strdup(ifnameRequested);
 }
 
-if (!ifname) {
-/* ifnameRequested was NULL, or it was an already in use
- * autogenerated name, so now we look for an unused
- * autogenerated name.
- */
-virNetDevGenNameType type;
-if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP)
-type = VIR_NET_DEV_GEN_NAME_MACVTAP;
-else
-type = VIR_NET_DEV_GEN_NAME_MACVLAN;
+if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP)
+type = VIR_NET_DEV_GEN_NAME_MACVTAP;
+else
+type = VIR_NET_DEV_GEN_NAME_MACVLAN;
 
-if (virNetDevGenerateName(&ifname, type) < 0 ||
-virNetDevMacVLanCreate(ifname, macaddress,
-   linkdev, macvtapMode, flags) < 0)
-return -1;
+if (virNetDevGenerateName(&ifname, type) < 0 ||
+virNetDevMacVLanCreate(ifname, macaddress,
+   linkdev, macvtapMode, flags) < 0) {
+return -1;
 }
 
-/* all done creating the device */
-
 if (virNetDevVPortProfileAssociate(ifname,
virtPortProfile,
macaddress,
-- 
2.28.0

[PATCH 5/5] util: minor comment/formatting changes to virNetDevTapCreate()

[Laine Stump]

The comment about auto-generating names was obsoleted by recent
changes, and there was an unnecessary set of braces around a single
line conditional body.

Signed-off-by: Laine Stump 
---
 src/util/virnetdevtap.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
index cca2f614fe..8649978e53 100644
--- a/src/util/virnetdevtap.c
+++ b/src/util/virnetdevtap.c
@@ -174,15 +174,14 @@ int virNetDevTapCreate(char **ifname,
 int ret = -1;
 int fd = -1;
 
-/* if ifname is "vnet%d", then auto-generate a name for the new
+/* if ifname is empty, then auto-generate a name for the new
  * device (the kernel could do this for us, but has a bad habit of
  * immediately re-using names that have just been released, which
- * can lead to race conditions).
- * if ifname is just a user-provided name, virNetDevGenerateName
- * leaves it unchanged. */
-if (virNetDevGenerateName(ifname, VIR_NET_DEV_GEN_NAME_VNET) < 0) {
+ * can lead to race conditions).  if ifname is just a
+ * user-provided name, virNetDevGenerateName leaves it
+ * unchanged. */
+if (virNetDevGenerateName(ifname, VIR_NET_DEV_GEN_NAME_VNET) < 0)
 return -1;
-}
 
 if (!tunpath)
 tunpath = "/dev/net/tun";
-- 
2.28.0

[PATCH 2/5] bhyve: remove redundant code that adds "template" netdev name

[Laine Stump]

The FreeBSD version of virNetDevTapCreate() now calls
virNetDevGenerateName(), and virNetDevGenerateName() understands that
a blank ifname should be replaced with a generated name based on a
device-type-specific template - so there is no longer any need for the
higher level functions to stuff a template name ("vnet%d") into
ifname.

Signed-off-by: Laine Stump 
---
 src/bhyve/bhyve_command.c | 7 ---
 1 file changed, 7 deletions(-)

diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index 4cf98c0eb1..daf313c9c1 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -79,13 +79,6 @@ bhyveBuildNetArgStr(const virDomainDef *def,
 goto cleanup;
 }
 
-if (!net->ifname ||
-STRPREFIX(net->ifname, VIR_NET_GENERATED_VNET_PREFIX) ||
-strchr(net->ifname, '%')) {
-VIR_FREE(net->ifname);
-net->ifname = g_strdup(VIR_NET_GENERATED_VNET_PREFIX "%d");
-}
-
 if (!dryRun) {
 if (virNetDevTapCreateInBridgePort(brname, &net->ifname, &net->mac,
def->uuid, NULL, NULL, 0,
-- 
2.28.0

[PATCH 0/5] Followup to virNetDevGenerateName() patches

[Laine Stump]

A few issues came up during review of this series that were better
fixed in cleanups rather than requiring yet another round of
review. (A couple of things I noticed after the other patches were
already pushed).

Laine Stump (5):
  util: fix tap device name auto-generation for FreeBSD
  bhyve: remove redundant code that adds "template" netdev name
  qemu: remove redundant code that adds "template" netdev name
  util: simplify virNetDevMacVLanCreateWithVPortProfile()
  util: minor comment/formatting changes to virNetDevTapCreate()

 src/bhyve/bhyve_command.c   |  7 
 src/qemu/qemu_interface.c   | 18 +++
 src/util/virnetdevmacvlan.c | 64 ++---
 src/util/virnetdevtap.c | 46 +++---
 4 files changed, 25 insertions(+), 110 deletions(-)

-- 
2.28.0

[PATCH 3/5] qemu: remove redundant code that adds "template" netdev name

[Laine Stump]

The lower level function virNetDevGenerateName() now understands that
a blank ifname should be replaced with a generated name based on a
template that it knows about itself - there is no need for the higher
level functions to stuff a template name ("vnet%d") into ifname.

Signed-off-by: Laine Stump 
---
 src/qemu/qemu_interface.c | 18 --
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
index 197c0aa239..be2f53945c 100644
--- a/src/qemu/qemu_interface.c
+++ b/src/qemu/qemu_interface.c
@@ -455,14 +455,10 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def,
 goto cleanup;
 }
 } else {
-if (!net->ifname ||
-STRPREFIX(net->ifname, VIR_NET_GENERATED_VNET_PREFIX) ||
-strchr(net->ifname, '%')) {
-VIR_FREE(net->ifname);
-net->ifname = g_strdup(VIR_NET_GENERATED_VNET_PREFIX "%d");
-/* avoid exposing vnet%d in getXMLDesc or error outputs */
+
+if (!net->ifname)
 template_ifname = true;
-}
+
 if (virNetDevTapCreate(&net->ifname, tunpath, tapfd, tapfdSize,
tap_create_flags) < 0) {
 goto cleanup;
@@ -559,14 +555,8 @@ qemuInterfaceBridgeConnect(virDomainDefPtr def,
 goto cleanup;
 }
 
-if (!net->ifname ||
-STRPREFIX(net->ifname, VIR_NET_GENERATED_VNET_PREFIX) ||
-strchr(net->ifname, '%')) {
-VIR_FREE(net->ifname);
-net->ifname = g_strdup(VIR_NET_GENERATED_VNET_PREFIX "%d");
-/* avoid exposing vnet%d in getXMLDesc or error outputs */
+if (!net->ifname)
 template_ifname = true;
-}
 
 if (qemuInterfaceIsVnetCompatModel(net))
 tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR;
-- 
2.28.0

Re: [PATCHv3 5/5] netdev: Prevent functions that call virNetDevTapCreate from adding 'vnet%d' into ifname

[Laine Stump]

(NB: As I looked further into the details of this, I decided it would be 
simpler for me to just post a few patches to take the place of this one 
patch. I've left in the reasoning for a couple of those patches which I 
had typed before I made the decision to just make a new series, but this 
patch can just be dropped, as the series I just sent here:


https://www.redhat.com/archives/libvir-list/2020-December/msg00744.html

takes care of everything done here, plus some other things.)

On 12/13/20 8:50 PM, Shi Lei wrote:

Those functions that call virNetDevTapCreate don't need to adding
'vnet%d' into ifname when it is empty, since virNetDevGenerateName
which is in virNetDevTapCreate can deal with it.

Signed-off-by: Shi Lei 
---
  src/bhyve/bhyve_command.c |  1 -
  src/qemu/qemu_interface.c | 12 
  2 files changed, 13 deletions(-)

diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index 4cf98c0e..92b31a6e 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -83,7 +83,6 @@ bhyveBuildNetArgStr(const virDomainDef *def,
  STRPREFIX(net->ifname, VIR_NET_GENERATED_VNET_PREFIX) ||
  strchr(net->ifname, '%')) {
  VIR_FREE(net->ifname);
-net->ifname = g_strdup(VIR_NET_GENERATED_VNET_PREFIX "%d");
  }



Actually

1) this entire clause can be eliminated - from the start if the "if (" 
to the "}" just before my comment - it is a remnant of code added all 
the way back in 2007  (commit a8977b62ba), but we have been doing the 
same clearing of "vnetN" generated names in the parser itself since 
commit 282d135d (2008), and this was made more complete/consistent in 
commit 77f72a861 (August 2019) which gives more details of the history 
(in case you're interested, which you probably aren't, and shouldn't be 
:-)). This is in Patch 2 of the patchset linked above.


2) Hmm. But I just noticed that in the case of the virNetDevCreateTap() 
used by FreeBSD/OpenBSD, we actually had forgotten to switch it to use 
virNetDevGenerateName() in your patches! I've remedied that in Patch 1 
of the new patchset I linked above).


  
  if (!dryRun) {

diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
index 197c0aa2..87cfb8fc 100644
--- a/src/qemu/qemu_interface.c
+++ b/src/qemu/qemu_interface.c
@@ -413,7 +413,6 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def,
  virMacAddr tapmac;
  int ret = -1;
  unsigned int tap_create_flags = VIR_NETDEV_TAP_CREATE_IFUP;
-bool template_ifname = false;


This bool was added by commit 2b1f67d418 from 2009, and is used to clear 
out the autogenerated ifname in case of failure. We do want to preserve 
that behavior (in order to avoid some unwanted "cleaning up what isn't 
there" during failure), so while we can get rid of everything about 
adding VIR_NET_GENERATED_VNET_PREFIX + "%d", we need to remember that we 
generated this name, and clear it during a failure exit from the 
function (see Patch 3 of my new patchset)



  g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
  const char *tunpath = "/dev/net/tun";
  const char *auditdev = tunpath;
@@ -459,9 +458,6 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def,
  STRPREFIX(net->ifname, VIR_NET_GENERATED_VNET_PREFIX) ||
  strchr(net->ifname, '%')) {
  VIR_FREE(net->ifname);
-net->ifname = g_strdup(VIR_NET_GENERATED_VNET_PREFIX "%d");
-/* avoid exposing vnet%d in getXMLDesc or error outputs */
-template_ifname = true;
  }
  if (virNetDevTapCreate(&net->ifname, tunpath, tapfd, tapfdSize,
 tap_create_flags) < 0) {
@@ -512,8 +508,6 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def,
  virDomainAuditNetDevice(def, net, auditdev, false);
  for (i = 0; i < tapfdSize && tapfd[i] >= 0; i++)
  VIR_FORCE_CLOSE(tapfd[i]);
-if (template_ifname)
-VIR_FREE(net->ifname);
  }
  
  return ret;

@@ -541,7 +535,6 @@ qemuInterfaceBridgeConnect(virDomainDefPtr def,
  const char *brname;
  int ret = -1;
  unsigned int tap_create_flags = VIR_NETDEV_TAP_CREATE_IFUP;
-bool template_ifname = false;
  g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
  const char *tunpath = "/dev/net/tun";
  
@@ -563,9 +556,6 @@ qemuInterfaceBridgeConnect(virDomainDefPtr def,

  STRPREFIX(net->ifname, VIR_NET_GENERATED_VNET_PREFIX) ||
  strchr(net->ifname, '%')) {
  VIR_FREE(net->ifname);
-net->ifname = g_strdup(VIR_NET_GENERATED_VNET_PREFIX "%d");
-/* avoid exposing vnet%d in getXMLDesc or error outputs */
-template_ifname = true;
  }
  
  if (qemuInterfaceIsVnetCompatModel(net))

@@ -630,8 +620,6 @@ qemuInterfaceBridgeConnect(virDomainDefPtr def,
  size_t i;
  for (i = 0; i < *tapfdSize && tapfd[i] >= 0; i++)
  VIR_FORCE_CLOSE(tapfd[i]);
- 

Re: [PATCHv3 0/5] netdev: Extract GenerateName/ReserveName as common functions

[Laine Stump]

On 12/13/20 8:50 PM, Shi Lei wrote:

V2 here: https://www.redhat.com/archives/libvir-list/2020-December/msg00563.html

Since V2:
  *  Fix libxl driver for missing changing virNetDevMacVLanReserveName

V1 here: https://www.redhat.com/archives/libvir-list/2020-December/msg00308.html

Since V1:
 (1) Remove virNetDev[Lock|Unlock]GenName.
 Only *lastID* needs to be protected. Now we lock *lastID*
 inside virNetDevReserveName and virNetDevGenerateName, then lock and
 unlock functions are no longer needed.

 (2) Shorten the locking range for generating names for tap and macvlan.
 Since virNetDevReserveName and virNetDevGenerateName are now with lock,
 we can call them directly rather than adding lock outside.

 (3) Rename *_GEN_NAME_TAP to *_GEN_NAME_VNET.

 (4) Now veth and tap share the same prefix "vnet".

 (5) Use name rather than type as the argument of virNetDevReserveName.
 Just follow the style of virNetDev[Tap|MacVlan]ReserveName.

 (6) Remove those in-between functions for tap and macvlan.

 (7) Remove useless ENUM_[DECL|IMPL] for enum virNetDevGenNameType.

 (8) Remove the *_NONE item for enum virNetDevGenNameType.

 (9) Remove useless  in virnetdevtap.

 (10) When @ifname of virNetDevGenerateName is NOT a template or NULL,
 just leave it unchanged.

 (11) Take advantage of the "g_strdup_printf(*ifname, id)" in
 virNetDevGenerateName, prevent the functions that call 
virNetDevTapCreate()
 from adding in "vnet%d" when ifname is empty.

 (12) Use VIR_NETDEV_MACVLAN_CREATE_WITH_TAP to distinguish macvtap and
 macvlan and remove those useless macros.



Thanks for working through all of those.


For Patches 1-4:

Reviewed-by: Laine Stump 


and pushed. Thanks for your contribution!


For Patch 5 - it turned out simpler to just write a short series of 
patches to replace that, which I posted as 
https://www.redhat.com/archives/libvir-list/2020-December/msg00744.html






Shi Lei (5):
   netdev: Introduce several helper functions for generating unique netdev name
   netdevtap: Use common helper function to create unique tap name
   netdevmacvlan: Use helper function to create unique macvlan/macvtap name
   netdevveth: Simplify virNetDevVethCreate by using virNetDevGenerateName
   netdev: Prevent functions that call virNetDevTapCreate from adding 'vnet%d' 
into ifname

  src/bhyve/bhyve_command.c  |   3 +-
  src/conf/domain_conf.c |   4 +-
  src/interface/interface_backend_udev.c |   2 +-
  src/libvirt_private.syms   |   4 +-
  src/libxl/libxl_driver.c   |   2 +-
  src/lxc/lxc_process.c  |   5 +-
  src/qemu/qemu_interface.c  |  16 +--
  src/qemu/qemu_process.c|   4 +-
  src/util/virnetdev.c   | 116 
  src/util/virnetdev.h   |  27 +++-
  src/util/virnetdevmacvlan.c| 177 +++--
  src/util/virnetdevmacvlan.h|  14 +-
  src/util/virnetdevtap.c| 100 +-
  src/util/virnetdevtap.h|   4 -
  src/util/virnetdevveth.c   | 140 +--
  15 files changed, 218 insertions(+), 400 deletions(-)

[PATCHv3 0/3] Use netlink to create veth device pair when netlink is supported

[Shi Lei]

V2 here: https://www.redhat.com/archives/libvir-list/2020-November/msg01239.html
Since V2:
* Remove the argument 'status' of virNetDevVethCreateInternal
* fix a memory leak in virLXCProcessSetupInterfaceTap

V1 here: https://www.redhat.com/archives/libvir-list/2020-November/msg00898.html
Since V1:
* Include  rather than introducing enum VETH_INFO_*
* Have complete functions within an #ifdefs rather than
  putting #ifdefs in function

Shi Lei (3):
  util:netlink: Enable virNetlinkNewLink to support veth
  util:veth: Create veth device pair by netlink
  lxc: fix a memory leak

 src/lxc/lxc_process.c|   4 +-
 src/util/virnetdevveth.c | 126 ++-
 src/util/virnetlink.c|  14 +
 src/util/virnetlink.h|   1 +
 4 files changed, 89 insertions(+), 56 deletions(-)

-- 
2.25.1

[PATCHv3 2/3] util:veth: Create veth device pair by netlink

[Shi Lei]

When netlink is supported, use netlink to create veth device pair
rather than 'ip link' command.

Signed-off-by: Shi Lei 
---
 src/util/virnetdevveth.c | 126 ++-
 1 file changed, 72 insertions(+), 54 deletions(-)

diff --git a/src/util/virnetdevveth.c b/src/util/virnetdevveth.c
index 194f595a..7133af44 100644
--- a/src/util/virnetdevveth.c
+++ b/src/util/virnetdevveth.c
@@ -27,21 +27,69 @@
 #include "virfile.h"
 #include "virstring.h"
 #include "virnetdev.h"
+#include "virnetlink.h"
 
 #define VIR_FROM_THIS VIR_FROM_NONE
 
 VIR_LOG_INIT("util.netdevveth");
 
 
+#if defined(WITH_LIBNL)
+static int
+virNetDevVethCreateInternal(const char *veth1, const char *veth2)
+{
+int status; /* Just ignore it */
+virNetlinkNewLinkData data = { .veth_peer = veth2 };
+
+return virNetlinkNewLink(veth1, "veth", &data, &status);
+}
+
+static int
+virNetDevVethDeleteInternal(const char *veth)
+{
+return virNetlinkDelLink(veth, NULL);
+}
+#else
+static int
+virNetDevVethCreateInternal(const char *veth1, const char *veth2)
+{
+g_autoptr(virCommand) cmd = virCommandNew("ip");
+virCommandAddArgList(cmd, "link", "add", veth1, "type", "veth",
+ "peer", "name", veth2, NULL);
+
+return virCommandRun(cmd, NULL);
+}
+
+static int
+virNetDevVethDeleteInternal(const char *veth)
+{
+int status;
+g_autoptr(virCommand) cmd = virCommandNewArgList("ip", "link",
+ "del", veth, NULL);
+
+if (virCommandRun(cmd, &status) < 0)
+return -1;
+
+if (status != 0) {
+if (!virNetDevExists(veth)) {
+VIR_DEBUG("Device %s already deleted (by kernel namespace 
cleanup)", veth);
+return 0;
+}
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Failed to delete veth device %s"), veth);
+return -1;
+}
+
+return 0;
+}
+#endif /* WITH_LIBNL */
+
 /**
  * virNetDevVethCreate:
- * @veth1: pointer to name for parent end of veth pair
- * @veth2: pointer to return name for container end of veth pair
+ * @veth1: pointer to name for one end of veth pair
+ * @veth2: pointer to name for another end of veth pair
  *
- * Creates a veth device pair using the ip command:
- * ip link add veth1 type veth peer name veth2
- * If veth1 points to NULL on entry, it will be a valid interface on
- * return.  veth2 should point to NULL on entry.
+ * Creates a veth device pair.
  *
  * NOTE: If veth1 and veth2 names are not specified, ip will auto assign
  *   names.  There seems to be two problems here -
@@ -58,44 +106,31 @@ VIR_LOG_INIT("util.netdevveth");
  *
  * Returns 0 on success or -1 in case of error
  */
-int virNetDevVethCreate(char** veth1, char** veth2)
+int virNetDevVethCreate(char **veth1, char **veth2)
 {
-int status;
-g_autofree char *veth1auto = NULL;
-g_autofree char *veth2auto = NULL;
-g_autoptr(virCommand) cmd = NULL;
+const char *orig1 = *veth1;
+const char *orig2 = *veth2;
 
-if (virNetDevGenerateName(&veth1auto, VIR_NET_DEV_GEN_NAME_VNET) < 0)
-return -1;
+if (virNetDevGenerateName(veth1, VIR_NET_DEV_GEN_NAME_VNET) < 0)
+goto cleanup;
 
-if (virNetDevGenerateName(&veth2auto, VIR_NET_DEV_GEN_NAME_VNET) < 0)
-return -1;
+if (virNetDevGenerateName(veth2, VIR_NET_DEV_GEN_NAME_VNET) < 0)
+goto cleanup;
 
-cmd = virCommandNew("ip");
-virCommandAddArgList(cmd, "link", "add",
- *veth1 ? *veth1 : veth1auto,
- "type", "veth", "peer", "name",
- *veth2 ? *veth2 : veth2auto,
- NULL);
+if (virNetDevVethCreateInternal(*veth1, *veth2) < 0)
+goto cleanup;
 
-if (virCommandRun(cmd, &status) < 0 || status) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   "%s", _("Failed to allocate free veth pair"));
-return -1;
-}
+VIR_DEBUG("Create Host: %s guest: %s", *veth1, *veth2);
+return 0;
 
-VIR_DEBUG("create veth host: %s guest: %s: %d",
-  *veth1 ? *veth1 : veth1auto,
-  *veth2 ? *veth2 : veth2auto,
-  status);
+ cleanup:
+if (orig1 == NULL)
+VIR_FREE(*veth1);
 
-if (veth1auto)
-*veth1 = g_steal_pointer(&veth1auto);
-if (veth2auto)
-*veth2 = g_steal_pointer(&veth2auto);
+if (orig2 == NULL)
+VIR_FREE(*veth2);
 
-VIR_DEBUG("Create Host: %s guest: %s", *veth1, *veth2);
-return 0;
+return -1;
 }
 
 /**
@@ -111,22 +146,5 @@ int virNetDevVethCreate(char** veth1, char** veth2)
  */
 int virNetDevVethDelete(const char *veth)
 {
-int status;
-g_autoptr(virCommand) cmd = virCommandNewArgList("ip", "link",
-   "del", veth, NULL);
-
-if (virCommandRun(cmd, &status) < 0)
-return -1;
-
-if (status != 0) {
-if (!virNetDevExists(veth)) {
-

[PATCHv3 3/3] lxc: fix a memory leak

[Shi Lei]

In virLXCProcessSetupInterfaceTap, containerVeth needs to be freed on
failure.

Signed-off-by: Shi Lei 
---
 src/lxc/lxc_process.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 85d0287a..0f7c9295 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -303,7 +303,7 @@ virLXCProcessSetupInterfaceTap(virDomainDefPtr vm,
const char *brname)
 {
 char *parentVeth;
-char *containerVeth = NULL;
+g_autofree char *containerVeth = NULL;
 const virNetDevVPortProfile *vport = 
virDomainNetGetActualVirtPortProfile(net);
 
 VIR_DEBUG("calling vethCreate()");
@@ -357,7 +357,7 @@ virLXCProcessSetupInterfaceTap(virDomainDefPtr vm,
 virDomainConfNWFilterInstantiate(vm->name, vm->uuid, net, false) < 0)
 return NULL;
 
-return containerVeth;
+return g_steal_pointer(&containerVeth);
 }
 
 
-- 
2.25.1

[PATCHv3 1/3] util:netlink: Enable virNetlinkNewLink to support veth

[Shi Lei]

Signed-off-by: Shi Lei 
---
 src/util/virnetlink.c | 14 ++
 src/util/virnetlink.h |  1 +
 2 files changed, 15 insertions(+)

diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
index fdd3a6a4..8625b896 100644
--- a/src/util/virnetlink.c
+++ b/src/util/virnetlink.c
@@ -24,6 +24,7 @@
 #include 
 
 #include 
+#include 
 
 #include "virnetlink.h"
 #include "virnetdev.h"
@@ -535,6 +536,19 @@ virNetlinkNewLink(const char *ifname,
 NETLINK_MSG_NEST_END(nl_msg, infodata);
 }
 
+if (STREQ(type, "veth") && extra_args && extra_args->veth_peer) {
+struct nlattr *infoveth = NULL;
+
+NETLINK_MSG_NEST_START(nl_msg, infodata, IFLA_INFO_DATA);
+NETLINK_MSG_NEST_START(nl_msg, infoveth, VETH_INFO_PEER);
+nlmsg_reserve(nl_msg, sizeof(struct ifinfomsg), 0);
+NETLINK_MSG_PUT(nl_msg, IFLA_IFNAME,
+(strlen(extra_args->veth_peer) + 1),
+extra_args->veth_peer);
+NETLINK_MSG_NEST_END(nl_msg, infoveth);
+NETLINK_MSG_NEST_END(nl_msg, infodata);
+}
+
 NETLINK_MSG_NEST_END(nl_msg, linkinfo);
 
 if (extra_args) {
diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h
index 7121eac4..7c4ed202 100644
--- a/src/util/virnetlink.h
+++ b/src/util/virnetlink.h
@@ -84,6 +84,7 @@ struct _virNetlinkNewLinkData {
 const int *ifindex; /* The index for the 'link' device */
 const virMacAddr *mac;  /* The MAC address of the device */
 const uint32_t *macvlan_mode;   /* The mode of macvlan */
+const char *veth_peer;  /* The peer name for veth */
 };
 
 int virNetlinkNewLink(const char *ifname,
-- 
2.25.1