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

Wed Sep 19 08:15:05 UTC 2018

On Wed, Sep 19, 2018 at 03:51:55PM +0800, wang.yi59 at zte.com.cn wrote:
> > 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.
>
> Ok, I agree with what you said. However, the error report now is not user-friendly after
> all, so should I send a new patch to improve the code, such like:

Well, I spent some time yesterday thinking about that, especially because I
didn't want to duplicate the error message, that I particularly didn't like. If
you look at what we report when virBitmapIsAllClear fails, that's what we
should report here too, however, that only happens after the bitmap is parsed
which would have already reported that crappy error in our case. That's why I
mentioned commit 106a2ddaa7b which changed it to the today's form and why I
CC'd Peter (who's on holiday this week) to shed a bit more light on why we went
for a more generic error message that doesn't help anyone from one that was
very specific, IOW user_experience--. Because of that, I don't agree with the
snippet below, because it takes the easy way and handles only one of several
occurrences where we would probably hit the same issue after failing to parse a
bitmap. Personally, I'd like to revert that commit and report a more useful
error, but that would be more work than just what you're proposing below.

Erik

>
>   if (tmp) {
>       if (strlen(tmp) == 0) {
>           virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>               _("Invalid value of 'cpuset': %s"), tmp);
>           goto cleanup;
>       }
>   }
>
> Thank you, Erik.
>
> ---
> Best wishes
> Yi Wang