Martin,
Yep that's definitely broken.
I've reworked your patch to rename firstSample to havePrev and changed
to comments to more accurately reflect what this control variable is
used for.
Also, all the bad error conditions cause explicit exit() with non-zero
status ... I think you can simply return 0 if control gets to the end of
main().
Let me know if the attached patch works for you and I'll commit it.
On Fri, 2010-12-17 at 15:56 -0500, Martin Hicks wrote:
> Hi,
>
> It has bugged me for a long time that pmval seem to always exit with "1"
> when everything goes correctly:
>
> mort@laplace:~$ pmval -s1 hinv.physmem
>
> metric: hinv.physmem
> host: localhost
> semantics: discrete instantaneous value
> units: Mbyte
> samples: 1
> 2002
> mort@laplace:~$ echo $?
> 1
>
>
> Here's an attempt to fix the problem, although I'm not completely sure
> what we should be returning if we get errors like "No samples found" in
> an archive. As written, this will exit with non-zero.
>
> The existing code almost does exactly the opposite of what I'd expect.
> i.e., return 1 on success.
>
> QA #088 fails due to a "No samples found" test as the final act, which
> now returns non-zero so the QA tests assume failure even though the
> output matches.
>
> Added a few comments, but mostly my goal was to make pmval return 0
> when there are no errors.
> ---
> src/pmval/pmval.c | 29 +++++++++++++++++++----------
> 1 files changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/src/pmval/pmval.c b/src/pmval/pmval.c
> index 158f2bd..32a67cd 100644
> --- a/src/pmval/pmval.c
> +++ b/src/pmval/pmval.c
> @@ -360,14 +360,14 @@ static int
> howide(int type)
> {
> switch (type) {
> - case PM_TYPE_32: return(11);
> - case PM_TYPE_U32: return(11);
> - case PM_TYPE_64: return(21);
> - case PM_TYPE_U64: return(21);
> - case PM_TYPE_FLOAT: return(13);
> - case PM_TYPE_DOUBLE: return(21);
> - case PM_TYPE_STRING: return(21);
> - case PM_TYPE_AGGREGATE: return(21);
> + case PM_TYPE_32: return 11;
> + case PM_TYPE_U32: return 11;
> + case PM_TYPE_64: return 21;
> + case PM_TYPE_U64: return 21;
> + case PM_TYPE_FLOAT: return 13;
> + case PM_TYPE_DOUBLE: return 21;
> + case PM_TYPE_STRING: return 21;
> + case PM_TYPE_AGGREGATE: return 21;
> default:
> fprintf(stderr, "pmval: unknown performance metric value type %s\n",
> pmTypeStr(type));
> exit(EXIT_FAILURE);
> @@ -1243,6 +1243,10 @@ main(int argc, char *argv[])
> if (gui)
> pmTimeStateVector(&controls, pmtime);
> if (firstSample) {
> + /*
> + * On the first time through fetch values that will act as the
> + * "previous" sample so we can compute a rate.
> + */
> if ((idx2 = getvals(&cntxt, &rslt2)) >= 0) {
> /* first-time success */
> firstSample = 0;
> @@ -1278,13 +1282,18 @@ main(int argc, char *argv[])
> __pmtimevalSleep(delta);
>
> if (firstSample)
> - continue; /* keep trying */
> + continue; /* keep trying to get the first sample */
>
> /* next sample */
> if ((idx1 = getvals(&cntxt, &rslt1)) == -2)
> /* out the end of the window */
> break;
> else if (idx1 < 0) {
> + /*
> + * Fall back to trying to get an initial sample because
> + * although we got the first sample, we failed to get the
> + * next sample.
> + */
> firstSample = 1;
> continue;
> }
> @@ -1314,5 +1323,5 @@ main(int argc, char *argv[])
> idx2 = idx1;
> }
>
> - exit(firstSample == 0);
> + return firstSample || idx1 < 0 || idx2 < 0;
> }
> --
> 1.7.2.3
>
>
patch.pmval
Description: Text Data
|