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@zte.com.cn>
Reviewed-by: Xi Xu <xu.xi8@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) {
                 if (virBitmapParse(tmp, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0)
                     goto cleanup;
 
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@zte.com.cn>
> Reviewed-by: Xi Xu <xu.xi8@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.

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.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

> 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@zte.com.cn>
> > Reviewed-by: Xi Xu <xu.xi8@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.


---
Best wishes
Yi Wang--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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


---
Best wishes
Yi Wang--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, Sep 19, 2018 at 08:11:34AM +0800, wang.yi59@zte.com.cn wrote:
> > On Mon, Sep 17, 2018 at 06:53:13PM +0800, wang.yi59@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@zte.com.cn>
> > > > > Reviewed-by: Xi Xu <xu.xi8@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

> On Wed, Sep 19, 2018 at 08:11:34AM +0800, wang.yi59@zte.com.cn wrote:
> > > On Mon, Sep 17, 2018 at 06:53:13PM +0800, wang.yi59@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@zte.com.cn>
> > > > > > Reviewed-by: Xi Xu <xu.xi8@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:

  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--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, Sep 19, 2018 at 03:51:55PM +0800, wang.yi59@zte.com.cn wrote:
> > On Wed, Sep 19, 2018 at 08:11:34AM +0800, wang.yi59@zte.com.cn wrote:
> > > > On Mon, Sep 17, 2018 at 06:53:13PM +0800, wang.yi59@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@zte.com.cn>
> > > > > > > Reviewed-by: Xi Xu <xu.xi8@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list