-Werror=unused-but-set-variable

[alejandro-colomar]

Another one.

---

Revisions:

v1b

• Rebase

$ git rd 
1:  7983b7a52 = 1:  12c878515 autogen.sh: Enable -Werror=unused-but-set-variable diagnostic
2:  e12a73742 = 2:  a691a2cf0 src/login.c: main(): Wrap uses of 'subroot' in ifndef USE_PAM
3:  ddd475daa = 3:  deb5d8de9 src/useradd.c: Wrap uses of 'process_selinux' in #ifdef WITH_SELINUX

v1c

• Rebase

$ git rd 
1:  12c878515 = 1:  aa50a095b autogen.sh: Enable -Werror=unused-but-set-variable diagnostic
2:  a691a2cf0 = 2:  80d581b46 src/login.c: main(): Wrap uses of 'subroot' in ifndef USE_PAM
3:  deb5d8de9 = 3:  e4652d30a src/useradd.c: Wrap uses of 'process_selinux' in #ifdef WITH_SELINUX

v1d

• Rebase

$ git rd 
1:  aa50a095bc23 = 1:  cb7ebfc61fa7 autogen.sh: Enable -Werror=unused-but-set-variable diagnostic
2:  80d581b46e94 = 2:  4e1a23e08061 src/login.c: main(): Wrap uses of 'subroot' in ifndef USE_PAM
3:  e4652d30abdb = 3:  13253d9ffd2d src/useradd.c: Wrap uses of 'process_selinux' in #ifdef WITH_SELINUX

v1e

• Rebase

$ git rd 
1:  cb7ebfc6 = 1:  e38aa079 autogen.sh: Enable -Werror=unused-but-set-variable diagnostic
2:  4e1a23e0 = 2:  6b459eba src/login.c: main(): Wrap uses of 'subroot' in ifndef USE_PAM
3:  13253d9f = 3:  f752f33b src/useradd.c: Wrap uses of 'process_selinux' in #ifdef WITH_SELINUX

v1f

• Rebase

$ git rd 
1:  e38aa079 = 1:  11f511af autogen.sh: Enable -Werror=unused-but-set-variable diagnostic
2:  6b459eba = 2:  ef78a9cb src/login.c: main(): Wrap uses of 'subroot' in ifndef USE_PAM
3:  f752f33b = 3:  f010ba35 src/useradd.c: Wrap uses of 'process_selinux' in #ifdef WITH_SELINUX

v1g

• Rebase

$ git rd 
1:  11f511af = 1:  bc7468bd autogen.sh: Enable -Werror=unused-but-set-variable diagnostic
2:  ef78a9cb = 2:  0353f01f src/login.c: main(): Wrap uses of 'subroot' in ifndef USE_PAM
3:  f010ba35 = 3:  997b468f src/useradd.c: Wrap uses of 'process_selinux' in #ifdef WITH_SELINUX

v1h

• Rebase

$ git rd 
1:  bc7468bd394d ! 1:  df25f9213fac autogen.sh: Enable -Werror=unused-but-set-variable diagnostic
    @@ Commit message
     
      ## autogen.sh ##
     @@ autogen.sh: CFLAGS="$CFLAGS -Werror=int-conversion"
    - CFLAGS="$CFLAGS -Werror=sign-compare"
    + CFLAGS="$CFLAGS -Werror=overflow"
      CFLAGS="$CFLAGS -Werror=sizeof-pointer-div"
      CFLAGS="$CFLAGS -Werror=unused-but-set-parameter"
     +CFLAGS="$CFLAGS -Werror=unused-but-set-variable"
2:  0353f01f2318 = 2:  45a1a8b4a883 src/login.c: main(): Wrap uses of 'subroot' in ifndef USE_PAM
3:  997b468f5ab8 = 3:  fcd76be9243a src/useradd.c: Wrap uses of 'process_selinux' in #ifdef WITH_SELINUX

[hallyn]

Patch 2 could avoid adding any ifdefs by doing something like:

diff --git a/src/login.c b/src/login.c
index 6798bb1d..680653d1 100644
--- a/src/login.c
+++ b/src/login.c
@@ -64,6 +64,9 @@ static pam_handle_t *pamh = NULL;
 #define PAM_END { retcode = pam_close_session(pamh,0); \
                (void) pam_end(pamh,retcode); }

+#define mark_subroot() { subroot = true; }
+#else                          /* USE_PAM */
+#define mark_subroot() {}
 #endif                         /* USE_PAM */


