Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor part of server settings code #351

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

reddevillg
Copy link
Contributor

It's difficult to fix some corner case, such as

  1. turn share_printers and remote_any on, keep remote_admin off
  2. turn share_printers off.
    remote_admin is still take effect.

found a similar problem #158

@michaelrsweet michaelrsweet self-requested a review March 14, 2022 16:55
@michaelrsweet michaelrsweet self-assigned this Mar 14, 2022
@michaelrsweet michaelrsweet added enhancement New feature or request priority-low labels Mar 14, 2022
@michaelrsweet michaelrsweet added this to the v2.4.x milestone Mar 14, 2022
@michaelrsweet
Copy link
Member

We need to do a lot of regression testing here - the proposed changes look like they might cause problems when updating a customized cupsd.conf file, and we absolutely want to minimize how much of the cupsd.conf file is changed. The most common support advice is "enable debug logging with cupsctl --debug-logging", but if doing so wrecks an otherwise working configuration then we'll have angry users...

@reddevillg
Copy link
Contributor Author

The intention here is only change the order of options in cupsd.conf. If this is not desired, we need write options in two place as before.

  1. First time encountering a option.
  2. Write any required missing option.

But we don‘t need to record if the option was changed (don't need test the condition remote_admin >=0, just assign old_remote_admin to remote_admin in first place), because it will write to new cupsd.conf file anyway。It is this part that complicates things。I can update the patch if it makes sense to you。

@michaelrsweet
Copy link
Member

@reddevillg Thanks for the feedback! I'm not specifically opposed to your changes, I just want to make sure they don't introduce any regressions... I'd be doing the same if I was making the changes, if only because I've broken things a time or two in the past... :)

@AreaZR
Copy link
Contributor

AreaZR commented Nov 6, 2022

Any update on this?

@zdohnal
Copy link
Member

zdohnal commented Nov 7, 2022

Hi @AtariDreams ,

I'm planning refactor the cupsctl code to prevent removal of comments, so cupsctl can be used more widely (this will need more explicit comments in the cupsd.conf, so then we won't need to add comments of our own during the execution of the command) - I'm keeping this PR opened for later checking the functionality from PR is in the new code as well.

@zdohnal
Copy link
Member

zdohnal commented May 31, 2023

Hi @reddevillg ,

thank you for the PR! We are trying to implement comment preserving functionality in #640 , so it would be great if the PR didn't add comments and didn't change the position of directives, because if it does, it can move the directive from the comment which is relevant to the directive.

Would you mind adjusting the PR based on this premise or you don't mind if I do it?

Either way we should add cupsctl tests into test suite as well before merging this PR.

@reddevillg
Copy link
Contributor Author

@zdohnal I'm not sure if "comment preserving" is a good idea. Thinking about someone adding a comment to explain the following directives, cupsctl changes server settings later, but preserving the comment which explain the directives changed before.
I think user(admin) should aware that if change cupsd.conf by manual, it should do it always.

@zdohnal
Copy link
Member

zdohnal commented Jun 1, 2023

@zdohnal I'm not sure if "comment preserving" is a good idea. Thinking about someone adding a comment to explain the following directives, cupsctl changes server settings later, but preserving the comment which explain the directives changed before.

We've updated the default comments to be as generic as they can be, so they could make sense with more configuration combinations. So unless the user doesn't add the exact comment like "This allows remote access", we're ok. But in such cases IMO it is a user error - if user adds comments on his own, he is responsible for its contents.

I think user(admin) should aware that if change cupsd.conf by manual, it should do it always.

IMO the combined use of manual edit and of a tool is ok - tool should manipulate with directives and its values, limit or policy scopes and shouldn't touch comments at all, and manual edit is here for more precise changes.

@reddevillg
Copy link
Contributor Author

reddevillg commented Jun 2, 2023

@zdohnal OK, I can live with that :)
Back to the original issue, i'm glad to continue the PR, I will see how to do it in a few days.

We guarantee that the following conditions are met:

share_printers  remote_any   remote_admin  Listen      Browsering Location-/   Location-/admin
   off            off           off       localhost:631   off        deny            deny
   off            off           on          631           off       Allow @Local   Allow @Local
   off            on            off         631           off       Allow all        deny
   off            on            on          631           off       Allow all      Allow all
   on             off           off         631           on        Allow @Local     deny
   on             off           on          631           on        Allow @Local   Allow @Local
   on             on            off         631           on        Allow all        deny
   on             on            on          631           on        Allow all      Allow all
@michaelrsweet michaelrsweet modified the milestones: v2.4.x, v2.5 Apr 2, 2024
@michaelrsweet michaelrsweet requested review from zdohnal and removed request for michaelrsweet April 2, 2024 23:00
@michaelrsweet
Copy link
Member

Definitely not for 2.4.x, maybe for 2.5.

@zdohnal Can I get you to look at this since it affects the cupsd.conf code?

@michaelrsweet michaelrsweet removed their assignment Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority-low
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants