Re: [PATCH GIT] drivers/block/ub.c - misc. cleanup/indentation, removed unneeded return

Re: [PATCH GIT] drivers/block/ub.c - misc. cleanup/indentation, removed unneeded return

[Pete Zaitcev]

On Wed, 8 Feb 2006 10:29:15 +0100, Marc Koschewski <marc@osknowledge.org> wrote:

> I created this little patch while reading through drivers/block/ub.c. It fixes
> some indentation/whitespace as well as some empty commenting, an hardcoded
> module name and an unneeded return.

I strongly disagree.

The only segment which has some merit is the one which replaces the .name
with DRV_NAME. It could have been "usb-block" or something, but we probably
won't be using that, so it's all right.

But the rest is quite bad, the worst being indenting the switch statement.

Is there nothing else you can do in the whole kernel?

-- Pete

Re: [PATCH GIT] drivers/block/ub.c - misc. cleanup/indentation, removed unneeded return

[Marc Koschewski]

* Pete Zaitcev <zaitcev@redhat.com> [2006-02-08 19:40:57 -0800]:

> On Wed, 8 Feb 2006 10:29:15 +0100, Marc Koschewski <marc@osknowledge.org> wrote:
> 
> > I created this little patch while reading through drivers/block/ub.c. It fixes
> > some indentation/whitespace as well as some empty commenting, an hardcoded
> > module name and an unneeded return.
> 
> I strongly disagree.

OK.

> 
> The only segment which has some merit is the one which replaces the .name
> with DRV_NAME. It could have been "usb-block" or something, but we probably
> won't be using that, so it's all right.

I just dislike using the same values twice and not using constants. More of a
personal thing I guess.

> 
> But the rest is quite bad, the worst being indenting the switch statement.

I see. Why do you use if-statement-indenting then?

What is the sense of the empty comments? What sense does the 'immediate
return' make when the value is returned 2 lines below (where one line is just
a closing brace)?

> 
> Is there nothing else you can do in the whole kernel?

Sure, but I guess you don't have to tell me what files I put my nose in, do
you? I didn't mean to personally piss you off by sending this in. Tzzz ...

Unfortunately my understanding of how ie. the Linux VM works is not very good.
In fact it's poor. But I will dig into this and try to make even you happy with
a patch that makes sense _in your eyes_.

> 
> -- Pete
> 

Marc

Re: [PATCH GIT] drivers/block/ub.c - misc. cleanup/indentation, removed unneeded return

[Pete Zaitcev]

On Thu, 9 Feb 2006 10:21:43 +0100, Marc Koschewski <marc@osknowledge.org> wrote:

> > But the rest is quite bad, the worst being indenting the switch statement.
> 
> I see. Why do you use if-statement-indenting then?

Insides of case blocks are indented, didn't you notice? Why waste a tab?

> What is the sense of the empty comments? What sense does the 'immediate
> return' make when the value is returned 2 lines below (where one line is just
> a closing brace)?

Empty comments segregate related groups of functions.

The return is there in case something else is needed between the if and
the function's end. It's there to underscore the regular structure of the
code. Though the big block comment obscures the intent there. Perhaps
I should relocate it.

You didn't mention empty lines on top of functions. They appear where
variables are likely to be, or if the function body has a break, to make
it fair to all little segments :-)

> > Is there nothing else you can do in the whole kernel?
> 
> Sure, but I guess you don't have to tell me what files I put my nose in, do
> you? I didn't mean to personally piss you off by sending this in. Tzzz ...
> 
> Unfortunately my understanding of how ie. the Linux VM works is not very good.
> In fact it's poor. But I will dig into this and try to make even you happy with
> a patch that makes sense _in your eyes_.

I don't know anything about VM either and I do not see how this is
relevant.

The bigger point is, if you strive to make a meaningful contribution,
meaningless code rearrangements is not the way to go about it. And
we have plenty of work under the subsystem level, though usually it
has something to do with specific hardware. For example, Firewire
appears to be in dire need of fixing right now.

