Re: [PATCH 32/38] fs: fat: Shrink the size of a few strings

From: Heinrich Schuchardt <xypron.glpk@gmx.de>
To: Ilias Apalodimas <ilias.apalodimas@linaro.org>,
	Simon Glass <sjg@chromium.org>
Cc: "U-Boot Mailing List" <u-boot@lists.denx.de>,
	"Bin Meng" <bmeng.cn@gmail.com>,
	"Benoît Thébaudeau" <benoit.thebaudeau@advansee.com>
Subject: Re: [PATCH 32/38] fs: fat: Shrink the size of a few strings
Date: Fri, 31 Mar 2023 12:19:57 +0200	[thread overview]
Message-ID: <90675a84-3c3e-dc95-b8f5-565f8bd5f6be@gmx.de> (raw)
In-Reply-To: <CAC_iWjJQeHAt2OhddfWebD-0OW9nnJTvU2+PwCBRkbFEnRh_Yw@mail.gmail.com>

On 3/31/23 09:33, Ilias Apalodimas wrote:
> On Fri, 31 Mar 2023 at 09:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>>
>>
>> Am 31. März 2023 03:16:13 MESZ schrieb Simon Glass <sjg@chromium.org>:
>>> Hi Heinrich,
>>>
>>> On Fri, 31 Mar 2023 at 13:05, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>
>>>>
>>>>
>>>> Am 31. März 2023 01:49:35 MESZ schrieb Simon Glass <sjg@chromium.org>:
>>>>> Hi Heinrich,
>>>>>
>>>>> On Fri, 31 Mar 2023 at 11:48, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> Am 30. März 2023 23:32:21 MESZ schrieb Simon Glass <sjg@chromium.org>:
>>>>>>> To save a few bytes, replace Error with ** and try to use the same string
>>>>>>> for multiple messages where possible.
>>>>>>>
>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>>> ---
>>>>>>>
>>>>>>> fs/fat/fat.c       | 12 ++++++------
>>>>>>> fs/fat/fat_write.c | 14 ++++----------
>>>>>>> 2 files changed, 10 insertions(+), 16 deletions(-)
>>>>>>>
>>>>>>> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
>>>>>>> index 2da93dae3cf3..f0df7988e172 100644
>>>>>>> --- a/fs/fat/fat.c
>>>>>>> +++ b/fs/fat/fat.c
>>>>>>> @@ -97,8 +97,8 @@ int fat_register_device(struct blk_desc *dev_desc, int part_no)
>>>>>>>        /* Read the partition table, if present */
>>>>>>>        if (part_get_info(dev_desc, part_no, &info)) {
>>>>>>>                if (part_no != 0) {
>>>>>>> -                      printf("** Partition %d not valid on device %d **\n",
>>>>>>> -                                      part_no, dev_desc->devnum);
>>>>>>> +                      printf("** Partition %d invalid on device %d **\n",
>>>>>>> +                             part_no, dev_desc->devnum);
>>>>>>>                        return -1;
>>>>>>>                }
>>>>>>>
>>>>>>> @@ -168,7 +168,7 @@ static __u32 get_fatent(fsdata *mydata, __u32 entry)
>>>>>>>        __u32 ret = 0x00;
>>>>>>>
>>>>>>>        if (CHECK_CLUST(entry, mydata->fatsize)) {
>>>>>>> -              printf("Error: Invalid FAT entry: 0x%08x\n", entry);
>>>>>>> +              printf("** Invalid FAT entry: %#08x\n", entry);
>>>>>>
>>>>>> The ** is superfluous. The text makes it clear that an error occured
>>>>>
>>>>> So should I drop the other ** strings in these files too? Please take
>>>>> a look and see what you think.
>>>>
>>>> I suggest to avoid prefixes like 'Error:' and '**' in all our code if the message text already indicates an error.
>>>
>>> That makes sense to me. It is sometimes hard to know whether something
>>> indicates an error, though. If you look through fat.c you'll see what
>>> I mean.
>>
>> Users might prefer log_err writing messages in red on the console.
>
> FWIW I agree with Heinrich here.  We got 'Error: XXXXX' sprinkled
> around from older code, but it makes sense to convert them to log_err
> where possible

We should move all error output to log_err() and then can later add
decoration (color or prefix) in log_err().

Best regards

Heinrich

>
> Regards
> /Ilias
>>
>>>
>>> Regards,
>>> Simon