linux-kernel - Re: [PATCH] Staging: rar: fixed up rar

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]

Message-ID: <m3eil0o72j.fsf@intrepid.localdomain>
Date:	Fri, 05 Feb 2010 01:32:52 +0100
From:	Krzysztof Halasa <khc@...waw.pl>
To:	Josh Holland <jrh@...hh.co.uk>
Cc:	gregkh@...e.de, alan@...ux.intel.com, andre.goddard@...il.com,
	devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Staging: rar: fixed up rar_driver.{h,c}

Josh Holland <jrh@...hh.co.uk> writes:

> This is a patch to the rar_driver.c and rar_driver.h files to remove
> style issues found by the checkpatch.pl script.
>
> +++ b/drivers/staging/rar/rar_driver.c
> @@ -68,7 +68,8 @@ static void __exit rar_exit_handler(void);
>  /*
>    function that is activated on the successfull probe of the RAR device
>  */
> -static int __devinit rar_probe(struct pci_dev *pdev, const struct pci_device_id *ent);
> +static int __devinit rar_probe(struct pci_dev *pdev,
> +	const struct pci_device_id *ent);

It's agreed such changes make it worse. The 80-column "ERROR" should be
ignored, and it will be removed from checkpatch.

> -	printk(KERN_WARNING "rar- result from send ctl register is %x\n"
> -	  ,result);
> +	printk(KERN_WARNING "rar- result from send ctl register is %x\n",
> +			result);

Also, here (and then) - I'd just make it a single line if you're changing
it. I'd be far from "unwrapping" all code across the kernel, though
(without otherwise changing the lines in question).

> +		if (memrar_get_rar_addr(pdev, (*i).low, &(rar_addr[n].low))
> +			|| memrar_get_rar_addr(pdev, (*i).high,
> +			   &(rar_addr[n].high))) {
> +			result = -1;
> +			break;
> +		}

Isn't the following a bit more readable?

+		if (memrar_get_rar_addr(pdev, i->low, &rar_addr[n].low) ||
+		    memrar_get_rar_addr(pdev, i->high, &rar_addr[n].high)) {
+			result = -1;
+			break;
+		}

It doesn't make sense to split the printk, at least every single output
line printed shouldn't be broken into pieces (but perhaps one single
line for the whole printk() is best).
Also I like the post-increments (z++) more, but maybe it's just me.

> +		size_t z;
> +		for (z = 0; z != MRST_NUM_RAR; ++z) {
> +			printk(KERN_WARNING "rar - "
> +			"BRAR[%Zd] physical address low\n"
> +			"\tlow:  0x%08x\n"
> +			"\thigh: 0x%08x\n",
> +			z,
> +			rar_addr[z].low,
> +			rar_addr[z].high);


> -#define DEBUG_PRINT_0(DEBUG_LEVEL , info) \
> -do \
> -{ \
> -  if(DEBUG_LEVEL) \
> -  { \
> -    printk(KERN_WARNING info); \
> -  } \
> -}while(0)
> +#define DEBUG_PRINT_0(DEBUG_LEVEL, info) \
> +do { \
> +	if (DEBUG_LEVEL) \
> +		printk(KERN_WARNING info); \
> +} while (0)

Also I think moving these backslashes to the right of the macro code is
preferred, isn't it?

Just my 0.01$CURRENCY as usual :-)
-- 
Krzysztof Halasa
--
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/