From: "Mohamed Abuelfotoh, Hazem" <abuehaze@amazon.com>
To: Jarkko Sakkinen <jarkko@kernel.org>
Cc: <dhowells@redhat.com>, <keyrings@vger.kernel.org>,
<benh@amazon.com>, <ptyadav@amazon.com>
Subject: Re: [PATCH] Pass "err" argument by address to "_nsError" function
Date: Tue, 18 Feb 2025 12:10:53 +0000 [thread overview]
Message-ID: <e9c6482f-52a1-43a6-9713-ab77990dc135@amazon.com> (raw)
In-Reply-To: <Z7NjZTjRZSt3pmZh@kernel.org>
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
prev parent reply other threads:[~2025-02-18 12:10 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-02-17 0:54 [PATCH] Pass "err" argument by address to "_nsError" function Hazem Mohamed Abuelfotoh
2025-02-17 13:03 ` Pratyush Yadav
2025-02-17 15:25 ` Mohamed Abuelfotoh, Hazem
2025-02-17 16:27 ` Jarkko Sakkinen
2025-02-18 12:10 ` Mohamed Abuelfotoh, Hazem [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e9c6482f-52a1-43a6-9713-ab77990dc135@amazon.com \
--to=abuehaze@amazon.com \
--cc=benh@amazon.com \
--cc=dhowells@redhat.com \
--cc=jarkko@kernel.org \
--cc=keyrings@vger.kernel.org \
--cc=ptyadav@amazon.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).