[PATCH] shadow: Implement putspent

[PATCH] shadow: Implement putspent

[Michael Forney]

---
When I brought up implementing the shadow.h functions on IRC, several concerns
were raised. However, it turns out that the shadow package only requires
putspent. I didn't understand the concerns on IRC, so if there is an issue with
the implementation of this function, please let me know and hopefully I'll be
able to address it.

 src/passwd/fgetspent.c |  5 -----
 src/passwd/putspent.c  | 30 ++++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 5 deletions(-)
 create mode 100644 src/passwd/putspent.c

diff --git a/src/passwd/fgetspent.c b/src/passwd/fgetspent.c
index a9a3c97..3dda784 100644
--- a/src/passwd/fgetspent.c
+++ b/src/passwd/fgetspent.c
@@ -4,8 +4,3 @@ struct spwd *fgetspent(FILE *f)
 {
 	return 0;
 }
-
-int putspent(const struct spwd *sp, FILE *f)
-{
-	return -1;
-}
diff --git a/src/passwd/putspent.c b/src/passwd/putspent.c
new file mode 100644
index 0000000..bb0a410
--- /dev/null
+++ b/src/passwd/putspent.c
@@ -0,0 +1,30 @@
+#include <shadow.h>
+#include <stdio.h>
+
+int putspent(const struct spwd *sp, FILE *f)
+{
+	flockfile(f);
+	if (sp->sp_namp && fputs(sp->sp_namp, f) == EOF) goto fail;
+	if (fputc(':', f) == EOF) goto fail;
+	if (sp->sp_pwdp && fputs(sp->sp_pwdp, f) == EOF) goto fail;
+	if (fputc(':', f) == EOF) goto fail;
+	if (sp->sp_lstchg != -1 && fprintf(f, "%d", sp->sp_lstchg) < 0) goto fail;
+	if (fputc(':', f) == EOF) goto fail;
+	if (sp->sp_min != -1 && fprintf(f, "%d", sp->sp_min) < 0) goto fail;
+	if (fputc(':', f) == EOF) goto fail;
+	if (sp->sp_max != -1 && fprintf(f, "%d", sp->sp_max) < 0) goto fail;
+	if (fputc(':', f) == EOF) goto fail;
+	if (sp->sp_warn != -1 && fprintf(f, "%d", sp->sp_warn) < 0) goto fail;
+	if (fputc(':', f) == EOF) goto fail;
+	if (sp->sp_inact != -1 && fprintf(f, "%d", sp->sp_inact) < 0) goto fail;
+	if (fputc(':', f) == EOF) goto fail;
+	if (sp->sp_expire != -1 && fprintf(f, "%d", sp->sp_expire) < 0) goto fail;
+	if (fputc(':', f) == EOF) goto fail;
+	if (sp->sp_flag != -1 && fprintf(f, "%d", sp->sp_flag) < 0) goto fail;
+	if (fputc('\n', f) == EOF) goto fail;
+	funlockfile(f);
+	return 0;
+fail:
+	funlockfile(f);
+	return -1;
+}
-- 
1.8.4.2

Re: [PATCH] shadow: Implement putspent

[Rich Felker]

