src/conf/domain_validate.c | 9 +++++++++ ...t-user-slirp-portforward.x86_64-latest.err | 1 + .../net-user-slirp-portforward.xml | 20 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 4 files changed, 31 insertions(+) create mode 100644 tests/qemuxml2argvdata/net-user-slirp-portforward.x86_64-latest.err create mode 100644 tests/qemuxml2argvdata/net-user-slirp-portforward.xml
That's already the case in practice, but it's a better
experience for the user if we reject this configuration
outright instead of silently ignoring part of it.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
src/conf/domain_validate.c | 9 +++++++++
...t-user-slirp-portforward.x86_64-latest.err | 1 +
.../net-user-slirp-portforward.xml | 20 +++++++++++++++++++
tests/qemuxml2argvtest.c | 1 +
4 files changed, 31 insertions(+)
create mode 100644 tests/qemuxml2argvdata/net-user-slirp-portforward.x86_64-latest.err
create mode 100644 tests/qemuxml2argvdata/net-user-slirp-portforward.xml
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index ce6b8bf5a0..9c7ee6d75d 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -2097,6 +2097,15 @@ virDomainNetDefValidate(const virDomainNetDef *net)
}
}
+ if (net->nPortForwards > 0 &&
+ (net->type != VIR_DOMAIN_NET_TYPE_USER ||
+ (net->type == VIR_DOMAIN_NET_TYPE_USER &&
+ net->backend.type != VIR_DOMAIN_NET_BACKEND_PASST))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("The <portForward> element can only be used with <interface type='user'> and its 'passt' backend"));
+ return -1;
+ }
+
switch (net->type) {
case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
if (!virDomainNetIsVirtioModel(net)) {
diff --git a/tests/qemuxml2argvdata/net-user-slirp-portforward.x86_64-latest.err b/tests/qemuxml2argvdata/net-user-slirp-portforward.x86_64-latest.err
new file mode 100644
index 0000000000..f296db1e8c
--- /dev/null
+++ b/tests/qemuxml2argvdata/net-user-slirp-portforward.x86_64-latest.err
@@ -0,0 +1 @@
+internal error: The <portForward> element can only be used with <interface type='user'> and its 'passt' backend
diff --git a/tests/qemuxml2argvdata/net-user-slirp-portforward.xml b/tests/qemuxml2argvdata/net-user-slirp-portforward.xml
new file mode 100644
index 0000000000..721f04c878
--- /dev/null
+++ b/tests/qemuxml2argvdata/net-user-slirp-portforward.xml
@@ -0,0 +1,20 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219136</memory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <interface type='user'>
+ <mac address='00:11:22:33:44:55'/>
+ <portForward proto='tcp'>
+ <range start='443' to='344'/>
+ </portForward>
+ <model type='virtio'/>
+ </interface>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 1808d9fc02..23e0c4054c 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1413,6 +1413,7 @@ mymain(void)
DO_TEST_NOCAPS("net-user-addr");
DO_TEST_CAPS_LATEST("net-user-passt");
DO_TEST_CAPS_VER("net-user-passt", "7.2.0");
+ DO_TEST_CAPS_LATEST_PARSE_ERROR("net-user-slirp-portforward");
DO_TEST_NOCAPS("net-virtio");
DO_TEST_NOCAPS("net-virtio-device");
DO_TEST_NOCAPS("net-virtio-disable-offloads");
--
2.39.2
On a Tuesday in 2023, Andrea Bolognani wrote: >That's already the case in practice, but it's a better >experience for the user if we reject this configuration >outright instead of silently ignoring part of it. > >Signed-off-by: Andrea Bolognani <abologna@redhat.com> >--- > src/conf/domain_validate.c | 9 +++++++++ > ...t-user-slirp-portforward.x86_64-latest.err | 1 + > .../net-user-slirp-portforward.xml | 20 +++++++++++++++++++ > tests/qemuxml2argvtest.c | 1 + > 4 files changed, 31 insertions(+) > create mode 100644 tests/qemuxml2argvdata/net-user-slirp-portforward.x86_64-latest.err > create mode 100644 tests/qemuxml2argvdata/net-user-slirp-portforward.xml > Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Tue, Apr 18, 2023 at 03:19:45PM +0200, Ján Tomko wrote: > On a Tuesday in 2023, Andrea Bolognani wrote: > > That's already the case in practice, but it's a better > > experience for the user if we reject this configuration > > outright instead of silently ignoring part of it. > > > > Signed-off-by: Andrea Bolognani <abologna@redhat.com> > > --- > > src/conf/domain_validate.c | 9 +++++++++ > > ...t-user-slirp-portforward.x86_64-latest.err | 1 + > > .../net-user-slirp-portforward.xml | 20 +++++++++++++++++++ > > tests/qemuxml2argvtest.c | 1 + > > 4 files changed, 31 insertions(+) > > create mode 100644 tests/qemuxml2argvdata/net-user-slirp-portforward.x86_64-latest.err > > create mode 100644 tests/qemuxml2argvdata/net-user-slirp-portforward.xml > > Reviewed-by: Ján Tomko <jtomko@redhat.com> Thanks for the review! Right before pushing, I realized that VIR_ERR_INTERNAL_ERROR is probably not the best fit for this scenario. Are you okay with me squashing in the changes below? diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index 9c7ee6d75d..e04b85fee4 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -2101,7 +2101,7 @@ virDomainNetDefValidate(const virDomainNetDef *net) (net->type != VIR_DOMAIN_NET_TYPE_USER || (net->type == VIR_DOMAIN_NET_TYPE_USER && net->backend.type != VIR_DOMAIN_NET_BACKEND_PASST))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("The <portForward> element can only be used with <interface type='user'> and its 'passt' backend")); return -1; } diff --git a/tests/qemuxml2argvdata/net-user-slirp-portforward.x86_64-latest.err b/tests/qemuxml2argvdata/net-user-slirp-portforward.x86_64-latest.err index f296db1e8c..eaa934742e 100644 --- a/tests/qemuxml2argvdata/net-user-slirp-portforward.x86_64-latest.err +++ b/tests/qemuxml2argvdata/net-user-slirp-portforward.x86_64-latest.err @@ -1 +1 @@ -internal error: The <portForward> element can only be used with <interface type='user'> and its 'passt' backend +unsupported configuration: The <portForward> element can only be used with <interface type='user'> and its 'passt' backend -- Andrea Bolognani / Red Hat / Virtualization
On a Tuesday in 2023, Andrea Bolognani wrote: >On Tue, Apr 18, 2023 at 03:19:45PM +0200, Ján Tomko wrote: >> On a Tuesday in 2023, Andrea Bolognani wrote: >> > That's already the case in practice, but it's a better >> > experience for the user if we reject this configuration >> > outright instead of silently ignoring part of it. >> > >> > Signed-off-by: Andrea Bolognani <abologna@redhat.com> >> > --- >> > src/conf/domain_validate.c | 9 +++++++++ >> > ...t-user-slirp-portforward.x86_64-latest.err | 1 + >> > .../net-user-slirp-portforward.xml | 20 +++++++++++++++++++ >> > tests/qemuxml2argvtest.c | 1 + >> > 4 files changed, 31 insertions(+) >> > create mode 100644 tests/qemuxml2argvdata/net-user-slirp-portforward.x86_64-latest.err >> > create mode 100644 tests/qemuxml2argvdata/net-user-slirp-portforward.xml >> >> Reviewed-by: Ján Tomko <jtomko@redhat.com> > >Thanks for the review! > >Right before pushing, I realized that VIR_ERR_INTERNAL_ERROR is >probably not the best fit for this scenario. Are you okay with me >squashing in the changes below? > Yes. VIR_ERR_CONFIG_UNSUPPORTED = 67, /* unsupported configuration construct (Since: 0.7.3) */ We also use VIR_ERR_XML_ERROR in similar cases, but I'm not sure whether it's more fitting, given its description: VIR_ERR_XML_ERROR = 27, /* an XML description is not well formed or broken (Since: 0.1.1) */ Jano > >diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c >index 9c7ee6d75d..e04b85fee4 100644 >--- a/src/conf/domain_validate.c >+++ b/src/conf/domain_validate.c >@@ -2101,7 +2101,7 @@ virDomainNetDefValidate(const virDomainNetDef *net) > (net->type != VIR_DOMAIN_NET_TYPE_USER || > (net->type == VIR_DOMAIN_NET_TYPE_USER && > net->backend.type != VIR_DOMAIN_NET_BACKEND_PASST))) { >- virReportError(VIR_ERR_INTERNAL_ERROR, "%s", >+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > _("The <portForward> element can only be used >with <interface type='user'> and its 'passt' backend")); > return -1; > } >diff --git a/tests/qemuxml2argvdata/net-user-slirp-portforward.x86_64-latest.err >b/tests/qemuxml2argvdata/net-user-slirp-portforward.x86_64-latest.err >index f296db1e8c..eaa934742e 100644 >--- a/tests/qemuxml2argvdata/net-user-slirp-portforward.x86_64-latest.err >+++ b/tests/qemuxml2argvdata/net-user-slirp-portforward.x86_64-latest.err >@@ -1 +1 @@ >-internal error: The <portForward> element can only be used with ><interface type='user'> and its 'passt' backend >+unsupported configuration: The <portForward> element can only be used >with <interface type='user'> and its 'passt' backend >-- >Andrea Bolognani / Red Hat / Virtualization >
On 4/18/23 9:43 AM, Ján Tomko wrote: > On a Tuesday in 2023, Andrea Bolognani wrote: >> On Tue, Apr 18, 2023 at 03:19:45PM +0200, Ján Tomko wrote: >>> On a Tuesday in 2023, Andrea Bolognani wrote: >>> > That's already the case in practice, but it's a better >>> > experience for the user if we reject this configuration >>> > outright instead of silently ignoring part of it. >>> > >>> > Signed-off-by: Andrea Bolognani <abologna@redhat.com> >>> > --- >>> > src/conf/domain_validate.c | 9 +++++++++ >>> > ...t-user-slirp-portforward.x86_64-latest.err | 1 + >>> > .../net-user-slirp-portforward.xml | 20 +++++++++++++++++++ >>> > tests/qemuxml2argvtest.c | 1 + >>> > 4 files changed, 31 insertions(+) >>> > create mode 100644 >>> tests/qemuxml2argvdata/net-user-slirp-portforward.x86_64-latest.err >>> > create mode 100644 >>> tests/qemuxml2argvdata/net-user-slirp-portforward.xml >>> >>> Reviewed-by: Ján Tomko <jtomko@redhat.com> >> >> Thanks for the review! >> >> Right before pushing, I realized that VIR_ERR_INTERNAL_ERROR is >> probably not the best fit for this scenario. Are you okay with me >> squashing in the changes below? >> > > Yes. > > VIR_ERR_CONFIG_UNSUPPORTED = 67, /* unsupported configuration > construct (Since: 0.7.3) */ > > We also use VIR_ERR_XML_ERROR in similar cases, > but I'm not sure whether it's more fitting, given its description: > > VIR_ERR_XML_ERROR = 27, /* an XML description is not well > formed or broken (Since: > 0.1.1) */ While I think we probably have too many different error categories (even though often *none* of them is exactly right for a given circumstance), in this case CONFIG_UNSUPPORTED is better, since (IMO) XML_ERROR should only be used for something that is *never* valid under any cirsumstance (I guess that could also be interpreted in many ways, though) BTW, an alternate method of fixing this problem would have been to add <portForward> support to the slirp code (which is actually item 406 on my personal todo list :-P)
On Tue, Apr 18, 2023 at 03:43:41PM +0200, Ján Tomko wrote: > On a Tuesday in 2023, Andrea Bolognani wrote: > > On Tue, Apr 18, 2023 at 03:19:45PM +0200, Ján Tomko wrote: > > > On a Tuesday in 2023, Andrea Bolognani wrote: > > > > That's already the case in practice, but it's a better > > > > experience for the user if we reject this configuration > > > > outright instead of silently ignoring part of it. > > > > > > > > Signed-off-by: Andrea Bolognani <abologna@redhat.com> > > > > --- > > > > src/conf/domain_validate.c | 9 +++++++++ > > > > ...t-user-slirp-portforward.x86_64-latest.err | 1 + > > > > .../net-user-slirp-portforward.xml | 20 +++++++++++++++++++ > > > > tests/qemuxml2argvtest.c | 1 + > > > > 4 files changed, 31 insertions(+) > > > > create mode 100644 tests/qemuxml2argvdata/net-user-slirp-portforward.x86_64-latest.err > > > > create mode 100644 tests/qemuxml2argvdata/net-user-slirp-portforward.xml > > > > > > Reviewed-by: Ján Tomko <jtomko@redhat.com> > > > > Thanks for the review! > > > > Right before pushing, I realized that VIR_ERR_INTERNAL_ERROR is > > probably not the best fit for this scenario. Are you okay with me > > squashing in the changes below? > > Yes. > > VIR_ERR_CONFIG_UNSUPPORTED = 67, /* unsupported configuration > construct (Since: 0.7.3) */ > > We also use VIR_ERR_XML_ERROR in similar cases, > but I'm not sure whether it's more fitting, given its description: > > VIR_ERR_XML_ERROR = 27, /* an XML description is not well > formed or broken (Since: 0.1.1) */ Yeah, not quite clear-cut, but XML_ERROR seems more suitable for a situation where the XML is structurally incorrect (e.g. <disk> nested inside <interface> or something like that) as opposed to simply trying to enable a set of features that don't work well together. I'll stick with CONFIG_UNSUPPORTED. -- Andrea Bolognani / Red Hat / Virtualization
© 2016 - 2024 Red Hat, Inc.