I remember a period of time when the janitoring project produced
meaningful cleanups, such as verification of failure paths. This work
is still relevant. Just the other day I sent a patch to Dmitry for
a leak in mousedev. And remember the debacle of memset(p, size, 0)!
I'm not going to tell you where to put your nose in, but I would like
to hint that the janitoring project might use some help with something
going beyond the uglification of my pretty code :-)

-- Pete

Re: [PATCH GIT] drivers/block/ub.c - misc. cleanup/indentation, removed unneeded return

[Jan Engelhardt]

>> I created this little patch while reading through drivers/block/ub.c. It fixes
>> some indentation/whitespace as well as some empty commenting, an hardcoded
>> module name and an unneeded return.
>
>I strongly disagree.
>
>But the rest is quite bad, the worst being indenting the switch statement.
>
switch(a) {
case A:
    dosomething;
    break;
}

Now what if

switch(a) {
case A: {
    int tmp;
    do_something;
    break;
}
}


Jan Engelhardt
-- 

Re: [PATCH GIT] drivers/block/ub.c - misc. cleanup/indentation, removed unneeded return

[Marc Koschewski]

* Pete Zaitcev <zaitcev@redhat.com> [2006-02-09 08:55:01 -0800]:

> On Thu, 9 Feb 2006 10:21:43 +0100, Marc Koschewski <marc@osknowledge.org> wrote:
> 
> > > But the rest is quite bad, the worst being indenting the switch statement.
> > 
> > I see. Why do you use if-statement-indenting then?
> 
> Insides of case blocks are indented, didn't you notice? Why waste a tab?

The 'case' block are, however, _within_ the switch, thus they need to be
indented - just my 2 cents. But OK ...

> 
> > What is the sense of the empty comments? What sense does the 'immediate
> > return' make when the value is returned 2 lines below (where one line is just
> > a closing brace)?
> 
> Empty comments segregate related groups of functions.
>

Didn't know. Is there any documentation on this _really_ helpful mechanism?

> 
> The return is there in case something else is needed between the if and

OK, so why is _any_

	if (1)
		get_lost;

stuff left there without the braces? I guess one day there _could_ another
line to lead to 

	if (1) {
		beat_it;
		get_lost;
	}

where the braces are definitely missing. You could also have pinned the return
there when it was needed. And for now it's definitely not. Maybe for readability.
But that's another thing...

> the function's end. It's there to underscore the regular structure of the
> code. Though the big block comment obscures the intent there. Perhaps
> I should relocate it.

.. to be discussed right here. You're snippet is not _more readable_ due to the
return. Any when it comes to such things as 'return the rc right now as we're
setup and ready' I could point out some other locations where the rc is set, the
code did its setup for ie. a device and many if-statements follow but no change
to rc is made. Why not use 'immediate return' there?

Maybe on could clarify for me.

> 
> You didn't mention empty lines on top of functions. They appear where
> variables are likely to be, or if the function body has a break, to make
> it fair to all little segments :-)

I did understand the fact that one line-breaks the function declaration. But I
don't understand the need to put a blank line where someday a var could
defined.

I could put another 60 blank lines somewhere just in case some vendor brings any
device that needs some special treatment on init or something.

> 
> > > Is there nothing else you can do in the whole kernel?
> > 
> > Sure, but I guess you don't have to tell me what files I put my nose in, do
> > you? I didn't mean to personally piss you off by sending this in. Tzzz ...
> > 
> > Unfortunately my understanding of how ie. the Linux VM works is not very good.
> > In fact it's poor. But I will dig into this and try to make even you happy with
> > a patch that makes sense _in your eyes_.
> 
> I don't know anything about VM either and I do not see how this is
> relevant.

It was just an example. To me it looks like some sorta 'this is my code and you
please let your finger off it'-thing. I do understand that whitespace/indenting-only
patches are just about to get us in trouble when it comes to other patches
applied to the tree as ie. Andi Kleen stated in 'Re: [PATCH] early_printk:
cleanup trailiing whitespace' today.

The remove-return-thing, however, was not a whitespace thing. I still disagree
it makes the code more readable or whatever. It's just redundant as this time.
When there's a need, we could add it.

