Update YAML permissions in configuration

[nick-enoent]

Add cache_ip option to aggregators->peers
Update documentation

[morrone]

Looks like this basically works, and I like the documentation updates.

I would just suggest that we can improve the error checking and the decimal number conversion. See in-code comments.

[morrone]

I see what you are going for here: differentiating between the unix-line string case, and the case where it is all digits (a octal permsion). I think we can make this a step more pythonic, and clearer to read if we use:

  if not perm_str.isdecimal()

[morrone]

And there isn't checking to see if the letters are correctly "r" or "w, or that the "x" positions are "-". So this would be accepted right now

Qf-ltoxep

No bits set, of course, but I think that needs to raise an error.:)

I think I would probably just use a regex something like below. The regex takes care of all the correctness checking, and we just need to sum up the bit values.

perms_pattern = re.compile('(?:(r)|-)(?:(w)|-)-(?:(r)|-)(?:(w)|-)-(?:(r)|-)(?:(w)|-)-')
m = perms_pattern.fullmatch(sys.argv[1])
if not m:
    raise ValueError("whatever")
perms = 0
if m.group(1):
    perms += 0o400
if m.group(2):
    perms += 0o200
if m.group(3):
    perms += 0o040
if m.group(4):
    perms += 0o020
if m.group(5):
    perms += 0o004
if m.group(6):
    perms += 0o002
perm_str = '0'+oct(perms)[2:]

[morrone]

I would suggest that we always treat the decimal string as octal:

  z = int(perm_str, base=8)
  perm_str = '0'+oct(z)[:2]

Then the user doesn't have to be as careful with octal. All of these would work from a string:

 perm: "640"
 perm: "0640"
 perm: "0o640"

I don't think we need to support non-octal decimals for this.

[tom95858]

I think this would be staggeringly confusing for the user. No need to be clever. Support something we can document. Let's support what strtoul(x, NULL, 0) would do and then refer to this in our doc.

[morrone]

I think this would be staggeringly confusing for the user. No need to be clever. Support something we can document. Let's support what strtoul(x, NULL, 0) would do and then refer to this in our doc.

I can't see how interpreting "640" as rw-r-----, what basically every user would expect for permissions bits, is "staggeringly confusing" or even particularly clever. Could you elaborate on why that would be? Do you know any people that regularly do permission bits in base 10?

I think using base 8 translation for things that are octal like this is pretty ordinary. I don't claim to have thought that up.

I think most users would be rather confused to put in "640" and get, at best, "r--------". (Is there even any error checking for numbers with bits out of the expected range?)

[morrone]

I'll point out too, that "chmod" doesn't accept base-10 numbers, only base-8. Decimals digits are assumed to be base-8 as I have proposed.

Likewise, when dealing with file permissions in the "find" command, it only interprets digits as base-8.

The more that I look around, the only staggeringly confusing thing seems to be that ldmsd has ever accepted perms that were not in octal form.

[tom95858]

Yes @morrone, there is a third parameter to strtoul, which is the base. I was talking about the python format "0o123".isdecimal() which is what you proposed and returns false. This is a $1 problem on which OGC has spent $1000. Nick is working on adding the checking for any character other than 'r', 'w', and '-'...which is 1 line of code. I agree with you that specifying permissions in base 10 doesn't make much sense....which leaves...exactly what was suggested.

[morrone]

OK, you didn't say that. I'm just fine with "0o123".isdecimal() not working. That wasn't at all the driving issue. The main issue was making it octal-only rather than base 10. I supplied most of the code to make the changes that I suggested, and if it is a monetary issue to integrate them further, you could say that as well, and I'll supply the full patch.

[morrone]

or the regex:

  (0o)?\d+

instead of .decimal()

[tom95858]

@morrone, @nick-enoent, @emdonat happy with this? I need to merge it very soon.

[tom95858]

@nick-enoent could you please fix the conflicts and force push the result.

[nick-enoent]

@nick-enoent could you please fix the conflicts and force push the result.

Updated.