[libvirt PATCH] hypervisor: Move interface mgmt methods to hypervisor

[Praveen K Paladugu]

Move guest interface management methods from qemu to hypervisor. These
methods will be shared by networking support in ch driver.

Signed-off-by: Praveen K Paladugu 
---
 po/POTFILES   |   1 +
 src/hypervisor/domain_interface.c | 279 ++
 src/hypervisor/domain_interface.h |  38 
 src/hypervisor/meson.build|   1 +
 src/libvirt_private.syms  |   6 +
 src/qemu/qemu_command.c   |   7 +-
 src/qemu/qemu_hotplug.c   |   3 +-
 src/qemu/qemu_interface.c | 246 +-
 src/qemu/qemu_interface.h |   6 -
 src/qemu/qemu_process.c   |   3 +-
 10 files changed, 341 insertions(+), 249 deletions(-)
 create mode 100644 src/hypervisor/domain_interface.c
 create mode 100644 src/hypervisor/domain_interface.h

diff --git a/po/POTFILES b/po/POTFILES
index 3a51aea5cb..023c041f61 100644
--- a/po/POTFILES
+++ b/po/POTFILES
@@ -92,6 +92,7 @@ src/hyperv/hyperv_util.c
 src/hyperv/hyperv_wmi.c
 src/hypervisor/domain_cgroup.c
 src/hypervisor/domain_driver.c
+src/hypervisor/domain_interface.c
 src/hypervisor/virhostdev.c
 src/interface/interface_backend_netcf.c
 src/interface/interface_backend_udev.c
diff --git a/src/hypervisor/domain_interface.c 
b/src/hypervisor/domain_interface.c
new file mode 100644
index 00..b01b6ee767
--- /dev/null
+++ b/src/hypervisor/domain_interface.c
@@ -0,0 +1,279 @@
+/*
+ * Copyright Microsoft Corp. 2023
+ *
+ * domain_interface.c: methods to manage guest/domain interfaces
+ *
+ * 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 "virmacaddr.h"
+#include "virnetdevtap.h"
+#include "virconftypes.h"
+#include "domain_conf.h"
+#include "domain_interface.h"
+#include "domain_nwfilter.h"
+#include "domain_audit.h"
+#include "virebtables.h"
+#include "virlog.h"
+#include "virfile.h"
+#include "viralloc.h"
+#include "virnetdevbridge.h"
+#include "network_conf.h"
+
+#define VIR_FROM_THIS VIR_FROM_INTERFACE
+
+VIR_LOG_INIT("interface.interface_connect");
+
+bool
+virDomainInterfaceIsVnetCompatModel(const virDomainNetDef *net)
+{
+return (virDomainNetIsVirtioModel(net) ||
+net->model == VIR_DOMAIN_NET_MODEL_E1000E ||
+net->model == VIR_DOMAIN_NET_MODEL_IGB ||
+net->model == VIR_DOMAIN_NET_MODEL_VMXNET3);
+}
+
+/* virDomainInterfaceEthernetConnect:
+ * @def: the definition of the VM
+ * @driver: qemu driver data
+ * @net: pointer to the VM's interface description
+ * @tapfd: array of file descriptor return value for the new device
+ * @tapfdsize: number of file descriptors in @tapfd
+ *
+ * Called *only* called if actualType is VIR_DOMAIN_NET_TYPE_ETHERNET
+ * (i.e. if the connection is made with a tap device)
+ */
+int
+virDomainInterfaceEthernetConnect(virDomainDef *def,
+virDomainNetDef *net,
+ebtablesContext *ebtables,
+bool macFilter,
+bool privileged,
+int *tapfd,
+size_t tapfdSize)
+{
+virMacAddr tapmac;
+int ret = -1;
+unsigned int tap_create_flags = VIR_NETDEV_TAP_CREATE_IFUP;
+bool template_ifname = false;
+const char *tunpath = "/dev/net/tun";
+const char *auditdev = tunpath;
+
+if (net->backend.tap) {
+tunpath = net->backend.tap;
+if (!privileged) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("cannot use custom tap device in session mode"));
+goto cleanup;
+}
+}
+VIR_WARN("PPK: interfaceEthernetConnect Called\n");
+if (virDomainInterfaceIsVnetCompatModel(net))
+tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR;
+
+if (net->managed_tap == VIR_TRISTATE_BOOL_NO) {
+if (!net->ifname) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("target dev must be supplied when managed='no'"));
+goto cleanup;
+}
+if (virNetDevExists(net->ifname) != 1) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("target managed='no' but specified dev doesn't 
exist"));
+goto cleanup;
+}
+
+tap_create_flags |= VIR_NETDEV_TAP_CREAT

[PATCH 6/7] conf: Save translated disk definition for disk type='volume' to status XML

[Peter Krempa]

Re-translating the disk source pools when reconnecting to a VM makes no
sense as the volume might have changed or pool became inactive. The VM
still uses the original volume though. Failing to re-translate the pool
also causes the VM to be killed.

Fix this by storing the original translation in the status XML.

Resolves: https://issues.redhat.com/browse/RHEL-7345
Signed-off-by: Peter Krempa 
---
 src/conf/domain_conf.c  | 3 ++-
 src/conf/virdomainobjlist.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9b636215e9..80f467ae7a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -28247,7 +28247,8 @@ virDomainObjSave(virDomainObj *obj,
   VIR_DOMAIN_DEF_FORMAT_STATUS |
   VIR_DOMAIN_DEF_FORMAT_ACTUAL_NET |
   VIR_DOMAIN_DEF_FORMAT_PCI_ORIG_STATES |
-  VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST);
+  VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST |
+  VIR_DOMAIN_DEF_FORMAT_VOLUME_TRANSLATED);

 g_autofree char *xml = NULL;

diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 6be5de5e2b..0bd833257d 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -532,7 +532,8 @@ virDomainObjListLoadStatus(virDomainObjList *doms,
   VIR_DOMAIN_DEF_PARSE_ACTUAL_NET |
   VIR_DOMAIN_DEF_PARSE_PCI_ORIG_STATES |
   VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE |
-  
VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL)))
+  
VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL |
+  VIR_DOMAIN_DEF_PARSE_VOLUME_TRANSLATED)))
 goto error;

 virUUIDFormat(obj->def->uuid, uuidstr);
-- 
2.41.0

[PATCH 1/7] virStorageSourcePoolDef: Turn 'mode' member into proper enum type

[Peter Krempa]

Use proper enum type and refactor the formatter accordingly.

Signed-off-by: Peter Krempa 
---
 src/conf/domain_conf.c | 12 
 src/conf/storage_source_conf.h |  2 +-
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 4435ee2ad4..d7f167a469 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7011,7 +7011,6 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node,
 {
 virStorageSourcePoolDef *source;
 int ret = -1;
-g_autofree char *mode = NULL;

 *srcpool = NULL;

@@ -7019,7 +7018,6 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node,

 source->pool = virXMLPropString(node, "pool");
 source->volume = virXMLPropString(node, "volume");
-mode = virXMLPropString(node, "mode");

 /* CD-ROM and Floppy allows no source */
 if (!source->pool && !source->volume) {
@@ -7033,13 +7031,11 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node,
 goto cleanup;
 }

-if (mode &&
-(source->mode = virStorageSourcePoolModeTypeFromString(mode)) <= 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("unknown source mode '%1$s' for volume type disk"),
-   mode);
+if (virXMLPropEnum(node, "mode",
+   virStorageSourcePoolModeTypeFromString,
+   VIR_XML_PROP_NONZERO,
+   &source->mode) < 0)
 goto cleanup;
-}

 *srcpool = g_steal_pointer(&source);
 ret = 0;
diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h
index fc6c67f426..bfa8d625e5 100644
--- a/src/conf/storage_source_conf.h
+++ b/src/conf/storage_source_conf.h
@@ -201,7 +201,7 @@ struct _virStorageSourcePoolDef {
 int voltype; /* virStorageVolType, internal only */
 int pooltype; /* virStoragePoolType from storage_conf.h, internal only */
 virStorageType actualtype; /* internal only */
-int mode; /* virStorageSourcePoolMode, currently makes sense only for 
iscsi pool */
+virStorageSourcePoolMode mode; /* currently makes sense only for iscsi 
pool */
 };


-- 
2.41.0

[PATCH 5/7] qemustatusxml2xmltest: Demonstrate use of VIR_DOMAIN_DEF_(PARSE|FORMAT)_VOLUME_TRANSLATED

[Peter Krempa]

Enable the flags in the status xml2xmtest and add an exaple to the test
data.

Signed-off-by: Peter Krempa 
---
 tests/qemustatusxml2xmldata/modern-in.xml | 4 ++--
 tests/qemustatusxml2xmltest.c | 6 --
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/tests/qemustatusxml2xmldata/modern-in.xml 
b/tests/qemustatusxml2xmldata/modern-in.xml
index e139c8d38c..67e0aa4952 100644
--- a/tests/qemustatusxml2xmldata/modern-in.xml
+++ b/tests/qemustatusxml2xmldata/modern-in.xml
@@ -378,9 +378,9 @@
 
 
   
-  
+  
 
-
+
 
 
 
diff --git a/tests/qemustatusxml2xmltest.c b/tests/qemustatusxml2xmltest.c
index 418a724b94..f1589345c3 100644
--- a/tests/qemustatusxml2xmltest.c
+++ b/tests/qemustatusxml2xmltest.c
@@ -30,7 +30,8 @@ testCompareStatusXMLToXMLFiles(const void *opaque)
   VIR_DOMAIN_DEF_PARSE_ACTUAL_NET |
   VIR_DOMAIN_DEF_PARSE_PCI_ORIG_STATES |
   VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE |
-  
VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL))) {
+  
VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL |
+  
VIR_DOMAIN_DEF_PARSE_VOLUME_TRANSLATED))) {
 VIR_TEST_DEBUG("\nfailed to parse '%s'", data->infile);
 goto cleanup;
 }
@@ -40,7 +41,8 @@ testCompareStatusXMLToXMLFiles(const void *opaque)
   VIR_DOMAIN_DEF_FORMAT_STATUS |
   VIR_DOMAIN_DEF_FORMAT_ACTUAL_NET |
   VIR_DOMAIN_DEF_FORMAT_PCI_ORIG_STATES |
-  VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST))) {
+  VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST |
+  
VIR_DOMAIN_DEF_FORMAT_VOLUME_TRANSLATED))) {
 VIR_TEST_DEBUG("\nfailed to format back '%s'", data->infile);
 goto cleanup;
 }
-- 
2.41.0

[PATCH 2/7] virDomainDiskSourcePoolDefParse: Refactor cleanup

[Peter Krempa]

Register autoptr cleanup function for virStorageSourcePoolDef and
refactor the parser to simplify the logic.

