Re: [libvirt PATCH 04/17] util: Introduce virProcess{Get,Set}Limit()

2021-03-08 Thread Michal Privoznik

On 3/5/21 8:13 PM, Andrea Bolognani wrote:

These functions abstract part of the existing logic, which is
the same in all virProcessSetMax*() functions, and changes it
so that which underlying syscall is used depends on their
availability rather than on the context in which they are
called: since prlimit() and {g,s}etrlimit() have slightly
different requirements, using the same one every single time
should make for a more consistent experience.

As part of the change, we also remove the special case for
passing zero to virProcessSetMax*() functions: we have removed
all callers that depended on that functionality in the previous
commit, so this is now safe to do and makes the semantics
simpler.

This commit is better viewed with 'git show -w'.

Signed-off-by: Andrea Bolognani 
---
  src/util/virprocess.c | 175 +++---
  1 file changed, 95 insertions(+), 80 deletions(-)

diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index e01ff25540..38b248217e 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -738,10 +738,66 @@ virProcessPrLimit(pid_t pid G_GNUC_UNUSED,
  }
  #endif
  
+#if WITH_GETRLIMIT

+static int
+virProcessGetRLimit(int resource,
+struct rlimit *old_limit)
+{
+return getrlimit(resource, old_limit);
+}
+#endif /* WITH_GETRLIMIT */
+
+#if WITH_SETRLIMIT
+static int
+virProcessSetRLimit(int resource,
+const struct rlimit *new_limit)
+{
+return setrlimit(resource, new_limit);
+}
+#endif /* WITH_SETRLIMIT */
+
+#if WITH_GETRLIMIT


Question of preference. I'd rather have these two WITH_GETRLIMIT 
sections joined together [1]



+static int
+virProcessGetLimit(pid_t pid,
+   int resource,
+   struct rlimit *old_limit)
+{
+pid_t current_pid = getpid();
+bool same_process = (pid == current_pid);
+
+if (virProcessPrLimit(pid, resource, NULL, old_limit) == 0)
+return 0;
+
+if (same_process && virProcessGetRLimit(resource, old_limit) == 0)
+return 0;
+
+return -1;
+}
+#endif /* WITH_GETRLIMIT */
+
+#if WITH_SETRLIMIT


1: ... just like these two WITH_SETRLIMIT. But it's matter of taste, I 
agree.



+static int
+virProcessSetLimit(pid_t pid,
+   int resource,
+   const struct rlimit *new_limit)
+{
+pid_t current_pid = getpid();
+bool same_process = (pid == current_pid);
+
+if (virProcessPrLimit(pid, resource, new_limit, NULL) == 0)
+return 0;
+
+if (same_process && virProcessSetRLimit(resource, new_limit) == 0)
+return 0;
+
+return -1;
+}
+#endif /* WITH_SETRLIMIT */
+


Michal



[libvirt PATCH 04/17] util: Introduce virProcess{Get,Set}Limit()

2021-03-05 Thread Andrea Bolognani
These functions abstract part of the existing logic, which is
the same in all virProcessSetMax*() functions, and changes it
so that which underlying syscall is used depends on their
availability rather than on the context in which they are
called: since prlimit() and {g,s}etrlimit() have slightly
different requirements, using the same one every single time
should make for a more consistent experience.

As part of the change, we also remove the special case for
passing zero to virProcessSetMax*() functions: we have removed
all callers that depended on that functionality in the previous
commit, so this is now safe to do and makes the semantics
simpler.

This commit is better viewed with 'git show -w'.

Signed-off-by: Andrea Bolognani 
---
 src/util/virprocess.c | 175 +++---
 1 file changed, 95 insertions(+), 80 deletions(-)

diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index e01ff25540..38b248217e 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -738,10 +738,66 @@ virProcessPrLimit(pid_t pid G_GNUC_UNUSED,
 }
 #endif
 
