Re: [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