[bug#61744] [PATCH v2 1/2] services: base: Deprecate 'pam-limits-service' procedure.

[Bruno Victal]

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