[U-Boot] [PATCH 3/5] net: Make copy_filename() accept NULL src

[Joe Hershberger]

Hey Alex,

On Thu, Jul 5, 2018 at 12:27 PM, Joe Hershberger <joe.hershberger at ni.com> wrote:
> On Thu, Jul 5, 2018 at 6:49 AM, Alexander Graf <agraf at suse.de> wrote:
>> On 07/04/2018 06:18 PM, Joe Hershberger wrote:
>>>
>>> On Wed, Jul 4, 2018 at 4:25 AM, Alexander Graf <agraf at suse.de> wrote:
>>>>
>>>> On 07/04/2018 02:36 AM, Joe Hershberger wrote:
>>>>>
>>>>> Rather than crashing, check the src ptr and set dst to empty string.
>>>>>
>>>>> Signed-off-by: Joe Hershberger <joe.hershberger at ni.com>
>>>>
>>>>
>>>> Wouldn't it make more sense to check for the existence outside at the
>>>> caller's side? That way it's much easier to see what really is happening.
>>>
>>> It's much easier to allow NULL so that we can directly pass the return
>>> result of getenv().
>>
>>
>> I know, and I see how it looks insanely smart and simple today. Until you
>> realize that the amazing "copy_filename" function doesn't touch the target
>> at all if it gets passed in NULL. And all of that implicitly. So implicitly
>> it will leave the old value in the filename if nothing is set in env.
>
> I think you are mis-reading the code. If src is NULL, it will set
> dst[0] = '\0';  I think the behavior is quite reasonable.

Do you have any outstanding issues with this?

>> Imaging you're trying to read the code 4 years from now. Will you remember
>> all of the side effects of copy_filename()? Imagine you're someone who's new
>> to the code and doesn't know all the implicit side effects of its functions.
>> Will you understand what is going on at a glimpse?
>
> That's an argument for documentation... and anyway, yes, I think the
> function is sensible and I would expect it to do what it does.
>
> -Joe