[libvirt] [PATCH 0/2] qemu: Fix reporting completed migration stats on destination
https://bugzilla.redhat.com/show_bug.cgi?id=1584071 Jiri Denemark (2): qemu: Report error on unexpected job stats type qemu: Fix reporting completed migration stats on destination src/qemu/qemu_domain.c | 8 +++++--- src/qemu/qemu_migration.c | 1 + 2 files changed, 6 insertions(+), 3 deletions(-) -- 2.17.1
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
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
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
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
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.
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 :|
This has been broken since commit v4.0.0-165-g93412bb827 which added jobInfo->statsType enum to distinguish various statistics types. During migration the type will always be QEMU_DOMAIN_JOB_STATS_TYPE_MIGRATION, however the destination code consuming the statistics data from migration cookie failed to properly set the type. So even though everything was filled in, the type remained *_NONE and any attempt to fetch the statistics data of a completed migration on the destination host failed. https://bugzilla.redhat.com/show_bug.cgi?id=1584071 Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_migration.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 68663eac47..70616b568e 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -5160,6 +5160,7 @@ qemuMigrationDstFinish(virQEMUDriverPtr driver, if (jobInfo) { VIR_STEAL_PTR(priv->job.completed, jobInfo); priv->job.completed->status = QEMU_DOMAIN_JOB_STATUS_COMPLETED; + priv->job.completed->statsType = QEMU_DOMAIN_JOB_STATS_TYPE_MIGRATION; } if (qemuMigrationBakeCookie(mig, driver, vm, -- 2.17.1
On Fri, Jun 01, 2018 at 10:46:03 +0200, Jiri Denemark wrote:
This has been broken since commit v4.0.0-165-g93412bb827 which added jobInfo->statsType enum to distinguish various statistics types. During migration the type will always be QEMU_DOMAIN_JOB_STATS_TYPE_MIGRATION, however the destination code consuming the statistics data from migration cookie failed to properly set the type. So even though everything was filled in, the type remained *_NONE and any attempt to fetch the statistics data of a completed migration on the destination host failed.
https://bugzilla.redhat.com/show_bug.cgi?id=1584071
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_migration.c | 1 + 1 file changed, 1 insertion(+)
ACK && freeze-worthy
participants (3)
-
Daniel P. Berrangé -
Jiri Denemark -
Peter Krempa