@@ -448,7 +451,6 @@ static /*@observer@*/const char *get_failent_user (/*@returned@*/const char *use
 int main (int argc, char **argv)
 {
        int            err;
-       bool           subroot = false;
        char           **envp = environ;
        char           *host = NULL;
        char           tty[BUFSIZ];
@@ -464,6 +466,7 @@ int main (int argc, char **argv)
        struct passwd  *pwd = NULL;

 #if defined(USE_PAM)
+       bool           subroot = false;
        int            retcode;
        char           *pam_user = NULL;
        pid_t          child;
@@ -1018,7 +1021,7 @@ int main (int argc, char **argv)
        if (strprefix(pwd->pw_shell, "*")) {  /* subsystem root */
                pwd->pw_shell++;        /* skip the '*' */
                subsystem (pwd);        /* figure out what to execute */
-               subroot = true; /* say I was here again */
+               mark_subroot(); /* say I was here again */
                endpwent ();    /* close all of the file which were */
                endgrent ();    /* open in the original rooted file */
                endspent ();    /* system. they will be re-opened */

Similarly in patch 3, it may be cleaner to define a noop set_selinux_file_context() in src/selinux.c in the !WITH_SELINUX case, and then just drop the #ifdef WITH_SELINUX from src/useradd.c:2218 and 2356.

[alejandro-colomar]

Patch 2 could avoid adding any ifdefs by doing something like:

diff --git a/src/login.c b/src/login.c
index 6798bb1d..680653d1 100644
--- a/src/login.c
+++ b/src/login.c
@@ -64,6 +64,9 @@ static pam_handle_t *pamh = NULL;
 #define PAM_END { retcode = pam_close_session(pamh,0); \
                (void) pam_end(pamh,retcode); }

+#define mark_subroot() { subroot = true; }

I find macros that affect undeclared objects to be worse than ifdefs.

And after all, you're still using an ifdef, just an existing one.

+#else /* USE_PAM /
+#define mark_subroot() {}
#endif / USE_PAM */

@@ -448,7 +451,6 @@ static /@observer@/const char get_failent_user (/@returned@*/const char *use
int main (int argc, char **argv)
{
int err;

•   bool           subroot = false;
    char           **envp = environ;
    char           *host = NULL;
    char           tty[BUFSIZ];
  

@@ -464,6 +466,7 @@ int main (int argc, char **argv)
struct passwd *pwd = NULL;

#if defined(USE_PAM)

•   bool           subroot = false;
    int            retcode;
    char           *pam_user = NULL;
    pid_t          child;
  

@@ -1018,7 +1021,7 @@ int main (int argc, char **argv)
if (strprefix(pwd->pw_shell, "")) { / subsystem root /
pwd->pw_shell++; / skip the '*' /
subsystem (pwd); / figure out what to execute */

•           subroot = true; /* say I was here again */
  

•           mark_subroot(); /* say I was here again */
            endpwent ();    /* close all of the file which were */
            endgrent ();    /* open in the original rooted file */
            endspent ();    /* system. they will be re-opened */
  

Similarly in patch 3, it may be cleaner to define a noop set_selinux_file_context() in src/selinux.c in the !WITH_SELINUX case, and then just drop the #ifdef WITH_SELINUX from src/useradd.c:2218 and 2356.

[hallyn]

On Sat, Dec 06, 2025 at 03:41:16AM -0800, Alejandro Colomar wrote: alejandro-colomar left a comment (shadow-maint/shadow#1402) > Patch 2 could avoid adding any ifdefs by doing something like: > > ``` > diff --git a/src/login.c b/src/login.c > index 6798bb1d..680653d1 100644 > --- a/src/login.c > +++ b/src/login.c > @@ -64,6 +64,9 @@ static pam_handle_t *pamh = NULL; > #define PAM_END { retcode = pam_close_session(pamh,0); \ > (void) pam_end(pamh,retcode); } > > +#define mark_subroot() { subroot = true; } I find macros that affect undeclared objects to be worse than ifdefs. And after all, you're still using an ifdef, just an existing one.

If there were one ifdef, that might not sound so bad. But look at that file! I'd like to rewrite the whole thing so a person can actually follow the flow.

[alejandro-colomar]

On Sat, Dec 06, 2025 at 03:41:16AM -0800, Alejandro Colomar wrote: alejandro-colomar left a comment (shadow-maint/shadow#1402) > Patch 2 could avoid adding any ifdefs by doing something like: > > ``` > diff --git a/src/login.c b/src/login.c > index 6798bb1d..680653d1 100644 > --- a/src/login.c > +++ b/src/login.c > @@ -64,6 +64,9 @@ static pam_handle_t *pamh = NULL; > #define PAM_END { retcode = pam_close_session(pamh,0); \ > (void) pam_end(pamh,retcode); } > > +#define mark_subroot() { subroot = true; } I find macros that affect undeclared objects to be worse than ifdefs. And after all, you're still using an ifdef, just an existing one.
If there were one ifdef, that might not sound so bad. But look at that file! I'd like to rewrite the whole thing so a person can actually follow the flow.

I'll have a look at simplifying that file further (in a separate PR). I'm seeing a few things I could get rid of.