Re: [PATCH v2] report error when virProcessGetStatInfo() is unable to parse data
On Tue, Jan 11, 2022 at 15:16:36 +0530, Ani Sinha wrote:
>
>
> On Tue, 11 Jan 2022, Michal Prívozník wrote:
>
> > On 1/11/22 04:42, Ani Sinha wrote:
> > > Currently virProcessGetStatInfo() always returns success and only logs
> > > error
> > > when it is unable to parse the data. Make this function actually report
> > > the
> > > error and return a negative value in this error scenario.
> > >
> > > Fix the callers so that they do not override the error generated.
> > >
> > > Signed-off-by: Ani Sinha
> > > ---
> > > src/ch/ch_driver.c | 2 --
> > > src/qemu/qemu_driver.c | 7 +--
> > > src/util/virprocess.c | 6 +-
> > > 3 files changed, 6 insertions(+), 9 deletions(-)
> > >
> > > changelog:
> > > v2: fixed the callers
> > >
> >
> >
> > > diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> > > index c74bd16fe6..b9f498d5d8 100644
> > > --- a/src/util/virprocess.c
> > > +++ b/src/util/virprocess.c
> > > @@ -1783,7 +1783,11 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
> > > virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10,
> > > ) < 0 ||
> > > virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, )
> > > < 0 ||
> > > virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10,
> > > ) < 0) {
> > > -VIR_WARN("cannot parse process status data");
> > > +virReportError(VIR_ERR_INTERNAL_ERROR,
> > > + _("cannot parse process status data for pid
> > > '%d/%d'"),
> > > + (int) pid, (int) tid);
> > > +
> > > +return -1;
> > > }
> > >
> > > /* We got jiffies
> >
> > There's an alternative implementation for non-Linux platforms, but you
>
> but that has not been pushed yet I believe. Correct me if I am mistaken.
It was pushed already as it was fixing the build, which had higher
priority than any cosmetic issues which can be fixed later:
commit d7c64453aa0e9e0718e9292d7269ea3fe34f7c4c
Author: Michal Prívozník
Date: Thu Jan 6 20:13:08 2022 +0100
virprocess: Provide non-Linux stubs for virProcessGet{Stat,Sched}Info
Both virProcessGetStatInfo() and virProcessGetSchedInfo() are
Linux centric. Provide stubs for non-Linux platforms.
Fixes: d73852c49962fbfe652fc7bec595a3f242faf847
Signed-off-by: Michal Privoznik
Reviewed-by: Peter Krempa
Reviewed-by: Ján Tomko
Re: [PATCH v2] report error when virProcessGetStatInfo() is unable to parse data
On Tue, 11 Jan 2022, Michal Prívozník wrote:
> On 1/11/22 04:42, Ani Sinha wrote:
> > Currently virProcessGetStatInfo() always returns success and only logs error
> > when it is unable to parse the data. Make this function actually report the
> > error and return a negative value in this error scenario.
> >
> > Fix the callers so that they do not override the error generated.
> >
> > Signed-off-by: Ani Sinha
> > ---
> > src/ch/ch_driver.c | 2 --
> > src/qemu/qemu_driver.c | 7 +--
> > src/util/virprocess.c | 6 +-
> > 3 files changed, 6 insertions(+), 9 deletions(-)
> >
> > changelog:
> > v2: fixed the callers
> >
>
>
> > diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> > index c74bd16fe6..b9f498d5d8 100644
> > --- a/src/util/virprocess.c
> > +++ b/src/util/virprocess.c
> > @@ -1783,7 +1783,11 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
> > virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10,
> > ) < 0 ||
> > virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, ) <
> > 0 ||
> > virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10,
> > ) < 0) {
> > -VIR_WARN("cannot parse process status data");
> > +virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("cannot parse process status data for pid
> > '%d/%d'"),
> > + (int) pid, (int) tid);
> > +
> > +return -1;
> > }
> >
> > /* We got jiffies
>
> There's an alternative implementation for non-Linux platforms, but you
but that has not been pushed yet I believe. Correct me if I am mistaken.
> are introducing error reporting only to this implementation. Both
> versions have to behave the same, i.e. the non-Linux implementation now
> has to report error too. Can you fix that in v3?
I think we should keep the scope of this patch as is. Why don't we do
this. Lets push this patch. Then I will fix the patch for non-linux
platforms and send it over.
Re: [PATCH v2] report error when virProcessGetStatInfo() is unable to parse data
On 1/11/22 04:42, Ani Sinha wrote:
> Currently virProcessGetStatInfo() always returns success and only logs error
> when it is unable to parse the data. Make this function actually report the
> error and return a negative value in this error scenario.
>
> Fix the callers so that they do not override the error generated.
>
> Signed-off-by: Ani Sinha
> ---
> src/ch/ch_driver.c | 2 --
> src/qemu/qemu_driver.c | 7 +--
> src/util/virprocess.c | 6 +-
> 3 files changed, 6 insertions(+), 9 deletions(-)
>
> changelog:
> v2: fixed the callers
>
> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> index c74bd16fe6..b9f498d5d8 100644
> --- a/src/util/virprocess.c
> +++ b/src/util/virprocess.c
> @@ -1783,7 +1783,11 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
> virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10,
> ) < 0 ||
> virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, ) < 0
> ||
> virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10,
> ) < 0) {
> -VIR_WARN("cannot parse process status data");
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("cannot parse process status data for pid '%d/%d'"),
> + (int) pid, (int) tid);
> +
> +return -1;
> }
>
> /* We got jiffies
There's an alternative implementation for non-Linux platforms, but you
are introducing error reporting only to this implementation. Both
versions have to behave the same, i.e. the non-Linux implementation now
has to report error too. Can you fix that in v3?
Michal
[PATCH v2] report error when virProcessGetStatInfo() is unable to parse data
Currently virProcessGetStatInfo() always returns success and only logs error
when it is unable to parse the data. Make this function actually report the
error and return a negative value in this error scenario.
Fix the callers so that they do not override the error generated.
Signed-off-by: Ani Sinha
---
src/ch/ch_driver.c | 2 --
src/qemu/qemu_driver.c | 7 +--
src/util/virprocess.c | 6 +-
3 files changed, 6 insertions(+), 9 deletions(-)
changelog:
v2: fixed the callers
diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
index 53e0872207..3cbc668489 100644
--- a/src/ch/ch_driver.c
+++ b/src/ch/ch_driver.c
@@ -1073,8 +1073,6 @@ chDomainHelperGetVcpus(virDomainObj *vm,
if (virProcessGetStatInfo(>cpuTime,
>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 4974450333..015ffb2ce7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1359,8 +1359,6 @@ qemuDomainHelperGetVcpus(virDomainObj *vm,
if (virProcessGetStatInfo(>cpuTime,
>cpu, NULL,
vm->pid, vcpupid) < 0) {
-virReportSystemError(errno, "%s",
- _("cannot get vCPU placement & pCPU
time"));
return -1;
}
}
@@ -2521,8 +2519,6 @@ 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;
}
}
@@ -10530,8 +10526,7 @@ qemuDomainMemoryStatsInternal(virQEMUDriver *driver,
}
if (virProcessGetStatInfo(NULL, NULL, , vm->pid, 0) < 0) {
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
- _("cannot get RSS for domain"));
+return -1;
} 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 c74bd16fe6..b9f498d5d8 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -1783,7 +1783,11 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10,
) < 0 ||
virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, ) < 0 ||
virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, )
< 0) {
-VIR_WARN("cannot parse process status data");
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot parse process status data for pid '%d/%d'"),
+ (int) pid, (int) tid);
+
+return -1;
}
/* We got jiffies
--
2.25.1
[PATCH v2] report error when virProcessGetStatInfo() is unable to parse data
Currently virProcessGetStatInfo() always returns success and only logs error
when it is unable to parse the data. Make this function actually report the
error and return a negative value in this error scenario.
Fix the callers so that they do not override the error generated.
Signed-off-by: Ani Sinha
---
src/ch/ch_driver.c | 2 --
src/qemu/qemu_driver.c | 7 +--
src/util/virprocess.c | 6 +-
3 files changed, 6 insertions(+), 9 deletions(-)
changelog:
v2: fixed the callers
diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
index 53e0872207..3cbc668489 100644
--- a/src/ch/ch_driver.c
+++ b/src/ch/ch_driver.c
@@ -1073,8 +1073,6 @@ chDomainHelperGetVcpus(virDomainObj *vm,
if (virProcessGetStatInfo(>cpuTime,
>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 4974450333..015ffb2ce7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1359,8 +1359,6 @@ qemuDomainHelperGetVcpus(virDomainObj *vm,
if (virProcessGetStatInfo(>cpuTime,
>cpu, NULL,
vm->pid, vcpupid) < 0) {
-virReportSystemError(errno, "%s",
- _("cannot get vCPU placement & pCPU
time"));
return -1;
}
}
@@ -2521,8 +2519,6 @@ 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;
}
}
@@ -10530,8 +10526,7 @@ qemuDomainMemoryStatsInternal(virQEMUDriver *driver,
}
if (virProcessGetStatInfo(NULL, NULL, , vm->pid, 0) < 0) {
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
- _("cannot get RSS for domain"));
+return -1;
} 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 c74bd16fe6..b9f498d5d8 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -1783,7 +1783,11 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10,
) < 0 ||
virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, ) < 0 ||
virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, )
< 0) {
-VIR_WARN("cannot parse process status data");
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot parse process status data for pid '%d/%d'"),
+ (int) pid, (int) tid);
+
+return -1;
}
/* We got jiffies
--
2.25.1
