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@xxxxxxxxxx>
>> ---
>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list