[bug#41143] [PATCH 1/2] mapped-devices: Allow target to be list of strings

[Mikhail Tsykalov <tsymsh@gmail.com>]

Hi, Ludovic

On 25.09.2020 12:34, Ludovic Courtès wrote:
> Hi Mikhail,
>
> Mikhail Tsykalov <tsymsh@gmail.com> skribis:
>
>> On 09.09.2020 23:38, Ludovic Courtès wrote:
>>>> diff --git a/gnu/services/base.scm b/gnu/services/base.scm
>>>> index 0c154d1c4e..3d09e8220c 100644
>>>> --- a/gnu/services/base.scm
>>>> +++ b/gnu/services/base.scm
>>>> @@ -408,7 +408,10 @@ FILE-SYSTEM."
>>>>    (define (mapped-device->shepherd-service-name md)
>>>>      "Return the symbol that denotes the shepherd service of MD, a
>>>> <mapped-device>."
>>>>      (symbol-append 'device-mapping-
>>>> -                 (string->symbol (mapped-device-target md))))
>>>> +                 (string->symbol (string-join
>>>> +                                  (let ((t (mapped-device-target md)))
>>>> +                                    (if (list? t) t (list t)))
>>>> +                                  "-"))))
>>> To avoid duplicating the (if (list? t) …) everywhere, I propose instead
>>> the following approach:
>>>
>>>     1. Rename ‘target’ to ‘targets’ (plural) and likewise for the
>>>        accessor, and agree that it always contains a list;
>>>
>>>     2. Rename ‘mapped-device’ to ‘%mapped-device’ and add a
>>>        ‘mapped-device’ backward-compatibility macro that allows for a
>>>        ‘target’ (singular) field and automatically turns its value into a
>>>        list.  See the ‘origin’ macro in (guix packages) for an example of
>>>        how to do that (that macro allows users to specify ‘sha256’ instead
>>>        of ‘hash’).
>>>
>>>     3. Add a deprecated ‘mapped-device-target’ (singular) that returns the
>>>        first element returned by ‘mapped-device-targets’.
>> While this looks like a good idea, doesn't this break code that
>> implements mapped-device and assumes that target is a string. Suddenly
>> passing a string to a mapped-device constructor results in a list
>> passed to open/close. Also, what functions should do if they expect a
>> string but get a list of them? Ignore everything but the first item?
>> Implement mandatory check function? Doesn't this change push
>> complexity out of mapped-device to implementations of it.
> The intent of what I propose above is (1) to not break existing code,
> and (2) to avoid duplicating checks and conversions at every call site.
>
> #1 is achieved by providing a deprecated ‘mapped-device-target’
> (singular) procedure, for example.
>
> Does that make sense?

I'm sorry if I didn't make myself clear, but it doesn't seem like 
open/close functions even use any mapped-device-* procedures, they just 
get passed source and target field directly. What I meant was this 
change will require changes to luks-device-mapping, raid-device-mapping 
and all other device mappings that users may have implemented in their 
local trees/config.

To be fair, after thinking about it for a bit, I think that this issue 
can be solved by renaming mapped-device-kind and providing compatibility 
macros similar to %mapped-device. Still question remains about what 
should we do if a list gets passed to a kind that doesn't expect it, but 
I think we can just raise an error in macro if that's the case. Does 
this sound fine to you?

Thanks,
Mikhail