1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/2] qemu: Fix reporting completed migration stats on destination
  4. View patch

[libvirt] [PATCH 1/2] qemu: Report error on unexpected job stats type

Jiri Denemark posted 2 patches 6 years, 11 months ago
[libvirt] [PATCH 1/2] qemu: Report error on unexpected job stats type
Posted by Jiri Denemark 6 years, 11 months ago 
If we ever fail to properly set jobinfo->statsType,
qemuDomainJobInfoToParams would return -1 without setting an error.

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
---
 src/qemu/qemu_domain.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2c51e4c0d8..360379b26c 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -717,10 +717,12 @@ qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo,
         return qemuDomainDumpJobInfoToParams(jobInfo, type, params, nparams);
 
     case QEMU_DOMAIN_JOB_STATS_TYPE_NONE:
-        break;
+    default:
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("unexpected type of job stats: %d"),
+                       jobInfo->statsType);
+        return -1;
     }
-
-    return -1;
 }
 
 
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: Report error on unexpected job stats type
Posted by Peter Krempa 6 years, 11 months ago 
On Fri, Jun 01, 2018 at 10:46:02 +0200, Jiri Denemark wrote:
> If we ever fail to properly set jobinfo->statsType,
> qemuDomainJobInfoToParams would return -1 without setting an error.
> 
> Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> ---
>  src/qemu/qemu_domain.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 2c51e4c0d8..360379b26c 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -717,10 +717,12 @@ qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo,
>          return qemuDomainDumpJobInfoToParams(jobInfo, type, params, nparams);
>  
>      case QEMU_DOMAIN_JOB_STATS_TYPE_NONE:
> -        break;
> +    default:
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("unexpected type of job stats: %d"),
> +                       jobInfo->statsType);

virReportEnumRangeError?

> +        return -1;

ACK, ... 


>      }
> -
> -    return -1;

.. but only push it during the freeze with the above part dropped. I'm
not going to second-guess which compiler decides that the function will
be missing a return statement.


>  }
>  
>  
> -- 
> 2.17.1
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: Report error on unexpected job stats type
Posted by Jiri Denemark 6 years, 11 months ago 
On Fri, Jun 01, 2018 at 17:31:02 +0200, Peter Krempa wrote:
> On Fri, Jun 01, 2018 at 10:46:02 +0200, Jiri Denemark wrote:
> > If we ever fail to properly set jobinfo->statsType,
> > qemuDomainJobInfoToParams would return -1 without setting an error.
> > 
> > Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> > ---
> >  src/qemu/qemu_domain.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index 2c51e4c0d8..360379b26c 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -717,10 +717,12 @@ qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo,
> >          return qemuDomainDumpJobInfoToParams(jobInfo, type, params, nparams);
> >  
> >      case QEMU_DOMAIN_JOB_STATS_TYPE_NONE:
> > -        break;
> > +    default:
> > +        virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                       _("unexpected type of job stats: %d"),
> > +                       jobInfo->statsType);
> 
> virReportEnumRangeError?

Right, in that case, I need to split default and NONE branch, and report
different errors for each of them.

> 
> > +        return -1;
> 
> ACK, ... 
> 
> 
> >      }
> > -
> > -    return -1;
> 
> .. but only push it during the freeze with the above part dropped. I'm
> not going to second-guess which compiler decides that the function will
> be missing a return statement.

OK, I'll wait after the release.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: Report error on unexpected job stats type
Posted by Jiri Denemark 6 years, 11 months ago 
On Fri, Jun 01, 2018 at 17:55:12 +0200, Jiri Denemark wrote:
> On Fri, Jun 01, 2018 at 17:31:02 +0200, Peter Krempa wrote:
> > On Fri, Jun 01, 2018 at 10:46:02 +0200, Jiri Denemark wrote:
> > > If we ever fail to properly set jobinfo->statsType,
> > > qemuDomainJobInfoToParams would return -1 without setting an error.
> > > 
> > > Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> > > ---
> > >  src/qemu/qemu_domain.c | 8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > > index 2c51e4c0d8..360379b26c 100644
> > > --- a/src/qemu/qemu_domain.c
> > > +++ b/src/qemu/qemu_domain.c
> > > @@ -717,10 +717,12 @@ qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo,
> > >          return qemuDomainDumpJobInfoToParams(jobInfo, type, params, nparams);
> > >  
> > >      case QEMU_DOMAIN_JOB_STATS_TYPE_NONE:
> > > -        break;
> > > +    default:
> > > +        virReportError(VIR_ERR_INTERNAL_ERROR,
> > > +                       _("unexpected type of job stats: %d"),
> > > +                       jobInfo->statsType);
> > 
> > virReportEnumRangeError?
> 
> Right, in that case, I need to split default and NONE branch, and report
> different errors for each of them.

