sysrc: refactor

[dlundgren]

SUMMARY

Refactors the sysrc module based on comments from prior PR's. Also fixes #10394.

Closes #10400.

ISSUE TYPE

• Refactoring Pull Request

COMPONENT NAME

sysrc

ADDITIONAL INFORMATION

Refactored the sysrc module.

[dlundgren]

I had been sitting on the actual code for a month, and just finished up with the tests this morning. I wanted to get this off my plate before I forgot about it again.

Sorry for a PR that competes with another one.

[dlundgren]

What are the differences between FreeBSD 14.2 - 1, FreeBSD 14.2 - 2 and FreeBSD 14.2 - 3?

Never mind, this was my fault on the test for not updating the correct file.

[dlundgren]

@vbotka Would you mind checking my logic on the way load_sysrc_value works?

I'm fairly certain that using -c is the correct answer here so that we don't have to parse the variables. Since we are using -n self.name to get the values when appending/subtracting, I figured it would work better to consolidate the value loading to one place.

[vbotka]

using -c is the correct answer

Yes, it is. load_sysrc_value works fine. This is better than config parsing.

When it is that far, it would be good to understand how sysrc updates rc.conf/rc.conf.d. I'm not sure -f works in all corner cases. I'll test it.

[felixfontein]

One potential problem is that this is a larger refactoring, which needs a very good reason to be backported to stable-10 as well (by default we only backport new features and refactorings to the current stable branch, which is currently stable-11). It is not clear to me how large this refactoring is, and whether it is safe enough to backport to stable-10 as well. Or should we apply #10400 to stable-10 and this PR to main and stable-11?

[felixfontein]

(Also please add a changelog fragment :-) )

[vbotka]

whether it is safe enough

IMO, it is as safe as before.

The error handling is missing (as before). For example, insufficient permissions are silently ignored in run_sysrc

-> return (rc, out, err)
(Pdb) p rc
1
(Pdb) p out
''
(Pdb) p err
'/usr/sbin/sysrc: cannot create /etc/rc.conf: Permission denied\n'

As a result, the task is reported changed, but the file has no changes.

I would like to proceed on top of this refactoring.

[vbotka]

I'm not sure -f works in all corner cases. I'll test it.

IMO, there are no problems. This PR should be merged as is. The error handling may be added later.

[dlundgren]

The error handling is missing (as before). For example, insufficient permissions are silently ignored in run_sysrc

'/usr/sbin/sysrc: cannot create /etc/rc.conf: Permission denied\n'

As a result, the task is reported changed, but the file has no changes.

Hopefully the permission check I added works to prevent this.

It did locally, but I have molecule using a vagrant and so I can easily create a test that has become: false. Within the Azure pipeline it connects as root, so I can't easily write a test for checking permissions.

[vbotka]

add tests for the use case this PR fixes

There is no reason to test the parsing. After refactoring, the module uses only "sysrc" to parse the configuration files. The sysrc output is not transformed in the module; hence, testing parsing would mean testing the sysrc binary, not the module.

[vbotka]

I want to propose this patch

@@ -152,7 +152,11 @@ class Sysrc(object):
             return changed(out.split(' -> ')[1].strip().split(self.delim))

     def present(self):
-        if self.load_sysrc_value() == self.value:
+        value = self.load_sysrc_value()
+        if value == self.value:
+            return False
+        if self.value is None:
+            self.value = value
             return False

         if not self.module.check_mode:
@@ -232,6 +236,7 @@ def main():
     try:
         sysrc = Sysrc(module, name, result['value'], result['path'], result['delim'], result['jail'])
         result['changed'] = getattr(sysrc, result['state'])()
+        result['value'] = getattr(sysrc, 'value')
     except OSError as err:
         module.fail_json(msg=str(err))

The option value is not required. If value is not used, this patch makes the module return the current value if state: present(default). This is how the sysrc binary behaves. For example,

shell> sysrc k1
k1: v1

Without this patch, the module replaces the current value, e.g. k1="None", if name is present.

[felixfontein]

Instead of getattr(sysrc, 'value') please use sysrc.value :)

[russoz]

Hi @dlundgren thanks for the contribution!

Couple of comments on it.

[vbotka]

Without the name validity test, the error name contains characters not allowed in shell is silently ignored. As a result, the module reports changed, but the file has no changes.

[dlundgren]

@vbotka thanks for finding that! I added the check back in and a test for it as well.