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) {

> 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.

> 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.

> On Wed, Sep 19, 2018 at 08:11:34AM +0800, wang.yi59(a)zte.com.cn wrote: > > > 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. > > 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:

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