* [PATCH] shadow: Implement putspent
@ 2013-11-05 7:47 Michael Forney
2013-11-05 19:24 ` Rich Felker
0 siblings, 1 reply; 9+ messages in thread
From: Michael Forney @ 2013-11-05 7:47 UTC (permalink / raw)
To: musl
---
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] shadow: Implement putspent
2013-11-05 7:47 [PATCH] shadow: Implement putspent Michael Forney
@ 2013-11-05 19:24 ` Rich Felker
2013-11-05 22:30 ` [PATCH v2] " Michael Forney
0 siblings, 1 reply; 9+ messages in thread
From: Rich Felker @ 2013-11-05 19:24 UTC (permalink / raw)
To: musl
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] shadow: Implement putspent
2013-11-05 19:24 ` Rich Felker
@ 2013-11-05 22:30 ` Michael Forney
2013-11-05 23:31 ` Rich Felker
0 siblings, 1 reply; 9+ messages in thread
From: Michael Forney @ 2013-11-05 22:30 UTC (permalink / raw)
To: musl
---
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] shadow: Implement putspent
2013-11-05 22:30 ` [PATCH v2] " Michael Forney
@ 2013-11-05 23:31 ` Rich Felker
2013-11-06 13:20 ` Jens Gustedt
2013-11-24 6:10 ` [PATCH v3] " Michael Forney
0 siblings, 2 replies; 9+ messages in thread
From: Rich Felker @ 2013-11-05 23:31 UTC (permalink / raw)
To: musl
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] shadow: Implement putspent
2013-11-05 23:31 ` Rich Felker
@ 2013-11-06 13:20 ` Jens Gustedt
2013-11-06 16:36 ` Rich Felker
2013-11-24 6:10 ` [PATCH v3] " Michael Forney
1 sibling, 1 reply; 9+ messages in thread
From: Jens Gustedt @ 2013-11-06 13:20 UTC (permalink / raw)
To: musl
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] shadow: Implement putspent
2013-11-06 13:20 ` Jens Gustedt
@ 2013-11-06 16:36 ` Rich Felker
2013-11-07 22:35 ` Jens Gustedt
0 siblings, 1 reply; 9+ messages in thread
From: Rich Felker @ 2013-11-06 16:36 UTC (permalink / raw)
To: musl
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] shadow: Implement putspent
2013-11-06 16:36 ` Rich Felker
@ 2013-11-07 22:35 ` Jens Gustedt
2013-11-08 0:37 ` Szabolcs Nagy
0 siblings, 1 reply; 9+ messages in thread
From: Jens Gustedt @ 2013-11-07 22:35 UTC (permalink / raw)
To: musl
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] shadow: Implement putspent
2013-11-07 22:35 ` Jens Gustedt
@ 2013-11-08 0:37 ` Szabolcs Nagy
0 siblings, 0 replies; 9+ messages in thread
From: Szabolcs Nagy @ 2013-11-08 0:37 UTC (permalink / raw)
To: musl
* 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3] shadow: Implement putspent
2013-11-05 23:31 ` Rich Felker
2013-11-06 13:20 ` Jens Gustedt
@ 2013-11-24 6:10 ` Michael Forney
1 sibling, 0 replies; 9+ messages in thread
From: Michael Forney @ 2013-11-24 6:10 UTC (permalink / raw)
To: musl
---
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-11-24 6:10 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-05 7:47 [PATCH] shadow: Implement putspent Michael Forney
2013-11-05 19:24 ` Rich Felker
2013-11-05 22:30 ` [PATCH v2] " Michael Forney
2013-11-05 23:31 ` Rich Felker
2013-11-06 13:20 ` Jens Gustedt
2013-11-06 16:36 ` Rich Felker
2013-11-07 22:35 ` Jens Gustedt
2013-11-08 0:37 ` Szabolcs Nagy
2013-11-24 6:10 ` [PATCH v3] " Michael Forney
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/musl/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).