Patchwork UBI: block: do not use term "attach"

login
register
mail settings
Submitter Artem Bityutskiy
Date March 4, 2014, 10:02 a.m.
Message ID <1393927373-1472-1-git-send-email-dedekind1@gmail.com>
Download mbox | patch
Permalink /patch/326204/
State Superseded
Headers show

Comments

Artem Bityutskiy - March 4, 2014, 10:02 a.m.
From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

We already use term attach/detach for UBI->MTD relations, let's not use this
for UBI->ubiblock relations to avoid confusion. Just use 'create' and 'delete'
instead. E.g., "create a R/O block device on top of a UBI volume".

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 drivers/mtd/ubi/block.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)
Ezequiel Garcia - March 4, 2014, 10:46 a.m.
On Tue, Mar 04, 2014 at 12:02:53PM +0200, Artem Bityutskiy wrote:
> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> 
> We already use term attach/detach for UBI->MTD relations, let's not use this
> for UBI->ubiblock relations to avoid confusion. Just use 'create' and 'delete'

s/delete/remove

> instead. E.g., "create a R/O block device on top of a UBI volume".
> 
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> ---
>  drivers/mtd/ubi/block.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 

Change looks good, except I'd like to change ubiblock_add/ubiblock_del
with ubiblock_create/ubiblock_remove for consistency.

I haven't been too consistent with this terms and it's a good time to
fix it.

Want me to prepare a patch?
Artem Bityutskiy - March 4, 2014, 12:57 p.m.
On Tue, 2014-03-04 at 12:02 +0200, Artem Bityutskiy wrote:
> + * This feature is compiled in the UBI core, and adds a 'block' parameter
> + * to allow early creation of block devices on top of UBI volumes. Runtime
> + * block creation/removal for UBI volumes is provided through two UBI ioctls:
> + * UBI_IOCVOLATTBLK and UBI_IOCVOLDETBLK.
>   */

I think the ioctl names should also be consistent.
s/AT/CR/, s/DET/RM/, may be?
Ezequiel Garcia - March 4, 2014, 1:59 p.m.
On Tue, Mar 04, 2014 at 02:57:38PM +0200, Artem Bityutskiy wrote:
> On Tue, 2014-03-04 at 12:02 +0200, Artem Bityutskiy wrote:
> > + * This feature is compiled in the UBI core, and adds a 'block' parameter
> > + * to allow early creation of block devices on top of UBI volumes. Runtime
> > + * block creation/removal for UBI volumes is provided through two UBI ioctls:
> > + * UBI_IOCVOLATTBLK and UBI_IOCVOLDETBLK.
> >   */
> 
> I think the ioctl names should also be consistent.
> s/AT/CR/, s/DET/RM/, may be?
> 

Ah, yes. Good catch.

Let's write them down, so we can see how they result: UBI_IOCVOLCRBLK,
UBI_IOCVOLRMBLK. Aren't these too unreadable?


How about something along UBI_IOCBLK_CREAT UBI_IOCBLK_RM ?
Artem Bityutskiy - March 4, 2014, 2:13 p.m.
On Tue, 2014-03-04 at 10:59 -0300, Ezequiel Garcia wrote:
> On Tue, Mar 04, 2014 at 02:57:38PM +0200, Artem Bityutskiy wrote:
> > On Tue, 2014-03-04 at 12:02 +0200, Artem Bityutskiy wrote:
> > > + * This feature is compiled in the UBI core, and adds a 'block' parameter
> > > + * to allow early creation of block devices on top of UBI volumes. Runtime
> > > + * block creation/removal for UBI volumes is provided through two UBI ioctls:
> > > + * UBI_IOCVOLATTBLK and UBI_IOCVOLDETBLK.
> > >   */
> > 
> > I think the ioctl names should also be consistent.
> > s/AT/CR/, s/DET/RM/, may be?
> > 
> 
> Ah, yes. Good catch.
> 
> Let's write them down, so we can see how they result: UBI_IOCVOLCRBLK,
> UBI_IOCVOLRMBLK. Aren't these too unreadable?

Dunno, the other ones are similarly unreadable.

> How about something along UBI_IOCBLK_CREAT UBI_IOCBLK_RM ?

