From: NeilBrown <neilb@suse.de>
To: Steve Dickson <steved@redhat.com>,
Linux NFS Mailing list <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH] mount: parse default values correctly
Date: Fri, 29 Jan 2021 10:39:21 +1100 [thread overview]
Message-ID: <87o8h8fx7a.fsf@notabene.neil.brown.name> (raw)
In-Reply-To: <20210106184028.150925-1-steved@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 2193 bytes --]
On Wed, Jan 06 2021, Steve Dickson wrote:
> Commit 88c22f92 converted the configfile.c routines
> to use the parse_opt interfaces which broke how
> default values from nfsmount.conf are managed.
>
> Default values can not be added to the mount string
> handed to the kernel. They must be interpreted into
> the correct mount options then passed to the kernel.
>
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1912877
>
> Signed-off-by: Steve Dickson <steved@redhat.com>
> ---
> utils/mount/configfile.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/utils/mount/configfile.c b/utils/mount/configfile.c
> index 7934f4f..e865998 100644
> --- a/utils/mount/configfile.c
> +++ b/utils/mount/configfile.c
> @@ -277,8 +277,10 @@ conf_parse_mntopts(char *section, char *arg, struct mount_options *options)
> }
> if (buf[0] == '\0')
> continue;
> + if (default_value(buf))
> + continue;
> +
> po_append(options, buf);
> - default_value(buf);
> }
> conf_free_list(list);
> }
> --
> 2.29.2
Hi,
unfortunately this is incorrect. As the referenced patch said:
The default_value() call is now made as soon as the option has been
parsed. It is left on the options list so that new instances of the
value are ignored.
The new code doesn't leave it on the option list, so new instances are
not ignored.
conf_get_mntopts() processes options for the mount point, then options
for the server, then global options. If there is e.g. a defaultvers
for both the mountpoint and as a global option, the global option will
now not be ignored, so it takes precedence.
The real problem as identified in the bugzilla discussion, is that
po_remove_all(options, "default");
doesn't remove all the "default*" options as was the intent. Clearly I
never tested that.
This is easily fixed with
- po_remove_all(options, "default");
+ while (po_contains_prefix(options, "default", &ptr, 0) == PO_FOUND)
+ po_remove_all(options, ptr);
which I have tested :-)
I'll follow up with a patch which reverts the incorrect fix, and adds
this one.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 853 bytes --]
next prev parent reply other threads:[~2021-01-28 23:40 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-06 18:40 [PATCH] mount: parse default values correctly Steve Dickson
2021-01-07 16:03 ` Steve Dickson
2021-01-28 23:39 ` NeilBrown [this message]
2021-01-28 23:40 ` [PATCH nfs-utils] mount: fix parsing of default options NeilBrown
2021-02-01 5:00 ` [PATCH nfs-utils v2] " NeilBrown
2021-02-02 17:00 ` Steve Dickson
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=87o8h8fx7a.fsf@notabene.neil.brown.name \
--to=neilb@suse.de \
--cc=linux-nfs@vger.kernel.org \
--cc=steved@redhat.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.