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.

Sign up for GitHub

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

Jump to bottom

Added the functionality of Preserved Comments in cupsd.conf when cups… #640

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Added the functionality of Preserved Comments in cupsd.conf when cups… #640

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
4d13be6
Added the functionality of Preserved Comments in cupsd.conf when cups…
Ankit3002 Mar 22, 2023
b9c57d1
Done some changes in cupsFileGetConf() to preserve conmments in cupsd…
Ankit3002 Mar 22, 2023
ccf71a9
Added: Private Api to read comments and configuration from cupsd.conf
Ankit3002 Mar 29, 2023
d999b0d
Fixed : Coding Style issues
Ankit3002 Apr 1, 2023
1c0ae05
Updated: cupsd.conf.in
Ankit3002 Apr 3, 2023
d08c54f
Added: Support to read inline comments from cupsd.conf
Ankit3002 Apr 5, 2023
0a6b3b3
Simplified : _cupsFileGetConfAndComments function logic
Ankit3002 Apr 6, 2023
d351867
Fixed: Coding style issues
Ankit3002 Apr 13, 2023
d92236a
Fixed: Coding Style Issues
Ankit3002 Apr 13, 2023
f241386
Simplified: _cupsFileGetConfAndComments()
Ankit3002 Apr 16, 2023
952fd3f
Fixed: Missing handling of escaped #
Ankit3002 Apr 16, 2023
7470d73
Added: escaped # support
Ankit3002 Apr 17, 2023
d9c1986
Fixed: Coding Style Issues
Ankit3002 Apr 19, 2023
04b6730
Fixed coding style issues
Ankit3002 Apr 19, 2023
07e9450
Added : Private Api for Striping Inline Comment
Ankit3002 Jun 2, 2023
5150a47
Added : Debug Statement for cupsFileStripComment
Ankit3002 Jun 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions cups/adminutil.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,6 +459,7 @@ cupsAdminSetServerSettings(
cups_option_t *cupsd_settings, /* New settings */
*setting; /* Current setting */
_cups_globals_t *cg = _cupsGlobals(); /* Global data */
int linenum_check; /* same linenumber? */


/*
Expand Down Expand Up @@ -667,6 +668,7 @@ cupsAdminSetServerSettings(
* Copy the old file to the new, making changes along the way...
*/

linenum_check = 0;
cupsd_num_settings = 0;
in_admin_location = 0;
in_cancel_job = 0;
Expand Down Expand Up @@ -700,6 +702,16 @@ cupsAdminSetServerSettings(

while (cupsFileGetConf(cupsd, line, sizeof(line), &value, &linenum))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ankit3002 We can't change the behavior of public function like below.

The idea was to have a new internal function, which has the same arguments as cupsFileGetConf(), but it will be able to return comments via line array. In general, it will be almost similar functionality as cupsFileGetConf(), but it will return comment line (without leading and trailing whitespaces) as well as normal lines.

{
if (strchr(line, '#') != NULL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Several issues:

  1. linenum checking looks irrelevant
  2. once a new function returns comment line, you can check the first character
  3. the conditional should be part of the if- else if - else control structure below - then you don't need to check for # again in the end
  4. you should use indent variable when printing the line, as it is done in other cupsFilePut()s to have the correct indentation in the cupsd.conf

{
if(linenum == linenum_check+1)
cupsFilePrintf(temp, "%s\n", line);
else
cupsFilePrintf(temp, "\n%s\n", line);
linenum_check = linenum;
}


if ((!_cups_strcasecmp(line, "Port") || !_cups_strcasecmp(line, "Listen")) &&
(remote_admin >= 0 || remote_any >= 0 || share_printers >= 0))
{
Expand Down Expand Up @@ -774,8 +786,6 @@ cupsAdminSetServerSettings(

if (debug_logging)
{
cupsFilePuts(temp,
"# Show troubleshooting information in error_log.\n");
cupsFilePuts(temp, "LogLevel debug\n");
}
else
Expand Down Expand Up @@ -1058,7 +1068,11 @@ cupsAdminSetServerSettings(
cupsFilePrintf(temp, "%*s%s %s\n", indent, "", line, value);
}
else
cupsFilePrintf(temp, "%*s%s\n", indent, "", line);
{
if (strchr(line, '#') == NULL)
cupsFilePrintf(temp, "%*s%s\n", indent, "", line);
}

}

/*
Expand Down
1 change: 1 addition & 0 deletions cups/file.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -769,6 +769,7 @@ cupsFileGetConf(cups_file_t *fp, /* I - CUPS file */
ptr --;
}

return (buf);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes behavior of public function, we mustn't do that. We need new internal function.

*ptr = '\0';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will become a dead code if we return before that.

Additionally I've realized we should deal with inline comments in case they appear (even though they are prohibited by cupsd.conf man page) in a way.

So I propose:

  • check whether there is text before '#':
    • if there is, strip the comment, since it is invalid and process further in the function and return the text before the '#' char
    • if there is only whitespace before '#', strip the leading whitespace and return the comment.

So the block if ((ptr = strchr(buf, '#')) != NULL) can become a handler for inline comments, which will strip the comment in case there is text before it, and the execution will process further.

}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally you should review every cupsFilePut() below in cupsAdminSetServerSettings() which add a comment - once we start supporting comments, we can't put new comments in.

Rephrase the comments mentioned in cupsFilePut() and put it to the proper place at conf/cupsd.conf.in, f.e. when line contains "" and we are in /admin location:

The code:

  if (remote_admin)
          cupsFilePuts(temp, "  # Allow remote administration...\n");
  else
          cupsFilePuts(temp, "  # Restrict access to the admin pages...\n");

The default cupsd.conf.in:

# Restrict access to the admin pages...
<Location /admin>
  AuthType Default
  Require user @SYSTEM
  Order allow,deny
</Location>

so once you remove the cupsFilePut(), update the cupsd.conf.in to look like this f.e.:

# Access to the admin pages:
# - default action defined by Order', see 'man cupsd.conf'
# - use 'Allow'/'Deny' for configuring access
<Location /admin>
  AuthType Default
  Require user @SYSTEM
  Order allow,deny
</Location>

Copy link
Author

@Ankit3002 Ankit3002 Mar 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else if (in_root_location && (remote_admin >= 0 || remote_any >= 0 || share_printers >= 0)) { wrote_root_location = 1;

if (remote_admin > 0 && share_printers > 0)
      cupsFilePuts(temp, "  # Allow shared printing and remote "
                     "administration...\n");
else if (remote_admin > 0)
      cupsFilePuts(temp, "  # Allow remote administration...\n");
else if (share_printers > 0)
      cupsFilePuts(temp, "  # Allow shared printing...\n");
else if (remote_any > 0)
      cupsFilePuts(temp, "  # Allow remote access...\n");
else
      cupsFilePuts(temp, "  # Restrict access to the server...\n");

    cupsFilePuts(temp, "  Order allow,deny\n");

if (remote_admin > 0 || remote_any > 0 || share_printers > 0)
{
  if (remote_any >= 0)
    cupsFilePrintf(temp, "  Allow %s\n", remote_any > 0 ? "all" : "@LOCAL");
  else
    cupsFilePrintf(temp, "  Allow %s\n", old_remote_any > 0 ? "all" : "@LOCAL");
}
  }`

@zdohnal for the above part what should be the comment that I need to write in conf/cupsd.conf.in ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example:

# Access to the server root (/):
# - default action defined by 'Order', see 'man cupsd.conf'
# - use 'Allow'/'Deny' for configuring access
# - allowing access is required for printer sharing or remote administration

Expand Down