[PATCH] autofs: handle old configuration format

[PATCH] autofs: handle old configuration format

[Josef Bacik]

This is a patch to fix

http://bugzilla.centos.org/view.php?id=8614

The configuration stuff was redone and it broke backwards compatiblity.
Unfortunately this was backported to Centos/RHEL, so configurations that were
working in 6.4 suddenly broke in 6.6, which is not helpful.  Fix this by
noticing if we are looking at the old configuration file, strip out the DEFAULT_
bit of the variable if it is there and tolower the rest of the string.  This
makes it so our old configuration works properly and now people get their home
dirs automounted properly with either versions of autofs.  Thanks,

Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 lib/defaults.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/lib/defaults.c b/lib/defaults.c
index 5711e65..cd43a60 100644
--- a/lib/defaults.c
+++ b/lib/defaults.c
@@ -776,6 +776,27 @@ static int check_set_config_value(const char *section,
 	return ret;
 }
 
+static int check_set_old_config_value(const char *section, const char *key,
+				      const char *value)
+{
+	const char *pkey = key;
+	char lkey[PATH_MAX+1];
+	char *plkey = &lkey[0];
+
+	/*
+	 * The old format was DEFAULT_whatever, so just trim the DEFAULT_ bit if
+	 * it's there.
+	 */
+	if (strstr(key, "DEFAULT_"))
+		pkey += strlen("DEFAULT_");
+
+	/* Lower case everything */
+	while (*pkey)
+		*plkey++ = tolower(*pkey++);
+	*plkey = '\0';
+	return check_set_config_value(section, lkey, value);
+}
+
 static int parse_line(char *line, char **sec, char **res, char **value)
 {
 	char *key, *val, *trailer;
@@ -924,7 +945,10 @@ static int read_config(unsigned int to_syslog, FILE *f, const char *name)
 				"%s is not used by autofs, ignored", res);
 			continue;
 		}
