[dm-devel] Re: [PATCH 3/7] dm/md dependency tree in sysfs: bd_claim_by_kobject

[Jun'ichi Nomura]

Hi Andrew,

Thank you for the review and comments.

Attached patch reflects your comments.
The patch also reflects the other comment regarding uninlining
bd_claim_by_disk() and bd_release_from_disk().

Could you please replace
dm-md-dependency-tree-in-sysfs-bd_claim_by_kobject.patch with this
and drop dm-md-dependency-tree-in-sysfs-bd_claim_by_disk.patch?
Or am I better to send a patch relative to the previous ones?


Andrew Morton wrote:
> Bear in mind that sysfs is Kconfigurable (CONFIG_SYSFS).  If possible,
> newly-added code which doesn't make sense without sysfs should not appear
> in a CONFIG_SYSFS=n vmlinux.

OK, fixed.
CONFIG_SYSFS=n built ok and confirmed no symbols related to
my sysfs symlinking exist in it.

>>+static inline struct kobject * bdev_get_kobj(struct block_device *bdev)
> 
> pet-peeve: The space after asterisk has no use.

Fixed.

>>+static inline struct kobject * bdev_get_holder(struct block_device *bdev)
>>+static inline void add_symlink(struct kobject *from, struct kobject *to)
>>+static inline void del_symlink(struct kobject *from, struct kobject *to)
>>+static inline int bd_holder_grab_dirs(struct block_device *bdev,
>>+static inline void bd_holder_release_dirs(struct bd_holder *bo)
> 
> I suposse the inlines are OK, given that we "know" that there's only a
> single callsite.  But recent gcc's take care of that automatically.

I removed 'inline's from them and let it cared by gcc.

>>+static int add_bd_holder(struct block_device *bdev, struct bd_holder *bo)
>>+{
>>+        struct bd_holder *tmp;
>>+
>>+	if (!bo)
>>+		return 0;
> 
> whitespace went wild there.

Fixed. Thank you.

> Some comments which describe what these do, what they return and why they
> return it would be nice.

Comments are added for non-trivial functions and a structure.

>>+}
>>+
>>+EXPORT_SYMBOL(bd_claim_by_kobject);
> 
> I don't think the blank line before the EXPORT_SYMBOL() does anything useful.

Sure. They just followed the other exports in the file.
Fixed.

> EXPORT_SYMBOL_GPL() might be preferred.

I changed them to EXPORT_SYMBOL_GPL().

Thanks,
-- 
Jun'ichi Nomura, NEC Solutions (America), Inc.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 03-bd_claim_by_kobj.patch
Type: text/x-patch
Size: 9545 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/dm-devel/attachments/20060314/34f5ae9f/attachment.bin>