> 
> The bigger point is, if you strive to make a meaningful contribution,
> meaningless code rearrangements is not the way to go about it. And
> we have plenty of work under the subsystem level, though usually it
> has something to do with specific hardware. For example, Firewire
> appears to be in dire need of fixing right now.

OK, let's face it: I don't have any FireWire hardware around. Not even a cable.
So I'm not gonna do anyting there.

I just stumbled upon the ub.c code on my way to get my USB chipcard reader module
from crashing when it's unloaded. The patch just exists because I read the file
and wondered what all these comments should 'comment' if now comment was
actually in there.

> 
> I remember a period of time when the janitoring project produced
> meaningful cleanups, such as verification of failure paths. This work
> is still relevant. Just the other day I sent a patch to Dmitry for
> a leak in mousedev. And remember the debacle of memset(p, size, 0)!
> I'm not going to tell you where to put your nose in, but I would like
> to hint that the janitoring project might use some help with something
> going beyond the uglification of my pretty code :-)

Could you point me to some location?

> 
> -- Pete
> 

Marc

Re: [PATCH GIT] drivers/block/ub.c - misc. cleanup/indentation, removed unneeded return

[Pete Zaitcev]

On Thu, 9 Feb 2006 18:24:20 +0100 (MET), Jan Engelhardt <jengelh@linux01.gwdg.de> wrote:

> Now what if
> 
> switch(a) {
> case A: {
>     int tmp;
>     do_something;
>     break;
> }
> }

Not in _my_ code.

-- Pete

[PATCH GIT] drivers/block/ub.c - misc. cleanup/indentation, removed unneeded return

[Marc Koschewski]

[-- Attachment #1: Type: text/plain, Size: 252 bytes --]

Hey hackers. ;)

I created this little patch while reading through drivers/block/ub.c. It fixes
some indentation/whitespace as well as some empty commenting, an hardcoded
module name and an unneeded return.

The patch is against the latest -git.

Marc

[-- Attachment #2: ub.c.patch-git --]
[-- Type: text/plain, Size: 6819 bytes --]

--- ub.c.orig	2006-02-07 23:35:32.000000000 +0100
+++ ub.c	2006-02-07 23:53:56.000000000 +0100
@@ -33,10 +33,10 @@
 #include <linux/timer.h>
 #include <scsi/scsi.h>
 
-#define DRV_NAME "ub"
-#define DEVFS_NAME DRV_NAME
+#define DRV_NAME		"ub"
+#define DEVFS_NAME		DRV_NAME
 
-#define UB_MAJOR 180
+#define UB_MAJOR		180
 
 /*
  * The command state machine is the key model for understanding of this driver.
@@ -109,19 +109,13 @@
  * This many LUNs per USB device.
  * Every one of them takes a host, see UB_MAX_HOSTS.
  */
-#define UB_MAX_LUNS   9
+#define UB_MAX_LUNS		9
 
-/*
- */
-
-#define UB_PARTS_PER_LUN      8
+#define UB_PARTS_PER_LUN	8
 
-#define UB_MAX_CDB_SIZE      16		/* Corresponds to Bulk */
+#define UB_MAX_CDB_SIZE		16	/* Corresponds to Bulk */
 
-#define UB_SENSE_SIZE  18
-
-/*
- */
+#define UB_SENSE_SIZE		18
 
 /* command block wrapper */
 struct bulk_cb_wrap {
@@ -161,28 +155,28 @@
  */
 struct ub_dev;
 
-#define UB_MAX_REQ_SG	9	/* cdrecord requires 32KB and maybe a header */
-#define UB_MAX_SECTORS 64
+#define UB_MAX_REQ_SG		9	/* cdrecord requires 32KB and maybe a header */
+#define UB_MAX_SECTORS		64
 
 /*
  * A second is more than enough for a 32K transfer (UB_MAX_SECTORS)
  * even if a webcam hogs the bus, but some devices need time to spin up.
  */
-#define UB_URB_TIMEOUT	(HZ*2)
-#define UB_DATA_TIMEOUT	(HZ*5)	/* ZIP does spin-ups in the data phase */
-#define UB_STAT_TIMEOUT	(HZ*5)	/* Same spinups and eject for a dataless cmd. */
-#define UB_CTRL_TIMEOUT	(HZ/2)	/* 500ms ought to be enough to clear a stall */
+#define UB_URB_TIMEOUT		(HZ*2)
+#define UB_DATA_TIMEOUT		(HZ*5)	/* ZIP does spin-ups in the data phase */
+#define UB_STAT_TIMEOUT		(HZ*5)	/* Same spinups and eject for a dataless cmd. */
+#define UB_CTRL_TIMEOUT		(HZ/2)	/* 500ms ought to be enough to clear a stall */
 
 /*
  * An instance of a SCSI command in transit.
  */
-#define UB_DIR_NONE	0
-#define UB_DIR_READ	1
-#define UB_DIR_ILLEGAL2	2
-#define UB_DIR_WRITE	3
+#define UB_DIR_NONE		0
+#define UB_DIR_READ		1
+#define UB_DIR_ILLEGAL2		2
+#define UB_DIR_WRITE		3
 
-#define UB_DIR_CHAR(c)  (((c)==UB_DIR_WRITE)? 'w': \
-			 (((c)==UB_DIR_READ)? 'r': 'n'))
+#define UB_DIR_CHAR(c)		(((c)==UB_DIR_WRITE)? 'w': \
+					(((c)==UB_DIR_READ)? 'r': 'n'))
 
 enum ub_scsi_cmd_state {
 	UB_CMDST_INIT,			/* Initial state */
@@ -241,8 +235,6 @@
 	struct scatterlist sgv[UB_MAX_REQ_SG];
 };
 