-		check_set_config_value(new_sec, key, value);
+		if (name == OLD_CONFIG_FILE)
+			check_set_old_config_value(new_sec, key, value);
+		else
+			check_set_config_value(new_sec, key, value);
 	}
 
 	if (!feof(f) || ferror(f)) {
-- 
2.1.0

Re: [PATCH] autofs: handle old configuration format

[Ian Kent]

Hi Josef,

Hope you are well.

On Wed, 2015-06-17 at 09:29 -0700, Josef Bacik wrote:
> This is a patch to fix
> 
> http://bugzilla.centos.org/view.php?id=8614
> 
> The configuration stuff was redone and it broke backwards compatiblity.

OK, but I put quite a bit of effort in to not do that.

I thought it more likely I'd get complaints from people not realizing an
old configuration left in place would override changes in the new
configuration.

Can you give me an example of the problem you have seen please.

> Unfortunately this was backported to Centos/RHEL, so configurations that were
> working in 6.4 suddenly broke in 6.6, which is not helpful.  Fix this by
> noticing if we are looking at the old configuration file, strip out the DEFAULT_
> bit of the variable if it is there and tolower the rest of the string.  This
> makes it so our old configuration works properly and now people get their home
> dirs automounted properly with either versions of autofs.  Thanks,

The way this is supposed to work is that the new configuration file is
read and then the old one is read and anything in the old configuration
file should override anything in the new one.

When looking up configuration values if the key isn't found and the key
starts with "DEFAULT_" the the key is looked for again without the
"DEFAULT_". Also, key comparisons are case insensitive so that shouldn't
make a difference.

And I'm pretty sure I strip white space too so extra spaces shouldn't
cause a problem either.

But clearly you have a case that I've missed which I'm keen to hear
about.
  
> 
> Signed-off-by: Josef Bacik <jbacik@fb.com>
> ---
>  lib/defaults.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/defaults.c b/lib/defaults.c
> index 5711e65..cd43a60 100644
> --- a/lib/defaults.c
> +++ b/lib/defaults.c
> @@ -776,6 +776,27 @@ static int check_set_config_value(const char *section,
>  	return ret;
>  }
>  
> +static int check_set_old_config_value(const char *section, const char *key,
> +				      const char *value)
> +{
> +	const char *pkey = key;
> +	char lkey[PATH_MAX+1];
> +	char *plkey = &lkey[0];
> +
> +	/*
> +	 * The old format was DEFAULT_whatever, so just trim the DEFAULT_ bit if
> +	 * it's there.
> +	 */
> +	if (strstr(key, "DEFAULT_"))
> +		pkey += strlen("DEFAULT_");
> +
> +	/* Lower case everything */
> +	while (*pkey)
> +		*plkey++ = tolower(*pkey++);
> +	*plkey = '\0';
> +	return check_set_config_value(section, lkey, value);
> +}
> +
>  static int parse_line(char *line, char **sec, char **res, char **value)
>  {
>  	char *key, *val, *trailer;
> @@ -924,7 +945,10 @@ static int read_config(unsigned int to_syslog, FILE *f, const char *name)
>  				"%s is not used by autofs, ignored", res);
>  			continue;
>  		}
> -		check_set_config_value(new_sec, key, value);
> +		if (name == OLD_CONFIG_FILE)
> +			check_set_old_config_value(new_sec, key, value);
> +		else
> +			check_set_config_value(new_sec, key, value);
>  	}
>  
>  	if (!feof(f) || ferror(f)) {

Re: [PATCH] autofs: handle old configuration format

[Josef Bacik]

On 06/17/2015 05:39 PM, Ian Kent wrote:
> Hi Josef,
>
> Hope you are well.
>
> On Wed, 2015-06-17 at 09:29 -0700, Josef Bacik wrote:
>> This is a patch to fix
>>
>> http://bugzilla.centos.org/view.php?id=8614
>>
>> The configuration stuff was redone and it broke backwards compatiblity.
>
> OK, but I put quite a bit of effort in to not do that.
>
> I thought it more likely I'd get complaints from people not realizing an
> old configuration left in place would override changes in the new
> configuration.
>
> Can you give me an example of the problem you have seen please.
>
>> Unfortunately this was backported to Centos/RHEL, so configurations that were
>> working in 6.4 suddenly broke in 6.6, which is not helpful.  Fix this by
>> noticing if we are looking at the old configuration file, strip out the DEFAULT_
>> bit of the variable if it is there and tolower the rest of the string.  This
>> makes it so our old configuration works properly and now people get their home
>> dirs automounted properly with either versions of autofs.  Thanks,
>
> The way this is supposed to work is that the new configuration file is
> read and then the old one is read and anything in the old configuration
> file should override anything in the new one.
>
> When looking up configuration values if the key isn't found and the key
> starts with "DEFAULT_" the the key is looked for again without the
> "DEFAULT_". Also, key comparisons are case insensitive so that shouldn't
> make a difference.
>
> And I'm pretty sure I strip white space too so extra spaces shouldn't
> cause a problem either.
>
> But clearly you have a case that I've missed which I'm keen to hear
> about.
>

Huh sorry I got confused by looking at the diffs that I thought you had 
dropped that bit, but now I remember looking at this code a few days 
ago.  I'm not sure why it's not working, our /etc/sysconfig/autofs looks 
something like this

DEFAULT_LOGGING="debug or some shit"
DEFAULT_MAP_OBJECT_CLASS="blah"
DEFAULT_ENTRY_OBJECT_CLASS="foo"
DEFAULT_MAP_ATTRIBUTE="something else"
DEFAULT_ENTRY_ATTRIBUTE="whatever"
DEFAULT_VALUE_ATTRIBUTE="bar"

When we upgrade it cycles through the normal map_object_class things 
instead of using ours, it thinks there's no schema.  With my change it 
works fine.  I don't have VPN access working at the moment so I can't 
screw around with it now, but if you want me to poke at it some more I 
can do it in the morning.  Thanks,

Josef

Re: [PATCH] autofs: handle old configuration format

[Josef Bacik]

On 06/17/2015 05:39 PM, Ian Kent wrote:
> Hi Josef,
>
> Hope you are well.
>
> On Wed, 2015-06-17 at 09:29 -0700, Josef Bacik wrote:
>> This is a patch to fix
>>
>> http://bugzilla.centos.org/view.php?id=8614
>>
>> The configuration stuff was redone and it broke backwards compatiblity.
>
> OK, but I put quite a bit of effort in to not do that.
>
> I thought it more likely I'd get complaints from people not realizing an
> old configuration left in place would override changes in the new
> configuration.
>
> Can you give me an example of the problem you have seen please.
>
>> Unfortunately this was backported to Centos/RHEL, so configurations that were
>> working in 6.4 suddenly broke in 6.6, which is not helpful.  Fix this by
>> noticing if we are looking at the old configuration file, strip out the DEFAULT_
>> bit of the variable if it is there and tolower the rest of the string.  This
>> makes it so our old configuration works properly and now people get their home
>> dirs automounted properly with either versions of autofs.  Thanks,
>
> The way this is supposed to work is that the new configuration file is
> read and then the old one is read and anything in the old configuration
> file should override anything in the new one.
>
> When looking up configuration values if the key isn't found and the key
> starts with "DEFAULT_" the the key is looked for again without the
> "DEFAULT_". Also, key comparisons are case insensitive so that shouldn't
> make a difference.

Oh now I realize why it's not working.  When you do defaults_get_schema 
you are looking up the new values, so your check in conf_lookup isn't 
helping anything, because we aren't looking up DEFAULT_*, we're looking 
up the new values.  And since we don't have those variables set in 
autofs.conf they don't get set until we load the old things, and so we 
have DEFAULT_* set in the conf but not the normal variables.  So we can 
do what I've done here, or change conf_lookup to add the DEFAULT_ to the 
front of anything we search for if we can't find it.  Thanks,

Josef

Re: [PATCH] autofs: handle old configuration format

[Ian Kent]

On Wed, 2015-06-17 at 20:52 -0700, Josef Bacik wrote:
> On 06/17/2015 05:39 PM, Ian Kent wrote:
> > Hi Josef,
> >
> > Hope you are well.
> >
> > On Wed, 2015-06-17 at 09:29 -0700, Josef Bacik wrote:
> >> This is a patch to fix
> >>
> >> http://bugzilla.centos.org/view.php?id=8614
> >>
> >> The configuration stuff was redone and it broke backwards compatiblity.
> >
> > OK, but I put quite a bit of effort in to not do that.
> >
> > I thought it more likely I'd get complaints from people not realizing an
> > old configuration left in place would override changes in the new
> > configuration.
> >
> > Can you give me an example of the problem you have seen please.
> >
> >> Unfortunately this was backported to Centos/RHEL, so configurations that were
> >> working in 6.4 suddenly broke in 6.6, which is not helpful.  Fix this by
> >> noticing if we are looking at the old configuration file, strip out the DEFAULT_
> >> bit of the variable if it is there and tolower the rest of the string.  This
> >> makes it so our old configuration works properly and now people get their home
> >> dirs automounted properly with either versions of autofs.  Thanks,
> >
> > The way this is supposed to work is that the new configuration file is
> > read and then the old one is read and anything in the old configuration
> > file should override anything in the new one.
> >
> > When looking up configuration values if the key isn't found and the key
> > starts with "DEFAULT_" the the key is looked for again without the
> > "DEFAULT_". Also, key comparisons are case insensitive so that shouldn't
> > make a difference.
> 
> Oh now I realize why it's not working.  When you do defaults_get_schema 
> you are looking up the new values, so your check in conf_lookup isn't 
> helping anything, because we aren't looking up DEFAULT_*, we're looking 
> up the new values.  And since we don't have those variables set in 
> autofs.conf they don't get set until we load the old things, and so we 
> have DEFAULT_* set in the conf but not the normal variables.  So we can 
> do what I've done here, or change conf_lookup to add the DEFAULT_ to the 
> front of anything we search for if we can't find it.  Thanks,

Yes, I was thinking about this and I think my handling of configuration
entries with a "DEFAULT_" prefix might be backward.

It's been so long (from an upstream POV) since the "DEFAULT_" prefix was
used I've probably got it wrong.

Thought I tested it though ....

Let me look further.

> 
> Josef
> 

Re: [PATCH] autofs: handle old configuration format

[Ian Kent]

On Wed, 2015-06-17 at 20:52 -0700, Josef Bacik wrote:
> On 06/17/2015 05:39 PM, Ian Kent wrote:
> > Hi Josef,
> >
> > Hope you are well.
> >
> > On Wed, 2015-06-17 at 09:29 -0700, Josef Bacik wrote:
> >> This is a patch to fix
> >>
> >> http://bugzilla.centos.org/view.php?id=8614
> >>
> >> The configuration stuff was redone and it broke backwards compatiblity.
> >
> > OK, but I put quite a bit of effort in to not do that.
> >
> > I thought it more likely I'd get complaints from people not realizing an
> > old configuration left in place would override changes in the new
> > configuration.
> >
> > Can you give me an example of the problem you have seen please.
> >
> >> Unfortunately this was backported to Centos/RHEL, so configurations that were
> >> working in 6.4 suddenly broke in 6.6, which is not helpful.  Fix this by
> >> noticing if we are looking at the old configuration file, strip out the DEFAULT_
> >> bit of the variable if it is there and tolower the rest of the string.  This
> >> makes it so our old configuration works properly and now people get their home
> >> dirs automounted properly with either versions of autofs.  Thanks,
> >
> > The way this is supposed to work is that the new configuration file is
> > read and then the old one is read and anything in the old configuration
> > file should override anything in the new one.
> >
> > When looking up configuration values if the key isn't found and the key
> > starts with "DEFAULT_" the the key is looked for again without the
> > "DEFAULT_". Also, key comparisons are case insensitive so that shouldn't
> > make a difference.
> 
> Oh now I realize why it's not working.  When you do defaults_get_schema 
> you are looking up the new values, so your check in conf_lookup isn't 
> helping anything, because we aren't looking up DEFAULT_*, we're looking 
> up the new values.  And since we don't have those variables set in 
> autofs.conf they don't get set until we load the old things, and so we 
> have DEFAULT_* set in the conf but not the normal variables.  So we can 
> do what I've done here, or change conf_lookup to add the DEFAULT_ to the 
> front of anything we search for if we can't find it.  Thanks,

Yes, that looks to be the case.

I can't just strip the DEFAULT_ prefix on adding values because there
could be configuration key names that are meant to have the prefix.

So I think it's best to try and handle this in the conf_lookup()
function.

You may get some conflicts with this as it's against current upstream
but if you can resolve them, does this patch resolve the problem?

If you have difficulty with conflicts then give me the package revision
and I'll back port the patch to it so you can test it.

autofs-5.1.1 - fix config old name lookup

From: Ian Kent <raven@themaw.net>

There are three cases needed to handle configuration name lookup.

First there's the configuration key name, the name match is case
insensitive so the recent case change isn't a seperate case.

But the much older configuration key names that began with "DEFAULT_"
need special handling.

There are two cases that need to be covered:
1) an old name is given but a new name needs to be located.
2) a new name is given but an old name needs to be located.

