On Wed, Sep 19, 2018 at 08:11:34AM +0800, [email protected] wrote: > > On Mon, Sep 17, 2018 at 06:53:13PM +0800, [email protected] 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 <[email protected]> > > > > > Reviewed-by: Xi Xu <[email protected]> > > > > > --- > > > > > 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 -- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
