[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#61744] [PATCH v2 1/2] services: base: Deprecate 'pam-limits-service
From: |
Bruno Victal |
Subject: |
[bug#61744] [PATCH v2 1/2] services: base: Deprecate 'pam-limits-service' procedure. |
Date: |
Sat, 11 Mar 2023 11:25:13 +0000 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 |
Hi Ricardo,
On 2023-03-10 17:52, Ricardo Wurmus wrote:
> Hi,
>
> thank you for the patches!
>
> The effective change looks fine to me, but I’m confused about why these
> are two patches. The first one introduces this as an example in the
> docs:
[...]
>
> +(service
> + pam-limits-service-type
> + (plain-file
> + "limits.conf"
> + (string-join
> + (map pam-limits-entry->string
> + (list (pam-limits-entry "@@realtime" 'both 'rtprio 99)
> + (pam-limits-entry "@@realtime" 'both 'memlock 'unlimited)))
> + "\n")))
>
> But the second removes this again in favour of this prettier form:
This was to ensure that each commit is "atomic".
>
> +(service pam-limits-service-type
> + (list
> + (pam-limits-entry "@@realtime" 'both 'rtprio 99)
> + (pam-limits-entry "@@realtime" 'both 'memlock 'unlimited)))
>
> Which is really close to the original form:
>
> -(pam-limits-service
> - (list
> - (pam-limits-entry "@@realtime" 'both 'rtprio 99)
> - (pam-limits-entry "@@realtime" 'both 'memlock 'unlimited)))
>
> Could you merge these two patches to reduce the number of unnecessary
> changes? I don’t think we should change to file-likes as the argument
> value for the pam-limits-service-type.
The v2 patch-series are a dis-aggregation of the v1 series (save for a bug fix
in the match clauses, test suite and using raise instead of report-error)
as indicated in the 10/27 patch-series review from #61789.
>
> Another thing that confused me:
>
> + (test-equal "/etc/security/limits.conf content matches"
> + #$(string-join (map pam-limits-entry->string pam-limit-entries)
> + "\n" 'suffix)
> + (marionette-eval
> + '(call-with-input-file "/etc/security/limits.conf"
> + get-string-all)
> + marionette))
>
> Why use the gexp with a computed value here instead of using just the
> plain text of the expected contents of that file? Computing
> the expected value in a test where the compared value is computed in the
> same way feels like begging the question.
>
> Or perhaps I’m misunderstanding something here?
>
I wrote this test suite to simply check that both deprecated and "new"
service-type forms
work correctly, i.e. the files are present in their locations.
(this actually caught a bug within the match clauses in the v1 patch)
Cheers,
Bruno