[libvirt] [PATCH 0/2] Restore error messages for {iothread|vcpu}sched id out of range.
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
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@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
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@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.
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@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
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@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.
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@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
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@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.
participants (2)
-
John Ferlan -
Peter Krempa