Signed-off-by: Peter Krempa 
---
 src/conf/domain_conf.c | 35 +++---
 src/conf/storage_source_conf.h |  1 +
 2 files changed, 12 insertions(+), 24 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d7f167a469..3e0989e2e8 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7005,44 +7005,31 @@ virDomainLeaseDefParseXML(xmlNodePtr node,
 return NULL;
 }

-static int
-virDomainDiskSourcePoolDefParse(xmlNodePtr node,
-virStorageSourcePoolDef **srcpool)
+static virStorageSourcePoolDef *
+virDomainDiskSourcePoolDefParse(xmlNodePtr node)
 {
-virStorageSourcePoolDef *source;
-int ret = -1;
-
-*srcpool = NULL;
-
-source = g_new0(virStorageSourcePoolDef, 1);
+g_autoptr(virStorageSourcePoolDef) source = 
g_new0(virStorageSourcePoolDef, 1);

 source->pool = virXMLPropString(node, "pool");
 source->volume = virXMLPropString(node, "volume");

-/* CD-ROM and Floppy allows no source */
-if (!source->pool && !source->volume) {
-ret = 0;
-goto cleanup;
-}
+/* CD-ROM and Floppy allows no source -> empty pool */
+if (!source->pool && !source->volume)
+return g_steal_pointer(&source);

 if (!source->pool || !source->volume) {
 virReportError(VIR_ERR_XML_ERROR, "%s",
_("'pool' and 'volume' must be specified together for 
'pool' type source"));
-goto cleanup;
+return NULL;
 }

 if (virXMLPropEnum(node, "mode",
virStorageSourcePoolModeTypeFromString,
VIR_XML_PROP_NONZERO,
&source->mode) < 0)
-goto cleanup;
-
-*srcpool = g_steal_pointer(&source);
-ret = 0;
+return NULL;

- cleanup:
-virStorageSourcePoolDefFree(source);
-return ret;
+return g_steal_pointer(&source);
 }


@@ -7482,7 +7469,7 @@ virDomainStorageSourceParse(xmlNodePtr node,
 return -1;
 break;
 case VIR_STORAGE_TYPE_VOLUME:
-if (virDomainDiskSourcePoolDefParse(node, &src->srcpool) < 0)
+if (!(src->srcpool = virDomainDiskSourcePoolDefParse(node)))
 return -1;
 break;
 case VIR_STORAGE_TYPE_NVME:
@@ -8660,7 +8647,7 @@ virDomainFSDefParseXML(virDomainXMLOption *xmlopt,
 units = virXMLPropString(source_node, "units");
 } else if (def->type == VIR_DOMAIN_FS_TYPE_VOLUME) {
 def->src->type = VIR_STORAGE_TYPE_VOLUME;
-if (virDomainDiskSourcePoolDefParse(source_node, 
&def->src->srcpool) < 0)
+if (!(def->src->srcpool = 
virDomainDiskSourcePoolDefParse(source_node)))
 goto error;
 }
 }
diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h
index bfa8d625e5..0cd5cd0192 100644
--- a/src/conf/storage_source_conf.h
+++ b/src/conf/storage_source_conf.h
@@ -498,6 +498,7 @@ virStorageSourceInitChainElement(virStorageSource *newelem,

 void
 virStorageSourcePoolDefFree(virStorageSourcePoolDef *def);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStorageSourcePoolDef, 
virStorageSourcePoolDefFree);

 void
 virStorageSourceClear(virStorageSource *def);
-- 
2.41.0

[PATCH 4/7] qemu: domain: Allow preserving translated disk type='volume' data into XML if needed

[Peter Krempa]

Re-translating a disk type='volume' definition from a storage pool is
not a good idea in cases when the volume might have changed or we might
not have access to the storage driver.

Specific cases are if a storage pool is not activated on daemon restart,
then re-connecting to a VM fails, or if the virt-aa-helper program tries
to setup labelling for apparmor.

Add a new flag which will preserve the translated data in the
definition.

Signed-off-by: Peter Krempa 
---
 src/conf/domain_conf.c | 69 ++
 src/conf/domain_conf.h |  4 +++
 2 files changed, 53 insertions(+), 20 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e128457b00..9b636215e9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7006,7 +7006,8 @@ virDomainLeaseDefParseXML(xmlNodePtr node,
 }

 static virStorageSourcePoolDef *
-virDomainDiskSourcePoolDefParse(xmlNodePtr node)
+virDomainDiskSourcePoolDefParse(xmlNodePtr node,
+virDomainDefParseFlags flags)
 {
 g_autoptr(virStorageSourcePoolDef) source = 
g_new0(virStorageSourcePoolDef, 1);

@@ -7029,6 +7030,14 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node)
&source->mode) < 0)
 return NULL;

+if (flags & VIR_DOMAIN_DEF_PARSE_VOLUME_TRANSLATED) {
+if (virXMLPropEnum(node, "actualType",
+   virStorageTypeFromString,
+   VIR_XML_PROP_NONZERO,
+   &source->actualtype) < 0)
+return NULL;
+}
+
 return g_steal_pointer(&source);
 }

