[libvirt] [PATCH 4/8] process: Translate "unlimited" correctly

Andrea Bolognani posted 8 patches 8 years, 10 months ago
There is a newer version of this series
[libvirt] [PATCH 4/8] process: Translate "unlimited" correctly
Posted by Andrea Bolognani 8 years, 10 months ago
The value we use internally to represent the lack of a memory
locking limit, VIR_DOMAIN_MEMORY_PARAM_UNLIMITED, doesn't
match the value setrlimit() and prlimit() use for the same
purpose, RLIM_INFINITY, so we have to handle the translation
ourselves.
---
 src/util/virprocess.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 16eb412..1fbbbb3 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -747,7 +747,15 @@ virProcessSetMaxMemLock(pid_t pid, unsigned long long bytes)
     if (bytes == 0)
         return 0;
 
-    rlim.rlim_cur = rlim.rlim_max = bytes;
+    /* We use VIR_DOMAIN_MEMORY_PARAM_UNLIMITED internally to represent
+     * unlimited memory amounts, but setrlimit() and prlimit() use
+     * RLIM_INFINITY for the same purpose, so we need to translate between
+     * the two conventions */
+    if (virMemoryLimitIsSet(bytes))
+        rlim.rlim_cur = rlim.rlim_max = bytes;
+    else
+        rlim.rlim_cur = rlim.rlim_max = RLIM_INFINITY;
+
     if (pid == 0) {
         if (setrlimit(RLIMIT_MEMLOCK, &rlim) < 0) {
             virReportSystemError(errno,
@@ -810,8 +818,14 @@ virProcessGetMaxMemLock(pid_t pid,
     }
 
     /* virProcessSetMaxMemLock() sets both rlim_cur and rlim_max to the
-     * same value, so we can retrieve just rlim_max here */
-    *bytes = rlim.rlim_max;
+     * same value, so we can retrieve just rlim_max here. We use
+     * VIR_DOMAIN_MEMORY_PARAM_UNLIMITED internally to represent unlimited
+     * memory amounts, but setrlimit() and prlimit() use RLIM_INFINITY for the
+     * same purpose, so we need to translate between the two conventions */
+    if (rlim.rlim_max == RLIM_INFINITY)
+        *bytes = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED;
+    else
+        *bytes = rlim.rlim_max;
 
     return 0;
 }
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/8] process: Translate "unlimited" correctly
Posted by Luiz Capitulino 8 years, 10 months ago
On Thu, 23 Mar 2017 19:16:43 +0100
Andrea Bolognani <abologna@redhat.com> wrote:

> The value we use internally to represent the lack of a memory
> locking limit, VIR_DOMAIN_MEMORY_PARAM_UNLIMITED, doesn't
> match the value setrlimit() and prlimit() use for the same
> purpose, RLIM_INFINITY, so we have to handle the translation
> ourselves.
> ---
>  src/util/virprocess.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> index 16eb412..1fbbbb3 100644
> --- a/src/util/virprocess.c
> +++ b/src/util/virprocess.c
> @@ -747,7 +747,15 @@ virProcessSetMaxMemLock(pid_t pid, unsigned long long bytes)
>      if (bytes == 0)
>          return 0;
>  
> -    rlim.rlim_cur = rlim.rlim_max = bytes;
> +    /* We use VIR_DOMAIN_MEMORY_PARAM_UNLIMITED internally to represent
> +     * unlimited memory amounts, but setrlimit() and prlimit() use
> +     * RLIM_INFINITY for the same purpose, so we need to translate between
> +     * the two conventions */
> +    if (virMemoryLimitIsSet(bytes))
> +        rlim.rlim_cur = rlim.rlim_max = bytes;
> +    else
> +        rlim.rlim_cur = rlim.rlim_max = RLIM_INFINITY;

I know I'm not very smart, but I had trouble parsing this. What
about:

if (virMemoryLimitIsInfinity(bytes))
	rlim.rlim_cur = rlim.rlim_max = RLIM_INFINITY;
...

This reads better, and avoids using virMemoryLimitIsSet() which
seems very error-prone. It doesn't check for zero and it's strange
that "limit < infinity" means "limit is set".

> +
>      if (pid == 0) {
>          if (setrlimit(RLIMIT_MEMLOCK, &rlim) < 0) {
>              virReportSystemError(errno,
> @@ -810,8 +818,14 @@ virProcessGetMaxMemLock(pid_t pid,
>      }
>  
>      /* virProcessSetMaxMemLock() sets both rlim_cur and rlim_max to the
> -     * same value, so we can retrieve just rlim_max here */
> -    *bytes = rlim.rlim_max;
> +     * same value, so we can retrieve just rlim_max here. We use
> +     * VIR_DOMAIN_MEMORY_PARAM_UNLIMITED internally to represent unlimited
> +     * memory amounts, but setrlimit() and prlimit() use RLIM_INFINITY for the
> +     * same purpose, so we need to translate between the two conventions */
> +    if (rlim.rlim_max == RLIM_INFINITY)
> +        *bytes = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED;
> +    else
> +        *bytes = rlim.rlim_max;
>  
>      return 0;
>  }

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/8] process: Translate "unlimited" correctly
Posted by Andrea Bolognani 8 years, 10 months ago
On Fri, 2017-03-24 at 13:47 -0400, Luiz Capitulino wrote:
> > @@ -747,7 +747,15 @@ virProcessSetMaxMemLock(pid_t pid, unsigned long long bytes)
> >      if (bytes == 0)
> >          return 0;
> >  
> > -    rlim.rlim_cur = rlim.rlim_max = bytes;
> > +    /* We use VIR_DOMAIN_MEMORY_PARAM_UNLIMITED internally to represent
> > +     * unlimited memory amounts, but setrlimit() and prlimit() use
> > +     * RLIM_INFINITY for the same purpose, so we need to translate between
> > +     * the two conventions */
> > +    if (virMemoryLimitIsSet(bytes))
> > +        rlim.rlim_cur = rlim.rlim_max = bytes;
> > +    else
> > +        rlim.rlim_cur = rlim.rlim_max = RLIM_INFINITY;
> 
> I know I'm not very smart, but I had trouble parsing this. What
> about:
> 
> if (virMemoryLimitIsInfinity(bytes))
> 	rlim.rlim_cur = rlim.rlim_max = RLIM_INFINITY;
> ...
> 
> This reads better, and avoids using virMemoryLimitIsSet() which
> seems very error-prone. It doesn't check for zero and it's strange
> that "limit < infinity" means "limit is set".

I would love to have something like that, and in fact I
spent a significant amount of time trying to clean up the
way libvirt stores and represents memory limits. The short
version is: not gonna happen ;)

Moreover, while the current API looks poorly named in this
context, it's also used for other memory limits and the
names make more sense there, so it's not a terrible API
when you look at the big picture.

The case where bytes is zero is accounted for, though. You
can see it right at the beginning of the hunk.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list