[libvirt] [PATCH] qemu: Report error on unexpected job stats type
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 | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index fee44812c1..9afe705929 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -730,6 +730,12 @@ qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo, return qemuDomainDumpJobInfoToParams(jobInfo, type, params, nparams); case QEMU_DOMAIN_JOB_STATS_TYPE_NONE: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid job statistics type")); + break; + + default: + virReportEnumRangeError(qemuDomainJobStatsType, jobInfo->statsType); break; } -- 2.18.0
On 06/28/2018 03:29 PM, Jiri Denemark wrote:
If we ever fail to properly set jobinfo->statsType, qemuDomainJobInfoToParams would return -1 without setting an error.
Have you actually seen such error? Looks to me like both callers handle this case properly.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_domain.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index fee44812c1..9afe705929 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -730,6 +730,12 @@ qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo, return qemuDomainDumpJobInfoToParams(jobInfo, type, params, nparams);
case QEMU_DOMAIN_JOB_STATS_TYPE_NONE: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid job statistics type")); + break; + + default: + virReportEnumRangeError(qemuDomainJobStatsType, jobInfo->statsType); break; }
Can't we join TYPE_NONE and default together and just report enum range error? What is the point in differentiating the two? Michal
On Thu, Jun 28, 2018 at 15:45:12 +0200, Michal Privoznik wrote:
On 06/28/2018 03:29 PM, Jiri Denemark wrote:
If we ever fail to properly set jobinfo->statsType, qemuDomainJobInfoToParams would return -1 without setting an error.
Have you actually seen such error? Looks to me like both callers handle this case properly.
Yes, when the code for destination migration filled in migration stats, but failed to set the stats type. As a result no stats were reported and the programmer's error was silently ignored.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_domain.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index fee44812c1..9afe705929 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -730,6 +730,12 @@ qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo, return qemuDomainDumpJobInfoToParams(jobInfo, type, params, nparams);
case QEMU_DOMAIN_JOB_STATS_TYPE_NONE: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid job statistics type")); + break; + + default: + virReportEnumRangeError(qemuDomainJobStatsType, jobInfo->statsType); break; }
Can't we join TYPE_NONE and default together and just report enum range error? What is the point in differentiating the two?
I did it in the first version of this patch (a long time ago), but Daniel explained that virReportEnumRangeError is supposed to be used only for unknown enum values. A different error should be used for known values which do not make sense in a specific place in the code. Jirka
On 06/28/2018 04:30 PM, Jiri Denemark wrote:
On Thu, Jun 28, 2018 at 15:45:12 +0200, Michal Privoznik wrote:
On 06/28/2018 03:29 PM, Jiri Denemark wrote:
If we ever fail to properly set jobinfo->statsType, qemuDomainJobInfoToParams would return -1 without setting an error.
Have you actually seen such error? Looks to me like both callers handle this case properly.
Yes, when the code for destination migration filled in migration stats, but failed to set the stats type. As a result no stats were reported and the programmer's error was silently ignored.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_domain.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index fee44812c1..9afe705929 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -730,6 +730,12 @@ qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo, return qemuDomainDumpJobInfoToParams(jobInfo, type, params, nparams);
case QEMU_DOMAIN_JOB_STATS_TYPE_NONE: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid job statistics type")); + break; + + default: + virReportEnumRangeError(qemuDomainJobStatsType, jobInfo->statsType); break; }
Can't we join TYPE_NONE and default together and just report enum range error? What is the point in differentiating the two?
I did it in the first version of this patch (a long time ago), but Daniel explained that virReportEnumRangeError is supposed to be used only for unknown enum values. A different error should be used for known values which do not make sense in a specific place in the code.
Ah, okay. ACK then. Michal
participants (2)
-
Jiri Denemark -
Michal Privoznik