On Mon, Nov 04, 2013 at 11:47:40PM -0800, Michael Forney wrote:
> ---
> When I brought up implementing the shadow.h functions on IRC, several concerns
> were raised. However, it turns out that the shadow package only requires
> putspent. I didn't understand the concerns on IRC, so if there is an issue with
> the implementation of this function, please let me know and hopefully I'll be
> able to address it.
> 
>  src/passwd/fgetspent.c |  5 -----
>  src/passwd/putspent.c  | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 30 insertions(+), 5 deletions(-)
>  create mode 100644 src/passwd/putspent.c
> 
> diff --git a/src/passwd/fgetspent.c b/src/passwd/fgetspent.c
> index a9a3c97..3dda784 100644
> --- a/src/passwd/fgetspent.c
> +++ b/src/passwd/fgetspent.c
> @@ -4,8 +4,3 @@ struct spwd *fgetspent(FILE *f)
>  {
>  	return 0;
>  }
> -
> -int putspent(const struct spwd *sp, FILE *f)
> -{
> -	return -1;
> -}
> diff --git a/src/passwd/putspent.c b/src/passwd/putspent.c
> new file mode 100644
> index 0000000..bb0a410
> --- /dev/null
> +++ b/src/passwd/putspent.c
> @@ -0,0 +1,30 @@
> +#include <shadow.h>
> +#include <stdio.h>
> +
> +int putspent(const struct spwd *sp, FILE *f)
> +{
> +	flockfile(f);
> +	if (sp->sp_namp && fputs(sp->sp_namp, f) == EOF) goto fail;
> +	if (fputc(':', f) == EOF) goto fail;
> +	if (sp->sp_pwdp && fputs(sp->sp_pwdp, f) == EOF) goto fail;
> +	if (fputc(':', f) == EOF) goto fail;
> +	if (sp->sp_lstchg != -1 && fprintf(f, "%d", sp->sp_lstchg) < 0) goto fail;
> +	if (fputc(':', f) == EOF) goto fail;

Is there a reason for all this logic instead of a single call to
fprintf with a proper format string? Writing the value -1 as an empty
slot can be handled with a simple macro that expands to 0,0 for an
argument of -1 and -1,val for other values, along with a %.*d format.

Rich

[PATCH v2] shadow: Implement putspent

[Michael Forney]

---
Thanks Rich,

I didn't know about negative and zero precision specifiers. This turned out
much cleaner.

I've also changed the fields to use %ld because they are longs, and changed the
last field to %lu because it is unsigned (though unused).

 src/passwd/fgetspent.c |  5 -----
 src/passwd/putspent.c  | 13 +++++++++++++
 2 files changed, 13 insertions(+), 5 deletions(-)
 create mode 100644 src/passwd/putspent.c

diff --git a/src/passwd/fgetspent.c b/src/passwd/fgetspent.c
index a9a3c97..3dda784 100644
--- a/src/passwd/fgetspent.c
+++ b/src/passwd/fgetspent.c
@@ -4,8 +4,3 @@ struct spwd *fgetspent(FILE *f)
 {
 	return 0;
 }
-
-int putspent(const struct spwd *sp, FILE *f)
-{
-	return -1;
-}
diff --git a/src/passwd/putspent.c b/src/passwd/putspent.c
new file mode 100644
index 0000000..fbf4278
--- /dev/null
+++ b/src/passwd/putspent.c
@@ -0,0 +1,13 @@
+#include <shadow.h>
+#include <stdio.h>
+
+#define NUM(n) (n == -1 ? 0 : -1), (n == -1 ? 0 : n)
+#define STR(s) (s ? s : "")
+
+int putspent(const struct spwd *sp, FILE *f)
+{
+	return fprintf(f, "%s:%s:%.*ld:%.*ld:%.*ld:%.*ld:%.*ld:%.*ld:%.*lu\n",
+		STR(sp->sp_namp), STR(sp->sp_pwdp), NUM(sp->sp_lstchg),
+		NUM(sp->sp_min), NUM(sp->sp_max), NUM(sp->sp_warn),
+		NUM(sp->sp_inact), NUM(sp->sp_expire), NUM(sp->sp_flag)) < 0 ? -1 : 0;
+}
-- 
1.8.4.2

Re: [PATCH v2] shadow: Implement putspent

[Rich Felker]

On Tue, Nov 05, 2013 at 02:30:00PM -0800, Michael Forney wrote:
> ---
> Thanks Rich,
> 
> I didn't know about negative and zero precision specifiers. This turned out
> much cleaner.
> 
> I've also changed the fields to use %ld because they are longs, and changed the
> last field to %lu because it is unsigned (though unused).

Thanks.

>  src/passwd/fgetspent.c |  5 -----
>  src/passwd/putspent.c  | 13 +++++++++++++
>  2 files changed, 13 insertions(+), 5 deletions(-)
>  create mode 100644 src/passwd/putspent.c
> 
> diff --git a/src/passwd/fgetspent.c b/src/passwd/fgetspent.c
> index a9a3c97..3dda784 100644
> --- a/src/passwd/fgetspent.c
> +++ b/src/passwd/fgetspent.c
> @@ -4,8 +4,3 @@ struct spwd *fgetspent(FILE *f)
>  {
>  	return 0;
>  }
> -
> -int putspent(const struct spwd *sp, FILE *f)
> -{
> -	return -1;
> -}
> diff --git a/src/passwd/putspent.c b/src/passwd/putspent.c
> new file mode 100644
> index 0000000..fbf4278
> --- /dev/null
> +++ b/src/passwd/putspent.c
> @@ -0,0 +1,13 @@
> +#include <shadow.h>
> +#include <stdio.h>
> +
> +#define NUM(n) (n == -1 ? 0 : -1), (n == -1 ? 0 : n)
> +#define STR(s) (s ? s : "")

While it doesn't really matter in this file, in general, macro
arguments should be properly parenthesized, as in:

+#define NUM(n) ((n) == -1 ? 0 : -1), ((n) == -1 ? 0 : (n))
+#define STR(s) ((s) ? (s) : "")

Otherwise this patch is probably ok. If nobody else has comments on it
I can commit it soon. I'll look at the others again too but in general
I didn't see anything objectionable.

Rich

Re: [PATCH v2] shadow: Implement putspent

[Jens Gustedt]

[-- Attachment #1: Type: text/plain, Size: 1156 bytes --]

Hi Rich,

Am Dienstag, den 05.11.2013, 18:31 -0500 schrieb Rich Felker:
> While it doesn't really matter in this file, in general, macro
> arguments should be properly parenthesized, as in:

> +#define NUM(n) ((n) == -1 ? 0 : -1), ((n) == -1 ? 0 : (n))

for such a macro that is replacing two function arguments, I'd go for
a much more descriptive name, something like NUM2ARGS

> +#define STR(s) ((s) ? (s) : "")

in the context of the actual function that would certainly overkill,
but generally it is not a good idea to mix user strings and string
literals without consting them. So in a general context I'd go for
something like

#define STR(S) ((char const*)((S) ? (S) : ""))

or even

#define STR(S) ((S) ? (char const*){ (S) } : "")

to have a better type check for the argument

Jens



-- 
:: INRIA Nancy Grand Est :: http://www.loria.fr/~gustedt/   ::
:: AlGorille ::::::::::::::: office Nancy : +33 383593090   ::
:: ICube :::::::::::::: office Strasbourg : +33 368854536   ::
:: ::::::::::::::::::::::::::: gsm France : +33 651400183   ::
:: :::::::::::::::::::: gsm international : +49 15737185122 ::




[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2] shadow: Implement putspent

[Rich Felker]

On Wed, Nov 06, 2013 at 02:20:54PM +0100, Jens Gustedt wrote:
> Hi Rich,
> 
> Am Dienstag, den 05.11.2013, 18:31 -0500 schrieb Rich Felker:
> > While it doesn't really matter in this file, in general, macro
> > arguments should be properly parenthesized, as in:
> 
> > +#define NUM(n) ((n) == -1 ? 0 : -1), ((n) == -1 ? 0 : (n))
> 
> for such a macro that is replacing two function arguments, I'd go for
> a much more descriptive name, something like NUM2ARGS

I agree with you in principle, but I don't think it really matters
here. The file is small enough that you see both the definition and
usage together. A better improvement might be adding a comment that
the macro expands to two arguments to fprintf, a precision and a
value, but we're getting well into bikeshed territory here. :-)

> > +#define STR(s) ((s) ? (s) : "")
> 
> in the context of the actual function that would certainly overkill,
> but generally it is not a good idea to mix user strings and string
> literals without consting them. So in a general context I'd go for
> something like
> 
> #define STR(S) ((char const*)((S) ? (S) : ""))
> 
> or even
> 
> #define STR(S) ((S) ? (char const*){ (S) } : "")
> 
> to have a better type check for the argument

I disagree with this change. The type of string literals is char *,
not const char *, so it's not a type consistency issue. Even if it
were, the ?: operator handles the type correctly anyway. My view is
that casts are a code smell, and no-op casts are harmful in that they
obfuscate the correctness of types (since the reader has to question
whether the cast is hiding a type mismatch).

Rich

Re: [PATCH v2] shadow: Implement putspent

[Jens Gustedt]

[-- Attachment #1: Type: text/plain, Size: 2160 bytes --]

Am Mittwoch, den 06.11.2013, 11:36 -0500 schrieb Rich Felker:
> On Wed, Nov 06, 2013 at 02:20:54PM +0100, Jens Gustedt wrote:
> > in the context of the actual function that would certainly overkill,
> > but generally it is not a good idea to mix user strings and string
> > literals without consting them. So in a general context I'd go for
> > something like
> > 
> > #define STR(S) ((char const*)((S) ? (S) : ""))
> > 
> > or even
> > 
> > #define STR(S) ((S) ? (char const*){ (S) } : "")
> > 
> > to have a better type check for the argument
> 
> I disagree with this change. The type of string literals is char *,
> not const char *, so it's not a type consistency issue. Even if it
> were, the ?: operator handles the type correctly anyway. My view is
> that casts are a code smell, and no-op casts are harmful in that they
> obfuscate the correctness of types (since the reader has to question
> whether the cast is hiding a type mismatch).

The second variant isn't a casţ but a conversion and it just checks if
S is assignment compatible with `char const*`. A completely type safe
variant then would be

#define STR(S) ((S) ? (char const*){ (S) } : (char const[]){ 0 })

which wouldn't imply any conversion. (And which a compiler *may*
realize by using a static object for the empty string.)

Just to be clear for the reason I mentioned it: the original macro has
the danger that it is used in places where it is passed as char* to a
function which then modifies the string.

updatefunction(STR(toto));

The user who calls first the macro and then the function with his own
variable thinks that he is passing a modifiable string "toto" to the
function. The bug only appears when by accident someday "toto" is 0,
and a then a string literal is passed into the function.

Jens


-- 
:: INRIA Nancy Grand Est :: http://www.loria.fr/~gustedt/   ::
:: AlGorille ::::::::::::::: office Nancy : +33 383593090   ::
:: ICube :::::::::::::: office Strasbourg : +33 368854536   ::
:: ::::::::::::::::::::::::::: gsm France : +33 651400183   ::
:: :::::::::::::::::::: gsm international : +49 15737185122 ::



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2] shadow: Implement putspent

[Szabolcs Nagy]

* Jens Gustedt <jens.gustedt@inria.fr> [2013-11-07 23:35:01 +0100]:
> The second variant isn't a cas?? but a conversion and it just checks if
> S is assignment compatible with `char const*`. A completely type safe
> variant then would be
> 
> #define STR(S) ((S) ? (char const*){ (S) } : (char const[]){ 0 })
> 
> which wouldn't imply any conversion. (And which a compiler *may*
> realize by using a static object for the empty string.)

