10:49 a.m.
The value is stored in unsigned long long, so ULLONG_MAX is the proper
upper limit to use.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
Even though this is a build-breaker (for 32bit systems
memtune-unlimited fails to parse), I'm not pushing it as one because
it feels odd that such change wouldn't break anything else.
src/conf/domain_conf.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a351382..beb3d26 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6388,16 +6388,10 @@ virDomainParseMemory(const char *xpath, xmlXPathContextPtr ctxt,
unsigned long long *mem, bool required)
{
int ret = -1;
- unsigned long long bytes, max;
-
- /* On 32-bit machines, our bound is 0xffffffff * KiB. On 64-bit
- * machines, our bound is off_t (2^63). */
- if (sizeof(unsigned long) < sizeof(long long))
- max = 1024ull * ULONG_MAX;
- else
- max = LLONG_MAX;
+ unsigned long long bytes;
- ret = virDomainParseScaledValue(xpath, ctxt, &bytes, 1024, max, required);
+ ret = virDomainParseScaledValue(xpath, ctxt, &bytes, 1024,
+ ULLONG_MAX, required);
if (ret < 0)
goto cleanup;
--
2.1.2
10:55 a.m.
On 10/30/2014 09:49 AM, Martin Kletzander wrote:
The value is stored in unsigned long long, so ULLONG_MAX is the
proper
upper limit to use.
No, it's not.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
Even though this is a build-breaker (for 32bit systems
memtune-unlimited fails to parse), I'm not pushing it as one because
it feels odd that such change wouldn't break anything else.
What's the compile failure? This patch is intentionally trying to fit
the largest value that will fit in an unsigned long when scaled by
kilobytes, while still parsing by bytes. Thus, on 64-bit machines, you
can parse 0xffffffffffffffff bytes, then scale down by 1024 and store in
unsigned long; on 32-bit machines, your maximum has to be limited to
0xffffffff*1024 bytes before scaling back down.
src/conf/domain_conf.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a351382..beb3d26 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6388,16 +6388,10 @@ virDomainParseMemory(const char *xpath, xmlXPathContextPtr ctxt,
unsigned long long *mem, bool required)
{
int ret = -1;
- unsigned long long bytes, max;
-
- /* On 32-bit machines, our bound is 0xffffffff * KiB. On 64-bit
- * machines, our bound is off_t (2^63). */
- if (sizeof(unsigned long) < sizeof(long long))
- max = 1024ull * ULONG_MAX;
- else
- max = LLONG_MAX;
+ unsigned long long bytes;
- ret = virDomainParseScaledValue(xpath, ctxt, &bytes, 1024, max, required);
+ ret = virDomainParseScaledValue(xpath, ctxt, &bytes, 1024,
+ ULLONG_MAX, required);
NACK. I need to see the compiler failure to see the proper fix, but
this fix is not going to work.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:09 a.m.
On 10/30/2014 09:55 AM, Eric Blake wrote:
On 10/30/2014 09:49 AM, Martin Kletzander wrote:
> The value is stored in unsigned long long, so ULLONG_MAX is the proper
> upper limit to use.
No, it's not.
>
> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> ---
> Even though this is a build-breaker (for 32bit systems
> memtune-unlimited fails to parse), I'm not pushing it as one because
> it feels odd that such change wouldn't break anything else.
What's the compile failure? This patch is intentionally trying to fit
the largest value that will fit in an unsigned long when scaled by
kilobytes, while still parsing by bytes. Thus, on 64-bit machines, you
can parse 0xffffffffffffffff bytes, then scale down by 1024 and store in
unsigned long; on 32-bit machines, your maximum has to be limited to
0xffffffff*1024 bytes before scaling back down.
Sounds like the real bug is in the memtune-unlimited test. 2^53-1 is
the maximum memory for a 64-bit host, but invalid on a 32-bit host. Any
interface that uses 'unsigned long' as its capping point has to have
different maximum limits on 32-bit hosts.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:23 a.m.
On 10/30/2014 10:09 AM, Eric Blake wrote:
On 10/30/2014 09:55 AM, Eric Blake wrote:
> On 10/30/2014 09:49 AM, Martin Kletzander wrote:
>> The value is stored in unsigned long long, so ULLONG_MAX is the proper
>> upper limit to use.
>
> No, it's not.
>
>>
>> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
>> ---
>> Even though this is a build-breaker (for 32bit systems
>> memtune-unlimited fails to parse), I'm not pushing it as one because
>> it feels odd that such change wouldn't break anything else.
>
> What's the compile failure? This patch is intentionally trying to fit
> the largest value that will fit in an unsigned long when scaled by
> kilobytes, while still parsing by bytes. Thus, on 64-bit machines, you
> can parse 0xffffffffffffffff bytes, then scale down by 1024 and store in
> unsigned long; on 32-bit machines, your maximum has to be limited to
> 0xffffffff*1024 bytes before scaling back down.
Sounds like the real bug is in the memtune-unlimited test. 2^53-1 is
the maximum memory for a 64-bit host, but invalid on a 32-bit host. Any
interface that uses 'unsigned long' as its capping point has to have
different maximum limits on 32-bit hosts.
Or maybe the problem is that at some point we used unsigned long, and
later moved to unsigned long long, but never updated the comment? I'm
trying to investigate the history of this code...
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:51 a.m.
On 10/30/2014 10:23 AM, Eric Blake wrote:
Or maybe the problem is that at some point we used unsigned long, and
later moved to unsigned long long, but never updated the comment? I'm
trying to investigate the history of this code...
Looking a bit deeper, commit 4888f0fb5 was where we changed all internal
tracking to use unsigned long long; but we still have the public API
virDomain{Get,Set}MaxMemory that are tied to 'unsigned long'. I guess
what I was trying to do in 2e22f23 (both in 2012) was to ensure that
virDomainGetMaxMemory would never have to fail due to overflow, by
capping the input to something that would fit in the output. And we
still got it wrong then, since commit 19e7c04 (in 2013) fixed a case
where 32-bit hosts were truncating incorrectly.
Meanwhile, it looks like virDomain{Get,Set}MemoryParamters have ALWAYS
used ullong. But this interface doesn't report max memory - so we still
have to live with the fact that there is currently no interface for the
user getting at max memory other than crippled API.
Maybe the solution is to enhance virDomain{Get,Set}MemoryParameters to
be a superset of the older APIs of virDomain{Get,Set}MaxMemory, and
teach the older APIs to fail with VIR_ERR_OVERFLOW when the value can
only be passed through the newer API.
Or maybe the solution is to make virDomainParseMemory add a bool
parameter that says whether the caller is old-style (32-bit cap) or
new-style (full 64-bit width), and set the maximum according to that
information. That's probably the easiest for doing right now. I can
take a stab at it, since it was my commit in 2012 that originally
introduced the weird 32-bit cap even when parsing a 64-bit field.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3:01 p.m.
On 10/30/2014 10:51 AM, Eric Blake wrote:
On 10/30/2014 10:23 AM, Eric Blake wrote:
>
> Or maybe the problem is that at some point we used unsigned long, and
> later moved to unsigned long long, but never updated the comment? I'm
> trying to investigate the history of this code...
Or maybe the solution is to make virDomainParseMemory add a bool
parameter that says whether the caller is old-style (32-bit cap) or
new-style (full 64-bit width), and set the maximum according to that
information. That's probably the easiest for doing right now. I can
take a stab at it, since it was my commit in 2012 that originally
introduced the weird 32-bit cap even when parsing a 64-bit field.
I went with this idea:
https://www.redhat.com/archives/libvir-list/2014-October/msg01084.html
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3660
days inactive
3660
days old
5 comments
2 participants
participants (2)
-
Eric Blake -
Martin Kletzander