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