Re: [PATCH 1/7] conf: Add NFS disk protocol

[Han Han]

Generally, for the patch series, it is better to be sent with a cover
letter to describe the content
and the background of the patch series.
Since this is your second version of the nfs disk, please mention the patch
version in the prefix of titles, like:
[PATCH v2 1/7]...
That could be done for all patches with the "-v2" option of git-send-email.

BTW, mention the changes from the previous version in the cover letter.
You can refer to
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

On Wed, Dec 30, 2020 at 5:24 AM Ryan Gahagan  wrote:

> Per Issue 90, Libvirt does not support attaching an NFS disk even though
> QEMU has added support for it. This series of patches seeks to implement
> this support in Libvirt and begins by adding in flags for an NFS disk.
>
> Signed-off-by: Ryan Gahagan 
> ---
>  src/libxl/libxl_conf.c| 1 +
>  src/libxl/xen_xl.c| 1 +
>  src/qemu/qemu_block.c | 3 +++
>  src/qemu/qemu_command.c   | 1 +
>  src/qemu/qemu_domain.c| 2 ++
>  src/qemu/qemu_snapshot.c  | 3 +++
>  src/util/virstoragefile.c | 6 ++
>  src/util/virstoragefile.h | 1 +
>  8 files changed, 18 insertions(+)
>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 00748e21e8..6a8ae27f54 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -941,6 +941,7 @@ libxlMakeNetworkDiskSrcStr(virStorageSourcePtr src,
>  case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG:
>  case VIR_STORAGE_NET_PROTOCOL_SSH:
>  case VIR_STORAGE_NET_PROTOCOL_VXHS:
> +case VIR_STORAGE_NET_PROTOCOL_NFS:
>  case VIR_STORAGE_NET_PROTOCOL_LAST:
>  case VIR_STORAGE_NET_PROTOCOL_NONE:
>  virReportError(VIR_ERR_NO_SUPPORT,
> diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c
> index ba0942601f..17b93d0f5c 100644
> --- a/src/libxl/xen_xl.c
> +++ b/src/libxl/xen_xl.c
> @@ -1600,6 +1600,7 @@ xenFormatXLDiskSrcNet(virStorageSourcePtr src)
>  case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG:
>  case VIR_STORAGE_NET_PROTOCOL_SSH:
>  case VIR_STORAGE_NET_PROTOCOL_VXHS:
> +case VIR_STORAGE_NET_PROTOCOL_NFS:
>  case VIR_STORAGE_NET_PROTOCOL_LAST:
>  case VIR_STORAGE_NET_PROTOCOL_NONE:
>  virReportError(VIR_ERR_NO_SUPPORT,
> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> index 4640e339c0..b224a550f3 100644
> --- a/src/qemu/qemu_block.c
> +++ b/src/qemu/qemu_block.c
> @@ -1180,6 +1180,7 @@
> qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src,
>  return NULL;
>  break;
>
> +case VIR_STORAGE_NET_PROTOCOL_NFS:
>  case VIR_STORAGE_NET_PROTOCOL_NONE:
>  case VIR_STORAGE_NET_PROTOCOL_LAST:
>  virReportEnumRangeError(virStorageNetProtocol, src->protocol);
> @@ -2111,6 +2112,7 @@ qemuBlockGetBackingStoreString(virStorageSourcePtr
> src,
>  case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG:
>  case VIR_STORAGE_NET_PROTOCOL_RBD:
>  case VIR_STORAGE_NET_PROTOCOL_VXHS:
> +case VIR_STORAGE_NET_PROTOCOL_NFS:
>  case VIR_STORAGE_NET_PROTOCOL_SSH:
>  case VIR_STORAGE_NET_PROTOCOL_LAST:
>  case VIR_STORAGE_NET_PROTOCOL_NONE:
> @@ -2502,6 +2504,7 @@
> qemuBlockStorageSourceCreateGetStorageProps(virStorageSourcePtr src,
>  case VIR_STORAGE_NET_PROTOCOL_NBD:
>  case VIR_STORAGE_NET_PROTOCOL_ISCSI:
>  case VIR_STORAGE_NET_PROTOCOL_VXHS:
> +case VIR_STORAGE_NET_PROTOCOL_NFS:
>  case VIR_STORAGE_NET_PROTOCOL_HTTP:
>  case VIR_STORAGE_NET_PROTOCOL_HTTPS:
>  case VIR_STORAGE_NET_PROTOCOL_FTP:
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index b06a086e18..c58f39ebf1 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1044,6 +1044,7 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src,
> _("'ssh' protocol is not yet supported"));
>  return NULL;
>
> +case VIR_STORAGE_NET_PROTOCOL_NFS:
>  case VIR_STORAGE_NET_PROTOCOL_LAST:
>  case VIR_STORAGE_NET_PROTOCOL_NONE:
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index bfb6e23942..d91c32b2c5 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -9630,6 +9630,8 @@
> qemuDomainPrepareStorageSourceTLS(virStorageSourcePtr src,
>  case VIR_STORAGE_NET_PROTOCOL_FTP:
>  case VIR_STORAGE_NET_PROTOCOL_FTPS:
>  case VIR_STORAGE_NET_PROTOCOL_TFTP:
> +case VIR_STORAGE_NET_PROTOCOL_NFS:
> +/* Assumed NFS doesn't support TLS (needs Kerberos) */
>  case VIR_STORAGE_NET_PROTOCOL_SSH:
>  if (src->haveTLS == VIR_TRISTATE_BOOL_YES) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index 15494c3415..7e89a8839b 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -413,6 +413,7 

Re: [PATCH libvirt v1] tests: add capabilities for QEMU 5.2.0 on s390x

[Andrea Bolognani]

On Mon, 2020-12-28 at 12:41 +0100, Shalini Chellathurai Saroja wrote:
> On 12/17/20 12:19 PM, Andrea Bolognani wrote:
> > On Wed, 2020-12-16 at 10:10 +0100, Shalini Chellathurai Saroja wrote:
> > > +++ b/tests/qemucapabilitiesdata/caps_5.2.0.s390x.xml
> > > @@ -0,0 +1,3300 @@
> > > +
> > > +  /usr/bin/qemu-system-s390x
> > > +  5002000
> > > +  0
> > > +  39100243
> > > +  qemu-5.2.0-20201215.0.ba93e22c.fc32
> >
> > ... the version string seems to indicate you're grabbing the replies
> > from a packaged version rather than a build made from pristine
> > upstream sources: this is consistent with what was done for earlier
> > QEMU capabilities on s390x, but not with how we usually do things for
> > other architectures - see the other caps_5.2.0.*.replies files.
> > 
> > I don't think this is a blocker, because a Fedora-based package will
> > be quite close to upstream anyway, but it would be great if you could
> > generate the replies file again against a QEMU binary that's been
> > built exclusively from upstream sources. You can then submit the
> > update as a follow-up patch - I expect such patch to be fairly small.
> 
> The replies are actually generated from the QEMU 5.2.0 binary built 
> exclusively
> from upstream. This is also true for the other s390 replies generated for
> the earlier versions of QEMU.

So how are you actually building the binary? Because if you just
clone the upstream repository and run the usual ./configure && make
inside it, the version number will not look like that... The presence
of .fc32 specifically seems to indicate a .spec file is involved in
some capacity.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [RFC PATCH 0/3] Fix "virsh domfsinfo" on s390x (again)

[Michal Privoznik]

On 1/2/21 6:53 AM, Thomas Huth wrote:

On 10/12/2020 11.41, Michal Privoznik wrote:

I'll keep these in a local branch and push after QEMU patch.


The QEMU patch has now been merged:

  https://git.qemu.org/?p=qemu.git;a=commitdiff;h=5b723a5d8df44b69

Could you now push your rebased version, or do you want me to respin a v4?


Pushed upstream as:

bf63f6549a domain_conf: Allow to look up scsi disks when controller uses 
a CCW address
5db43b5a76 domain_conf: Allow to look up virtio-block devices by their 
CCW address
f5c8cf9e0e qemu: agent: Store CCW address in qemuAgentDiskInfo if 
provided by the guest


Michal

[PATCH] qemu: Drop has_ccw_address from _qemuAgentDiskAddress

[Michal Privoznik]

In recent patches new mambers to _qemuAgentDiskAddress struct
were introduced to keep optional CCW address sent by the guest
agent. These two members are a struct to store CCW address into
and a boolean to keep track whether the CCW address is valid.
Well, we can hold the same information with a pointer - instead
of storing the CCW address structure let's keep just a pointer to
it.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_agent.c  | 18 --
 src/qemu/qemu_agent.h  |  3 +--
 src/qemu/qemu_driver.c |  9 +++--
 3 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index b155896e78..af0397e6e2 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -1824,6 +1824,7 @@ qemuAgentDiskAddressFree(qemuAgentDiskAddressPtr info)
 g_free(info->serial);
 g_free(info->bus_type);
 g_free(info->devnode);
+g_free(info->ccw_addr);
 g_free(info);
 }
 
@@ -1899,12 +1900,17 @@ qemuAgentGetDiskAddress(virJSONValuePtr json)
 GET_DISK_ADDR(pci, &addr->pci_controller.function, "function");
 
 if ((ccw = virJSONValueObjectGet(json, "ccw-address"))) {
-addr->has_ccw_address = true;
-GET_DISK_ADDR(ccw, &addr->ccw_addr.cssid, "cssid");
-if (addr->ccw_addr.cssid == 0)  /* Guest CSSID 0 is 0xfe on host */
-addr->ccw_addr.cssid = 0xfe;
-GET_DISK_ADDR(ccw, &addr->ccw_addr.ssid, "ssid");
-GET_DISK_ADDR(ccw, &addr->ccw_addr.devno, "devno");
+g_autofree virDomainDeviceCCWAddressPtr ccw_addr = NULL;
+
+ccw_addr = g_new0(virDomainDeviceCCWAddress, 1);
+
+GET_DISK_ADDR(ccw, &ccw_addr->cssid, "cssid");
+if (ccw_addr->cssid == 0)  /* Guest CSSID 0 is 0xfe on host */
+ccw_addr->cssid = 0xfe;
+GET_DISK_ADDR(ccw, &ccw_addr->ssid, "ssid");
+GET_DISK_ADDR(ccw, &ccw_addr->devno, "devno");
+
+addr->ccw_addr = g_steal_pointer(&ccw_addr);
 }
 #undef GET_DISK_ADDR
 
diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
index 4ea9b9dc1e..0d47230161 100644
--- a/src/qemu/qemu_agent.h
+++ b/src/qemu/qemu_agent.h
@@ -77,8 +77,7 @@ struct _qemuAgentDiskAddress {
 unsigned int target;
 unsigned int unit;
 char *devnode;
-bool has_ccw_address;
-virDomainDeviceCCWAddress ccw_addr;
+virDomainDeviceCCWAddressPtr ccw_addr;
 };
 void qemuAgentDiskAddressFree(qemuAgentDiskAddressPtr addr);
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuAgentDiskAddress, qemuAgentDiskAddressFree);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 62b0852c33..a376824854 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18887,8 +18887,7 @@ qemuAgentFSInfoToPublic(qemuAgentFSInfoPtr agent,
 
 diskDef = virDomainDiskByAddress(vmdef,
  &agentdisk->pci_controller,
- agentdisk->has_ccw_address ?
- &agentdisk->ccw_addr : NULL,
+ agentdisk->ccw_addr,
  agentdisk->bus,
  agentdisk->target,
  agentdisk->unit);
@@ -19931,8 +19930,7 @@ qemuAgentDiskInfoFormatParams(qemuAgentDiskInfoPtr 
*info,
 /* match the disk to the target in the vm definition */
 diskdef = virDomainDiskByAddress(vmdef,
  &info[i]->address->pci_controller,
- info[i]->address->has_ccw_address 
?
-&info[i]->address->ccw_addr : 
NULL,
+ info[i]->address->ccw_addr,
  info[i]->address->bus,
  info[i]->address->target,
  info[i]->address->unit);
@@ -20017,8 +20015,7 @@ qemuAgentFSInfoFormatParams(qemuAgentFSInfoPtr *fsinfo,
 /* match the disk to the target in the vm definition */
 diskdef = virDomainDiskByAddress(vmdef,
  &d->pci_controller,
- d->has_ccw_address ?
- &d->ccw_addr : NULL,
+ d->ccw_addr,
  d->bus,
  d->target,
  d->unit);
-- 
2.26.2

Re: [PATCH 7/7] tests: Added tests for NFS disk protocol

[Peter Krempa]

On Mon, Jan 04, 2021 at 15:53:08 +0800, Han Han wrote:
> On Wed, Dec 30, 2020 at 5:23 AM Ryan Gahagan  wrote:
> 
> > Signed-off-by: Ryan Gahagan 
> > ---

[...]

> > function='0x0'/>
> > +
> > +
> > +  
> > +  
> > +
> > +  
> > +  
> > +
> > +
> > +  
> > +  
> >
> I am curious why the uid/gid here is formatted as +NUMBER instead of the
> NUMBER itself...

The '+' prefix means that the user is explicitly an UID (numeric) and
must not be translated to an uid from a username (you can have numeric
user name which maps to a different UID).

In the tests we want to bypass the translation to preserve stable
output.

[libvirt PATCH 0/9] Merge scripts in tests/cputestdata

[Tim Wiederhake]

This series merges the functionality of cpu-cpuid.py into cpu-gather.py
and completes the transition.

For background, see
https://www.redhat.com/archives/libvir-list/2020-December/msg00706.html

Tim Wiederhake (9):
  cpu-cpuid: Use argparse to parse arguments
  cpu-cpuid: Remove xmltodict usage in parseMap
  cpu-cpuid: Remove xmltodict usage in parseCPU
  cpu-cpuid: Merge addFeature functions
  cpu-cpuid: Merge checkFeature functions
  cpu-cpuid: Deduplicate register list
  cpu-gather: Use actions instead of flags for action argument
  cpu-gather: Factor out call to cpu-cpuid.py
  cpu-gather: Merge cpu-cpuid.py

 tests/cputestdata/cpu-cpuid.py  | 229 
 tests/cputestdata/cpu-gather.py | 178 ++---
 2 files changed, 156 insertions(+), 251 deletions(-)
 delete mode 100755 tests/cputestdata/cpu-cpuid.py

-- 
2.26.2

[libvirt PATCH 3/9] cpu-cpuid: Remove xmltodict usage in parseCPU

[Tim Wiederhake]

'xmltodict' is a Python module that is not installed by default.
Replace it, so the dependencies of cpu-gather.py do not change
when both scripts are merged.

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

diff --git a/tests/cputestdata/cpu-cpuid.py b/tests/cputestdata/cpu-cpuid.py
index 8e06baea85..d570884db6 100755
--- a/tests/cputestdata/cpu-cpuid.py
+++ b/tests/cputestdata/cpu-cpuid.py
@@ -4,7 +4,6 @@ import argparse
 import os
 import sys
 import json
-import xmltodict
 import xml.etree.ElementTree
 
 
@@ -105,31 +104,19 @@ def parseQemu(path, features):
 
 
 def parseCPUData(path):
-cpuData = {}
-with open(path, "rb") as f:
-data = xmltodict.parse(f)
-
-for leaf in data["cpudata"]["cpuid"]:
-feature = {"type": "cpuid"}
-feature["eax_in"] = int(leaf["@eax_in"], 0)
-feature["ecx_in"] = int(leaf["@ecx_in"], 0)
-for reg in ["eax", "ebx", "ecx", "edx"]:
-feature[reg] = int(leaf["@" + reg], 0)
+cpuData = dict()
+for f in xml.etree.ElementTree.parse(path).getroot():
+if f.tag == "cpuid":
+reg_list = ["eax_in", "ecx_in", "eax", "ebx", "ecx", "edx"]
+elif f.tag == "msr":
+reg_list = ["index", "eax", "edx"]
+else:
+continue
 
+feature = {"type": f.tag}
+for reg in reg_list:
+feature[reg] = int(f.attrib.get(reg, "0"), 0)
 addFeature(cpuData, feature)
-
-if "msr" in data["cpudata"]:
-if not isinstance(data["cpudata"]["msr"], list):
-data["cpudata"]["msr"] = [data["cpudata"]["msr"]]
-
-for msr in data["cpudata"]["msr"]:
-feature = {"type": "msr"}
-feature["index"] = int(msr["@index"], 0)
-feature["edx"] = int(msr["@edx"], 0)
-feature["eax"] = int(msr["@eax"], 0)
-
-addFeature(cpuData, feature)
-
 return cpuData
 
 
-- 
2.26.2

[libvirt PATCH 1/9] cpu-cpuid: Use argparse to parse arguments

[Tim Wiederhake]

Using 'argparse' for argument handling simplifies merging this script
with cpu-gather.py in a later patch.

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

diff --git a/tests/cputestdata/cpu-cpuid.py b/tests/cputestdata/cpu-cpuid.py
index dac43debb6..6ca72d2262 100755
--- a/tests/cputestdata/cpu-cpuid.py
+++ b/tests/cputestdata/cpu-cpuid.py
@@ -1,5 +1,6 @@
 #!/usr/bin/env python3
 
+import argparse
 import os
 import sys
 import json
@@ -191,39 +192,46 @@ def formatCPUData(cpuData, path, comment):
 f.write("\n")
 
 
-def diff(cpuMap, path):
-base = path.replace(".json", "")
-jsonFile = path
-cpuDataFile = base + ".xml"
-enabledFile = base + "-enabled.xml"
-disabledFile = base + "-disabled.xml"
-
-cpuData = parseCPUData(cpuDataFile)
-qemu = parseQemu(jsonFile, cpuMap)
-
-enabled = {"cpuid": {}}
-disabled = {"cpuid": {}}
-for feature in cpuMap.values():
-if checkFeature(qemu, feature):
-addFeature(enabled, feature)
-elif checkFeature(cpuData, feature):
-addFeature(disabled, feature)
-
-formatCPUData(enabled, enabledFile, "Features enabled by QEMU")
-formatCPUData(disabled, disabledFile, "Features disabled by QEMU")
-
-
-if len(sys.argv) < 3:
-print("Usage: %s diff json_file..." % sys.argv[0])
-sys.exit(1)
-
-action = sys.argv[1]
-args = sys.argv[2:]
-
-if action == "diff":
+def diff(args):
 cpuMap = parseMap()
-for path in args:
-diff(cpuMap, path)
-else:
-print("Unknown action: %s" % action)
-sys.exit(1)
+
+for jsonFile in args.json_files:
+cpuDataFile = jsonFile.replace(".json", ".xml")
+enabledFile = jsonFile.replace(".json", "-enabled.xml")
+disabledFile = jsonFile.replace(".json", "-disabled.xml")
+
+cpuData = parseCPUData(cpuDataFile)
+qemu = parseQemu(jsonFile, cpuMap)
+
+enabled = dict()
+disabled = dict()
+for feature in cpuMap.values():
+if checkFeature(qemu, feature):
+addFeature(enabled, feature)
+elif checkFeature(cpuData, feature):
+addFeature(disabled, feature)
+
+formatCPUData(enabled, enabledFile, "Features enabled by QEMU")
+formatCPUData(disabled, disabledFile, "Features disabled by QEMU")
+
+
+def main():
+parser = argparse.ArgumentParser(description="Diff cpuid results")
+subparsers = parser.add_subparsers(dest="action", required=True)
+diffparser = subparsers.add_parser(
+"diff",
+help="Diff json description of CPU model against known features.")
+diffparser.add_argument(
+"json_files",
+nargs="+",
+metavar="FILE",
+type=os.path.realpath,
+help="Path to one or more json CPU model descriptions.")
+args = parser.parse_args()
+
+diff(args)
+exit(0)
+
+
+if __name__ == "__main__":
+main()
-- 
2.26.2

[libvirt PATCH 6/9] cpu-cpuid: Deduplicate register list

[Tim Wiederhake]

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

diff --git a/tests/cputestdata/cpu-cpuid.py b/tests/cputestdata/cpu-cpuid.py
index e90165e563..3e852d5d55 100755
--- a/tests/cputestdata/cpu-cpuid.py
+++ b/tests/cputestdata/cpu-cpuid.py
@@ -7,47 +7,36 @@ import json
 import xml.etree.ElementTree
 
 
+_KEYS = {
+"cpuid": ["eax_in", "ecx_in"],
+"msr": ["index"],
+}
+
+_REGS = {
+"cpuid": ["eax", "ebx", "ecx", "edx"],
+"msr": ["eax", "edx"],
+}
+
+
 def checkFeature(cpuData, feature):
-if feature["type"] == "cpuid":
-# cpuData["cpuid"][eax_in][ecx_in] = {eax:, ebx:, ecx:, edx:}
-keyList = ["type", "eax_in", "ecx_in"]
-regList = ["eax", "ebx", "ecx", "edx"]
-elif feature["type"] == "msr":
-# cpuData["msr"][index] = {eax:, edx:}
-keyList = ["type", "index"]
-regList = ["eax", "edx"]
-else:
-return False
-
-for key in keyList:
+for key in ["type"] + _KEYS.get(feature["type"], list()):
 if feature[key] not in cpuData:
 return False
 cpuData = cpuData[feature[key]]
 
-for reg in regList:
+for reg in _REGS.get(feature["type"], list()):
 if feature[reg] > 0 and feature[reg] == feature[reg] & cpuData[reg]:
 return True
 return False
 
 
 def addFeature(cpuData, feature):
-if feature["type"] == "cpuid":
-# cpuData["cpuid"][eax_in][ecx_in] = {eax:, ebx:, ecx:, edx:}
-keyList = ["type", "eax_in", "ecx_in"]
-regList = ["eax", "ebx", "ecx", "edx"]
-elif feature["type"] == "msr":
-# cpuData["msr"][index] = {eax:, edx:}
-keyList = ["type", "index"]
-regList = ["eax", "edx"]
-else:
-return
-
-for key in keyList:
+for key in ["type"] + _KEYS.get(feature["type"], list()):
 if feature[key] not in cpuData:
 cpuData[feature[key]] = dict()
 cpuData = cpuData[feature[key]]
 
-for reg in regList:
+for reg in _REGS.get(feature["type"], list()):
 cpuData[reg] = cpuData.get(reg, 0) | feature[reg]
 
 
@@ -66,15 +55,11 @@ def parseQemu(path, features):
 def parseCPUData(path):
 cpuData = dict()
 for f in xml.etree.ElementTree.parse(path).getroot():
-if f.tag == "cpuid":
-reg_list = ["eax_in", "ecx_in", "eax", "ebx", "ecx", "edx"]
-elif f.tag == "msr":
-reg_list = ["index", "eax", "edx"]
-else:
+if f.tag not in ("cpuid", "msr"):
 continue
 
 feature = {"type": f.tag}
-for reg in reg_list:
+for reg in _KEYS[f.tag] + _REGS[f.tag]:
 feature[reg] = int(f.attrib.get(reg, "0"), 0)
 addFeature(cpuData, feature)
 return cpuData
@@ -86,15 +71,11 @@ def parseMap():
 
 cpuMap = dict()
 for f in xml.etree.ElementTree.parse(path).getroot().iter("feature"):
-if f[0].tag == "cpuid":
-reg_list = ["eax_in", "ecx_in", "eax", "ebx", "ecx", "edx"]
-elif f[0].tag == "msr":
-reg_list = ["index", "eax", "edx"]
-else:
+if f[0].tag not in ("cpuid", "msr"):
 continue
 
 feature = {"type": f[0].tag}
-for reg in reg_list:
+for reg in _KEYS[f[0].tag] + _REGS[f[0].tag]:
 feature[reg] = int(f[0].attrib.get(reg, "0"), 0)
 cpuMap[f.attrib["name"]] = feature
 return cpuMap
-- 
2.26.2

[libvirt PATCH 4/9] cpu-cpuid: Merge addFeature functions

[Tim Wiederhake]

Prepare to deduplicate the list of relevant registers for cpuid and
msr information.

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

diff --git a/tests/cputestdata/cpu-cpuid.py b/tests/cputestdata/cpu-cpuid.py
index d570884db6..c9948da6f8 100755
--- a/tests/cputestdata/cpu-cpuid.py
+++ b/tests/cputestdata/cpu-cpuid.py
@@ -54,41 +54,25 @@ def checkFeature(cpuData, feature):
 return checkMSRFeature(cpuData, feature)
 
 
-def addCPUIDFeature(cpuData, feature):
-if "cpuid" not in cpuData:
-cpuData["cpuid"] = {}
-cpuid = cpuData["cpuid"]
-
-if feature["eax_in"] not in cpuid:
-cpuid[feature["eax_in"]] = {}
-leaf = cpuid[feature["eax_in"]]
-
-if feature["ecx_in"] not in leaf:
-leaf[feature["ecx_in"]] = {"eax": 0, "ebx": 0, "ecx": 0, "edx": 0}
-leaf = leaf[feature["ecx_in"]]
-
-for reg in ["eax", "ebx", "ecx", "edx"]:
-leaf[reg] |= feature[reg]
-
-
-def addMSRFeature(cpuData, feature):
-if "msr" not in cpuData:
-cpuData["msr"] = {}
-msr = cpuData["msr"]
-
-if feature["index"] not in msr:
-msr[feature["index"]] = {"edx": 0, "eax": 0}
-msr = msr[feature["index"]]
-
-for reg in ["edx", "eax"]:
-msr[reg] |= feature[reg]
-
-
 def addFeature(cpuData, feature):
 if feature["type"] == "cpuid":
-addCPUIDFeature(cpuData, feature)
+# cpuData["cpuid"][eax_in][ecx_in] = {eax:, ebx:, ecx:, edx:}
+keyList = ["type", "eax_in", "ecx_in"]
+regList = ["eax", "ebx", "ecx", "edx"]
 elif feature["type"] == "msr":
-addMSRFeature(cpuData, feature)
+# cpuData["msr"][index] = {eax:, edx:}
+keyList = ["type", "index"]
+regList = ["eax", "edx"]
+else:
+return
+
+for key in keyList:
+if feature[key] not in cpuData:
+cpuData[feature[key]] = dict()
+cpuData = cpuData[feature[key]]
+
+for reg in regList:
+cpuData[reg] = cpuData.get(reg, 0) | feature[reg]
 
 
 def parseQemu(path, features):
-- 
2.26.2

[libvirt PATCH 7/9] cpu-gather: Use actions instead of flags for action argument

[Tim Wiederhake]

This allows for the functionality of cpu-cpuid.py script to be
integrated more naturally in a later patch.

Changes the way this script should be called:
  cpu-gather.py   -> cpu-gather.py
  cpu-gather.py --gather  -> cpu-gather.py gather
  cpu-gather.py --parse   -> cpu-gather.py parse
  cpu-gather.py --gather --parse  -> cpu-gather.py full

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

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index a01c615504..5ca19e5f2b 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -324,21 +324,22 @@ 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'.")
-parser.add_argument(
-"--gather",
-action="store_true",
-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. "
-"If '--gather' is not set, expects input on stdin.")
+subparsers = parser.add_subparsers(dest="action")
+subparsers.add_parser(
+"gather",
+help="Acquire data on target system and outputs to stdout. "
+"This is the default. ")
+subparsers.add_parser(
+"parse",
+help="Reads data from stdin and parses data for libvirt use.")
+subparsers.add_parser(
+"full",
+help="Equivalent to `cpu-gather gather | cpu-gather parse`.")
 
 args = parser.parse_args()
 
-if not args.gather and not args.parse:
-args.gather = True
+if not args.action:
+args.action = "gather"
 
 if not args.path_to_qemu:
 args.path_to_qemu = "qemu-system-x86_64"
@@ -350,13 +351,13 @@ def main():
 if os.path.isfile(f):
 args.path_to_qemu = f
 
-if args.gather:
+if args.action in ["gather", "full"]:
 data = gather(args)
-if not args.parse:
+if args.action == "gather":
 json.dump(data, sys.stdout, indent=2)
 
-if args.parse:
-if not args.gather:
+if args.action in ["parse", "full"]:
+if args.action == "parse":
 data = json.load(sys.stdin)
 parse(data)
 
-- 
2.26.2

[libvirt PATCH 5/9] cpu-cpuid: Merge checkFeature functions

[Tim Wiederhake]

Prepare to deduplicate the list of relevant registers for cpuid and
msr information.

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

diff --git a/tests/cputestdata/cpu-cpuid.py b/tests/cputestdata/cpu-cpuid.py
index c9948da6f8..e90165e563 100755
--- a/tests/cputestdata/cpu-cpuid.py
+++ b/tests/cputestdata/cpu-cpuid.py
@@ -7,51 +7,27 @@ import json
 import xml.etree.ElementTree
 
 
-def checkCPUIDFeature(cpuData, feature):
-eax_in = feature["eax_in"]
-ecx_in = feature["ecx_in"]
-eax = feature["eax"]
-ebx = feature["ebx"]
-ecx = feature["ecx"]
-edx = feature["edx"]
-
-if "cpuid" not in cpuData:
-return False
-
-cpuid = cpuData["cpuid"]
-if eax_in not in cpuid or ecx_in not in cpuid[eax_in]:
-return False
-
-leaf = cpuid[eax_in][ecx_in]
-return ((eax > 0 and leaf["eax"] & eax == eax) or
-(ebx > 0 and leaf["ebx"] & ebx == ebx) or
-(ecx > 0 and leaf["ecx"] & ecx == ecx) or
-(edx > 0 and leaf["edx"] & edx == edx))
-
-
-def checkMSRFeature(cpuData, feature):
-index = feature["index"]
-edx = feature["edx"]
-eax = feature["eax"]
-
-if "msr" not in cpuData:
-return False
-
-msr = cpuData["msr"]
-if index not in msr:
-return False
-
-msr = msr[index]
-return ((edx > 0 and msr["edx"] & edx == edx) or
-(eax > 0 and msr["eax"] & eax == eax))
-
-
 def checkFeature(cpuData, feature):
 if feature["type"] == "cpuid":
-return checkCPUIDFeature(cpuData, feature)
+# cpuData["cpuid"][eax_in][ecx_in] = {eax:, ebx:, ecx:, edx:}
+keyList = ["type", "eax_in", "ecx_in"]
+regList = ["eax", "ebx", "ecx", "edx"]
+elif feature["type"] == "msr":
+# cpuData["msr"][index] = {eax:, edx:}
+keyList = ["type", "index"]
+regList = ["eax", "edx"]
+else:
+return False
+
+for key in keyList:
+if feature[key] not in cpuData:
+return False
+cpuData = cpuData[feature[key]]
 
-if feature["type"] == "msr":
-return checkMSRFeature(cpuData, feature)
+for reg in regList:
+if feature[reg] > 0 and feature[reg] == feature[reg] & cpuData[reg]:
+return True
+return False
 
 
 def addFeature(cpuData, feature):
-- 
2.26.2

[libvirt PATCH 8/9] cpu-gather: Factor out call to cpu-cpuid.py

[Tim Wiederhake]

This is a preparatory step to merge cpu-cpuid.py.

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

diff --git a/tests/cputestdata/cpu-gather.py b/tests/cputestdata/cpu-gather.py
index 5ca19e5f2b..a4a4050e42 100755
--- a/tests/cputestdata/cpu-gather.py
+++ b/tests/cputestdata/cpu-gather.py
@@ -288,7 +288,7 @@ def output_json(data, filename):
 f.write("\n")
 
 
-def parse(data):
+def parse(args, data):
 filename = parse_filename(data)
 filename_xml = "{}.xml".format(filename)
 filename_json = "{}.json".format(filename)
@@ -301,10 +301,7 @@ def parse(data):
 if os.path.getsize(filename_json) == 0:
 return
 
-output = subprocess.check_output(
-["./cpu-cpuid.py", "diff", filename_json],
-universal_newlines=True)
-print(output)
+args.json_files = getattr(args, "json_files", list()) + [filename_json]
 
 
 def main():
@@ -359,7 +356,12 @@ def main():
 if args.action in ["parse", "full"]:
 if args.action == "parse":
 data = json.load(sys.stdin)
-parse(data)
+parse(args, data)
+
+if "json_files" in args:
+cmd = ["./cpu-cpuid.py", "diff"]
+cmd.extend(args.json_files)
+subprocess.check_call(cmd, universal_newlines=True)
 
 
 if __name__ == "__main__":
-- 
2.26.2

[libvirt PATCH 9/9] cpu-gather: Merge cpu-cpuid.py

[Tim Wiederhake]

Old usage:
  cpu-cpuid.py diff FILE...
New usage:
  cpu-gather.py diff FILE...

Signed-off-by: Tim Wiederhake 
---
 tests/cputestdata/cpu-cpuid.py  | 152 
 tests/cputestdata/cpu-gather.py | 137 +++-
 2 files changed, 134 insertions(+), 155 deletions(-)
 delete mode 100755 tests/cputestdata/cpu-cpuid.py

diff --git a/tests/cputestdata/cpu-cpuid.py b/tests/cputestdata/cpu-cpuid.py
deleted file mode 100755
index 3e852d5d55..00
--- a/tests/cputestdata/cpu-cpuid.py
+++ /dev/null
@@ -1,152 +0,0 @@
-#!/usr/bin/env python3
-
-import argparse
-import os
-import sys
-import json
-import xml.etree.ElementTree
-
-
-_KEYS = {
-"cpuid": ["eax_in", "ecx_in"],
-"msr": ["index"],
-}
-
-_REGS = {
-"cpuid": ["eax", "ebx", "ecx", "edx"],
-"msr": ["eax", "edx"],
-}
-
-
-def checkFeature(cpuData, feature):
-for key in ["type"] + _KEYS.get(feature["type"], list()):
-if feature[key] not in cpuData:
-return False
-cpuData = cpuData[feature[key]]
-
-for reg in _REGS.get(feature["type"], list()):
-if feature[reg] > 0 and feature[reg] == feature[reg] & cpuData[reg]:
-return True
-return False
-
-
-def addFeature(cpuData, feature):
-for key in ["type"] + _KEYS.get(feature["type"], list()):
-if feature[key] not in cpuData:
-cpuData[feature[key]] = dict()
-cpuData = cpuData[feature[key]]
-
-for reg in _REGS.get(feature["type"], list()):
-cpuData[reg] = cpuData.get(reg, 0) | feature[reg]
-
-
-def parseQemu(path, features):
-cpuData = {}
-with open(path, "r") as f:
-data, pos = json.JSONDecoder().raw_decode(f.read())
-
-for (prop, val) in data["return"]["model"]["props"].items():
-if val and prop in features:
-addFeature(cpuData, features[prop])
-
-return cpuData
-
-
-def parseCPUData(path):
-cpuData = dict()
-for f in xml.etree.ElementTree.parse(path).getroot():
-if f.tag not in ("cpuid", "msr"):
-continue
-
-feature = {"type": f.tag}
-for reg in _KEYS[f.tag] + _REGS[f.tag]:
-feature[reg] = int(f.attrib.get(reg, "0"), 0)
-addFeature(cpuData, feature)
-return cpuData
-
-
-def parseMap():
-path = os.path.dirname(sys.argv[0])
-path = os.path.join(path, "..", "..", "src", "cpu_map", "x86_features.xml")
-
-cpuMap = dict()
-for f in xml.etree.ElementTree.parse(path).getroot().iter("feature"):
-if f[0].tag not in ("cpuid", "msr"):
-continue
-
-feature = {"type": f[0].tag}
-for reg in _KEYS[f[0].tag] + _REGS[f[0].tag]:
-feature[reg] = int(f[0].attrib.get(reg, "0"), 0)
-cpuMap[f.attrib["name"]] = feature
-return cpuMap
-
-
-def formatCPUData(cpuData, path, comment):
-print(path)
-with open(path, "w") as f:
-f.write("\n")
-f.write("\n")
-
-cpuid = cpuData["cpuid"]
-for eax_in in sorted(cpuid.keys()):
-for ecx_in in sorted(cpuid[eax_in].keys()):
-leaf = cpuid[eax_in][ecx_in]
-line = ("  \n")
-f.write(line % (
-eax_in, ecx_in,
-leaf["eax"], leaf["ebx"], leaf["ecx"], leaf["edx"]))
-
-if "msr" in cpuData:
-msr = cpuData["msr"]
-for index in sorted(msr.keys()):
-f.write("  \n" %
-(index, msr[index]['edx'], msr[index]['eax']))
-
-f.write("\n")
-
-
-def diff(args):
-cpuMap = parseMap()
-
-for jsonFile in args.json_files:
-cpuDataFile = jsonFile.replace(".json", ".xml")
-enabledFile = jsonFile.replace(".json", "-enabled.xml")
-disabledFile = jsonFile.replace(".json", "-disabled.xml")
-
-cpuData = parseCPUData(cpuDataFile)
-qemu = parseQemu(jsonFile, cpuMap)
-
-enabled = dict()
-disabled = dict()
-for feature in cpuMap.values():
-if checkFeature(qemu, feature):
-addFeature(enabled, feature)
-elif checkFeature(cpuData, feature):
-addFeature(disabled, feature)
-
-formatCPUData(enabled, enabledFile, "Features enabled by QEMU")
-formatCPUData(disabled, disabledFile, "Features disabled by QEMU")
-
-
-def main():
-parser = argparse.ArgumentParser(description="Diff cpuid results")
-subparsers = parser.add_subparsers(dest="action", required=True)
-diffparser = subparsers.add_parser(
-"diff",
-help="Diff json description of CPU model against known features.")
-diffparser.add_argument(
-"json_files",
-nargs="+",
-metavar="FILE",
-type=os.path.realpath,
-help="Path to one or more json CPU model descriptions.")
-args = parser.parse_args()
-
-diff(args)
-exit(0)
-
-
-if __name__ == "__main__":
-main()
diff --git a/tests/cputestdata/cpu-gather.py b/tests

[libvirt PATCH 2/9] cpu-cpuid: Remove xmltodict usage in parseMap

[Tim Wiederhake]

'xmltodict' is a Python module that is not installed by default.
Replace it, so the dependencies of cpu-gather.py do not change
when both scripts are merged.

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

diff --git a/tests/cputestdata/cpu-cpuid.py b/tests/cputestdata/cpu-cpuid.py
index 6ca72d2262..8e06baea85 100755
--- a/tests/cputestdata/cpu-cpuid.py
+++ b/tests/cputestdata/cpu-cpuid.py
@@ -5,6 +5,7 @@ import os
 import sys
 import json
 import xmltodict
+import xml.etree.ElementTree
 
 
 def checkCPUIDFeature(cpuData, feature):
@@ -132,37 +133,23 @@ def parseCPUData(path):
 return cpuData
 
 
-def parseMapFeature(fType, data):
-ret = {"type": fType}
-
-if fType == "cpuid":
-fields = ["eax_in", "ecx_in", "eax", "ebx", "ecx", "edx"]
-elif fType == "msr":
-fields = ["index", "edx", "eax"]
-
-for field in fields:
-attr = "@%s" % field
-if attr in data:
-ret[field] = int(data[attr], 0)
-else:
-ret[field] = 0
-
-return ret
-
-
 def parseMap():
 path = os.path.dirname(sys.argv[0])
 path = os.path.join(path, "..", "..", "src", "cpu_map", "x86_features.xml")
-with open(path, "rb") as f:
-data = xmltodict.parse(f)
 
-cpuMap = {}
-for feature in data["cpus"]["feature"]:
-for fType in ["cpuid", "msr"]:
-if fType not in feature:
-continue
-cpuMap[feature["@name"]] = parseMapFeature(fType, feature[fType])
+cpuMap = dict()
+for f in xml.etree.ElementTree.parse(path).getroot().iter("feature"):
+if f[0].tag == "cpuid":
+reg_list = ["eax_in", "ecx_in", "eax", "ebx", "ecx", "edx"]
+elif f[0].tag == "msr":
+reg_list = ["index", "eax", "edx"]
+else:
+continue
 
+feature = {"type": f[0].tag}
+for reg in reg_list:
+feature[reg] = int(f[0].attrib.get(reg, "0"), 0)
+cpuMap[f.attrib["name"]] = feature
 return cpuMap
 
 
-- 
2.26.2

Re: Ping: [PATCH] Qemu: migration: Not bind RAM info with active migration status

[Daniel P . Berrangé]

On Fri, Dec 18, 2020 at 04:38:22PM +0800, Keqian Zhu wrote:
> Hi Daniel and Jiri,
> 
> On 2020/12/8 18:31, Jiri Denemark wrote:
> > On Tue, Dec 08, 2020 at 09:27:39 +, Daniel P. Berrangé wrote:
> >> On Tue, Dec 08, 2020 at 10:06:25AM +0800, zhukeqian wrote:
> >>>
> >>> On 2020/12/7 18:38, Daniel P. Berrangé wrote:
>  On Mon, Dec 07, 2020 at 09:55:53AM +0800, zhukeqian wrote:
> > Hi Daniel,
> [...]
> 
> >>>
> >>> Hi Daniel,
> >>>
> >>> The purpose is to remove this failure check for QEMU v2.12.
> >>> In QEMU commit 65ace0604551, it decoupled the RAM status from the active 
> >>> migration status.
> >>>
> >>> The usage scenario is querying migration status at destination side, 
> >>> which may contain
> >>> active migration status, but without RAM status, so we will see that 
> >>> libvirt report error here.
> >>
> >> I'm confused, because AFAIK, libvirt does not need to run
> >> query-migrate on the destination, so there shouldn't be anything
> >> that needs fixing.
> > 
> > Moreover, you can't even request migration statistics on the destination
> > manually because libvirt blocks that:
> > 
> > # virsh domjobinfo nest
> > error: Operation not supported: migration statistics are available only
> > on the source host
> > 
> > Jirka
> > 
> > .
> > 
> Sorry for delay reply.
> 
> The purpose of QEMU commit 65ace0604551 (migration: add postcopy total 
> blocktime into query-migrate)
> is to query some postcopy related information on destination side.
> 
> We can call query-migrate on destination side *after* migration complete, 
> thanks.

But nothing in libvirt ever tries to call query-migrate on the dest
side. 

Do you have more patches that add such calls ? If so, then please send a
patch series that does the full job.

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: live migration is not using secondary interface

[Daniel P . Berrangé]

On Sat, Dec 19, 2020 at 12:05:51AM +0100, Martin Kletzander wrote:
> One thing to add, though.  If you would rather configure the destination in a
> way that its hostname resolves to the IP you would prefer for the migration it
> might be enough to make this work the simple way around.

If you /always/ want migration to use a different IP than the defaut
hostname resolves to, then /etc/libvirt/qemu.conf on the target host
can set the "migration_host" parameter to this IP.  This avoids the
need to specify the IP every time you invoke a migration.

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

[PATCH 01/20] virpci, domain_audit: use virPCIDeviceAddressAsString()

[Daniel Henrique Barboza]

There is no need to open code the PCI address string format
when we have a function that does exactly that.

Signed-off-by: Daniel Henrique Barboza 
---
 src/conf/domain_audit.c | 6 +-
 src/util/virpci.c   | 6 ++
 2 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c
index 8bc6633af4..5fa65a8078 100644
--- a/src/conf/domain_audit.c
+++ b/src/conf/domain_audit.c
@@ -361,11 +361,7 @@ virDomainAuditHostdev(virDomainObjPtr vm, 
virDomainHostdevDefPtr hostdev,
 case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS:
 switch ((virDomainHostdevSubsysType) hostdev->source.subsys.type) {
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
-address = g_strdup_printf(VIR_PCI_DEVICE_ADDRESS_FMT,
-  pcisrc->addr.domain,
-  pcisrc->addr.bus,
-  pcisrc->addr.slot,
-  pcisrc->addr.function);
+address = virPCIDeviceAddressAsString(&pcisrc->addr);
 break;
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
 address = g_strdup_printf("%.3d.%.3d", usbsrc->bus, 
usbsrc->device);
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 9bfc743fbd..3d9d583622 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1471,8 +1471,7 @@ virPCIDeviceNew(unsigned int domain,
 dev->address.slot = slot;
 dev->address.function = function;
 
-dev->name = g_strdup_printf(VIR_PCI_DEVICE_ADDRESS_FMT, domain, bus, slot,
-function);
+dev->name = virPCIDeviceAddressAsString(&dev->address);
 
 dev->path = g_strdup_printf(PCI_SYSFS "devices/%s/config", dev->name);
 
@@ -1998,8 +1997,7 @@ 
virPCIDeviceAddressGetIOMMUGroupNum(virPCIDeviceAddressPtr addr)
 g_autofree char *groupNumStr = NULL;
 unsigned int groupNum;
 
-devName = g_strdup_printf(VIR_PCI_DEVICE_ADDRESS_FMT, addr->domain, 
addr->bus,
-  addr->slot, addr->function);
+devName = virPCIDeviceAddressAsString(addr);
 
 devPath = virPCIFile(devName, "iommu_group");
 
-- 
2.26.2

[PATCH 00/20] handle AWOL SR-IOV VF hostdevs during domain lifetime

[Daniel Henrique Barboza]

Hi,

This series is an attempt to fix https://gitlab.com/libvirt/libvirt/-/issues/72.

The root issue is that, for obvious reasons,  virPCIDeviceNew() requires the
device to exist in the host (i.e. the config file must be present in sysfs).
The unexpected absence of the device (e.g. removing the VFs from the host while
a domain was using them) will make virPCIDeviceNew() return -1 in several
code paths, and the effects in Libvirt can be bothersome. Patch 20 will
explain it in greater detail.

Changing virPCIDeviceNew() internals to not fail in this scenario would
be a herculean refactor of innocent code that doesn't have anything to
do with the problem. My approach here is to check if the device exists
in the sysfs in key places, keeping the default virPCIDeviceNew()
behavior intact.

I mentioned this is patch 20 but to emphasize: this is NOT an attempt
to implement surprise hostdev hotunplug support in Libvirt. Removing
SR-IOVs VFs used by any domain is still not cool and should be avoided.
But after this series, Libvirt can at least fall on its feet if this
situation happens.

The series was tested using a Mellanox MT28800 card in a Power 9 server.

Daniel Henrique Barboza (20):
  virpci, domain_audit: use virPCIDeviceAddressAsString()
  qemu, lxc: move NodeDeviceGetPCIInfo() function to domain_driver.c
  domain_driver.c: use PCI address with
virDomainDriverNodeDeviceGetPCIInfo()
  virpci.c: simplify virPCIDeviceNew() signature
  virpci: introduce virPCIDeviceExists()
  virhostdev.c: virHostdevGetPCIHostDevice() now reports missing device
  security_selinux.c: modernize set/restore hostdev subsys label
functions
  security_dac.c: modernize hostdev label set/restore functions
  dac, selinux: skip setting/restoring label for absent PCI devices
  libvirt-nodedev.c: remove return value from virNodeDeviceFree()
  qemu_driver.c: modernize qemuNodeDeviceReAttach()
  libxl_driver.c: modernize libxlNodeDeviceReAttach()
  virsh-domain.c: modernize cmdDetachDevice()
  virhostdev.c: add virHostdevIsPCIDevice() helper
  qemu_cgroup.c: skip absent PCI devices in qemuTeardownHostdevCgroup()
  virpci.c: use virPCIDeviceAddressPtr in virPCIDeviceListFindIndex()
  virpci.c: use virPCIDeviceAddressPtr in virPCIDeviceListFind()
  virpci.c: use virPCIDeviceAddressPtr in virPCIDeviceListSteal()
  virpci.c: use virPCIDeviceAddressPtr in virPCIDeviceListDel()
  virhostdev.c: remove missing PCI devs from hostdev manager

 include/libvirt/libvirt-nodedev.h  |  2 +-
 src/conf/domain_audit.c|  6 +-
 src/datatypes.h|  2 +
 src/hypervisor/domain_driver.c | 30 ++
 src/hypervisor/domain_driver.h |  4 ++
 src/hypervisor/virhostdev.c| 88 ++--
 src/hypervisor/virhostdev.h|  2 +
 src/libvirt-nodedev.c  | 15 +++--
 src/libvirt_private.syms   |  3 +
 src/libxl/libxl_driver.c   | 87 
 src/node_device/node_device_udev.c | 11 ++--
 src/qemu/qemu_cgroup.c | 10 
 src/qemu/qemu_domain_address.c |  5 +-
 src/qemu/qemu_driver.c | 81 +++---
 src/security/security_apparmor.c   |  3 +-
 src/security/security_dac.c| 61 
 src/security/security_selinux.c| 66 +
 src/security/virt-aa-helper.c  |  6 +-
 src/util/virnetdev.c   |  3 +-
 src/util/virnvme.c |  5 +-
 src/util/virpci.c  | 93 ++
 src/util/virpci.h  | 14 ++---
 tests/virhostdevtest.c |  3 +-
 tests/virpcitest.c | 35 ---
 tools/virsh-domain.c   | 18 ++
 25 files changed, 325 insertions(+), 328 deletions(-)

-- 
2.26.2

[PATCH 03/20] domain_driver.c: use PCI address with virDomainDriverNodeDeviceGetPCIInfo()

[Daniel Henrique Barboza]

Instead of receiving 4 uints in order and write domain/bus/slot/function,
receive a virPCIDeviceAddressPtr instead and write into it.

This change will allow us to simplify the API for virPCIDeviceNew()
in the next patch.

Signed-off-by: Daniel Henrique Barboza 
---
 src/hypervisor/domain_driver.c | 13 +
 src/hypervisor/domain_driver.h |  5 +
 src/libxl/libxl_driver.c   | 18 +-
 src/qemu/qemu_driver.c | 18 +-
 4 files changed, 24 insertions(+), 30 deletions(-)

diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c
index 68dbf10ac6..cdb53bcf3e 100644
--- a/src/hypervisor/domain_driver.c
+++ b/src/hypervisor/domain_driver.c
@@ -341,20 +341,17 @@ 
virDomainDriverSetupPersistentDefBlkioParams(virDomainDefPtr persistentDef,
 
 int
 virDomainDriverNodeDeviceGetPCIInfo(virNodeDeviceDefPtr def,
-unsigned *domain,
-unsigned *bus,
-unsigned *slot,
-unsigned *function)
+virPCIDeviceAddressPtr devAddr)
 {
 virNodeDevCapsDefPtr cap;
 
 cap = def->caps;
 while (cap) {
 if (cap->data.type == VIR_NODE_DEV_CAP_PCI_DEV) {
-*domain   = cap->data.pci_dev.domain;
-*bus  = cap->data.pci_dev.bus;
-*slot = cap->data.pci_dev.slot;
-*function = cap->data.pci_dev.function;
+devAddr->domain = cap->data.pci_dev.domain;
+devAddr->bus = cap->data.pci_dev.bus;
+devAddr->slot = cap->data.pci_dev.slot;
+devAddr->function = cap->data.pci_dev.function;
 break;
 }
 
diff --git a/src/hypervisor/domain_driver.h b/src/hypervisor/domain_driver.h
index 2bb053d559..86b92d0284 100644
--- a/src/hypervisor/domain_driver.h
+++ b/src/hypervisor/domain_driver.h
@@ -48,7 +48,4 @@ int 
virDomainDriverSetupPersistentDefBlkioParams(virDomainDefPtr persistentDef,
  int nparams);
 
 int virDomainDriverNodeDeviceGetPCIInfo(virNodeDeviceDefPtr def,
-unsigned *domain,
-unsigned *bus,
-unsigned *slot,
-unsigned *function);
+virPCIDeviceAddressPtr devAddr);
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index ac2564a563..691cecb1e9 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -5786,7 +5786,7 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev,
unsigned int flags)
 {
 virPCIDevicePtr pci = NULL;
-unsigned domain = 0, bus = 0, slot = 0, function = 0;
+virPCIDeviceAddress devAddr;
 int ret = -1;
 virNodeDeviceDefPtr def = NULL;
 char *xml = NULL;
@@ -5821,10 +5821,10 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev,
 if (virNodeDeviceDetachFlagsEnsureACL(dev->conn, def) < 0)
 goto cleanup;
 
-if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, 
&function) < 0)
+if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0)
 goto cleanup;
 
-pci = virPCIDeviceNew(domain, bus, slot, function);
+pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, 
devAddr.function);
 if (!pci)
 goto cleanup;
 
@@ -5859,7 +5859,7 @@ static int
 libxlNodeDeviceReAttach(virNodeDevicePtr dev)
 {
 virPCIDevicePtr pci = NULL;
-unsigned domain = 0, bus = 0, slot = 0, function = 0;
+virPCIDeviceAddress devAddr;
 int ret = -1;
 virNodeDeviceDefPtr def = NULL;
 char *xml = NULL;
@@ -5892,10 +5892,10 @@ libxlNodeDeviceReAttach(virNodeDevicePtr dev)
 if (virNodeDeviceReAttachEnsureACL(dev->conn, def) < 0)
 goto cleanup;
 
-if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, 
&function) < 0)
+if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0)
 goto cleanup;
 
-pci = virPCIDeviceNew(domain, bus, slot, function);
+pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, 
devAddr.function);
 if (!pci)
 goto cleanup;
 
@@ -5917,7 +5917,7 @@ static int
 libxlNodeDeviceReset(virNodeDevicePtr dev)
 {
 virPCIDevicePtr pci = NULL;
-unsigned domain = 0, bus = 0, slot = 0, function = 0;
+virPCIDeviceAddress devAddr;
 int ret = -1;
 virNodeDeviceDefPtr def = NULL;
 char *xml = NULL;
@@ -5950,10 +5950,10 @@ libxlNodeDeviceReset(virNodeDevicePtr dev)
 if (virNodeDeviceResetEnsureACL(dev->conn, def) < 0)
 goto cleanup;
 
-if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, 
&function) < 0)
+if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0)
 goto cleanup;
 
-pci = virPCIDeviceNew(domain, bus, slot, function);
+pci = virP

[PATCH 04/20] virpci.c: simplify virPCIDeviceNew() signature

[Daniel Henrique Barboza]

The current virPCIDeviceNew() signature, receiving 4 uints in sequence
(domain, bus, slot, function), is not neat.

We already have a way to represent a PCI address in virPCIDeviceAddress
that is used in the code. Aside from the test files, most of
virPCIDeviceNew() callers have access to a virPCIDeviceAddress reference,
but then we need to retrieve the 4 required uints (addr.domain, addr.bus,
addr.slot, addr.function) to satisfy virPCIDeviceNew(). The result is
that we have extra verbosity/boilerplate to retrieve an information that
is already available in virPCIDeviceAddress.

A better way is presented by virNVMEDeviceNew(), where the caller just
supplies a virPCIDeviceAddress pointer and the function handles the
details internally.

This patch changes virPCIDeviceNew() to receive a virPCIDeviceAddress
pointer instead of 4 uints.

Signed-off-by: Daniel Henrique Barboza 
---
 src/hypervisor/virhostdev.c|  3 +--
 src/libxl/libxl_driver.c   |  6 ++---
 src/node_device/node_device_udev.c | 11 +---
 src/qemu/qemu_domain_address.c |  5 +---
 src/qemu/qemu_driver.c |  6 ++---
 src/security/security_apparmor.c   |  3 +--
 src/security/security_dac.c|  6 ++---
 src/security/security_selinux.c|  6 ++---
 src/security/virt-aa-helper.c  |  6 +
 src/util/virnetdev.c   |  3 +--
 src/util/virnvme.c |  5 +---
 src/util/virpci.c  | 40 +-
 src/util/virpci.h  |  5 +---
 tests/virhostdevtest.c |  3 ++-
 tests/virpcitest.c | 35 +++---
 15 files changed, 64 insertions(+), 79 deletions(-)

diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c
index 120187b07a..7338ac0700 100644
--- a/src/hypervisor/virhostdev.c
+++ b/src/hypervisor/virhostdev.c
@@ -235,8 +235,7 @@ virHostdevGetPCIHostDevice(const virDomainHostdevDef 
*hostdev,
 hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
 return 0;
 
-actual = virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus,
- pcisrc->addr.slot, pcisrc->addr.function);
+actual = virPCIDeviceNew(&pcisrc->addr);
 
 if (!actual)
 return -1;
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 691cecb1e9..9b93fd77d2 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -5824,7 +5824,7 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev,
 if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0)
 goto cleanup;
 
-pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, 
devAddr.function);
+pci = virPCIDeviceNew(&devAddr);
 if (!pci)
 goto cleanup;
 
@@ -5895,7 +5895,7 @@ libxlNodeDeviceReAttach(virNodeDevicePtr dev)
 if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0)
 goto cleanup;
 
-pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, 
devAddr.function);
+pci = virPCIDeviceNew(&devAddr);
 if (!pci)
 goto cleanup;
 
@@ -5953,7 +5953,7 @@ libxlNodeDeviceReset(virNodeDevicePtr dev)
 if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0)
 goto cleanup;
 
-pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, 
devAddr.function);
+pci = virPCIDeviceNew(&devAddr);
 if (!pci)
 goto cleanup;
 
diff --git a/src/node_device/node_device_udev.c 
b/src/node_device/node_device_udev.c
index 55a2731681..fceb135aa5 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -367,6 +367,7 @@ udevProcessPCI(struct udev_device *device,
 virNodeDevCapPCIDevPtr pci_dev = &def->caps->data.pci_dev;
 virPCIEDeviceInfoPtr pci_express = NULL;
 virPCIDevicePtr pciDev = NULL;
+virPCIDeviceAddress devAddr;
 int ret = -1;
 char *p;
 bool privileged;
@@ -416,10 +417,12 @@ udevProcessPCI(struct udev_device *device,
 if (virNodeDeviceGetPCIDynamicCaps(def->sysfs_path, pci_dev) < 0)
 goto cleanup;
 
-if (!(pciDev = virPCIDeviceNew(pci_dev->domain,
-   pci_dev->bus,
-   pci_dev->slot,
-   pci_dev->function)))
+devAddr.domain = pci_dev->domain;
+devAddr.bus = pci_dev->bus;
+devAddr.slot = pci_dev->slot;
+devAddr.function = pci_dev->function;
+
+if (!(pciDev = virPCIDeviceNew(&devAddr)))
 goto cleanup;
 
 /* We need to be root to read PCI device configs */
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index f0ba318cc8..ae4ad6677c 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -857,10 +857,7 @@ 
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
 return 0;
 }
 
-if (!(pciDev = virPCIDeviceNew(hostAddr->domain,
-   hostAddr->bus,
- 

[PATCH 02/20] qemu, lxc: move NodeDeviceGetPCIInfo() function to domain_driver.c

[Daniel Henrique Barboza]

libxlNodeDeviceGetPCIInfo() and qemuNodeDeviceGetPCIInfo() are equal.
Let's move the logic to a new virDomainDriverNodeDeviceGetPCIInfo()
info to be used by libxl_driver.c and qemu_driver.c.

Signed-off-by: Daniel Henrique Barboza 
---
 src/hypervisor/domain_driver.c | 33 +
 src/hypervisor/domain_driver.h |  7 +++
 src/libvirt_private.syms   |  1 +
 src/libxl/libxl_driver.c   | 38 --
 src/qemu/qemu_driver.c | 37 +++--
 5 files changed, 48 insertions(+), 68 deletions(-)

diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c
index 8dc5870a61..68dbf10ac6 100644
--- a/src/hypervisor/domain_driver.c
+++ b/src/hypervisor/domain_driver.c
@@ -21,6 +21,7 @@
 #include 
 
 #include "domain_driver.h"
+#include "node_device_conf.h"
 #include "viralloc.h"
 #include "virstring.h"
 #include "vircrypto.h"
@@ -336,3 +337,35 @@ 
virDomainDriverSetupPersistentDefBlkioParams(virDomainDefPtr persistentDef,
 
 return ret;
 }
+
+
+int
+virDomainDriverNodeDeviceGetPCIInfo(virNodeDeviceDefPtr def,
+unsigned *domain,
+unsigned *bus,
+unsigned *slot,
+unsigned *function)
+{
+virNodeDevCapsDefPtr cap;
+
+cap = def->caps;
+while (cap) {
+if (cap->data.type == VIR_NODE_DEV_CAP_PCI_DEV) {
+*domain   = cap->data.pci_dev.domain;
+*bus  = cap->data.pci_dev.bus;
+*slot = cap->data.pci_dev.slot;
+*function = cap->data.pci_dev.function;
+break;
+}
+
+cap = cap->next;
+}
+
+if (!cap) {
+virReportError(VIR_ERR_INVALID_ARG,
+   _("device %s is not a PCI device"), def->name);
+return -1;
+}
+
+return 0;
+}
diff --git a/src/hypervisor/domain_driver.h b/src/hypervisor/domain_driver.h
index b66ae2d421..2bb053d559 100644
--- a/src/hypervisor/domain_driver.h
+++ b/src/hypervisor/domain_driver.h
@@ -21,6 +21,7 @@
 #pragma once
 
 #include "domain_conf.h"
+#include "node_device_conf.h"
 
 char *
 virDomainDriverGenerateRootHash(const char *drivername,
@@ -45,3 +46,9 @@ int virDomainDriverParseBlkioDeviceStr(char *blkioDeviceStr, 
const char *type,
 int virDomainDriverSetupPersistentDefBlkioParams(virDomainDefPtr persistentDef,
  virTypedParameterPtr params,
  int nparams);
+
+int virDomainDriverNodeDeviceGetPCIInfo(virNodeDeviceDefPtr def,
+unsigned *domain,
+unsigned *bus,
+unsigned *slot,
+unsigned *function);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 583fc5800e..fe4ee25f3a 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1445,6 +1445,7 @@ virDomainCgroupSetupMemtune;
 virDomainDriverGenerateMachineName;
 virDomainDriverGenerateRootHash;
 virDomainDriverMergeBlkioDevice;
+virDomainDriverNodeDeviceGetPCIInfo;
 virDomainDriverParseBlkioDeviceStr;
 virDomainDriverSetupPersistentDefBlkioParams;
 
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index d58af1a869..ac2564a563 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -56,6 +56,7 @@
 #include "cpu/cpu.h"
 #include "virutil.h"
 #include "domain_validate.h"
+#include "domain_driver.h"
 
 #define VIR_FROM_THIS VIR_FROM_LIBXL
 
@@ -5779,37 +5780,6 @@ libxlConnectSupportsFeature(virConnectPtr conn, int 
feature)
 }
 }
 
-static int
-libxlNodeDeviceGetPCIInfo(virNodeDeviceDefPtr def,
-  unsigned *domain,
-  unsigned *bus,
-  unsigned *slot,
-  unsigned *function)
-{
-virNodeDevCapsDefPtr cap;
-
-cap = def->caps;
-while (cap) {
-if (cap->data.type == VIR_NODE_DEV_CAP_PCI_DEV) {
-*domain   = cap->data.pci_dev.domain;
-*bus  = cap->data.pci_dev.bus;
-*slot = cap->data.pci_dev.slot;
-*function = cap->data.pci_dev.function;
-break;
-}
-
-cap = cap->next;
-}
-
-if (!cap) {
-virReportError(VIR_ERR_INVALID_ARG,
-   _("device %s is not a PCI device"), def->name);
-return -1;
-}
-
-return 0;
-}
-
 static int
 libxlNodeDeviceDetachFlags(virNodeDevicePtr dev,
const char *driverName,
@@ -5851,7 +5821,7 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev,
 if (virNodeDeviceDetachFlagsEnsureACL(dev->conn, def) < 0)
 goto cleanup;
 
-if (libxlNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0)
+if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, 

[PATCH 05/20] virpci: introduce virPCIDeviceExists()

[Daniel Henrique Barboza]

We're going to add logic to handle the case where a previously
existing PCI device does not longer exist in the host.

The logic was copied from virPCIDeviceNew(), which verifies if a
PCI device exists in the host, returning NULL and throwing an
error if it doesn't. The NULL is used for other errors as well
(product/vendor id read errors, dev id overflow), meaning that we
can't re-use virPCIDeviceNew() for the purpose of detecting
if the device exists.

Signed-off-by: Daniel Henrique Barboza 
---
 src/libvirt_private.syms |  1 +
 src/util/virpci.c| 10 ++
 src/util/virpci.h|  1 +
 3 files changed, 12 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index fe4ee25f3a..81ce142635 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2817,6 +2817,7 @@ virPCIDeviceAddressIsValid;
 virPCIDeviceAddressParse;
 virPCIDeviceCopy;
 virPCIDeviceDetach;
+virPCIDeviceExists;
 virPCIDeviceFileIterate;
 virPCIDeviceFree;
 virPCIDeviceGetAddress;
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 545b2650a0..1a542b18b6 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1448,6 +1448,16 @@ virPCIDeviceAddressAsString(const virPCIDeviceAddress 
*addr)
 return str;
 }
 
+bool
+virPCIDeviceExists(const virPCIDeviceAddress *addr)
+{
+g_autofree char *devName = virPCIDeviceAddressAsString(addr);
+g_autofree char *devPath = g_strdup_printf(PCI_SYSFS "devices/%s/config",
+   devName);
+
+return virFileExists(devPath);
+}
+
 virPCIDevicePtr
 virPCIDeviceNew(const virPCIDeviceAddress *address)
 {
diff --git a/src/util/virpci.h b/src/util/virpci.h
index d4451848c1..a9c597a428 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -201,6 +201,7 @@ int 
virPCIDeviceAddressGetIOMMUGroupAddresses(virPCIDeviceAddressPtr devAddr,
   size_t *nIommuGroupDevices);
 int virPCIDeviceAddressGetIOMMUGroupNum(virPCIDeviceAddressPtr addr);
 char *virPCIDeviceAddressGetIOMMUGroupDev(const virPCIDeviceAddress *devAddr);
+bool virPCIDeviceExists(const virPCIDeviceAddress *addr);
 char *virPCIDeviceGetIOMMUGroupDev(virPCIDevicePtr dev);
 
 int virPCIDeviceIsAssignable(virPCIDevicePtr dev,
-- 
2.26.2

[PATCH 06/20] virhostdev.c: virHostdevGetPCIHostDevice() now reports missing device

[Daniel Henrique Barboza]

Gitlab issue #72 [1] reports that removing SR-IOVs VFs before
removing the devices from the running domains can have strange
consequences. QEMU might be able to hotunplug the device inside the
guest, but Libvirt will not be aware of that, and then the guest is
now inconsistent with the domain definition.

There's also the possibility of the VFs removal not succeeding
while the domain is running but then, as soon as the domain
is shutdown, all the VFs are removed. Libvirt can't handle
the removal of the PCI devices while trying to reattach the
hostdevs, and the Libvirt daemon can be left in an inconsistent
state (see [2]).

This patch starts to address the issue related in Gitlab #72, most
notably the issue described in [2]. When shutting down a domain
with SR-IOV hostdevs that got missing, virHostdevReAttachPCIDevices()
is failing the whole process and failing to reattach all the
PCI devices, including the ones that aren't related to the VFs that
went missing. Let's make it more resilient with host changes by
changing virHostdevGetPCIHostDevice() to return an exclusive error
code '-2' for this case. virHostdevGetPCIHostDeviceList() can then
tell when virHostdevGetPCIHostDevice() failed to find the PCI
device of a hostdev and continue to make the list of PCI devices.

virHostdevReAttachPCIDevices() will now be able to proceed reattaching
all other valid PCI devices, at least. The 'ghost hostdevs' will be
handled later on.

[1] https://gitlab.com/libvirt/libvirt/-/issues/72
[2] https://gitlab.com/libvirt/libvirt/-/issues/72#note_459032148

Signed-off-by: Daniel Henrique Barboza 
---
 src/hypervisor/virhostdev.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c
index 7338ac0700..18373deb41 100644
--- a/src/hypervisor/virhostdev.c
+++ b/src/hypervisor/virhostdev.c
@@ -220,7 +220,8 @@ virHostdevManagerGetDefault(void)
  * is returned.
  *
  * Returns: 0 on success (@pci might be NULL though),
- * -1 otherwise (with error reported).
+ * -1 otherwise (with error reported),
+ * -2 PCI device not found. @pci will be NULL
  */
 static int
 virHostdevGetPCIHostDevice(const virDomainHostdevDef *hostdev,
@@ -235,6 +236,9 @@ virHostdevGetPCIHostDevice(const virDomainHostdevDef 
*hostdev,
 hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
 return 0;
 
+if (!virPCIDeviceExists(&pcisrc->addr))
+return -2;
+
 actual = virPCIDeviceNew(&pcisrc->addr);
 
 if (!actual)
@@ -270,7 +274,7 @@ virHostdevGetPCIHostDeviceList(virDomainHostdevDefPtr 
*hostdevs, int nhostdevs)
 virDomainHostdevDefPtr hostdev = hostdevs[i];
 g_autoptr(virPCIDevice) pci = NULL;
 
-if (virHostdevGetPCIHostDevice(hostdev, &pci) < 0)
+if (virHostdevGetPCIHostDevice(hostdev, &pci) == -1)
 return NULL;
 
 if (!pci)
-- 
2.26.2

[PATCH 07/20] security_selinux.c: modernize set/restore hostdev subsys label functions

[Daniel Henrique Barboza]

Use g_auto* cleanup to avoid free() calls.

Signed-off-by: Daniel Henrique Barboza 
---
 src/security/security_selinux.c | 54 ++---
 1 file changed, 16 insertions(+), 38 deletions(-)

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index c43d326314..bf53932ccc 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -2086,7 +2086,7 @@ 
virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManagerPtr mgr,
 
 switch ((virDomainHostdevSubsysType)dev->source.subsys.type) {
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: {
-virUSBDevicePtr usb;
+g_autoptr(virUSBDevice) usb = NULL;
 
 if (dev->missing)
 return 0;
@@ -2098,39 +2098,34 @@ 
virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManagerPtr mgr,
 return -1;
 
 ret = virUSBDeviceFileIterate(usb, virSecuritySELinuxSetUSBLabel, 
&data);
-virUSBDeviceFree(usb);
 break;
 }
 
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
-virPCIDevicePtr pci =
-virPCIDeviceNew(&pcisrc->addr);
+g_autoptr(virPCIDevice) pci = virPCIDeviceNew(&pcisrc->addr);
 
 if (!pci)
 return -1;
 
 if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
-char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
+g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
 
-if (!vfioGroupDev) {
-virPCIDeviceFree(pci);
+if (!vfioGroupDev)
 return -1;
-}
+
 ret = virSecuritySELinuxSetHostdevLabelHelper(vfioGroupDev,
   false,
   &data);
-VIR_FREE(vfioGroupDev);
 } else {
 ret = virPCIDeviceFileIterate(pci, virSecuritySELinuxSetPCILabel, 
&data);
 }
-virPCIDeviceFree(pci);
 break;
 }
 
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: {
 virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host;
 
-virSCSIDevicePtr scsi =
+g_autoptr(virSCSIDevice) scsi =
 virSCSIDeviceNew(NULL,
  scsihostsrc->adapter, scsihostsrc->bus,
  scsihostsrc->target, scsihostsrc->unit,
@@ -2142,13 +2137,11 @@ 
virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManagerPtr mgr,
 ret = virSCSIDeviceFileIterate(scsi,
virSecuritySELinuxSetSCSILabel,
&data);
-virSCSIDeviceFree(scsi);
-
 break;
 }
 
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST: {
-virSCSIVHostDevicePtr host = virSCSIVHostDeviceNew(hostsrc->wwpn);
+g_autoptr(virSCSIVHostDevice) host = 
virSCSIVHostDeviceNew(hostsrc->wwpn);
 
 if (!host)
 return -1;
@@ -2156,19 +2149,16 @@ 
virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManagerPtr mgr,
 ret = virSCSIVHostDeviceFileIterate(host,
 virSecuritySELinuxSetHostLabel,
 &data);
-virSCSIVHostDeviceFree(host);
 break;
 }
 
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: {
-char *vfiodev = NULL;
+g_autofree char *vfiodev = NULL;
 
 if (!(vfiodev = virMediatedDeviceGetIOMMUGroupDev(mdevsrc->uuidstr)))
 return ret;
 
 ret = virSecuritySELinuxSetHostdevLabelHelper(vfiodev, true, &data);
-
-VIR_FREE(vfiodev);
 break;
 }
 
@@ -2324,7 +2314,7 @@ 
virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManagerPtr mgr,
 
 switch ((virDomainHostdevSubsysType)dev->source.subsys.type) {
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: {
-virUSBDevicePtr usb;
+g_autoptr(virUSBDevice) usb = NULL;
 
 if (dev->missing)
 return 0;
@@ -2336,37 +2326,31 @@ 
virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManagerPtr mgr,
 return -1;
 
 ret = virUSBDeviceFileIterate(usb, virSecuritySELinuxRestoreUSBLabel, 
mgr);
-virUSBDeviceFree(usb);
-
 break;
 }
 
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
-virPCIDevicePtr pci =
-virPCIDeviceNew(&pcisrc->addr);
+g_autoptr(virPCIDevice) pci = virPCIDeviceNew(&pcisrc->addr);
 
 if (!pci)
 return -1;
 
 if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
-char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
+g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
 
-if (!vfioGroupDev) {
-virPCIDeviceFree(pci);
+if (!vfioGroupDev)
 return -1;
-}
+
 ret = virSecuritySELinuxRestoreFileLabel(mgr, vfioGroupDev, false);
-   

[PATCH 08/20] security_dac.c: modernize hostdev label set/restore functions

[Daniel Henrique Barboza]

Use g_auto* cleanup to avoid free() calls.

Signed-off-by: Daniel Henrique Barboza 
---
 src/security/security_dac.c | 49 +++--
 1 file changed, 14 insertions(+), 35 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index b5e56feaa8..0085982bb1 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1251,7 +1251,7 @@ virSecurityDACSetHostdevLabel(virSecurityManagerPtr mgr,
 
 switch ((virDomainHostdevSubsysType)dev->source.subsys.type) {
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: {
-virUSBDevicePtr usb;
+g_autoptr(virUSBDevice) usb = NULL;
 
 if (dev->missing)
 return 0;
@@ -1262,41 +1262,35 @@ virSecurityDACSetHostdevLabel(virSecurityManagerPtr mgr,
 ret = virUSBDeviceFileIterate(usb,
   virSecurityDACSetUSBLabel,
   &cbdata);
-virUSBDeviceFree(usb);
 break;
 }
 
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
-virPCIDevicePtr pci =
-virPCIDeviceNew(&pcisrc->addr);
+g_autoptr(virPCIDevice) pci = virPCIDeviceNew(&pcisrc->addr);
 
 if (!pci)
 return -1;
 
 if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
-char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
+g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
 
-if (!vfioGroupDev) {
-virPCIDeviceFree(pci);
+if (!vfioGroupDev)
 return -1;
-}
+
 ret = virSecurityDACSetHostdevLabelHelper(vfioGroupDev,
   false,
   &cbdata);
-VIR_FREE(vfioGroupDev);
 } else {
 ret = virPCIDeviceFileIterate(pci,
   virSecurityDACSetPCILabel,
   &cbdata);
 }
-
-virPCIDeviceFree(pci);
 break;
 }
 
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: {
 virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host;
-virSCSIDevicePtr scsi =
+g_autoptr(virSCSIDevice) scsi =
 virSCSIDeviceNew(NULL,
  scsihostsrc->adapter, scsihostsrc->bus,
  scsihostsrc->target, scsihostsrc->unit,
@@ -1308,13 +1302,11 @@ virSecurityDACSetHostdevLabel(virSecurityManagerPtr mgr,
 ret = virSCSIDeviceFileIterate(scsi,
virSecurityDACSetSCSILabel,
&cbdata);
-virSCSIDeviceFree(scsi);
-
 break;
 }
 
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST: {
-virSCSIVHostDevicePtr host = virSCSIVHostDeviceNew(hostsrc->wwpn);
+g_autoptr(virSCSIVHostDevice) host = 
virSCSIVHostDeviceNew(hostsrc->wwpn);
 
 if (!host)
 return -1;
@@ -1322,19 +1314,16 @@ virSecurityDACSetHostdevLabel(virSecurityManagerPtr mgr,
 ret = virSCSIVHostDeviceFileIterate(host,
 virSecurityDACSetHostLabel,
 &cbdata);
-virSCSIVHostDeviceFree(host);
 break;
 }
 
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: {
-char *vfiodev = NULL;
+g_autofree char *vfiodev = NULL;
 
 if (!(vfiodev = virMediatedDeviceGetIOMMUGroupDev(mdevsrc->uuidstr)))
 return -1;
 
 ret = virSecurityDACSetHostdevLabelHelper(vfiodev, true, &cbdata);
-
-VIR_FREE(vfiodev);
 break;
 }
 
@@ -1420,7 +1409,7 @@ virSecurityDACRestoreHostdevLabel(virSecurityManagerPtr 
mgr,
 
 switch ((virDomainHostdevSubsysType)dev->source.subsys.type) {
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: {
-virUSBDevicePtr usb;
+g_autoptr(virUSBDevice) usb = NULL;
 
 if (dev->missing)
 return 0;
@@ -1429,20 +1418,17 @@ virSecurityDACRestoreHostdevLabel(virSecurityManagerPtr 
mgr,
 return -1;
 
 ret = virUSBDeviceFileIterate(usb, virSecurityDACRestoreUSBLabel, mgr);
-virUSBDeviceFree(usb);
-
 break;
 }
 
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
-virPCIDevicePtr pci =
-virPCIDeviceNew(&pcisrc->addr);
+g_autoptr(virPCIDevice) pci = virPCIDeviceNew(&pcisrc->addr);
 
 if (!pci)
 return -1;
 
 if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
-char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
+g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
 
 if (!vfioGroupDev) {
 virPCIDeviceFree(pci);
@@ -1450,17 +1436,15 @@ virSecurityDACRestoreHostdevLabel(virSecurityManagerPtr 
mgr,
 }
 ret = virSecurityDACRestor

[PATCH 13/20] virsh-domain.c: modernize cmdDetachDevice()

[Daniel Henrique Barboza]

Use g_auto* pointers to avoid the need of a cleanup label. The
type of the pointer 'virDomainPtr dom' was changed to its alias
'virshDomainPtr' to allow the use of g_autoptr().

Signed-off-by: Daniel Henrique Barboza 
---
 tools/virsh-domain.c | 18 ++
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 1ef9b8d606..a8a0f1d3cc 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -11953,11 +11953,10 @@ static const vshCmdOptDef opts_detach_device[] = {
 static bool
 cmdDetachDevice(vshControl *ctl, const vshCmd *cmd)
 {
-virDomainPtr dom = NULL;
+g_autoptr(virshDomain) dom = NULL;
 const char *from = NULL;
-char *buffer = NULL;
+g_autofree char *buffer = NULL;
 int ret;
-bool funcRet = false;
 bool current = vshCommandOptBool(cmd, "current");
 bool config = vshCommandOptBool(cmd, "config");
 bool live = vshCommandOptBool(cmd, "live");
@@ -11982,11 +11981,11 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd)
 flags |= VIR_DOMAIN_AFFECT_LIVE;
 
 if (vshCommandOptStringReq(ctl, cmd, "file", &from) < 0)
-goto cleanup;
+return false;
 
 if (virFileReadAll(from, VSH_MAX_XML_FILE, &buffer) < 0) {
 vshReportError(ctl);
-goto cleanup;
+return false;
 }
 
 if (flags != 0 || current)
@@ -11996,16 +11995,11 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd)
 
 if (ret < 0) {
 vshError(ctl, _("Failed to detach device from %s"), from);
-goto cleanup;
+return false;
 }
 
 vshPrintExtra(ctl, "%s", _("Device detached successfully\n"));
-funcRet = true;
-
- cleanup:
-VIR_FREE(buffer);
-virshDomainFree(dom);
-return funcRet;
+return true;
 }
 
 
-- 
2.26.2

[PATCH 09/20] dac, selinux: skip setting/restoring label for absent PCI devices

[Daniel Henrique Barboza]

If the underlying PCI device of a hostdev does not exist in the
host (e.g. a SR-IOV VF that was removed while the domain was
running), skip security label handling for it.

This will avoid errors that happens during qemuProcessStop() time,
where a VF that was being used by the domain is not present anymore.
The restore label functions of both DAC and SELinux drivers will
trigger errors in virPCIDeviceNew().

Signed-off-by: Daniel Henrique Barboza 
---
 src/security/security_dac.c | 14 --
 src/security/security_selinux.c | 14 --
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 0085982bb1..a2528aeb2d 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1266,7 +1266,12 @@ virSecurityDACSetHostdevLabel(virSecurityManagerPtr mgr,
 }
 
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
-g_autoptr(virPCIDevice) pci = virPCIDeviceNew(&pcisrc->addr);
+g_autoptr(virPCIDevice) pci = NULL;
+
+if (!virPCIDeviceExists(&pcisrc->addr))
+break;
+
+pci = virPCIDeviceNew(&pcisrc->addr);
 
 if (!pci)
 return -1;
@@ -1422,7 +1427,12 @@ virSecurityDACRestoreHostdevLabel(virSecurityManagerPtr 
mgr,
 }
 
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
-g_autoptr(virPCIDevice) pci = virPCIDeviceNew(&pcisrc->addr);
+g_autoptr(virPCIDevice) pci = NULL;
+
+if (!virPCIDeviceExists(&pcisrc->addr))
+break;
+
+pci = virPCIDeviceNew(&pcisrc->addr);
 
 if (!pci)
 return -1;
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index bf53932ccc..cf4984d8b1 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -2102,7 +2102,12 @@ 
virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManagerPtr mgr,
 }
 
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
-g_autoptr(virPCIDevice) pci = virPCIDeviceNew(&pcisrc->addr);
+g_autoptr(virPCIDevice) pci = NULL;
+
+if (!virPCIDeviceExists(&pcisrc->addr))
+break;
+
+pci = virPCIDeviceNew(&pcisrc->addr);
 
 if (!pci)
 return -1;
@@ -2330,7 +2335,12 @@ 
virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManagerPtr mgr,
 }
 
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
-g_autoptr(virPCIDevice) pci = virPCIDeviceNew(&pcisrc->addr);
+g_autoptr(virPCIDevice) pci = NULL;
+
+if (!virPCIDeviceExists(&pcisrc->addr))
+break;
+
+pci = virPCIDeviceNew(&pcisrc->addr);
 
 if (!pci)
 return -1;
-- 
2.26.2

[PATCH 15/20] qemu_cgroup.c: skip absent PCI devices in qemuTeardownHostdevCgroup()

[Daniel Henrique Barboza]

There is no need to bother with cgroup tearing down for absent
PCI devices, given that their entries in the sysfs are already
gone.

Signed-off-by: Daniel Henrique Barboza 
---
 src/qemu/qemu_cgroup.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index f7146a71c9..050df21d87 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -467,6 +467,16 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm,
 if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES))
 return 0;
 
+/* Skip tearing down Cgroup for hostdevs that represents absent
+ * PCI devices, e.g. SR-IOV virtual functions that were removed from
+ * the host while the domain was still running. */
+if (virHostdevIsPCIDevice(dev)) {
+const virDomainHostdevSubsysPCI *pcisrc = &dev->source.subsys.u.pci;
+
+if (!virPCIDeviceExists(&pcisrc->addr))
+return 0;
+}
+
 if (qemuDomainGetHostdevPath(dev, &path, NULL) < 0)
 return -1;
 
-- 
2.26.2

[PATCH 11/20] qemu_driver.c: modernize qemuNodeDeviceReAttach()

[Daniel Henrique Barboza]

Add virObjectUnref an autoptr cleanup func for virNodeDevice,
then remove all unref and free calls from qemuNodeDeviceReAttach().

Signed-off-by: Daniel Henrique Barboza 
---
 src/datatypes.h|  2 ++
 src/qemu/qemu_driver.c | 32 
 2 files changed, 14 insertions(+), 20 deletions(-)

diff --git a/src/datatypes.h b/src/datatypes.h
index ade3779e43..7a88aba0df 100644
--- a/src/datatypes.h
+++ b/src/datatypes.h
@@ -707,6 +707,8 @@ struct _virNodeDevice {
 char *parentName;   /* parent device name */
 };
 
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNodeDevice, virObjectUnref);
+
 /**
  * _virSecret:
  *
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c450501c4d..7b8350dff3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12041,17 +12041,16 @@ static int
 qemuNodeDeviceReAttach(virNodeDevicePtr dev)
 {
 virQEMUDriverPtr driver = dev->conn->privateData;
-virPCIDevicePtr pci = NULL;
+g_autoptr(virPCIDevice) pci = NULL;
 virPCIDeviceAddress devAddr;
-int ret = -1;
-virNodeDeviceDefPtr def = NULL;
+g_autoptr(virNodeDeviceDef) def = NULL;
 g_autofree char *xml = NULL;
 virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;
-virConnectPtr nodeconn = NULL;
-virNodeDevicePtr nodedev = NULL;
+g_autoptr(virConnect) nodeconn = NULL;
+g_autoptr(virNodeDevice) nodedev = NULL;
 
 if (!(nodeconn = virGetConnectNodeDev()))
-goto cleanup;
+return -1;
 
 /* 'dev' is associated with the QEMU virConnectPtr,
  * so for split daemons, we need to get a copy that
@@ -12059,36 +12058,29 @@ qemuNodeDeviceReAttach(virNodeDevicePtr dev)
  */
 if (!(nodedev = virNodeDeviceLookupByName(
   nodeconn, virNodeDeviceGetName(dev
-goto cleanup;
+return -1;
 
 xml = virNodeDeviceGetXMLDesc(nodedev, 0);
 if (!xml)
-goto cleanup;
+return -1;
 
 def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL);
 if (!def)
-goto cleanup;
+return -1;
 
 /* ACL check must happen against original 'dev',
  * not the new 'nodedev' we acquired */
 if (virNodeDeviceReAttachEnsureACL(dev->conn, def) < 0)
-goto cleanup;
+return -1;
 
 if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0)
-goto cleanup;
+return -1;
 
 pci = virPCIDeviceNew(&devAddr);
 if (!pci)
-goto cleanup;
-
-ret = virHostdevPCINodeDeviceReAttach(hostdev_mgr, pci);
+return -1;
 
-virPCIDeviceFree(pci);
- cleanup:
-virNodeDeviceDefFree(def);
-virObjectUnref(nodedev);
-virObjectUnref(nodeconn);
-return ret;
+return virHostdevPCINodeDeviceReAttach(hostdev_mgr, pci);
 }
 
 static int
-- 
2.26.2

[PATCH 12/20] libxl_driver.c: modernize libxlNodeDeviceReAttach()

[Daniel Henrique Barboza]

Use g_auto* wherever we can and remove the 'cleanup' label.

Signed-off-by: Daniel Henrique Barboza 
---
 src/libxl/libxl_driver.c | 37 ++---
 1 file changed, 14 insertions(+), 23 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 9b93fd77d2..b83e35f67d 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -5858,18 +5858,17 @@ libxlNodeDeviceDettach(virNodeDevicePtr dev)
 static int
 libxlNodeDeviceReAttach(virNodeDevicePtr dev)
 {
-virPCIDevicePtr pci = NULL;
+g_autoptr(virPCIDevice) pci = NULL;
 virPCIDeviceAddress devAddr;
-int ret = -1;
-virNodeDeviceDefPtr def = NULL;
-char *xml = NULL;
+g_autoptr(virNodeDeviceDef) def = NULL;
+g_autofree char *xml = NULL;
 libxlDriverPrivatePtr driver = dev->conn->privateData;
 virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;
-virConnectPtr nodeconn = NULL;
-virNodeDevicePtr nodedev = NULL;
+g_autoptr(virConnect) nodeconn = NULL;
+g_autoptr(virNodeDevice) nodedev = NULL;
 
 if (!(nodeconn = virGetConnectNodeDev()))
-goto cleanup;
+return -1;
 
 /* 'dev' is associated with the QEMU virConnectPtr,
  * so for split daemons, we need to get a copy that
@@ -5877,40 +5876,32 @@ libxlNodeDeviceReAttach(virNodeDevicePtr dev)
  */
 if (!(nodedev = virNodeDeviceLookupByName(
   nodeconn, virNodeDeviceGetName(dev
-goto cleanup;
+return -1;
 
 xml = virNodeDeviceGetXMLDesc(nodedev, 0);
 if (!xml)
-goto cleanup;
+return -1;
 
 def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL);
 if (!def)
-goto cleanup;
+return -1;
 
 /* ACL check must happen against original 'dev',
  * not the new 'nodedev' we acquired */
 if (virNodeDeviceReAttachEnsureACL(dev->conn, def) < 0)
-goto cleanup;
+return -1;
 
 if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0)
-goto cleanup;
+return -1;
 
 pci = virPCIDeviceNew(&devAddr);
 if (!pci)
-goto cleanup;
+return -1;
 
 if (virHostdevPCINodeDeviceReAttach(hostdev_mgr, pci) < 0)
-goto cleanup;
-
-ret = 0;
+return -1;
 
- cleanup:
-virPCIDeviceFree(pci);
-virNodeDeviceDefFree(def);
-virObjectUnref(nodedev);
-virObjectUnref(nodeconn);
-VIR_FREE(xml);
-return ret;
+return 0;
 }
 
 static int
-- 
2.26.2

[PATCH 10/20] libvirt-nodedev.c: remove return value from virNodeDeviceFree()

[Daniel Henrique Barboza]

The function returns -1 on error, but no caller is actually checking
the return value. Making it 'void' makes more sense with its
current use.

Signed-off-by: Daniel Henrique Barboza 
---
 include/libvirt/libvirt-nodedev.h |  2 +-
 src/libvirt-nodedev.c | 15 +++
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/include/libvirt/libvirt-nodedev.h 
b/include/libvirt/libvirt-nodedev.h
index eab8abf6ab..5634980a75 100644
--- a/include/libvirt/libvirt-nodedev.h
+++ b/include/libvirt/libvirt-nodedev.h
@@ -114,7 +114,7 @@ char *  virNodeDeviceGetXMLDesc 
(virNodeDevicePtr dev,
  unsigned int flags);
 
 int virNodeDeviceRef(virNodeDevicePtr dev);
-int virNodeDeviceFree   (virNodeDevicePtr dev);
+voidvirNodeDeviceFree   (virNodeDevicePtr dev);
 
 int virNodeDeviceDettach(virNodeDevicePtr dev);
 int virNodeDeviceDetachFlags(virNodeDevicePtr dev,
diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c
index eb8c735a8c..fcca40f47b 100644
--- a/src/libvirt-nodedev.c
+++ b/src/libvirt-nodedev.c
@@ -445,19 +445,26 @@ virNodeDeviceListCaps(virNodeDevicePtr dev,
  * Drops a reference to the node device, freeing it if
  * this was the last reference.
  *
- * Returns the 0 for success, -1 for error.
+ * Throws a VIR_ERR_INVALID_NODE_DEVICE error if @dev is
+ * not a valid node device. Does nothing if @dev is
+ * NULL.
  */
-int
+void
 virNodeDeviceFree(virNodeDevicePtr dev)
 {
+if (!dev)
+return;
+
 VIR_DEBUG("dev=%p, conn=%p", dev, dev ? dev->conn : NULL);
 
 virResetLastError();
 
-virCheckNodeDeviceReturn(dev, -1);
+virCheckNodeDeviceGoto(dev, invalid_device);
 
 virObjectUnref(dev);
-return 0;
+
+ invalid_device:
+return;
 }
 
 
-- 
2.26.2

[PATCH 17/20] virpci.c: use virPCIDeviceAddressPtr in virPCIDeviceListFind()

[Daniel Henrique Barboza]

Signed-off-by: Daniel Henrique Barboza 
---
 src/hypervisor/virhostdev.c | 12 
 src/util/virpci.c   | 16 
 src/util/virpci.h   |  2 +-
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c
index 16a21f240b..e7ef615f60 100644
--- a/src/hypervisor/virhostdev.c
+++ b/src/hypervisor/virhostdev.c
@@ -657,7 +657,8 @@ virHostdevReattachAllPCIDevices(virHostdevManagerPtr mgr,
 
 /* We need to look up the actual device because that's what
  * virPCIDeviceReattach() expects as its argument */
-if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci)))
+if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs,
+virPCIDeviceGetAddress(pci
 continue;
 
 if (virPCIDeviceGetManaged(actual)) {
@@ -777,7 +778,8 @@ virHostdevPreparePCIDevicesImpl(virHostdevManagerPtr mgr,
 
 /* Unmanaged devices should already have been marked as
  * inactive: if that's the case, we can simply move on */
-if (virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci)) {
+if (virPCIDeviceListFind(mgr->inactivePCIHostdevs,
+ virPCIDeviceGetAddress(pci))) {
 VIR_DEBUG("Not detaching unmanaged PCI device %s",
   virPCIDeviceGetName(pci));
 continue;
@@ -860,7 +862,8 @@ virHostdevPreparePCIDevicesImpl(virHostdevManagerPtr mgr,
  * there because 'pci' only contain address information and will
  * be released at the end of the function */
 pci = virPCIDeviceListGet(pcidevs, i);
-actual = virPCIDeviceListFind(mgr->activePCIHostdevs, pci);
+actual = virPCIDeviceListFind(mgr->activePCIHostdevs,
+  virPCIDeviceGetAddress(pci));
 
 VIR_DEBUG("Setting driver and domain information for PCI device %s",
   virPCIDeviceGetName(pci));
@@ -992,7 +995,8 @@ virHostdevReAttachPCIDevicesImpl(virHostdevManagerPtr mgr,
  * information such as by which domain and driver it is used. As a
  * side effect, by looking it up we can also tell whether it was
  * really active in the first place */
-actual = virPCIDeviceListFind(mgr->activePCIHostdevs, pci);
+actual = virPCIDeviceListFind(mgr->activePCIHostdevs,
+  virPCIDeviceGetAddress(pci));
 if (actual) {
 const char *actual_drvname;
 const char *actual_domname;
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 122056dbe0..938ffeaaed 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -705,7 +705,7 @@ virPCIDeviceSharesBusWithActive(virPCIDevicePtr dev, 
virPCIDevicePtr check, void
 return 0;
 
 /* same bus, but inactive, i.e. about to be assigned to guest */
-if (inactiveDevs && virPCIDeviceListFind(inactiveDevs, check))
+if (inactiveDevs && virPCIDeviceListFind(inactiveDevs, &check->address))
 return 0;
 
 return 1;
@@ -1022,7 +1022,7 @@ virPCIDeviceReset(virPCIDevicePtr dev,
 return -1;
 }
 
-if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) {
+if (activeDevs && virPCIDeviceListFind(activeDevs, &dev->address)) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Not resetting active device %s"), dev->name);
 return -1;
@@ -1294,7 +1294,7 @@ virPCIDeviceDetach(virPCIDevicePtr dev,
 if (virPCIProbeStubDriver(dev->stubDriver) < 0)
 return -1;
 
-if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) {
+if (activeDevs && virPCIDeviceListFind(activeDevs, &dev->address)) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Not detaching active device %s"), dev->name);
 return -1;
@@ -1306,7 +1306,7 @@ virPCIDeviceDetach(virPCIDevicePtr dev,
 /* Add *a copy of* the dev into list inactiveDevs, if
  * it's not already there.
  */
-if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, dev)) {
+if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, &dev->address)) {
 VIR_DEBUG("Adding PCI device %s to inactive list", dev->name);
 if (virPCIDeviceListAddCopy(inactiveDevs, dev) < 0)
 return -1;
@@ -1324,7 +1324,7 @@ virPCIDeviceReattach(virPCIDevicePtr dev,
  virPCIDeviceListPtr activeDevs,
  virPCIDeviceListPtr inactiveDevs)
 {
-if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) {
+if (activeDevs && virPCIDeviceListFind(activeDevs, &dev->address)) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Not reattaching active device %s"), dev->name);
 return -1;
@@ -1684,7 +1684,7 @@ int
 virPCIDeviceListAdd(virPCIDeviceListPtr list,
 virPCIDevicePtr dev)
 {
-if

[PATCH 14/20] virhostdev.c: add virHostdevIsPCIDevice() helper

[Daniel Henrique Barboza]

Add a helper to quickly determine if a hostdev is a PCI device,
instead of doing a tedius 'if' check with hostdev mode and
subsys type.

Signed-off-by: Daniel Henrique Barboza 
---
 src/hypervisor/virhostdev.c | 12 +---
 src/hypervisor/virhostdev.h |  2 ++
 src/libvirt_private.syms|  1 +
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c
index 18373deb41..16a21f240b 100644
--- a/src/hypervisor/virhostdev.c
+++ b/src/hypervisor/virhostdev.c
@@ -347,12 +347,18 @@ virHostdevNetDevice(virDomainHostdevDefPtr hostdev,
 }
 
 
+bool
+virHostdevIsPCIDevice(const virDomainHostdevDef *hostdev)
+{
+return hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
+hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI;
+}
+
+
 static bool
 virHostdevIsPCINetDevice(const virDomainHostdevDef *hostdev)
 {
-return hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
-hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
-hostdev->parentnet != NULL;
+return virHostdevIsPCIDevice(hostdev) && hostdev->parentnet != NULL;
 }
 
 
diff --git a/src/hypervisor/virhostdev.h b/src/hypervisor/virhostdev.h
index 811bda40ed..b9407cd837 100644
--- a/src/hypervisor/virhostdev.h
+++ b/src/hypervisor/virhostdev.h
@@ -235,3 +235,5 @@ virHostdevUpdateActiveNVMeDevices(virHostdevManagerPtr 
hostdev_mgr,
   const char *dom_name,
   virDomainDiskDefPtr *disks,
   size_t ndisks);
+
+bool virHostdevIsPCIDevice(const virDomainHostdevDef *hostdev);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 81ce142635..ac72a384ed 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1461,6 +1461,7 @@ virCloseCallbacksUnset;
 
 # hypervisor/virhostdev.h
 virHostdevFindUSBDevice;
+virHostdevIsPCIDevice;
 virHostdevManagerGetDefault;
 virHostdevPCINodeDeviceDetach;
 virHostdevPCINodeDeviceReAttach;
-- 
2.26.2

[PATCH 16/20] virpci.c: use virPCIDeviceAddressPtr in virPCIDeviceListFindIndex()

[Daniel Henrique Barboza]

We're going to need a way to remove a PCI Device from a list without having
a valid virPCIDevicePtr, because the device is missing from the host. This
means that virPCIDevicesListDel() must operate with a PCI Device address
instead.

Turns out that virPCIDevicesListDel() and its related functions only use
the virPCIDeviceAddressPtr of the virPCIDevicePtr, so this change is
simple to do and will not cause hassle in all other callers. Let's
start adapting virPCIDeviceListFindIndex() and crawl our way up to
virPCIDevicesListDel().

Signed-off-by: Daniel Henrique Barboza 
---
 src/util/virpci.c | 15 ---
 src/util/virpci.h |  2 +-
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/src/util/virpci.c b/src/util/virpci.c
index 1a542b18b6..122056dbe0 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1745,7 +1745,7 @@ virPCIDevicePtr
 virPCIDeviceListSteal(virPCIDeviceListPtr list,
   virPCIDevicePtr dev)
 {
-return virPCIDeviceListStealIndex(list, virPCIDeviceListFindIndex(list, 
dev));
+return virPCIDeviceListStealIndex(list, virPCIDeviceListFindIndex(list, 
&dev->address));
 }
 
 void
@@ -1756,16 +1756,17 @@ virPCIDeviceListDel(virPCIDeviceListPtr list,
 }
 
 int
-virPCIDeviceListFindIndex(virPCIDeviceListPtr list, virPCIDevicePtr dev)
+virPCIDeviceListFindIndex(virPCIDeviceListPtr list,
+  virPCIDeviceAddressPtr devAddr)
 {
 size_t i;
 
 for (i = 0; i < list->count; i++) {
 virPCIDevicePtr other = list->devs[i];
-if (other->address.domain   == dev->address.domain &&
-other->address.bus  == dev->address.bus&&
-other->address.slot == dev->address.slot   &&
-other->address.function == dev->address.function)
+if (other->address.domain   == devAddr->domain &&
+other->address.bus  == devAddr->bus&&
+other->address.slot == devAddr->slot   &&
+other->address.function == devAddr->function)
 return i;
 }
 return -1;
@@ -1798,7 +1799,7 @@ virPCIDeviceListFind(virPCIDeviceListPtr list, 
virPCIDevicePtr dev)
 {
 int idx;
 
-if ((idx = virPCIDeviceListFindIndex(list, dev)) >= 0)
+if ((idx = virPCIDeviceListFindIndex(list, &dev->address)) >= 0)
 return list->devs[idx];
 else
 return NULL;
diff --git a/src/util/virpci.h b/src/util/virpci.h
index a9c597a428..8c6776da21 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -175,7 +175,7 @@ virPCIDeviceListFindByIDs(virPCIDeviceListPtr list,
   unsigned int slot,
   unsigned int function);
 int virPCIDeviceListFindIndex(virPCIDeviceListPtr list,
-  virPCIDevicePtr dev);
+  virPCIDeviceAddressPtr devAddr);
 
 /*
  * Callback that will be invoked once for each file
-- 
2.26.2

[PATCH 18/20] virpci.c: use virPCIDeviceAddressPtr in virPCIDeviceListSteal()

[Daniel Henrique Barboza]

Signed-off-by: Daniel Henrique Barboza 
---
 src/hypervisor/virhostdev.c | 8 +---
 src/util/virpci.c   | 6 +++---
 src/util/virpci.h   | 2 +-
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c
index e7ef615f60..4539c55e09 100644
--- a/src/hypervisor/virhostdev.c
+++ b/src/hypervisor/virhostdev.c
@@ -846,7 +846,7 @@ virHostdevPreparePCIDevicesImpl(virHostdevManagerPtr mgr,
 
 VIR_DEBUG("Removing PCI device %s from inactive list",
   virPCIDeviceGetName(pci));
-actual = virPCIDeviceListSteal(mgr->inactivePCIHostdevs, pci);
+actual = virPCIDeviceListSteal(mgr->inactivePCIHostdevs, 
virPCIDeviceGetAddress(pci));
 
 VIR_DEBUG("Adding PCI device %s to active list",
   virPCIDeviceGetName(pci));
@@ -918,7 +918,8 @@ virHostdevPreparePCIDevicesImpl(virHostdevManagerPtr mgr,
 
 VIR_DEBUG("Removing PCI device %s from active list",
   virPCIDeviceGetName(pci));
-if (!(actual = virPCIDeviceListSteal(mgr->activePCIHostdevs, pci)))
+if (!(actual = virPCIDeviceListSteal(mgr->activePCIHostdevs,
+ virPCIDeviceGetAddress(pci
 continue;
 
 VIR_DEBUG("Adding PCI device %s to inactive list",
@@ -1022,7 +1023,8 @@ virHostdevReAttachPCIDevicesImpl(virHostdevManagerPtr mgr,
 
 VIR_DEBUG("Removing PCI device %s from active list",
   virPCIDeviceGetName(pci));
-actual = virPCIDeviceListSteal(mgr->activePCIHostdevs, pci);
+actual = virPCIDeviceListSteal(mgr->activePCIHostdevs,
+   virPCIDeviceGetAddress(pci));
 
 VIR_DEBUG("Adding PCI device %s to inactive list",
   virPCIDeviceGetName(pci));
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 938ffeaaed..8973c7c754 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1743,16 +1743,16 @@ virPCIDeviceListStealIndex(virPCIDeviceListPtr list,
 
 virPCIDevicePtr
 virPCIDeviceListSteal(virPCIDeviceListPtr list,
-  virPCIDevicePtr dev)
+  virPCIDeviceAddressPtr devAddr)
 {
-return virPCIDeviceListStealIndex(list, virPCIDeviceListFindIndex(list, 
&dev->address));
+return virPCIDeviceListStealIndex(list, virPCIDeviceListFindIndex(list, 
devAddr));
 }
 
 void
 virPCIDeviceListDel(virPCIDeviceListPtr list,
 virPCIDevicePtr dev)
 {
-virPCIDeviceFree(virPCIDeviceListSteal(list, dev));
+virPCIDeviceFree(virPCIDeviceListSteal(list, &dev->address));
 }
 
 int
diff --git a/src/util/virpci.h b/src/util/virpci.h
index 628a293972..e3458a75fa 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -161,7 +161,7 @@ virPCIDevicePtr virPCIDeviceListGet(virPCIDeviceListPtr 
list,
 int idx);
 size_t virPCIDeviceListCount(virPCIDeviceListPtr list);
 virPCIDevicePtr virPCIDeviceListSteal(virPCIDeviceListPtr list,
-  virPCIDevicePtr dev);
+  virPCIDeviceAddressPtr devAddr);
 virPCIDevicePtr virPCIDeviceListStealIndex(virPCIDeviceListPtr list,
int idx);
 void virPCIDeviceListDel(virPCIDeviceListPtr list,
-- 
2.26.2

[PATCH 19/20] virpci.c: use virPCIDeviceAddressPtr in virPCIDeviceListDel()

[Daniel Henrique Barboza]

This change will allow us to remove PCI devices from a list
without the need of a PCI Device object, which will be need
in the next patch.

Signed-off-by: Daniel Henrique Barboza 
---
 src/hypervisor/virhostdev.c | 7 ---
 src/util/virpci.c   | 6 +++---
 src/util/virpci.h   | 2 +-
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c
index 4539c55e09..0510eb020c 100644
--- a/src/hypervisor/virhostdev.c
+++ b/src/hypervisor/virhostdev.c
@@ -1005,11 +1005,11 @@ virHostdevReAttachPCIDevicesImpl(virHostdevManagerPtr 
mgr,
 if (STRNEQ_NULLABLE(drv_name, actual_drvname) ||
 STRNEQ_NULLABLE(dom_name, actual_domname)) {
 
-virPCIDeviceListDel(pcidevs, pci);
+virPCIDeviceListDel(pcidevs, virPCIDeviceGetAddress(pci));
 continue;
 }
 } else {
-virPCIDeviceListDel(pcidevs, pci);
+virPCIDeviceListDel(pcidevs, virPCIDeviceGetAddress(pci));
 continue;
 }
 
@@ -2524,7 +2524,8 @@ virHostdevUpdateActiveNVMeDevices(virHostdevManagerPtr 
hostdev_mgr,
 while (lastGoodPCIIdx >= 0) {
 virPCIDevicePtr actual = virPCIDeviceListGet(pciDevices, i);
 
-virPCIDeviceListDel(hostdev_mgr->activePCIHostdevs, actual);
+virPCIDeviceListDel(hostdev_mgr->activePCIHostdevs,
+virPCIDeviceGetAddress(actual));
 
 lastGoodPCIIdx--;
 }
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 8973c7c754..cbf49357c6 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1336,7 +1336,7 @@ virPCIDeviceReattach(virPCIDevicePtr dev,
 /* Steal the dev from list inactiveDevs */
 if (inactiveDevs) {
 VIR_DEBUG("Removing PCI device %s from inactive list", dev->name);
-virPCIDeviceListDel(inactiveDevs, dev);
+virPCIDeviceListDel(inactiveDevs, &dev->address);
 }
 
 return 0;
@@ -1750,9 +1750,9 @@ virPCIDeviceListSteal(virPCIDeviceListPtr list,
 
 void
 virPCIDeviceListDel(virPCIDeviceListPtr list,
-virPCIDevicePtr dev)
+virPCIDeviceAddressPtr devAddr)
 {
-virPCIDeviceFree(virPCIDeviceListSteal(list, &dev->address));
+virPCIDeviceFree(virPCIDeviceListSteal(list, devAddr));
 }
 
 int
diff --git a/src/util/virpci.h b/src/util/virpci.h
index e3458a75fa..eb71eb4451 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -165,7 +165,7 @@ virPCIDevicePtr virPCIDeviceListSteal(virPCIDeviceListPtr 
list,
 virPCIDevicePtr virPCIDeviceListStealIndex(virPCIDeviceListPtr list,
int idx);
 void virPCIDeviceListDel(virPCIDeviceListPtr list,
- virPCIDevicePtr dev);
+ virPCIDeviceAddressPtr devAddr);
 virPCIDevicePtr virPCIDeviceListFind(virPCIDeviceListPtr list,
  virPCIDeviceAddressPtr devAddr);
 virPCIDevicePtr
-- 
2.26.2

[PATCH 20/20] virhostdev.c: remove missing PCI devs from hostdev manager

[Daniel Henrique Barboza]

virHostdevReAttachPCIDevices() is called when we want to re-attach
a list of hostdevs back to the host, either on the shutdown path or
via a 'virsh detach-device' call.  This function always count on the
existence of the device in the host to work, but this can lead to
problems. For example, a SR-IOV device can be removed via an admin
"echo 0 > /sys/bus/pci/devices//sriov_numvfs", making the kernel
fire up and eventfd_signal() to the process, asking for the process to
release the device. The result might vary depending on the device driver
and OS/arch, but two possible outcomes are:

1) the hypervisor driver will detach the device from the VM, issuing a
delete event to Libvirt. This can be observed in QEMU;

2) the 'echo 0 > ...' will hang waiting for the device to be unplugged.
This means that the VM process failed/refused to release the hostdev back
to the host, and the hostdev will be detached during VM shutdown.

Today we don't behave well for both cases. We'll fail to remove the PCI device
reference from mgr->activePCIHostdevs and mgr->inactivePCIHostdevs because
we rely on the existence of the PCI device conf file in the sysfs. Attempting
to re-utilize the same device (assuming it is now present back in the host)
can result in an error like this:

$ ./run tools/virsh start vm1-sriov --console
error: Failed to start domain vm1-sriov
error: Requested operation is not valid: PCI device :01:00.2 is in use by 
driver QEMU, domain vm1-sriov

For (1), a VM destroy/start cycle is needed to re-use the VF in the guest.
For (2), the effect is more nefarious, requiring a Libvirtd daemon restart
to use the VF again in any guest.

We can make it a bit better by checking, during virHostdevReAttachPCIDevices(),
if there is any missing PCI device that will be left behind in activePCIHostdevs
and inactivePCIHostdevs lists. Remove any missing device found from both lists,
unconditionally, matching the current state of the host. This change affects
the code path in (1) (processDeviceDeletedEvent into qemuDomainRemoveDevice, all
the way back to qemuHostdevReAttachPCIDevices) and also in (b) (qemuProcessStop
into qemuHostdevReAttachDomainDevices).

NB: this patch does **NOT** intend to implement support for surprise hotunplug
of PCI devices in Libvirt. Although this can now be achieved if the hypervisor
and the PCI driver copes with it, our goal is to mitigate what it is still 
considered
an user oopsie. For all supported purposes, the admin must remove the SR-IOV VFs
from all running domains before removing the VFs from the host.

Resolves:  https://gitlab.com/libvirt/libvirt/-/issues/72
Signed-off-by: Daniel Henrique Barboza 
---
 src/hypervisor/virhostdev.c | 38 +
 1 file changed, 38 insertions(+)

diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c
index 0510eb020c..50fe29f142 100644
--- a/src/hypervisor/virhostdev.c
+++ b/src/hypervisor/virhostdev.c
@@ -1077,6 +1077,40 @@ virHostdevReAttachPCIDevicesImpl(virHostdevManagerPtr 
mgr,
 }
 
 
+static void
+virHostdevDeleteMissingPCIDevices(virHostdevManagerPtr mgr,
+  virDomainHostdevDefPtr *hostdevs,
+  int nhostdevs)
+{
+size_t i;
+
+if (nhostdevs == 0)
+return;
+
+virObjectLock(mgr->activePCIHostdevs);
+virObjectLock(mgr->inactivePCIHostdevs);
+
+for (i = 0; i < nhostdevs; i++) {
+virDomainHostdevDef *hostdev = hostdevs[i];
+virDomainHostdevSubsysPCI *pcisrc = &hostdev->source.subsys.u.pci;
+g_autoptr(virPCIDevice) pci = NULL;
+
+if (virHostdevGetPCIHostDevice(hostdev, &pci) != -2)
+continue;
+
+/* The PCI device from 'hostdev' does not exist in the host
+ * anymore. Delete it from both active and inactive lists to
+ * reflect the current host state.
+ */
+virPCIDeviceListDel(mgr->activePCIHostdevs, &pcisrc->addr);
+virPCIDeviceListDel(mgr->inactivePCIHostdevs, &pcisrc->addr);
+}
+
+virObjectUnlock(mgr->activePCIHostdevs);
+virObjectUnlock(mgr->inactivePCIHostdevs);
+}
+
+
 /* @oldStateDir:
  * For upgrade purpose: see virHostdevRestoreNetConfig
  */
@@ -1102,6 +1136,10 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr,
 
 virHostdevReAttachPCIDevicesImpl(mgr, drv_name, dom_name, pcidevs,
  hostdevs, nhostdevs, oldStateDir);
+
+/* Handle the case where PCI devices from the host went missing
+ * during the domain lifetime */
+virHostdevDeleteMissingPCIDevices(mgr, hostdevs, nhostdevs);
 }
 
 
-- 
2.26.2

Re: [PATCH 2/7] util: Added nfs params to virStorageSource

[Peter Krempa]

On Tue, Dec 29, 2020 at 15:21:24 -0600, Ryan Gahagan wrote:
> Signed-off-by: Ryan Gahagan 
> ---
>  src/util/virstoragefile.c | 8 
>  src/util/virstoragefile.h | 5 +
>  2 files changed, 13 insertions(+)

[...]

> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> index c5d5f0233a..64fc519f87 100644
> --- a/src/util/virstoragefile.h
> +++ b/src/util/virstoragefile.h
> @@ -385,6 +385,11 @@ struct _virStorageSource {
>  /* these must not be used apart from formatting the output JSON in the 
> qemu driver */
>  char *ssh_user;
>  bool ssh_host_key_check_disabled;
> +
> +char *nfs_user;
> +char *nfs_group;
> +uid_t nfs_uid;
> +gid_t nfs_gid;

Please add comments for nfs_uid and nfs_gid which state that the
variables hold the converted/looked up uid/gid which is used at runtinme
. This will help any further reader of the code see what they are used
for and that they are not actually based on the configuration but are
rather runtime (such as when we'll be adding driver-specific private
data for virStorageSource).

[PATCH] kbase: debuglogs: Fix typo in unprivileged libvirtd config path

[Tomáš Janoušek]

Signed-off-by: Tomáš Janoušek 

---
 docs/kbase/debuglogs.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/kbase/debuglogs.rst b/docs/kbase/debuglogs.rst
index f3937bc337..3fb06bedc3 100644
--- a/docs/kbase/debuglogs.rst
+++ b/docs/kbase/debuglogs.rst
@@ -68,7 +68,7 @@ variable:
 
 However, when you are using the session mode ``qemu:///session`` or you run the
 ``libvirtd`` as unprivileged user you will find configuration file under
-``$XDG_CONFIG_HOME/libvirt/libvirt.conf``.
+``$XDG_CONFIG_HOME/libvirt/libvirtd.conf``.
 
 Runtime setting
 ^^^

-- 
Tomáš Janoušek, a.k.a. Pivník, a.k.a. Liskni_si, https://work.lisk.in/

[PATCH] Fix wrong use of path variable

[liyalei]

From: liyalei 

---
 src/util/virnetdevopenvswitch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
index d380b0cf22..7eabaa763d 100644
--- a/src/util/virnetdevopenvswitch.c
+++ b/src/util/virnetdevopenvswitch.c
@@ -494,7 +494,7 @@ virNetDevOpenvswitchGetVhostuserIfname(const char *path,
 if (server) {
 virCommandAddArgList(cmd, "--no-headings", "--columns=name", "find",
  "Interface", NULL);
-virCommandAddArgPair(cmd, "options:vhost-server-path", "path");
+virCommandAddArgPair(cmd, "options:vhost-server-path", path);
 } else {
 const char *tmpIfname = NULL;
 
-- 
2.27.0

Re: [PATCH] Fix wrong use of path variable

[Peter Krempa]

On Wed, Dec 30, 2020 at 18:39:35 +0800, liyalei wrote:
> From: liyalei 

Please provide some more description in the commit message, such as
describe the bug that this patch is fixing.

Additionally the libvirt project requires [1] you to certify that you
are in compliance of the 'Developer certificate of origin' [2] by
providing the appropriate tag in the commit message (read the docs to
figure out which).

[1]: https://libvirt.org/hacking.html#developer-certificate-of-origin
[2]: https://developercertificate.org/

> 
> ---
>  src/util/virnetdevopenvswitch.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
> index d380b0cf22..7eabaa763d 100644
> --- a/src/util/virnetdevopenvswitch.c
> +++ b/src/util/virnetdevopenvswitch.c
> @@ -494,7 +494,7 @@ virNetDevOpenvswitchGetVhostuserIfname(const char *path,
>  if (server) {
>  virCommandAddArgList(cmd, "--no-headings", "--columns=name", "find",
>   "Interface", NULL);
> -virCommandAddArgPair(cmd, "options:vhost-server-path", "path");
> +virCommandAddArgPair(cmd, "options:vhost-server-path", path);

Looks like this is already fixed:

commit 0dd029b7f2e7d9619ca6a22f43857aafb449c3a7
Author: Michal Prívozník 
Date:   Wed Dec 16 18:52:48 2020 +0100

virNetDevOpenvswitchGetVhostuserIfname: Actually use @path to lookup 
interface

In v6.10.0-rc1~221 I wanted to make virNetDevOpenvswitchGetVhostuserIfname()
lookup interface name even for vhostuser interfaces with mode='server'. For
these, we are given a socket path which is then created by QEMU and to which
OpenVSwitch connects to and creates an interface. Because of this, we don't
know the name of the interface upfront (when starting QEMU) and have to use
the path to query OpenVSwitch later (using ovs-vsctl). What I intended to 
use
was:

  ovs-vsctl --no-headings --columns=name find Interface 
options:vhost-server-path=$path

But what my code does is:

  ovs-vsctl --no-headings --columns=name find Interface 
options:vhost-server-path=path

and it's all because the argument to the function is named "path"
which I then enclosed in double quotes while it should have been
used as a variable.

Fixes: e4c29e2904197472919d050c67acfd59f0144bbc
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1767013
Signed-off-by: Michal Privoznik 
Reviewed-by: Laine Stump 

diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
index d380b0cf22..7eabaa763d 100644
--- a/src/util/virnetdevopenvswitch.c
+++ b/src/util/virnetdevopenvswitch.c
@@ -494,7 +494,7 @@ virNetDevOpenvswitchGetVhostuserIfname(const char *path,
 if (server) {
 virCommandAddArgList(cmd, "--no-headings", "--columns=name", "find",
  "Interface", NULL);
-virCommandAddArgPair(cmd, "options:vhost-server-path", "path");
+virCommandAddArgPair(cmd, "options:vhost-server-path", path);
 } else {
 const char *tmpIfname = NULL;


>  } else {
>  const char *tmpIfname = NULL;
>  
> -- 
> 2.27.0
> 

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

[Daniel P . Berrangé]

On Tue, Dec 29, 2020 at 03:21:25PM -0600, Ryan Gahagan wrote:
> Signed-off-by: Ryan Gahagan 
> ---
>  docs/formatdomain.rst | 11 +-
>  docs/schemas/domaincommon.rng | 38 +++
>  2 files changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index 512939679b..23a7bca643 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -2370,6 +2370,14 @@ paravirtualized driver is specified via the ``disk`` 
> element.
> 
> 
>   
> + 
> +   
> +   
> + 
> + 

I'd be inclined to call this  not 

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 3/7] docs: added rng schema and formatdomain for NFS

[Peter Krempa]

On Mon, Jan 04, 2021 at 13:19:25 +, Daniel Berrange wrote:
> On Tue, Dec 29, 2020 at 03:21:25PM -0600, Ryan Gahagan wrote:
> > Signed-off-by: Ryan Gahagan 
> > ---
> >  docs/formatdomain.rst | 11 +-
> >  docs/schemas/domaincommon.rng | 38 +++
> >  2 files changed, 48 insertions(+), 1 deletion(-)
> > 
> > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> > index 512939679b..23a7bca643 100644
> > --- a/docs/formatdomain.rst
> > +++ b/docs/formatdomain.rst
> > @@ -2370,6 +2370,14 @@ paravirtualized driver is specified via the ``disk`` 
> > element.
> > 
> > 
> >   
> > + 
> > +   
> > +   
> > + 
> > + 
> 
> I'd be inclined to call this  not 

Yeah, that seems a bit better

Re: [PATCH 00/10] Fix NSS plugin and net-dhcp-get-leases wrt to infinite leases

[Daniel P . Berrangé]

On Fri, Dec 18, 2020 at 04:09:06PM +0100, Michal Privoznik wrote:
> Some things are broken when using leases that don't expire. We don't
> store "expiry-time" in corresponding $brname.status file which sets off
> a spiral and we get errors from other places which expect it to be there
> always. These patches make sure that the attribute is always there. I've
> also implemented another approach, which puts "expiry-time" into the
> file only if not infinite and fixed the other places which expect it:
> 
>   https://gitlab.com/MichalPrivoznik/libvirt/-/commits/leases_docs/
> 
> but I like this version more.
> 
> Michal Prívozník (10):
>   docs: Document ability to configure lease time
>   leaseshelper: Report errors on failure
>   virlease: Rework virLeaseReadCustomLeaseFile()
>   virlease: Use virTrimSpaces() instead of open coded alternative
>   virlease: Allow infinite lease expiry time
>   network: Drop @custom_lease_file_len variable from
> networkGetDHCPLeases()
>   networkGetDHCPLeases: Use VIR_APPEND_ELEMENT() instead of
> VIR_INSERT_ELEMENT()
>   network: Rework networkGetDHCPLeases()
>   networkGetDHCPLeases: Handle leases with infinite expiry time
>   nss: handle leases with infinite expiry time
> 
>  docs/formatnetwork.html.in | 21 -
>  src/network/bridge_driver.c| 79 +-
>  src/network/leaseshelper.c |  2 +
>  src/util/virlease.c| 33 +++---
>  tests/nssdata/virbr0.status|  7 +++
>  tests/nsstest.c|  2 +-
>  tools/nss/libvirt_nss_leases.c |  4 +-
>  7 files changed, 87 insertions(+), 61 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: [PATCH] qemu: Drop has_ccw_address from _qemuAgentDiskAddress

[Erik Skultety]

On Mon, Jan 04, 2021 at 09:54:08AM +0100, Michal Privoznik wrote:
> In recent patches new mambers to _qemuAgentDiskAddress struct
> were introduced to keep optional CCW address sent by the guest
> agent. These two members are a struct to store CCW address into
> and a boolean to keep track whether the CCW address is valid.
> Well, we can hold the same information with a pointer - instead
> of storing the CCW address structure let's keep just a pointer to
> it.
> 
> Signed-off-by: Michal Privoznik 
> ---
Reviewed-by: Erik Skultety 

Re: [PATCH] kbase: debuglogs: Fix typo in unprivileged libvirtd config path

[Erik Skultety]

On Mon, Jan 04, 2021 at 11:35:02AM +0100, Tomáš Janoušek wrote:
> Signed-off-by: Tomáš Janoušek 
> 
> ---
Reviewed-by: Erik Skultety 

Congratulations on your first libvirt contribution.

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

[Peter Krempa]

On Tue, Dec 29, 2020 at 15:21:25 -0600, Ryan Gahagan wrote:
> Signed-off-by: Ryan Gahagan 
> ---
>  docs/formatdomain.rst | 11 +-
>  docs/schemas/domaincommon.rng | 38 +++
>  2 files changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index 512939679b..23a7bca643 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -2370,6 +2370,14 @@ paravirtualized driver is specified via the ``disk`` 
> element.
> 
> 
>   
> + 
> +   
> +   
> + 
> + 
> +   
> +   
> + 
>   
> 
>  name='iqn.2013-07.com.example:iscsi-nopool/0'>
> @@ -2491,7 +2499,7 @@ paravirtualized driver is specified via the ``disk`` 
> element.
> ``network``
>The ``protocol`` attribute specifies the protocol to access to the
>requested image. Possible values are "nbd", "iscsi", "rbd", "sheepdog",
> -  "gluster", "vxhs", "http", "https", "ftp", ftps", or "tftp".
> +  "gluster", "vxhs", "nfs", "http", "https", "ftp", ftps", or "tftp".

The docs is missing any description of the user/group fields. Please
include the syntax as well.

Additionally we tend to include any caveats of the initial
implementation. In this case you should include that NFS-backed disks
work only with TCP and port must be omitted.

>  
>For any ``protocol`` other than ``nbd`` an additional attribute 
> ``name``
>is mandatory to specify which volume/image will be used.
> @@ -2601,6 +2609,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 daemonone 
> 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 Systemonly 
> one ( :since:`Since 7.0.0` )2049

You've dropped the code which sets the default port so this isn't
accurate.


> === 
>  
>  
>gluster supports "tcp", "rdma", "unix" as valid values for the 
> transport
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 17e25f14f2..4ddbf13ec4 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1774,6 +1774,27 @@
>  
>
>  
> +  
> +

This will need changes after Daniel's suggestion.

> +  
> +
> +  
> +

Note that 'unsignedInt' doesn't actually allow the '+' prefix.

> +

But both numbers and strings started with + are covered by genericName,
so the choice isn't required.

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

Same here.

> +  
> +
> +  
> +
> +  
> +
>
>  
>
> @@ -2039,6 +2060,22 @@
>  
>
>  
> +  
> +
> +  
> +
> +  
> +nfs
> +  
> +
> +
> +
> +

This allows also 'port' and non TCP transports. It's okay to simplify
the schema though and use the generic type. The code will need to reject
those when we'll validate whether the disk is possible to represent for
qemu.

Re: [PATCH 4/7] conf: Added NFS XML format/parse methods

[Peter Krempa]

On Tue, Dec 29, 2020 at 15:21:26 -0600, Ryan Gahagan wrote:
> Signed-off-by: Ryan Gahagan 
> ---
>  src/conf/domain_conf.c | 33 +
>  1 file changed, 33 insertions(+)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index b301ac0a08..565ca680c9 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c

This patch can be merged into the previous one if you want.

> @@ -23805,6 +23825,19 @@ virDomainDiskSourceFormatNetwork(virBufferPtr 
> attrBuf,
>  virBufferAddLit(childBuf, "/>\n");
>  }
>  
> +if (src->protocol == VIR_STORAGE_NET_PROTOCOL_NFS &&
> +(src->nfs_user || src->nfs_group)) {
> +virBufferAddLit(childBuf, " +
> +if (src->nfs_user)

virBufferEscapeString has a special corner-case that it doesn't format
anything if the third argument is NULL, so the explicit check is not
necessary.

> +virBufferEscapeString(childBuf, " user='%s'", src->nfs_user);
> +if (src->nfs_group)
> +virBufferEscapeString(childBuf, " group='%s'", src->nfs_group);
> +
> +virBufferAddLit(childBuf, "/>\n");
> +}

With the code simplified:

Reviewed-by: Peter Krempa 

Re: [PATCH 5/7] qemu: Added NFS JSON props methods

[Peter Krempa]

On Tue, Dec 29, 2020 at 15:21:27 -0600, Ryan Gahagan wrote:
> Signed-off-by: Ryan Gahagan 
> ---
>  src/qemu/qemu_block.c  | 79 +-
>  src/qemu/qemu_domain.c | 47 +
>  2 files changed, 125 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> index b224a550f3..5413a4f0e8 100644
> --- a/src/qemu/qemu_block.c
> +++ b/src/qemu/qemu_block.c
> @@ -574,6 +574,35 @@ 
> qemuBlockStorageSourceBuildJSONInetSocketAddress(virStorageNetHostDefPtr host)
>  }
>  
>  
> +/**
> + * qemuBlockStorageSourceBuildJSONNFSServer(virStorageNetHostDefPtr host)
> + * @host: the virStorageNetHostDefPtr definition to build
> + *
> + * Formats @hosts into a json object conforming to the 'NFSServer' type
> + * in qemu.
> + *
> + * Returns a virJSONValuePtr for a single server.
> + */
> +static virJSONValuePtr
> +qemuBlockStorageSourceBuildJSONNFSServer(virStorageNetHostDefPtr host)
> +{
> +virJSONValuePtr ret = NULL;
> +
> +if (host->transport != VIR_STORAGE_NET_HOST_TRANS_TCP) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("only TCP protocol can be converted to NFSServer"));
> +return NULL;

This check belongs into the validation function.

> +}
> +
> +ignore_value(virJSONValueObjectCreate(&ret,
> +  "s:host", host->name,
> +  "s:type", "inet",
> +  NULL));
> +
> +return ret;
> +}
> +
> +
>  /**
>   * qemuBlockStorageSourceBuildHostsJSONInetSocketAddress:
>   * @src: disk storage source
> @@ -674,6 +703,44 @@ qemuBlockStorageSourceGetVxHSProps(virStorageSourcePtr 
> src,
>  }
>  
>  
> +static virJSONValuePtr
> +qemuBlockStorageSourceGetNFSProps(virStorageSourcePtr src)
> +{
> +g_autoptr(virJSONValue) server = NULL;
> +virJSONValuePtr ret = NULL;
> +
> +if (src->nhosts != 1) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("NFS protocol accepts only one host"));
> +return NULL;
> +}

This check belongs to the validation function.

> +
> +if (!(server = qemuBlockStorageSourceBuildJSONNFSServer(&src->hosts[0])))
> +return NULL;
> +
> +/* NFS disk specification example:
> + * { driver:"nfs",
> + *   user: "0",
> + *   group: "0",
> + *   path: "/foo/bar/baz",
> + *   server: {type:"tcp", host:"1.2.3.4"}}
> + */
> +ignore_value(virJSONValueObjectCreate(&ret,
> +  "a:server", &server,
> +  "S:path", src->path, NULL));
> +
> +if (src->nfs_uid != -1 &&
> +virJSONValueObjectAdd(ret, "i:user", src->nfs_uid, NULL) < 0)
> +return NULL;
> +
> +if (src->nfs_gid != -1 &&
> +virJSONValueObjectAdd(ret, "i:group", src->nfs_gid, NULL) < 0)
> +return NULL;
> +
> +return ret;
> +}
> +
> +
>  static virJSONValuePtr
>  qemuBlockStorageSourceGetCURLProps(virStorageSourcePtr src,
> bool onlytarget)
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index d91c32b2c5..8812df5138 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4626,6 +4626,14 @@ qemuDomainValidateStorageSource(virStorageSourcePtr 
> src,
>  return -1;
>  }
>  
> +/* NFS protocol may only be used if current QEMU supports blockdev */
> +if (actualType == VIR_STORAGE_TYPE_NETWORK &&
> +src->protocol == VIR_STORAGE_NET_PROTOCOL_NFS &&
> +!blockdev) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("'nfs' protocol is not supported with this QEMU 
> binary"));

Here you must also check that the 'port' is not provided and that the
protocol is TCP. Otherwise you'd accept configurations which can't be
represented in qemu silently.


> +}
> +
>  return 0;
>  }
>  
> @@ -9590,6 +9598,42 @@ 
> qemuProcessPrepareStorageSourceTLSNBD(virStorageSourcePtr src,
>  }
>  
>  
> +/* qemuPrepareStorageSourceNFS:
> + * @src: source for a disk
> + *
> + * If src is an NFS source, translate nfs_user and nfs_group
> + * into a uid and gid field. If these strings are empty (ie "")
> + * then provide the hypervisor default uid and gid.
> + */
> +static int
> +qemuDomainPrepareStorageSourceNFS(virStorageSourcePtr src)
> +{
> +if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK)
> +return 0;
> +
> +if ((virStorageNetProtocol) src->protocol != 
> VIR_STORAGE_NET_PROTOCOL_NFS)

The typecast to virStorageNetProtocol is required only when you want to
use a 'switch' statement so that the complier checks that all cases are
handled. It's not necessary in a single if-condition.

> +return 0;
> +
> +if (src->nfs_user) {
> +if (virGetUserID(src->nfs_user, &src->nfs_uid) < 0)
> +return -1;
> +} else {

Re: [PATCH 6/7] util: Added a backing store NFS parser

[Peter Krempa]

On Tue, Dec 29, 2020 at 15:21:28 -0600, Ryan Gahagan wrote:
> Signed-off-by: Ryan Gahagan 
> ---
>  src/util/virstoragefile.c | 51 +++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index b5a5f267b9..ee8e31ce58 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -3805,6 +3805,56 @@ 
> virStorageSourceParseBackingJSONVxHS(virStorageSourcePtr src,
>  }
>  
>  
> +static int
> +virStorageSourceParseBackingJSONNFS(virStorageSourcePtr src,
> +virJSONValuePtr json,
> +const char *jsonstr G_GNUC_UNUSED,
> +int opaque G_GNUC_UNUSED)
> +{
> +virJSONValuePtr server = virJSONValueObjectGetObject(json, "server");
> +int uidStore = -1;
> +int gidStore = -1;
> +int gotUID = virJSONValueObjectGetNumberInt(json, "user", &uidStore);
> +int gotGID = virJSONValueObjectGetNumberInt(json, "group", &gidStore);
> +g_auto(virBuffer) userBuf = VIR_BUFFER_INITIALIZER;
> +g_auto(virBuffer) groupBuf = VIR_BUFFER_INITIALIZER;
> +
> +if (!server) {
> +virReportError(VIR_ERR_INVALID_ARG, "%s",
> +   _("missing 'server' attribute in JSON backing 
> definition for NFS volume"));
> +return -1;
> +}
> +
> +if (gotUID < 0 || gotGID < 0) {
> +virReportError(VIR_ERR_INVALID_ARG, "%s",
> +   _("missing 'user' or 'group' attribute in JSON 
> backing definition for NFS volume"));
> +return -1;
> +}
> +
> +src->path = g_strdup(virJSONValueObjectGetString(json, "path"));

'path' is mandatory in 'BlockdevOptionsNfs' thus you must check that
it's present.

> +
> +src->nfs_uid = (uid_t) uidStore;
> +src->nfs_gid = (gid_t) gidStore;

This function must not fill in runtime data, just configuration. I
presume you did this to silence tests but you'll need to add a hack into
the test code rather than abusing this to fill runtime data.

Ideally in the future the runtime data will be split off into an opaque
sub-object so it will not be accessible in this code. Don't touch
nfs_uid/nfs_gid in this function at all.

> +
> +virBufferAsprintf(&userBuf, "+%d", src->nfs_uid);
> +virBufferAsprintf(&groupBuf, "+%d", src->nfs_gid);
> +src->nfs_user = g_strdup(virBufferCurrentContent(&userBuf));
> +src->nfs_group = g_strdup(virBufferCurrentContent(&groupBuf));

This is overkill including a pointless copy of the string.
Replace it by:

  src->nfs_user = g_strdup_printf("+%d", uidStore);
  ...


> +
> +src->type = VIR_STORAGE_TYPE_NETWORK;
> +src->protocol = VIR_STORAGE_NET_PROTOCOL_NFS;
> +
> +src->hosts = g_new0(virStorageNetHostDef, 1);
> +src->nhosts = 1;
> +
> +if (virStorageSourceParseBackingJSONInetSocketAddress(src->hosts,
> +  server) < 0)
> +return -1;
> +
> +return 0;
> +}
> +
> +
>  static int
>  virStorageSourceParseBackingJSONNVMe(virStorageSourcePtr src,
>   virJSONValuePtr json,
> @@ -3864,6 +3914,7 @@ static const struct virStorageSourceJSONDriverParser 
> jsonParsers[] = {
>  {"ssh", false, virStorageSourceParseBackingJSONSSH, 0},
>  {"rbd", false, virStorageSourceParseBackingJSONRBD, 0},
>  {"raw", true, virStorageSourceParseBackingJSONRaw, 0},
> +{"nfs", false, virStorageSourceParseBackingJSONNFS, 0},
>  {"vxhs", false, virStorageSourceParseBackingJSONVxHS, 0},
>  {"nvme", false, virStorageSourceParseBackingJSONNVMe, 0},

The test case which tests this addition should be part of this patch
rather than stashed in the separate patch at the end of the series.

Re: Release of libvirt-6.5.0

[Andrea Bolognani]

On Fri, 2020-07-03 at 09:56 +0200, Daniel Veillard wrote:
>   Half a day late, but I pushed the 6.5.0 release out, it is as usual
> available as a signed tarball and source rpms from the server:
> 
> https://libvirt.org/sources/
> 
> I also tagged and pushed the 6.5.0 python bindings that one can find at
> 
> https://libvirt.org/sources/python/
> 
> This release includes a number of new features and some improvement,
> as well as a crash which had made its way in 6.4.0.
> It will also be my last release of libvirt after close to 15 years,
> so expect new releases to be signed by Jiri Denemark from now on.

Hi Daniel,

unfortunately the way the handover of release responsibilities was
handled is not considered up to scratch for at least one downstream
community, Arch Linux.

More specifically, since no formal trust path between your PGP key
and Jiri's has been established, the Arch maintainers are not
comfortable updating the package past 6.5.0. The situation has been
in a standstill for several months now, and Arch users are
increasingly suffering from it. See [1] and following comments for
more details on this.

Can you and Jiri (CC'd) please get in touch and arrange for his key
to be signed with yours, so that a proper trust path is established?
I would really love to see this resolved once and for all.

Thanks in advance to both of you!


[1] https://bugs.archlinux.org/task/67921#comment193381
-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH 7/7] tests: Added tests for NFS disk protocol

[Peter Krempa]

On Tue, Dec 29, 2020 at 15:21:29 -0600, Ryan Gahagan wrote:
> Signed-off-by: Ryan Gahagan 
> ---
>  tests/qemuxml2argvdata/disk-network-nfs.args  |  1 +
>  .../disk-network-nfs.x86_64-latest.args   | 56 +++
>  tests/qemuxml2argvdata/disk-network-nfs.xml   | 48 
>  tests/qemuxml2argvtest.c  |  1 +
>  ...isk-network-nfs-inactive.x86_64-latest.xml | 54 ++
>  .../disk-network-nfs.x86_64-latest.xml| 54 ++
>  tests/qemuxml2xmltest.c   |  1 +
>  tests/virstoragetest.c| 13 +
>  8 files changed, 228 insertions(+)
>  create mode 100644 tests/qemuxml2argvdata/disk-network-nfs.args
>  create mode 100644 tests/qemuxml2argvdata/disk-network-nfs.x86_64-latest.args
>  create mode 100644 tests/qemuxml2argvdata/disk-network-nfs.xml
>  create mode 100644 
> tests/qemuxml2xmloutdata/disk-network-nfs-inactive.x86_64-latest.xml
>  create mode 100644 
> tests/qemuxml2xmloutdata/disk-network-nfs.x86_64-latest.xml
> 
> diff --git a/tests/qemuxml2argvdata/disk-network-nfs.args 
> b/tests/qemuxml2argvdata/disk-network-nfs.args
> new file mode 100644
> index 00..fdc2941925
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/disk-network-nfs.args
> @@ -0,0 +1 @@
> +qemu_nfs

This file is not used by the tests, drop it.

[...]

> diff --git a/tests/qemuxml2argvdata/disk-network-nfs.xml 
> b/tests/qemuxml2argvdata/disk-network-nfs.xml
> new file mode 100644
> index 00..67d2843e01
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/disk-network-nfs.xml
> @@ -0,0 +1,48 @@
> +
> +  QEMUGuest1
> +  c7a5fdbd-edaf-9455-926a-d65c16db1809
> +  219136
> +  219136
> +  1
> +  
> +hvm
> +
> +  
> +  
> +  destroy
> +  restart
> +  destroy
> +  
> +/usr/bin/qemu-system-x86_64
> +
> +  
> +  
> +

'port' can't be controlled so you shouldn't add it into the XML.

> +
> +  
> +  
> +  eb90327c-8302-4725-9e1b-4e85ed4dc251
> +   function='0x0'/>
> +
> +
> +  
> +  
> +
> +  
> +  
> +
> +
> +  
> +  
> +
> +
> +  
> +  
> +
> +
> +
> +
> +
> +
> +  
> +

[...]

> diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
> index 2e466ecb99..08e2723235 100644
> --- a/tests/virstoragetest.c
> +++ b/tests/virstoragetest.c
> @@ -1630,6 +1630,19 @@ mymain(void)
> " name='c6718f6b-0401-441d-a8c3-1f0064d75ee0'>\n"
> "  \n"
> "\n");
> +TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"nfs\","
> +   "\"user\":2,"
> +   "\"group\":9,"
> +   "\"path\":\"/foo/bar/baz\","
> +   "\"server\": {  \"host\":\"example.com\","
> +  "\"port\":\"2049\""

'port' contradicts the QMP schema, so this snippet is wrong. Also the
transporrt type is missing.

As noted in the patch adding the backing store parser code, this also
belongs to that patch.

> +   "}"
> +  "}"
> +"}",
> +   "\n"
> +   "  \n"
> +   "  \n"
> +   "\n");
>  TEST_BACKING_PARSE_FULL("json:{ \"driver\": \"raw\","
>  "\"offset\": 10752,"
>  "\"size\": 4063232,"
> -- 
> 2.29.2
> 

Re: [PATCH 1/7] conf: Add NFS disk protocol

[Peter Krempa]

On Tue, Dec 29, 2020 at 15:21:23 -0600, Ryan Gahagan wrote:
> Per Issue 90, Libvirt does not support attaching an NFS disk even though
> QEMU has added support for it. This series of patches seeks to implement
> this support in Libvirt and begins by adding in flags for an NFS disk.
> 
> Signed-off-by: Ryan Gahagan 
> ---
>  src/libxl/libxl_conf.c| 1 +
>  src/libxl/xen_xl.c| 1 +
>  src/qemu/qemu_block.c | 3 +++
>  src/qemu/qemu_command.c   | 1 +
>  src/qemu/qemu_domain.c| 2 ++
>  src/qemu/qemu_snapshot.c  | 3 +++
>  src/util/virstoragefile.c | 6 ++
>  src/util/virstoragefile.h | 1 +
>  8 files changed, 18 insertions(+)

[...]

> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index fac93118fd..103dade0e7 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c

[...]

> @@ -4627,6 +4629,10 @@ 
> virStorageSourceNetworkDefaultPort(virStorageNetProtocol protocol)
>  case VIR_STORAGE_NET_PROTOCOL_VXHS:
>  return ;
>  
> +case VIR_STORAGE_NET_PROTOCOL_NFS:
> +/* Per 
> https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/4/html/reference_guide/ch-nfs
>  */
> +return 2049;

I don't think we want to add a default port since the qemu code will not
accept it at all.

Re: [libvirt PATCH 01/17] util: remove unused virStorageGenerateQcowPassphrase

[Peter Krempa]

On Mon, Dec 14, 2020 at 16:55:21 +0100, Pavel Hrdina wrote:
> The last user was removed by commit
> <40f0e0348dfc84f28a500e262c4953b0d3b44fa0>.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/libvirt_private.syms|  1 -
>  src/util/virstorageencryption.c | 34 -
>  src/util/virstorageencryption.h |  2 --
>  3 files changed, 37 deletions(-)

Reviewed-by: Peter Krempa 

Re: [libvirt PATCH 02/17] virstoragefile: remove unused virStorageFileChainCheckBroken

[Peter Krempa]

On Mon, Dec 14, 2020 at 16:55:22 +0100, Pavel Hrdina wrote:
> The last usage outside of tests was removed by commit
> <780f8c94ca8b3dee7eb59c1bfbc32f672f965df8>.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/libvirt_private.syms  |  1 -
>  src/util/virstoragefile.c | 33 -
>  src/util/virstoragefile.h |  3 ---
>  tests/virstoragetest.c|  6 --
>  4 files changed, 43 deletions(-)

Reviewed-by: Peter Krempa 

Re: [libvirt PATCH 03/17] util: move virQEMUBuildQemuImgKeySecretOpts into storage

[Peter Krempa]

On Mon, Dec 14, 2020 at 16:55:23 +0100, Pavel Hrdina wrote:
> Function virQEMUBuildQemuImgKeySecretOpts is not used anywhere else
> so there is no need to have it in util.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/libvirt_private.syms   |  1 -
>  src/storage/storage_util.c | 74 --
>  src/util/virqemu.c | 69 ---
>  src/util/virqemu.h |  6 
>  4 files changed, 72 insertions(+), 78 deletions(-)

Additionally, with blockdev-create in qemu it will not be used for any
image creation while the VM is running.

Reviewed-by: Peter Krempa 

Re: [libvirt PATCH v3 01/21] tests: remove extra trailing semicolon

[Erik Skultety]

On Thu, Dec 24, 2020 at 08:14:25AM -0600, Jonathon Jongsma wrote:
> The macro should not have a trailing semicolon so that when the macro is
> used, the user can add a semicolon themselves.

Well, it can, it has no effect. However, for consistency reasons, this is the
prevailing form, so sure, let's drop it.

> 
> Signed-off-by: Jonathon Jongsma 
> ---
Reviewed-by: Erik Skultety 

Re: [libvirt PATCH 04/17] util: move virStorageFileGetLVMKey to locking

[Peter Krempa]

On Mon, Dec 14, 2020 at 16:55:24 +0100, Pavel Hrdina wrote:
> The function doesn't take virStorageSource as argument and has nothing
> in common with virStorageSource or storage file.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/libvirt_private.syms|  1 -
>  src/locking/lock_driver_lockd.c | 66 -
>  src/util/virstoragefile.c   | 62 ---
>  src/util/virstoragefile.h   |  2 -
>  4 files changed, 65 insertions(+), 66 deletions(-)

[...]

> +#ifdef LVS
> +static int virLockManagerGetLVMKey(const char *path,
> +   char **key)
> +{

Preferably convert the header to the modern function header style where
the function name starts on a new line after the type. (also in the
#else section)

> +/*
> + *  # lvs --noheadings --unbuffered --nosuffix --options "uuid" LVNAME

Reviewed-by: Peter Krempa 

Re: [libvirt PATCH 05/17] util: move virStorageFileCheckCompat into conf

[Peter Krempa]

On Mon, Dec 14, 2020 at 16:55:25 +0100, Pavel Hrdina wrote:
> It is not used anywhere else.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/conf/storage_conf.c   | 23 ++-
>  src/util/virstoragefile.c | 24 
>  src/util/virstoragefile.h |  2 --
>  3 files changed, 22 insertions(+), 27 deletions(-)

Reviewed-by: Peter Krempa 

Re: [libvirt PATCH 06/17] virfile: refactor virFileNBDDeviceAssociate

[Peter Krempa]

On Mon, Dec 14, 2020 at 16:55:26 +0100, Pavel Hrdina wrote:
> The only reason why virstoragefile.h needs to be included in virfile.h
> is that virFileNBDDeviceAssociate() takes virStorageFileFormat argument.
> The function doesn't need the enum value as it converts the value to
> string and uses only that.
> 
> Change the argument to string which will allow us to remove that
> include.
> 
> The extra seemingly unrelated include changes is because all of the
> added includes where indirectly provided by virfile.h having
> virstoragefile.h.

Please split all the #include additions into a separate patch, since
that can be commited before and is a justifiable change. This will
cut-down the noise in this commit.

> Signed-off-by: Pavel Hrdina 
> ---
>  src/driver.c   | 1 +
>  src/lxc/lxc_controller.c   | 4 ++--
>  src/qemu/qemu_interop_config.c | 1 +
>  src/qemu/qemu_shim.c   | 1 +
>  src/storage/parthelper.c   | 1 +
>  src/util/virarptable.c | 1 +
>  src/util/vircgroupv1.c | 1 +
>  src/util/vircgroupv2devices.c  | 1 +
>  src/util/virfile.c | 8 ++--
>  src/util/virfile.h | 4 ++--
>  src/util/virpidfile.c  | 1 +
>  src/util/virresctrl.c  | 1 +
>  src/util/virsysinfo.c  | 1 +
>  src/util/virtpm.c  | 1 +
>  tools/virsh-console.c  | 1 +
>  tools/virsh-util.c | 1 +
>  16 files changed, 19 insertions(+), 10 deletions(-)

[...]

> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index f7283fa72f..3f4c6d1d0a 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -879,14 +879,13 @@ virFileNBDLoadDriver(void)
>  }
>  
>  int virFileNBDDeviceAssociate(const char *file,
> -  virStorageFileFormat fmt,
> +  const char *fmtstr,
>bool readonly,
>char **dev)
>  {
>  g_autofree char *nbddev = NULL;
>  g_autofree char *qemunbd = NULL;
>  g_autoptr(virCommand) cmd = NULL;
> -const char *fmtstr = NULL;
>  
>  if (!virFileNBDLoadDriver())
>  return -1;

You can use my:

Reviewed-by: Peter Krempa 

on the patch(es) adding the headers as well as this with the header
additions stripped (just don't forget to send them to the list before
pushing :) )

Re: [libvirt PATCH 07/17] virstoragefile: move virStorageFileResize into virfile

[Peter Krempa]

On Mon, Dec 14, 2020 at 16:55:27 +0100, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina 
> ---
>  src/libvirt_private.syms   |  2 +-
>  src/storage/storage_util.c |  2 +-
>  src/util/virfile.c | 47 ++
>  src/util/virfile.h |  4 
>  src/util/virstoragefile.c  | 47 --
>  src/util/virstoragefile.h  |  4 
>  6 files changed, 53 insertions(+), 53 deletions(-)

Yup, that has nothing to to with virStorageSource.

Reviewed-by: Peter Krempa 

Re: [libvirt PATCH 08/17] virstoragefile: move virStorageFileIsClusterFS into virfile

[Peter Krempa]

On Mon, Dec 14, 2020 at 16:55:28 +0100, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina 
> ---
>  src/libvirt_private.syms  |  2 +-
>  src/qemu/qemu_migration.c |  2 +-
>  src/util/virfile.c| 12 
>  src/util/virfile.h|  1 +
>  src/util/virstoragefile.c | 11 ---
>  src/util/virstoragefile.h |  1 -
>  6 files changed, 15 insertions(+), 14 deletions(-)

[...]

> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index 6e16780e30..3f58b98248 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -3709,6 +3709,18 @@ int virFileIsSharedFS(const char *path)
>  }
>  
>  
> +int virFileIsClusterFS(const char *path)

Please use the modern function header format.

> +{
> +/* These are coherent cluster filesystems known to be safe for
> + * migration with cache != none
> + */
> +return virFileIsSharedFSType(path,
> + VIR_FILE_SHFS_GFS2 |
> + VIR_FILE_SHFS_OCFS |
> + VIR_FILE_SHFS_CEPH);
> +}
> +
> +
>  #if defined(__linux__) && defined(WITH_SYS_MOUNT_H)
>  int
>  virFileSetupDev(const char *path,

Reviewed-by: Peter Krempa 

Re: [libvirt PATCH 09/17] virstoragefile: move virStorageIsFile into virfile

[Peter Krempa]

On Mon, Dec 14, 2020 at 16:55:29 +0100, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina 
> ---
>  src/libvirt_private.syms  |  2 +-
>  src/util/virfile.c| 21 +
>  src/util/virfile.h|  1 +
>  src/util/virstoragefile.c | 26 +++---
>  src/util/virstoragefile.h |  1 -
>  5 files changed, 26 insertions(+), 25 deletions(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 57f3b12000..0b560dfe45 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2090,6 +2090,7 @@ virFileIsCDROM;
>  virFileIsClusterFS;
>  virFileIsDir;
>  virFileIsExecutable;
> +virFileIsFile;

This doesn't really belong here. virFileIs* checks an actual file, while
the function checks only the filename to conform to some rules.

Additionally ...

>  virFileIsLink;
>  virFileIsMountPoint;
>  virFileIsRegular;
> @@ -3156,7 +3157,6 @@ virStorageFileSupportsBackingChainTraversal;
>  virStorageFileSupportsCreate;
>  virStorageFileSupportsSecurityDriver;
>  virStorageFileUnlink;
> -virStorageIsFile;
>  virStorageIsRelative;
>  virStorageNetHostDefClear;
>  virStorageNetHostDefCopy;

[...]

> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 3db85d8b89..f37802260b 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c

[...]

> @@ -783,7 +763,7 @@ virStorageIsRelative(const char *backing)
>  if (backing[0] == '/')
>  return false;
>  
> -if (!virStorageIsFile(backing))
> +if (!virFileIsFile(backing))

... all uses ...

>  return false;
>  
>  return true;
> @@ -1450,7 +1430,7 @@ virStorageFileChainLookup(virStorageSourcePtr chain,
>  {
>  virStorageSourcePtr prev;
>  const char *start = chain->path;
> -bool nameIsFile = virStorageIsFile(name);
> +bool nameIsFile = virFileIsFile(name);

... are within ...

>  
>  if (!parent)
>  parent = &prev;
> @@ -3794,7 +3774,7 @@ virStorageSourceNewFromBackingAbsolute(const char *path,
>  
>  *src = NULL;
>  
> -if (virStorageIsFile(path)) {
> +if (virFileIsFile(path)) {

... this one file. Just unexport it and keep it here, since it's only
relevant to backing chain traversal.

>  def->type = VIR_STORAGE_TYPE_FILE;
>  
>  def->path = g_strdup(path);

You can use my R-b on the patch which unexports it.

NACK to this movement.

Re: [libvirt PATCH 10/17] virstoragefile: move virStorageIsRelative into virfile

[Peter Krempa]

On Mon, Dec 14, 2020 at 16:55:30 +0100, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina 
> ---
>  src/libvirt_private.syms  |  2 +-
>  src/qemu/qemu_block.c |  2 +-
>  src/qemu/qemu_snapshot.c  |  2 +-
>  src/util/virfile.c| 13 +
>  src/util/virfile.h|  2 ++
>  src/util/virstoragefile.c | 15 +--
>  src/util/virstoragefile.h |  2 --
>  7 files changed, 19 insertions(+), 19 deletions(-)

NACK based on the same grounds as previous patch. This function is not
checking whether a file is relative, but just the filename. This is
relevant just to backing chain manipulation.

Re: [libvirt PATCH 11/17] virstoragefile: change virStorageSource->drv to void pointer

[Peter Krempa]

On Mon, Dec 14, 2020 at 16:55:31 +0100, Pavel Hrdina wrote:
> This will allow following patches to move virStorageSource into conf
> directory and virStorageDriverData into a new storage_file directory.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/storage/storage_file_fs.c  |  21 +++--
>  src/storage/storage_file_gluster.c |  31 +---
>  src/util/virstoragefile.c  | 118 +
>  src/util/virstoragefile.h  |   5 +-
>  src/util/virstoragefilebackend.h   |   2 +
>  5 files changed, 122 insertions(+), 55 deletions(-)

Reviewed-by: Peter Krempa 

Re: [libvirt PATCH v3 02/21] nodedev: introduce concept of 'active' node devices

[Erik Skultety]

On Thu, Dec 24, 2020 at 08:14:26AM -0600, Jonathon Jongsma wrote:
> we will be able to define mediated devices that can be started or
> stopped, so we need to be able to indicate whether the device is active
> or not, similar to other resources (storage pools, domains, etc.)
> 
> Signed-off-by: Jonathon Jongsma 
> ---
Reviewed-by: Erik Skultety 

Re: [libvirt PATCH v3 03/21] nodedev: Add ability to filter by active state

[Erik Skultety]

On Thu, Dec 24, 2020 at 08:14:27AM -0600, Jonathon Jongsma wrote:
> Add two flag values for virConnectListAllNodeDevices() so that we can
> list only node devices that are active or inactive.
> 
> Signed-off-by: Jonathon Jongsma 
> ---
Reviewed-by: Erik Skultety 

Re: [libvirt PATCH 13/17] util: extract virStorageFile code into storage_file

[Peter Krempa]

On Mon, Dec 14, 2020 at 16:55:33 +0100, Pavel Hrdina wrote:
> Up until now we had a runtime code and XML related code in the same
> source file inside util directory.
> 
> This patch takes the runtime part and extracts it into the new
> storage_file directory.

I like this idea but I'm not a fan of the direction of the rename that
happens along with it. Specifically rename of virStorageSource* to
virStorageFile.

All the functions here basically work on a virStorageSource so
preferably we keep the name in both places the code for it exists, thus
src/conf/storage_source.[ch] and src/storage_source/storage_source.[ch].

I'm aware that it might be confusing at first to have two files which
can take functions for a single type, unfortunately the distinction
seems necessary, since we have two sources which can create
configuration based on virStorageSource.

Alternatively when we want to avoid files with same name we can use
storage_source_conf.c and storage_source.c

> 
> Signed-off-by: Pavel Hrdina 
> ---

[...]

> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 1bbf567847..43be8dd61a 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1618,6 +1618,43 @@ virSecurityManagerVerify;
>  virSecurityXATTRNamespaceDefined;

Having the name duality then also brings up the question how you picked
the functions to move here, since ...


The following ones actually access the storage directly:

> +# storage_file/storage_file.h
> +virStorageFileAccess;
> +virStorageFileCanonicalizePath;
> +virStorageFileChown;
> +virStorageFileCreate;
> +virStorageFileGetBackingStoreStr;
> +virStorageFileGetUniqueIdentifier;
> +virStorageFileProbeFormat;
> +virStorageFileRead;
> +virStorageFileStat;
> +virStorageFileUnlink;
> +virStorageSourceUpdateBackingSizes;
> +virStorageSourceUpdateCapacity;
> +virStorageSourceUpdatePhysicalSize;

These are helper function to the backend access:
> +virStorageFileInit;
> +virStorageFileInitAs;
> +virStorageFileDeinit;
> +virStorageFileSupportsAccess;
> +virStorageFileSupportsBackingChainTraversal;
> +virStorageFileSupportsCreate;
> +virStorageFileSupportsSecurityDriver;

These access storage indirectly:
> +virStorageFileGetMetadata;
> +virStorageFileGetMetadataFromBuf;
> +virStorageFileGetMetadataFromFD;
> +virStorageFileReportBrokenChain;

This one is a qemuism and should be rather moved to
src/qemu/qemu_block.c or qemu_domain.c

> +virStorageSourceFindByNodeName;

These just allocate a virStorageSource based on parsing a string:

> +virStorageSourceNewFromBacking;
> +virStorageSourceNewFromBackingAbsolute;

This is almost same as the above (actually it could be replaced by
virStorageSourceNewFromBackingAbsolute + a check that the result is
_PROTOCOL_RBD)

> +virStorageSourceParseRBDColonString;

These are just helpers which look through a chain of virStorageSources
or are not really accessing anything

> +virStorageFileChainLookup;
> +virStorageFileGetRelativeBackingPath;
> +virStorageFileParseBackingStoreStr;
> +virStorageFileParseChainIndex;

These are actually private data XML formatter/parser

> +virStorageSourcePrivateDataFormatRelPath;
> +virStorageSourcePrivateDataParseRelPath;

The naming mismatch comes from the years this code was gradually
modified and I'd really prefer if everything unifies on virStorageSource
since everything is actually related to virStorageSource.

For the backends which do the actual access of storage we could rename
it to virStorageSourceBackend.

> diff --git a/src/storage_file/storage_file.c b/src/storage_file/storage_file.c
> new file mode 100644
> index 00..6bfeb26233
> --- /dev/null
> +++ b/src/storage_file/storage_file.c
> @@ -0,0 +1,3845 @@
> +/*
> + * storage_file.c: file utility functions for FS storage backend
> + *
> + * Copyright (C) 2007-2017 Red Hat, Inc.
> + * Copyright (C) 2007-2008 Daniel P. Berrange
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library.  If not, see
> + * .
> + */
> +
> +#include 
> +
> +#include 
> +#include 
> +
> +#include "storage_file.h"
> +#include "viralloc.h"
> +#include "virendian.h"
> +#include "virfile.h"
> +#include "virhash.h"
> +#include "virjson.h"
> +#include "virlog.h"
> +#include "virstoragefilebackend.h"
> +#include "virstring.h"
> +#include "viruri.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_STORAGE
> +
> +VIR_LOG_INIT("storage

[libvirt PATCH 1/2] rpm: ensure swtpm tools are installed with QEMU

[Daniel P . Berrangé]

These are needed for the  devices to be usable.

Signed-off-by: Daniel P. Berrangé 
---
 libvirt.spec.in | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index f20a1c741b..2e026b0423 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -735,6 +735,9 @@ Requires: xz
 %if 0%{?fedora} || 0%{?rhel} > 7
 Requires: systemd-container
 %endif
+%if 0%{?fedora} || 0%{?rhel} > 7
+Requires: swtpm-tools
+%endif
 
 %description daemon-driver-qemu
 The qemu driver plugin for the libvirtd daemon, providing
-- 
2.29.2

[libvirt PATCH 0/2] rpm: minor swtpm fixes

[Daniel P . Berrangé]

Originally reported at

https://src.fedoraproject.org/rpms/libvirt/pull-request/9

Daniel P. Berrangé (2):
  rpm: ensure swtpm tools are installed with QEMU
  rpm: fix ownership of the swtpm log directory

 libvirt.spec.in | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

-- 
2.29.2

[libvirt PATCH 2/2] rpm: fix ownership of the swtpm log directory

[Daniel P . Berrangé]

As soon as a guest using a  device is launched, libvirt will change
the ownership to 'tss' user and group, which will cause RPM verify to
then fail.

Signed-off-by: Daniel P. Berrangé 
---
 libvirt.spec.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 2e026b0423..c455aa7788 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1747,7 +1747,7 @@ exit 0
 %{_datadir}/augeas/lenses/tests/test_libvirtd_qemu.aug
 %{_libdir}/%{name}/connection-driver/libvirt_driver_qemu.so
 %dir %attr(0711, root, root) %{_localstatedir}/lib/libvirt/swtpm/
-%dir %attr(0711, root, root) %{_localstatedir}/log/swtpm/libvirt/qemu/
+%dir %attr(0711, tss, tss) %{_localstatedir}/log/swtpm/libvirt/qemu/
 %{_bindir}/virt-qemu-run
 %{_mandir}/man1/virt-qemu-run.1*
 %endif
-- 
2.29.2

[PATCH 0/5] Fix qemuNodeGetSecurityModel() and some related cleanups

[Michal Privoznik]

While I'd love to give more context, I can't - I've found these on a
stale branch. However, they are still valid and worth of merging.

Michal Prívozník (5):
  qemu: Use virStrcpy in qemuNodeGetSecurityModel()
  qemu: Obtain @caps only after ACL check in qemuNodeGetSecurityModel
  qemu: Fix retval if ACL check fails in qemuNodeGetSecurityModel
  domain_conf: Parse full length of some  attributes
  use more virStrcpy() and virStrcpyStatic()

 src/conf/domain_conf.c  |  6 ++
 src/libvirt-lxc.c   |  5 ++---
 src/qemu/qemu_driver.c  | 23 +--
 src/remote/remote_driver.c  | 12 
 src/security/security_selinux.c |  3 +--
 5 files changed, 18 insertions(+), 31 deletions(-)

-- 
2.26.2

[PATCH 1/5] qemu: Use virStrcpy in qemuNodeGetSecurityModel()

[Michal Privoznik]

The code we have there to copy seclabel model or doi can be
replaced by virStrcpy() calls which do exactly the same checks.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_driver.c | 15 ++-
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a376824854..a9e8f660c7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5930,7 +5930,6 @@ static int qemuNodeGetSecurityModel(virConnectPtr conn,
 virSecurityModelPtr secmodel)
 {
 virQEMUDriverPtr driver = conn->privateData;
-char *p;
 g_autoptr(virCaps) caps = NULL;
 
 memset(secmodel, 0, sizeof(*secmodel));
@@ -5946,23 +5945,21 @@ static int qemuNodeGetSecurityModel(virConnectPtr conn,
 caps->host.secModels[0].model == NULL)
 return 0;
 
-p = caps->host.secModels[0].model;
-if (strlen(p) >= VIR_SECURITY_MODEL_BUFLEN-1) {
+if (virStrcpy(secmodel->model, caps->host.secModels[0].model,
+  VIR_SECURITY_MODEL_BUFLEN) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("security model string exceeds max %d bytes"),
-   VIR_SECURITY_MODEL_BUFLEN-1);
+   VIR_SECURITY_MODEL_BUFLEN - 1);
 return -1;
 }
-strcpy(secmodel->model, p);
 
-p = caps->host.secModels[0].doi;
-if (strlen(p) >= VIR_SECURITY_DOI_BUFLEN-1) {
+if (virStrcpy(secmodel->doi, caps->host.secModels[0].doi,
+  VIR_SECURITY_DOI_BUFLEN) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("security DOI string exceeds max %d bytes"),
-   VIR_SECURITY_DOI_BUFLEN-1);
+   VIR_SECURITY_DOI_BUFLEN - 1);
 return -1;
 }
-strcpy(secmodel->doi, p);
 
 return 0;
 }
-- 
2.26.2

[PATCH 2/5] qemu: Obtain @caps only after ACL check in qemuNodeGetSecurityModel

[Michal Privoznik]

Even though we are getting driver capabilities with
refresh=false (so that it is not expensive), we still should do
ACL check first because there is no point in bothering with the
capabilities if caller doesn't have permissions to call the API.
Also, this way the comment makes more sense.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_driver.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a9e8f660c7..96ec84bd1c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5934,14 +5934,12 @@ static int qemuNodeGetSecurityModel(virConnectPtr conn,
 
 memset(secmodel, 0, sizeof(*secmodel));
 
-if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
-return 0;
-
 if (virNodeGetSecurityModelEnsureACL(conn) < 0)
 return 0;
 
 /* We treat no driver as success, but simply return no data in *secmodel */
-if (caps->host.nsecModels == 0 ||
+if (!(caps = virQEMUDriverGetCapabilities(driver, false)) ||
+caps->host.nsecModels == 0 ||
 caps->host.secModels[0].model == NULL)
 return 0;
 
-- 
2.26.2

[PATCH 4/5] domain_conf: Parse full length of some attributes

[Michal Privoznik]

In virSecurityLabelDefParseXML() we are parsing the 
element among with its attributes. Some of the attributes are
limited in length (because of virNodeGetSecurityModel()), however
some are not. And for the latter ones we don't need to use
virXMLPropStringLimit() to parse them. Moreover, using
VIR_SECURITY_LABEL_BUFLEN as the limit is wrong - we are not
storing the parsed strings into a static buffer of that size
rather than checking if the string passes string -> enum
conversion.

Signed-off-by: Michal Privoznik 
---
 src/conf/domain_conf.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 384710da40..5a8947eeec 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7713,8 +7713,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
 /* set default value */
 seclabel->type = VIR_DOMAIN_SECLABEL_DYNAMIC;
 
-p = virXMLPropStringLimit(ctxt->node, "type",
-  VIR_SECURITY_LABEL_BUFLEN - 1);
+p = virXMLPropString(ctxt->node, "type");
 if (p) {
 seclabel->type = virDomainSeclabelTypeFromString(p);
 if (seclabel->type <= 0) {
@@ -7729,8 +7728,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
 seclabel->relabel = false;
 
 VIR_FREE(p);
-p = virXMLPropStringLimit(ctxt->node, "relabel",
-  VIR_SECURITY_LABEL_BUFLEN-1);
+p = virXMLPropString(ctxt->node, "relabel");
 if (p) {
 if (virStringParseYesNo(p, &seclabel->relabel) < 0) {
 virReportError(VIR_ERR_XML_ERROR,
-- 
2.26.2

[PATCH 3/5] qemu: Fix retval if ACL check fails in qemuNodeGetSecurityModel

[Michal Privoznik]

While previously we returned 0 this is not correct. We have to
return a negative value to indicate error.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 96ec84bd1c..88324945ef 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5935,7 +5935,7 @@ static int qemuNodeGetSecurityModel(virConnectPtr conn,
 memset(secmodel, 0, sizeof(*secmodel));
 
 if (virNodeGetSecurityModelEnsureACL(conn) < 0)
-return 0;
+return -1;
 
 /* We treat no driver as success, but simply return no data in *secmodel */
 if (!(caps = virQEMUDriverGetCapabilities(driver, false)) ||
-- 
2.26.2

[PATCH 5/5] use more virStrcpy() and virStrcpyStatic()

[Michal Privoznik]

There are a few places where we open code virStrcpy() or
virStrcpyStatic(). Call respective functions instead.

Signed-off-by: Michal Privoznik 
---
 src/libvirt-lxc.c   |  5 ++---
 src/remote/remote_driver.c  | 12 
 src/security/security_selinux.c |  3 +--
 3 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/src/libvirt-lxc.c b/src/libvirt-lxc.c
index f6391214be..2a271b74f0 100644
--- a/src/libvirt-lxc.c
+++ b/src/libvirt-lxc.c
@@ -35,6 +35,7 @@
 # include 
 #endif
 #include "vircgroup.h"
+#include "virstring.h"
 
 #define VIR_FROM_THIS VIR_FROM_NONE
 
@@ -213,7 +214,7 @@ virDomainLxcEnterSecurityLabel(virSecurityModelPtr model,
 goto error;
 }
 
-if (strlen((char *) ctx) >= VIR_SECURITY_LABEL_BUFLEN) {
+if (virStrcpy(oldlabel->label, ctx, VIR_SECURITY_LABEL_BUFLEN) < 
0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("security label exceeds "
  "maximum length: %d"),
@@ -221,8 +222,6 @@ virDomainLxcEnterSecurityLabel(virSecurityModelPtr model,
 freecon(ctx);
 goto error;
 }
-
-strcpy(oldlabel->label, (char *) ctx);
 freecon(ctx);
 
 if ((oldlabel->enforcing = security_getenforce()) < 0) {
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index b0af3ee88e..1b784e61c7 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -2328,12 +2328,11 @@ remoteDomainGetSecurityLabel(virDomainPtr domain, 
virSecurityLabelPtr seclabel)
 }
 
 if (ret.label.label_val != NULL) {
-if (strlen(ret.label.label_val) >= sizeof(seclabel->label)) {
+if (virStrcpyStatic(seclabel->label, ret.label.label_val) < 0) {
 virReportError(VIR_ERR_RPC, _("security label exceeds maximum: 
%zu"),
sizeof(seclabel->label) - 1);
 goto cleanup;
 }
-strcpy(seclabel->label, ret.label.label_val);
 seclabel->enforcing = ret.enforcing;
 }
 
@@ -2372,13 +2371,12 @@ remoteDomainGetSecurityLabelList(virDomainPtr domain, 
virSecurityLabelPtr* secla
 for (i = 0; i < ret.labels.labels_len; i++) {
 remote_domain_get_security_label_ret *cur = &ret.labels.labels_val[i];
 if (cur->label.label_val != NULL) {
-if (strlen(cur->label.label_val) >= sizeof((*seclabels)->label)) {
+if (virStrcpyStatic((*seclabels)[i].label, cur->label.label_val) < 
0) {
 virReportError(VIR_ERR_RPC, _("security label exceeds maximum: 
%zd"),
sizeof((*seclabels)->label) - 1);
 VIR_FREE(*seclabels);
 goto cleanup;
 }
-strcpy((*seclabels)[i].label, cur->label.label_val);
 (*seclabels)[i].enforcing = cur->enforcing;
 }
 }
@@ -2444,21 +2442,19 @@ remoteNodeGetSecurityModel(virConnectPtr conn, 
virSecurityModelPtr secmodel)
 }
 
 if (ret.model.model_val != NULL) {
-if (strlen(ret.model.model_val) >= sizeof(secmodel->model)) {
+if (virStrcpyStatic(secmodel->model, ret.model.model_val) < 0) {
 virReportError(VIR_ERR_RPC, _("security model exceeds maximum: 
%zu"),
sizeof(secmodel->model) - 1);
 goto cleanup;
 }
-strcpy(secmodel->model, ret.model.model_val);
 }
 
 if (ret.doi.doi_val != NULL) {
-if (strlen(ret.doi.doi_val) >= sizeof(secmodel->doi)) {
+if (virStrcpyStatic(secmodel->doi, ret.doi.doi_val) < 0) {
 virReportError(VIR_ERR_RPC, _("security doi exceeds maximum: %zu"),
sizeof(secmodel->doi) - 1);
 goto cleanup;
 }
-strcpy(secmodel->doi, ret.doi.doi_val);
 }
 
 rv = 0;
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index e9cd95916e..2fc6ef2616 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1209,7 +1209,7 @@ virSecuritySELinuxGetProcessLabel(virSecurityManagerPtr 
mgr G_GNUC_UNUSED,
 return -1;
 }
 
-if (strlen((char *)ctx) >= VIR_SECURITY_LABEL_BUFLEN) {
+if (virStrcpy(sec->label, ctx, VIR_SECURITY_LABEL_BUFLEN) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("security label exceeds "
  "maximum length: %d"),
@@ -1218,7 +1218,6 @@ virSecuritySELinuxGetProcessLabel(virSecurityManagerPtr 
mgr G_GNUC_UNUSED,
 return -1;
 }
 
-strcpy(sec->label, (char *)ctx);
 freecon(ctx);
 
 VIR_DEBUG("label=%s", sec->label);
-- 
2.26.2

Re: [PATCH 0/5] Fix qemuNodeGetSecurityModel() and some related cleanups

[Ján Tomko]

On a Monday in 2021, Michal Privoznik wrote:

While I'd love to give more context, I can't - I've found these on a
stale branch. However, they are still valid and worth of merging.

Michal Prívozník (5):
 qemu: Use virStrcpy in qemuNodeGetSecurityModel()
 qemu: Obtain @caps only after ACL check in qemuNodeGetSecurityModel
 qemu: Fix retval if ACL check fails in qemuNodeGetSecurityModel
 domain_conf: Parse full length of some  attributes
 use more virStrcpy() and virStrcpyStatic()

src/conf/domain_conf.c  |  6 ++
src/libvirt-lxc.c   |  5 ++---
src/qemu/qemu_driver.c  | 23 +--
src/remote/remote_driver.c  | 12 
src/security/security_selinux.c |  3 +--
5 files changed, 18 insertions(+), 31 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH 0/2] rpm: minor swtpm fixes

[Laine Stump]

On 1/4/21 1:05 PM, Daniel P. Berrangé wrote:

Originally reported at

https://src.fedoraproject.org/rpms/libvirt/pull-request/9

Daniel P. Berrangé (2):
   rpm: ensure swtpm tools are installed with QEMU
   rpm: fix ownership of the swtpm log directory

  libvirt.spec.in | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)



Reviewed-by: Laine Stump 


for both.

(NB: for those who don't want to bother looking it up themselves - the 
Fedora pull request only mentioned installing the swtpm package, but I 
checked the spec file for swtpm to see what things only get installed by 
the swtpm-tools sub-package, and found that (at least) swtpm_setup and 
swtpm_ioctl (both used by libvirt via virCommand*()) are in swtpm-tools, 
so this is the correct Requires:).

Re: To start multiple KVM guests from one qcow2 image with transient disk option

[Masayoshi Mizuma]

On Sat, Dec 19, 2020 at 11:30:39PM -0500, Masayoshi Mizuma wrote:
> On Thu, Dec 17, 2020 at 10:39:36AM +0100, Peter Krempa wrote:
> > On Mon, Dec 14, 2020 at 22:49:03 -0500, Masayoshi Mizuma wrote:
> > > On Sat, Dec 12, 2020 at 11:57:15AM +0100, Peter Krempa wrote:
> > > > On Fri, Dec 11, 2020 at 20:58:48 -0500, Masayoshi Mizuma wrote:
> > > > > Hello,
> > > > > 
> > > > > I would like to start multiple KVM guests from one qcow2 image, and
> > > > > discard the changes which the KVM guests done.
> > > > > 
> > > > > transient disk option is useful for discarding the changes, however,
> > > > > we cannot start multiple KVM guest from one qcow2 image because the
> > > > > image is write-locked by the first guest to be started.
> > > > > 
> > > > > I suppose the disk which transient option is enabled don't need to
> > > > > get the write lock because any changes go to the overlay image, and
> > > > > the overlay image is removed when the guest shutdown.
> > > > > 
> > > > > qemu has 'locking' option and the write lock is disabled when 
> > > > > locking=off.
> > > > > To implement that, I have two ideas. I would appreciate it if you 
> > > > > could
> > > > > give me the ideas which way is better (or another way).
> > > > 
> > > > There are two aspects of this.
> > > > 
> > > > 1) Controlling the locking property of qemu may be worth in many cases,
> > > > so by itself this would be a worthwhile patchset to add control of
> > > > qemu locking for a per-backing store basis.
> > > > 
> > > > 2) Disabling locking to achieve this is wrong though. The qemu image
> > > > locking infrastructure is justified and prevents many problems which
> > > > users might get into by wrong configuration.
> > > > 
> > > > For  disks, we should rather instantiate the top level
> > > > format node as 'readonly' and then put a read-write overlay on top of
> > > > it. This still prevents from using the file which became a backing file
> > > > in read-write mode in another VM.
> > > 
> > > Thank you for the idea!
> > > I just tried to change the original image to read-only (changed the 
> > > "read-only":false
> > > to "read-only":true) simply, then created a read-write overlay.
> > > qemu stopped with following assertion:
> > > 
> > >   qemu-system-x86_64: ../block/io.c:1874: bdrv_co_write_req_prepare: 
> > > Assertion `child->perm & BLK_PERM_WRITE' failed.
> > > 
> > > It looks like the qemu hit the assertion because the permission of the 
> > > overlay image
> > > is same as the original image.
> > > Probably I'm missing something... I'll dive into that...
> > 
> > Could you please post the command line and the QMP commands? Maybe
> > something isn't configured right in libvirt. Alternatively qemu might
> > need modification.
> 
> Sure, here is the qemu command line options and QMP commands:
> 
> qemu command line options:
> 
> qemu-system-x86_64 \
>   -M q35,accel=kvm,usb=off,vmport=off,smm=on,dump-guest-core=off \
>   -smp 4 \
>   -m 4096 \
>   -blockdev 
> '{"driver":"file","filename":"/var/lib/libvirt/images/guest.qcow2","node-name":"storage1","auto-read-only":true,"discard":"unmap"}'
>  \
>   -blockdev 
> '{"node-name":"format1","read-only":true,"driver":"qcow2","file":"storage1","backing":null}'
>  \
>   -device virtio-blk-pci,drive=format1,id=virtio-disk0,bootindex=1 \
>   -nographic \
>   -nodefaults \
>   -no-user-config \
>   -serial telnet::1235,server,nowait \
>   -qmp tcp::1335,server,nowait \
>   -S
> 
> QMP commands:
> (Before running following QMP commands, I create 
> '/var/lib/libvirt/images/guest.qcow2.TRANSIENT'
>  by touch command)
> 
>{"execute":"qmp_capabilities"}
>
> {"execute":"blockdev-add","arguments":{"driver":"file","filename":"/var/lib/libvirt/images/guest.qcow2.TRANSIENT","node-name":"storage2","auto-read-only":true,"discard":"unmap"}}
>
> {"execute":"blockdev-create","arguments":{"job-id":"create","options":{"driver":"qcow2","file":"storage2","size":10737418240,"cluster-size":65536,"backing-file":"/var/lib/libvirt/images/guest.qcow2","backing-fmt":"qcow2"}}}
>
> {"execute":"blockdev-add","arguments":{"node-name":"format2","read-only":false,"driver":"qcow2","file":"storage2","backing":null}}
>
> {"execute":"blockdev-snapshot","arguments":{"node":"format1","overlay":"format2"}}
>{"execute":"cont"}
> 
> After "cont" command executed, qemu stops as following assertion:
> 
> qemu-system-x86_64: ../block/io.c:1865: bdrv_co_write_req_prepare: 
> Assertion `child->perm & BLK_PERM_WRITE' failed.

I think following qemu command line options and QMP commands work for sharing
the qcow2 disks. The following uses disk hotplug instead of snapshot overlay.
Does that make sense for libvirt...?

qemu command line options:

  qemu-system-x86_64 \
-M q35,accel=kvm,usb=off,vmport=off,smm=on,dump-guest-core=off \
-smp 1 \
-m 4096 \
-blockdev 
'{"driver":"file","filename":"/home/mmizuma/debug/guest.qcow2","node-name

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

[Ryan Gahagan]

On Mon, Jan 4, 2021 at 8:24 AM Peter Krempa  wrote:

> On Tue, Dec 29, 2020 at 15:21:25 -0600, Ryan Gahagan wrote:
> > +  
> > +
> > +  
> > +
> > +  
> > +nfs
> > +  
> > +
> > +
> > +
> > +
>
> This allows also 'port' and non TCP transports. It's okay to simplify
> the schema though and use the generic type. The code will need to reject
> those when we'll validate whether the disk is possible to represent for
> qemu.
>

Just to be 100% clear, does this mean we can leave the
diskSourceNetworkProtocolNFS element definition as is and simply enforce
the port omission/tcp transport in the code? Or would you prefer that we
re-write the schema to use a custom variety of the diskSourceNetworkHost
which only supports tcp and has no port option?

Re: [PATCH 6/7] util: Added a backing store NFS parser

[Ryan Gahagan]

On Mon, Jan 4, 2021 at 8:41 AM Peter Krempa  wrote:

> On Tue, Dec 29, 2020 at 15:21:28 -0600, Ryan Gahagan wrote:
> > +src->nfs_uid = (uid_t) uidStore;
> > +src->nfs_gid = (gid_t) gidStore;
>
> This function must not fill in runtime data, just configuration. I
> presume you did this to silence tests but you'll need to add a hack into
> the test code rather than abusing this to fill runtime data.
>
> Ideally in the future the runtime data will be split off into an opaque
> sub-object so it will not be accessible in this code. Don't touch
> nfs_uid/nfs_gid in this function at all.
>

We removed this code (specifically these assignments to nfs_uid and
nfs_gid) and did all the other refactors requested for this method.
Interestingly, the virstoragetest which tests this code still passes.
However, we're unaware of any "hack" in our test code which actually fixes
the missing uid and gid values. We're worried that this might indicate a
problem with our test. Where should this storage when parsing backing store
data actually be done?

For reference, here is the test

(on our branch after the changes) which is passing. Here

is
the parse backing store method after the changes. Maybe we're just
overreacting but we figured asking to make sure would be better than
submitting a new patch only to find a problem.

Re: [PATCH v2] conf: Add support for keeping TPM emulator state

[Daniel Henrique Barboza]

On 1/3/21 11:31 PM, Eiichi Tsukata wrote:

Currently, swtpm TPM state file is removed when a transient domain is
powered off or undefined. When we store TPM state on a shared storage
such as NFS and use transient domain, TPM states should be kept as it is.

Add per-TPM emulator option `persistent_sate` for keeping TPM state.
This option only works for the emulator type backend and looks as follows:

   
 
   

Signed-off-by: Eiichi Tsukata 
Reviewed-by: Stefan Berger 


Reviewed-by: Daniel Henrique Barboza 


---
  docs/formatdomain.rst |  7 
  docs/schemas/domaincommon.rng | 12 ++
  src/conf/domain_conf.c| 21 ++
  src/conf/domain_conf.h|  1 +
  src/qemu/qemu_tpm.c   |  3 +-
  ...pm-emulator-tpm2-pstate.x86_64-latest.args | 38 +++
  .../tpm-emulator-tpm2-pstate.xml  | 30 +++
  tests/qemuxml2argvtest.c  |  1 +
  ...tpm-emulator-tpm2-pstate.x86_64-latest.xml | 37 ++
  tests/qemuxml2xmltest.c   |  1 +
  10 files changed, 150 insertions(+), 1 deletion(-)
  create mode 100644 
tests/qemuxml2argvdata/tpm-emulator-tpm2-pstate.x86_64-latest.args
  create mode 100644 tests/qemuxml2argvdata/tpm-emulator-tpm2-pstate.xml
  create mode 100644 
tests/qemuxml2xmloutdata/tpm-emulator-tpm2-pstate.x86_64-latest.xml

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 512939679b..87bbd1a4f1 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -6986,6 +6986,13 @@ Example: usage of the TPM Emulator
 -  '1.2' : creates a TPM 1.2
 -  '2.0' : creates a TPM 2.0
  
+``persistent_state``

+   The ``persistent_state`` attribute indicates whether 'swtpm' TPM state is
+   kept or not when a transient domain is powered off or undefined. This
+   option can be used for preserving TPM state. By default the value is ``no``.
+   This attribute only works with the ``emulator`` backend. The accepted values
+   are ``yes`` and ``no``.
+
  ``encryption``
 The ``encryption`` element allows the state of a TPM emulator to be
 encrypted. The ``secret`` must reference a secret object that holds the
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 795b654feb..d7cedc014c 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -4780,6 +4780,18 @@

  

+  
+
+  
+
+  
+yes
+no
+  
+   
+  
+
+  
  

  
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c

index 23415b323c..82c3a68347 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13178,6 +13178,12 @@ virDomainSmartcardDefParseXML(virDomainXMLOptionPtr 
xmlopt,
   * 
   *   
   * 
+ *
+ * Emulator persistent_state is supported with the following:
+ *
+ * 
+ *   
+ * 
   */
  static virDomainTPMDefPtr
  virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt,
@@ -13193,6 +13199,7 @@ virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt,
  g_autofree char *backend = NULL;
  g_autofree char *version = NULL;
  g_autofree char *secretuuid = NULL;
+g_autofree char *persistent_state = NULL;
  g_autofree xmlNodePtr *backends = NULL;
  
  def = g_new0(virDomainTPMDef, 1);

@@ -13265,6 +13272,18 @@ virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt,
  }
  def->data.emulator.hassecretuuid = true;
  }
+
+persistent_state = virXMLPropString(backends[0], "persistent_state");
+if (persistent_state) {
+if (virStringParseYesNo(persistent_state,
+&def->data.emulator.persistent_state) < 0) 
{
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Invalid persistent_state value, either 'yes' or 
'no'"));
+goto error;
+}
+} else {
+def->data.emulator.persistent_state = false;
+}
  break;
  case VIR_DOMAIN_TPM_TYPE_LAST:
  goto error;
@@ -26952,6 +26971,8 @@ virDomainTPMDefFormat(virBufferPtr buf,
  case VIR_DOMAIN_TPM_TYPE_EMULATOR:
  virBufferAsprintf(buf, " version='%s'",
virDomainTPMVersionTypeToString(def->version));
+if (def->data.emulator.persistent_state)
+virBufferAddLit(buf, " persistent_state='yes'");
  if (def->data.emulator.hassecretuuid) {
  char uuidstr[VIR_UUID_STRING_BUFLEN];
  virBufferAddLit(buf, ">\n");
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 72771c46b9..109625828a 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1362,6 +1362,7 @@ struct _virDomainTPMDef {
  char *logfile;
  unsigned char secretuuid[VIR_UUI

Re: [libvirt PATCH v2 3/5] vmx: Make virVMXParseFileName return an integer

[Daniel Henrique Barboza]

On 12/21/20 1:19 PM, Martin Kletzander wrote:

And return the actual extracted value in a parameter.  This way we can later
return success even without any extracted value.

Signed-off-by: Martin Kletzander 
---
  src/esx/esx_driver.c | 31 ++-
  src/vmware/vmware_conf.c | 10 +-
  src/vmx/vmx.c| 21 ++---
  src/vmx/vmx.h|  2 +-
  tests/vmx2xmltest.c  | 19 ++-
  5 files changed, 44 insertions(+), 39 deletions(-)

diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index 51c26e29c65e..86d5396147a3 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -125,10 +125,11 @@ esxFreePrivate(esxPrivate **priv)
   * exception and need special handling. Parse the datastore name and use it
   * to lookup the datastore by name to verify that it exists.
   */
-static char *
-esxParseVMXFileName(const char *fileName, void *opaque)
+static int
+esxParseVMXFileName(const char *fileName,
+void *opaque,
+char **out)
  {
-char *result = NULL;
  esxVMX_Data *data = opaque;
  esxVI_String *propertyNameList = NULL;
  esxVI_ObjectContent *datastoreList = NULL;
@@ -140,18 +141,22 @@ esxParseVMXFileName(const char *fileName, void *opaque)
  char *strippedFileName = NULL;
  char *copyOfFileName = NULL;
  char *directoryAndFileName;
+int ret = -1;
+
+*out = NULL;
  
  if (!strchr(fileName, '/') && !strchr(fileName, '\\')) {

  /* Plain file name, use same directory as for the .vmx file */
-return g_strdup_printf("%s/%s", data->datastorePathWithoutFileName,
+*out = g_strdup_printf("%s/%s", data->datastorePathWithoutFileName,
 fileName);
+return 0;
  }
  
  if (esxVI_String_AppendValueToList(&propertyNameList,

 "summary.name") < 0 ||
  esxVI_LookupDatastoreList(data->ctx, propertyNameList,
&datastoreList) < 0) {
-return NULL;
+return -1;
  }
  
  /* Search for datastore by mount path */

@@ -189,13 +194,13 @@ esxParseVMXFileName(const char *fileName, void *opaque)
  ++tmp;
  }
  
-result = g_strdup_printf("[%s] %s", datastoreName, strippedFileName);

+*out = g_strdup_printf("[%s] %s", datastoreName, strippedFileName);
  
  break;

  }
  
  /* Fallback to direct datastore name match */

-if (!result && STRPREFIX(fileName, "/vmfs/volumes/")) {
+if (!*out && STRPREFIX(fileName, "/vmfs/volumes/")) {
  copyOfFileName = g_strdup(fileName);
  
  /* Expected format: '/vmfs/volumes//' */

@@ -223,22 +228,22 @@ esxParseVMXFileName(const char *fileName, void *opaque)
  goto cleanup;
  }
  
-result = g_strdup_printf("[%s] %s", datastoreName,

- directoryAndFileName);
+*out = g_strdup_printf("[%s] %s", datastoreName, directoryAndFileName);
  }
  
  /* If it's an absolute path outside of a datastore just use it as is */

-if (!result && *fileName == '/') {
+if (!*out && *fileName == '/') {
  /* FIXME: need to deal with Windows paths here too */
-result = g_strdup(fileName);
+*out = g_strdup(fileName);
  }
  
-if (!result) {

+if (!*out) {
  virReportError(VIR_ERR_INTERNAL_ERROR,
 _("Could not handle file name '%s'"), fileName);
  goto cleanup;
  }
  
+ret = 0;

   cleanup:
  esxVI_String_Free(&propertyNameList);
  esxVI_ObjectContent_Free(&datastoreList);
@@ -246,7 +251,7 @@ esxParseVMXFileName(const char *fileName, void *opaque)
  VIR_FREE(strippedFileName);
  VIR_FREE(copyOfFileName);
  
-return result;

+return ret;
  }
  
  
diff --git a/src/vmware/vmware_conf.c b/src/vmware/vmware_conf.c

index e44673247ff1..c90cb10faf7c 100644
--- a/src/vmware/vmware_conf.c
+++ b/src/vmware/vmware_conf.c
@@ -507,11 +507,11 @@ vmwareExtractPid(const char * vmxPath)
  return pid_value;
  }
  
-char *

-vmwareCopyVMXFileName(const char *datastorePath, void *opaque G_GNUC_UNUSED)
+int
+vmwareCopyVMXFileName(const char *datastorePath, void *opaque G_GNUC_UNUSED,
+  char **out)
  {
-char *path;
+*out = g_strdup(datastorePath);
  
-path = g_strdup(datastorePath);

-return path;
+return *out ? 0 : -1;
  }
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index b86dbe9ca267..b2b2244415a1 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -2411,7 +2411,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr 
xmlopt, virConfPtr con
  }
  
  virDomainDiskSetType(*def, VIR_STORAGE_TYPE_FILE);

-if (!(tmp = ctx->parseFileName(fileName, ctx->opaque)))
+if (ctx->parseFileName(fileName, ctx->opaque, &tmp) < 0)
  goto cleanup;
  

Re: [libvirt PATCH v3 00/21] Add support for persistent mediated devices

[Yan Fu]

Tested with v6.10.0-283-g1948d4e61e.

1.Can define/start/destroy mdev device successfully;

2.'virsh nodedev-list' has no '--active' option, which is inconsistent with
the description in the patch：
# virsh nodedev-list --active
error: command 'nodedev-list' doesn't support option --active

3.virsh client hang when trying to destroy a mdev device which is using by
a vm, and after that all 'virsh nodev*' cmds will hang. If restarting
llibvirtd after that, libvirtd will hang.

4.Define a mdev device with the uuid specified, but the mdev device defined
seems using another uuid. Maybe it make a little confusion about the use of
uuid in the xml:
#cat mdev.xml

  mdev_85531b6d_e5e4_41c1_aa2a_8844717f066a   

/sys/devices/pci:00/:00:02.0/85531b6d-e5e4-41c1-aa2a-8844717f066a
***
  pci__00_02_0
  
vfio_mdev
  
  


  


# virsh nodedev-define /root/mdev.xml
Node device ***mdev_6cdbaad0_7215_4741_82b9_c51e1a4decda defined from
/root/mdev.xml



--
Tested by Yan Fu(y...@redhat.com)


On Fri, Dec 25, 2020 at 10:19 AM Han Han  wrote:

>
>
> On Thu, Dec 24, 2020 at 10:15 PM Jonathon Jongsma 
> wrote:
>
>> This patch series follows the previously-merged series which added
>> support for
>> transient mediated devices. This series expands mdev support to include
>> persistent device definitions. Again, it relies on mdevctl as the backend.
>>
>> It follows the common libvirt pattern of APIs by adding the following new
>> APIs
>> for node devices:
>> - virNodeDeviceDefineXML() - defines a persistent device
>> - virNodeDeviceUndefine() - undefines a persistent device
>> - virNodeDeviceCreate() - starts a previously-defined device
>>
>> It also adds virsh commands mapping to these new APIs: nodedev-define,
>> nodedev-undefine, and nodedev-start.
>>
> Trivial suggestions:
> 1. Mention the bug to be resolved by this series in commit msg:
> https://bugzilla.redhat.com/show_bug.cgi?id=1699274
> 2. Add doc of man page for the new virsh commands and options
>
>>
>> Since we rely on mdevctl for the definition of ediated devices, we need a
>> way
>> to stay up-to-date with devices that are defined by mdevctl (outside of
>> libvirt).  The method for staying up-to-date is currently a little bit
>> crude
>> due to the fact that mdevctl does not emit any events when new devices are
>> added or removed. As a workaround, we create a file monitor for the
>> mdevctl
>> config directory and re-query mdevctl when we detect changes within that
>> directory. In the future, mdevctl may introduce a more elegant solution.
>>
>> changes in v3:
>>  - streamlined tests -- removed some unnecessary duplication
>>  - split out patch to factor out node device name generation function
>>  - split nodeDeviceParseMdevctlChildDevice() into a separate function
>>  - added follow-up patch to remove space-padded alignment in header
>>  - refactored the mdevctl update handling significantly:
>>- no longer a separate persistent thread that gets signaled by a timer
>>- now piggybacks onto the existing udev thread and signals the thread
>> in t=
>> he
>>  same way that the udev event does.
>>- Daniel suggested spawning a throw-away thread to handle mdevctl
>> updates,
>>  but that introduces the complexity of possibly serializing multiple
>>  throw-away threads (e.g. if we get an 'created' event followed
>> immediate=
>> ly
>>  by a 'deleted' event, two threads may be spawned and we'd need to
>> ensure
>>  they are properly ordered)
>>  - added virNodeDeviceObjListForEach() and
>> virNodeDeviceObjListRemoveLocked()
>>to simplify removing devices that are removed from mdevctl.
>>  - coding style fixes
>>  - NOTE: per Erik's request, I experimented with changing the way that
>> mdevctl
>>commands were generated and tested (e.g. introducing something like
>>virMdevctlGetCommand(def, MDEVCTL_COMMAND_, ...)), but it
>> was
>>too invasive and awkward and didn't seem worthwhile
>>
>> Changes in v2:
>>  - rebase to latest git master
>>
>> Jonathon Jongsma (21):
>>   tests: remove extra trailing semicolon
>>   nodedev: introduce concept of 'active' node devices
>>   nodedev: Add ability to filter by active state
>>   nodedev: expose internal helper for naming devices
>>   nodedev: add ability to list and parse defined mdevs
>>   nodedev: add STOPPED/STARTED lifecycle events
>>   nodedev: add mdevctl devices to node device list
>>   nodedev: add helper functions to remove node devices
>>   nodedev: handle mdevs that disappear from mdevctl
>>   nodedev: rename dataReady to udevReady
>>   nodedev: Refresh mdev devices when changes are detected
>>   api: add virNodeDeviceDefineXML()
>>   virsh: Add --active, --inactive, --all to nodedev-list
>>   virsh: add nodedev-define command
>>   nodedev: refactor tests to support mdev undefine
>>   api: add virNodeDeviceUndefine()
>>   virsh: Factor out function to find node device
>>   virsh: add nodedev-undefine command
>>   api: add virNodeDeviceCr