.../linux-aarch64-f21-mustang.expected | 2 +- .../linux-aarch64-rhel74-moonshot.expected | 2 +- .../linux-aarch64-rhelsa-3.19.0-mustang.expected | 2 +- .../linux-armv6l-raspberrypi.expected | 2 +- tests/virhostcputest.c | 28 +++++++++++++++++----- 5 files changed, 26 insertions(+), 10 deletions(-)
Instead of formatting 'MHz: 0', which can be confusing, skip the
field altogether. This behavior is consistent with that of 'virsh
nodeinfo'.
Suggested-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
.../linux-aarch64-f21-mustang.expected | 2 +-
.../linux-aarch64-rhel74-moonshot.expected | 2 +-
.../linux-aarch64-rhelsa-3.19.0-mustang.expected | 2 +-
.../linux-armv6l-raspberrypi.expected | 2 +-
tests/virhostcputest.c | 28 +++++++++++++++++-----
5 files changed, 26 insertions(+), 10 deletions(-)
diff --git a/tests/virhostcpudata/linux-aarch64-f21-mustang.expected b/tests/virhostcpudata/linux-aarch64-f21-mustang.expected
index ac950dc15..1ee3cf737 100644
--- a/tests/virhostcpudata/linux-aarch64-f21-mustang.expected
+++ b/tests/virhostcpudata/linux-aarch64-f21-mustang.expected
@@ -1 +1 @@
-CPUs: 8/8, MHz: 0, Nodes: 1, Sockets: 4, Cores: 2, Threads: 1
+CPUs: 8/8, Nodes: 1, Sockets: 4, Cores: 2, Threads: 1
diff --git a/tests/virhostcpudata/linux-aarch64-rhel74-moonshot.expected b/tests/virhostcpudata/linux-aarch64-rhel74-moonshot.expected
index 6776aa6c2..1926af836 100644
--- a/tests/virhostcpudata/linux-aarch64-rhel74-moonshot.expected
+++ b/tests/virhostcpudata/linux-aarch64-rhel74-moonshot.expected
@@ -1 +1 @@
-CPUs: 8/8, MHz: 0, Nodes: 1, Sockets: 1, Cores: 8, Threads: 1
+CPUs: 8/8, Nodes: 1, Sockets: 1, Cores: 8, Threads: 1
diff --git a/tests/virhostcpudata/linux-aarch64-rhelsa-3.19.0-mustang.expected b/tests/virhostcpudata/linux-aarch64-rhelsa-3.19.0-mustang.expected
index 6776aa6c2..1926af836 100644
--- a/tests/virhostcpudata/linux-aarch64-rhelsa-3.19.0-mustang.expected
+++ b/tests/virhostcpudata/linux-aarch64-rhelsa-3.19.0-mustang.expected
@@ -1 +1 @@
-CPUs: 8/8, MHz: 0, Nodes: 1, Sockets: 1, Cores: 8, Threads: 1
+CPUs: 8/8, Nodes: 1, Sockets: 1, Cores: 8, Threads: 1
diff --git a/tests/virhostcpudata/linux-armv6l-raspberrypi.expected b/tests/virhostcpudata/linux-armv6l-raspberrypi.expected
index 1c4c713d5..4961316fb 100644
--- a/tests/virhostcpudata/linux-armv6l-raspberrypi.expected
+++ b/tests/virhostcpudata/linux-armv6l-raspberrypi.expected
@@ -1 +1 @@
-CPUs: 1/1, MHz: 0, Nodes: 1, Sockets: 1, Cores: 1, Threads: 1
+CPUs: 1/1, Nodes: 1, Sockets: 1, Cores: 1, Threads: 1
diff --git a/tests/virhostcputest.c b/tests/virhostcputest.c
index d3ee18461..9a1a48098 100644
--- a/tests/virhostcputest.c
+++ b/tests/virhostcputest.c
@@ -32,6 +32,7 @@ linuxTestCompareFiles(const char *cpuinfofile,
const char *outputfile)
{
int ret = -1;
+ virBuffer buf = VIR_BUFFER_INITIALIZER;
char *actualData = NULL;
virNodeInfo nodeinfo;
FILE *cpuinfo;
@@ -57,20 +58,35 @@ linuxTestCompareFiles(const char *cpuinfofile,
}
VIR_FORCE_FCLOSE(cpuinfo);
- if (virAsprintf(&actualData,
- "CPUs: %u/%u, MHz: %u, Nodes: %u, Sockets: %u, "
- "Cores: %u, Threads: %u\n",
- nodeinfo.cpus, VIR_NODEINFO_MAXCPUS(nodeinfo),
- nodeinfo.mhz, nodeinfo.nodes, nodeinfo.sockets,
- nodeinfo.cores, nodeinfo.threads) < 0)
+ virBufferAsprintf(&buf,
+ "CPUs: %u/%u, ",
+ nodeinfo.cpus, VIR_NODEINFO_MAXCPUS(nodeinfo));
+
+ /* Only format the CPU frequency if it's known. This behavior is
+ * consistent with that of 'virsh nodeinfo' */
+ if (nodeinfo.mhz) {
+ virBufferAsprintf(&buf,
+ "MHz: %u, ",
+ nodeinfo.mhz);
+ }
+
+ virBufferAsprintf(&buf,
+ "Nodes: %u, Sockets: %u, Cores: %u, Threads: %u\n",
+ nodeinfo.nodes, nodeinfo.sockets,
+ nodeinfo.cores, nodeinfo.threads);
+
+ if (virBufferError(&buf))
goto fail;
+ actualData = virBufferContentAndReset(&buf);
+
if (virTestCompareToFile(actualData, outputfile) < 0)
goto fail;
ret = 0;
fail:
+ virBufferFreeAndReset(&buf);
VIR_FREE(actualData);
return ret;
}
--
2.14.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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'. > > Suggested-by: John Ferlan <jferlan@redhat.com> > Signed-off-by: Andrea Bolognani <abologna@redhat.com> > --- > .../linux-aarch64-f21-mustang.expected | 2 +- > .../linux-aarch64-rhel74-moonshot.expected | 2 +- > .../linux-aarch64-rhelsa-3.19.0-mustang.expected | 2 +- > .../linux-armv6l-raspberrypi.expected | 2 +- > tests/virhostcputest.c | 28 +++++++++++++++++----- > 5 files changed, 26 insertions(+), 10 deletions(-) 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. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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'. >> >> Suggested-by: John Ferlan <jferlan@redhat.com> >> Signed-off-by: Andrea Bolognani <abologna@redhat.com> >> --- >> .../linux-aarch64-f21-mustang.expected | 2 +- >> .../linux-aarch64-rhel74-moonshot.expected | 2 +- >> .../linux-aarch64-rhelsa-3.19.0-mustang.expected | 2 +- >> .../linux-armv6l-raspberrypi.expected | 2 +- >> tests/virhostcputest.c | 28 +++++++++++++++++----- >> 5 files changed, 26 insertions(+), 10 deletions(-) > > 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. If you want it, you have my ACK/R-B. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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?
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.