9:55 a.m.
These patches will restore error messages indicating the vcpu or iothread
range(s) for vcpusched or iothreadsched are invalid rather than just quietly
ignoring. If someone for some unknown reason had hand edited their domain
prior to starting libvirt, they'll at least get a message indicating that
"some" domain has an invaliv vcpu/iothread value.
NB: Removal of error message occurred in the following releases:
iothreadsched: Removed in v1.3.2
vcpusched: Removed in V2.1
John Ferlan (2):
conf: Provide error on undefined iothreadsched entry
conf: Provide error on undefined vcpusched entry
src/conf/domain_conf.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
--
2.7.4
9:55 a.m.
New subject: [libvirt] [PATCH 1/2] conf: Provide error on undefined iothreadsched entry
When commit id '6dfb4507' refactored where the iothreadsched data was
stored, the error message for when the virDomainIOThreadIDFind failed
to find an iothreadid ("iothreadsched attribute 'iothreads' uses
undefined iothread ids") was lost. This led to the possibility that
someone would try to use it, but receive the generic message "An error
occurred, but the cause is unknown".
This patch adds the error message back so that someone will know that
they have an invalid configuration.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_conf.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 82876f3..9037304 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15490,8 +15490,12 @@ virDomainDefGetIOThreadSched(virDomainDefPtr def,
{
virDomainIOThreadIDDefPtr iothrinfo;
- if (!(iothrinfo = virDomainIOThreadIDFind(def, iothread)))
+ if (!(iothrinfo = virDomainIOThreadIDFind(def, iothread))) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Cannot find 'iothread' : %u"),
+ iothread);
return NULL;
+ }
return &iothrinfo->sched;
}
--
2.7.4
10:07 a.m.
New subject: [libvirt] [PATCH 1/2] conf: Provide error on undefined iothreadsched entry
On Mon, Aug 15, 2016 at 10:55:33 -0400, John Ferlan wrote:
When commit id '6dfb4507' refactored where the iothreadsched
data was
stored, the error message for when the virDomainIOThreadIDFind failed
to find an iothreadid ("iothreadsched attribute 'iothreads' uses
undefined iothread ids") was lost. This led to the possibility that
someone would try to use it, but receive the generic message "An error
occurred, but the cause is unknown".
This patch adds the error message back so that someone will know that
they have an invalid configuration.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_conf.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
Same as for 2/2. Code paths that care should report the error in place
since we would report duplicate errors in cases where it can be ignored
or is overwritten.
11:03 a.m.
New subject: [libvirt] [PATCH 1/2] conf: Provide error on undefined iothreadsched entry
On 08/15/2016 11:07 AM, Peter Krempa wrote:
On Mon, Aug 15, 2016 at 10:55:33 -0400, John Ferlan wrote:
> When commit id '6dfb4507' refactored where the iothreadsched data was
> stored, the error message for when the virDomainIOThreadIDFind failed
> to find an iothreadid ("iothreadsched attribute 'iothreads' uses
> undefined iothread ids") was lost. This led to the possibility that
> someone would try to use it, but receive the generic message "An error
> occurred, but the cause is unknown".
>
> This patch adds the error message back so that someone will know that
> they have an invalid configuration.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/conf/domain_conf.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
Same as for 2/2. Code paths that care should report the error in place
since we would report duplicate errors in cases where it can be ignored
or is overwritten.
Unlike 2/2 the error here is being checked where appropriate unless you
are advocating a generic error message in virDomainDefParseXML after
calling virDomainIOThreadSchedParse and returning a < 0 value, but no
error message set, then create error message (which doesn't seem right)
e.g.:
for (i = 0; i < n; i++) {
if (virDomainIOThreadSchedParse(nodes[i], def) < 0) {
if (!virGetLastError()) {
virReportError(..., "some error occurred"...);
}
goto error;
}
For virDomainDefGetIOThreadSched the only callers that care are Parse
and Format. NB: All callers of virDomainIOThreadIDFind care to check if
the ID was valid or not and that's where I believe the check should be.
As for patch 2/2, fair enough I can move the error message to
virDomainDefGetVcpuSched... Of course that if for some unknown reason in
the future virDomainDefGetVcpu adds a new reason for NULL return the
assumption could be lost or the error message overwritten. Does that
then address your concern?
John
10:05 a.m.
New subject: [libvirt] [PATCH 1/2] conf: Provide error on undefined iothreadsched entry
On Mon, Aug 15, 2016 at 12:03:53 -0400, John Ferlan wrote:
On 08/15/2016 11:07 AM, Peter Krempa wrote:
> On Mon, Aug 15, 2016 at 10:55:33 -0400, John Ferlan wrote:
>> When commit id '6dfb4507' refactored where the iothreadsched data was
>> stored, the error message for when the virDomainIOThreadIDFind failed
>> to find an iothreadid ("iothreadsched attribute 'iothreads' uses
>> undefined iothread ids") was lost. This led to the possibility that
>> someone would try to use it, but receive the generic message "An error
>> occurred, but the cause is unknown".
>>
>> This patch adds the error message back so that someone will know that
>> they have an invalid configuration.
>>
>> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
>> ---
>> src/conf/domain_conf.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> Same as for 2/2. Code paths that care should report the error in place
> since we would report duplicate errors in cases where it can be ignored
> or is overwritten.
>
Unlike 2/2 the error here is being checked where appropriate unless you
are advocating a generic error message in virDomainDefParseXML after
calling virDomainIOThreadSchedParse and returning a < 0 value, but no
error message set, then create error message (which doesn't seem right)
e.g.:
for (i = 0; i < n; i++) {
if (virDomainIOThreadSchedParse(nodes[i], def) < 0) {
if (!virGetLastError()) {
virReportError(..., "some error occurred"...);
}
goto error;
}
Uh, yes, this indeed looks wrong.
For virDomainDefGetIOThreadSched the only callers that care are Parse
and Format. NB: All callers of virDomainIOThreadIDFind care to check if
the ID was valid or not and that's where I believe the check should be.
ACK to this patch then.
As for patch 2/2, fair enough I can move the error message to
virDomainDefGetVcpuSched... Of course that if for some unknown reason ina
virDomainFormatSchedDef won't call it on invalid vcpu thus it should be
okay to do so.
the future virDomainDefGetVcpu adds a new reason for NULL return the
assumption could be lost or the error message overwritten. Does that
then address your concern?
Yes that should be okay.
9:55 a.m.
New subject: [libvirt] [PATCH 2/2] conf: Provide error on undefined vcpusched entry
Commit id '9cc931f0b' removed the error message; however, it was reachable
via the virsh edit of a domain and attempting to add a vcpusched element
for "undefined" vcpu values. Without the error, the generic message
"An error occurred, but the cause is unknown" is displayed.
This is similar to bz 1366484 for the iothreadsched element.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_conf.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9037304..caebe99 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1467,8 +1467,12 @@ virDomainVcpuDefPtr
virDomainDefGetVcpu(virDomainDefPtr def,
unsigned int vcpu)
{
- if (vcpu >= def->maxvcpus)
+ if (vcpu >= def->maxvcpus) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("vCPU '%u' is not present in domain
definition"),
+ vcpu);
return NULL;
+ }
return def->vcpus[vcpu];
}
--
2.7.4
10:06 a.m.
New subject: [libvirt] [PATCH 2/2] conf: Provide error on undefined vcpusched entry
On Mon, Aug 15, 2016 at 10:55:34 -0400, John Ferlan wrote:
Commit id '9cc931f0b' removed the error message; however, it
was reachable
via the virsh edit of a domain and attempting to add a vcpusched element
for "undefined" vcpu values. Without the error, the generic message
"An error occurred, but the cause is unknown" is displayed.
This is similar to bz 1366484 for the iothreadsched element.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_conf.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9037304..caebe99 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1467,8 +1467,12 @@ virDomainVcpuDefPtr
virDomainDefGetVcpu(virDomainDefPtr def,
unsigned int vcpu)
{
- if (vcpu >= def->maxvcpus)
+ if (vcpu >= def->maxvcpus) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("vCPU '%u' is not present in domain
definition"),
+ vcpu);
return NULL;
+ }
This error would be overwritten by the many callers that care if it's
not found and report bogus error in cases where failure is okay.
The specific code path that cares should report the error.
NACK.
3020
days inactive
3021
days old
6 comments
2 participants
participants (2)
-
John Ferlan -
Peter Krempa