probe

[miekg]

• Check systemd supported options
• bla

[pires]

This code can easily be covered by tests ;)

I would prefer this code to be there when actual probing takes place but since this is not a draft PR, I'm assuming you are looking to have this merged. Therefore, I did a thorough review.

[pires]

Doesn't need to be exported. Or may it does as this is moved into the unit package where other unit stuff is?

[pires]

Or may it does as this is moved into the unit package where other unit stuff is?

[pires]

If this is something you are proposing to have merged without the actual probing (otherwise, it would be a draft PR, right?), please, state very clearly that this is a fake probe, as nothing is actually probed.

[pires]

Doesn't need to be exported. Or may it does as this is moved into the unit package where other unit stuff is?

[pires]

I'd say warning logs.

[pires]

Suggested change

	// supportedOptions contains a mappnig for supported systemd options. If an option
	// supportedOptions contains a mapping for supported systemd options. If an option

[pires]

Given we know exactly what options are used in systemk, supportedOptions would be complete for the purpose of probing. Why would this be a scenario?

[pires]

Suggested change

	// If name is the empty string this is a noop.
	// If name is an empty string, this is a noop.

[pires]

Suggested change

	// If name is the empty string this is a noop.
	// If name is an empty string, this is a noop.

[pires]

Please, add a comment above stating that this is empty only when the option is not supported by the current systemd and no alternative is available.

[pires]

Please, add a comment above stating that this is empty only when the option is not supported by the current systemd and no alternative is available.

[miekg]

Thanks Good point on providing alternative options. Every option that we use should be probed. I wonder if we can take RH7's systemd's version as a base of supported options

…

