| Message ID | 1526658340-1992-2-git-send-email-thuth@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Support network booting with pxelinux.cfg files | expand |
On Fri, 18 May 2018 17:45:30 +0200 Thomas Huth <thuth@redhat.com> wrote: > The blocksize is hard-coded to 1428 bytes in obp-tftp.fs, so instead of > hardcoding this in the Forth code, we could also move this into tftp.c > directly instead. A similar condition exists with the huge-tftp-load > parameter. While this non-standard variable could still be changed in the > obp-tftp package, it does not make much sense to set it to zero since you > only lose the possibility to do huge TFTP loads with index wrap-around in > that case. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > lib/libnet/libnet.code | 4 +--- > lib/libnet/netapps.h | 3 +-- > lib/libnet/netload.c | 11 ++++------- > lib/libnet/tftp.c | 13 +++---------- > lib/libnet/tftp.h | 2 +- > slof/fs/packages/obp-tftp.fs | 3 --- > 6 files changed, 10 insertions(+), 26 deletions(-) > > diff --git a/lib/libnet/libnet.code b/lib/libnet/libnet.code > index 2746782..419419d 100644 > --- a/lib/libnet/libnet.code > +++ b/lib/libnet/libnet.code > @@ -4,11 +4,9 @@ > PRIM(NET_X2d_LOAD) > int alen = TOS.n; POP; > char *arg = TOS.a; POP; > - int blocksize = TOS.n; POP; > - int hugeload = TOS.n; POP; > long maxlen = TOS.n; POP; > void *loadaddr = TOS.a; > - TOS.n = netload(loadaddr, maxlen, hugeload, blocksize, arg, alen); > + TOS.n = netload(loadaddr, maxlen, arg, alen); > MIRP > > PRIM(NET_X2d_PING) > diff --git a/lib/libnet/netapps.h b/lib/libnet/netapps.h > index 0e637e1..6e00466 100644 > --- a/lib/libnet/netapps.h > +++ b/lib/libnet/netapps.h > @@ -18,8 +18,7 @@ > > struct filename_ip; > > -extern int netload(char *buffer, int len, int huge_load, int block_size, > - char *args_fs, int alen); > +extern int netload(char *buffer, int len, char *args_fs, int alen); > extern int ping(char *args_fs, int alen); > extern int dhcp(char *ret_buffer, struct filename_ip *fn_ip, > unsigned int retries, int flags); > diff --git a/lib/libnet/netload.c b/lib/libnet/netload.c > index 5c37fe2..8dca654 100644 > --- a/lib/libnet/netload.c > +++ b/lib/libnet/netload.c > @@ -405,13 +405,12 @@ static void seed_rng(uint8_t mac[]) > } > > static int tftp_load(filename_ip_t *fnip, unsigned char *buffer, int len, > - unsigned int retries, int32_t mode, > - int32_t blksize, int ip_vers) > + unsigned int retries, int ip_vers) > { > tftp_err_t tftp_err; > int rc; > > - rc = tftp(fnip, buffer, len, retries, &tftp_err, mode, blksize, ip_vers); > + rc = tftp(fnip, buffer, len, retries, &tftp_err, ip_vers); > > if (rc > 0) { > printf(" TFTP: Received %s (%d KBytes)\n", fnip->filename, > @@ -510,8 +509,7 @@ static void encode_response(char *pkt_buffer, size_t size, int ip_init) > } > } > > -int netload(char *buffer, int len, int huge_load, int block_size, > - char *args_fs, int alen) > +int netload(char *buffer, int len, char *args_fs, int alen) > { > int rc; > filename_ip_t fn_ip; > @@ -755,8 +753,7 @@ int netload(char *buffer, int len, int huge_load, int block_size, > > /* Do the TFTP load and print error message if necessary */ > rc = tftp_load(&fn_ip, (unsigned char *)buffer, len, > - obp_tftp_args.tftp_retries, huge_load, > - block_size, ip_version); > + obp_tftp_args.tftp_retries, ip_version); > > if (obp_tftp_args.ip_init == IP_INIT_DHCP) > dhcp_send_release(fn_ip.fd); > diff --git a/lib/libnet/tftp.c b/lib/libnet/tftp.c > index 1656c27..5e7951f 100644 > --- a/lib/libnet/tftp.c > +++ b/lib/libnet/tftp.c > @@ -497,19 +497,15 @@ void handle_tftp_dun(uint8_t err_code) > * @param _len size of destination buffer > * @param _retries max number of retries > * @param _tftp_err contains info about TFTP-errors (e.g. lost packets) > - * @param _mode NON ZERO - multicast, ZERO - unicast > - * @param _blocksize blocksize for DATA-packets > * @return ZERO - error condition occurs > * NON ZERO - size of received file > */ > int tftp(filename_ip_t * _fn_ip, unsigned char *_buffer, int _len, > - unsigned int _retries, tftp_err_t * _tftp_err, > - int32_t _mode, int32_t _blocksize, int _ip_version) > + unsigned int _retries, tftp_err_t * _tftp_err, int _ip_version) > { > retries = _retries; > fn_ip = _fn_ip; > len = _len; > - huge_load = _mode; > ip_version = _ip_version; > tftp_errno = 0; > tftp_err = _tftp_err; > @@ -523,17 +519,14 @@ int tftp(filename_ip_t * _fn_ip, unsigned char *_buffer, int _len, > port_number = -1; > progress_first = -1; > progress_last_bytes = 0; > + huge_load = 1; > This static variable seems to have only two users, both in handle_tftp(): else if( block == 0xffff && huge_load != 0 && (tftp->th_data == 0 || tftp->th_data == 1) ) { block = tftp->th_data; } and if (block >= 0xffff && huge_load == 0) { tftp_errno = -9; goto error; } which could be simplified as well, but this can be done later I guess. Reviewed-by: Greg Kurz <groug@kaod.org> > /* Default blocksize must be 512 for TFTP servers > * which do not support the RRQ blocksize option */ > blocksize = 512; > > /* Preferred blocksize - used as option for the read request */ > - if (_blocksize < 8) > - _blocksize = 8; > - else if (_blocksize > MAX_BLOCKSIZE) > - _blocksize = MAX_BLOCKSIZE; > - sprintf(blocksize_str, "%d", _blocksize); > + sprintf(blocksize_str, "%d", MAX_BLOCKSIZE); > > printf(" Receiving data: "); > print_progress(-1, 0); > diff --git a/lib/libnet/tftp.h b/lib/libnet/tftp.h > index e32e473..a09cf71 100644 > --- a/lib/libnet/tftp.h > +++ b/lib/libnet/tftp.h > @@ -41,7 +41,7 @@ typedef struct { > } tftp_err_t; > > int tftp(filename_ip_t *, unsigned char *, int, unsigned int, > - tftp_err_t *, int32_t mode, int32_t blocksize, int ip_version); > + tftp_err_t *, int ip_version); > > int32_t handle_tftp(int fd, uint8_t *, int32_t); > void handle_tftp_dun(uint8_t err_code); > diff --git a/slof/fs/packages/obp-tftp.fs b/slof/fs/packages/obp-tftp.fs > index 17fb980..19c11e1 100644 > --- a/slof/fs/packages/obp-tftp.fs > +++ b/slof/fs/packages/obp-tftp.fs > @@ -12,8 +12,6 @@ > > s" obp-tftp" device-name > > -VARIABLE huge-tftp-load 1 huge-tftp-load ! > - > : open ( -- okay? ) > true > ; > @@ -28,7 +26,6 @@ VARIABLE huge-tftp-load 1 huge-tftp-load ! > > 60000000 ( addr maxlen ) > > - huge-tftp-load @ d# 1428 ( addr maxlen hugetftp blocksize ) > \ Add OBP-TFTP Bootstring argument, e.g. "10.128.0.1,bootrom.bin,10.128.40.1" > my-args > net-load dup 0< IF drop 0 THEN
On 22.05.2018 16:49, Greg Kurz wrote: > On Fri, 18 May 2018 17:45:30 +0200 > Thomas Huth <thuth@redhat.com> wrote: > >> The blocksize is hard-coded to 1428 bytes in obp-tftp.fs, so instead of >> hardcoding this in the Forth code, we could also move this into tftp.c >> directly instead. A similar condition exists with the huge-tftp-load >> parameter. While this non-standard variable could still be changed in the >> obp-tftp package, it does not make much sense to set it to zero since you >> only lose the possibility to do huge TFTP loads with index wrap-around in >> that case. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> lib/libnet/libnet.code | 4 +--- >> lib/libnet/netapps.h | 3 +-- >> lib/libnet/netload.c | 11 ++++------- >> lib/libnet/tftp.c | 13 +++---------- >> lib/libnet/tftp.h | 2 +- >> slof/fs/packages/obp-tftp.fs | 3 --- >> 6 files changed, 10 insertions(+), 26 deletions(-) >> >> diff --git a/lib/libnet/libnet.code b/lib/libnet/libnet.code >> index 2746782..419419d 100644 >> --- a/lib/libnet/libnet.code >> +++ b/lib/libnet/libnet.code >> @@ -4,11 +4,9 @@ >> PRIM(NET_X2d_LOAD) >> int alen = TOS.n; POP; >> char *arg = TOS.a; POP; >> - int blocksize = TOS.n; POP; >> - int hugeload = TOS.n; POP; >> long maxlen = TOS.n; POP; >> void *loadaddr = TOS.a; >> - TOS.n = netload(loadaddr, maxlen, hugeload, blocksize, arg, alen); >> + TOS.n = netload(loadaddr, maxlen, arg, alen); >> MIRP >> >> PRIM(NET_X2d_PING) >> diff --git a/lib/libnet/netapps.h b/lib/libnet/netapps.h >> index 0e637e1..6e00466 100644 >> --- a/lib/libnet/netapps.h >> +++ b/lib/libnet/netapps.h >> @@ -18,8 +18,7 @@ >> >> struct filename_ip; >> >> -extern int netload(char *buffer, int len, int huge_load, int block_size, >> - char *args_fs, int alen); >> +extern int netload(char *buffer, int len, char *args_fs, int alen); >> extern int ping(char *args_fs, int alen); >> extern int dhcp(char *ret_buffer, struct filename_ip *fn_ip, >> unsigned int retries, int flags); >> diff --git a/lib/libnet/netload.c b/lib/libnet/netload.c >> index 5c37fe2..8dca654 100644 >> --- a/lib/libnet/netload.c >> +++ b/lib/libnet/netload.c >> @@ -405,13 +405,12 @@ static void seed_rng(uint8_t mac[]) >> } >> >> static int tftp_load(filename_ip_t *fnip, unsigned char *buffer, int len, >> - unsigned int retries, int32_t mode, >> - int32_t blksize, int ip_vers) >> + unsigned int retries, int ip_vers) >> { >> tftp_err_t tftp_err; >> int rc; >> >> - rc = tftp(fnip, buffer, len, retries, &tftp_err, mode, blksize, ip_vers); >> + rc = tftp(fnip, buffer, len, retries, &tftp_err, ip_vers); >> >> if (rc > 0) { >> printf(" TFTP: Received %s (%d KBytes)\n", fnip->filename, >> @@ -510,8 +509,7 @@ static void encode_response(char *pkt_buffer, size_t size, int ip_init) >> } >> } >> >> -int netload(char *buffer, int len, int huge_load, int block_size, >> - char *args_fs, int alen) >> +int netload(char *buffer, int len, char *args_fs, int alen) >> { >> int rc; >> filename_ip_t fn_ip; >> @@ -755,8 +753,7 @@ int netload(char *buffer, int len, int huge_load, int block_size, >> >> /* Do the TFTP load and print error message if necessary */ >> rc = tftp_load(&fn_ip, (unsigned char *)buffer, len, >> - obp_tftp_args.tftp_retries, huge_load, >> - block_size, ip_version); >> + obp_tftp_args.tftp_retries, ip_version); >> >> if (obp_tftp_args.ip_init == IP_INIT_DHCP) >> dhcp_send_release(fn_ip.fd); >> diff --git a/lib/libnet/tftp.c b/lib/libnet/tftp.c >> index 1656c27..5e7951f 100644 >> --- a/lib/libnet/tftp.c >> +++ b/lib/libnet/tftp.c >> @@ -497,19 +497,15 @@ void handle_tftp_dun(uint8_t err_code) >> * @param _len size of destination buffer >> * @param _retries max number of retries >> * @param _tftp_err contains info about TFTP-errors (e.g. lost packets) >> - * @param _mode NON ZERO - multicast, ZERO - unicast >> - * @param _blocksize blocksize for DATA-packets >> * @return ZERO - error condition occurs >> * NON ZERO - size of received file >> */ >> int tftp(filename_ip_t * _fn_ip, unsigned char *_buffer, int _len, >> - unsigned int _retries, tftp_err_t * _tftp_err, >> - int32_t _mode, int32_t _blocksize, int _ip_version) >> + unsigned int _retries, tftp_err_t * _tftp_err, int _ip_version) >> { >> retries = _retries; >> fn_ip = _fn_ip; >> len = _len; >> - huge_load = _mode; >> ip_version = _ip_version; >> tftp_errno = 0; >> tftp_err = _tftp_err; >> @@ -523,17 +519,14 @@ int tftp(filename_ip_t * _fn_ip, unsigned char *_buffer, int _len, >> port_number = -1; >> progress_first = -1; >> progress_last_bytes = 0; >> + huge_load = 1; >> > > This static variable seems to have only two users, both in handle_tftp(): > > else if( block == 0xffff && huge_load != 0 > && (tftp->th_data == 0 || tftp->th_data == 1) ) { > block = tftp->th_data; > } > > and > > if (block >= 0xffff && huge_load == 0) { > tftp_errno = -9; > goto error; > } > > which could be simplified as well, but this can be done later I guess. Yes, I thought about that, too, but I then decided that I'd rather leave that code there ... if somebody ever wonders whether SLOF supports TFTP loading with index roll-over (i.e. huge TFTP loads), it's more obvious to spot this in the sources if we keep that "huge_load" variable in there. > Reviewed-by: Greg Kurz <groug@kaod.org> Thanks! Thomas
On 19/5/18 1:45 am, Thomas Huth wrote: > The blocksize is hard-coded to 1428 bytes in obp-tftp.fs, so instead of > hardcoding this in the Forth code, we could also move this into tftp.c > directly instead. A similar condition exists with the huge-tftp-load > parameter. While this non-standard variable could still be changed in the > obp-tftp package, it does not make much sense to set it to zero since you > only lose the possibility to do huge TFTP loads with index wrap-around in > that case. > > Signed-off-by: Thomas Huth <thuth@redhat.com> Thanks, applied.
diff --git a/lib/libnet/libnet.code b/lib/libnet/libnet.code index 2746782..419419d 100644 --- a/lib/libnet/libnet.code +++ b/lib/libnet/libnet.code @@ -4,11 +4,9 @@ PRIM(NET_X2d_LOAD) int alen = TOS.n; POP; char *arg = TOS.a; POP; - int blocksize = TOS.n; POP; - int hugeload = TOS.n; POP; long maxlen = TOS.n; POP; void *loadaddr = TOS.a; - TOS.n = netload(loadaddr, maxlen, hugeload, blocksize, arg, alen); + TOS.n = netload(loadaddr, maxlen, arg, alen); MIRP PRIM(NET_X2d_PING) diff --git a/lib/libnet/netapps.h b/lib/libnet/netapps.h index 0e637e1..6e00466 100644 --- a/lib/libnet/netapps.h +++ b/lib/libnet/netapps.h @@ -18,8 +18,7 @@ struct filename_ip; -extern int netload(char *buffer, int len, int huge_load, int block_size, - char *args_fs, int alen); +extern int netload(char *buffer, int len, char *args_fs, int alen); extern int ping(char *args_fs, int alen); extern int dhcp(char *ret_buffer, struct filename_ip *fn_ip, unsigned int retries, int flags); diff --git a/lib/libnet/netload.c b/lib/libnet/netload.c index 5c37fe2..8dca654 100644 --- a/lib/libnet/netload.c +++ b/lib/libnet/netload.c @@ -405,13 +405,12 @@ static void seed_rng(uint8_t mac[]) } static int tftp_load(filename_ip_t *fnip, unsigned char *buffer, int len, - unsigned int retries, int32_t mode, - int32_t blksize, int ip_vers) + unsigned int retries, int ip_vers) { tftp_err_t tftp_err; int rc; - rc = tftp(fnip, buffer, len, retries, &tftp_err, mode, blksize, ip_vers); + rc = tftp(fnip, buffer, len, retries, &tftp_err, ip_vers); if (rc > 0) { printf(" TFTP: Received %s (%d KBytes)\n", fnip->filename, @@ -510,8 +509,7 @@ static void encode_response(char *pkt_buffer, size_t size, int ip_init) } } -int netload(char *buffer, int len, int huge_load, int block_size, - char *args_fs, int alen) +int netload(char *buffer, int len, char *args_fs, int alen) { int rc; filename_ip_t fn_ip; @@ -755,8 +753,7 @@ int netload(char *buffer, int len, int huge_load, int block_size, /* Do the TFTP load and print error message if necessary */ rc = tftp_load(&fn_ip, (unsigned char *)buffer, len, - obp_tftp_args.tftp_retries, huge_load, - block_size, ip_version); + obp_tftp_args.tftp_retries, ip_version); if (obp_tftp_args.ip_init == IP_INIT_DHCP) dhcp_send_release(fn_ip.fd); diff --git a/lib/libnet/tftp.c b/lib/libnet/tftp.c index 1656c27..5e7951f 100644 --- a/lib/libnet/tftp.c +++ b/lib/libnet/tftp.c @@ -497,19 +497,15 @@ void handle_tftp_dun(uint8_t err_code) * @param _len size of destination buffer * @param _retries max number of retries * @param _tftp_err contains info about TFTP-errors (e.g. lost packets) - * @param _mode NON ZERO - multicast, ZERO - unicast - * @param _blocksize blocksize for DATA-packets * @return ZERO - error condition occurs * NON ZERO - size of received file */ int tftp(filename_ip_t * _fn_ip, unsigned char *_buffer, int _len, - unsigned int _retries, tftp_err_t * _tftp_err, - int32_t _mode, int32_t _blocksize, int _ip_version) + unsigned int _retries, tftp_err_t * _tftp_err, int _ip_version) { retries = _retries; fn_ip = _fn_ip; len = _len; - huge_load = _mode; ip_version = _ip_version; tftp_errno = 0; tftp_err = _tftp_err; @@ -523,17 +519,14 @@ int tftp(filename_ip_t * _fn_ip, unsigned char *_buffer, int _len, port_number = -1; progress_first = -1; progress_last_bytes = 0; + huge_load = 1; /* Default blocksize must be 512 for TFTP servers * which do not support the RRQ blocksize option */ blocksize = 512; /* Preferred blocksize - used as option for the read request */ - if (_blocksize < 8) - _blocksize = 8; - else if (_blocksize > MAX_BLOCKSIZE) - _blocksize = MAX_BLOCKSIZE; - sprintf(blocksize_str, "%d", _blocksize); + sprintf(blocksize_str, "%d", MAX_BLOCKSIZE); printf(" Receiving data: "); print_progress(-1, 0); diff --git a/lib/libnet/tftp.h b/lib/libnet/tftp.h index e32e473..a09cf71 100644 --- a/lib/libnet/tftp.h +++ b/lib/libnet/tftp.h @@ -41,7 +41,7 @@ typedef struct { } tftp_err_t; int tftp(filename_ip_t *, unsigned char *, int, unsigned int, - tftp_err_t *, int32_t mode, int32_t blocksize, int ip_version); + tftp_err_t *, int ip_version); int32_t handle_tftp(int fd, uint8_t *, int32_t); void handle_tftp_dun(uint8_t err_code); diff --git a/slof/fs/packages/obp-tftp.fs b/slof/fs/packages/obp-tftp.fs index 17fb980..19c11e1 100644 --- a/slof/fs/packages/obp-tftp.fs +++ b/slof/fs/packages/obp-tftp.fs @@ -12,8 +12,6 @@ s" obp-tftp" device-name -VARIABLE huge-tftp-load 1 huge-tftp-load ! - : open ( -- okay? ) true ; @@ -28,7 +26,6 @@ VARIABLE huge-tftp-load 1 huge-tftp-load ! 60000000 ( addr maxlen ) - huge-tftp-load @ d# 1428 ( addr maxlen hugetftp blocksize ) \ Add OBP-TFTP Bootstring argument, e.g. "10.128.0.1,bootrom.bin,10.128.40.1" my-args net-load dup 0< IF drop 0 THEN
The blocksize is hard-coded to 1428 bytes in obp-tftp.fs, so instead of hardcoding this in the Forth code, we could also move this into tftp.c directly instead. A similar condition exists with the huge-tftp-load parameter. While this non-standard variable could still be changed in the obp-tftp package, it does not make much sense to set it to zero since you only lose the possibility to do huge TFTP loads with index wrap-around in that case. Signed-off-by: Thomas Huth <thuth@redhat.com> --- lib/libnet/libnet.code | 4 +--- lib/libnet/netapps.h | 3 +-- lib/libnet/netload.c | 11 ++++------- lib/libnet/tftp.c | 13 +++---------- lib/libnet/tftp.h | 2 +- slof/fs/packages/obp-tftp.fs | 3 --- 6 files changed, 10 insertions(+), 26 deletions(-)