Well, would be a little inconsistent, but more readable, yes. Usually
ioctl names to not have underscores, though, no?
Ezequiel Garcia - March 4, 2014, 2:39 p.m.
On Tue, Mar 04, 2014 at 04:13:16PM +0200, Artem Bityutskiy wrote:
> On Tue, 2014-03-04 at 10:59 -0300, Ezequiel Garcia wrote:
> > On Tue, Mar 04, 2014 at 02:57:38PM +0200, Artem Bityutskiy wrote:
> > > On Tue, 2014-03-04 at 12:02 +0200, Artem Bityutskiy wrote:
> > > > + * This feature is compiled in the UBI core, and adds a 'block' parameter
> > > > + * to allow early creation of block devices on top of UBI volumes. Runtime
> > > > + * block creation/removal for UBI volumes is provided through two UBI ioctls:
> > > > + * UBI_IOCVOLATTBLK and UBI_IOCVOLDETBLK.
> > > >   */
> > > 
> > > I think the ioctl names should also be consistent.
> > > s/AT/CR/, s/DET/RM/, may be?
> > > 
> > 
> > Ah, yes. Good catch.
> > 
> > Let's write them down, so we can see how they result: UBI_IOCVOLCRBLK,
> > UBI_IOCVOLRMBLK. Aren't these too unreadable?
> 
> Dunno, the other ones are similarly unreadable.
> 

Yes, indeed.

> > How about something along UBI_IOCBLK_CREAT UBI_IOCBLK_RM ?
> 
> Well, would be a little inconsistent, but more readable, yes.

Indeed, it's less consistent.

> Usually
> ioctl names to not have underscores, though, no?
> 

A quick grep shows some ioctls follow that rule, some don't.
I'd say it's your call.

Patch

diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
index cea7d1c..69955ce 100644
--- a/drivers/mtd/ubi/block.c
+++ b/drivers/mtd/ubi/block.c
@@ -29,10 +29,10 @@ 
  *
  *   LEB number = addressed byte / LEB size
  *
- * This feature is compiled in the UBI core, and adds a new 'block' parameter
- * to allow early block device attaching. Runtime  block attach/detach for UBI
- * volumes is provided through two new UBI ioctls: UBI_IOCVOLATTBLK and
- * UBI_IOCVOLDETBLK.
+ * This feature is compiled in the UBI core, and adds a 'block' parameter
+ * to allow early creation of block devices on top of UBI volumes. Runtime
+ * block creation/removal for UBI volumes is provided through two UBI ioctls:
+ * UBI_IOCVOLATTBLK and UBI_IOCVOLDETBLK.
  */
 
 #include <linux/module.h>
@@ -528,7 +528,7 @@  static int ubiblock_notify(struct notifier_block *nb,
 	switch (notification_type) {
 	case UBI_VOLUME_ADDED:
 		/*
-		 * We want to enforce explicit block device attaching for
+		 * We want to enforce explicit block device creation for
 		 * volumes, so when a volume is added we do nothing.
 		 */
 		break;
@@ -561,7 +561,7 @@  open_volume_desc(const char *name, int ubi_num, int vol_id)
 		return ubi_open_volume(ubi_num, vol_id, UBI_READONLY);
 }
 
-static int __init ubiblock_attach_from_param(void)
+static int __init ubiblock_create_from_param(void)
 {
 	int i, ret;
 	struct ubiblock_param *p;
@@ -592,7 +592,7 @@  static int __init ubiblock_attach_from_param(void)
 	return ret;
 }
 
-static void ubiblock_detach_all(void)
+static void ubiblock_remove_all(void)
 {
 	struct ubiblock *next;
 	struct ubiblock *dev;
@@ -618,13 +618,13 @@  int __init ubiblock_init(void)
 		return ubiblock_major;
 
 	/* Attach block devices from 'block=' module param */
-	ret = ubiblock_attach_from_param();
+	ret = ubiblock_create_from_param();
 	if (ret)
-		goto err_detach;
+		goto err_remove;
 
 	/*
-	 * Block devices needs to be attached to volumes explicitly
-	 * upon user request. So we ignore existing volumes.
+	 * Block devices are only created upon user requests, so we ignore
+	 * existing volumes.
 	 */
 	ret = ubi_register_volume_notifier(&ubiblock_notifier, 1);
 	if (ret)
@@ -633,14 +633,14 @@  int __init ubiblock_init(void)
 
 err_unreg:
 	unregister_blkdev(ubiblock_major, "ubiblock");
-err_detach:
-	ubiblock_detach_all();
+err_remove:
+	ubiblock_remove_all();
 	return ret;
 }
 
 void __exit ubiblock_exit(void)
 {
 	ubi_unregister_volume_notifier(&ubiblock_notifier);
-	ubiblock_detach_all();
+	ubiblock_remove_all();
 	unregister_blkdev(ubiblock_major, "ubiblock");
 }