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

[Benny Halevy]

On Jan. 13, 2008, 0:01 +0200, Dean Hildebrand <seattleplus at gmail.com> wrote:
> 
> 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

More than a matter of temporal taste or mere preference, I try to follow
Documentation/SubmittingPatches:

	3) Separate your changes.

	Separate _logical changes_ into a single patch file.

	For example, if your changes include both bug fixes and performance
	enhancements for a single driver, separate those changes into two
	or more patches.  If your changes include an API update, and a new
	driver which uses that new API, separate those into two patches.

	On the other hand, if you make a single change to numerous files,
	group those changes into a single patch.  Thus a single logical change
	is contained within a single patch.

Breaking one logical change into smaller pieces just because it appears
too big is bad.  However, if that can be done by distilling it into several
smaller, logical, _and self-sufficient_ patches then go ahead with it.
This is certainly the way to go.

Benny

> 
>> 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 */
>>>>
>>>>
>>>>   
> _______________________________________________
> pNFS mailing list
> pNFS at linux-nfs.org
> http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
>