Added the functionality of preserved comments in cupsd.conf when cupsctl is used with command line arguments. #631

Ankit3002 · 2023-03-02T20:21:41Z

#408
I resolved this issue. I created a function cupsFileGetConfAndComments in cups/cups/file.c to read cupsd file. The function reads the cupsd.conf file. The settings which got to be changed are added in the cups option arrays.

Whenever cupsFileGetConfAndComments read cupsd.conf it changes those settings which need to be changed and comments got preserved in the cupsd.conf file as it is.

michaelrsweet

We don't want a new public API for this so please make it static to adminutil.c or a private API (_cupsFileGetConfAndComments) in file.c.

Ankit3002 · 2023-03-03T10:26:09Z

We don't want a new public API for this so please make it static to adminutil.c or a private API (_cupsFileGetConfAndComments) in file.c.

@michaelrsweet I marked the API as Private, Is it fine now.

michaelrsweet

Some general coding style issues that need to be resolved:

Space after reserved words like if
Space after commas in argument lists
Blank lines before/after blocks of code
Use comment[0] = '\0' instead of _cups_strcpy

Also, the new private API needs a length parameter for the comment string.

michaelrsweet · 2023-03-03T16:09:29Z

cups/adminutil.c

@@ -459,6 +459,9 @@ cupsAdminSetServerSettings(
  cups_option_t	*cupsd_settings,	/* New settings */
 		*setting;		/* Current setting */
  _cups_globals_t *cg = _cupsGlobals();	/* Global data */
+  char		comment_check[1024];		/* Comment already? */


Check comment indentation - should start at column 41 (5th tab stop)

michaelrsweet · 2023-03-03T16:09:53Z

cups/adminutil.c

@@ -459,6 +459,9 @@ cupsAdminSetServerSettings(
  cups_option_t	*cupsd_settings,	/* New settings */
 		*setting;		/* Current setting */
  _cups_globals_t *cg = _cupsGlobals();	/* Global data */
+  char		comment_check[1024];		/* Comment already? */
+  char 		comment[1024]="";		/* Comment */


Coding style - spaces before and after "=".

michaelrsweet · 2023-03-03T16:10:07Z

cups/adminutil.c

@@ -544,8 +547,11 @@ cupsAdminSetServerSettings(
  DEBUG_printf(("1cupsAdminSetServerSettings: old user_cancel_any=%d",
                old_user_cancel_any));

+


Remove extra whitespace...

michaelrsweet · 2023-03-03T16:10:20Z

cups/adminutil.c

@@ -666,7 +672,7 @@ cupsAdminSetServerSettings(
 /*
  * Copy the old file to the new, making changes along the way...
  */
-
+  comment_line 	       = 0;


Blank line before

michaelrsweet · 2023-03-03T16:11:01Z

cups/adminutil.c

@@ -698,7 +704,13 @@ cupsAdminSetServerSettings(
  if (server_port <= 0)
    server_port = IPP_PORT;

-  while (cupsFileGetConf(cupsd, line, sizeof(line), &value, &linenum))
+  /* Writing the Header Comment */


Use block command style and fix grammar, e.g.:

/* * Write the header comment... */

michaelrsweet · 2023-03-03T16:15:27Z

cups/adminutil.c

 	cupsd_num_settings = cupsAddOption(line, value, cupsd_num_settings,
                                           &cupsd_settings);
+


Eliminate extra blank line here, but need one after the close brace...

michaelrsweet · 2023-03-03T16:17:23Z

cups/file-private.h

@@ -83,7 +83,8 @@ typedef void (*_cups_fc_func_t)(void *context, _cups_fc_result_t result,
 /*
 * Prototypes...
 */
-
+
+extern char		*_cupsFileGetConfAndComments(char *comment,int *comment_line,cups_file_t *fp, char *buf, size_t buflen, char **value, int *linenum) _CUPS_PRIVATE;


Fix indentation to match

michaelrsweet · 2023-03-03T16:18:11Z

cups/file.c

+
+char *					/* O  - Line read or @code NULL@ on end of file or error */
+_cupsFileGetConfAndComments(
+		char *comment,		/* Comment */


Fix indentation - 4 spaces when the arguments can't be fully indented and line up all variable names.

Also, you need to add a "commentlen" argument to specify the size of the comment buffer.

Finally, "I" prefix for input variables, "O" for output, and "IO" for input + output (so "I" for comment and commentlen, "IO" for comment_line)

michaelrsweet · 2023-03-03T16:18:54Z

cups/file.c

+    	// Capturing multi line comment...
+    	if(*linenum == (*comment_line)+1)
+    	{
+    	  strcat(comment,"\n");


OK, need to use strlcat here as we don't want buffer overflows...

michaelrsweet · 2023-03-03T16:21:45Z

cups/file.c

+        ptr += strlen(ptr) - 1;
+
+        if (buf[0] == '<' && *ptr == '>')
+	  *ptr-- = '\0';


Coding style - if any of the if-else blocks uses braces, use braces for all of them.

Ankit3002 · 2023-03-04T13:52:42Z

@michaelrsweet I have fixed the coding style issues now.

michaelrsweet

OK, looks better. @zdohnal I'd appreciate you looking at this to make sure this resolves the issues you had reported.

cups/adminutil.c

Ankit3002 · 2023-03-05T06:55:27Z

@michaelrsweet I declared the prototype extern char *_cupsFileGetConfAndComments(char *comment, size_t commentlen, int *comment_line, cups_file_t *fp, char *buf, size_t buflen, char **value, int *linenum) _CUPS_PRIVATE;
in file.h instead of file-private.h to avoid implicit function declaration . Is it fine ?

Ankit3002 · 2023-03-09T05:07:41Z

@michaelrsweet I declared the prototype extern char *_cupsFileGetConfAndComments(char *comment, size_t commentlen, int *comment_line, cups_file_t *fp, char *buf, size_t buflen, char **value, int *linenum) _CUPS_PRIVATE; in file.h instead of file-private.h to avoid implicit function declaration . Is it fine ?

@michaelrsweet Any update on this ?

Ankit3002 · 2023-03-09T05:07:52Z

@michaelrsweet I declared the prototype extern char *_cupsFileGetConfAndComments(char *comment, size_t commentlen, int *comment_line, cups_file_t *fp, char *buf, size_t buflen, char **value, int *linenum) _CUPS_PRIVATE; in file.h instead of file-private.h to avoid implicit function declaration . Is it fine ?

@michaelrsweet Any update on this ?

zdohnal · 2023-03-09T07:23:23Z

I don't see any security issues in PR, so I've let the test run - going back to the review.

zdohnal

Hi,

thank you for the PR! Do see my comments regarding the code.

Additionally I've tested the PR, it works in most cases , but it moves inline comments to a new line before the original inline comment.

Can you look into it as well?

Semantically this behavior is correct, but some users can complain about moving the comment from being inline. WDYT, @jsmeix?

zdohnal · 2023-03-09T07:17:02Z

cups/adminutil.c

@@ -546,6 +549,8 @@ cupsAdminSetServerSettings(

  cupsFreeOptions(cupsd_num_settings, cupsd_settings);

+
+


Unneeded newlines.

zdohnal · 2023-03-09T07:34:34Z

cups/adminutil.c

@@ -1038,29 +1062,46 @@ cupsAdminSetServerSettings(
      * Write the new value in its place, without indentation since we
      * only support setting root directives, not in sections...
      */
-
+      


Additional newline.

zdohnal · 2023-03-09T07:34:56Z

cups/adminutil.c

      cupsFilePrintf(temp, "%s %s\n", line, val);
    }
    else if (value)
-    {
+    {      


Unintended whitespace.

zdohnal · 2023-03-09T07:36:59Z

cups/adminutil.c

      if (!in_policy && !in_location)
      {
       /*
        * Record the non-policy, non-location directives that we find
 	* in the server settings, since we cache this info and record it
 	* in cupsAdminGetServerSettings()...
 	*/
-


It might not follow the coding guidelines, but let's fix it in a separate PR regarding code style and focus on code style on new/changed lines your PR affects.

zdohnal · 2023-03-09T07:37:33Z

cups/adminutil.c

 	cupsd_num_settings = cupsAddOption(line, value, cupsd_num_settings,
-                                           &cupsd_settings);
+                                           &cupsd_settings);                                          


Unintended change in indentation.

zdohnal · 2023-03-09T14:44:28Z

cups/file.c

+    	{
+    	  comment[0] = '\0';
+    	  strlcat(comment, "\n", commentlen);
+    	  _cups_strcpy(comment,ptr);


Missing space after ','.

zdohnal · 2023-03-09T14:50:55Z

cups/file.h

@@ -59,6 +59,7 @@ typedef struct _cups_file_s cups_file_t;/**** CUPS file type ****/
 * Prototypes...
 */

+extern char			*_cupsFileGetConfAndComments(char *comment, size_t commentlen, int *comment_line, cups_file_t *fp, char *buf, size_t buflen, char **value, int *linenum) _CUPS_PRIVATE;


The indentation does not match with the rest of the code and please change the parameter's order.

zdohnal · 2023-03-09T14:54:28Z

cups/file.c

+    	if(*linenum == (*comment_line)+1)
+    	{
+    	  strlcat(comment, "\n", commentlen);
+      	  strlcat(comment, ptr, commentlen);


We have to check whether there is a space in comment for another line, and if not, allocate larger space (by a constant, f.e. 1024).

zdohnal · 2023-03-09T15:13:44Z

cups/file.c

+                ", value=%p, linenum=%p)", (void *)fp, (void *)buf, CUPS_LLCAST buflen, (void *)value, (void *)linenum));
+
+  if (!fp || (fp->mode != 'r' && fp->mode != 's') ||
+      !buf || buflen < 2 || !value)


We should check comment and commentlen here as well and if they aren't sane, return NULL.

zdohnal · 2023-03-09T15:44:54Z

cups/adminutil.c

+  * Write the header comment... 
+  */
+
+  cupsFilePuts(temp,"#\n"


If you run cupsctl more times, this comment section gets duplicated. The section is in the default cupsd.conf, so we can remove it.

jsmeix · 2023-03-10T08:25:48Z

@zdohnal
I think as long as things are kept semantically correct
it should be no problem to change the layout of comments.

I do not understand what you mean with 'inline comments'.

According to
https://openprinting.github.io/cups/doc/man-cupsd.conf.html

Each line in the file can be a configuration directive,
a blank line, or a comment.
...
Comment lines start with the # character.

so comments can be only whole lines that start with '#'.

This even means that indented comments e.g. as in
https://github.com/OpenPrinting/cups/blob/master/conf/cupsd.conf.in#L62

<Policy default>
  # Job/subscription privacy...

are not in compliance with the "man cupsd.conf" syntax specification.

But it seems in practice
"comment lines start with the '#' character
as the first non-horizontal-whitespace character"
or perhaps it is only that
"comment lines start with the '#' character
as the first non-space character"
where 'space' is only ASCII 0x20 and
horizontal whitespace is 'space' and 'horizontal tab' ASCII 0x09
or something like that?

By the way:
"man cupsd.conf" doesn't explicitly tell about the encoding.
So explore fantastic opportunities with various non-ASCII
(preferably UTF-8/Unicode) whitespace characters ;-)

Seriously:
What about non-ASCII characters in comments?
Users may write comments in their native language.
Could this be a problem here?

Somewhat off topic but perhaps even more serious:
In "man cups-files.conf" I notice 'filename' e.g.

AccessLog filename
ErrorLog filename

Nowadays filenames can be arbitrary sequences of non-NULL bytes
(except '/' ASCII 0x2F but including arbitrary whitespace characters),
cf. the section "Non-ASCII characters in file names" in
https://en.opensuse.org/SDB:Plain_Text_versus_Locale
Could this be a general problem in CUPS?

Ankit3002 · 2023-03-10T22:03:00Z

@zdohnal I made the changes as you requested. Please review them.

zdohnal

Hi @Ankit3002 ,

I'm sorry - I've probably misunderstood how you deal with multiline comments - I thought I concatenate the lines into one big array, but IIUC you print it after every line, correct?

If I'm right (now :D ), we don't need to realloc - IMO 1024 characters are okay for one line. But still we have to check it and cut 'ptr' to prevent overflow (IMO such long comment lines are not common, and if anyone complains, we can adjust it later).

Otherwise there are some indentation problems/removed lines etc, it would be great if you checked that as well.

Thank you for your work!

zdohnal · 2023-03-13T09:33:24Z

cups/adminutil.c

+  char		comment_check[1024];	/* Comment already? */
+  char 		*comment;	/* Comment */
+  int		comment_linenum;	/* Comment line? */
+  int    commentlen;


Wrong indentation.

@zdohnal No I 'm concatenating the comment lines for multiline comments.

zdohnal · 2023-03-13T09:48:56Z

cups/adminutil.c

-
+
+  commentlen = 1024;
+  comment = (char* ) malloc(sizeof(char)*commentlen);


I'm sorry - I might misunderstand the dealing with multiline comments - do you concatenate multiple comment lines? Or do you print a comment line in every iteration?

In case you print the line in every iteration, we don't need to reallocate the string (let's cut the line in case it is longer than 1024). I'm sorry for mystification.

@zdohnal I 'm concatenating multiple comment lines . Basically, whenever comment lines don't have a blank line in between them, I concatenate the comments.
Should I change it to static size of 1024 or is it fine ?

Why don't print a single line every time? We wouldn't need a dynamic allocation.

zdohnal · 2023-03-13T09:49:29Z

cups/adminutil.c

@@ -697,8 +704,8 @@ cupsAdminSetServerSettings(

  if (server_port <= 0)
    server_port = IPP_PORT;
-
-  while (cupsFileGetConf(cupsd, line, sizeof(line), &value, &linenum))
+


Unintended indentation.

zdohnal · 2023-03-13T09:50:01Z

cups/adminutil.c

@@ -1038,7 +1054,8 @@ cupsAdminSetServerSettings(
      * Write the new value in its place, without indentation since we
      * only support setting root directives, not in sections...
      */
-


Removed empty line.

zdohnal · 2023-03-13T12:30:36Z

cups/adminutil.c

-                                           &cupsd_settings);
+	    * in the server settings, since we cache this info and record it
+	    * in cupsAdminGetServerSettings()...
+	    */
+        cupsd_num_settings=cupsAddOption(line, value, cupsd_num_settings, &cupsd_settings);                                          
+      }


Why has this part been changed? I don't see any code change, only in style - if the current code style doesn't match the devel guidelines, please revert the style changes of this part of code.

zdohnal · 2023-03-13T13:20:28Z

cups/file.c

+
+    	// Capturing single line comment...
+    else


Squash the lines and move the comment into the scope.

zdohnal · 2023-03-13T13:37:11Z

cups/file.c

+      else
+      {
+        // Capturing multi line comment...
+    if(*linenum == (*comment_linenum)+1)


Missing space between ') + 1'

zdohnal · 2023-03-13T14:58:50Z

cups/file.c

+    if(*linenum == (*comment_linenum)+1)
+    {
+        // Increase the size of comment buffer when it overflows
+      if(strlen(comment) >= (*commentlen))


IMO we should check the length of 'ptr' and if it doesn't fit in 'comment', cut the 'ptr'.

zdohnal · 2023-03-13T15:00:16Z

cups/file.c

+    {
+       (*commentlen) = strlen(ptr+2);
+       comment[0] = '\0';
+       strlcat(comment, "\n", *commentlen);
+       _cups_strcpy(comment, ptr);
+       *comment_linenum = *linenum;
+    }


The whole if-else is incorrectly indented.

zdohnal · 2023-03-13T17:30:53Z

cups/file.c

+    	// Capturing single line comment...
+    else
+    {
+       (*commentlen) = strlen(ptr+2);


This will overwrite the current size of the allocated buffer with the length of current comment, which you probably don't want to (since you want to use the value for checking whether you can write into it).

Ankit3002 · 2023-03-17T12:15:59Z

@zdohnal I solve the indentation issue. Regarding size of comment variable When a comment has no blank line in between than I concatenate and when a comment has just one line, I simply copy it.
Please let me know if these changes are fine enough ?

Ankit3002 · 2023-03-21T05:41:26Z

@zdohnal Any update regarding the changes that I made above ?

zdohnal · 2023-03-21T14:56:11Z

Hi @Ankit3002 ,

I'm sorry I was answering to other issues.

Actually why do we have to concatenate the multiline comments and can't we print the line into the file every time we read it? Every comment line start with #, so the IIUC we can just print it after reading from cupsd.conf. Or am I wrong?

Concatenating multiple comment line would lead us into dynamic allocation requirements, where we would have to pass the size of allocated array in and back from the new function (because we would have to resize the other array as well).

zdohnal · 2023-03-21T14:58:16Z

cups/adminutil.c

-
+
+  commentlen = 1024;
+  comment = (char* ) malloc(sizeof(char)*commentlen);


Why don't print a single line every time? We wouldn't need a dynamic allocation.

zdohnal · 2023-03-21T14:58:49Z

cups/adminutil.c

+
+  commentlen = 1024;
+  comment = (char* ) malloc(sizeof(char)*commentlen);
+  comment_linenum 	   = 0;


Incorrect indentation.

zdohnal · 2023-03-21T15:02:04Z

cups/adminutil.c

@@ -775,7 +782,7 @@ cupsAdminSetServerSettings(
      if (debug_logging)
      {
        cupsFilePuts(temp,
-	             "# Show troubleshooting information in error_log.\n");
+	             "\n# Show troubleshooting information in error_log.\n");


Since we preserve the comments, we should adjust the line, remove it from here and move it to default cupsd.conf - otherwise we get duplicity.

A new comment in default cupsd.conf can look like:

# Set LogLevel to debug for turning on troubleshooting

Ankit3002 · 2023-03-22T07:56:43Z

Hi @Ankit3002 ,

I'm sorry I was answering to other issues.

Actually why do we have to concatenate the multiline comments and can't we print the line into the file every time we read it? Every comment line start with #, so the IIUC we can just print it after reading from cupsd.conf. Or am I wrong?

Concatenating multiple comment line would lead us into dynamic allocation requirements, where we would have to pass the size of allocated array in and back from the new function (because we would have to resize the other array as well).

Hi @zdohnal … I ‘m sorry , I probably overcomplicate the logic, I thought of gathering the comment into the comment variable until some configuration got written. But this will make the code more overcomplicated and I have to consider a lot of edge cases like putting indentation, reallocation for multiliine comments and new line characters in the comments.

I think it would be better if we print the comment line every time when we read it from the file.

I test the code with that approach and it’s working fine.

Because this PR has a lot of useless commits, should I create a new PR and make those modifications there?

zdohnal · 2023-03-22T12:41:56Z

@Ankit3002 you can commit to this PR and then rebase (if necessary) and squash the commits by (the steps count with you having the OpenPrinting/cups repo as 'upstream' name):

$ # in your fork in master branch, get the latest upstream master
$ git pull --rebase upstream master
$ # and push it
$ git push
$ # checkout to your branch
$ git checkout <branch>
$ # rebase it to the current master - there shouldn't be any conflicts, in case they are, you need to solve them (by making changes to the code, so your commit would apply cleanly to the master commit)
$ git rebase -i master
$ # use force push
$ git push --force
$ # find out SHA ID of the current master branch's HEAD (f.e. by ```git log --pretty=:%H -1 master```)
$ # soft reset your branch to the SHA ID from master branch
$ git reset --soft <SHA_ID>
$ # write nice new commit message for commit
$ git commit -a
$ git push --force

tillkamppeter · 2023-05-22T12:35:43Z

@Ankit3002 Why did you close this PR? Did you give up on it? I think we should merge it.

zdohnal · 2023-05-22T12:54:49Z

@tillkamppeter there is a new PR for this #640 .

michaelrsweet requested changes Mar 2, 2023

View reviewed changes

michaelrsweet reviewed Mar 3, 2023

View reviewed changes

michaelrsweet requested a review from zdohnal March 4, 2023 18:40

michaelrsweet reviewed Mar 4, 2023

View reviewed changes

michaelrsweet previously approved these changes Mar 4, 2023

View reviewed changes

github-advanced-security bot found potential problems Mar 4, 2023

View reviewed changes

cups/adminutil.c Fixed Show fixed Hide fixed

Ankit3002 dismissed michaelrsweet’s stale review via 0d8b55c March 5, 2023 06:53

zdohnal requested changes Mar 9, 2023

View reviewed changes

zdohnal requested changes Mar 13, 2023

View reviewed changes

Ankit3002 requested a review from zdohnal March 16, 2023 07:28

zdohnal requested changes Mar 21, 2023

View reviewed changes

Ankit3002 closed this Mar 23, 2023

Ankit3002 force-pushed the master branch from 9a3b29b to 6885402 Compare March 23, 2023 05:20

		@@ -544,8 +547,11 @@ cupsAdminSetServerSettings(
		DEBUG_printf(("1cupsAdminSetServerSettings: old user_cancel_any=%d",
		old_user_cancel_any));

		cupsd_num_settings = cupsAddOption(line, value, cupsd_num_settings,
		&cupsd_settings);

		@@ -546,6 +549,8 @@ cupsAdminSetServerSettings(

		cupsFreeOptions(cupsd_num_settings, cupsd_settings);



		commentlen = 1024;
		comment = (char* ) malloc(sizeof(char)*commentlen);

Added the functionality of preserved comments in cupsd.conf when cupsctl is used with command line arguments. #631

Added the functionality of preserved comments in cupsd.conf when cupsctl is used with command line arguments. #631

Conversation

Ankit3002 commented Mar 2, 2023

michaelrsweet left a comment

Choose a reason for hiding this comment

Ankit3002 commented Mar 3, 2023 • edited Loading

michaelrsweet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ankit3002 commented Mar 4, 2023

michaelrsweet left a comment

Choose a reason for hiding this comment

Ankit3002 commented Mar 5, 2023

Ankit3002 commented Mar 9, 2023

Ankit3002 commented Mar 9, 2023

zdohnal commented Mar 9, 2023

zdohnal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsmeix commented Mar 10, 2023 • edited Loading

Ankit3002 commented Mar 10, 2023

zdohnal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ankit3002 Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ankit3002 commented Mar 17, 2023 • edited Loading

Ankit3002 commented Mar 21, 2023

zdohnal commented Mar 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ankit3002 commented Mar 22, 2023

zdohnal commented Mar 22, 2023

tillkamppeter commented May 22, 2023

zdohnal commented May 22, 2023

Ankit3002 commented Mar 3, 2023 •

edited

Loading

jsmeix commented Mar 10, 2023 •

edited

Loading

Ankit3002 Mar 15, 2023 •

edited

Loading

Ankit3002 commented Mar 17, 2023 •

edited

Loading