Only 1) is currently covered, so fix that in conf_lookup().

Signed-off-by: Ian Kent <raven@themaw.net>
---
 CHANGELOG      |    1 +
 lib/defaults.c |   13 +++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/CHANGELOG b/CHANGELOG
index 1fcb763..606817c 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -2,6 +2,7 @@
 =======================
 - fix left mount count return from umount_multi_triggers().
 - fix rwlock unlock crash.
+- fix config old name lookup.
 
 21/04/2015 autofs-5.1.1
 =======================
diff --git a/lib/defaults.c b/lib/defaults.c
index 5711e65..dc162a6 100644
--- a/lib/defaults.c
+++ b/lib/defaults.c
@@ -31,6 +31,8 @@
 #include "log.h"
 #include "automount.h"
 
+#define MAX_CONF_KEY_LEN		40
+
 #define AUTOFS_GLOBAL_SECTION		"autofs"
 #define AMD_GLOBAL_SECTION		"amd"
 
@@ -728,6 +730,17 @@ static struct conf_option *conf_lookup(const char *section, const char *key)
 		 */
 		if (strlen(key) > 8 && !strncasecmp("DEFAULT_", key, 8))
 			co = conf_lookup_key(section, key + 8);
+		else {
+			/* A new key name has been given but the value
+			 * we seek is stored under an old key name (which
+			 * includes the "DEFAULT_" prefix.
+			 */
+			char old_key[MAX_CONF_KEY_LEN];
+
+			strcpy(old_key, "DEFAULT_");
+			strcat(old_key, key);
+			co = conf_lookup_key(section, old_key);
+		}
 	}
 
 	return co;

