linux-kernel - Re: [PATCH 4/5] ide: Add support for TRIM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 2 Apr 2009 09:28:46 -0700
From:	Matthew Wilcox <willy@...ux.intel.com>
To:	Sergei Shtylyov <sshtylyov@...mvista.com>
Cc:	linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org,
	jgarzik@...hat.com, David Woodhouse <David.Woodhouse@...el.com>
Subject: Re: [PATCH 4/5] ide: Add support for TRIM

On Thu, Apr 02, 2009 at 07:58:59PM +0400, Sergei Shtylyov wrote:
>>  +static int idedisk_prepare_discard(struct request_queue *q, struct 
>> request *rq,
>> +							struct bio *bio)
>
>    Weird indentation.

Not at all.  New line, so tab the next line over as far as it will go
without falling off the 80-column limit.  It's fairly common.

To quote Documentation/CodingStyle:

  Statements longer than 80 columns will be broken into sensible chunks.
  Descendants are always substantially shorter than the parent and are placed
  substantially to the right. The same applies to function headers with a long
  argument list. Long strings are as well broken into shorter strings.  The
  only exception to this is where exceeding 80 columns significantly increases
  readability and does not hide information.

>> +{
>> +	ide_task_t *task;
>
>    This patch is already obsolete as 'ide_task_t' is gone. Use 'struct  
> ide_cmd' instead.

Thanks, fixed.

>> +	unsigned size;
>> +	struct page *page = alloc_page(GFP_KERNEL);
>
>    Missing empty line after the declaration block...

Empty line not necessary.

>> +	if (!page)
>> +		goto error;
>
>    Unneeded goto -- why not just return?

Because it then looks the same as the other two cases where we can error
out.

>> +	task->tf_flags   = IDE_TFLAG_LBA48 | IDE_TFLAG_OUT_HOB |
>> +			   IDE_TFLAG_OUT_TF | IDE_TFLAG_OUT_DEVICE |
>
>    The last 3 flags are going to be obsoleted too...

So if I remove them today, the command will still work?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/