[libvirt] [PATCH] conf: fix starting a domain with cpuset=""

[Erik Skultety]

On Mon, Sep 17, 2018 at 06:53:13PM +0800, wang.yi59 at zte.com.cn wrote:
> > On Sat, Sep 15, 2018 at 04:29:24PM +0800, Yi Wang wrote:
> > > Domain fails to start when its config xml including:
> > >   <vcpu cpuset="" current="8">64</vcpu>
> > >
> > >   # virsh create vm.xml
> > >   error: Failed to create domain from vm.xml
> > >   error: invalid argument: Failed to parse bitmap ''
> > >
> > > This patch fixes this.
> > >
> > > Signed-off-by: Yi Wang <wang.yi59 at zte.com.cn>
> > > Reviewed-by: Xi Xu <xu.xi8 at zte.com.cn>
> > > ---
> > >  src/conf/domain_conf.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > > index 8619962..bacafb2 100644
> > > --- a/src/conf/domain_conf.c
> > > +++ b/src/conf/domain_conf.c
> > > @@ -18553,7 +18553,7 @@ virDomainVcpuParse(virDomainDefPtr def,
> > >
> > >          if (def->placement_mode != VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) {
> > >              tmp = virXMLPropString(vcpuNode, "cpuset");
> > > -            if (tmp) {
> > > +            if (tmp && strlen(tmp) != 0) {
> >
> > .... '&& *tmp' would suffice.
>
> Ok.
>
> >
> > The patch is correct, but I believe there is a number of spots in the massive
> > domain_conf.c file where a similar fix would be needed, it might be worth
> > checking all the spots where no conversion like string-to-int string-to-enum or
> > any other additional parsing like address parsing is performed, those might be
> > good candidates.
> >
> > I understand the file is massive, so let me know how that goes.
>
> In most similar places we have already checked by the return values of function
> like string-to-int string-to-enum, and if failed, libvirt will give explicit report.
>
> To create a domain with iothread="" of disk, for instance:
>
>   # virsh create vm.xml
>   error: Failed to create domain from vm.xml
>   error: XML error: Invalid iothread attribute in disk driver element:
>
> But the cpuset="" config error report may make people confuesd, and in the older
> version like 3.1.0 that's ok to start a domain with this config. So I think this patch
> is enough.

So, I thought about this quite a bit. The fact that 3.1.0 was okay with that
was because commit b71946af5c7 changed the helper we use during parsing. The
original one considered NULL and foo[0] == '\0' to be the same and always
returned NULL. However, now that I looked at the code more closely, I don't
think we want this patch, in fact, an empty cpuset is an invalid attribute,
what's the point of having an empty cpuset vs not having it at all, if you don't
want static pinning then don't specify one, there's no practical use case in
that. That being said, I also find the error very generic and vague, so if
anyone wants to actually figure out what the part causing problems in the XML
parsing is, they have no chance, we should improve that. Paradoxically, we had
that back in 2013 until commit 106a2ddaa7b changed it with absolutely no
rationale or a corresponding BZ on why decreasing user experience was a good
idea, I know, rather a rare use case what you have here, but still, why is okay
to parse numbers from string with no error, thus giving a clear error (like your
iothread example), but not okay for bitmaps, CC'ing Peter to give more
information about that change.

Erik