Skip to content

valid_field(): Improve readability #1208

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
Merged
merged 9 commits into from
Jun 3, 2025
Merged

valid_field(): Improve readability #1208

merged 9 commits into from
Jun 3, 2025

Conversation

alejandro-colomar
Copy link
Collaborator

@alejandro-colomar alejandro-colomar commented Feb 16, 2025 •
edited
Loading

This adds strisprint() and strchriscntrl().

Revisions:

v2
  • Rewrite the for loops in strisprint() and strchriscntrl().
  • Add comment desoupifying API names.
$ git range-diff shadow/master gh/vf vf 
1:  8561e822 ! 1:  9d2d0f66 lib/string/ctype/strisascii/: strisprint(): Add function
    @@ lib/string/ctype/strisascii/strisprint.c (new)
     
      ## lib/string/ctype/strisascii/strisprint.h (new) ##
     @@
    -+// SPDX-FileCopyrightText: 2024, Alejandro Colomar <alx@kernel.org>
    ++// SPDX-FileCopyrightText: 2024-2025, Alejandro Colomar <alx@kernel.org>
     +// SPDX-License-Identifier: BSD-3-Clause
     +
     +
    @@ lib/string/ctype/strisascii/strisprint.h (new)
     +  if (streq(s, ""))
     +          return false;
     +
    -+  for (unsigned char c = *s; !streq(s, ""); c = *s++) {
    ++  for (; !streq(s, ""); s++) {
    ++          unsigned char  c = *s;
    ++
     +          if (!isprint(c))
     +                  return false;
     +  }
2:  e0404e20 ! 2:  83feb72a lib/string/ctype/strchrisascii/: strchriscntrl(): Add function
    @@ lib/string/ctype/strchrisascii/strchriscntrl.c (new)
     
      ## lib/string/ctype/strchrisascii/strchriscntrl.h (new) ##
     @@
    -+// SPDX-FileCopyrightText: 2024, Alejandro Colomar <alx@kernel.org>
    ++// SPDX-FileCopyrightText: 2024-2025, Alejandro Colomar <alx@kernel.org>
     +// SPDX-License-Identifier: BSD-3-Clause
     +
     +
    @@ lib/string/ctype/strchrisascii/strchriscntrl.h (new)
     +inline bool strchriscntrl(const char *s);
     +
     +
    ++// string character is [:cntrl:]
     +// Return true if any iscntrl(3) character is found in the string.
     +inline bool
     +strchriscntrl(const char *s)
     +{
    -+  for (unsigned char c = *s; !streq(s, ""); c = *s++) {
    ++  for (; !streq(s, ""); s++) {
    ++          unsigned char  c = *s;
    ++
     +          if (iscntrl(c))
     +                  return true;
     +  }
3:  3bf69d20 = 3:  cb7ac416 lib/fields.c: valid_field(): Remove useless check
4:  dd9dcf99 = 4:  2633c872 lib/fields.c: valid_field(): Return early on error
5:  8c894903 = 5:  40bdb529 lib/fields.c: valid_field(): Use strisprint() instead of its pattern
6:  179652c2 = 6:  a47c6de7 lib/fields.c: valid_field(): Use strchriscntrl() instead of its pattern
7:  8032894a = 7:  fdee80fc lib/fields.c: valid_field(): Clarify comments
v2b
  • Rebase
$ git range-diff master..gh/vf shadow/master..vf 
1:  9d2d0f66 = 1:  793e5089 lib/string/ctype/strisascii/: strisprint(): Add function
2:  83feb72a = 2:  3cd6275c lib/string/ctype/strchrisascii/: strchriscntrl(): Add function
3:  cb7ac416 = 3:  ad633f26 lib/fields.c: valid_field(): Remove useless check
4:  2633c872 = 4:  4fe506c8 lib/fields.c: valid_field(): Return early on error
5:  40bdb529 = 5:  2ebb6672 lib/fields.c: valid_field(): Use strisprint() instead of its pattern
6:  a47c6de7 = 6:  316c8984 lib/fields.c: valid_field(): Use strchriscntrl() instead of its pattern
7:  fdee80fc = 7:  819f6ca0 lib/fields.c: valid_field(): Clarify comments
v2c
  • Rebase
$ git rd
1:  793e5089 = 1:  3c9ff60a lib/string/ctype/strisascii/: strisprint(): Add function
2:  3cd6275c = 2:  3935b6cb lib/string/ctype/strchrisascii/: strchriscntrl(): Add function
3:  ad633f26 = 3:  f7483326 lib/fields.c: valid_field(): Remove useless check
4:  4fe506c8 = 4:  e25096ae lib/fields.c: valid_field(): Return early on error
5:  2ebb6672 = 5:  923429ee lib/fields.c: valid_field(): Use strisprint() instead of its pattern
6:  316c8984 = 6:  9f9e7fa8 lib/fields.c: valid_field(): Use strchriscntrl() instead of its pattern
7:  819f6ca0 = 7:  dddb1d8e lib/fields.c: valid_field(): Clarify comments
v2d
  • Rebase
$ git rd
1:  3c9ff60a = 1:  26be5ef8 lib/string/ctype/strisascii/: strisprint(): Add function
2:  3935b6cb = 2:  dc105439 lib/string/ctype/strchrisascii/: strchriscntrl(): Add function
3:  f7483326 = 3:  e33796f0 lib/fields.c: valid_field(): Remove useless check
4:  e25096ae = 4:  0ccd7fc4 lib/fields.c: valid_field(): Return early on error
5:  923429ee = 5:  3857b4cd lib/fields.c: valid_field(): Use strisprint() instead of its pattern
6:  9f9e7fa8 = 6:  6875eb30 lib/fields.c: valid_field(): Use strchriscntrl() instead of its pattern
7:  dddb1d8e = 7:  1a6344c3 lib/fields.c: valid_field(): Clarify comments
v2e
  • Rebase
$ git rd
1:  26be5ef8 = 1:  4feaa04e lib/string/ctype/strisascii/: strisprint(): Add function
2:  dc105439 = 2:  80d6e2ad lib/string/ctype/strchrisascii/: strchriscntrl(): Add function
3:  e33796f0 = 3:  9b924437 lib/fields.c: valid_field(): Remove useless check
4:  0ccd7fc4 = 4:  699b0f7e lib/fields.c: valid_field(): Return early on error
5:  3857b4cd = 5:  3cae4b3a lib/fields.c: valid_field(): Use strisprint() instead of its pattern
6:  6875eb30 = 6:  c6ccbf77 lib/fields.c: valid_field(): Use strchriscntrl() instead of its pattern
7:  1a6344c3 = 7:  09291f44 lib/fields.c: valid_field(): Clarify comments
v2f
  • Rebase
$ git rd
1:  4feaa04e = 1:  e51e4c69 lib/string/ctype/strisascii/: strisprint(): Add function
2:  80d6e2ad = 2:  7789cb48 lib/string/ctype/strchrisascii/: strchriscntrl(): Add function
3:  9b924437 ! 3:  95c966d0 lib/fields.c: valid_field(): Remove useless check
    @@ lib/fields.c: int valid_field (const char *field, const char *illegal)
      
     -  /* For each character of field, search if it appears in the list
     -   * of illegal characters. */
    --  if (illegal && NULL != strpbrk (field, illegal)) {
    -+  if (NULL != strpbrk(field, illegal))
    +-  if (illegal && strpbrk(field, illegal)) {
    ++  if (strpbrk(field, illegal))
                return -1;
     -  }
      
4:  699b0f7e = 4:  76905a72 lib/fields.c: valid_field(): Return early on error
5:  3cae4b3a = 5:  88f683d3 lib/fields.c: valid_field(): Use strisprint() instead of its pattern
6:  c6ccbf77 ! 6:  a92b7c4b lib/fields.c: valid_field(): Use strchriscntrl() instead of its pattern
    @@ lib/fields.c
                return -1;
        }
      
    -   if (NULL != strpbrk(field, illegal))
    +   if (strpbrk(field, illegal))
                return -1;
     -
     -  /* Search if there are non-printable or control characters */
7:  09291f44 ! 7:  cedcbb3a lib/fields.c: valid_field(): Clarify comments
    @@ lib/fields.c
                return -1;
     -  }
      
    -   if (NULL != strpbrk(field, illegal))
    +   if (strpbrk(field, illegal))
                return -1;
     @@ lib/fields.c: int valid_field (const char *field, const char *illegal)
        if (streq(field, ""))
v2g
  • Rebase
$ git rd
1:  e51e4c69 ! 1:  1e5df9ee lib/string/ctype/strisascii/: strisprint(): Add function
    @@ lib/Makefile.am: libshadow_la_SOURCES = \
        string/ctype/strisascii/strisdigit.h \
     +  string/ctype/strisascii/strisprint.c \
     +  string/ctype/strisascii/strisprint.h \
    +   string/ctype/strtoascii/strtolower.c \
    +   string/ctype/strtoascii/strtolower.h \
        string/memset/memzero.c \
    -   string/memset/memzero.h \
    -   string/sprintf/snprintf.c \
     
      ## lib/string/ctype/strisascii/strisprint.c (new) ##
     @@
2:  7789cb48 ! 2:  0e235f86 lib/string/ctype/strchrisascii/: strchriscntrl(): Add function
    @@ Commit message
     
      ## lib/Makefile.am ##
     @@ lib/Makefile.am: libshadow_la_SOURCES = \
    -   string/ctype/strisascii/strisdigit.h \
    -   string/ctype/strisascii/strisprint.c \
    -   string/ctype/strisascii/strisprint.h \
    +   spawn.c \
    +   sssd.c \
    +   sssd.h \
     +  string/ctype/strchrisascii/strchriscntrl.c \
     +  string/ctype/strchrisascii/strchriscntrl.h \
    -   string/memset/memzero.c \
    -   string/memset/memzero.h \
    -   string/sprintf/snprintf.c \
    +   string/ctype/strisascii/strisdigit.c \
    +   string/ctype/strisascii/strisdigit.h \
    +   string/ctype/strisascii/strisprint.c \
     
      ## lib/string/ctype/strchrisascii/strchriscntrl.c (new) ##
     @@
3:  95c966d0 = 3:  09b8078b lib/fields.c: valid_field(): Remove useless check
4:  76905a72 = 4:  04c04996 lib/fields.c: valid_field(): Return early on error
5:  88f683d3 = 5:  8e8c0d1d lib/fields.c: valid_field(): Use strisprint() instead of its pattern
6:  a92b7c4b = 6:  a1a8d8dd lib/fields.c: valid_field(): Use strchriscntrl() instead of its pattern
7:  cedcbb3a = 7:  1c932329 lib/fields.c: valid_field(): Clarify comments
v2h
  • Rebase
$ git rd
1:  1e5df9ee = 1:  584e35d3 lib/string/ctype/strisascii/: strisprint(): Add function
2:  0e235f86 = 2:  6b215fb3 lib/string/ctype/strchrisascii/: strchriscntrl(): Add function
3:  09b8078b = 3:  f257fbd8 lib/fields.c: valid_field(): Remove useless check
4:  04c04996 = 4:  9adee2e0 lib/fields.c: valid_field(): Return early on error
5:  8e8c0d1d = 5:  bddb8418 lib/fields.c: valid_field(): Use strisprint() instead of its pattern
6:  a1a8d8dd = 6:  7b1a9c8a lib/fields.c: valid_field(): Use strchriscntrl() instead of its pattern
7:  1c932329 = 7:  44c20f7d lib/fields.c: valid_field(): Clarify comments
v3
  • Add guarantees that $2 is a string literal. [@hallyn ]
$ git rd
 1:  584e35d3 =  1:  584e35d3 lib/string/ctype/strisascii/: strisprint(): Add function
 2:  6b215fb3 =  2:  6b215fb3 lib/string/ctype/strchrisascii/: strchriscntrl(): Add function
 -:  -------- >  3:  8d92cee2 lib/, src/: Move prototypes of "lib/fields.c" to "lib/fields.h"
 -:  -------- >  4:  8786de13 lib/fields.*: valid_field: Make sure that $2 is a string literal
 3:  f257fbd8 !  5:  2924cf8c lib/fields.c: valid_field(): Remove useless check
    @@ Commit message
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## lib/fields.c ##
    -@@ lib/fields.c: int valid_field (const char *field, const char *illegal)
    +@@ lib/fields.c: valid_field_(const char *field, const char *illegal)
                return -1;
        }
      
 4:  9adee2e0 !  6:  8caca9ca lib/fields.c: valid_field(): Return early on error
    @@ Commit message
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## lib/fields.c ##
    -@@ lib/fields.c: int valid_field (const char *field, const char *illegal)
    +@@ lib/fields.c: valid_field_(const char *field, const char *illegal)
        /* Search if there are non-printable or control characters */
        for (cp = field; !streq(cp, ""); cp++) {
                unsigned char c = *cp;
 5:  bddb8418 !  7:  57031a30 lib/fields.c: valid_field(): Use strisprint() instead of its pattern
    @@ lib/fields.c
      #include "string/strcmp/streq.h"
      #include "string/strspn/stpspn.h"
      #include "string/strspn/stprspn.h"
    -@@
    - int valid_field (const char *field, const char *illegal)
    +@@ lib/fields.c: int
    + valid_field_(const char *field, const char *illegal)
      {
        const char *cp;
     -  int err = 0;
      
        if (NULL == field) {
                return -1;
    -@@ lib/fields.c: int valid_field (const char *field, const char *illegal)
    +@@ lib/fields.c: valid_field_(const char *field, const char *illegal)
                unsigned char c = *cp;
                if (iscntrl(c))
                        return -1;
 6:  7b1a9c8a !  8:  fc43aaba lib/fields.c: valid_field(): Use strchriscntrl() instead of its pattern
    @@ lib/fields.c
      #include "string/strspn/stpspn.h"
      #include "string/strspn/stprspn.h"
     @@
    -  */
    - int valid_field (const char *field, const char *illegal)
    + int
    + valid_field_(const char *field, const char *illegal)
      {
     -  const char *cp;
     -
 7:  44c20f7d !  9:  edcad794 lib/fields.c: valid_field(): Clarify comments
    @@ lib/fields.c
     + *        1       Non-ASCII characters are present.
     + *        0       All chatacters are legal and ASCII.
       */
    --int valid_field (const char *field, const char *illegal)
    -+int
    -+valid_field(const char *field, const char *illegal)
    + int
    + valid_field_(const char *field, const char *illegal)
      {
     -  if (NULL == field) {
     +  if (NULL == field)
    @@ lib/fields.c
      
        if (strpbrk(field, illegal))
                return -1;
    -@@ lib/fields.c: int valid_field (const char *field, const char *illegal)
    +@@ lib/fields.c: valid_field_(const char *field, const char *illegal)
        if (streq(field, ""))
                return 0;
v3b
  • Fix sorting of includes.
$ git rd
 1:  584e35d3 =  1:  584e35d3 lib/string/ctype/strisascii/: strisprint(): Add function
 2:  6b215fb3 =  2:  6b215fb3 lib/string/ctype/strchrisascii/: strchriscntrl(): Add function
 3:  8d92cee2 !  3:  c8011b63 lib/, src/: Move prototypes of "lib/fields.c" to "lib/fields.h"
    @@ lib/pwio.c
      #include <pwd.h>
      #include <stdio.h>
     +
    -+#include "prototypes.h"
      #include "commonio.h"
     +#include "defines.h"
     +#include "fields.h"
    ++#include "prototypes.h"
      #include "pwio.h"
      
      static /*@null@*/ /*@only@*/void *passwd_dup (const void *ent)
    @@ lib/shadowio.c
      #include <shadow.h>
      #include <stdio.h>
     +
    -+#include "prototypes.h"
      #include "commonio.h"
     +#include "defines.h"
     +#include "fields.h"
      #include "getdef.h"
    ++#include "prototypes.h"
      #include "shadowio.h"
     +
      #ifdef WITH_TCB
 4:  8786de13 =  4:  c21f9ce3 lib/fields.*: valid_field: Make sure that $2 is a string literal
 5:  2924cf8c =  5:  804c0f3b lib/fields.c: valid_field(): Remove useless check
 6:  8caca9ca =  6:  a486b062 lib/fields.c: valid_field(): Return early on error
 7:  57031a30 =  7:  98346110 lib/fields.c: valid_field(): Use strisprint() instead of its pattern
 8:  fc43aaba =  8:  548f2c0c lib/fields.c: valid_field(): Use strchriscntrl() instead of its pattern
 9:  edcad794 =  9:  9789471c lib/fields.c: valid_field(): Clarify comments
v3c
  • Rebase
$ git rd 
 1:  584e35d3 =  1:  f0de212a lib/string/ctype/strisascii/: strisprint(): Add function
 2:  6b215fb3 =  2:  dbe1f1e7 lib/string/ctype/strchrisascii/: strchriscntrl(): Add function
 3:  c8011b63 =  3:  e53ce352 lib/, src/: Move prototypes of "lib/fields.c" to "lib/fields.h"
 4:  c21f9ce3 =  4:  53fdda71 lib/fields.*: valid_field: Make sure that $2 is a string literal
 5:  804c0f3b =  5:  0629ddc2 lib/fields.c: valid_field(): Remove useless check
 6:  a486b062 =  6:  0fd492ca lib/fields.c: valid_field(): Return early on error
 7:  98346110 =  7:  3bac758b lib/fields.c: valid_field(): Use strisprint() instead of its pattern
 8:  548f2c0c =  8:  2f04db4a lib/fields.c: valid_field(): Use strchriscntrl() instead of its pattern
 9:  9789471c =  9:  99ffa0ee lib/fields.c: valid_field(): Clarify comments

@alejandro-colomar alejandro-colomar mentioned this pull request Feb 17, 2025
Open
@alejandro-colomar alejandro-colomar added the Simpler A good issue for a new beginner label Feb 18, 2025
@alejandro-colomar alejandro-colomar mentioned this pull request May 4, 2025
Closed
@alejandro-colomar alejandro-colomar force-pushed the vf branch 4 times, most recently from 1c93232 to 44c20f7 Compare June 2, 2025 09:25
hallyn
hallyn reviewed Jun 3, 2025
View reviewed changes
@alejandro-colomar alejandro-colomar force-pushed the vf branch 2 times, most recently from edcad79 to 9789471 Compare June 3, 2025 07:20
@alejandro-colomar
lib/string/ctype/strisascii/: strisprint(): Add function
f0de212 
Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
lib/string/ctype/strchrisascii/: strchriscntrl(): Add function
dbe1f1e 
Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
lib/, src/: Move prototypes of "lib/fields.c" to "lib/fields.h"
e53ce35 
Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
lib/fields.*: valid_field: Make sure that $2 is a string literal
53fdda7 
and thus, nonnull.

Suggested-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
lib/fields.c: valid_field(): Remove useless check
0629ddc 
We only call this function with a string literal, and it makes little
sense to pass something else.  Let's simplify.

Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
lib/fields.c: valid_field(): Return early on error
0fd492c 
Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
lib/fields.c: valid_field(): Use strisprint() instead of its pattern
3bac758 
Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
lib/fields.c: valid_field(): Use strchriscntrl() instead of its pattern
2f04db4 
Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
lib/fields.c: valid_field(): Clarify comments
99ffa0e 
And apply minor style changes.

Signed-off-by: Alejandro Colomar <alx@kernel.org>
@hallyn hallyn merged commit a5fc731 into shadow-maint:master Jun 3, 2025
10 checks passed
@alejandro-colomar alejandro-colomar deleted the vf branch June 3, 2025 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hallyn hallyn hallyn approved these changes

@ikerexxe ikerexxe Awaiting requested review from ikerexxe

Assignees
No one assigned
Labels
Simpler A good issue for a new beginner
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alejandro-colomar @hallyn