bootp: Fix bug in auto_load function

Patch: "Put common autoload code into auto_load() function" (sha1: 093498669e77597635a24f326f11efeab213d394) is not simple code cleanup but code change which introduce new bug.

If autoload variable is not setup it worked as autoload=yes.

Currently if autoload is not setup dhcp sends request in forever loop.

Hi Michal,

Quoted 8 lines · Michal Simek

Patch: "Put common autoload code into auto_load() function" (sha1: 093498669e77597635a24f326f11efeab213d394) is not simple code cleanup but code change which introduce new bug.

If autoload variable is not setup it worked as autoload=yes.

Currently if autoload is not setup dhcp sends request in forever loop.

Thanks for bring this up - there was some discussion at the time I remember. Also please excuse the verbatim code in this email but I want it to be clear.

The old code in v2011.03 is:

                       /* Obey the 'autoload' setting */
                       if ((s = getenv("autoload")) != NULL) {
                               if (*s == 'n') {
                                       /*
                                        * Just use BOOTP to configure
system;
                                        * Do not use TFTP to load the
bootfile.
                                        */
                                       NetState = NETLOOP_SUCCESS;
                                       return;
#if defined(CONFIG_CMD_NFS)
                               } else if (strcmp(s, "NFS") == 0) {
                                       /*
                                        * Use NFS to load the bootfile.
                                        */
                                       NfsStart();
                                       return;
#endif
                               }
                       }
                       TftpStart();
                       return;

and

       if ((s = getenv("autoload")) != NULL) {
               if (*s == 'n') {
                       /*
                        * Just use BOOTP to configure system;
                        * Do not use TFTP to load the bootfile.
                        */
                       NetState = NETLOOP_SUCCESS;
                       return;
#if defined(CONFIG_CMD_NFS)
               } else if (strcmp(s, "NFS") == 0) {
                       /*
                        * Use NFS to load the bootfile.
                        */
                       NfsStart();
                       return;
#endif
               }
       }

       TftpStart();

These two sites were changed to a call to auto_load:

static void auto_load(void)

{
       const char *s = getenv("autoload");

       if (s != NULL) {
               if (*s == 'n') {
                       /*
                        * Just use BOOTP to configure system;
                        * Do not use TFTP to load the bootfile.
                        */
                       NetState = NETLOOP_SUCCESS;
                       return;
               }
#if defined(CONFIG_CMD_NFS)
               if (strcmp(s, "NFS") == 0) {
                       /*
                        * Use NFS to load the bootfile.
                        */
                       NfsStart();
                       return;
               }
#endif
       TftpStart();
       }
}

I don't see a change in the functionality here - I may be missing something so please can you explain?

From my experience U-Boot has always performed an autoload unless you 'setenv autoload n', even if the variable is not set. From the README:

autoload - if set to "no" (any string beginning with 'n'),
"bootp" will just load perform a lookup of the configuration from the BOOTP server, but not try to load any image using TFTP

which suggests that this is the correct behavior (i.e. it defaults to autoload=yes)

Quoted 27 lines

There are two options how to fix it:
1. Move TftpStart() which is in this patch
2. Change functionality if autoload is not setup, set NetSate and ends.

(quoted diff) +2 -1

@@ -165,7 +165,8 @@ static void auto_load(void)
               }
 #endif
       TftpStart();
-       }
+       } else
+               NetState = NETLOOP_SUCCESS;
 }
 

CC Eric B?nard <[email protected]>

CC Simon Glass <[email protected]>

Signed-off-by Michal Simek <[email protected]>

---
 net/bootp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

net/bootp.c +0 -0

index 3db08ea..a003c42 100644

net/bootp.c static void auto_load(void) · line 169

Quoted 7 lines

net/bootp.c +1 -1

@@ -164,8 +164,8 @@ static void auto_load(void)
                       return;
               }
 #endif
-       TftpStart();
       }
+       TftpStart();
 }

Isn't this the opposite of the patch you want? I think you want to move TftpStart() inside the loop, like this:

static void auto_load(void)

{
       const char *s = getenv("autoload");

       if (s != NULL) {
               if (*s == 'n') {
                       /*
                        * Just use BOOTP to configure system;
                        * Do not use TFTP to load the bootfile.
                        */
                       NetState = NETLOOP_SUCCESS;
                       return;
               }
#if defined(CONFIG_CMD_NFS)
               if (strcmp(s, "NFS") == 0) {
                       /*
                        * Use NFS to load the bootfile.

                        */
                       NfsStart();
                       return;
               TftpStart();
               }
#endif
       }
}

Patch: "Put common autoload code into auto_load() function" (sha1: 093498669e77597635a24f326f11efeab213d394) is not simple code cleanup but code change which introduce new bug.

If autoload variable is not setup it worked as autoload=yes.

Currently if autoload is not setup dhcp sends request in forever loop.

There are two options how to fix it:
1. Move TftpStart() which is in this patch
2. Change functionality if autoload is not setup, set NetSate and ends.

Dear Michal Simek,

Quoted 5 lines · In message <[email protected]> you wrote:

Patch: "Put common autoload code into auto_load() function" (sha1: 093498669e77597635a24f326f11efeab213d394) is not simple code cleanup but code change which introduce new bug.

If autoload variable is not setup it worked as autoload=yes.

Yes, and this is intentionally, because it's what poople expect when they run the "dhcpboot" command.

Quoted 3 lines

Currently if autoload is not setup dhcp sends request in forever loop.

That's not correct. Only until a DHCP server provides the needed configurationinformation so TFTP gets started.

Quoted 4 lines

There are two options how to fix it:
1. Move TftpStart() which is in this patch
2. Change functionality if autoload is not setup, set NetSate and ends.

That would change behavious in a way that is not wanted.