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

[Artem Bityutskiy]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/