[U-Boot] [PATCH 1/5] net: When checking prerequisites, consider boot_file_name

[U-Boot] [PATCH 1/5] net: When checking prerequisites, consider boot_file_name

[Joe Hershberger]

For net_boot_common, we allow the serverip to be specified as part of
the boot file name. For net commands that require serverip, include that
source as a valid specification of serverip.

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
---

 include/net.h | 3 +++
 net/net.c     | 7 ++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/include/net.h b/include/net.h
index f9984ae86c..de2d7bba19 100644
--- a/include/net.h
+++ b/include/net.h
@@ -839,6 +839,9 @@ ushort env_get_vlan(char *);
 /* copy a filename (allow for "..." notation, limit length) */
 void copy_filename(char *dst, const char *src, int size);
 
+/* check if serverip is specified in filename from the command line */
+int is_serverip_in_cmd(void);
+
 /* get a random source port */
 unsigned int random_port(void);
 
diff --git a/net/net.c b/net/net.c
index f35695b4fc..bff3e9c5b5 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1341,7 +1341,7 @@ static int net_check_prereq(enum proto_t protocol)
 		/* Fall through */
 	case TFTPGET:
 	case TFTPPUT:
-		if (net_server_ip.s_addr == 0) {
+		if (net_server_ip.s_addr == 0 && !is_serverip_in_cmd()) {
 			puts("*** ERROR: `serverip' not set\n");
 			return 1;
 		}
@@ -1512,6 +1512,11 @@ void copy_filename(char *dst, const char *src, int size)
 	*dst = '\0';
 }
 
+int is_serverip_in_cmd(void)
+{
+	return !!strchr(net_boot_file_name, ':');
+}
+
 #if	defined(CONFIG_CMD_NFS)		|| \
 	defined(CONFIG_CMD_SNTP)	|| \
 	defined(CONFIG_CMD_DNS)
-- 
2.11.0

[U-Boot] [PATCH 2/5] net: Re-check prerequisites when autoloading

[Joe Hershberger]

With net autoload, we check the prerequisites for the initial command,
but the greater prerequisites when autoloading are not checked.

If we would attempt to autoload, check those prerequisites too.

If we are not expecting a serverip from the server, then don't worry
about it not being set, but don't attempt to load if it isn't.

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
---

 net/net.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/net/net.c b/net/net.c
index bff3e9c5b5..42a50e60f8 100644
--- a/net/net.c
+++ b/net/net.c
@@ -332,6 +332,16 @@ void net_auto_load(void)
 	const char *s = env_get("autoload");
 
 	if (s != NULL && strcmp(s, "NFS") == 0) {
+		if (net_check_prereq(NFS)) {
+/* We aren't expecting to get a serverip, so just accept the assigned IP */
+#ifdef CONFIG_BOOTP_SERVERIP
+			net_set_state(NETLOOP_SUCCESS);
+#else
+			printf("Cannot autoload with NFS\n");
+			net_set_state(NETLOOP_FAIL);
+#endif
+			return;
+		}
 		/*
 		 * Use NFS to load the bootfile.
 		 */
@@ -347,6 +357,16 @@ void net_auto_load(void)
 		net_set_state(NETLOOP_SUCCESS);
 		return;
 	}
+	if (net_check_prereq(TFTPGET)) {
+/* We aren't expecting to get a serverip, so just accept the assigned IP */
+#ifdef CONFIG_BOOTP_SERVERIP
+		net_set_state(NETLOOP_SUCCESS);
+#else
+		printf("Cannot autoload with TFTPGET\n");
+		net_set_state(NETLOOP_FAIL);
+#endif
+		return;
+	}
 	tftp_start(TFTPGET);
 }
 
-- 
2.11.0

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

[Joe Hershberger]

Rather than crashing, check the src ptr and set dst to empty string.

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
---

 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] [PATCH 4/5] net: Read bootfile from env on netboot_common()

[Joe Hershberger]

Instead of depending on a env callback for bootfile, read it explicitly.

