[libvirt] [PATCH 1/2] conf: Provide error on undefined iothreadsched entry

[Peter Krempa]

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 at 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.