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

Ankit3002 · 2023-03-23T06:33:02Z

…ctl is used with command line arguments

….conf

Ankit3002 · 2023-03-23T06:39:19Z

@zdohnal I made the changes as you requested. Now I 'm printing the comment whenever I read it from cupsd.conf file. I delete the useless commits of Last PR and erroneously drop the last PR. Please review this PR and do let me know if these changes are fine or not ?

zdohnal

IMO we can simplify the PR a little bit more - we can get comments via line itself.

zdohnal · 2023-03-23T15:16:42Z

cups/adminutil.c

@@ -698,8 +700,11 @@ 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, comment, sizeof(comment)))


What about returning the comment via line parameter and remove the new additional parameters? Then the comment printout can happen via last cupsFilePrintf(). WDYT?

zdohnal · 2023-03-23T15:41:25Z

cups/file.c

+ */
+
+ if ((ptr = strchr(buf, '#')) != NULL)
+ {


If we go with passing comments via line, we can detect the comment, then remove the leading and trailing whitespaces and return buf.

@zdohnal I made the changes as you requested. Now I 'm using line variable to get comments. Please review them.

zdohnal

We can't change the behavior of public function - I'm sorry if my comment sounded like it, I still meant to use a new private function.

Additionally we have to take care of adding comments in cupsAdminSetServerSettings().

zdohnal · 2023-03-24T06:20:53Z

cups/adminutil.c

@@ -700,6 +702,16 @@ cupsAdminSetServerSettings(

 while (cupsFileGetConf(cupsd, line, sizeof(line), &value, &linenum))


@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.

zdohnal · 2023-03-24T06:28:40Z

cups/adminutil.c

@@ -700,6 +702,16 @@ cupsAdminSetServerSettings(

 while (cupsFileGetConf(cupsd, line, sizeof(line), &value, &linenum))
 {
+ if (strchr(line, '#') != NULL)


Several issues:

linenum checking looks irrelevant

once a new function returns comment line, you can check the first character

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

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

zdohnal · 2023-03-24T06:39:06Z

cups/file.c

@@ -769,6 +769,7 @@ cupsFileGetConf(cups_file_t *fp, /* I - CUPS file */
 ptr --;
 }

+ return (buf);


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

zdohnal · 2023-03-24T07:14:20Z

cups/file.c

@@ -769,6 +769,7 @@ cupsFileGetConf(cups_file_t *fp, /* I - CUPS file */
 ptr --;
 }

+ return (buf);
 *ptr = '\0';
 }
 }


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>

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 ?

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

Ankit3002 · 2023-03-29T04:11:38Z

@zdohnal I made the changes as you requested. I create a private Api to read comments and lines.

The private api is using same line variable to read both comments and configurations.
I changed the comments in cupsd.conf.in as well.
I moved the if else block at the end as well. Linenum checking is used only to not include blank lines between multiline comments.

Ankit3002 · 2023-04-02T14:11:15Z

@zdohnal I made the changes as you requested.
1.I create a private Api to read comments and lines.
2. The private api use same line variable to read both comments and configurations.
3. I changed the comments in cupsd.conf.in as well.
4. I moved the if else block at the end as well. Linenum checking is used only to not include blank lines between multiline comments.

@zdohnal Any update on this ?

zdohnal · 2023-04-03T10:22:55Z

Hi @Ankit3002 ,

I'm sorry, I was on vacation - I'll review this today or tomorrow.

Ankit3002 · 2023-04-03T11:33:08Z

Hi @Ankit3002 ,

I'm sorry, I was on vacation - I'll review this today or tomorrow.

No issues !

Ankit3002 · 2023-04-03T12:35:36Z

@zdohnal cupsd.conf.in file is placed in .gitignore, should I remove it from there ? as I won't be able to push the changes in cupsd.conf.in because of this

zdohnal · 2023-04-03T12:47:20Z

Are you sure? I don't see the file in .gitignore.

Ankit3002 · 2023-04-03T13:46:55Z

Are you sure? I don't see the file in .gitignore.

I 'm sorry , I got confused it with cupsd.conf

zdohnal · 2023-04-04T06:27:06Z

@zdohnal I made the changes as you requested. I create a private Api to read comments and lines.
1. The private api is using same line variable to read both comments and configurations.

Ok, please update the comments and debug printf to match the new function - since it was copied from cupsGetConf(), the comments+debugs stayed the same.

2. I changed the comments in cupsd.conf.in as well.

cupsAdminSetServerSettings() still puts comments into cupsd.conf file, which we can't allow once we support reading comments in the functions (otherwise we would end up with duplicity and bigger and bigger cupsd.conf with every cupsctl call).
Please remove the other cupsFilePuts() which add comments from the function and migrate them into sane comments in cupsd.conf.in where it is possible.

3. I moved the if else block at the end as well. Linenum checking is used only to not include blank lines between multiline comments.

I would let blank lines between comments as it is - people adds blank lines between comments for readability, and the code will be simpler.

More comments in the review in the moment.

zdohnal · 2023-04-04T06:28:36Z

cups/adminutil.c

- cupsFilePrintf(temp, "%*s%s\n", indent, "", line);
+ {
+ if (strchr(line, '#') == NULL)
+ cupsFilePrintf(temp, "%*s%s\n", indent, "", line);


As I said in the comment, I would let here the simple cupsFilePrintf() as it was there - the code will be simpler and we get rid of the new variable.

zdohnal · 2023-04-04T06:29:04Z

cups/file.c

+ */
+
+ DEBUG_printf(("2cupsFileGetConf(fp=%p, buf=%p, buflen=" CUPS_LLFMT
+ ", value=%p, linenum=%p)", (void *)fp, (void *)buf, CUPS_LLCAST buflen, (void *)value, (void *)linenum));


Remains of copy&paste :)

zdohnal · 2023-04-04T06:32:04Z

cups/file.c

@@ -842,6 +842,142 @@ cupsFileGetConf(cups_file_t *fp, /* I - CUPS file */
 }


+/*


Move the function definition to an alphabetically correct place in the file (under _cupsFileCheckFilter())

zdohnal · 2023-04-04T06:32:26Z

cups/file.c

+ }
+
+ /*
+ * Read the next non-comment line...


Copy&paste remain...

zdohnal · 2023-04-04T06:32:46Z

cups/file.c

+ (*linenum) ++;
+
+ /*
+ * Strip any comments...


Copy and paste leftover.

zdohnal · 2023-04-04T07:09:57Z

cups/file.c

+ }
+
+ return (buf);
+ *ptr = '\0';


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.

zdohnal · 2023-04-04T07:18:53Z

cups/file.c

+
+ if (buf[0])
+ {
+ /*


Here we could check whether the first char is # and if so, return buf. With appropriate comment.

Ankit3002 · 2023-04-05T21:14:58Z

@zdohnal I made the changes as you requested. Please review.
This is how it works:

I am checking whether the line i 'm reading is comment or not .
if it's a comment than, than i will check for inline comments, and returns the text before '#' as you asked above. ... if it's a simple comment than i simply returns it.
if it's not a comment , than I read the configurations according to the earlier function.
Also now the function would take care about blank lines as well.
I have also placed the function at correct alphabetical order in file.c

zdohnal · 2023-04-06T11:56:29Z

@zdohnal I made the changes as you requested. Please review. This is how it works:

1. I am checking whether the line i 'm reading is comment or not .

2. if it's a comment than, than i will check for inline comments, and returns the text before '#' as you asked above. ... if it's a simple comment than i simply returns it.

You can simplify it a lot - inline comments are forbidden, so we can remove them and process as cupsGetConf() did. So in general check for '#' and then iterate over the array towards the start - if there is another text before the #, remove the # and text after # comment, don't return and let the function process the text before #.
In case there is no text, let the function process further - it will remove the leading whitespace and in if (buf[0]) scope you check if the first char is # - if it is - return the buf.

In the current code there lot of duplicit code, which can be simplified by the algorithm above.

3. if it's not a comment , than I read the configurations according to the earlier function.

4. Also now the function would take care about blank lines as well.

5. I have also placed the function at correct alphabetical order in file.c

There are still more cupsFilePuts() which have to be removed from function and added to cupsd.conf.in if it makes sense.

There are public holidays in my country, I'll be back on Tuesday.

Ankit3002 · 2023-04-08T11:04:50Z

@zdohnal I made the changes as you requested. Please review. This is how it works:
1. I am checking whether the line i 'm reading is comment or not .

2. if it's a comment than, than i will check for inline comments, and returns the text before '#' as you asked above. ... if it's a simple comment than i simply returns it.
You can simplify it a lot - inline comments are forbidden, so we can remove them and process as cupsGetConf() did. So in general check for '#' and then iterate over the array towards the start - if there is another text before the #, remove the # and text after # comment, don't return and let the function process the text before #. In case there is no text, let the function process further - it will remove the leading whitespace and in if (buf[0]) scope you check if the first char is # - if it is - return the buf.

In the current code there lot of duplicit code, which can be simplified by the algorithm above.
3. if it's not a comment , than I read the configurations according to the earlier function.

4. Also now the function would take care about blank lines as well.

5. I have also placed the function at correct alphabetical order in file.c
There are still more cupsFilePuts() which have to be removed from function and added to cupsd.conf.in if it makes sense.

There are public holidays in my country, I'll be back on Tuesday.

@zdohnal I simplified the logic as you requested, Please review them when you will be back from holidays, thank you!
Now,The only cupsFilePuts() in adminutil.c are the ones which are used to write missing info.

Ankit3002 · 2023-04-12T18:27:03Z

@zdohnal I made the changes as you requested. Please review. This is how it works:
1. I am checking whether the line i 'm reading is comment or not .

2. if it's a comment than, than i will check for inline comments, and returns the text before '#' as you asked above. ... if it's a simple comment than i simply returns it.
You can simplify it a lot - inline comments are forbidden, so we can remove them and process as cupsGetConf() did. So in general check for '#' and then iterate over the array towards the start - if there is another text before the #, remove the # and text after # comment, don't return and let the function process the text before #. In case there is no text, let the function process further - it will remove the leading whitespace and in if (buf[0]) scope you check if the first char is # - if it is - return the buf.
In the current code there lot of duplicit code, which can be simplified by the algorithm above.
3. if it's not a comment , than I read the configurations according to the earlier function.

4. Also now the function would take care about blank lines as well.

5. I have also placed the function at correct alphabetical order in file.c
There are still more cupsFilePuts() which have to be removed from function and added to cupsd.conf.in if it makes sense.
There are public holidays in my country, I'll be back on Tuesday.
@zdohnal I simplified the logic as you requested, Please review them when you will be back from holidays, thank you! Now,The only cupsFilePuts() in adminutil.c are the ones which are used to write missing info.

@zdohnal Any update on above ?

zdohnal

See the comments in the code and there are still cupsFilePuts() which insert comments, which mustn't happen.

zdohnal · 2023-04-13T14:01:20Z

cups/file.c

+ if ((ptr = strchr(buf, '#')) != NULL)
+ {
+ int index = (int) (ptr - buf);
+ for(int i=index-1; i>=0; i--)


Code style issues (spaces)

zdohnal · 2023-04-13T14:03:23Z

cups/file.c

+ for(int i=index-1; i>=0; i--)
+ if (!_cups_isspace(buf[i]))
+ {
+ buf[index] = '\0';


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

zdohnal · 2023-04-13T14:12:42Z

cups/file.c

+ * See if there is anything left...
+ */
+
+ if (buf[0] != '#')


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... */ ....

zdohnal · 2023-04-13T14:15:15Z

cups/adminutil.c

@@ -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"


Move this into the previous line.

zdohnal · 2023-04-13T14:19:07Z

cups/adminutil.c

@@ -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"


Move this into the previous line.

zdohnal · 2023-04-13T14:20:36Z

cups/file.c

@@ -22,6 +22,7 @@
 #include "debug-internal.h"
 #include <sys/stat.h>
 #include <sys/types.h>
+#include <stdbool.h>


No need for bool.

Ankit3002 · 2023-04-13T19:30:14Z

@zdohnal I simplified the code according to the above suggestions, Please review them. Thank you !

zdohnal · 2023-04-14T05:07:54Z

cups/file.c

+ {
+ int index = (int) (ptr - buf);
+ while (index > 0 && _cups_isspace(buf[index]))
+ buf[index--] = '\0';


setting the NULL terminator in every iteration is redundant - set it only once you know there is a non-whitespace character before #.

have you tested the code for a line " # comment"? What happens for such a line with the current code?

what will happen if escaped # appears? - # - this is not a comment.

do you think this can be solved with pointer arithmetic instead of introducing a new variable index?

Help: look at cupsFileGetConf() how it solves those situations, think about what is needed in our case, adjust it accordingly and use it here.

cups/file.c

zdohnal · 2023-04-14T05:24:55Z

cups/file.c

+
+ if (buf[0])
+ {
+ // return comment if any


I know that I used it in my pseudocode, but on a second thought it looks out of place when all other one line comments are written as multiline comments. Would you mind turning it into multiline comment and rephrase the comment, so it looks like proper English instead of a personal note :D .

zdohnal · 2023-04-14T05:34:30Z

cups/file.c

@@ -335,6 +335,131 @@ _cupsFileCheckFilter(
 #endif /* !_WIN32 */


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


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

zdohnal · 2023-04-14T05:35:53Z

cups/file.c

+
+ return (buf);
+ }
+ else


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.

Ankit3002 · 2023-04-16T16:54:34Z

@zdohnal Please review the changes that i made. Thanks!

Now the code is using pointer arithmetic, there is no use of index variable.
I removed the if-else block and simplified the code.
I corrected the function definition

zdohnal · 2023-04-17T08:49:38Z

cups/file.c

+ */
+ if ((ptr = strchr(buf, '#')) != NULL)
+ {
+ ptr--;


You don't have to move the pointer, if you check the character before your current position - see cupsFileGetConf

zdohnal · 2023-04-17T08:51:44Z

cups/file.c

+ if ((ptr = strchr(buf, '#')) != NULL)
+ {
+ ptr--;
+ while(ptr >= buf)


You will get an address in ptr which is before buf pointer with this conditional. Please fix it.

zdohnal · 2023-04-17T08:52:08Z

cups/file.c

+ break;
+ }
+ ptr--;
+ }


Missing handling of escaped # .

cups/file.c

Ankit3002 · 2023-04-17T11:18:15Z

@zdohnal I resolved the above issues... Now code will also takes care of line like this "--#--something" since this is not an inline comment so it won't get remove.
also i put back the if block , please review Thank you for the above clarification !
The code for removing inline comment look like this

    /*
     * Remove the inline comment...
     */
     if ((ptr = strchr(buf, '#')) != NULL && _cups_isspace(*(ptr-1)))
     {
        while(ptr > buf)
        {
         // Null-terminate the string after the last non-whitespace
         if(!_cups_isspace(*(ptr-1)))
         {
           *ptr = '\0';
           break;
         }
         ptr--;
        }
     }

cups/file.c

zdohnal · 2023-04-17T12:48:38Z

cups/file.c

+ /*
+ * Remove the inline comment...
+ */
+ if ((ptr = strchr(buf, '#')) != NULL && _cups_isspace(*(ptr-1)))


Please check how cupsFileGetConf() deals with escaped # . This way we would leave '' in the output file, which is not wanted.

zdohnal · 2023-04-17T12:50:08Z

cups/file.c

+ while(ptr > buf)
+ {
+ // Null-terminate the string after the last non-whitespace
+ if(!_cups_isspace(*(ptr-1)))


It would be great if you used ptr[-1] instead of *(ptr-1) - it is shorter and in alignment with cupsFileGetConf().

Ankit3002 · 2023-04-17T14:09:38Z

@zdohnal I made the changes as you requested . Please review them, The below is how we are taking care of inline comments now.

     if ((ptr = strchr(buf, '#')) != NULL)
     {
        while(ptr > buf)
        {
         // Null-terminate the string after the last non-whitespace, unless the '#' character is escaped by a backslash ('\')
         if(!_cups_isspace(ptr[-1]) && (ptr == buf || ptr[-1] != '\\'))
         {
           *ptr = '\0';
           break;
         }
         ptr--;
        }
     }

zdohnal · 2023-04-18T08:39:29Z

cups/file.c

+ while(ptr > buf)
+ {
+ // Null-terminate the string after the last non-whitespace, unless the '#' character is escaped by a backslash ('\')
+ if(!_cups_isspace(ptr[-1]) && (ptr == buf || ptr[-1] != '\\'))


We should deal with escaped # before we start the loop - we should enter the scope with loop only if the # is not escaped. See the cupsFileGetConf() function - you can even deal with it the similar way.

zdohnal · 2023-04-18T11:27:33Z

cups/file.c

+ */
+ if(buf[0])
+ {
+ if (buf[0] == '#')


I've thought a bit about dealing with the empty lines in the yesterday evening. We can return buf here together with buf[0] == '#', so the condition would look like:

/* * If we got empty line or comment, return it... */ if (!buf[0] || buf[0] == '#') return buf;

and we can remove the if(buf[0]) before that.

We just need to add a new if else if in cupsAdminSetServerSettings():

while (_cupsFileGetConfandComments(cupsd, line, sizeof(line), &value, &linenum)) { /* * Preserve empty lines */ if (!line[0]) cupsFilePuts(temp, "\n"); else if ((!_cups_strcasecmp(line, "Port") || !_cups_strcasecmp(line, "Listen")) &&

This should do the trick - WDYT? I guess you might show something similar in the past, but I didn't get it in the past, sorry for that :D .

zdohnal

I've reviewed the code from code style POV - tabs weren't used on places where they should be and the indentation is shifted since the second loop.

Would you mind looking into it?

zdohnal · 2023-04-18T11:51:32Z

cups/file.c

+
+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 */


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.

zdohnal · 2023-04-18T11:52:52Z

cups/file.c

+_cupsFileGetConfAndComments(cups_file_t *fp, /* I - CUPS file */
+ char *buf, /* O - String buffer */
+ size_t buflen, /* I - Size of string buffer */
+ char **value, /* O - Pointer to value */


Use tab instead of every 8 spaces here.

cups/file.c

zdohnal · 2023-04-18T11:53:52Z

cups/file.c

+ while(ptr > buf)
+ {
+ // Null-terminate the string after the last non-whitespace, unless the '#' character is escaped by a backslash ('\')
+ if(!_cups_isspace(ptr[-1]) && (ptr == buf || ptr[-1] != '\\'))
+ {
+ *ptr = '\0';
+ break;
+ }
+ ptr--;
+ }


Use tab instead of every 8 spaces here.

zdohnal · 2023-04-18T12:02:41Z

cups/file.c

+ /*
+ * Remove the inline comment...
+ */
+ if ((ptr = strchr(buf, '#')) != NULL)
+ {
+ while(ptr > buf)
+ {
+ // Null-terminate the string after the last non-whitespace, unless the '#' character is escaped by a backslash ('\')
+ if(!_cups_isspace(ptr[-1]) && (ptr == buf || ptr[-1] != '\\'))
+ {
+ *ptr = '\0';
+ break;
+ }
+ ptr--;
+ }
+ }


Incorrect indentation - every line (except for line with opening multiblock comment /*) is indented by even number of whitespaces - (2 spaces -> 4 spaces -> 6 spaces -> 1 tab -> 1 tab and 2 spaces etc). The initial indentation here is 5, which is odd number.

zdohnal · 2023-04-18T12:04:45Z

cups/file.c

+ /*
+ * Strip leading whitespace...
+ */
+
+ for (ptr = buf; _cups_isspace(*ptr); ptr ++);
+
+ if (ptr > buf)
+ _cups_strcpy(buf, ptr);
+
+ /*
+ * Return the comment if any...
+ */
+ if(buf[0])
+ {
+ if (buf[0] == '#')
+ return buf;
+
+ /*
+ * Otherwise 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);
+
+ }


The indentation is incorrect till this part - incorrect number of whitespaces and no tabs were used.

Ankit3002 · 2023-04-19T18:43:52Z

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

Ankit3002 · 2023-04-28T06:17:33Z

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

@zdohnal Any Update on this ?

zdohnal

Hi,

good thinking about possibility of having more # on one line! IMO it would be great if that could be handled by a function, so we can call it recursively there. Would you mind looking into it?
There are other code style issues, please look into them as well.

Thank you in advance!

I'm going to vacation for next two weeks and I'll return on 15th May, so I won't be available for reviews till I return, so see you in two weeks!

zdohnal · 2023-04-28T06:08:00Z

cups/file.c

+ * Remove the backslash and continue searching for unescaped '#'...
+ */
+ _cups_strcpy(ptr-1, ptr);
+ ptr = strchr(ptr+1, '#');


Good thinking here! Could you rework the functionality (recursively checking for #, because it can indicate forbidden inline comment or escaped #) into a static local function, which can be called recursively? So we can just do once:

if ((ptr = strchr(buf, '#')) != NULL) cups_handle_hash(ptr, buf);

this way you don't have to check for escaped # in the loop again, and the while loop can be in else scope, so we spare one conditional check.

There are code style issues as well (ptr-1 -> ptr - 1 , the comments are shifted by one space - look into other code in the file to see how they should look).

zdohnal · 2023-04-28T06:09:03Z

cups/file.c

+ if (ptr > buf)
+ _cups_strcpy(buf, ptr);
+
+ /*


Please change the comment here - now we don't return only comment, but in case of empty line as well.

cups/file.c

zdohnal · 2023-05-31T14:45:57Z

Hi @Ankit3002 ,

have you got time to look into the PR?

Ankit3002 · 2023-06-01T02:31:29Z

Hi @Ankit3002 ,

have you got time to look into the PR?

Yes, I will push the changes today.

Ankit3002 · 2023-06-02T20:26:06Z

@zdohnal Please review the changes that I made ...

Ankit3002 · 2023-06-09T07:04:34Z

@zdohnal Please review the changes that I made ...

@zdohnal Any Update on this ?

michaelrsweet · 2024-04-02T22:58:14Z

Definitely not for 2.4.x, maybe for 2.5.

@zdohnal Will leave this review process to you since you've been looking at this code more than me lately...

Ankit3002 added 2 commits March 22, 2023 16:23

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

4d13be6

…ctl is used with command line arguments

Done some changes in cupsFileGetConf() to preserve conmments in cupsd…

b9c57d1

….conf

zdohnal requested changes Mar 23, 2023

View reviewed changes

zdohnal requested changes Mar 24, 2023

View reviewed changes

Added: Private Api to read comments and configuration from cupsd.conf

ccf71a9

Fixed : Coding Style issues

d999b0d

Updated: cupsd.conf.in

1c0ae05

zdohnal requested changes Apr 4, 2023

View reviewed changes

Added: Support to read inline comments from cupsd.conf

d08c54f

Simplified : _cupsFileGetConfAndComments function logic

0a6b3b3

zdohnal requested changes Apr 13, 2023

View reviewed changes

Ankit3002 added 2 commits April 13, 2023 21:41

Fixed: Coding style issues

d351867

Fixed: Coding Style Issues

d92236a

zdohnal requested changes Apr 14, 2023

View reviewed changes

Ankit3002 added 2 commits April 16, 2023 18:56

Simplified: _cupsFileGetConfAndComments()

f241386

Fixed: Missing handling of escaped #

952fd3f

zdohnal requested changes Apr 17, 2023

View reviewed changes

Added: escaped # support

7470d73

zdohnal requested changes Apr 18, 2023

View reviewed changes

Ankit3002 added 2 commits April 19, 2023 18:53

Fixed: Coding Style Issues

d9c1986

Fixed coding style issues

04b6730

zdohnal requested changes Apr 28, 2023

View reviewed changes

zdohnal mentioned this pull request May 22, 2023

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

Closed

zdohnal mentioned this pull request May 31, 2023

Refactor part of server settings code #351

Open

zdohnal added this to the v2.4.x milestone May 31, 2023

Added : Private Api for Striping Inline Comment

07e9450

Added : Debug Statement for cupsFileStripComment

5150a47

michaelrsweet modified the milestones: v2.4.x, v2.5 Apr 2, 2024

michaelrsweet assigned zdohnal Apr 2, 2024

		@@ -700,6 +702,16 @@ cupsAdminSetServerSettings(

		while (cupsFileGetConf(cupsd, line, sizeof(line), &value, &linenum))

		@@ -842,6 +842,142 @@ cupsFileGetConf(cups_file_t fp, / I - CUPS file */
		}


		/*

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

Are you sure you want to change the base?

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

Conversation

Ankit3002 commented Mar 23, 2023

Ankit3002 commented Mar 23, 2023 • edited

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

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

Ankit3002 Mar 27, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ankit3002 commented Mar 29, 2023 • edited

Ankit3002 commented Apr 2, 2023 • edited

zdohnal commented Apr 3, 2023

Ankit3002 commented Apr 3, 2023

Ankit3002 commented Apr 3, 2023

zdohnal commented Apr 3, 2023

Ankit3002 commented Apr 3, 2023

zdohnal commented Apr 4, 2023

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 Apr 5, 2023 • edited

zdohnal commented Apr 6, 2023

Ankit3002 commented Apr 8, 2023 • edited

Ankit3002 commented Apr 12, 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

Ankit3002 commented Apr 13, 2023

zdohnal Apr 14, 2023 • edited

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 Apr 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ankit3002 commented Apr 17, 2023 • edited by tillkamppeter

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ankit3002 commented Apr 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Ankit3002 commented Apr 19, 2023

Ankit3002 commented Apr 28, 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

zdohnal commented May 31, 2023

Ankit3002 commented Jun 1, 2023

Ankit3002 commented Jun 2, 2023

Ankit3002 commented Jun 9, 2023

Ankit3002 commented Mar 23, 2023 •

edited

Ankit3002 Mar 27, 2023 •

edited

Ankit3002 commented Mar 29, 2023 •

edited

Ankit3002 commented Apr 2, 2023 •

edited

Ankit3002 commented Apr 5, 2023 •

edited

Ankit3002 commented Apr 8, 2023 •

edited

zdohnal Apr 14, 2023 •

edited

Ankit3002 commented Apr 17, 2023 •

edited by tillkamppeter

Ankit3002 commented Apr 17, 2023 •

edited