[SLOF] [PATCH v2 03/11] libnet: Pass tftp_retries and ip_version via struct filename_ip

[Alexey Kardashevskiy]

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 at kaod.org>
>>> Signed-off-by: Thomas Huth <thuth at 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.


-- 
Alexey