GNU bug report logs - #41143
[PATCH 1/2] Add 'lvm-device-mapping'

Previous Next

Full log

Message #40 received at 41143 <at> debbugs.gnu.org (full text, mbox):

From: Mikhail Tsykalov <tsymsh <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 41143 <at> debbugs.gnu.org
Subject: Re: [bug#41143] [PATCH 1/2] mapped-devices: Allow target to be list
 of strings
Date: Fri, 25 Sep 2020 16:36:11 +0300

Hi, Ludovic

On 25.09.2020 12:34, Ludovic Courtès wrote:
> Hi Mikhail,
>
> Mikhail Tsykalov <tsymsh <at> 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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.