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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list