| Message ID | 20250217005452.4873-1-abuehaze@amazon.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Pass "err" argument by address to "_nsError" function | expand |
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: So I would suggest doing something like the below patch (only compile-tested): --- 8< --- diff --git a/key.dns.h b/key.dns.h index 33d0ab3..9c2df89 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 int _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..3182828 100644 --- a/key.dns_resolver.c +++ b/key.dns_resolver.c @@ -157,27 +157,31 @@ static const int ns_errno_map[] = { [NO_DATA] = ENODATA, }; -void _nsError(int err, const char *domain) +int _nsError(int err, const char *domain) { + int errno; + if (isatty(2)) fprintf(stderr, "NS:%s: %s.\n", domain, hstrerror(err)); else syslog(LOG_INFO, "%s: %s", domain, hstrerror(err)); if (err >= sizeof(ns_errno_map) / sizeof(ns_errno_map[0])) - err = ECONNREFUSED; + errno = ECONNREFUSED; else - err = ns_errno_map[err]; + errno = ns_errno_map[err]; - info("Reject the key with error %d", err); + info("Reject the key with error %d", errno); + + return errno; } void nsError(int err, const char *domain) { unsigned timeout; - int ret; + int ret, errno; - _nsError(err, domain); + errno = _nsError(err, domain); switch (err) { case TRY_AGAIN: @@ -193,7 +197,7 @@ void nsError(int err, const char *domain) } if (!debug_mode) { - ret = keyctl_reject(key, timeout, err, KEY_REQKEY_DEFL_DEFAULT); + ret = keyctl_reject(key, timeout, errno, KEY_REQKEY_DEFL_DEFAULT); if (ret == -1) error("%s: keyctl_reject: %m", __func__); } --- >8 --- > switch (err) { > case TRY_AGAIN: > timeout = 1;
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; }
On Mon, Feb 17, 2025 at 12:54:52AM +0000, 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); Why a function declaration need extern anyway? You could do w/o. > 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); > switch (err) { > case TRY_AGAIN: > timeout = 1; > -- > 2.47.1 > > BR, Jarkko
Hi Jarkko, On 17/02/2025 16:27, Jarkko Sakkinen wrote: > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. > > > > On Mon, Feb 17, 2025 at 12:54:52AM +0000, 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); > > Why a function declaration need extern anyway? > > You could do w/o. > I agree that theoretically "extern" in function declaration shouldn't be needed as functions implicitly has "extern" by default however I noticed that all functions in keyutils are declared like that(not sure if there are some stupid compilers which really need this)". "extern" has been added to declaration of "_nsError" function in Commit 0d71523ab584 (“DNS: Support AFS SRV records andcell db config files”) so it's not something that I changed in this new patch. Given the previously mentioned facts I am more into keeping the current declaration as it's to align with the trend in declaring other functions in the package. Removing "extern" from all functions' declarations is better to be done in another patch unless people have objection on that. keyctl.h:extern nr void do_command(int, char **, const struct command *, const char *); keyctl.h:extern nr void format(void) __attribute__((noreturn)); keyctl.h:extern nr void error(const char *) __attribute__((noreturn)); keyctl.h:extern key_serial_t get_key_id(char *); keyctl.h:extern nr void act_keyctl_test(int, char *[]); keyctl.h:extern nr void act_keyctl_watch(int , char *[]); keyctl.h:extern nr void act_keyctl_watch_add(int , char *[]); keyctl.h:extern nr void act_keyctl_watch_rm(int , char *[]); keyctl.h:extern nr void act_keyctl_watch_session(int , char *[]); keyctl.h:extern nr void act_keyctl_watch_sync(int , char *[]); >> 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); >> switch (err) { >> case TRY_AGAIN: >> timeout = 1; >> -- >> 2.47.1 >> >> > > BR, Jarkko
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); switch (err) { case TRY_AGAIN: timeout = 1;
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(-)