[pnfs] [PATCH 03/13] pnfsd: Simplify getdeviceinfo export ops.

[Dean Hildebrand]

Benny Halevy wrote:
> Dean Hildebrand wrote:
>>
>>
>> Benny Halevy wrote:
>>> Dean, this patch would break compilation of the tree
>>> so it makes the patchset non-bisectable.  The main problem
>>> with that is if a regression is found we can't use git-bisect
>>> to semi-automatically find the offending patch in O(log(#patches)).
>>>   
>> I agree, but I'm not sure how.  That patch changes a data structure 
>> that is used in a lot of different places.  Should I merge all 
>> patches together that are affected by the change to this data 
>> structure? (I think that is almost all of my patches)
> I'm not sure what is the best approach in this case, but in general,
> off the top of my head we can use several techniques:
>
> 1. make the change atomic, i.e. make the change in data structures and 
> in the code using them
> in the same patch.
ok, I guess these days bisection is more important than small patches, 
so I'll go with option 1.
Dean

>
> 2. If that is too complicated, consider making a serious of patches 
> that implements the above
> in layers, each atomically changing part of the data structure and its 
> usage.
>
> 3. If the approaches above still don't work, the new data structures 
> can be given new names
> and their usage can be implemented gradually, e.g. one function in a 
> patch.
> Once everything got converted the old data structures can be deleted, 
> and if really wanted
> the new structures can then be renamed to the old names (in an atomic 
> change).
> Each patch in this series should at least compile and ideally run 
> correctly.
> If there's a correctness issue while the new API is introduced you can 
> introduce the
> new functions with new names, without removing the old code yet (and I 
> think it makes sense
> in this case because the change in functionality is through enough 
> that the diff in the patches
> that you sent that show the new code with the existing code in a 
> merged view are almost useless
> as they don't help understanding what you did but rather make it 
> harder...)
> and then when the new API is fully implemented, change the layer that 
> uses it and remove
> the old implementation and interface.
>
> Benny
>
>> Dean
>>> Also, minor nit below.
>>>
>>> Thanks for sending the patches!
>>>
>>> Benny
>>>
>>> On Jan. 10, 2008, 23:38 +0200, seattleplus at gmail.com wrote:
>>> <snip>
>>>
>>>  
>>>> @@ -35,6 +35,35 @@ enum fid_type {
>>>>      FILEID_INO32_GEN_PARENT = 2,
>>>>  };
>>>>  
>>>> +#if defined(CONFIG_PNFS)
>>>> +
>>>> +/* XDR stream arguments and results.  Exported file system uses this
>>>> + * struct to encode information and return how many bytes were 
>>>> encoded.
>>>> + */
>>>> +struct pnfs_xdr_info {
>>>> +    u32 *p;            /* in */
>>>> +    u32 *end;        /* in */
>>>> +    u32 maxcount;        /* in */
>>>> +    u32 bytes_written;    /* out */
>>>> +};
>>>> +
>>>> +/* Used by get_device_info to encode a device (da_addr_body in spec)
>>>> + * Args:
>>>> + * xdr - xdr stream
>>>> + * device - pointer to device to be encoded
>>>> +*/
>>>> +typedef int (*pnfs_encodedev_t)(struct pnfs_xdr_info *xdr, void 
>>>> *device);
>>>> +
>>>> +/* Arguments for get_device_info */
>>>> +struct pnfs_devinfo_arg {
>>>> +    u32 type;
>>>> +    u32 devid;
>>>> +    struct pnfs_xdr_info xdr;
>>>> +    pnfs_encodedev_t func;
>>>> +};
>>>> +
>>>> +#endif
>>>>     
>>>
>>> please add comment after #endif, like this:
>>> #endif /* CONFIG_PNFS */
>>>
>>>
>>>   
>