From: Ben Boeckel <me@benboeckel.net>
To: David Howells <dhowells@redhat.com>
Cc: linux-nfs@vger.kernel.org, linux-cifs@vger.kernel.org,
linux-afs@lists.infradead.org, ceph-devel@vger.kernel.org,
keyrings@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, fweimer@redhat.com
Subject: Re: [PATCH] dns: Apply a default TTL to records obtained from getaddrinfo()
Date: Mon, 18 May 2020 11:51:48 -0400 [thread overview]
Message-ID: <20200518155148.GA2595638@erythro.dev.benboeckel.internal> (raw)
In-Reply-To: <158981176590.872823.11683683537698750702.stgit@warthog.procyon.org.uk>
On Mon, May 18, 2020 at 15:22:45 +0100, David Howells wrote:
> Address records obtained from getaddrinfo() don't come with any TTL
> information, even if they're obtained from the DNS, with the result that
> key.dns_resolver upcall program doesn't set an expiry time on dns_resolver
> records unless they include a component obtained directly from the DNS,
> such as an SRV or AFSDB record.
>
> Fix this to apply a default TTL of 10mins in the event that we haven't got
> one. This can be configured in /etc/keyutils/key.dns_resolver.conf by
> adding the line:
>
> default_ttl: <number-of-seconds>
>
> to the file.
Is there precedent for this config file format? It looks like possible
YAML, but this patch doesn't mention that anywhere. Is there a good
reason to not be using an existing format (ini, toml, json, shell-alike,
anything)? I have no preference for anything other than a format that
already exists out there. Maybe one that supports comments too so that
these settings can have context associated with them in real
deployments (which effectively rules out json)?
> + while (fgets(buf, sizeof(buf) - 1, f)) {
> + line++;
> + if (buf[0] == '#')
> + continue;
So it does support comments...
> + p = strchr(buf, '\n');
> + if (!p)
> + error("%s:%u: line missing newline or too long", config_file, line);
> + while (p > buf && isspace(p[-1]))
> + p--;
> + *p = 0;
> +
> + if (strncmp(buf, "default_ttl:", 12) == 0) {
> + p = buf + 12;
> + while (isspace(*p))
> + p++;
...but not if it starts with whitespace. So if one does indent the
`default_ttl` setting for whatever reason, the comment is stuck at the
first column.
> + if (sscanf(p, "%u%n", &u, &n) != 1)
> + error("%s:%u: default_ttl: Bad value",
> + config_file, line);
> + if (p[n])
> + error("%s:%u: default_ttl: Extra data supplied",
> + config_file, line);
But no trailing whitespace is allowed?
> + if (u < 1 || u > INT_MAX)
> + error("%s:%u: default_ttl: Out of range",
> + config_file, line);
The valid range should be mentioned in the docs (basically that 0 is not
allowed and has no special meaning (it could mean leaving off the TTL as
previously done)).
> + key_expiry = u;
> + } else {
> + error("%s:%u: Unknown option", config_file, line);
Forwards compatibility is hard with such behavior. Is there any reason
this can't be a warning?
> @@ -24,6 +26,21 @@ It can be called in debugging mode to test its functionality by passing a
> \fB\-D\fR flag on the command line. For this to work, the key description and
> the callout information must be supplied. Verbosity can be increased by
> supplying one or more \fB\-v\fR flags.
> +.P
> +A configuration file can be supplied to adjust various parameters. The file
> +is looked for at:
> +.IP
> +/etc/keyutils/key.dns_resolver.conf
> +.P
> +unless otherwise specified with the \fB\-c\fR flag.
> +.P
> +Configuration options include:
> +.TP
> +.B default_ttl: <number>
> +The number of seconds to set as the expiration on a cached record. This will
> +be overridden if the program manages to retrieve TTL information along with
> +the addresses (if, for example, it accesses the DNS directly). The default is
> +600 seconds.
There's no mention of the leading whitespace support or comments here.
Does the file deserve its own manpage?
--Ben
next prev parent reply other threads:[~2020-05-18 15:51 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-18 14:22 [PATCH] dns: Apply a default TTL to records obtained from getaddrinfo() David Howells
2020-05-18 15:51 ` Ben Boeckel [this message]
2020-05-19 13:39 ` David Howells
2020-05-19 14:14 ` Ben Boeckel
2020-05-19 16:06 ` David Howells
2020-05-19 16:24 ` Ben Boeckel
2020-05-20 15:07 ` Jeff Layton
2020-05-19 14:17 ` Florian Weimer
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=20200518155148.GA2595638@erythro.dev.benboeckel.internal \
--to=me@benboeckel.net \
--cc=ceph-devel@vger.kernel.org \
--cc=dhowells@redhat.com \
--cc=fweimer@redhat.com \
--cc=keyrings@vger.kernel.org \
--cc=linux-afs@lists.infradead.org \
--cc=linux-cifs@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=netdev@vger.kernel.org \
/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 NNTP newsgroup(s).