yes but we are talking about a translation unit that
implements a single function with a single fprintf call

fprintf does not modify its arguments so any hackery
around the const qualifier is just clutter

[PATCH v3] shadow: Implement putspent

[Michael Forney]

---
 src/passwd/fgetspent.c |  5 -----
 src/passwd/putspent.c  | 13 +++++++++++++
 2 files changed, 13 insertions(+), 5 deletions(-)
 create mode 100644 src/passwd/putspent.c

diff --git a/src/passwd/fgetspent.c b/src/passwd/fgetspent.c
index a9a3c97..3dda784 100644
--- a/src/passwd/fgetspent.c
+++ b/src/passwd/fgetspent.c
@@ -4,8 +4,3 @@ struct spwd *fgetspent(FILE *f)
 {
 	return 0;
 }
-
-int putspent(const struct spwd *sp, FILE *f)
-{
-	return -1;
-}
diff --git a/src/passwd/putspent.c b/src/passwd/putspent.c
new file mode 100644
index 0000000..55c41bb
--- /dev/null
+++ b/src/passwd/putspent.c
@@ -0,0 +1,13 @@
+#include <shadow.h>
+#include <stdio.h>
+
+#define NUM(n) ((n) == -1 ? 0 : -1), ((n) == -1 ? 0 : (n))
+#define STR(s) ((s) ? (s) : "")
+
+int putspent(const struct spwd *sp, FILE *f)
+{
+	return fprintf(f, "%s:%s:%.*ld:%.*ld:%.*ld:%.*ld:%.*ld:%.*ld:%.*lu\n",
+		STR(sp->sp_namp), STR(sp->sp_pwdp), NUM(sp->sp_lstchg),
+		NUM(sp->sp_min), NUM(sp->sp_max), NUM(sp->sp_warn),
+		NUM(sp->sp_inact), NUM(sp->sp_expire), NUM(sp->sp_flag)) < 0 ? -1 : 0;
+}
-- 
1.8.4.2