Re: [PATCH] Pass "err" argument by address to "_nsError" function

["Mohamed Abuelfotoh, Hazem" <abuehaze@amazon.com>]

Hi Pratyush,

On 17/02/2025 13:03, Pratyush Yadav wrote:
> Hi Hazem,
> 
> On Mon, Feb 17 2025, Hazem Mohamed Abuelfotoh wrote:
> 
>> Commit 0d71523ab584 (“DNS: Support AFS SRV records and
>> cell db config files”) has refactored the "nsError" function
>> by moving some of error handling to "_nsError" function
>> however we are passing the "err" argument to "_nsError"
>> by value not by address which is wrong as that basically
>> waste any processing we do in the "_nsError" function
>> so correcting that by passing "err" by address.
>>
>> Reported-by: Pratyush Yadav <ptyadav@amazon.com>
>> Signed-off-by: Hazem Mohamed Abuelfotoh <abuehaze@amazon.com>
>> ---
>>   dns.afsdb.c        |  4 ++--
>>   key.dns.h          |  2 +-
>>   key.dns_resolver.c | 20 ++++++++++----------
>>   3 files changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/dns.afsdb.c b/dns.afsdb.c
>> index 986c0f3..7bffb60 100644
>> --- a/dns.afsdb.c
>> +++ b/dns.afsdb.c
>> @@ -228,7 +228,7 @@ static int dns_query_AFSDB(const char *cell)
>>   
>>   	if (response_len < 0) {
>>   		/* negative result */
>> -		_nsError(h_errno, cell);
>> +		_nsError(&h_errno, cell);
>>   		return -1;
>>   	}
>>   
>> @@ -267,7 +267,7 @@ static int dns_query_VL_SRV(const char *cell)
>>   
>>   	if (response_len < 0) {
>>   		/* negative result */
>> -		_nsError(h_errno, cell);
>> +		_nsError(&h_errno, cell);
>>   		return -1;
>>   	}
>>   
>> diff --git a/key.dns.h b/key.dns.h
>> index 33d0ab3..2fedbc3 100644
>> --- a/key.dns.h
>> +++ b/key.dns.h
>> @@ -59,7 +59,7 @@ extern __attribute__((format(printf, 1, 2)))
>>   void info(const char *fmt, ...);
>>   extern __attribute__((noreturn))
>>   void nsError(int err, const char *domain);
>> -extern void _nsError(int err, const char *domain);
>> +extern void _nsError(int *err, const char *domain);
>>   extern __attribute__((format(printf, 1, 2)))
>>   void debug(const char *fmt, ...);
>>   
>> diff --git a/key.dns_resolver.c b/key.dns_resolver.c
>> index 7a7ec42..6b16427 100644
>> --- a/key.dns_resolver.c
>> +++ b/key.dns_resolver.c
>> @@ -157,19 +157,20 @@ static const int ns_errno_map[] = {
>>   	[NO_DATA]		= ENODATA,
>>   };
>>   
>> -void _nsError(int err, const char *domain)
>> +void _nsError(int *err, const char *domain)
>>   {
>>   	if (isatty(2))
>> -		fprintf(stderr, "NS:%s: %s.\n", domain, hstrerror(err));
>> +		fprintf(stderr, "NS:%s: %s.\n", domain, hstrerror(*err));
>>   	else
>> -		syslog(LOG_INFO, "%s: %s", domain, hstrerror(err));
>> +		syslog(LOG_INFO, "%s: %s", domain, hstrerror(*err));
>>   
>> -	if (err >= sizeof(ns_errno_map) / sizeof(ns_errno_map[0]))
>> -		err = ECONNREFUSED;
>> -	else
>> -		err = ns_errno_map[err];
>> +	if (*err >= sizeof(ns_errno_map) / sizeof(ns_errno_map[0]))
>> +		*err = ECONNREFUSED;
>> +	else{
>> +		*err = ns_errno_map[*err];
>> +	}
>>   
>> -	info("Reject the key with error %d", err);
>> +	info("Reject the key with error %d", *err);
>>   }
>>   
>>   void nsError(int err, const char *domain)
>> @@ -177,8 +178,7 @@ void nsError(int err, const char *domain)
>>   	unsigned timeout;
>>   	int ret;
>>   
>> -	_nsError(err, domain);
>> -
>> +	_nsError(&err, domain);
> 
> Doing this breaks the switch block below since it checks against the
> h_errno instead of the errno that err now contains. So it would end up
> always using the default case. So we should either update the switch
> block below or make _nsError() return the errno instead of modifying
> err. IMO the latter is neater way of doing it. In that case, we could do
> something like:


Nice catch. You are absolutely right, the switch conditions has to be 
changed
to check for "h_errno" matching the previous version of that function,
otherwise we will always go to default. I thought about returning 
converted error number from "_nsError" while writing this patch but I 
chose passing the error argument by pointer as there are other functions
like "dns_query_AFSDB" & "dns_query_VL_SRV" that don't seem to care 
about the return value of "_nsError", Also in "nsError" there's kind of 
duplication in the switch condition where we are checking for "case 0" 
(which is not very descriptive) & "case NO_RECOVERY" and both lead to 
the same action so I feel like using single condition with descriptive 
name like "ECONNREFUSED" is better. Lastly the "_nsError" used mainly 
for logging & error conversion and it looks like "nsError" the only 
function where converted error post processing is happening so it's very 
unlikely for other users of "_nsError" to care about the return code. 
Given these facts I will go ahead with submitting v2 of this patch to 
accommodate the proposed switch condition changes.

static int dns_query_AFSDB(const char *cell)
{
         int     response_len;           /* buffer length */
         ns_msg  handle;                 /* handle for response message */
         union {
                 HEADER hdr;
                 u_char buf[NS_PACKETSZ];
         } response;             /* response buffers */

         debug("Get AFSDB RR for cell name:'%s'", cell);

         /* query the dns for an AFSDB resource record */
         response_len = res_query(cell,
                                  ns_c_in,
                                  ns_t_afsdb,
                                  response.buf,
                                  sizeof(response));

         if (response_len < 0) {
                 /* negative result */
                 _nsError(&h_errno, cell);
                 return -1;
         }

static int dns_query_VL_SRV(const char *cell)
{
         int     response_len;           /* buffer length */
         ns_msg  handle;                 /* handle for response message */
         union {
                 HEADER hdr;
                 u_char buf[NS_PACKETSZ];
         } response;
         char name[1024];

         snprintf(name, sizeof(name), "_afs3-vlserver._udp.%s", cell);

         debug("Get VL SRV RR for name:'%s'", name);

         response_len = res_query(name,
                                  ns_c_in,
                                  ns_t_srv,
                                  response.buf,
                                  sizeof(response));

         if (response_len < 0) {
                 /* negative result */
                 _nsError(&h_errno, cell);
                 return -1;
         }

         if (ns_initparse(response.buf, response_len, &handle) < 0)
                 error("ns_initparse: %m");

         /* look up the hostnames we've obtained to get the actual 
addresses */
         srv_hosts_to_addrs(handle, ns_s_an);

         info("DNS query VL SRV RR results:%u ttl:%u", payload_index, 
key_expiry);
         return 0;
}