[libvirt] [PATCH] tests: Only format the CPU frequency if it's known

[John Ferlan]

On 01/09/2018 04:06 AM, Andrea Bolognani wrote:
> On Mon, 2018-01-08 at 11:19 -0500, John Ferlan wrote:
>> On 01/08/2018 09:50 AM, Peter Krempa wrote:
>>> On Mon, Jan 08, 2018 at 15:10:29 +0100, Andrea Bolognani wrote:
>>>> Instead of formatting 'MHz: 0', which can be confusing, skip the
>>>> field altogether. This behavior is consistent with that of 'virsh
>>>> nodeinfo'.
>>>
>>> Well, these are tests, so confusion really should not be a problem.
>>> Formatting all the values unconditionally has a benefit that you don't
>>> have to look at the code to see when some are formatted, but rather know
>>> the raw value instead.
>>>
>>> I'd suggest to not push this.
>>
>> My suggestion was less based on confusion and more on what's the
>> purpose. While I understand Peter's point - looking at the code and
>> digging into the data would still perhaps be necessary because we don't
>> know if 0 was because we couldn't get data or if it's not relevant.
>> Distinguishing between 0 as a read value (haha) and 0 as a non read
>> value is not possible.
>>
>> As I read Andrea's commit message from patch 5/5 (now commit id
>> a63ea8141) it seemed perhaps a better thing to do to not report MHz
>> since it's either not reported or incorrectly reported.
>>
>> The crux being if MHz is something that one cannot ascertain from an ARM
>> processor at all, then why report it at all. In this case, there is no
>> valid raw value.
> 
> On the other hand, the virNodeInfo struct is part of the public API
> and clients are going to have to deal sensibly with zeros being in
> there. It's even documented:
> 
>   struct _virNodeInfo {
>     ...
>     unsigned int mhz;     /* expected CPU frequency, 0 if not known or
>                              on unusual architectures */
> 
> virsh should of course avoid formatting the information altogether
> in that case, and it does. But when it comes to tests, we don't need
> to make the output pretty or omit information: we're basically just
> dumping the contents of the structure, so it's okay for the zero to
> be there.
> 
> So I guess what I'm trying to say is that Peter convinced me this
> patch might not be such a good idea after all. Are you okay with
> dropping it?
> 

I'm fine with dropping it.

John