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 7 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: 14 additions & 6 deletions conf/cupsd.conf.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@
# complete description of this file.
#

# Log general information in error_log - change "@CUPS_LOG_LEVEL@" to "debug"
# for troubleshooting...
# Set LogLevel to debug for turning on troubleshooting
LogLevel @CUPS_LOG_LEVEL@
@CUPS_PAGE_LOG_FORMAT@

Expand All @@ -31,26 +30,35 @@ WebInterface @CUPS_WEBIF@
# Timeout after cupsd exits if idle (applied only if cupsd runs on-demand - with -l)
IdleExitTimeout @EXIT_TIMEOUT@

# Restrict access to the server...
# 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
<Location />
Order allow,deny
</Location>

# Restrict access to the admin pages...
# 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>

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

# Restrict access to log files...
# Access to the log files:
# - default action defined by Order', see 'man cupsd.conf'
# - use 'Allow'/'Deny' for configuring access
<Location /admin/log>
AuthType Default
Require user @SYSTEM
Expand Down
47 changes: 3 additions & 44 deletions cups/adminutil.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -698,7 +698,7 @@ cupsAdminSetServerSettings(
if (server_port <= 0)
server_port = IPP_PORT;

while (cupsFileGetConf(cupsd, line, sizeof(line), &value, &linenum))
while (_cupsFileGetConfAndComments(cupsd, line, sizeof(line), &value, &linenum))
{
if ((!_cups_strcasecmp(line, "Port") || !_cups_strcasecmp(line, "Listen")) &&
(remote_admin >= 0 || remote_any >= 0 || share_printers >= 0))
Expand All @@ -709,13 +709,10 @@ cupsAdminSetServerSettings(

if (remote_admin > 0 || remote_any > 0 || share_printers > 0)
{
cupsFilePuts(temp, "# Allow remote access\n");
cupsFilePrintf(temp, "Port %d\n", server_port);
}
else
{
cupsFilePuts(temp, "# Only listen for connections from the local "
"machine.\n");
cupsFilePrintf(temp, "Listen localhost:%d\n", server_port);
}

Expand Down Expand Up @@ -749,7 +746,6 @@ cupsAdminSetServerSettings(
localp = cupsGetOption("BrowseLocalProtocols", cupsd_num_settings,
cupsd_settings);

cupsFilePuts(temp, "# Share local printers on the local network.\n");
cupsFilePuts(temp, "Browsing On\n");

if (!localp)
Expand All @@ -763,7 +759,6 @@ cupsAdminSetServerSettings(
}
else
{
cupsFilePuts(temp, "# Disable printer sharing.\n");
cupsFilePuts(temp, "Browsing Off\n");
}
}
Expand All @@ -774,13 +769,10 @@ cupsAdminSetServerSettings(

if (debug_logging)
{
cupsFilePuts(temp,
"# Show troubleshooting information in error_log.\n");
cupsFilePuts(temp, "LogLevel debug\n");
}
else
{
cupsFilePuts(temp, "# Show general information in error_log.\n");
cupsFilePuts(temp, "LogLevel " CUPS_DEFAULT_LOG_LEVEL "\n");
}
}
Expand All @@ -800,8 +792,7 @@ cupsAdminSetServerSettings(
wrote_policy = 1;

if (!user_cancel_any)
cupsFilePuts(temp, " # Only the owner or an administrator can "
"cancel a job...\n"
cupsFilePuts(temp,
" <Limit Cancel-Job>\n"
Copy link
Member

Choose a reason for hiding this comment

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

Move this into the previous line.

" Order deny,allow\n"
" Require user @OWNER "
Expand Down Expand Up @@ -837,11 +828,6 @@ cupsAdminSetServerSettings(
{
wrote_admin_location = 1;

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

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

if (remote_admin)
Expand All @@ -856,13 +842,6 @@ cupsAdminSetServerSettings(
{
wrote_conf_location = 1;

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

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

if (remote_admin)
Expand All @@ -877,13 +856,6 @@ cupsAdminSetServerSettings(
{
wrote_log_location = 1;

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

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

if (remote_admin)
Expand All @@ -899,18 +871,6 @@ cupsAdminSetServerSettings(
{
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)
Expand Down Expand Up @@ -991,8 +951,7 @@ cupsAdminSetServerSettings(
wrote_policy = 1;

if (!user_cancel_any)
cupsFilePuts(temp, " # Only the owner or an administrator can cancel "
"a job...\n"
cupsFilePuts(temp,
" <Limit Cancel-Job>\n"
Copy link
Member

Choose a reason for hiding this comment

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

Move this into the previous line.

" Order deny,allow\n"
" Require user @OWNER "
Expand Down
126 changes: 126 additions & 0 deletions cups/file.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "debug-internal.h"
#include <sys/stat.h>
#include <sys/types.h>
#include <stdbool.h>
Copy link
Member

Choose a reason for hiding this comment

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

No need for bool.


# ifdef HAVE_LIBZ
# include <zlib.h>
Expand Down Expand Up @@ -335,6 +336,131 @@ _cupsFileCheckFilter(
#endif /* !_WIN32 */


/*
* 'cupsFileGetConfAndComments()' - Get line and comments from a configuration file.
Copy link
Member

Choose a reason for hiding this comment

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

The name in the comment doesn't match with the function definition.

*/

char * /* O - Line read or @code NULL@ on end of file or error */
_cupsFileGetConfAndComments(cups_file_t *fp, /* I - CUPS file */
char *buf, /* O - String buffer */
Copy link
Member

Choose a reason for hiding this comment

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

According DEVELOPING.md file every 8 spaces have to be a tab - please reflect that in the new code. I'll mark every place where there is a problem.

size_t buflen, /* I - Size of string buffer */
char **value, /* O - Pointer to value */
Copy link
Member

Choose a reason for hiding this comment

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

Use tab instead of every 8 spaces here.

int *linenum) /* IO - Current line number */
{
char *ptr; /* Pointer into line */


/*
* Range check input...
*/

DEBUG_printf(("2cupsFileGetConfAndComments(fp=%p, buf=%p, buflen=" CUPS_LLFMT
", value=%p, linenum=%p)", (void *)fp, (void *)buf, CUPS_LLCAST buflen, (void *)value, (void *)linenum));
zdohnal marked this conversation as resolved.
Show resolved Hide resolved

if (!fp || (fp->mode != 'r' && fp->mode != 's') ||
!buf || buflen < 2 || !value)
{
if (value)
*value = NULL;

return (NULL);
}

/*
* Read the next line...
*/

*value = NULL;

while (cupsFileGets(fp, buf, buflen))
{
(*linenum) ++;

/*
zdohnal marked this conversation as resolved.
Show resolved Hide resolved
* Strip the comment from inline comment...
*/
if ((ptr = strchr(buf, '#')) != NULL)
{
int index = (int) (ptr - buf);
for(int i=index-1; i>=0; i--)
Copy link
Member

Choose a reason for hiding this comment

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

Code style issues (spaces)

if (!_cups_isspace(buf[i]))
{
buf[index] = '\0';
Copy link
Member

Choose a reason for hiding this comment

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

We should put NULL terminator instead of last whitespace in this case.

break;
}
}

/*
* Strip leading whitespace...
*/

for (ptr = buf; _cups_isspace(*ptr); ptr ++);

if (ptr > buf)
_cups_strcpy(buf, ptr);

zdohnal marked this conversation as resolved.
Show resolved Hide resolved
/*
* See if there is anything left...
*/

if (buf[0] != '#')
Copy link
Member

Choose a reason for hiding this comment

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

We have to check whether there is anything in the buffer, and if so, check for # - this big if-else block is unnecessary and add unnecessary indentation - the good practice is to not nest the code too much, the code should be as straight as possible.

if (buf[0])
{
  // return comment if any
  if (buf[0] == '#')
    return ptr;

  /*
   * Otherwise grab any value and return...
   */
....

{
/*
* Yes, grab any value and return...
*/

for (ptr = buf; *ptr; ptr ++)
if (_cups_isspace(*ptr))
break;

if (*ptr)
{
/*
* Have a value, skip any other spaces...
*/

while (_cups_isspace(*ptr))
*ptr++ = '\0';

if (*ptr)
*value = ptr;

/*
* Strip trailing whitespace and > for lines that begin with <...
*/

ptr += strlen(ptr) - 1;

if (buf[0] == '<' && *ptr == '>')
*ptr-- = '\0';
else if (buf[0] == '<' && *ptr != '>')
{
/*
* Syntax error...
*/

*value = NULL;
return (buf);
}

while (ptr > *value && _cups_isspace(*ptr))
*ptr-- = '\0';
}

/*
* Return the line...
*/

return (buf);
}
else
Copy link
Member

Choose a reason for hiding this comment

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

The snippet I've provided in the previous review was pseudocode - not actual code which you can put into the code and have it done, but a code which is supposed to express an idea and has to be adjusted for actual source code.
Additionally I mentioned the if-else structure you introduced in the previous commit is redundant. Think about which part could be removed.

return (buf);
}

return (NULL);
}


/*
* 'cupsFileClose()' - Close a CUPS file.
*
Expand Down
1 change: 1 addition & 0 deletions cups/file.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ typedef struct _cups_file_s cups_file_t;/**** CUPS file type ****/
* Prototypes...
*/

extern char *_cupsFileGetConfAndComments(cups_file_t *fp, char *buf, size_t buflen, char **value, int *linenum) _CUPS_PRIVATE;
extern int cupsFileClose(cups_file_t *fp) _CUPS_API_1_2;
extern int cupsFileCompression(cups_file_t *fp) _CUPS_API_1_2;
extern int cupsFileEOF(cups_file_t *fp) _CUPS_API_1_2;
Expand Down