[SLOF] [PATCH v2 04/11] libnet: Put code for determing TFTP error strings into a separate function

[Thomas Huth]

On 24.05.2018 11:01, Alexey Kardashevskiy wrote:
> On 19/5/18 1:45 am, Thomas Huth wrote:
>> This way we can easily re-use the rc --> string translation in later
>> patches.
>>
>> Reviewed-by: Greg Kurz <groug at kaod.org>
>> Signed-off-by: Thomas Huth <thuth at redhat.com>
>> ---
>>  lib/libnet/netload.c |  79 +++------------------------------------
>>  lib/libnet/tftp.c    | 103 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  lib/libnet/tftp.h    |   2 +
>>  3 files changed, 111 insertions(+), 73 deletions(-)
[...]
>> diff --git a/lib/libnet/tftp.c b/lib/libnet/tftp.c
>> index b7121fe..ec8d631 100644
>> --- a/lib/libnet/tftp.c
>> +++ b/lib/libnet/tftp.c
>> @@ -690,3 +690,106 @@ int parse_tftp_args(char buffer[], char *server_ip, char filename[], int fd,
>>  		return 0;
>>  	}
>>  }
>> +
>> +int tftp_get_error_info(filename_ip_t *fnip, tftp_err_t *tftperr, int rc,
> 
> tftperr is not used.

Oops, good catch, I missed to rename tftp_err to tftperr later in this
function, so this accidentially used the "global" tftp_err of tftp.c :-(
... I'll fix it in v3.

> 
>> +                        const char **errstr, int *ecode)
>> +{
>> +	static char estrbuf[80];
> 
> 
> 
>> +	int dummy;
>> +
>> +	if (!ecode)
>> +		ecode = &dummy;
> 
> Nit: the other caller could also allocate this on the stack.

Ok.

>> +
>> +	if (rc == -1) {
>> +		*ecode = 0x3003;
>> +		*errstr = "unknown TFTP error";
>> +		return -103;
>> +	} else if (rc == -2) {
>> +		*ecode = 0x3004;
>> +		sprintf(estrbuf, "TFTP buffer of %d bytes is too small for %s",
>> +			len, fnip->filename);
>> +		*errstr = estrbuf;
>> +		return -104;
>> +	} else if (rc == -3) {
>> +		*ecode = 0x3009;
>> +		sprintf(estrbuf, "file not found: %s", fnip->filename);
> 
> estrbuf is 80 bytes, filename_ip::filename is 256. vsnprintf or increase
> the estrbuf size or simply malloc it.

I'd like to avoid malloc() here ... otherwise I'd have to put a strdup()
around all the other strings and free() the memory in the caller again.
Since most of the strings are static here, and the malloc()
implementation in SLOF's libc is certainly not the best (it does not
defragment the memory during free()), I think it's better to keep the
"static" strings here.

So I think I'll either increase the size of the buffer, or switch to
snprintf (or strcpy + strncat) here instead.

>> +		*errstr = estrbuf;
>> +		return -108;
>> +	} else if (rc == -4) {
>> +		*ecode = 0x3010;
>> +		*errstr = "TFTP access violation";
>> +		return -109;
>> +	} else if (rc == -5) {
>> +		*ecode = 0x3011;
>> +		*errstr = "illegal TFTP operation";
>> +		return -110;
>> +	} else if (rc == -6) {
>> +		*ecode = 0x3012;
>> +		*errstr = "unknown TFTP transfer ID";
>> +		return -111;
>> +	} else if (rc == -7) {
>> +		*ecode = 0x3013;
>> +		*errstr = "no such TFTP user";
>> +		return -112;
>> +	} else if (rc == -8) {
>> +		*ecode = 0x3017;
>> +		*errstr = "TFTP blocksize negotiation failed";
>> +		return -116;
>> +	} else if (rc == -9) {
>> +		*ecode = 0x3018;
>> +		*errstr = "file exceeds maximum TFTP transfer size";
>> +		return -117;
>> +	} else if (rc <= -10 && rc >= -15) {
>> +		const char *icmp_err_str;
>> +		switch (rc) {
>> +		case -ICMP_NET_UNREACHABLE - 10:
>> +			icmp_err_str = "net unreachable";
>> +			break;
>> +		case -ICMP_HOST_UNREACHABLE - 10:
>> +			icmp_err_str = "host unreachable";
>> +			break;
>> +		case -ICMP_PROTOCOL_UNREACHABLE - 10:
>> +			icmp_err_str = "protocol unreachable";
>> +			break;
>> +		case -ICMP_PORT_UNREACHABLE - 10:
>> +			icmp_err_str = "port unreachable";
>> +			break;
>> +		case -ICMP_FRAGMENTATION_NEEDED - 10:
>> +			icmp_err_str = "fragmentation needed and DF set";
>> +			break;
>> +		case -ICMP_SOURCE_ROUTE_FAILED - 10:
>> +			icmp_err_str = "source route failed";
>> +			break;
>> +		default:
>> +			icmp_err_str = "UNKNOWN";
>> +			break;
>> +		}
>> +		*ecode = 0x3005;
>> +		sprintf(estrbuf, "ICMP ERROR \"%s\"", icmp_err_str);
>> +		*errstr = estrbuf;
>> +		return -105;
>> +	} else if (rc == -40) {
>> +		*ecode = 0x3014;
>> +		sprintf(estrbuf,
>> +			"TFTP error occurred after %d bad packets received",
>> +			tftp_err->bad_tftp_packets);
>> +		*errstr = estrbuf;
>> +		return -113;
>> +	} else if (rc == -41) {
>> +		*ecode = 0x3015;
>> +		sprintf(estrbuf,
>> +			"TFTP error occurred after missing %d responses",
>> +			tftp_err->no_packets);
>> +		*errstr = estrbuf;
>> +		return -114;
>> +	} else if (rc == -42) {
>> +		*ecode = 0x3016;
>> +		sprintf(estrbuf,
>> +			"TFTP error missing block %d, expected block was %d",
>> +			tftp_err->blocks_missed, tftp_err->blocks_received);

That should have been tftperr instead of tftp_err ...

>> +		*errstr = estrbuf;
>> +		return -115;
>> +	}
>> +
>> +	return rc;
>> +}
>> diff --git a/lib/libnet/tftp.h b/lib/libnet/tftp.h
>> index c94c94d..da743d3 100644
>> --- a/lib/libnet/tftp.h
>> +++ b/lib/libnet/tftp.h
>> @@ -46,5 +46,7 @@ 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);
>> +int tftp_get_error_info(filename_ip_t *fnip, tftp_err_t *tftperr, int rc,
>> +                        const char **errstr, int *ecode);
>>  
>>  #endif

 Thomas