Well, I was wrong. Calling virReportEnumRangeError for both cases is OK.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: Report error on unexpected job stats type
Posted by Peter Krempa 6 years, 11 months ago 
On Fri, Jun 01, 2018 at 18:04:29 +0200, Jiri Denemark wrote:
> On Fri, Jun 01, 2018 at 17:55:12 +0200, Jiri Denemark wrote:
> > On Fri, Jun 01, 2018 at 17:31:02 +0200, Peter Krempa wrote:
> > > On Fri, Jun 01, 2018 at 10:46:02 +0200, Jiri Denemark wrote:
> > > > If we ever fail to properly set jobinfo->statsType,
> > > > qemuDomainJobInfoToParams would return -1 without setting an error.
> > > > 
> > > > Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> > > > ---
> > > >  src/qemu/qemu_domain.c | 8 +++++---
> > > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > > > index 2c51e4c0d8..360379b26c 100644
> > > > --- a/src/qemu/qemu_domain.c
> > > > +++ b/src/qemu/qemu_domain.c
> > > > @@ -717,10 +717,12 @@ qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo,
> > > >          return qemuDomainDumpJobInfoToParams(jobInfo, type, params, nparams);
> > > >  
> > > >      case QEMU_DOMAIN_JOB_STATS_TYPE_NONE:
> > > > -        break;
> > > > +    default:
> > > > +        virReportError(VIR_ERR_INTERNAL_ERROR,
> > > > +                       _("unexpected type of job stats: %d"),
> > > > +                       jobInfo->statsType);
> > > 
> > > virReportEnumRangeError?
> > 
> > Right, in that case, I need to split default and NONE branch, and report
> > different errors for each of them.
> 
> Well, I was wrong. Calling virReportEnumRangeError for both cases is OK.

Both are programming errors so I think it's fine.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: Report error on unexpected job stats type
Posted by Daniel P. Berrangé 6 years, 11 months ago 
On Fri, Jun 01, 2018 at 06:04:29PM +0200, Jiri Denemark wrote:
> On Fri, Jun 01, 2018 at 17:55:12 +0200, Jiri Denemark wrote:
> > On Fri, Jun 01, 2018 at 17:31:02 +0200, Peter Krempa wrote:
> > > On Fri, Jun 01, 2018 at 10:46:02 +0200, Jiri Denemark wrote:
> > > > If we ever fail to properly set jobinfo->statsType,
> > > > qemuDomainJobInfoToParams would return -1 without setting an error.
> > > > 
> > > > Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> > > > ---
> > > >  src/qemu/qemu_domain.c | 8 +++++---
> > > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > > > index 2c51e4c0d8..360379b26c 100644
> > > > --- a/src/qemu/qemu_domain.c
> > > > +++ b/src/qemu/qemu_domain.c
> > > > @@ -717,10 +717,12 @@ qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo,
> > > >          return qemuDomainDumpJobInfoToParams(jobInfo, type, params, nparams);
> > > >  
> > > >      case QEMU_DOMAIN_JOB_STATS_TYPE_NONE:
> > > > -        break;
> > > > +    default:
> > > > +        virReportError(VIR_ERR_INTERNAL_ERROR,
> > > > +                       _("unexpected type of job stats: %d"),
> > > > +                       jobInfo->statsType);
> > > 
> > > virReportEnumRangeError?
> > 
> > Right, in that case, I need to split default and NONE branch, and report
> > different errors for each of them.
> 
> Well, I was wrong. Calling virReportEnumRangeError for both cases is OK.

Actally it was really only intended for the case where the enum value
is out of range. ie does not correspond to a known enum contant.

If it is a valid enum value, which is just not applicable in this usage
scenario, something else should be used, which reports the string version
of the error instead of a integer value.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.