7:58 a.m.
I've noticed these while reviewing Boris' patch:
https://www.redhat.com/archives/libvir-list/2021-January/msg01149.html
Michal Prívozník (3):
conf: Move virDomainCheckVirtioOptions() into domain_validate.c
conf: Drop empty virDomainNetDefPostParse()
conf: Improve virDomainVirtioOptionsCheckABIStability()
src/conf/domain_conf.c | 83 +++++++++-----------------------------
src/conf/domain_validate.c | 55 ++++++++++++++++++++-----
2 files changed, 64 insertions(+), 74 deletions(-)
--
2.26.2
7:58 a.m.
New subject: [PATCH 1/3] conf: Move virDomainCheckVirtioOptions() into domain_validate.c
The aim of virDomainCheckVirtioOptions() function is to check
whether no virtio options are set, i.e. no @iommu no @ats and no
@packed attributes were present in given device's XML (yeah, the
function has very misleading name). Nevertheless, this kind of
check belongs to validation phase, but now is done in post parse
phase. Move the function and its calls to domain_validate.c so
that future code is not tempted to repeat this mistake.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 40 +--------------------------
src/conf/domain_validate.c | 55 +++++++++++++++++++++++++++++++-------
2 files changed, 47 insertions(+), 48 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index dab4f10326..69ebd5d66d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5132,34 +5132,6 @@ virDomainHostdevDefPostParse(virDomainHostdevDefPtr dev,
}
-static int
-virDomainCheckVirtioOptions(virDomainVirtioOptionsPtr virtio)
-{
- if (!virtio)
- return 0;
-
- if (virtio->iommu != VIR_TRISTATE_SWITCH_ABSENT) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("iommu driver option is only supported "
- "for virtio devices"));
- return -1;
- }
- if (virtio->ats != VIR_TRISTATE_SWITCH_ABSENT) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("ats driver option is only supported "
- "for virtio devices"));
- return -1;
- }
- if (virtio->packed != VIR_TRISTATE_SWITCH_ABSENT) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("packed driver option is only supported "
- "for virtio devices"));
- return -1;
- }
- return 0;
-}
-
-
static void
virDomainChrDefPostParse(virDomainChrDefPtr chr,
const virDomainDef *def)
@@ -5256,11 +5228,6 @@ virDomainDiskDefPostParse(virDomainDiskDefPtr disk,
virDomainPostParseCheckISCSIPath(&disk->src->path);
}
- if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO &&
- virDomainCheckVirtioOptions(disk->virtio) < 0) {
- return -1;
- }
-
if (disk->src->type == VIR_STORAGE_TYPE_NVME) {
if (disk->src->nvme->managed == VIR_TRISTATE_BOOL_ABSENT)
disk->src->nvme->managed = VIR_TRISTATE_BOOL_YES;
@@ -5310,13 +5277,8 @@ virDomainControllerDefPostParse(virDomainControllerDefPtr cdev)
static int
-virDomainNetDefPostParse(virDomainNetDefPtr net)
+virDomainNetDefPostParse(virDomainNetDefPtr net G_GNUC_UNUSED)
{
- if (!virDomainNetIsVirtioModel(net) &&
- virDomainCheckVirtioOptions(net->virtio) < 0) {
- return -1;
- }
-
return 0;
}
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 649fc335ac..37c09ff648 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -226,6 +226,34 @@ virSecurityDeviceLabelDefValidate(virSecurityDeviceLabelDefPtr
*seclabels,
}
+static int
+virDomainCheckVirtioOptions(virDomainVirtioOptionsPtr virtio)
+{
+ if (!virtio)
+ return 0;
+
+ if (virtio->iommu != VIR_TRISTATE_SWITCH_ABSENT) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("iommu driver option is only supported "
+ "for virtio devices"));
+ return -1;
+ }
+ if (virtio->ats != VIR_TRISTATE_SWITCH_ABSENT) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("ats driver option is only supported "
+ "for virtio devices"));
+ return -1;
+ }
+ if (virtio->packed != VIR_TRISTATE_SWITCH_ABSENT) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("packed driver option is only supported "
+ "for virtio devices"));
+ return -1;
+ }
+ return 0;
+}
+
+
#define VENDOR_LEN 8
#define PRODUCT_LEN 16
@@ -277,15 +305,19 @@ virDomainDiskDefValidate(const virDomainDef *def,
return -1;
}
- if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO &&
- (disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO ||
- disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL ||
- disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("disk model '%s' not supported for bus
'%s'"),
- virDomainDiskModelTypeToString(disk->model),
- virDomainDiskBusTypeToString(disk->bus));
- return -1;
+ if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) {
+ if (disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO ||
+ disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL ||
+ disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("disk model '%s' not supported for bus
'%s'"),
+ virDomainDiskModelTypeToString(disk->model),
+ virDomainDiskBusTypeToString(disk->bus));
+ return -1;
+ }
+
+ if (virDomainCheckVirtioOptions(disk->virtio) < 0)
+ return -1;
}
if (disk->src->type == VIR_STORAGE_TYPE_NVME) {
@@ -1330,6 +1362,11 @@ virDomainNetDefValidate(const virDomainNetDef *net)
return -1;
}
+ if (!virDomainNetIsVirtioModel(net) &&
+ virDomainCheckVirtioOptions(net->virtio) < 0) {
+ return -1;
+ }
+
return 0;
}
--
2.26.2
7:58 a.m.
New subject: [PATCH 2/3] conf: Drop empty virDomainNetDefPostParse()
The previous commit rendered this function empty and needless.
Remove it.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 12 +-----------
1 file changed, 1 insertion(+), 11 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 69ebd5d66d..6e90b8e180 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5276,13 +5276,6 @@ virDomainControllerDefPostParse(virDomainControllerDefPtr cdev)
}
-static int
-virDomainNetDefPostParse(virDomainNetDefPtr net G_GNUC_UNUSED)
-{
- return 0;
-}
-
-
static void
virDomainVsockDefPostParse(virDomainVsockDefPtr vsock)
{
@@ -5367,10 +5360,6 @@ virDomainDeviceDefPostParseCommon(virDomainDeviceDefPtr dev,
ret = virDomainControllerDefPostParse(dev->data.controller);
break;
- case VIR_DOMAIN_DEVICE_NET:
- ret = virDomainNetDefPostParse(dev->data.net);
- break;
-
case VIR_DOMAIN_DEVICE_VSOCK:
virDomainVsockDefPostParse(dev->data.vsock);
ret = 0;
@@ -5382,6 +5371,7 @@ virDomainDeviceDefPostParseCommon(virDomainDeviceDefPtr dev,
case VIR_DOMAIN_DEVICE_LEASE:
case VIR_DOMAIN_DEVICE_FS:
+ case VIR_DOMAIN_DEVICE_NET:
case VIR_DOMAIN_DEVICE_INPUT:
case VIR_DOMAIN_DEVICE_SOUND:
case VIR_DOMAIN_DEVICE_WATCHDOG:
--
2.26.2
7:58 a.m.
New subject: [PATCH 3/3] conf: Improve virDomainVirtioOptionsCheckABIStability()
The virDomainVirtioOptionsCheckABIStability() function is called
from various ABI stability check functions. Every caller checks
if both old and new definitions have virtio options set and only
after that they call the function. This is suboptimal because:
a) this check can be done in the function itself (making all
callers shorter),
b) is inherently wrong, because it doesn't catch case where one
definition has virtio options set and the other doesn't.
Do proper checks at the beginning of the function and simplify
its calls.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 33 +++++++++++++++++----------------
1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6e90b8e180..905f8f0691 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -21533,6 +21533,15 @@ static bool
virDomainVirtioOptionsCheckABIStability(virDomainVirtioOptionsPtr src,
virDomainVirtioOptionsPtr dst)
{
+ if (!src && !dst)
+ return true;
+
+ if (!src || !dst) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Target device virtio options don't match the
source"));
+ return false;
+ }
+
if (src->iommu != dst->iommu) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Target device iommu option '%s' does not "
@@ -21617,8 +21626,7 @@ virDomainDiskDefCheckABIStability(virDomainDiskDefPtr src,
return false;
}
- if (src->virtio && dst->virtio &&
- !virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
+ if (!virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
return false;
if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info))
@@ -21677,8 +21685,7 @@ virDomainControllerDefCheckABIStability(virDomainControllerDefPtr
src,
}
}
- if (src->virtio && dst->virtio &&
- !virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
+ if (!virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
return false;
if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info))
@@ -21711,8 +21718,7 @@ virDomainFsDefCheckABIStability(virDomainFSDefPtr src,
return false;
}
- if (src->virtio && dst->virtio &&
- !virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
+ if (!virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
return false;
if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info))
@@ -21760,8 +21766,7 @@ virDomainNetDefCheckABIStability(virDomainNetDefPtr src,
return false;
}
- if (src->virtio && dst->virtio &&
- !virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
+ if (!virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
return false;
if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info))
@@ -21799,8 +21804,7 @@ virDomainInputDefCheckABIStability(virDomainInputDefPtr src,
return false;
}
- if (src->virtio && dst->virtio &&
- !virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
+ if (!virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
return false;
if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info))
@@ -21899,8 +21903,7 @@ virDomainVideoDefCheckABIStability(virDomainVideoDefPtr src,
}
}
- if (src->virtio && dst->virtio &&
- !virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
+ if (!virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
return false;
if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info))
@@ -22113,8 +22116,7 @@ virDomainMemballoonDefCheckABIStability(virDomainMemballoonDefPtr
src,
return false;
}
- if (src->virtio && dst->virtio &&
- !virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
+ if (!virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
return false;
if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info))
@@ -22136,8 +22138,7 @@ virDomainRNGDefCheckABIStability(virDomainRNGDefPtr src,
return false;
}
- if (src->virtio && dst->virtio &&
- !virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
+ if (!virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
return false;
if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info))
--
2.26.2
4:26 a.m.
On a Thursday in 2021, Michal Privoznik wrote:
I've noticed these while reviewing Boris' patch:
https://www.redhat.com/archives/libvir-list/2021-January/msg01149.html
Michal Prívozník (3):
conf: Move virDomainCheckVirtioOptions() into domain_validate.c
conf: Drop empty virDomainNetDefPostParse()
conf: Improve virDomainVirtioOptionsCheckABIStability()
src/conf/domain_conf.c | 83 +++++++++-----------------------------
src/conf/domain_validate.c | 55 ++++++++++++++++++++-----
2 files changed, 64 insertions(+), 74 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
1374
days inactive
1375
days old
4 comments
2 participants
participants (2)
-
Ján Tomko -
Michal Privoznik