linux-kernel - Re: [PATCH v2] mtd: ubi: Extend UBI layer debug/messaging capabilities

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:	Fri, 03 Oct 2014 18:27:15 +0300
From:	Artem Bityutskiy <dedekind1@...il.com>
To:	Tanya Brokhman <tlinder@...eaurora.org>
Cc:	linux-mtd@...ts.infradead.org, linux-arm-msm@...r.kernel.org,
	David Woodhouse <dwmw2@...radead.org>,
	Brian Norris <computersforpeace@...il.com>,
	Richard Weinberger <richard@....at>,
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] mtd: ubi: Extend UBI layer debug/messaging
 capabilities

Yes, I guess a single patch is indeed OK. I have few nit-picks, though.

On Tue, 2014-09-30 at 18:13 +0300, Tanya Brokhman wrote:
> -		ubi_err("'ubi_io_read_ec_hdr()' returned unknown code %d", err);
> +		ubi_err(ubi,
> +		  "'ubi_io_read_ec_hdr()' returned unknown code %d", err);
>  		return -EINVAL;

I think it is fine if the line is long in these cases, let's keep the
message on the same line, this split does not contribute to better
readability, quite the opposite, in my opinion.

One line:
ubi_err(ubi, "long line")

Multiple lines:
ubi_err(ubi, "long line,
        parameters)

> -				ubi_msg("\"delete\" compatible internal volume %d:%d found, will remove it",
> +				ubi_msg(ubi,
> +				   "\"delete\" compatible internal volume %d:%d found, will remove it",
>  					vol_id, lnum);

Ditto.

> -			ubi_msg("read-only compatible internal volume %d:%d found, switch to read-only mode",
> +			ubi_msg(ubi,
> +				"read-only compatible internal volume %d:%d found, switch to read-only mode",
>  				vol_id, lnum);

Ditto. And so on, in many places.

> -				ubi_err("bad compat %d", vidh->compat);
> +				ubi_err(ubi,
> +					"bad compat %d", vidh->compat);

And for consistency, places like this would be:

				ubi_err(ubi, bad compat %d",
				        vidh->compat);
>  			if (av->used_ebs != be32_to_cpu(vidh->used_ebs)) {
> -				ubi_err("bad used_ebs %d", av->used_ebs);
> +				ubi_err(ubi,
> +					"bad used_ebs %d", av->used_ebs);

Ditto for all the similar cases.


> -	ubi_msg("volume %d (\"%s\") re-sized from %d to %d LEBs", vol_id,
> +	ubi_msg(ubi, "volume %d (\"%s\") re-sized from %d to %d LEBs",
> +		vol_id,
>  		vol->name, old_reserved_pebs, vol->reserved_pebs);

Please, in cases like this, try to pack more arguments to the second
line, and move those which do not fit there to the third line. So this
would be like:

	ubi_msg(ubi, "volume %d (\"%s\") re-sized from %d to %d LEBs",
		vol_id,	vol->name, old_reserved_pebs,
		vol->reserved_pebs);
 

--
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/