Message ID | 1526658340-1992-4-git-send-email-thuth@redhat.com |
---|---|
State | Superseded |
Headers | show |
Series | Support network booting with pxelinux.cfg files | expand |
On 19/5/18 1:45 am, Thomas Huth wrote: > When we will support loading of pxelinux.cfg files later, we have to call > the tftp load function multiple times from different places. To avoid that > we've also got to pass around the tftp_retries and ip_version information > via function parameters to all spots, let's rather put them into struct > filename_ip since we've got this struct filename_ip info available every- > where already. > While we're at it, also drop the __attribute__((packed)) from the struct. > The struct is only used internally, without exchanging it with the outside > world, so the attribute is certainly not necessary here. > > Reviewed-by: Greg Kurz <groug@kaod.org> > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > lib/libnet/netload.c | 11 ++++++----- > lib/libnet/tftp.c | 6 +++--- > lib/libnet/tftp.h | 10 +++++----- > 3 files changed, 14 insertions(+), 13 deletions(-) > > diff --git a/lib/libnet/netload.c b/lib/libnet/netload.c > index 8dca654..407d173 100644 > --- a/lib/libnet/netload.c > +++ b/lib/libnet/netload.c > @@ -404,13 +404,12 @@ static void seed_rng(uint8_t mac[]) > srand(seed); > } > > -static int tftp_load(filename_ip_t *fnip, unsigned char *buffer, int len, > - unsigned int retries, int ip_vers) > +static int tftp_load(filename_ip_t *fnip, void *buffer, int len) > { > tftp_err_t tftp_err; > int rc; > > - rc = tftp(fnip, buffer, len, retries, &tftp_err, ip_vers); > + rc = tftp(fnip, buffer, len, &tftp_err); > > if (rc > 0) { > printf(" TFTP: Received %s (%d KBytes)\n", fnip->filename, > @@ -737,6 +736,9 @@ int netload(char *buffer, int len, char *args_fs, int alen) > fn_ip.filename[sizeof(fn_ip.filename)-1] = 0; > } > > + fn_ip.ip_version = ip_version; > + fn_ip.tftp_retries = obp_tftp_args.tftp_retries; > + > if (ip_version == 4) { > printf(" Requesting file \"%s\" via TFTP from %d.%d.%d.%d\n", > fn_ip.filename, > @@ -752,8 +754,7 @@ int netload(char *buffer, int len, char *args_fs, int alen) > } > > /* Do the TFTP load and print error message if necessary */ > - rc = tftp_load(&fn_ip, (unsigned char *)buffer, len, > - obp_tftp_args.tftp_retries, ip_version); > + rc = tftp_load(&fn_ip, buffer, len); > > 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 5e7951f..b7121fe 100644 > --- a/lib/libnet/tftp.c > +++ b/lib/libnet/tftp.c > @@ -501,12 +501,12 @@ void handle_tftp_dun(uint8_t err_code) > * 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, int _ip_version) > + tftp_err_t * _tftp_err) > { > - retries = _retries; > + retries = _fn_ip->tftp_retries; > fn_ip = _fn_ip; > len = _len; > - ip_version = _ip_version; > + ip_version = _fn_ip->ip_version; > tftp_errno = 0; > tftp_err = _tftp_err; > tftp_err->bad_tftp_packets = 0; > diff --git a/lib/libnet/tftp.h b/lib/libnet/tftp.h > index a09cf71..c94c94d 100644 > --- a/lib/libnet/tftp.h > +++ b/lib/libnet/tftp.h > @@ -29,8 +29,10 @@ struct filename_ip { > ip6_addr_t server_ip6; > ip6_addr_t dns_ip6; > char filename[256]; > - int fd; > -} __attribute__ ((packed)); > + int fd; > + int ip_version; This one makes sense. > + int tftp_retries; This one does not. It is filename_ip, not obp_tftp_args or anything else with "_args" in the end. The mac address (from pxelinux_load_cfg, for example) would make more sense as a part of filename_ip imho. > +}; > typedef struct filename_ip filename_ip_t; > > typedef struct { > @@ -40,9 +42,7 @@ typedef struct { > uint32_t blocks_received; > } tftp_err_t; > > -int tftp(filename_ip_t *, unsigned char *, int, unsigned int, > - tftp_err_t *, int ip_version); > - > +int tftp(filename_ip_t *fnip, unsigned char *buf, int len, tftp_err_t *err); > int32_t handle_tftp(int fd, uint8_t *, int32_t); > void handle_tftp_dun(uint8_t err_code); > int parse_tftp_args(char buffer[], char *server_ip, char filename[], int fd, int len); >
On 24.05.2018 11:01, Alexey Kardashevskiy wrote: > On 19/5/18 1:45 am, Thomas Huth wrote: >> When we will support loading of pxelinux.cfg files later, we have to call >> the tftp load function multiple times from different places. To avoid that >> we've also got to pass around the tftp_retries and ip_version information >> via function parameters to all spots, let's rather put them into struct >> filename_ip since we've got this struct filename_ip info available every- >> where already. >> While we're at it, also drop the __attribute__((packed)) from the struct. >> The struct is only used internally, without exchanging it with the outside >> world, so the attribute is certainly not necessary here. >> >> Reviewed-by: Greg Kurz <groug@kaod.org> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> lib/libnet/netload.c | 11 ++++++----- >> lib/libnet/tftp.c | 6 +++--- >> lib/libnet/tftp.h | 10 +++++----- >> 3 files changed, 14 insertions(+), 13 deletions(-) >> >> diff --git a/lib/libnet/netload.c b/lib/libnet/netload.c >> index 8dca654..407d173 100644 >> --- a/lib/libnet/netload.c >> +++ b/lib/libnet/netload.c >> @@ -404,13 +404,12 @@ static void seed_rng(uint8_t mac[]) >> srand(seed); >> } >> >> -static int tftp_load(filename_ip_t *fnip, unsigned char *buffer, int len, >> - unsigned int retries, int ip_vers) >> +static int tftp_load(filename_ip_t *fnip, void *buffer, int len) >> { >> tftp_err_t tftp_err; >> int rc; >> >> - rc = tftp(fnip, buffer, len, retries, &tftp_err, ip_vers); >> + rc = tftp(fnip, buffer, len, &tftp_err); >> >> if (rc > 0) { >> printf(" TFTP: Received %s (%d KBytes)\n", fnip->filename, >> @@ -737,6 +736,9 @@ int netload(char *buffer, int len, char *args_fs, int alen) >> fn_ip.filename[sizeof(fn_ip.filename)-1] = 0; >> } >> >> + fn_ip.ip_version = ip_version; >> + fn_ip.tftp_retries = obp_tftp_args.tftp_retries; >> + >> if (ip_version == 4) { >> printf(" Requesting file \"%s\" via TFTP from %d.%d.%d.%d\n", >> fn_ip.filename, >> @@ -752,8 +754,7 @@ int netload(char *buffer, int len, char *args_fs, int alen) >> } >> >> /* Do the TFTP load and print error message if necessary */ >> - rc = tftp_load(&fn_ip, (unsigned char *)buffer, len, >> - obp_tftp_args.tftp_retries, ip_version); >> + rc = tftp_load(&fn_ip, buffer, len); >> >> 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 5e7951f..b7121fe 100644 >> --- a/lib/libnet/tftp.c >> +++ b/lib/libnet/tftp.c >> @@ -501,12 +501,12 @@ void handle_tftp_dun(uint8_t err_code) >> * 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, int _ip_version) >> + tftp_err_t * _tftp_err) >> { >> - retries = _retries; >> + retries = _fn_ip->tftp_retries; >> fn_ip = _fn_ip; >> len = _len; >> - ip_version = _ip_version; >> + ip_version = _fn_ip->ip_version; >> tftp_errno = 0; >> tftp_err = _tftp_err; >> tftp_err->bad_tftp_packets = 0; >> diff --git a/lib/libnet/tftp.h b/lib/libnet/tftp.h >> index a09cf71..c94c94d 100644 >> --- a/lib/libnet/tftp.h >> +++ b/lib/libnet/tftp.h >> @@ -29,8 +29,10 @@ struct filename_ip { >> ip6_addr_t server_ip6; >> ip6_addr_t dns_ip6; >> char filename[256]; >> - int fd; >> -} __attribute__ ((packed)); >> + int fd; >> + int ip_version; > > This one makes sense. > >> + int tftp_retries; > > This one does not. It is filename_ip, not obp_tftp_args or anything else > with "_args" in the end. obp_tftp_args is only there for the parameter parsing of the string that is passed from obp-tftp Open Firmware package to the libnet code. It is strictly limited to the netload.c file, and it is for example also not used at all by the netload code of the s390x firmware in QEMU (which uses the other parts of SLOF's libnet code). So it would not make sense to pass obp_tftp_args to the code in tftp.c. If you're confused by the "filename_ip" name, we could maybe simply rename that one to "netload_params" or "netload_args" instead ... or I could simply revert that change in my patches so that we continue to pass the tftp retries variable as normal function parameter instead... just let me know what you prefer. > The mac address (from pxelinux_load_cfg, for example) would make more sense > as a part of filename_ip imho. Yes, maybe ... but it didn't annoy me as much as the tftp_retries when I reworked the code, so unless you really want me to include it into the structure here, I'd rather keep it separated for now. Thomas
On 25/5/18 12:26 am, Thomas Huth wrote: > On 24.05.2018 11:01, Alexey Kardashevskiy wrote: >> On 19/5/18 1:45 am, Thomas Huth wrote: >>> When we will support loading of pxelinux.cfg files later, we have to call >>> the tftp load function multiple times from different places. To avoid that >>> we've also got to pass around the tftp_retries and ip_version information >>> via function parameters to all spots, let's rather put them into struct >>> filename_ip since we've got this struct filename_ip info available every- >>> where already. >>> While we're at it, also drop the __attribute__((packed)) from the struct. >>> The struct is only used internally, without exchanging it with the outside >>> world, so the attribute is certainly not necessary here. >>> >>> Reviewed-by: Greg Kurz <groug@kaod.org> >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> --- >>> lib/libnet/netload.c | 11 ++++++----- >>> lib/libnet/tftp.c | 6 +++--- >>> lib/libnet/tftp.h | 10 +++++----- >>> 3 files changed, 14 insertions(+), 13 deletions(-) >>> >>> diff --git a/lib/libnet/netload.c b/lib/libnet/netload.c >>> index 8dca654..407d173 100644 >>> --- a/lib/libnet/netload.c >>> +++ b/lib/libnet/netload.c >>> @@ -404,13 +404,12 @@ static void seed_rng(uint8_t mac[]) >>> srand(seed); >>> } >>> >>> -static int tftp_load(filename_ip_t *fnip, unsigned char *buffer, int len, >>> - unsigned int retries, int ip_vers) >>> +static int tftp_load(filename_ip_t *fnip, void *buffer, int len) >>> { >>> tftp_err_t tftp_err; >>> int rc; >>> >>> - rc = tftp(fnip, buffer, len, retries, &tftp_err, ip_vers); >>> + rc = tftp(fnip, buffer, len, &tftp_err); >>> >>> if (rc > 0) { >>> printf(" TFTP: Received %s (%d KBytes)\n", fnip->filename, >>> @@ -737,6 +736,9 @@ int netload(char *buffer, int len, char *args_fs, int alen) >>> fn_ip.filename[sizeof(fn_ip.filename)-1] = 0; >>> } >>> >>> + fn_ip.ip_version = ip_version; >>> + fn_ip.tftp_retries = obp_tftp_args.tftp_retries; >>> + >>> if (ip_version == 4) { >>> printf(" Requesting file \"%s\" via TFTP from %d.%d.%d.%d\n", >>> fn_ip.filename, >>> @@ -752,8 +754,7 @@ int netload(char *buffer, int len, char *args_fs, int alen) >>> } >>> >>> /* Do the TFTP load and print error message if necessary */ >>> - rc = tftp_load(&fn_ip, (unsigned char *)buffer, len, >>> - obp_tftp_args.tftp_retries, ip_version); >>> + rc = tftp_load(&fn_ip, buffer, len); >>> >>> 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 5e7951f..b7121fe 100644 >>> --- a/lib/libnet/tftp.c >>> +++ b/lib/libnet/tftp.c >>> @@ -501,12 +501,12 @@ void handle_tftp_dun(uint8_t err_code) >>> * 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, int _ip_version) >>> + tftp_err_t * _tftp_err) >>> { >>> - retries = _retries; >>> + retries = _fn_ip->tftp_retries; >>> fn_ip = _fn_ip; >>> len = _len; >>> - ip_version = _ip_version; >>> + ip_version = _fn_ip->ip_version; >>> tftp_errno = 0; >>> tftp_err = _tftp_err; >>> tftp_err->bad_tftp_packets = 0; >>> diff --git a/lib/libnet/tftp.h b/lib/libnet/tftp.h >>> index a09cf71..c94c94d 100644 >>> --- a/lib/libnet/tftp.h >>> +++ b/lib/libnet/tftp.h >>> @@ -29,8 +29,10 @@ struct filename_ip { >>> ip6_addr_t server_ip6; >>> ip6_addr_t dns_ip6; >>> char filename[256]; >>> - int fd; >>> -} __attribute__ ((packed)); >>> + int fd; >>> + int ip_version; >> >> This one makes sense. >> >>> + int tftp_retries; >> >> This one does not. It is filename_ip, not obp_tftp_args or anything else >> with "_args" in the end. > > obp_tftp_args is only there for the parameter parsing of the string that > is passed from obp-tftp Open Firmware package to the libnet code. It is > strictly limited to the netload.c file, and it is for example also not > used at all by the netload code of the s390x firmware in QEMU (which > uses the other parts of SLOF's libnet code). So it would not make sense > to pass obp_tftp_args to the code in tftp.c. I was not suggesting reuse, I just used it as an example of a slightly more informative name ;) btw obp_tftp_args_t::filename[100] vs. filename_ip::filename[256] - it feels like the latter can be easily reduced to 140-ish (path + uuid) with no harm but probably this is for another day (both definitions came in the initial commit). > If you're confused by the "filename_ip" name, we could maybe simply > rename that one to "netload_params" or "netload_args" instead ... or I > could simply revert that change in my patches so that we continue to > pass the tftp retries variable as normal function parameter instead... > just let me know what you prefer. Passing "retries" separately seems better. >> The mac address (from pxelinux_load_cfg, for example) would make more sense >> as a part of filename_ip imho. > > Yes, maybe ... but it didn't annoy me as much as the tftp_retries when I > reworked the code, so unless you really want me to include it into the > structure here, I'd rather keep it separated for now. Sure.
On 25.05.2018 04:57, Alexey Kardashevskiy wrote: > On 25/5/18 12:26 am, Thomas Huth wrote: >> On 24.05.2018 11:01, Alexey Kardashevskiy wrote: [...] > btw obp_tftp_args_t::filename[100] vs. filename_ip::filename[256] - it > feels like the latter can be easily reduced to 140-ish (path + uuid) with > no harm but probably this is for another day (both definitions came in the > initial commit). filename_ip::filename can also come from the DHCP code, so decreasing its size might change the behavior wrt to DHCP. >> If you're confused by the "filename_ip" name, we could maybe simply >> rename that one to "netload_params" or "netload_args" instead ... or I >> could simply revert that change in my patches so that we continue to >> pass the tftp retries variable as normal function parameter instead... >> just let me know what you prefer. > > Passing "retries" separately seems better. Ok, will do. Thomas
On 25/5/18 2:28 pm, Thomas Huth wrote: > On 25.05.2018 04:57, Alexey Kardashevskiy wrote: >> On 25/5/18 12:26 am, Thomas Huth wrote: >>> On 24.05.2018 11:01, Alexey Kardashevskiy wrote: > [...] >> btw obp_tftp_args_t::filename[100] vs. filename_ip::filename[256] - it >> feels like the latter can be easily reduced to 140-ish (path + uuid) with >> no harm but probably this is for another day (both definitions came in the >> initial commit). > > filename_ip::filename can also come from the DHCP code, so decreasing Where else can it come from? And what can use it? TFTP can but what else? > its size might change the behavior wrt to DHCP.
On 25.05.2018 07:01, Alexey Kardashevskiy wrote: > On 25/5/18 2:28 pm, Thomas Huth wrote: >> On 25.05.2018 04:57, Alexey Kardashevskiy wrote: >>> On 25/5/18 12:26 am, Thomas Huth wrote: >>>> On 24.05.2018 11:01, Alexey Kardashevskiy wrote: >> [...] >>> btw obp_tftp_args_t::filename[100] vs. filename_ip::filename[256] - it >>> feels like the latter can be easily reduced to 140-ish (path + uuid) with >>> no harm but probably this is for another day (both definitions came in the >>> initial commit). >> >> filename_ip::filename can also come from the DHCP code, so decreasing > > Where else can it come from? And what can use it? TFTP can but what else? dhcpv4() in dhcp.c and dhcp6_process_options() in dhcpv6.c can set it, too. tftp.c and pxelinux.c are the only consumers, as far as I know. Thomas
On 25/5/18 3:08 pm, Thomas Huth wrote: > On 25.05.2018 07:01, Alexey Kardashevskiy wrote: >> On 25/5/18 2:28 pm, Thomas Huth wrote: >>> On 25.05.2018 04:57, Alexey Kardashevskiy wrote: >>>> On 25/5/18 12:26 am, Thomas Huth wrote: >>>>> On 24.05.2018 11:01, Alexey Kardashevskiy wrote: >>> [...] >>>> btw obp_tftp_args_t::filename[100] vs. filename_ip::filename[256] - it >>>> feels like the latter can be easily reduced to 140-ish (path + uuid) with >>>> no harm but probably this is for another day (both definitions came in the >>>> initial commit). >>> >>> filename_ip::filename can also come from the DHCP code, so decreasing >> >> Where else can it come from? And what can use it? TFTP can but what else? > > dhcpv4() in dhcp.c and dhcp6_process_options() in dhcpv6.c can set it, > too. tftp.c and pxelinux.c are the only consumers, as far as I know. My point is if consumers cannot use all 256 bytes, then there is no point having this much in filename_ip. This is not to fix now though, I am just commenting.
On 25.05.2018 07:11, Alexey Kardashevskiy wrote: > On 25/5/18 3:08 pm, Thomas Huth wrote: >> On 25.05.2018 07:01, Alexey Kardashevskiy wrote: >>> On 25/5/18 2:28 pm, Thomas Huth wrote: >>>> On 25.05.2018 04:57, Alexey Kardashevskiy wrote: >>>>> On 25/5/18 12:26 am, Thomas Huth wrote: >>>>>> On 24.05.2018 11:01, Alexey Kardashevskiy wrote: >>>> [...] >>>>> btw obp_tftp_args_t::filename[100] vs. filename_ip::filename[256] - it >>>>> feels like the latter can be easily reduced to 140-ish (path + uuid) with >>>>> no harm but probably this is for another day (both definitions came in the >>>>> initial commit). >>>> >>>> filename_ip::filename can also come from the DHCP code, so decreasing >>> >>> Where else can it come from? And what can use it? TFTP can but what else? >> >> dhcpv4() in dhcp.c and dhcp6_process_options() in dhcpv6.c can set it, >> too. tftp.c and pxelinux.c are the only consumers, as far as I know. > > My point is if consumers cannot use all 256 bytes, then there is no point > having this much in filename_ip. This is not to fix now though, I am just > commenting. As far as I can see, the boot file name in dhcpv4 can be up to 255 bytes, and the one from dhcpv6 could even be longer, so 256 sounds like an adequate size for filename to me. Thomas
diff --git a/lib/libnet/netload.c b/lib/libnet/netload.c index 8dca654..407d173 100644 --- a/lib/libnet/netload.c +++ b/lib/libnet/netload.c @@ -404,13 +404,12 @@ static void seed_rng(uint8_t mac[]) srand(seed); } -static int tftp_load(filename_ip_t *fnip, unsigned char *buffer, int len, - unsigned int retries, int ip_vers) +static int tftp_load(filename_ip_t *fnip, void *buffer, int len) { tftp_err_t tftp_err; int rc; - rc = tftp(fnip, buffer, len, retries, &tftp_err, ip_vers); + rc = tftp(fnip, buffer, len, &tftp_err); if (rc > 0) { printf(" TFTP: Received %s (%d KBytes)\n", fnip->filename, @@ -737,6 +736,9 @@ int netload(char *buffer, int len, char *args_fs, int alen) fn_ip.filename[sizeof(fn_ip.filename)-1] = 0; } + fn_ip.ip_version = ip_version; + fn_ip.tftp_retries = obp_tftp_args.tftp_retries; + if (ip_version == 4) { printf(" Requesting file \"%s\" via TFTP from %d.%d.%d.%d\n", fn_ip.filename, @@ -752,8 +754,7 @@ int netload(char *buffer, int len, char *args_fs, int alen) } /* Do the TFTP load and print error message if necessary */ - rc = tftp_load(&fn_ip, (unsigned char *)buffer, len, - obp_tftp_args.tftp_retries, ip_version); + rc = tftp_load(&fn_ip, buffer, len); 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 5e7951f..b7121fe 100644 --- a/lib/libnet/tftp.c +++ b/lib/libnet/tftp.c @@ -501,12 +501,12 @@ void handle_tftp_dun(uint8_t err_code) * 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, int _ip_version) + tftp_err_t * _tftp_err) { - retries = _retries; + retries = _fn_ip->tftp_retries; fn_ip = _fn_ip; len = _len; - ip_version = _ip_version; + ip_version = _fn_ip->ip_version; tftp_errno = 0; tftp_err = _tftp_err; tftp_err->bad_tftp_packets = 0; diff --git a/lib/libnet/tftp.h b/lib/libnet/tftp.h index a09cf71..c94c94d 100644 --- a/lib/libnet/tftp.h +++ b/lib/libnet/tftp.h @@ -29,8 +29,10 @@ struct filename_ip { ip6_addr_t server_ip6; ip6_addr_t dns_ip6; char filename[256]; - int fd; -} __attribute__ ((packed)); + int fd; + int ip_version; + int tftp_retries; +}; typedef struct filename_ip filename_ip_t; typedef struct { @@ -40,9 +42,7 @@ typedef struct { uint32_t blocks_received; } tftp_err_t; -int tftp(filename_ip_t *, unsigned char *, int, unsigned int, - tftp_err_t *, int ip_version); - +int tftp(filename_ip_t *fnip, unsigned char *buf, int len, tftp_err_t *err); int32_t handle_tftp(int fd, uint8_t *, int32_t); void handle_tftp_dun(uint8_t err_code); int parse_tftp_args(char buffer[], char *server_ip, char filename[], int fd, int len);