@@ -7448,12 +7457,22 @@ virDomainStorageSourceParse(xmlNodePtr node,
 unsigned int flags,
 virDomainXMLOption *xmlopt)
 {
+virStorageType actualType = src->type;
 VIR_XPATH_NODE_AUTORESTORE(ctxt)
 xmlNodePtr tmp;

 ctxt->node = node;

-switch (src->type) {
+if (src->type == VIR_STORAGE_TYPE_VOLUME) {
+if (!(src->srcpool = virDomainDiskSourcePoolDefParse(node, flags)))
+return -1;
+
+/* If requested we need to also parse the translated volume runtime 
data */
+if (flags & VIR_DOMAIN_DEF_PARSE_VOLUME_TRANSLATED)
+actualType = virStorageSourceGetActualType(src);
+}
+
+switch (actualType) {
 case VIR_STORAGE_TYPE_FILE:
 src->path = virXMLPropString(node, "file");
 src->fdgroup = virXMLPropString(node, "fdgroup");
@@ -7469,8 +7488,7 @@ virDomainStorageSourceParse(xmlNodePtr node,
 return -1;
 break;
 case VIR_STORAGE_TYPE_VOLUME:
-if (!(src->srcpool = virDomainDiskSourcePoolDefParse(node)))
-return -1;
+/* parsed above */
 break;
 case VIR_STORAGE_TYPE_NVME:
 if (virDomainDiskSourceNVMeParse(node, ctxt, src) < 0)
@@ -7488,7 +7506,7 @@ virDomainStorageSourceParse(xmlNodePtr node,
 case VIR_STORAGE_TYPE_LAST:
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected disk type %1$s"),
-   virStorageTypeToString(src->type));
+   virStorageTypeToString(actualType));
 return -1;
 }

@@ -8647,7 +8665,7 @@ virDomainFSDefParseXML(virDomainXMLOption *xmlopt,
 units = virXMLPropString(source_node, "units");
 } else if (def->type == VIR_DOMAIN_FS_TYPE_VOLUME) {
 def->src->type = VIR_STORAGE_TYPE_VOLUME;
-if (!(def->src->srcpool = 
virDomainDiskSourcePoolDefParse(source_node)))
+if (!(def->src->srcpool = 
virDomainDiskSourcePoolDefParse(source_node, flags)))
 goto error;
 }
 }
@@ -22335,10 +22353,28 @@ virDomainDiskSourceFormat(virBuffer *buf,
   bool skipEnc,
   virDomainXMLOption *xmlopt)
 {
+virStorageType actualType = src->type;
 g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
 g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);

-switch (src->type) {
+if (src->type == VIR_STORAGE_TYPE_VOLUME) {
+if (src->srcpool) {
+virBufferEscapeString(&attrBuf, " pool='%s'", src->srcpool->pool);
+virBufferEscapeString(&attrBuf, " volume='%s'", 
src->srcpool->volume);
+if (src->srcpool->mode)
+virBufferAsprintf(&attrBuf, " mode='%s'",
+  
virStorageSourcePoolModeTypeToString(src->srcpool->mode));
+}
+
+if (flags & VIR_DOMAIN_DEF_FORMAT_VOLUME_TRANSLATED &&
+src->srcpool->actualtype != VIR_STORAGE_TYPE_NONE) {
+virBufferAsprintf(&attrBuf, " actualType='%s'",
+  
virStorageTypeToString(src->srcpool->actualtype));
+actualType = virStorageSourceGetActualType(src);
+}
+}
+
+switch (actualType) {
 case VIR_STORAGE_TYPE_FILE:
 virBufferEscapeString(&attrBuf, " file='%s'", src->path);

[PATCH 0/7] qemu: Fix two pool-translation related bugs

[Peter Krempa]

Fix following problems:
 - fix disk type='volume' disks resolving to local images on apparmor-protected 
hosts
 - VMs are killed if pool is not available after libvirtd/virtqemud restart

The first one will be annoying for users of Cockpit on apparmor based
boxes. NOTE: I didn't test it though as I don't have such distro handy.

Peter Krempa (7):
  virStorageSourcePoolDef: Turn 'mode' member into proper enum type
  virDomainDiskSourcePoolDefParse: Refactor cleanup
  virDomainDiskTranslateSourcePool: Don't re-translate already
translated defs
  qemu: domain: Allow preserving translated disk type='volume' data into
XML if needed
  qemustatusxml2xmltest: Demonstrate use of
VIR_DOMAIN_DEF_(PARSE|FORMAT)_VOLUME_TRANSLATED
  conf: Save translated disk definition for disk type='volume' to status
XML
  security: apparmor: Use translated disk definitions for disk
type=volume

 src/conf/domain_conf.c| 111 --
 src/conf/domain_conf.h|   4 +
 src/conf/storage_source_conf.h|   3 +-
 src/conf/virdomainobjlist.c   |   3 +-
 src/security/security_apparmor.c  |   8 +-
 src/security/virt-aa-helper.c |   3 +-
 tests/qemustatusxml2xmldata/modern-in.xml |   4 +-
 tests/qemustatusxml2xmltest.c |   6 +-
 8 files changed, 84 insertions(+), 58 deletions(-)

-- 
2.41.0

[PATCH 3/7] virDomainDiskTranslateSourcePool: Don't re-translate already translated defs

[Peter Krempa]

If a disk definition was already translated re-doing it makes no sense.

Skip the translation if the 'actualtype' is already populated.

Signed-off-by: Peter Krempa 
---
 src/conf/domain_conf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3e0989e2e8..e128457b00 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -30529,7 +30529,7 @@ virDomainDiskTranslateSourcePool(virDomainDiskDef *def)
 virStorageSource *n;

 for (n = def->src; virStorageSourceIsBacking(n); n = n->backingStore) {
-if (n->type != VIR_STORAGE_TYPE_VOLUME || !n->srcpool)
+if (n->type != VIR_STORAGE_TYPE_VOLUME || !n->srcpool || 
n->srcpool->actualtype != VIR_STORAGE_TYPE_NONE)
 continue;

 if (!conn) {
-- 
2.41.0

[PATCH 7/7] security: apparmor: Use translated disk definitions for disk type=volume

[Peter Krempa]

The 'virt-aa-helper' process gets a XML of the VM it needs to create a
profile for. For a disk type='volume' this XML contained only the
pool and volume name.

The 'virt-aa-helper' needs a local path though for anything it needs to
label. This means that we'd either need to invoke connection to the
storage driver and re-resolve the volume. Alternative which makes more
sense is to pass the proper data in the XML already passed to it via the
new XML formatter and parser flags.

This was indirectly reported upstream in
https://gitlab.com/libvirt/libvirt/-/issues/546

The configuration in the issue above was created by Cockpit on Debian.
Since Cockpit is getting more popular it's more likely that users will
be impacted by this problem.

Signed-off-by: Peter Krempa 
---
 src/security/security_apparmor.c | 8 ++--
 src/security/virt-aa-helper.c| 3 ++-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index bce797de7c..6fd0aedacf 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -159,13 +159,17 @@ load_profile(virSecurityManager *mgr G_GNUC_UNUSED,
  bool append)
 {
 bool create = true;
+g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
 g_autofree char *xml = NULL;
 g_autoptr(virCommand) cmd = NULL;

-xml = virDomainDefFormat(def, NULL, VIR_DOMAIN_DEF_FORMAT_SECURE);
-if (!xml)
+if (virDomainDefFormatInternal(def, NULL, &buf,
+   VIR_DOMAIN_DEF_FORMAT_SECURE |
+   VIR_DOMAIN_DEF_FORMAT_VOLUME_TRANSLATED) < 
0)
 return -1;

+xml = virBufferContentAndReset(&buf);
+
 if (profile_status_file(profile) >= 0)
 create = false;

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 0855eb68ca..be13979cef 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -654,7 +654,8 @@ get_definition(vahControl * ctl, const char *xmlStr)
 ctl->def = virDomainDefParseString(xmlStr,
ctl->xmlopt, NULL,
VIR_DOMAIN_DEF_PARSE_SKIP_SECLABEL |
-   VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE);
+   VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE |
+   VIR_DOMAIN_DEF_PARSE_VOLUME_TRANSLATED);

 if (ctl->def == NULL) {
 vah_error(ctl, 0, _("could not parse XML"));
-- 
2.41.0

Re: [PATCH v3 4/4] migration: Deprecate old compression method

[Juan Quintela]

Markus Armbruster  wrote:
> Juan Quintela  writes:
>
>> -# @deprecated: @disk migration is deprecated.  Use driver-mirror
>> -# with NBD instead.
>> +# @deprecated: @disk migration is deprecated.  Use driver-mirror with
>> +# NBD instead.  @compression is unreliable and untested. It is
>> +# recommended to use multifd migration, which offers an
>> +# alternative compression implementation that is reliable and
>> +# tested.
>
> Again, single deprecation note under "Features:", please.

Done in all places.

>
>>  #
>>  # Since: 0.14
>>  ##
>> @@ -290,7 +295,7 @@
>> '*blocked-reasons': ['str'],
>> '*postcopy-blocktime': 'uint32',
>> '*postcopy-vcpu-blocktime': ['uint32'],
>> -   '*compression': 'CompressionStats',
>> +   '*compression': { 'type': 'CompressionStats', 'features': 
>> ['deprecated'] },
>> '*socket-address': ['SocketAddress'],
>> '*dirty-limit-throttle-time-per-round': 'uint64',
>> '*dirty-limit-ring-full-time': 'uint64'} }
>> @@ -445,7 +450,8 @@
>>  # compress and xbzrle are both on, compress only takes effect in
>>  # the ram bulk stage, after that, it will be disabled and only
>>  # xbzrle takes effect, this can help to minimize migration
>> -# traffic.  The feature is disabled by default.  (since 2.4 )
>> +# traffic.  The feature is disabled by default.  Obsolete.  Use
>
> Some places call it "deprecated", others "obsolete".  Best to always use
> "deprecated".

"obsolete" word removed from this series O:-)

> More of the same below.
>
>> +# multifd compression methods if needed. (since 2.4 )
>>  #
>>  # @events: generate events for each migration state change (since 2.4
>>  # )
>
> [...]
>
> Migration has grown way too many options.  Thanks for pruning them back
> some.

I am going to redo the 1st patches to just print an error if you use
-i/-b on HMP.  So that will clean up your last comment for that patch.

Thanks.

Re: [PATCH v3 3/4] migration: Deprecate block migration

[Juan Quintela]

Markus Armbruster  wrote:
> Juan Quintela  writes:
>
>> It is obsolete.  It is better to use driver-mirror with NBD instead.
>
> drive-mirror
>
> Several more below.

Done.

>> +# Features:
>> +#
>> +# @deprecated: @disk migration is deprecated.  Use driver-mirror
>> +# with NBD instead.
>> +#
>
> Suggest:
>
># @deprecated: Member @disk is deprecated because block migration is.

Done.

>>  # Since: 0.14
>>  ##
>>  { 'struct': 'MigrationInfo',
>>'data': {'*status': 'MigrationStatus', '*ram': 'MigrationStats',
>> -   '*disk': 'MigrationStats',
>> +   '*disk': { 'type': 'MigrationStats', 'features': ['deprecated'] 
>> },
>> '*vfio': 'VfioStats',
>> '*xbzrle-cache': 'XBZRLECacheStats',
>> '*total-time': 'int',
>> @@ -526,6 +531,9 @@
>>  #
>>  # Features:
>>  #
>> +# @deprecated: @block migration is deprecated.  Use driver-mirror
>> +# with NBD instead.
>> +#
>
> Suggest:
>
># @deprecated: Member @block is deprecated.  Use drrive-mirror with
># NBD instead.

Done.

>>  # @unstable: Members @x-colo and @x-ignore-shared are experimental.
>>  #
>>  # Since: 1.2
>> @@ -535,7 +543,8 @@
>> 'compress', 'events', 'postcopy-ram',
>> { 'name': 'x-colo', 'features': [ 'unstable' ] },
>> 'release-ram',
>> -   'block', 'return-path', 'pause-before-switchover', 'multifd',
>> +   { 'name': 'block', 'features': [ 'deprecated' ] },
>> +   'return-path', 'pause-before-switchover', 'multifd',
>> 'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
>> { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] },
>> 'validate-uuid', 'background-snapshot',
>> @@ -826,6 +835,9 @@
>>  #
>>  # Features:
>>  #
>> +# @deprecated: Member @block-incremental is obsolete. Use
>> +# driver-mirror with NBD instead.
>> +#
>
> Wait!  This is what PATCH 1 tells users to use instead of deprecated
> @inc.  You need to update that deprecation note to point to a
> non-deprecated replacement.

I don't have.
I want to deprecate the whole thing.
But the options are more urgent to deprecate/remove.  They are
implemented in a very hacky way.  Even for migration/qemu standards.

Later, Juan.

Re: [PATCH v3 3/4] migration: Deprecate block migration

[Kevin Wolf]

Am 12.10.2023 um 12:01 hat Markus Armbruster geschrieben:
> Juan Quintela  writes:
> 
> > It is obsolete.  It is better to use driver-mirror with NBD instead.
> 
> drive-mirror
> 
> Several more below.

Actually, blockdev-mirror, please. We don't want to move people from the
oldest way to the next oldest one. Pointing them to the modern way makes
more sense.

> >
> > CC: Kevin Wolf 
> > CC: Eric Blake 
> > CC: Stefan Hajnoczi 
> > CC: Hanna Czenczek 
> >
> > Signed-off-by: Juan Quintela 

Kevin

Re: [PATCH v3 1/4] migration: migrate 'inc' command option is deprecated.

[Juan Quintela]

Markus Armbruster  wrote:
> Juan Quintela  writes:
>
>> Set the 'block_incremental' migration parameter to 'true' instead.
>>
>>  # @blk: do block migration (full disk copy)
>>  #
>> -# @inc: incremental disk copy migration
>> +# @inc: incremental disk copy migration.  This option is deprecated.
>> +# Set the 'block-incremetantal' migration parameter to 'true'
>> +# instead.
>
> 'block-incremental'

Done, thanks.

>>  #
>>  # @detach: this argument exists only for compatibility reasons and is
>>  # ignored by QEMU
>>  #
>>  # @resume: resume one paused migration, default "off". (since 3.0)
>>  #
>> +# Features:
>> +#
>> +# @deprecated: option @inc should be enabled by setting the
>> +# 'block-incremental' migration parameter to 'true'.
>> +#
>
> You add deprecation notices, one to the member documentation, and one to
> the "Features:" section.  You should add just one, to the "Features:"
> section.  Suggest:
>
># @deprecated: Member @inc is deprecated.  Use migration parameter
># @block-incremental instead.

Done.

>>  # Returns: nothing on success
>>  #
>>  # Since: 0.14
>> @@ -1514,7 +1521,8 @@
>>  # <- { "return": {} }
>>  ##
>>  { 'command': 'migrate',
>> -  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
>> +  'data': {'uri': 'str', '*blk': 'bool',
>> +   '*inc': { 'type': 'bool', 'features': ['deprecated'] },
>
> For better or worse, we format like [ 'deprecated' ].

Done.

>> '*detach': 'bool', '*resume': 'bool' } }
>>  
>>  ##
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 1c6c81ad49..c7e4c37b8a 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1601,6 +1601,12 @@ static bool migrate_prepare(MigrationState *s, bool 
>> blk, bool blk_inc,
>>  {
>>  Error *local_err = NULL;
>>  
>> +if (blk_inc) {
>> +warn_report("-inc migrate option is deprecated, set the "
>> +"'block-incremental' migration parameter to 'true'"
>> +" instead.");
>
> There is no "-inc migrate option".  You're refering to QMP command
> migrate's parameter @inc / HMP command migrate's flag -i.

Changed to:


s|-inc|@inc/-i|

>> +}
>> +
>>  if (resume) {
>>  if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) {
>>  error_setg(errp, "Cannot resume if there is no "
>
> As far as I can see, HMP command migrate still uses the deprecated
> interface:
>
> qmp_migrate(uri, !!blk, blk, !!inc, inc,
> false, false, true, resume, &err);
>
> Its use should be replaced before we deprecate it.

We need to drop it.

Blockjobs are much more flexible.  We want to get rid of the whole
concept of block migration inside the migration protocol/machinery.

Block migration requires that one:

- migrate all devices, i.e. no way to select some shared some local.

- I think that incremental bit requires that you use qcow2 images, but I
  haven't even double checked them.

I just want to drop it in the near future, if 9.0 is too soon, for
10.0.

Later, Juan.

Re: [PATCH v3 4/4] migration: Deprecate old compression method

[Markus Armbruster]

Juan Quintela  writes:

> Signed-off-by: Juan Quintela 
> Acked-by: Peter Xu 
> ---
>  docs/about/deprecated.rst |   8 +++
>  qapi/migration.json   | 102 --
>  migration/options.c   |  13 +
>  3 files changed, 86 insertions(+), 37 deletions(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index e46f3df376..b92621996f 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -441,3 +441,11 @@ Please see "QMP invocation for live storage migration 
> with
>  ``driver-mirror`` + NBD" in docs/interop/live-block-operations.rst for
>  a detailed explanation.
>  
> +old compression method (since 8.2)
> +''
> +
> +Compression method fails too much.  Too many races.  We are going to
> +remove it if nobody fixes it.  For starters, migration-test
> +compression tests are disabled becase they fail randomly.  If you need
> +compression, use multifd compression methods.
> +
> diff --git a/qapi/migration.json b/qapi/migration.json
> index bf6e1ac5b5..549306fa76 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -243,7 +243,9 @@
>  #
>  # @compression: migration compression statistics, only returned if
>  # compression feature is on and status is 'active' or 'completed'
> -# (Since 3.1)
> +# This feature is unreliable and not tested. It is recommended to
> +# use multifd migration instead, which offers an alternative
> +# reliable and tested compression implementation.  (Since 3.1)
>  #
>  # @socket-address: Only used for tcp, to know what the real port is
>  # (Since 4.0)
> @@ -271,8 +273,11 @@
>  #
>  # Features:
>  #
> -# @deprecated: @disk migration is deprecated.  Use driver-mirror
> -# with NBD instead.
> +# @deprecated: @disk migration is deprecated.  Use driver-mirror with
> +# NBD instead.  @compression is unreliable and untested. It is
> +# recommended to use multifd migration, which offers an
> +# alternative compression implementation that is reliable and
> +# tested.

Again, single deprecation note under "Features:", please.

>  #
>  # Since: 0.14
>  ##
> @@ -290,7 +295,7 @@
> '*blocked-reasons': ['str'],
> '*postcopy-blocktime': 'uint32',
> '*postcopy-vcpu-blocktime': ['uint32'],
> -   '*compression': 'CompressionStats',
> +   '*compression': { 'type': 'CompressionStats', 'features': 
> ['deprecated'] },
> '*socket-address': ['SocketAddress'],
> '*dirty-limit-throttle-time-per-round': 'uint64',
> '*dirty-limit-ring-full-time': 'uint64'} }
> @@ -445,7 +450,8 @@
>  # compress and xbzrle are both on, compress only takes effect in
>  # the ram bulk stage, after that, it will be disabled and only
>  # xbzrle takes effect, this can help to minimize migration
> -# traffic.  The feature is disabled by default.  (since 2.4 )
> +# traffic.  The feature is disabled by default.  Obsolete.  Use

Some places call it "deprecated", others "obsolete".  Best to always use
"deprecated".

More of the same below.

> +# multifd compression methods if needed. (since 2.4 )
>  #
>  # @events: generate events for each migration state change (since 2.4
>  # )

[...]

Migration has grown way too many options.  Thanks for pruning them back
some.

Re: [PATCH v3 3/4] migration: Deprecate block migration

[Markus Armbruster]

Juan Quintela  writes:

> It is obsolete.  It is better to use driver-mirror with NBD instead.

drive-mirror

Several more below.

>
> CC: Kevin Wolf 
> CC: Eric Blake 
> CC: Stefan Hajnoczi 
> CC: Hanna Czenczek 
>
> Signed-off-by: Juan Quintela 
> ---
>  docs/about/deprecated.rst | 10 ++
>  qapi/migration.json   | 30 +-
>  migration/block.c |  3 +++
>  migration/options.c   |  9 -
>  4 files changed, 46 insertions(+), 6 deletions(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index b2b7e11742..e46f3df376 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -431,3 +431,13 @@ The new way to modify migration is using migration 
> parameters.
>  ``blk`` functionality can be achieved by setting the
>  ``block`` migration capability to ``true``.
>  
> +block migration (since 8.2)
> +'''
> +
> +Block migration is too inflexible.  It needs to migrate all block
> +devices or none.
> +
> +Please see "QMP invocation for live storage migration with
> +``driver-mirror`` + NBD" in docs/interop/live-block-operations.rst for
> +a detailed explanation.
> +
> diff --git a/qapi/migration.json b/qapi/migration.json
> index e4949e0d8e..bf6e1ac5b5 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -269,11 +269,16 @@
>  # average memory load of the virtual CPU indirectly.  Note that
>  # zero means guest doesn't dirty memory.  (Since 8.1)
>  #
> +# Features:
> +#
> +# @deprecated: @disk migration is deprecated.  Use driver-mirror
> +# with NBD instead.
> +#

Suggest:

   # @deprecated: Member @disk is deprecated because block migration is.

>  # Since: 0.14
>  ##
>  { 'struct': 'MigrationInfo',
>'data': {'*status': 'MigrationStatus', '*ram': 'MigrationStats',
> -   '*disk': 'MigrationStats',
> +   '*disk': { 'type': 'MigrationStats', 'features': ['deprecated'] },
> '*vfio': 'VfioStats',
> '*xbzrle-cache': 'XBZRLECacheStats',
> '*total-time': 'int',
> @@ -526,6 +531,9 @@
>  #
>  # Features:
>  #
> +# @deprecated: @block migration is deprecated.  Use driver-mirror
> +# with NBD instead.
> +#

Suggest:

   # @deprecated: Member @block is deprecated.  Use drrive-mirror with
   # NBD instead.

>  # @unstable: Members @x-colo and @x-ignore-shared are experimental.
>  #
>  # Since: 1.2
> @@ -535,7 +543,8 @@
> 'compress', 'events', 'postcopy-ram',
> { 'name': 'x-colo', 'features': [ 'unstable' ] },
> 'release-ram',
> -   'block', 'return-path', 'pause-before-switchover', 'multifd',
> +   { 'name': 'block', 'features': [ 'deprecated' ] },
> +   'return-path', 'pause-before-switchover', 'multifd',
> 'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
> { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] },
> 'validate-uuid', 'background-snapshot',
> @@ -826,6 +835,9 @@
>  #
>  # Features:
>  #
> +# @deprecated: Member @block-incremental is obsolete. Use
> +# driver-mirror with NBD instead.
> +#

Wait!  This is what PATCH 1 tells users to use instead of deprecated
@inc.  You need to update that deprecation note to point to a
non-deprecated replacement.

>  # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
>  # are experimental.
>  #
> @@ -841,7 +853,7 @@
> 'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth',
> 'downtime-limit',
> { 'name': 'x-checkpoint-delay', 'features': [ 'unstable' ] },
> -   'block-incremental',
> +   { 'name': 'block-incremental', 'features': [ 'deprecated' ] },
> 'multifd-channels',
> 'xbzrle-cache-size', 'max-postcopy-bandwidth',
> 'max-cpu-throttle', 'multifd-compression',
> @@ -992,6 +1004,9 @@
>  #
>  # Features:
>  #
> +# @deprecated: Member @block-incremental is obsolete. Use
> +# driver-mirror with NBD instead.
> +#
>  # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
>  # are experimental.
>  #
> @@ -1020,7 +1035,8 @@
>  '*downtime-limit': 'uint64',
>  '*x-checkpoint-delay': { 'type': 'uint32',
>   'features': [ 'unstable' ] },
> -'*block-incremental': 'bool',
> +'*block-incremental': { 'type': 'bool',
> +'features': [ 'deprecated' ] },
>  '*multifd-channels': 'uint8',
>  '*xbzrle-cache-size': 'size',
>  '*max-postcopy-bandwidth': 'size',
> @@ -1195,6 +1211,9 @@
>  #
>  # Features:
>  #
> +# @deprecated: Member @block-incremental is obsolete. Use
> +# driver-mirror with NBD instead.
> +#
>  # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
>  # are experimental.
>  #
> @@ -1220,7 +1239,8 @@
>  '*downtime-limit': 'uint64',
> 

Re: [PATCH v3 1/4] migration: migrate 'inc' command option is deprecated.

[Markus Armbruster]

Juan Quintela  writes:

> Set the 'block_incremental' migration parameter to 'true' instead.
>
> Reviewed-by: Thomas Huth 
> Signed-off-by: Juan Quintela 
> ---
>  docs/about/deprecated.rst |  7 +++
>  qapi/migration.json   | 12 ++--
>  migration/migration.c |  6 ++
>  3 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 8b136320e2..eb0f326f00 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -417,3 +417,10 @@ Migration
>  ``skipped`` field in Migration stats has been deprecated.  It hasn't
>  been used for more than 10 years.
>  
> +``inc`` migrate command option (since 8.2)
> +''
> +
> +The new way to modify migration is using migration parameters.
> +``inc`` functionality can be achieved by setting the
> +``block-incremental`` migration parameter to ``true``.
> +
> diff --git a/qapi/migration.json b/qapi/migration.json
> index d7dfaa5db9..7669c98c7a 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1486,13 +1486,20 @@
>  #
>  # @blk: do block migration (full disk copy)
>  #
> -# @inc: incremental disk copy migration
> +# @inc: incremental disk copy migration.  This option is deprecated.
> +# Set the 'block-incremetantal' migration parameter to 'true'
> +# instead.

'block-incremental'

>  #
>  # @detach: this argument exists only for compatibility reasons and is
>  # ignored by QEMU
>  #
>  # @resume: resume one paused migration, default "off". (since 3.0)
>  #
> +# Features:
> +#
> +# @deprecated: option @inc should be enabled by setting the
> +# 'block-incremental' migration parameter to 'true'.
> +#

You add deprecation notices, one to the member documentation, and one to
the "Features:" section.  You should add just one, to the "Features:"
section.  Suggest:

   # @deprecated: Member @inc is deprecated.  Use migration parameter
   # @block-incremental instead.

>  # Returns: nothing on success
>  #
>  # Since: 0.14
> @@ -1514,7 +1521,8 @@
>  # <- { "return": {} }
>  ##
>  { 'command': 'migrate',
> -  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
> +  'data': {'uri': 'str', '*blk': 'bool',
> +   '*inc': { 'type': 'bool', 'features': ['deprecated'] },

For better or worse, we format like [ 'deprecated' ].

> '*detach': 'bool', '*resume': 'bool' } }
>  
>  ##
> diff --git a/migration/migration.c b/migration/migration.c
> index 1c6c81ad49..c7e4c37b8a 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1601,6 +1601,12 @@ static bool migrate_prepare(MigrationState *s, bool 
> blk, bool blk_inc,
>  {
>  Error *local_err = NULL;
>  
> +if (blk_inc) {
> +warn_report("-inc migrate option is deprecated, set the "
> +"'block-incremental' migration parameter to 'true'"
> +" instead.");

There is no "-inc migrate option".  You're refering to QMP command
migrate's parameter @inc / HMP command migrate's flag -i.

> +}
> +
>  if (resume) {
>  if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) {
>  error_setg(errp, "Cannot resume if there is no "

As far as I can see, HMP command migrate still uses the deprecated
interface:

qmp_migrate(uri, !!blk, blk, !!inc, inc,
false, false, true, resume, &err);

Its use should be replaced before we deprecate it.