2014-06-17 14:28:18

by Jeff Liu

[permalink] [raw]
Subject: [patch 00/24] lib/kobject: kset_create_and_add return error clean up

Hello,

Currently, to verify if kset_create_and_add() is succeed or not, almost
all subsystems with sysfs support are check up the return value against
NULL, then return -ENOMEM on failure, since kset_create_and_add() always
return NULL in case of anything wrong. However, kset_register() can fail
due to other reasons, hence it's better to return the actual error on
kset_create_and_add(), this patch series is just did that and this is
inspired by Christoph in another thread:
http://www.spinics.net/lists/linux-mm/msg74729.html


Jeff Liu (24)
kobject: return actual error on kset_create_and_add
sample/kobject: update kset-example code
slub: return actual error on sysfs_slab_add
powerpc: check/return acutal error on sysfs init
s390: check/return actual error on sysfs init
crypt: return actual error code on pcrypt_init
kernel: return actual error on param_sysfs_init
drivers/firmware: return actual error on sysfs init
drivers/iommu: check actual error on iommu_init
drivers/base: return actual error on sysfs init
drviers/scsi: check sysfs init return value via IS_ERR at iscsi_boot_create_kset
drivers/parisc: return actual error on pdc_stable_init
drivers/pci: return actual error on pci_slot_init
drivers/s390: return actual error on sysfs init functions
drivers/of: return actual error on of_init
btrfs: return actual error on btrfs_init_sysfs
gfs2: return actual error on gfs2_sys_init
exofs: return actual error on exofs_sysfs_odev_add
fs/dlm: return actual error on dlm_lockspace_init
ext4: return actual error on ext4_init_fs
f2fs: return actual error on init_f2fs_fs
ocfs2: return actual error on sysfs init
net: return actual error on register_queue_kobjects
security/smack: return acutal error on smk_init_sysfs


Cheers,
-Jeff


2014-06-17 15:15:39

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [patch 00/24] lib/kobject: kset_create_and_add return error clean up

On Tue, Jun 17, 2014 at 10:25:30PM +0800, Jeff Liu wrote:
> Hello,
>
> Currently, to verify if kset_create_and_add() is succeed or not, almost
> all subsystems with sysfs support are check up the return value against
> NULL, then return -ENOMEM on failure, since kset_create_and_add() always
> return NULL in case of anything wrong. However, kset_register() can fail
> due to other reasons, hence it's better to return the actual error on
> kset_create_and_add(), this patch series is just did that and this is
> inspired by Christoph in another thread:
> http://www.spinics.net/lists/linux-mm/msg74729.html

Number one, please, please, please use git send-email with
sendmail.chainreplyto set to true in your .gitconfig (which is the
default) or use --chain-reply-to. Without reply chaining, it's
incredibly painful to find related patches, and these patches are very
clearly related.

Number two, for changes like this, my strong recommendation is to have
a single large patch that makes all of the change at once. Breaking
up the commits into each individual patch just adds a lot of noise
into the system, and it also invites potential problems when people
find a single patch out of context, and might accidentally backport
the patch (which as described, sounds like it's a bug fix) without
realizing that it's actively harmful until you apply the first patch
in this patch series.

In additoin, when you make an API change like this, and then spread
out the changes across two dozen patches, it also breaks
bisectability, although in this case the chance that
kset_create_and_add would fail is relatively small. It's the
principle of the thing, though.

For the ext4 portion of this patch set (which I think should be a
single patch):

Acked-by: Theodore Ts'o <[email protected]>

- Ted