| Message ID | 20171221024049.36509-1-yanaijie@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
Jason, On 12/21/17 11:40, Jason Yan wrote: > My editor always try to remove the extra white space at the end of the > line when I make some changes. I'm tired of adjusting them manually. > Can we remove them in mainline? > > Signed-off-by: Jason Yan <yanaijie@huawei.com> > --- > drivers/scsi/scsi.c | 2 +- > drivers/scsi/scsi_lib.c | 20 ++++++++++---------- > drivers/scsi/scsi_scan.c | 10 +++++----- > drivers/scsi/scsi_sysfs.c | 4 ++-- > 4 files changed, 18 insertions(+), 18 deletions(-) There are plenty in drivers/scsi/sd.c too :) I would also love to get rid of these white spaces so that my emacs stops looking like a Christmas tree all year long. Last time I tried I got nacked... Martin, What about a (may be big) whitespace-only patch to clean up all the scsi code and finally get checkpatch to stop screaming at us if we touch a line near one with whitespaces (which can happen a lot...) ? Best regards. -- Damien Le Moal, Western Digital
On Thu, Dec 21, 2017 at 05:41:00AM +0000, Damien Le Moal wrote: > Jason, > > On 12/21/17 11:40, Jason Yan wrote: > > My editor always try to remove the extra white space at the end of the > > line when I make some changes. I'm tired of adjusting them manually. > > Can we remove them in mainline? > > > > Signed-off-by: Jason Yan <yanaijie@huawei.com> > > --- > > drivers/scsi/scsi.c | 2 +- > > drivers/scsi/scsi_lib.c | 20 ++++++++++---------- > > drivers/scsi/scsi_scan.c | 10 +++++----- > > drivers/scsi/scsi_sysfs.c | 4 ++-- > > 4 files changed, 18 insertions(+), 18 deletions(-) > > There are plenty in drivers/scsi/sd.c too :) > I would also love to get rid of these white spaces so that my emacs > stops looking like a Christmas tree all year long. > Last time I tried I got nacked... > > Martin, > > What about a (may be big) whitespace-only patch to clean up all the scsi > code and finally get checkpatch to stop screaming at us if we touch a > line near one with whitespaces (which can happen a lot...) ? +1 Please let's get rid of this whitespace mess.
On Thu, 2017-12-21 at 10:40 +0800, Jason Yan wrote: > My editor always try to remove the extra white space at the end of the > line when I make some changes. I'm tired of adjusting them manually. > Can we remove them in mainline? Hello Jason, I think this means that your editor has been misconfigured. Any kernel code changes should be the result of a deliberate action by a human. Configuring an editor to apply any changes automatically to existing code is wrong. There are several reasons why most kernel maintainers ignore patches like this one silently: * Whitespace changes make it harder than necessary to backport patches to distro kernels. Before a patch that came later than the whitespace changes can be backported, the whitespace change has to be backported. Additionally, if a whitespace change touches many source files, the order in which to backport patches becomes really important. * Before a kernel developer posts a patch that fixes a bug she or he has to verify the change history (git log -p) to figure out which patch introduced the bug. Patches that only change coding style pollute the change history. * Accepting a patch that only changes whitespace would open the floodgates for other kernel coding style change patches. If a patch that only changes whitespace would get accepted it will become hard to keep other kernel coding style change patches out. A few examples of recently posted coding style patches that do not change any functionality are: - scsi_dh: fix format of struct members (https://marc.info/?l=linux-scsi&m=151344115716122&w=2). - PS3: Adjustments for six function implementations (https://www.spinics.net/lists/linux-kernel-janitors/msg38252.html). See also https://btrfs.wiki.kernel.org/index.php/Developer%27s_FAQ#How_not_to_start. Thanks, Bart.
On 2017/12/22 0:09, Bart Van Assche wrote: > On Thu, 2017-12-21 at 10:40 +0800, Jason Yan wrote: >> My editor always try to remove the extra white space at the end of the >> line when I make some changes. I'm tired of adjusting them manually. >> Can we remove them in mainline? > > Hello Jason, > > I think this means that your editor has been misconfigured. Any kernel code > changes should be the result of a deliberate action by a human. Configuring an > editor to apply any changes automatically to existing code is wrong. > > There are several reasons why most kernel maintainers ignore patches like this > one silently: > * Whitespace changes make it harder than necessary to backport patches to > distro kernels. Before a patch that came later than the whitespace changes > can be backported, the whitespace change has to be backported. Additionally, > if a whitespace change touches many source files, the order in which to > backport patches becomes really important. > * Before a kernel developer posts a patch that fixes a bug she or he has to > verify the change history (git log -p) to figure out which patch introduced > the bug. Patches that only change coding style pollute the change history. > * Accepting a patch that only changes whitespace would open the floodgates for > other kernel coding style change patches. If a patch that only changes > whitespace would get accepted it will become hard to keep other kernel > coding style change patches out. A few examples of recently posted coding > style patches that do not change any functionality are: > - scsi_dh: fix format of struct members > (https://marc.info/?l=linux-scsi&m=151344115716122&w=2). > - PS3: Adjustments for six function implementations > (https://www.spinics.net/lists/linux-kernel-janitors/msg38252.html). > > See also https://btrfs.wiki.kernel.org/index.php/Developer%27s_FAQ#How_not_to_start. > > Thanks, > > Bart. > Hi Bart. I understand all these reasons. But after all these years of development, the scsi subsystem still have this *huge* number of whitespaces, especially in the scsi core files. I have not seen this in other subsystems. Thanks for your patient explain. So please ignore this patch. Jason
On Thu, Dec 21, 2017 at 04:09:07PM +0000, Bart Van Assche wrote: > On Thu, 2017-12-21 at 10:40 +0800, Jason Yan wrote: > > My editor always try to remove the extra white space at the end of the > > line when I make some changes. I'm tired of adjusting them manually. > > Can we remove them in mainline? > > Hello Jason, > > I think this means that your editor has been misconfigured. Any kernel code > changes should be the result of a deliberate action by a human. Configuring an > editor to apply any changes automatically to existing code is wrong. > > There are several reasons why most kernel maintainers ignore patches like this > one silently: > * Whitespace changes make it harder than necessary to backport patches to > distro kernels. Before a patch that came later than the whitespace changes > can be backported, the whitespace change has to be backported. Additionally, > if a whitespace change touches many source files, the order in which to > backport patches becomes really important. Honestly this is not so much of a problem, you either take the whitespace patch once or work around it. All in all I'm so much in favour of this cleanup patch (and even would apprechiate more of these, especailly getting rid of all those camelCase foo we have in the scsi hba drivers). Let's see how Martin and James deceide about it. Byte, Johannes
Hi Bart, On Thu, 21 Dec 2017, Bart Van Assche wrote: > There are several reasons why most kernel maintainers ignore patches > like this one silently: > I don't disagree with your conclusion but I think that these objections may have solutions, at least in principle. Your message prompted me write them down. I've long thought that our process has some room for improvement. I think Linux needs an official automatic code reformatter. C.f. TeX and math typesetting. Not a bunch of regular expressions, but an actual C parser that can produce correct line breaks and indentation based on the depth of the parse tree and correctly deal with macro definitions and so on. The style guide is probably ambiguous and inconsistent so I'd make this reformatter the final arbiter on style matters. Then it would be possible to automatically regenerate a patch (or rebase a commit) so it could still be applied to the tip even after the tip has had some clean-up. This becomes possible when all clean-up uses the same process, so it produces the same result. > * Whitespace changes make it harder than necessary to backport patches > to distro kernels. Before a patch that came later than the whitespace > changes can be backported, the whitespace change has to be backported. > Additionally, if a whitespace change touches many source files, the > order in which to backport patches becomes really important. When a post-clean-up commit needs to be backported to a branch made pre-clean-up, the prerequisite changes could be automatically generated for the old branch as needed. Note that this is already possible where the clean-up is scripted e.g. $ sed -i -e 's,[ \t]*$,,' drivers/scsi/scsi_lib.c which is actually the subject of this thread. > * Before a kernel developer posts a patch that fixes a bug she or he has > to verify the change history (git log -p) to figure out which patch > introduced the bug. Patches that only change coding style pollute the > change history. A clean-up commit would be pollution but (assuming the code style rules don't change) as a one-off event that patch might be preferable to perpetuating readability issues indefinitely. IMHO open source code should aim to maximize readability and re-usability (in general). > * Accepting a patch that only changes whitespace would open the > floodgates for other kernel coding style change patches. If a patch > that only changes whitespace would get accepted it will become hard to > keep other kernel coding style change patches out. Given the right tooling, code style patches need never blight maintainers' inboxes again. Such patches could be ignored and/or interpreted as suggestions that the maintainer run a script to improve some part of their tree under development (given that the resulting merge conflicts would be resolved automatically). In the event that someone needed to work on a messy file, they could start with an automatic clean-up but they would not submit the clean-up patch because the maintainer can just run the same automatic process. Reviewing that patch would be a tedious waste of time. The rest of the submission also becomes easier to review. E.g. a patch like this would end up shorter, static char foo[] = { 'b', - 'a' + 'a', + 'r', }; because the missing comma would not appear in the diff, as it was fixed implicitly. Thus irrelevant style changes disappear from the submission and cease to distract reviewers from the effect of the changes, which is what matters. The unreliable checkpatch style checks could then be replaced with a simple diff against the output of the reformatter. If a maintainer still receives patches with style issues but no other issues, they should be able to automatically correct the patch so it produces the correct code style and then commit the corrected version without everyone having to go through another email submission iteration (which polutes many inboxes). This need not break the rest of a patch series or patch queue, which could be automatically rebased/regenerated. Of course, the fly in the ointment is that the reformatter tool would have to be infallible, or at least beyond reproach. This seems to imply either infinite bikeshedding or else a final decision from above and probably an implementation too (something like 'sparse'). AFAIK this idea is purely hypothetical so thanks for entertaining it. Happy Christmas. --
Johannes, > All in all I'm so much in favour of this cleanup patch (and even would > apprechiate more of these, especailly getting rid of all those > camelCase foo we have in the scsi hba drivers). > > Let's see how Martin and James deceide about it. I am thoroughly annoyed by all the legacy whitespace problems. I've been working on two different sd patch series over the holidays and both caused me no end of grief due to legacy formatting issues. I have had an unbreak-sd patch sitting in my queue for several years but never pulled the trigger on it. For the usual reasons. I'm not particularly worried about bisection. But fixing whitespace does make it harder on the distro backporting front (Very pleased that you have now inadvertently volunteered to deal with all the issues that may arise at SUSE from such a subsystem-wide cleanup :). Anyway. I'm OK with fixing up the core pieces since they are the ones that annoy me the most. But I'm not sure we should enforce cleanups on drivers without an ack from the relevant maintainer. And for the unmaintained legacy baggage, I'm just not sure it's worth the hassle to clean things up. Fixing the crufty old things gives an illusion of the driver being actively worked on. I'd rather see dead code being left as such. Gives us a good indication of when it's safe to drop. One thing I specifically don't want is to open the flood gates for drive-by whitespace patches. I have no interest in wasting cycles on that. I generally only take arbitrary 3rd party cleanups if a driver is actively maintained and the maintainer specifically acks the change. PS. I'll at least partially unbreak sd.c as part of the series I'll be posting shortly.
"Martin K. Petersen" <martin.petersen@oracle.com> writes: > I am thoroughly annoyed by all the legacy whitespace problems. I've been > working on two different sd patch series over the holidays and both > caused me no end of grief due to legacy formatting issues. > > I have had an unbreak-sd patch sitting in my queue for several years but > never pulled the trigger on it. For the usual reasons. So we all share the same common pain points. > I'm not particularly worried about bisection. But fixing whitespace does > make it harder on the distro backporting front (Very pleased that you > have now inadvertently volunteered to deal with all the issues that may > arise at SUSE from such a subsystem-wide cleanup :). > > Anyway. I'm OK with fixing up the core pieces since they are the ones > that annoy me the most. But I'm not sure we should enforce cleanups on > drivers without an ack from the relevant maintainer. And for the > unmaintained legacy baggage, I'm just not sure it's worth the hassle to > clean things up. Fixing the crufty old things gives an illusion of the > driver being actively worked on. I'd rather see dead code being left as > such. Gives us a good indication of when it's safe to drop. Well the drivers are more of a personal pain pain point but yes cleaning up the core would be very much appreciated. > One thing I specifically don't want is to open the flood gates for > drive-by whitespace patches. I have no interest in wasting cycles on > that. I generally only take arbitrary 3rd party cleanups if a driver is > actively maintained and the maintainer specifically acks the change. Agreed > PS. I'll at least partially unbreak sd.c as part of the series I'll be > posting shortly. :-) Byte, Johannes
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c index a7e4fba..0f5bc03 100644 --- a/drivers/scsi/scsi.c +++ b/drivers/scsi/scsi.c @@ -185,7 +185,7 @@ void scsi_log_completion(struct scsi_cmnd *cmd, int disposition) void scsi_cmd_get_serial(struct Scsi_Host *host, struct scsi_cmnd *cmd) { cmd->serial_number = host->cmd_serial_number++; - if (cmd->serial_number == 0) + if (cmd->serial_number == 0) cmd->serial_number = host->cmd_serial_number++; } EXPORT_SYMBOL(scsi_cmd_get_serial); diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index bcc1694..a8dffd0 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -385,7 +385,7 @@ static void scsi_single_lun_run(struct scsi_device *current_sdev) spin_unlock_irqrestore(shost->host_lock, flags); scsi_kick_queue(sdev->request_queue); spin_lock_irqsave(shost->host_lock, flags); - + scsi_device_put(sdev); } out: @@ -1018,7 +1018,7 @@ static int scsi_init_sgtable(struct request *req, struct scsi_data_buffer *sdb) blk_rq_nr_phys_segments(req), sdb->table.sgl))) return BLKPREP_DEFER; - /* + /* * Next, walk the list, and fill in the addresses and sizes of * each segment. */ @@ -1826,7 +1826,7 @@ static void scsi_request_fn(struct request_queue *q) if (!scsi_host_queue_ready(q, shost, sdev)) goto host_not_ready; - + if (sdev->simple_tags) cmd->flags |= SCMD_TAGGED; else @@ -2412,7 +2412,7 @@ scsi_mode_select(struct scsi_device *sdev, int pf, int sp, int modepage, real_buffer[1] = data->medium_type; real_buffer[2] = data->device_specific; real_buffer[3] = data->block_descriptor_length; - + cmd[0] = MODE_SELECT; cmd[4] = len; @@ -2496,7 +2496,7 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage, if (scsi_sense_valid(sshdr)) { if ((sshdr->sense_key == ILLEGAL_REQUEST) && (sshdr->asc == 0x20) && (sshdr->ascq == 0)) { - /* + /* * Invalid command operation code */ sdev->use_10_for_ms = 0; @@ -2598,7 +2598,7 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state) goto illegal; } break; - + case SDEV_RUNNING: switch (oldstate) { case SDEV_CREATED: @@ -2909,7 +2909,7 @@ static void scsi_wait_for_queuecommand(struct scsi_device *sdev) * (which must be a legal transition). When the device is in this * state, only special requests will be accepted, all others will * be deferred. Since special requests may also be requeued requests, - * a successful return doesn't guarantee the device will be + * a successful return doesn't guarantee the device will be * totally quiescent. * * Must be called with user context, may sleep. @@ -3014,10 +3014,10 @@ int scsi_internal_device_block_nowait(struct scsi_device *sdev) return err; } - /* + /* * The device has transitioned to SDEV_BLOCK. Stop the * block layer from calling the midlayer with this device's - * request queue. + * request queue. */ if (q->mq_ops) { blk_mq_quiesce_queue_nowait(q); @@ -3067,7 +3067,7 @@ static int scsi_internal_device_block(struct scsi_device *sdev) return err; } - + void scsi_start_queue(struct scsi_device *sdev) { struct request_queue *q = sdev->request_queue; diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c index 4012464..c122f49 100644 --- a/drivers/scsi/scsi_scan.c +++ b/drivers/scsi/scsi_scan.c @@ -108,7 +108,7 @@ MODULE_PARM_DESC(scan, "sync, async, manual, or none. " static unsigned int scsi_inq_timeout = SCSI_TIMEOUT/HZ + 18; module_param_named(inq_timeout, scsi_inq_timeout, uint, S_IRUGO|S_IWUSR); -MODULE_PARM_DESC(inq_timeout, +MODULE_PARM_DESC(inq_timeout, "Timeout (in seconds) waiting for devices to answer INQUIRY." " Default is 20. Some devices may need more; most need less."); @@ -612,7 +612,7 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result, * not-ready to ready transition [asc/ascq=0x28/0x0] * or power-on, reset [asc/ascq=0x29/0x0], continue. * INQUIRY should not yield UNIT_ATTENTION - * but many buggy devices do so anyway. + * but many buggy devices do so anyway. */ if ((driver_byte(result) & DRIVER_SENSE) && scsi_sense_valid(&sshdr)) { @@ -858,7 +858,7 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result, * Don't set the device offline here; rather let the upper * level drivers eval the PQ to decide whether they should * attach. So remove ((inq_result[0] >> 5) & 7) == 1 check. - */ + */ sdev->inq_periph_qual = (inq_result[0] >> 5) & 7; sdev->lockable = sdev->removable; @@ -1001,7 +1001,7 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result, } #ifdef CONFIG_SCSI_LOGGING -/** +/** * scsi_inq_str - print INQUIRY data from min to max index, strip trailing whitespace * @buf: Output buffer with at least end-first+1 bytes of space * @inq: Inquiry buffer (input) @@ -1501,7 +1501,7 @@ EXPORT_SYMBOL(__scsi_add_device); int scsi_add_device(struct Scsi_Host *host, uint channel, uint target, u64 lun) { - struct scsi_device *sdev = + struct scsi_device *sdev = __scsi_add_device(host, channel, target, lun, NULL); if (IS_ERR(sdev)) return PTR_ERR(sdev); diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c index f796bd6..66fc622 100644 --- a/drivers/scsi/scsi_sysfs.c +++ b/drivers/scsi/scsi_sysfs.c @@ -622,7 +622,7 @@ static int scsi_sdev_check_buf_bit(const char *buf) return 1; else if (buf[0] == '0') return 0; - else + else return -EINVAL; } else return -EINVAL; @@ -788,7 +788,7 @@ store_queue_type_field(struct device *dev, struct device_attribute *attr, if (!sdev->tagged_supported) return -EINVAL; - + sdev_printk(KERN_INFO, sdev, "ignoring write to deprecated queue_type attribute"); return count;
My editor always try to remove the extra white space at the end of the line when I make some changes. I'm tired of adjusting them manually. Can we remove them in mainline? Signed-off-by: Jason Yan <yanaijie@huawei.com> --- drivers/scsi/scsi.c | 2 +- drivers/scsi/scsi_lib.c | 20 ++++++++++---------- drivers/scsi/scsi_scan.c | 10 +++++----- drivers/scsi/scsi_sysfs.c | 4 ++-- 4 files changed, 18 insertions(+), 18 deletions(-)