src/ch/ch_driver.c | 2 ++ src/qemu/qemu_driver.c | 7 ++++++- src/util/virprocess.c | 8 ++------ 3 files changed, 10 insertions(+), 7 deletions(-)
This reverts commit 938382b60ae5bd1f83b5cb09e1ce68b9a88f679a.
Turns out, the commit did more harm than good. It changed
semantics on some public APIs. For instance, while
qemuDomainGetInfo() previously did not returned an error it does
now. While the calls to virProcessGetStatInfo() is guarded with
virDomainObjIsActive() it doesn't necessarily mean that QEMU's
PID is still alive. QEMU might be gone but we just haven't
realized it (e.g. because the eof handler thread is waiting for a
job).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2041610
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
src/ch/ch_driver.c | 2 ++
src/qemu/qemu_driver.c | 7 ++++++-
src/util/virprocess.c | 8 ++------
3 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
index 3cbc668489..53e0872207 100644
--- a/src/ch/ch_driver.c
+++ b/src/ch/ch_driver.c
@@ -1073,6 +1073,8 @@ chDomainHelperGetVcpus(virDomainObj *vm,
if (virProcessGetStatInfo(&vcpuinfo->cpuTime,
&vcpuinfo->cpu, NULL,
vm->pid, vcpupid) < 0) {
+ virReportSystemError(errno, "%s",
+ _("cannot get vCPU placement & pCPU time"));
return -1;
}
}
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e150b86cef..373cd62536 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1357,6 +1357,8 @@ qemuDomainHelperGetVcpus(virDomainObj *vm,
if (virProcessGetStatInfo(&vcpuinfo->cpuTime,
&vcpuinfo->cpu, NULL,
vm->pid, vcpupid) < 0) {
+ virReportSystemError(errno, "%s",
+ _("cannot get vCPU placement & pCPU time"));
return -1;
}
}
@@ -2517,6 +2519,8 @@ qemuDomainGetInfo(virDomainPtr dom,
if (virDomainObjIsActive(vm)) {
if (virProcessGetStatInfo(&(info->cpuTime), NULL, NULL,
vm->pid, 0) < 0) {
+ virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+ _("cannot read cputime for domain"));
goto cleanup;
}
}
@@ -10524,7 +10528,8 @@ qemuDomainMemoryStatsInternal(virQEMUDriver *driver,
}
if (virProcessGetStatInfo(NULL, NULL, &rss, vm->pid, 0) < 0) {
- virResetLastError();
+ virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+ _("cannot get RSS for domain"));
} else {
stats[ret].tag = VIR_DOMAIN_MEMORY_STAT_RSS;
stats[ret].val = rss;
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 85d8c8e747..b559a4257e 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -1784,10 +1784,7 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10, &systime) < 0 ||
virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, &rss) < 0 ||
virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, &cpu) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot parse process status data for pid '%d/%d'"),
- (int) pid, (int) tid);
- return -1;
+ VIR_WARN("cannot parse process status data");
}
/* We got jiffies
@@ -1884,8 +1881,7 @@ virProcessGetStatInfo(unsigned long long *cpuTime G_GNUC_UNUSED,
pid_t pid G_GNUC_UNUSED,
pid_t tid G_GNUC_UNUSED)
{
- virReportSystemError(ENOSYS, "%s",
- _("Process statistics data is not supported on this platform"));
+ errno = ENOSYS;
return -1;
}
--
2.34.1
On Tue, Jan 18, 2022 at 5:15 PM Michal Privoznik <mprivozn@redhat.com>
wrote:
> This reverts commit 938382b60ae5bd1f83b5cb09e1ce68b9a88f679a.
>
> Turns out, the commit did more harm than good. It changed
> semantics on some public APIs. For instance, while
> qemuDomainGetInfo() previously did not returned an error it does
> now. While the calls to virProcessGetStatInfo() is guarded with
> virDomainObjIsActive() it doesn't necessarily mean that QEMU's
> PID is still alive. QEMU might be gone but we just haven't
> realized it (e.g. because the eof handler thread is waiting for a
> job).
Doh!
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2041610
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> src/ch/ch_driver.c | 2 ++
> src/qemu/qemu_driver.c | 7 ++++++-
> src/util/virprocess.c | 8 ++------
> 3 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
> index 3cbc668489..53e0872207 100644
> --- a/src/ch/ch_driver.c
> +++ b/src/ch/ch_driver.c
> @@ -1073,6 +1073,8 @@ chDomainHelperGetVcpus(virDomainObj *vm,
> if (virProcessGetStatInfo(&vcpuinfo->cpuTime,
> &vcpuinfo->cpu, NULL,
> vm->pid, vcpupid) < 0) {
> + virReportSystemError(errno, "%s",
> + _("cannot get vCPU placement & pCPU
> time"));
> return -1;
> }
> }
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index e150b86cef..373cd62536 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1357,6 +1357,8 @@ qemuDomainHelperGetVcpus(virDomainObj *vm,
> if (virProcessGetStatInfo(&vcpuinfo->cpuTime,
> &vcpuinfo->cpu, NULL,
> vm->pid, vcpupid) < 0) {
> + virReportSystemError(errno, "%s",
> + _("cannot get vCPU placement & pCPU
> time"));
> return -1;
> }
> }
> @@ -2517,6 +2519,8 @@ qemuDomainGetInfo(virDomainPtr dom,
> if (virDomainObjIsActive(vm)) {
> if (virProcessGetStatInfo(&(info->cpuTime), NULL, NULL,
> vm->pid, 0) < 0) {
> + virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> + _("cannot read cputime for domain"));
> goto cleanup;
> }
> }
> @@ -10524,7 +10528,8 @@ qemuDomainMemoryStatsInternal(virQEMUDriver
> *driver,
> }
>
> if (virProcessGetStatInfo(NULL, NULL, &rss, vm->pid, 0) < 0) {
> - virResetLastError();
> + virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> + _("cannot get RSS for domain"));
> } else {
> stats[ret].tag = VIR_DOMAIN_MEMORY_STAT_RSS;
> stats[ret].val = rss;
> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> index 85d8c8e747..b559a4257e 100644
> --- a/src/util/virprocess.c
> +++ b/src/util/virprocess.c
> @@ -1784,10 +1784,7 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
> virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10,
> &systime) < 0 ||
> virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, &rss) <
> 0 ||
> virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10,
> &cpu) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("cannot parse process status data for pid
> '%d/%d'"),
> - (int) pid, (int) tid);
> - return -1;
> + VIR_WARN("cannot parse process status data");
> }
>
> /* We got jiffies
> @@ -1884,8 +1881,7 @@ virProcessGetStatInfo(unsigned long long *cpuTime
> G_GNUC_UNUSED,
> pid_t pid G_GNUC_UNUSED,
> pid_t tid G_GNUC_UNUSED)
> {
> - virReportSystemError(ENOSYS, "%s",
> - _("Process statistics data is not supported on
> this platform"));
> + errno = ENOSYS;
> return -1;
> }
>
> --
> 2.34.1
>
>
On Tue, Jan 18, 2022 at 12:44:51PM +0100, Michal Privoznik wrote:
> +++ b/src/util/virprocess.c
> @@ -1784,10 +1784,7 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
> virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10, &systime) < 0 ||
> virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, &rss) < 0 ||
> virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, &cpu) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("cannot parse process status data for pid '%d/%d'"),
> - (int) pid, (int) tid);
> - return -1;
> + VIR_WARN("cannot parse process status data");
Shame to lose the improved error/warning message. Perhaps it could be
reintroduced separately.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
On Thu, 20 Jan 2022, Andrea Bolognani wrote:
> On Tue, Jan 18, 2022 at 12:44:51PM +0100, Michal Privoznik wrote:
> > +++ b/src/util/virprocess.c
> > @@ -1784,10 +1784,7 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
> > virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10, &systime) < 0 ||
> > virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, &rss) < 0 ||
> > virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, &cpu) < 0) {
> > - virReportError(VIR_ERR_INTERNAL_ERROR,
> > - _("cannot parse process status data for pid '%d/%d'"),
> > - (int) pid, (int) tid);
> > - return -1;
> > + VIR_WARN("cannot parse process status data");
>
> Shame to lose the improved error/warning message. Perhaps it could be
> reintroduced separately.
>
Functions in general are not coded around success path only. Most
well written functions have a success path and an error path. In case of
error, they should be able to report warning/errors. If raising an error
from a function causes a breakage of an external api, then that is an
architectural problem imho. Instead of reverting the error/warning, we
should try to fix the larger problem at hand in the longer term.
On 1/20/22 16:31, Ani Sinha wrote:
>
>
> On Thu, 20 Jan 2022, Andrea Bolognani wrote:
>
>> On Tue, Jan 18, 2022 at 12:44:51PM +0100, Michal Privoznik wrote:
>>> +++ b/src/util/virprocess.c
>>> @@ -1784,10 +1784,7 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
>>> virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10, &systime) < 0 ||
>>> virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, &rss) < 0 ||
>>> virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, &cpu) < 0) {
>>> - virReportError(VIR_ERR_INTERNAL_ERROR,
>>> - _("cannot parse process status data for pid '%d/%d'"),
>>> - (int) pid, (int) tid);
>>> - return -1;
>>> + VIR_WARN("cannot parse process status data");
>>
>> Shame to lose the improved error/warning message. Perhaps it could be
>> reintroduced separately.
>>
>
> Functions in general are not coded around success path only. Most
> well written functions have a success path and an error path. In case of
> error, they should be able to report warning/errors. If raising an error
> from a function causes a breakage of an external api, then that is an
> architectural problem imho. Instead of reverting the error/warning, we
> should try to fix the larger problem at hand in the longer term.
>
Well, until we get there we should fix the upstream so that we don't
have another broken release.
Michal
On Thu, Jan 20, 2022 at 21:14 Michal Prívozník <mprivozn@redhat.com> wrote:
> On 1/20/22 16:31, Ani Sinha wrote:
> >
> >
> > On Thu, 20 Jan 2022, Andrea Bolognani wrote:
> >
> >> On Tue, Jan 18, 2022 at 12:44:51PM +0100, Michal Privoznik wrote:
> >>> +++ b/src/util/virprocess.c
> >>> @@ -1784,10 +1784,7 @@ virProcessGetStatInfo(unsigned long long
> *cpuTime,
> >>> virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL,
> 10, &systime) < 0 ||
> >>> virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10,
> &rss) < 0 ||
> >>> virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL,
> 10, &cpu) < 0) {
> >>> - virReportError(VIR_ERR_INTERNAL_ERROR,
> >>> - _("cannot parse process status data for pid
> '%d/%d'"),
> >>> - (int) pid, (int) tid);
> >>> - return -1;
> >>> + VIR_WARN("cannot parse process status data");
> >>
> >> Shame to lose the improved error/warning message. Perhaps it could be
> >> reintroduced separately.
> >>
> >
> > Functions in general are not coded around success path only. Most
> > well written functions have a success path and an error path. In case of
> > error, they should be able to report warning/errors. If raising an error
> > from a function causes a breakage of an external api, then that is an
> > architectural problem imho. Instead of reverting the error/warning, we
> > should try to fix the larger problem at hand in the longer term.
> >
>
> Well, until we get there we should fix the upstream so that we don't
> have another broken release.
AKA kicking the can one more time 🙃
On 1/20/22 16:48, Ani Sinha wrote: > > > > AKA kicking the can one more time 🙃 Well, I should have been more careful and not merge the patch in the first place. Changing API behavior is something we should never do. Looking at the code closer, it looks like all callers of this function would need to ignore the reported error so that their behavior is not changed. At this point, does it make sense to report an error in the function? Michal
On Thu, Jan 20, 2022 at 21:29 Michal Prívozník <mprivozn@redhat.com> wrote: > On 1/20/22 16:48, Ani Sinha wrote: > > > > > > > > > AKA kicking the can one more time 🙃 > > Well, I should have been more careful and not merge the patch in the > first place. Changing API behavior is something we should never do. > > Looking at the code closer, it looks like all callers of this function > would need to ignore the reported error so that their behavior is not > changed. At this point, does it make sense to report an error in the > function? The callers can decide what do with the error raised by the function. We should not write functions that cannot fail. >
On 1/20/22 18:23, Ani Sinha wrote: > > > On Thu, Jan 20, 2022 at 21:29 Michal Prívozník <mprivozn@redhat.com > <mailto:mprivozn@redhat.com>> wrote: > > On 1/20/22 16:48, Ani Sinha wrote: > > > > > > > > > AKA kicking the can one more time 🙃 > > Well, I should have been more careful and not merge the patch in the > first place. Changing API behavior is something we should never do. > > Looking at the code closer, it looks like all callers of this function > would need to ignore the reported error so that their behavior is not > changed. At this point, does it make sense to report an error in the > function? > > > The callers can decide what do with the error raised by the function. We > should not write functions that cannot fail. > But that's not what the commit does, is it. It changed some public APIs from best effort to fail early. Therefore, it was reverted until we can come up with proper fix. Libvirt's promise and value is in stability of its APIs. We want users to update libvirt without having to rewrite their app (or even rebuild it = ABI stability). And the commit broke that promise. It's only fair that it is reverted until proper solution is found. Michal
On Fri, 21 Jan 2022, Michal Prívozník wrote: > On 1/20/22 18:23, Ani Sinha wrote: > > > > > > On Thu, Jan 20, 2022 at 21:29 Michal Prívozník <mprivozn@redhat.com > > <mailto:mprivozn@redhat.com>> wrote: > > > > On 1/20/22 16:48, Ani Sinha wrote: > > > > > > > > > > > > > > AKA kicking the can one more time 🙃 > > > > Well, I should have been more careful and not merge the patch in the > > first place. Changing API behavior is something we should never do. > > > > Looking at the code closer, it looks like all callers of this function > > would need to ignore the reported error so that their behavior is not > > changed. At this point, does it make sense to report an error in the > > function? > > > > > > The callers can decide what do with the error raised by the function. We > > should not write functions that cannot fail. > > > > But that's not what the commit does, is it. It changed some public APIs > from best effort to fail early. That is the side effect and I consider it as a bug. Imagine we add some more code into that function tomorrow and there is an error path. Now because of this bug, we will have to ignore the error condition and not report it. How ridiculous is that? We should have kept the patch as is and fix the callers so that the public APIs were not broken. > Libvirt's promise and value is in stability of its APIs. We want users > to update libvirt without having to rewrite their app (or even rebuild > it = ABI stability). And the commit broke that promise. It's only fair > that it is reverted until proper solution is found. > I have no argument with the above.
On Fri, Jan 21, 2022 at 07:38:35PM +0530, Ani Sinha wrote: > > > On Fri, 21 Jan 2022, Michal Prívozník wrote: > > > On 1/20/22 18:23, Ani Sinha wrote: > > > > > > > > > On Thu, Jan 20, 2022 at 21:29 Michal Prívozník <mprivozn@redhat.com > > > <mailto:mprivozn@redhat.com>> wrote: > > > > > > On 1/20/22 16:48, Ani Sinha wrote: > > > > > > > > > > > > > > > > > > > AKA kicking the can one more time 🙃 > > > > > > Well, I should have been more careful and not merge the patch in the > > > first place. Changing API behavior is something we should never do. > > > > > > Looking at the code closer, it looks like all callers of this function > > > would need to ignore the reported error so that their behavior is not > > > changed. At this point, does it make sense to report an error in the > > > function? > > > > > > > > > The callers can decide what do with the error raised by the function. We > > > should not write functions that cannot fail. > > > > > > > But that's not what the commit does, is it. It changed some public APIs > > from best effort to fail early. > > That is the side effect and I consider it as a bug. Imagine we add some > more code into that function tomorrow and there is an error path. Now > because of this bug, we will have to ignore the error condition and not > report it. How ridiculous is that? > > We should have kept the patch as is and fix the callers so that the public > APIs were not broken. That is not the libvirt approach. Our #1 priority is public API compatibility, everything else is subservient to that. So when we have a regression we fix that in the quickest possible way. Simply reverting the broken patch was the quickest option here. Figuring out a correct long term solution can follow up later 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 :|
On Fri, 21 Jan 2022, Daniel P. Berrangé wrote: > On Fri, Jan 21, 2022 at 07:38:35PM +0530, Ani Sinha wrote: > > > > > > On Fri, 21 Jan 2022, Michal Prívozník wrote: > > > > > On 1/20/22 18:23, Ani Sinha wrote: > > > > > > > > > > > > On Thu, Jan 20, 2022 at 21:29 Michal Prívozník <mprivozn@redhat.com > > > > <mailto:mprivozn@redhat.com>> wrote: > > > > > > > > On 1/20/22 16:48, Ani Sinha wrote: > > > > > > > > > > > > > > > > > > > > > > > > AKA kicking the can one more time 🙃 > > > > > > > > Well, I should have been more careful and not merge the patch in the > > > > first place. Changing API behavior is something we should never do. > > > > > > > > Looking at the code closer, it looks like all callers of this function > > > > would need to ignore the reported error so that their behavior is not > > > > changed. At this point, does it make sense to report an error in the > > > > function? > > > > > > > > > > > > The callers can decide what do with the error raised by the function. We > > > > should not write functions that cannot fail. > > > > > > > > > > But that's not what the commit does, is it. It changed some public APIs > > > from best effort to fail early. > > > > That is the side effect and I consider it as a bug. Imagine we add some > > more code into that function tomorrow and there is an error path. Now > > because of this bug, we will have to ignore the error condition and not > > report it. How ridiculous is that? > > > > We should have kept the patch as is and fix the callers so that the public > > APIs were not broken. > > That is not the libvirt approach. Our #1 priority is public API > compatibility, everything else is subservient to that. So when we > have a regression we fix that in the quickest possible way. Simply > reverting the broken patch> (a) I take exception to the fact that the patch reverted was "broken". (b) I have sent a patch adding a comment to that function as to why we cannot report any error there. Wider people can message the comment in any way they like. It would remind us to fix the code in the future and at the same time prevent others from wasting their time going down the path I took.
© 2016 - 2024 Red Hat, Inc.