-/*
- */
 struct ub_capacity {
 	unsigned long nsec;		/* Linux size - 512 byte sectors */
 	unsigned int bsize;		/* Linux hardsect_size */
@@ -253,8 +245,8 @@
  * The SCSI command tracing structure.
  */
 
-#define SCMD_ST_HIST_SZ   8
-#define SCMD_TRACE_SZ    63		/* Less than 4KB of 61-byte lines */
+#define SCMD_ST_HIST_SZ		8	/* history size */
+#define SCMD_TRACE_SZ		63	/* Less than 4KB of 61-byte lines */
 
 struct ub_scsi_cmd_trace {
 	int hcur;
@@ -263,7 +255,7 @@
 	unsigned char op;
 	unsigned char dir;
 	unsigned char key, asc, ascq;
-	char st_hst[SCMD_ST_HIST_SZ];	
+	char st_hst[SCMD_ST_HIST_SZ];
 };
 
 struct ub_scsi_trace {
@@ -313,8 +305,6 @@
 	return ret;
 }
 
-/*
- */
 struct ub_scsi_cmd_queue {
 	int qlen, qmax;
 	struct ub_scsi_cmd *head, *tail;
@@ -393,8 +383,6 @@
 	struct ub_scsi_trace tr;
 };
 
-/*
- */
 static void ub_cleanup(struct ub_dev *sc);
 static int ub_request_fn_1(struct ub_lun *lun, struct request *rq);
 static void ub_cmd_build_block(struct ub_dev *sc, struct ub_lun *lun,
@@ -428,8 +416,6 @@
 static int ub_probe_clear_stall(struct ub_dev *sc, int stalled_pipe);
 static int ub_probe_lun(struct ub_dev *sc, int lnum);
 
-/*
- */
 #ifdef CONFIG_USB_LIBUSUAL
 
 #define ub_usb_ids  storage_usb_ids
@@ -450,10 +436,10 @@
  * This has to be thought out in detail before changing.
  * If UB_MAX_HOST was 1000, we'd use a bitmap. Or a better data structure.
  */
-#define UB_MAX_HOSTS  26
+#define UB_MAX_HOSTS		26
 static char ub_hostv[UB_MAX_HOSTS];
 
-#define UB_QLOCK_NUM 5
+#define UB_QLOCK_NUM		5
 static spinlock_t ub_qlockv[UB_QLOCK_NUM];
 static int ub_qlock_next = 0;
 
@@ -785,12 +771,11 @@
 	return cmd;
 }
 
-#define ub_cmdq_peek(sc)  ((sc)->cmd_queue.head)
+#define ub_cmdq_peek(sc)	((sc)->cmd_queue.head)
 
 /*
  * The request function is our main entry point
  */
-
 static void ub_request_fn(request_queue_t *q)
 {
 	struct ub_lun *lun = q->queuedata;
@@ -1420,19 +1405,19 @@
 		}
 
 		switch (bcs->Status) {
-		case US_BULK_STAT_OK:
-			break;
-		case US_BULK_STAT_FAIL:
-			ub_state_sense(sc, cmd);
-			return;
-		case US_BULK_STAT_PHASE:
-			/* P3 */ printk("%s: status PHASE\n", sc->name);
-			goto Bad_End;
-		default:
-			printk(KERN_INFO "%s: unknown CSW status 0x%x\n",
-			    sc->name, bcs->Status);
-			ub_state_done(sc, cmd, -EINVAL);
-			return;
+			case US_BULK_STAT_OK:
+				break;
+			case US_BULK_STAT_FAIL:
+				ub_state_sense(sc, cmd);
+				return;
+			case US_BULK_STAT_PHASE:
+				/* P3 */ printk("%s: status PHASE\n", sc->name);
+				goto Bad_End;
+			default:
+				printk(KERN_INFO "%s: unknown CSW status 0x%x\n",
+				sc->name, bcs->Status);
+				ub_state_done(sc, cmd, -EINVAL);
+				return;
 		}
 
 		/* Not zeroing error to preserve a babble indicator */
@@ -1548,7 +1533,6 @@
  */
 static void ub_state_stat(struct ub_dev *sc, struct ub_scsi_cmd *cmd)
 {
-
 	if (__ub_state_stat(sc, cmd) != 0)
 		return;
 
@@ -1660,8 +1644,6 @@
 	return 0;
 }
 
-/*
- */
 static void ub_top_sense_done(struct ub_dev *sc, struct ub_scsi_cmd *scmd)
 {
 	unsigned char *sense = sc->top_sense;
@@ -1705,10 +1687,8 @@
  * XXX Move usb_reset_device to khubd. Hogging kevent is not a good thing.
  * XXX Make usb_sync_reset asynchronous.
  */
-
 static void ub_reset_enter(struct ub_dev *sc, int try)
 {
-
 	if (sc->reset) {
 		/* This happens often on multi-LUN devices. */
 		return;
@@ -1799,7 +1779,6 @@
  */
 static void ub_revalidate(struct ub_dev *sc, struct ub_lun *lun)
 {
-
 	lun->readonly = 0;	/* XXX Query this from the device */
 
 	lun->capacity.nsec = 0;
@@ -1894,8 +1873,6 @@
 	return rc;
 }
 
-/*
- */
 static int ub_bd_release(struct inode *inode, struct file *filp)
 {
 	struct gendisk *disk = inode->i_bdev->bd_disk;
@@ -1969,7 +1946,6 @@
 	 */
 	if (ub_sync_tur(lun->udev, lun) != 0) {
 		lun->changed = 1;
-		return 1;
 	}
 
 	return lun->changed;
@@ -2132,8 +2108,6 @@
 	return rc;
 }
 
-/*
- */
 static void ub_probe_urb_complete(struct urb *urb, struct pt_regs *pt)
 {
 	struct completion *cop = urb->context;
@@ -2686,7 +2660,6 @@
 	 * At this point there must be no commands coming from anyone
 	 * and no URBs left in transit.
 	 */
-
 	device_remove_file(&sc->intf->dev, &dev_attr_diag);
 	usb_set_intfdata(intf, NULL);
 	// usb_put_intf(sc->intf);
@@ -2698,7 +2671,7 @@
 }
 
 static struct usb_driver ub_driver = {
-	.name =		"ub",
+	.name =		DRV_NAME,
 	.probe =	ub_probe,
 	.disconnect =	ub_disconnect,
 	.id_table =	ub_usb_ids,