Re: [PATCH] UBI: Extend UBI layer debug/messaging capabilities - cosmetics

[Richard Weinberger]

On Mon, Nov 10, 2014 at 1:53 PM, Tanya Brokhman <tlinder@xxxxxxxxxxxxxx> wrote:
> On 11/10/2014 2:18 PM, Artem Bityutskiy wrote:
>>
>> On Sun, 2014-11-09 at 13:06 +0200, Tanya Brokhman wrote:
>>>
>>>
>>>   /* Normal UBI messages */
>>>   #define ubi_msg(ubi, fmt, ...) pr_notice("UBI-%d: %s:" fmt "\n", \
>>> -                                        ubi->ubi_num, __func__,
>>> ##__VA_ARGS__)
>>> +                               (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \
>>> +                               __func__, ##__VA_ARGS__)
>>>   /* UBI warning messages */
>>>   #define ubi_warn(ubi, fmt, ...) pr_warn("UBI-%d warning: %s: " fmt
>>> "\n", \
>>> -                                       ubi->ubi_num, __func__,
>>> ##__VA_ARGS__)
>>> +                               (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \
>>> +                               __func__, ##__VA_ARGS__)
>>>   /* UBI error messages */
>>>   #define ubi_err(ubi, fmt, ...) pr_err("UBI-%d error: %s: " fmt "\n", \
>>> -                                     ubi->ubi_num, __func__,
>>> ##__VA_ARGS__)
>>> +                               (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \
>>> +                               __func__, ##__VA_ARGS__)
>>
>>
>> Why did you make these changes? It is preferable to not add another 'if'
>> statement to this macro to handle one or 2 cases - much bloat, little
>> gain.
>>
>> Could we please avoid this?
>
>
> I just wanted to be on the safe side and prevent this macro being called
> with ubi=NULL that may crash the system. If you still prefer the "if"
> removed will do.
>
>>
>>>
>>> -       if (!ubi->free.rb_node || (ubi->free_count - ubi->beb_rsvd_pebs <
>>> 1)) {
>>> -               ubi_warn(ubi, "Can't get peb for fastmap:anchor=%d,
>>> free_cnt=%d, reserved=%d",
>>> -                        anchor, ubi->free_count, ubi->beb_rsvd_pebs);
>>> +       if (!ubi->free.rb_node || (ubi->free_count - ubi->beb_rsvd_pebs <
>>> 1))
>>>                  goto out;
>>
>>
>> The warning looks pretty poor, so I do not mind to remove it, but I
>> thought your patch is about adding a parameter, but you mix different
>> kinds of things there. Please, be stricter to the similar UBIFS patch
>> which you was going to send.
>
>
> Now I'm confused. I added this msg as part of the patch you already pushed
> to your branch but later you requested NOT to add additional msgs and if
> required add it in a different patch. So this was added by me and now
> removed by me - as per your request.

Why do you need that new warning anyways?
It was added by "UBI: Extend UBI layer debug/messaging capabilities".

>>
>>
>>> -               if (kthread_should_stop()) {
>>> -                       ubi_msg(ubi, "background thread \"%s\" should
>>> stop, PID %d",
>>> -                               ubi->bgt_name, task_pid_nr(current));
>>> +               if (kthread_should_stop())
>>>                          break;
>>> -               }
>>
>>
>> How about just turning this into a debug message, not removing?
>
>
> Same here. Removing this because *you* requested it.
> Quoting you from V5:
> "Yes, please, remove these messages or turn them into debugging messages.
> And yes, these should have been added in a separate patch."
>
>>
>> Artem.
>>
>
>
> Thanks,
> Tanya Brokhman
> --
> Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> --
> 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/



-- 
Thanks,
//richard
--
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/