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

> On Mon, Sep 17, 2018 at 06:53:13PM +0800, wang.yi59(a)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(a)zte.com.cn&gt; > > > > Reviewed-by: Xi Xu <xu.xi8(a)zte.com.cn&gt; > > > > --- > > > > 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.