Re: [PATCH] autofs: handle old configuration format

[Josef Bacik]

On 06/17/2015 11:37 PM, Ian Kent wrote:
> On Wed, 2015-06-17 at 20:52 -0700, Josef Bacik wrote:
>> On 06/17/2015 05:39 PM, Ian Kent wrote:
>>> Hi Josef,
>>>
>>> Hope you are well.
>>>
>>> On Wed, 2015-06-17 at 09:29 -0700, Josef Bacik wrote:
>>>> This is a patch to fix
>>>>
>>>> https://urldefense.proofpoint.com/v1/url?u=http://bugzilla.centos.org/view.php?id%3D8614&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=cKCbChRKsMpTX8ybrSkonQ%3D%3D%0A&m=2pMrg2mSVkYIRbq9PsXATsOwwHLUv7M%2FC1vSREeuAMA%3D%0A&s=bce3fa3f755b02b8849de7338fa74adb71fd4cd84a2a318820c29830ed16f699
>>>>
>>>> The configuration stuff was redone and it broke backwards compatiblity.
>>>
>>> OK, but I put quite a bit of effort in to not do that.
>>>
>>> I thought it more likely I'd get complaints from people not realizing an
>>> old configuration left in place would override changes in the new
>>> configuration.
>>>
>>> Can you give me an example of the problem you have seen please.
>>>
>>>> Unfortunately this was backported to Centos/RHEL, so configurations that were
>>>> working in 6.4 suddenly broke in 6.6, which is not helpful.  Fix this by
>>>> noticing if we are looking at the old configuration file, strip out the DEFAULT_
>>>> bit of the variable if it is there and tolower the rest of the string.  This
>>>> makes it so our old configuration works properly and now people get their home
>>>> dirs automounted properly with either versions of autofs.  Thanks,
>>>
>>> The way this is supposed to work is that the new configuration file is
>>> read and then the old one is read and anything in the old configuration
>>> file should override anything in the new one.
>>>
>>> When looking up configuration values if the key isn't found and the key
>>> starts with "DEFAULT_" the the key is looked for again without the
>>> "DEFAULT_". Also, key comparisons are case insensitive so that shouldn't
>>> make a difference.
>>
>> Oh now I realize why it's not working.  When you do defaults_get_schema
>> you are looking up the new values, so your check in conf_lookup isn't
>> helping anything, because we aren't looking up DEFAULT_*, we're looking
>> up the new values.  And since we don't have those variables set in
>> autofs.conf they don't get set until we load the old things, and so we
>> have DEFAULT_* set in the conf but not the normal variables.  So we can
>> do what I've done here, or change conf_lookup to add the DEFAULT_ to the
>> front of anything we search for if we can't find it.  Thanks,
>
> Yes, that looks to be the case.
>
> I can't just strip the DEFAULT_ prefix on adding values because there
> could be configuration key names that are meant to have the prefix.
>
> So I think it's best to try and handle this in the conf_lookup()
> function.
>
> You may get some conflicts with this as it's against current upstream
> but if you can resolve them, does this patch resolve the problem?
>
> If you have difficulty with conflicts then give me the package revision
> and I'll back port the patch to it so you can test it.
>
> autofs-5.1.1 - fix config old name lookup
>

