-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

[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. I'm seeing a few things I could get rid of.