Re: [U-Boot] [PATCH 3/5] net: Make copy_filename() accept NULL src
> Am 11.07.2018 um 22:54 schrieb Joe Hershberger : > > Hey Alex, > >> On Thu, Jul 5, 2018 at 12:27 PM, Joe Hershberger >> wrote: >>> On Thu, Jul 5, 2018 at 6:49 AM, Alexander Graf wrote: On 07/04/2018 06:18 PM, Joe Hershberger wrote: > On Wed, Jul 4, 2018 at 4:25 AM, Alexander Graf 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 > > > 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? Nope, your call :) Alex ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 3/5] net: Make copy_filename() accept NULL src
Hey Alex, On Thu, Jul 5, 2018 at 12:27 PM, Joe Hershberger wrote: > On Thu, Jul 5, 2018 at 6:49 AM, Alexander Graf wrote: >> On 07/04/2018 06:18 PM, Joe Hershberger wrote: >>> >>> On Wed, Jul 4, 2018 at 4:25 AM, Alexander Graf 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 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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 3/5] net: Make copy_filename() accept NULL src
On Thu, Jul 5, 2018 at 6:49 AM, Alexander Graf wrote: > On 07/04/2018 06:18 PM, Joe Hershberger wrote: >> >> On Wed, Jul 4, 2018 at 4:25 AM, Alexander Graf 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 >>> >>> >>> 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. > 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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 3/5] net: Make copy_filename() accept NULL src
On 07/04/2018 06:18 PM, Joe Hershberger wrote: On Wed, Jul 4, 2018 at 4:25 AM, Alexander Graf 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 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. 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? Alex ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 3/5] net: Make copy_filename() accept NULL src
On Wed, Jul 4, 2018 at 4:25 AM, Alexander Graf 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 > > > 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(). > > Alex > > > ___ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 3/5] net: Make copy_filename() accept NULL src
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 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. Alex ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH 3/5] net: Make copy_filename() accept NULL src
Rather than crashing, check the src ptr and set dst to empty string.
Signed-off-by: Joe Hershberger
---
net/net.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/net.c b/net/net.c
index 42a50e60f8..333102ea79 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1522,12 +1522,12 @@ void net_set_udp_header(uchar *pkt, struct in_addr
dest, int dport, int sport,
void copy_filename(char *dst, const char *src, int size)
{
- if (*src && (*src == '"')) {
+ if (src && *src && (*src == '"')) {
++src;
--size;
}
- while ((--size > 0) && *src && (*src != '"'))
+ while ((--size > 0) && src && *src && (*src != '"'))
*dst++ = *src++;
*dst = '\0';
}
--
2.11.0
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