That did it, thanks!

Josef

Re: [PATCH] autofs: handle old configuration format

[Ian Kent]

On Thu, 2015-06-18 at 09:41 -0700, Josef Bacik wrote:
> On 06/17/2015 11:37 PM, Ian Kent wrote:
> > On Wed, 2015-06-17 at 20:52 -0700, Josef Bacik wrote:
> >> On 06/17/2015 05:39 PM, Ian Kent wrote:
> >>> Hi Josef,
> >>>
> >>> Hope you are well.
> >>>
> >>> On Wed, 2015-06-17 at 09:29 -0700, Josef Bacik wrote:
> >>>> This is a patch to fix
> >>>>
> >>>> https://urldefense.proofpoint.com/v1/url?u=http://bugzilla.centos.org/view.php?id%3D8614&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=cKCbChRKsMpTX8ybrSkonQ%3D%3D%0A&m=2pMrg2mSVkYIRbq9PsXATsOwwHLUv7M%2FC1vSREeuAMA%3D%0A&s=bce3fa3f755b02b8849de7338fa74adb71fd4cd84a2a318820c29830ed16f699
> >>>>
> >>>> The configuration stuff was redone and it broke backwards compatiblity.
> >>>
> >>> OK, but I put quite a bit of effort in to not do that.
> >>>
> >>> I thought it more likely I'd get complaints from people not realizing an
> >>> old configuration left in place would override changes in the new
> >>> configuration.
> >>>
> >>> Can you give me an example of the problem you have seen please.
> >>>
> >>>> Unfortunately this was backported to Centos/RHEL, so configurations that were
> >>>> working in 6.4 suddenly broke in 6.6, which is not helpful.  Fix this by
> >>>> noticing if we are looking at the old configuration file, strip out the DEFAULT_
> >>>> bit of the variable if it is there and tolower the rest of the string.  This
> >>>> makes it so our old configuration works properly and now people get their home
> >>>> dirs automounted properly with either versions of autofs.  Thanks,
> >>>
> >>> The way this is supposed to work is that the new configuration file is
> >>> read and then the old one is read and anything in the old configuration
> >>> file should override anything in the new one.
> >>>
> >>> When looking up configuration values if the key isn't found and the key
> >>> starts with "DEFAULT_" the the key is looked for again without the
> >>> "DEFAULT_". Also, key comparisons are case insensitive so that shouldn't
> >>> make a difference.
> >>
> >> Oh now I realize why it's not working.  When you do defaults_get_schema
> >> you are looking up the new values, so your check in conf_lookup isn't
> >> helping anything, because we aren't looking up DEFAULT_*, we're looking
> >> up the new values.  And since we don't have those variables set in
> >> autofs.conf they don't get set until we load the old things, and so we
> >> have DEFAULT_* set in the conf but not the normal variables.  So we can
> >> do what I've done here, or change conf_lookup to add the DEFAULT_ to the
> >> front of anything we search for if we can't find it.  Thanks,
> >
> > Yes, that looks to be the case.
> >
> > I can't just strip the DEFAULT_ prefix on adding values because there
> > could be configuration key names that are meant to have the prefix.
> >
> > So I think it's best to try and handle this in the conf_lookup()
> > function.
> >
> > You may get some conflicts with this as it's against current upstream
> > but if you can resolve them, does this patch resolve the problem?
> >
> > If you have difficulty with conflicts then give me the package revision
> > and I'll back port the patch to it so you can test it.
> >
> > autofs-5.1.1 - fix config old name lookup
> >
> 
> That did it, thanks!

I'm still thinking about this so there might be more changes, not sure
though.

I'll let you know if I make more changes and will commit what I have
next time I push changes to the upstream repo (and post the patches at
the usual autofs location on kernel.org).

Ian