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

[Erik Skultety]

On Wed, Sep 19, 2018 at 08:11:34AM +0800, wang.yi59 at zte.com.cn wrote:
> > 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.
>
> I have checked the commit b71946af5c7 before sending this patch and came up with two solutions:
>
> - make cpuset="" passed the check, like this patch does
> - add specified error report, rather than complaining about parsing bitmap
>
> The first method can keep consistence with the older version, so I chose this one.

Well, in this case being consistent with something which wasn't imho entirely
correct is not the way to go, the consistency should have been with all the
other cases where the parsing simply fails because it can't parse an empty
string into anything meaningful. As I said, I don't see a point in using
cpuset="" in real world. It would be different if the kind of change commit
b71946af5c7 introduced made some VMs disappear, but that's not the case here,
you just happen to use an old XML to create a transient VM, so I'd suggest
updating the XML you use instead.

Erik