On 03/19/2018 09:56 AM, Tony Asleson wrote:
> I was wondering if this discussion (and ones like it) should be done on
> the public mailing list? To encourage community involvement.
Yes please, in the future. For now, I'm CCing the list here, so our
throngs of theoretical list subscribers :-) could catch up by reading
the quoted conversation below.
Thanks -- Andy
>
>
> On 03/19/2018 11:02 AM, Tony Asleson wrote:
>> My preference is what you refer to as class II, with placing much of the
>> base functionality into devicemapper.rs and limiting or removing dm
>> reference passing to functions and methods.
>>
>> Regards,
>> -Tony
>>
>>
>> On 03/16/2018 04:10 PM, Anne Mulhern wrote:
>>> Hi!
>>>
>>> I have identified several options. Do you have a preference among them,
>>> or have you another option in mind? Why?
>>>
>>> The options fall into two classes:
>>>
>>> I. Pass the DM context around as a parameter.
>>> II. Instantiate it once and use a method to get a reference to the parameter where necessary.
>>>
>>> For (I) the question is where to instantiate the context. The two consistent options are
>>> A. At the outermost level
>>> B. Exactly where needed.
>>>
>>> A drawback of (A) is that it requires a bit of maintenance; a merit is that it is
>>> informative, since the caller knows if the context is needed somewhere by any of the
>>> method's callees. (A) is implemented here: https://github.com/stratis-storage/stratisd/pull/835.
>>> It was mostly already done, this is the last little bit.
>>>
>>> A drawback of (B) is that it makes failure getting the context need to be handled wherever
>>> the context is obtained. Mostly, this is fairly benign, since a method that needs a DM
>>> context probably has a lot of other ways to fail, too, so usually the code should be as
>>> simple as a "?". Another drawback is that it is an action that is repeated unnecessarily.
>>> A merit is that fewer arguments are passed in many methods.
>>>
>>> For (II) there are three tasks:
>>> 1. Write the necessary code that memoizes the DM after the first call and returns a reference to the DM.
>>> This method is called get_dm_context. This is a bit tricky, there is an unsafe block.
>>> 2. Write the code that calls get_dm_context() and correctly handles the possible error that may occur the first
>>> time get_dm_context() is called (using catch_unwind, which is probably a bit risky, kind of like unsafe).
>>> 3. Alter the other code accordingly (this is rather broad)
>>>
>>> and three possible options:
>>> A. Do (1), (2), and (3) in stratisd. In this case (3) consists of calling get_dm_context() whenever it
>>> is needed to pass to a devicemapper-rs method, e.g., LinearDev::setup() and eliminating where
>>> the DM context is instantiated and passed down to callees. See https://github.com/stratis-storage/stratisd/pull/841
>>> for a partial implementation of this approach.
>>> B. Do (1) in devicemapper-rs and (2) and (3) in stratisd. See https://github.com/stratis-storage/devicemapper-rs/pull/286
>>> for (1) and https://github.com/stratis-storage/stratisd/pull/843 for (2) and partial implementation of (3).
>>> C. Do (1) in devicemapper-rs, (2) in stratisd, and (3) in both. For (3) in devicemapper-rs, eliminate all &DM parameters
>>> in all methods. See https://github.com/stratis-storage/devicemapper-rs/pull/288 for rough devicemapper-rs implementation
>>> of this part. In stratisd, call all devicemapper-rs methods w/out passing any DM parameter, since the methods no longer
>>> require any such. Do not pass DM parameter anywhere. This part has not been implemented, but you get the idea.
>>>
>>> I tend to prefer (II), because it changes the places to fail from many to one, at least in theory.
>>>
>>> - mulhern
>>>
>>
>