+#if WITH_GETRLIMIT
+static int
+virProcessGetRLimit(int resource,
+struct rlimit *old_limit)
+{
+return getrlimit(resource, old_limit);
+}
+#endif /* WITH_GETRLIMIT */
+
+#if WITH_SETRLIMIT
+static int
+virProcessSetRLimit(int resource,
+const struct rlimit *new_limit)
+{
+return setrlimit(resource, new_limit);
+}
+#endif /* WITH_SETRLIMIT */
+
+#if WITH_GETRLIMIT
+static int
+virProcessGetLimit(pid_t pid,
+   int resource,
+   struct rlimit *old_limit)
+{
+pid_t current_pid = getpid();
+bool same_process = (pid == current_pid);
+
+if (virProcessPrLimit(pid, resource, NULL, old_limit) == 0)
+return 0;
+
+if (same_process && virProcessGetRLimit(resource, old_limit) == 0)
+return 0;
+
+return -1;
+}
+#endif /* WITH_GETRLIMIT */
+
+#if WITH_SETRLIMIT
+static int
+virProcessSetLimit(pid_t pid,
+   int resource,
+   const struct rlimit *new_limit)
+{
+pid_t current_pid = getpid();
+bool same_process = (pid == current_pid);
+
+if (virProcessPrLimit(pid, resource, new_limit, NULL) == 0)
+return 0;
+
+if (same_process && virProcessSetRLimit(resource, new_limit) == 0)
+return 0;
+
+return -1;
+}
+#endif /* WITH_SETRLIMIT */
+
 #if WITH_SETRLIMIT && defined(RLIMIT_MEMLOCK)
 /**
  * virProcessSetMaxMemLock:
- * @pid: process to be changed (0 for the current process)
+ * @pid: process to be changed
  * @bytes: new limit (0 for no change)
  *
  * Sets a new limit on the amount of locked memory for a process.
@@ -765,21 +821,11 @@ virProcessSetMaxMemLock(pid_t pid, unsigned long long 
bytes)
 else
 rlim.rlim_cur = rlim.rlim_max = RLIM_INFINITY;
 
-if (pid == 0) {
-if (setrlimit(RLIMIT_MEMLOCK, ) < 0) {
-virReportSystemError(errno,
- _("cannot limit locked memory to %llu"),
- bytes);
-return -1;
-}
-} else {
-if (virProcessPrLimit(pid, RLIMIT_MEMLOCK, , NULL) < 0) {
-virReportSystemError(errno,
- _("cannot limit locked memory "
-   "of process %lld to %llu"),
- (long long int)pid, bytes);
-return -1;
-}
+if (virProcessSetLimit(pid, RLIMIT_MEMLOCK, ) < 0) {
+virReportSystemError(errno,
+ _("cannot limit locked memory "
+   "of process %lld to %llu"),
+ (long long int)pid, bytes);
 }
 
 VIR_DEBUG("Locked memory for process %lld limited to %llu bytes",
@@ -802,7 +848,7 @@ virProcessSetMaxMemLock(pid_t pid G_GNUC_UNUSED, unsigned 
long long bytes)
 #if WITH_GETRLIMIT && defined(RLIMIT_MEMLOCK)
 /**
  * virProcessGetMaxMemLock:
- * @pid: process to be queried (0 for the current process)
+ * @pid: process to be queried
  * @bytes: return location for the limit
  *
  * Obtain the current limit on the amount of locked memory for a process.
@@ -818,21 +864,12 @@ virProcessGetMaxMemLock(pid_t pid,
 if (!bytes)
 return 0;
 
-if (pid == 0) {
-if (getrlimit(RLIMIT_MEMLOCK, ) < 0) {
-virReportSystemError(errno,
- "%s",
- _("cannot get locked memory limit"));
-return -1;
-}
-} else {
-if (virProcessPrLimit(pid, RLIMIT_MEMLOCK, NULL, ) < 0) {
-virReportSystemError(errno,
- _("cannot get locked memory limit "
-   "of process %lld"),
- (long long int) pid);
-return -1;
-}
+if (virProcessGetLimit(pid, RLIMIT_MEMLOCK, ) < 0) {
+virReportSystemError(errno,