On Sat, 23 Jan 2021, 20:28 Pires, ***@***.***> wrote: ***@***.**** commented on this pull request. This code can easily be covered by tests ;) I would prefer this code to be there when actual probing takes place but since this is not a draft PR, I'm assuming you are looking to have this merged. Therefore, I did a thorough review. ------------------------------ In internal/provider/probe.go <#74 (comment)> : > @@ -0,0 +1,39 @@ +package provider + +// supportedOptions contains a mappnig for supported systemd options. If an option +// is supported the key name will be returned. Unsupported either return an +// empty string (really not supported) or an alternative option that's better +// than nothing at all. +var supportedOptions = map[string]string{ + "BindReadOnlyPaths": "BindReadOnlyPaths", +} + +// ProbeSupportedOptions checks it the options in SupportedOptions are +// supported by the systemd version running on this system. It will emit Info +// logs for each unsupported option. +func ProbeSupportedOptions() { Doesn't need to be exported. Or may it does as this is moved into the unit package where other unit stuff is? ------------------------------ In internal/provider/probe.go <#74 (comment)> : > +// supported by the systemd version running on this system. It will emit Info +// logs for each unsupported option. +func ProbeSupportedOptions() { + for option := range supportedOptions { + ok := probe(option) + switch option { + case "BindReadOnlyPaths": + if !ok { + supportedOptions[option] = "BindPaths" // drop the RO bit + } + } + } +} + +// probe probes system to see if option is supported. +func probe(option string) bool { If this is something you are proposing to have merged without the actual probing (otherwise, it would be a draft PR, right?), please, state very clearly that this is a fake probe, as nothing is actually probed. ------------------------------ In internal/provider/probe.go <#74 (comment)> : > + switch option { + case "BindReadOnlyPaths": + if !ok { + supportedOptions[option] = "BindPaths" // drop the RO bit + } + } + } +} + +// probe probes system to see if option is supported. +func probe(option string) bool { + return true +} + +// Option return the option that is supported by the detected systemd. +func Option(option string) string { Doesn't need to be exported. Or may it does as this is moved into the unit package where other unit stuff is? ------------------------------ In internal/provider/probe.go <#74 (comment)> : > @@ -0,0 +1,39 @@ +package provider + +// supportedOptions contains a mappnig for supported systemd options. If an option +// is supported the key name will be returned. Unsupported either return an +// empty string (really not supported) or an alternative option that's better +// than nothing at all. +var supportedOptions = map[string]string{ + "BindReadOnlyPaths": "BindReadOnlyPaths", +} + +// ProbeSupportedOptions checks it the options in SupportedOptions are +// supported by the systemd version running on this system. It will emit Info I'd say warning logs. ------------------------------ In internal/provider/probe.go <#74 (comment)> : > @@ -0,0 +1,39 @@ +package provider + +// supportedOptions contains a mappnig for supported systemd options. If an option ⬇️ Suggested change -// supportedOptions contains a mappnig for supported systemd options. If an option +// supportedOptions contains a mapping for supported systemd options. If an option ------------------------------ In internal/provider/probe.go <#74 (comment)> : > @@ -0,0 +1,39 @@ +package provider + +// supportedOptions contains a mappnig for supported systemd options. If an option +// is supported the key name will be returned. Unsupported either return an ⬇️ Suggested change -// is supported the key name will be returned. Unsupported either return an +// is supported, the value equals the key, otherwise the value is either an ------------------------------ In internal/provider/probe.go <#74 (comment)> : > @@ -0,0 +1,39 @@ +package provider + +// supportedOptions contains a mappnig for supported systemd options. If an option +// is supported the key name will be returned. Unsupported either return an +// empty string (really not supported) or an alternative option that's better +// than nothing at all. It's unclear how it is *better than nothing at all*. Is the value another option that we know is supported by older systemd versions? ------------------------------ In internal/provider/probe.go <#74 (comment)> : > @@ -0,0 +1,39 @@ +package provider + +// supportedOptions contains a mappnig for supported systemd options. If an option +// is supported the key name will be returned. Unsupported either return an +// empty string (really not supported) or an alternative option that's better +// than nothing at all. +var supportedOptions = map[string]string{ + "BindReadOnlyPaths": "BindReadOnlyPaths", +} + +// ProbeSupportedOptions checks it the options in SupportedOptions are ⬇️ Suggested change -// ProbeSupportedOptions checks it the options in SupportedOptions are +// ProbeSupportedOptions checks if the options in supportedOptions are ------------------------------ In internal/provider/probe.go <#74 (comment)> : > +// empty string (really not supported) or an alternative option that's better +// than nothing at all. +var supportedOptions = map[string]string{ + "BindReadOnlyPaths": "BindReadOnlyPaths", +} + +// ProbeSupportedOptions checks it the options in SupportedOptions are +// supported by the systemd version running on this system. It will emit Info +// logs for each unsupported option. +func ProbeSupportedOptions() { + for option := range supportedOptions { + ok := probe(option) + switch option { + case "BindReadOnlyPaths": + if !ok { + supportedOptions[option] = "BindPaths" // drop the RO bit This switch may grow a bit. Why not have another map with the alternatives? Eg alternativeOptions := map[string]string{ "BindReadOnlyPaths": "BindPaths", } for option := range supportedOptions { if !probe(option) { supportedOptions[option] = alternativeOptions[option] } } ------------------------------ In internal/provider/probe.go <#74 (comment)> : > + } + } + } +} + +// probe probes system to see if option is supported. +func probe(option string) bool { + return true +} + +// Option return the option that is supported by the detected systemd. +func Option(option string) string { + opt, ok := supportedOptions[option] + if !ok { + // not found in map, return option as-is + return option Given we know exactly what options are used in systemk, supportedOptions would be complete for the purpose of probing. Why would this be a scenario? ------------------------------ In internal/unit/file.go <#74 (comment)> : > @@ -84,7 +84,11 @@ func (u *File) String() string { } // Insert adds name=value to section and returns a newly parsed pointer to File. +// If name is the empty string this is a noop. ⬇️ Suggested change -// If name is the empty string this is a noop. +// If name is an empty string, this is a noop. ------------------------------ In internal/unit/file.go <#74 (comment)> : > @@ -98,7 +102,11 @@ func (u *File) Insert(section, name string, value ...string) *File { } // Overwrite overwrites name=value in the section and returns a new File. +// If name is the empty string this is a noop. ⬇️ Suggested change -// If name is the empty string this is a noop. +// If name is an empty string, this is a noop. ------------------------------ In internal/unit/file.go <#74 (comment)> : > func (u *File) Insert(section, name string, value ...string) *File { + if name == "" { Please, add a comment above stating that this is empty only when the option is not supported by the current systemd and no alternative is available. ------------------------------ In internal/unit/file.go <#74 (comment)> : > func (u *File) Overwrite(section, name string, value ...string) *File { + if name == "" { Please, add a comment above stating that this is empty only when the option is not supported by the current systemd and no alternative is available. ------------------------------ In internal/provider/probe.go <#74 (comment)> : > @@ -0,0 +1,39 @@ +package provider + +// supportedOptions contains a mappnig for supported systemd options. If an option +// is supported the key name will be returned. Unsupported either return an +// empty string (really not supported) or an alternative option that's better +// than nothing at all. +var supportedOptions = map[string]string{ + "BindReadOnlyPaths": "BindReadOnlyPaths", +} + +// ProbeSupportedOptions checks it the options in SupportedOptions are +// supported by the systemd version running on this system. It will emit Info +// logs for each unsupported option. +func ProbeSupportedOptions() { Or may it does as this is moved into the unit package where other unit stuff is? ------------------------------ In internal/provider/probe.go <#74 (comment)> : > @@ -0,0 +1,39 @@ +package provider + +// supportedOptions contains a mappnig for supported systemd options. If an option +// is supported the key name will be returned. Unsupported either return an +// empty string (really not supported) or an alternative option that's better +// than nothing at all. I'd also add that the actual probing doesn't happen here but, eventually, when ProbeSupportedOptions is called. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#74 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACWIWY7HVTAFD6ORWJ2MC3S3MPMHANCNFSM4WP2ZQAQ> .

[pires]

I think it's a safe bet because it's systemd 219, a really old version. We can also not actually probe and basically do it by hand in code based on this small document.

[pires]

For instance, TemporaryFileSystem is introduced in 238. Both BindPaths and BindReadOnlyPaths are introduced in 233.

[pires]

Maybe the idea of alternative unit attributes needs to mature. For instance, mapping a string to an alternative string may be too limited. Say we are on RHEL7 and systemd 219 (<233) where we don't have BindPaths and BindReadOnlyPaths. Is there an alternative? And if there is, is it just another option or a combination of multiple options?