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

[Tanya Brokhman <tlinder@codeaurora.org>]

On 9/28/2014 3:01 PM, Artem Bityutskiy wrote:
> On Sun, 2014-09-28 at 09:36 +0300, Tanya Brokhman wrote:
>> If there is more then one UBI device mounted, there is no way to
>> distinguish between messages from different UBI devices.
>> Add device number to all ubi layer message types.
>
> Hi, the goal looks legit to me, but the patch is so large that I do not
> think that I can really review it in this form.
>
> a) A patch which changes the macros (ubi_err(), etc)

If I divide the patches like this, patch (a) wont compile

> b) A set of patches which do not change messages at all, but add the
> 'ubi' parameter to the places where it is missing.
> c) A patch which changes the messages.

I think patches (b)+(C) can be combined into one patch. Don't you agree? 
I changed ~2 or 3 messages that were printing ubi number by themselves. 
No need for a separate patch for this. Don't you agree?

>
> So a) will be the most important patch for the reviewer. b) - more or
> less mechanical changes of a similar kind. c) - the same.
>
> Also, if you add a parameter to 'ubi_err()' and the other printing
> wrappers, add 'ubi' there, not 'ubi_num'. This will allow to prefix
> messages with vary different things, not just the device number in the
> future. So the calls would look like
>
> ubi_err(ubi, "inconsistent used_ebs");
>
> Once this is done, the series should be more reviewable. The next thing
> I'd check is whether we really need to change all the messages, or most
> of them, or we actually need to change only a small part of them. In the
> former case, it is OK to do what you do, I guess. In the latter case we
> probably better off with introducing a separate set of printing macros
> and leave the existing ones as they are.

Large portion of the messages needs updating. I think perhaps I'll 
overcome the messages during init (when I don't' have the ubi yet) in a 
different manner and add "ubi" not "ubi_num" parameter to the macros, as 
you suggested

>
> Thanks!
>


-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
hosted by The Linux Foundation