We do this because the bootfile can be specified on the command line and
if it is, we will overwrite the internal variable. If a netboot_common()
is called again with no bootfile parameter, we want to use the one in
the environment.

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
---

 cmd/net.c |  6 ++++++
 net/net.c | 20 --------------------
 2 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/cmd/net.c b/cmd/net.c
index eca6dd8918..89721b8f8b 100644
--- a/cmd/net.c
+++ b/cmd/net.c
@@ -192,6 +192,9 @@ static int netboot_common(enum proto_t proto, cmd_tbl_t *cmdtp, int argc,
 
 	switch (argc) {
 	case 1:
+		/* refresh bootfile name from env */
+		copy_filename(net_boot_file_name, env_get("bootfile"),
+			      sizeof(net_boot_file_name));
 		break;
 
 	case 2:	/*
@@ -203,6 +206,9 @@ static int netboot_common(enum proto_t proto, cmd_tbl_t *cmdtp, int argc,
 		addr = simple_strtoul(argv[1], &end, 16);
 		if (end == (argv[1] + strlen(argv[1]))) {
 			load_addr = addr;
+			/* refresh bootfile name from env */
+			copy_filename(net_boot_file_name, env_get("bootfile"),
+				      sizeof(net_boot_file_name));
 		} else {
 			net_boot_file_name_explicit = true;
 			copy_filename(net_boot_file_name, argv[1],
diff --git a/net/net.c b/net/net.c
index 333102ea79..1b6781d358 100644
--- a/net/net.c
+++ b/net/net.c
@@ -216,26 +216,6 @@ int __maybe_unused net_busy_flag;
 
 /**********************************************************************/
 
-static int on_bootfile(const char *name, const char *value, enum env_op op,
-	int flags)
-{
-	if (flags & H_PROGRAMMATIC)
-		return 0;
-
-	switch (op) {
-	case env_op_create:
-	case env_op_overwrite:
-		copy_filename(net_boot_file_name, value,
-			      sizeof(net_boot_file_name));
-		break;
-	default:
-		break;
-	}
-
-	return 0;
-}
-U_BOOT_ENV_CALLBACK(bootfile, on_bootfile);
-
 static int on_ipaddr(const char *name, const char *value, enum env_op op,
 	int flags)
 {
-- 
2.11.0

[U-Boot] [PATCH 5/5] net: Consolidate the parsing of bootfile

[Joe Hershberger]

The same basic parsing was implemented in tftp and nfs, so add a helper
function to do the work once.

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>

---

 include/net.h | 11 +++++++++++
 net/net.c     | 20 ++++++++++++++++++++
 net/nfs.c     | 15 ++-------------
 net/tftp.c    | 13 +------------
 4 files changed, 34 insertions(+), 25 deletions(-)

diff --git a/include/net.h b/include/net.h
index de2d7bba19..62f82c4dca 100644
--- a/include/net.h
+++ b/include/net.h
@@ -842,6 +842,17 @@ void copy_filename(char *dst, const char *src, int size);
 /* check if serverip is specified in filename from the command line */
 int is_serverip_in_cmd(void);
 
+/**
+ * net_parse_bootfile - Parse the bootfile env var / cmd line param
+ *
+ * @param ipaddr - a pointer to the ipaddr to populate if included in bootfile
+ * @param filename - a pointer to the string to save the filename part
+ * @param max_len - The longest - 1 that the filename part can be
+ *
+ * return 1 if parsed, 0 if bootfile is empty
+ */
+int net_parse_bootfile(struct in_addr *ipaddr, char *filename, int max_len);
+
 /* get a random source port */
 unsigned int random_port(void);
 
diff --git a/net/net.c b/net/net.c
index 1b6781d358..31cf306ae7 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1517,6 +1517,26 @@ int is_serverip_in_cmd(void)
 	return !!strchr(net_boot_file_name, ':');
 }
 
+int net_parse_bootfile(struct in_addr *ipaddr, char *filename, int max_len)
+{
+	char *colon;
+
+	if (net_boot_file_name[0] == '\0')
+		return 0;
+
+	colon = strchr(net_boot_file_name, ':');
+	if (colon) {
+		if (ipaddr)
+			*ipaddr = string_to_ip(net_boot_file_name);
+		strncpy(filename, colon + 1, max_len);
+	} else {
+		strncpy(filename, net_boot_file_name, max_len);
+	}
+	filename[max_len - 1] = '\0';
+
+	return 1;
+}
+
 #if	defined(CONFIG_CMD_NFS)		|| \
 	defined(CONFIG_CMD_SNTP)	|| \
 	defined(CONFIG_CMD_DNS)
diff --git a/net/nfs.c b/net/nfs.c
index 9a16765ba1..ed6a64d722 100644
--- a/net/nfs.c
+++ b/net/nfs.c
@@ -859,7 +859,8 @@ void nfs_start(void)
 		return;
 	}
 
-	if (net_boot_file_name[0] == '\0') {
+	if (!net_parse_bootfile(&nfs_server_ip, nfs_path,
+				sizeof(nfs_path_buff))) {
 		sprintf(nfs_path, "/nfsroot/%02X%02X%02X%02X.img",
 			net_ip.s_addr & 0xFF,
 			(net_ip.s_addr >>  8) & 0xFF,
@@ -868,18 +869,6 @@ void nfs_start(void)
 
 		debug("*** Warning: no boot file name; using '%s'\n",
 		      nfs_path);
-	} else {
-		char *p = net_boot_file_name;
-
-		p = strchr(p, ':');
-
-		if (p != NULL) {
-			nfs_server_ip = string_to_ip(net_boot_file_name);
-			++p;
-			strcpy(nfs_path, p);
-		} else {
-			strcpy(nfs_path, net_boot_file_name);
-		}
 	}
 
 	nfs_filename = basename(nfs_path);
diff --git a/net/tftp.c b/net/tftp.c
index 6671b1f7ca..68ffd81414 100644
--- a/net/tftp.c
+++ b/net/tftp.c
@@ -735,7 +735,7 @@ void tftp_start(enum proto_t protocol)
 	      tftp_block_size_option, timeout_ms);
 
 	tftp_remote_ip = net_server_ip;
-	if (net_boot_file_name[0] == '\0') {
+	if (!net_parse_bootfile(&tftp_remote_ip, tftp_filename, MAX_LEN)) {
 		sprintf(default_filename, "%02X%02X%02X%02X.img",
 			net_ip.s_addr & 0xFF,
 			(net_ip.s_addr >>  8) & 0xFF,
@@ -747,17 +747,6 @@ void tftp_start(enum proto_t protocol)
 
 		printf("*** Warning: no boot file name; using '%s'\n",
 		       tftp_filename);
-	} else {
-		char *p = strchr(net_boot_file_name, ':');
-
-		if (p == NULL) {
-			strncpy(tftp_filename, net_boot_file_name, MAX_LEN);
-			tftp_filename[MAX_LEN - 1] = 0;
-		} else {
-			tftp_remote_ip = string_to_ip(net_boot_file_name);
-			strncpy(tftp_filename, p + 1, MAX_LEN);
-			tftp_filename[MAX_LEN - 1] = 0;
-		}
 	}
 
 	printf("Using %s device\n", eth_get_name());
-- 
2.11.0

[U-Boot] [PATCH 1/5] net: When checking prerequisites, consider boot_file_name

[Alexander Graf]

On 07/04/2018 02:36 AM, Joe Hershberger wrote:
> For net_boot_common, we allow the serverip to be specified as part of
> the boot file name. For net commands that require serverip, include that
> source as a valid specification of serverip.
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>

Reviewed-by: Alexander Graf <agraf@suse.de>

Alex

[U-Boot] [PATCH 2/5] net: Re-check prerequisites when autoloading

[Alexander Graf]

On 07/04/2018 02:36 AM, Joe Hershberger wrote:
> With net autoload, we check the prerequisites for the initial command,
> but the greater prerequisites when autoloading are not checked.
>
> If we would attempt to autoload, check those prerequisites too.
>
> If we are not expecting a serverip from the server, then don't worry
> about it not being set, but don't attempt to load if it isn't.
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> ---
>
>   net/net.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
>
> diff --git a/net/net.c b/net/net.c
> index bff3e9c5b5..42a50e60f8 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -332,6 +332,16 @@ void net_auto_load(void)
>   	const char *s = env_get("autoload");
>   
>   	if (s != NULL && strcmp(s, "NFS") == 0) {
> +		if (net_check_prereq(NFS)) {
> +/* We aren't expecting to get a serverip, so just accept the assigned IP */
> +#ifdef CONFIG_BOOTP_SERVERIP
> +			net_set_state(NETLOOP_SUCCESS);
> +#else
> +			printf("Cannot autoload with NFS\n");
> +			net_set_state(NETLOOP_FAIL);
> +#endif

I don't understand the #ifdef. In the CONFIG_BOOTP_SERVERIP case, you 
should already have net_server_ip set from the variable setter, so when 
do you realistically get into the case where net_check_prereq() fails 
here? I can only see that happening when serverip is not set (read: 
net_server_ip == 0.0.0.0) in which case we should also error out in the 
CONFIG_BOOTP_SERVERIP case, no?


Alex

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

[Alexander Graf]

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@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.


Alex

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

[Joe Hershberger]

On Wed, Jul 4, 2018 at 4:25 AM, Alexander Graf <agraf@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@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().

>
> Alex
>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH 2/5] net: Re-check prerequisites when autoloading

[Joe Hershberger]

On Wed, Jul 4, 2018 at 4:20 AM, Alexander Graf <agraf@suse.de> wrote:
> On 07/04/2018 02:36 AM, Joe Hershberger wrote:
>>
>> With net autoload, we check the prerequisites for the initial command,
>> but the greater prerequisites when autoloading are not checked.
>>
>> If we would attempt to autoload, check those prerequisites too.
>>
>> If we are not expecting a serverip from the server, then don't worry
>> about it not being set, but don't attempt to load if it isn't.
>>
>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>> ---
>>
>>   net/net.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/net/net.c b/net/net.c
>> index bff3e9c5b5..42a50e60f8 100644
>> --- a/net/net.c
>> +++ b/net/net.c
>> @@ -332,6 +332,16 @@ void net_auto_load(void)
>>         const char *s = env_get("autoload");
>>         if (s != NULL && strcmp(s, "NFS") == 0) {
>> +               if (net_check_prereq(NFS)) {
>> +/* We aren't expecting to get a serverip, so just accept the assigned IP
>> */
>> +#ifdef CONFIG_BOOTP_SERVERIP
>> +                       net_set_state(NETLOOP_SUCCESS);
>> +#else
>> +                       printf("Cannot autoload with NFS\n");
>> +                       net_set_state(NETLOOP_FAIL);
>> +#endif
>
>
> I don't understand the #ifdef. In the CONFIG_BOOTP_SERVERIP case, you should
> already have net_server_ip set from the variable setter, so when do you
> realistically get into the case where net_check_prereq() fails here? I can

My thinking here was that if the user is in control of the serverip
and chooses not to set it, then at least populate the dhcp variables
that were successful. If we return a fail from here, even though DHCP
was successful, the result will not be saved to the env for the user.

> only see that happening when serverip is not set (read: net_server_ip ==
> 0.0.0.0) in which case we should also error out in the CONFIG_BOOTP_SERVERIP
> case, no?
>
>
> Alex
>
>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

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

[Alexander Graf]

On 07/04/2018 06:18 PM, Joe Hershberger wrote:
> On Wed, Jul 4, 2018 at 4:25 AM, Alexander Graf <agraf@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@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.

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] [PATCH 2/5] net: Re-check prerequisites when autoloading

[Alexander Graf]

On 07/04/2018 06:23 PM, Joe Hershberger wrote:
> On Wed, Jul 4, 2018 at 4:20 AM, Alexander Graf <agraf@suse.de> wrote:
>> On 07/04/2018 02:36 AM, Joe Hershberger wrote:
>>> With net autoload, we check the prerequisites for the initial command,
>>> but the greater prerequisites when autoloading are not checked.
>>>
>>> If we would attempt to autoload, check those prerequisites too.
>>>
>>> If we are not expecting a serverip from the server, then don't worry
>>> about it not being set, but don't attempt to load if it isn't.
>>>
>>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>>> ---
>>>
>>>    net/net.c | 20 ++++++++++++++++++++
>>>    1 file changed, 20 insertions(+)
>>>
>>> diff --git a/net/net.c b/net/net.c
>>> index bff3e9c5b5..42a50e60f8 100644
>>> --- a/net/net.c
>>> +++ b/net/net.c
>>> @@ -332,6 +332,16 @@ void net_auto_load(void)
>>>          const char *s = env_get("autoload");
>>>          if (s != NULL && strcmp(s, "NFS") == 0) {
>>> +               if (net_check_prereq(NFS)) {
>>> +/* We aren't expecting to get a serverip, so just accept the assigned IP
>>> */
>>> +#ifdef CONFIG_BOOTP_SERVERIP
>>> +                       net_set_state(NETLOOP_SUCCESS);
>>> +#else
>>> +                       printf("Cannot autoload with NFS\n");
>>> +                       net_set_state(NETLOOP_FAIL);
>>> +#endif
>>
>> I don't understand the #ifdef. In the CONFIG_BOOTP_SERVERIP case, you should
>> already have net_server_ip set from the variable setter, so when do you
>> realistically get into the case where net_check_prereq() fails here? I can
> My thinking here was that if the user is in control of the serverip
> and chooses not to set it, then at least populate the dhcp variables
> that were successful. If we return a fail from here, even though DHCP
> was successful, the result will not be saved to the env for the user.

IMHO CONFIG_BOOTP_SERVERIP is a very esoteric use case already. Let's 
not interpret too much into it. Instead, I would prefer if we could just 
treat it the same as the normal case as often as we can ...

Or maybe just move its functionality (do not set serverip from dhcp 
command) into an environment variable.


Alex

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

[Joe Hershberger]

On Thu, Jul 5, 2018 at 6:49 AM, Alexander Graf <agraf@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@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@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.

> 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] [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@ni.com> wrote:
> On Thu, Jul 5, 2018 at 6:49 AM, Alexander Graf <agraf@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@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@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

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

[Alexander Graf]

> Am 11.07.2018 um 22:54 schrieb Joe Hershberger <joe.hershberger@ni.com>:
> 
> Hey Alex,
> 
>> On Thu, Jul 5, 2018 at 12:27 PM, Joe Hershberger <joe.hershberger@ni.com> wrote:
>>> On Thu, Jul 5, 2018 at 6:49 AM, Alexander Graf <agraf@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@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@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?

Nope, your call :)

Alex

[U-Boot] net: When checking prerequisites, consider boot_file_name

[Joe Hershberger]

Hi Joe,

https://patchwork.ozlabs.org/patch/939043/ was applied to http://git.denx.de/?p=u-boot/u-boot-net.git

Thanks!
-Joe

[U-Boot] net: Re-check prerequisites when autoloading

[Joe Hershberger]

Hi Joe,

https://patchwork.ozlabs.org/patch/939041/ was applied to http://git.denx.de/?p=u-boot/u-boot-net.git

Thanks!
-Joe

[U-Boot] net: Make copy_filename() accept NULL src

[Joe Hershberger]

Hi Joe,

https://patchwork.ozlabs.org/patch/939042/ was applied to http://git.denx.de/?p=u-boot/u-boot-net.git

Thanks!
-Joe

[U-Boot] net: Read bootfile from env on netboot_common()

[Joe Hershberger]

Hi Joe,

https://patchwork.ozlabs.org/patch/939044/ was applied to http://git.denx.de/?p=u-boot/u-boot-net.git

Thanks!
-Joe

[U-Boot] net: Consolidate the parsing of bootfile

[Joe Hershberger]

Hi Joe,

https://patchwork.ozlabs.org/patch/939045/ was applied to http://git.denx.de/?p=u-boot/u-boot-net.git

Thanks!
-Joe