[libvirt] [PATCH 2/6] process: Add virProcessGetMaxMemLock()

[John Ferlan]

On 12/10/2015 11:33 AM, Andrea Bolognani wrote:
> On Wed, 2015-12-09 at 12:00 -0500, John Ferlan wrote:
>> On 11/24/2015 08:56 AM, Andrea Bolognani wrote:
>>> @@ -788,6 +788,48 @@ virProcessSetMaxMemLock(pid_t pid ATTRIBUTE_UNUSED, unsigned long long bytes)
>>>   }
>>>   #endif /* ! (HAVE_SETRLIMIT && defined(RLIMIT_MEMLOCK)) */
>>>   
>>> +#if HAVE_GETRLIMIT && defined(RLIMIT_MEMLOCK)
>>> +int
>>> +virProcessGetMaxMemLock(pid_t pid,
>>> +                        unsigned long long *bytes)
>>
>> Another option would be to return bytes...
>>
>> where at least as I read patch 3 bytes == 0 is no different than
>> original_memlock == 0 especially if arg2 is NULL
>>
>> Of course, does calling getrlimit or virProcessPrLimit() potentially
>> multiple times make sense? Does blasting those error messages each time
>> to the log, but continuing on cause confusion?
>>
>> I guess what I'm thinking about is how it's eventually used in patch 3
>> and the concept of failure because something in here fails or perhaps
>> the getrlimit *or* prlimit isn't supported on the platform...
> 
> I think ignoring failures in getting the current memory locking limit is
> the right thing to do in the upper layers, as that just means we won't
> be able to restore the original limit afterwards and that's not really a
> huge problem.
> 
> However, at this level, we're dealing with low-level functionality and I
> think all failures should be reported appropriately, eg. with a negative
> return value and a proper error logged.
> 
>>> +{
>>> +    struct rlimit rlim;
>>> +
>>> +    if (!bytes)
>>> +        return 0;
>>
>> Since you return 0 here if passed a NULL bytes, then I think you'd have
>> to do so in the other API
> 
> What other API do you mean?
> 

in the code after this:

+#else /* ! (HAVE_GETRLIMIT && defined(RLIMIT_MEMLOCK)) */
+int
+virProcessGetMaxMemLock(pid_t pid ATTRIBUTE_UNUSED,
+                        unsigned long long *bytes)


>>> +
>>> +    if (pid == 0) {
>>> +        if (getrlimit(RLIMIT_MEMLOCK, &rlim) < 0) {
>>> +            virReportSystemError(errno,
>>> +                                 "%s",
>>
>> This one could go on previous line - no big deal other than extra line
> 
> I'd rather squeeze the second and third lines together or leave it as
> it is, if that's the same to you.
> 

It really doesn't matter - just seems strange to have "%s", on its own
line especially when there's space on the prior line.

>>> +                                 _("cannot get locked memory limit"));
>>> +            return -1;
>>> +        }
>>> +    } else {
>>> +        if (virProcessPrLimit(pid, RLIMIT_MEMLOCK, NULL, &rlim) < 0) {
>>> +            virReportSystemError(errno,
>>> +                                 _("cannot get locked memory limit "
>>> +                                   "of process %lld"),
>>> +                                 (long long int) pid);
>>> +            return -1;
>>> +        }
>>> +    }
>>> +
>>> +    /* virProcessSetMaxMemLock() sets both rlim_cur and rlim_max to the
>>> +     * same value, so we can retrieve just rlim_max here */
>>> +    *bytes = rlim.rlim_max;
>>
>> One oddball thought... what if rlim.rlim_max == 0?  Is that possible? I
>> suppose only if rlim_cur == 0 too, right?  Not sure it makes sense and I
>> didn't chase the syscall. It does say rlim_cur can be 0 and that
>> rlim_max must be equal or higher...
>>
>> I'm just trying to logically follow through on the thought of how 0 is
>> used in patch 3.
> 
> I don't know, but I don't think we should concern ourself too much with
> that case as virProcessSetMaxMemLock(), which will be used to restore
> whatever value this function returns, ignores the value 0.
> 

Fair enough...

>>> +
>>> +    return 0;
>>> +}
>>> +#else /* ! (HAVE_GETRLIMIT && defined(RLIMIT_MEMLOCK)) */
>>> +int
>>> +virProcessGetMaxMemLock(pid_t pid ATTRIBUTE_UNUSED,
>>> +                        unsigned long long *bytes)  
>>
>> Would technically be unused if kept as is... However since the other API
>> returns 0 when value is passed as NULL, this could too.
> 
> Having a function that either fails as unimplemented or succeeds
> depending on its arguments doesn't feel right to me.
> 
> I see, however, that all virProcessSetMax*() functions behave this way
> already so it makes sense to change do the same for consistency's sake.
> 
>
Who's to say which is the right way...  But since we declare using 0
does nothing, then playing following the leader shouldn't hurt in this case.

John