[PATCH] 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. Signed-off-by: Ani Sinha <ani@anisinha.ca> --- src/util/virprocess.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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, &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) { - 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
On 1/7/22 10:09, 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.
Signed-off-by: Ani Sinha <ani@anisinha.ca> --- src/util/virprocess.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
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, &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) { - 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
Couple of problems with this patch as is. I'm not against the idea of reporting an error here. But couple of things needs to be addressed first: 1) Currently, all callers check for retval and report an error if -1 was returned. This means, that even though this new message is reported it is immediately overwritten in caller. 2) The non-linux implementation now has to report error too. I believe it's obvious why from our previous discussion this morning. Michal
On Fri, 7 Jan 2022, Michal Prívozník wrote:
On 1/7/22 10:09, 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.
Signed-off-by: Ani Sinha <ani@anisinha.ca> --- src/util/virprocess.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
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, &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) { - 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
Couple of problems with this patch as is. I'm not against the idea of reporting an error here.
Good. now we are moving in the right direction. But couple of things needs to be addressed first:
1) Currently, all callers check for retval and report an error if -1 was returned. This means, that even though this new message is reported it is immediately overwritten in caller.
Let me fix the callers and send an updated patch. Meanwhile ...
2) The non-linux implementation now has to report error too. I believe it's obvious why from our previous discussion this morning.
Maybe you can fix your patch.
On 1/7/22 12:55, Ani Sinha wrote:
On Fri, 7 Jan 2022, Michal Prívozník wrote:
On 1/7/22 10:09, 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.
Signed-off-by: Ani Sinha <ani@anisinha.ca> --- src/util/virprocess.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
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, &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) { - 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
Couple of problems with this patch as is. I'm not against the idea of reporting an error here.
Good. now we are moving in the right direction.
But couple of things needs to be addressed first:
1) Currently, all callers check for retval and report an error if -1 was returned. This means, that even though this new message is reported it is immediately overwritten in caller.
Let me fix the callers and send an updated patch. Meanwhile ...
2) The non-linux implementation now has to report error too. I believe it's obvious why from our previous discussion this morning.
Maybe you can fix your patch.
There is nothing to fix. With current master nor Linux nor non-Linux variant reports any error, i.e. are consistent. This patch introduced inconsistency and in my review I have pointed it out. Looking forward to v2. Michal
participants (2)
-
Ani Sinha -
Michal Prívozník