> On Jun 19, 2019, at 3:42 PM, Dave Wysochanski <dwysocha@xxxxxxxxxx> wrote:
>
> On Wed, 2019-06-19 at 13:45 -0400, Chuck Lever wrote:
>>> On Jun 19, 2019, at 1:42 PM, Chuck Lever <chuck.lever@xxxxxxxxxx>
>>> wrote:
>>>
>>>
>>>
>>>> On Jun 19, 2019, at 1:22 PM, David Wysochanski <
>>>> dwysocha@xxxxxxxxxx> wrote:
>>>>
>>>>
>>>>
>>>> On Wed, Jun 19, 2019 at 12:35 PM Chuck Lever <
>>>> chuck.lever@xxxxxxxxxx> wrote:
>>>>
>>>>
>>>>> On Jun 13, 2019, at 8:03 AM, Dave Wysochanski <
>>>>> dwysocha@xxxxxxxxxx> wrote:
>>>>>
>>>>> Add explicit check for statsvers instead of array based check.
>>>>
>>>> Hi Dave,
>>>>
>>>> I don't understand why this change is necessary. The patch
>>>> description
>>>> should explain.
>>>>
>>>>
>>>> Steve had already taken commit 73491ef for mountstats which was
>>>> an array based check. This just makes this patch consistent with
>>>> the others. Is that what you mean - you want a statement about
>>>> consistency and refer to the other commit? How about:
>>>>
>>>> Commit 73491ef added per-op error counts for mountstats command
>>>> but used an array based check rather than checking statsver. Add
>>>> explicit check for statsver instead of array based check for
>>>> consistency with other tools.
>>>
>>> This is a better patch description (explains "why" not "what"),
>>> but I'm not clear why this change is necessary in either place.
>>
>> In other words, was this change necessary to fix a bug? Or is
>> this a defensive change to make parsing more robust?
>>
>
> I try to state "fix" somewhere in there when it is a bug fix - so no
> this does not fix a bug. In in some ways the original check was better
> because it makes no assumption of what 'statsver' means at any time.
> I'm not sure if you're really concerned about the commit message or you
> would prefer the array check?
Both. The array check is done for all the other variables too, IIRC.
There doesn't seem to be a reason to check the statvers. If it's not
too much trouble, please resubmit so that the new checks are consistent
with existing checks.
> I can see argument for array check and I
> can change the other patches and resubmit if you prefer that.
>
> Commit 73491ef added per-op error counts for mountstats command
> but used an array based check rather than checking statsver. Check
> statsver >= 1.1 explicitly as this documents when this new count was
> added to the kernel.
Not sure there's a need for the statvers bump here either. There
are some rules about when a statvers bump is necessary, but in my
old age I don't remember what they are. Anyway, if the statvers is
bumped already, no big deal.
>>>>> Signed-off-by: Dave Wysochanski <dwysocha@xxxxxxxxxx>
>>>>> ---
>>>>> tools/mountstats/mountstats.py | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/tools/mountstats/mountstats.py
>>>>> b/tools/mountstats/mountstats.py
>>>>> index 5f13bf8e..2ebbf945 100755
>>>>> --- a/tools/mountstats/mountstats.py
>>>>> +++ b/tools/mountstats/mountstats.py
>>>>> @@ -476,7 +476,7 @@ class DeviceData:
>>>>> if retrans != 0:
>>>>> print('\t%d retrans (%d%%)' % (retrans,
>>>>> ((retrans * 100) / count)), end=' ')
>>>>> print('\t%d major timeouts' % stats[3],
>>>>> end='')
>>>>> - if len(stats) >= 10 and stats[9] != 0:
>>>>> + if self.__rpc_data['statsvers'] >= 1.1 and
>>>>> stats[9] != 0:
>>>>> print('\t%d errors (%d%%)' % (stats[9],
>>>>> ((stats[9] * 100) / count)))
>>>>> else:
>>>>> print('')
>>>>> --
>>>>> 2.20.1
>>>>>
>>>>
>>>> --
>>>> Chuck Lever
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Dave Wysochanski
>>>> Principal Software Maintenance Engineer
>>>> T: 919-754-4024
>>>
>>> --
>>> Chuck Lever
>>
>> --
>> Chuck Lever
--
Chuck Lever
