Re: dateutils: FTBFS on hurd-i386 (for review)
On Fri, 2014-05-16 at 15:29 +0200, Philipp A. Hartmann wrote:
> Svante,
>
> On 15/05/14 22:39, Svante Signell wrote:
> > On Thu, 2014-05-15 at 13:38 +0200, Philipp A. Hartmann wrote:
> >
> > Yes, you are right. However, setting len to zero would have solved that.
>
> I'm not sure if passing len=0 to snprintf is portable in all cases.
The manpage said the implementation differs. For Hurd it worked, I
tested. No big deal.
> If
> you insist on reporting the error, I'd prefer to use "error()" and avoid
> the snprintf call by early returning NULL.
Not needed, anybody else wanting it?
> >>> #if !defined L_tmpnam
> >>> # define L_tmpnam (PATH_MAX)
> >>> #endif /* !L_tmpnam */
> >>> - static char expfn[PATH_MAX];
> >>> - static char actfn[PATH_MAX];
> >>
> >> Why not simply replace PATH_MAX with L_tmpnam and be done with
> >> everything? Is this defined on GNU/Hurd? Is it used elsewhere in the file?
> >
> > No and no.
>
> If this is currently not used, it should probably be dropped to avoid a
> later reintroduction of PATH_MAX.
OK, we will se what the debian maintainers/upstream says.
> >>> + static char *expfn = NULL;
> >>> + static char *actfn = NULL;
> >>
> >> Why keep them static?
> >
> > Why not?
>
> Because static variables are shared across multiple invocations of the
> function. In the case at hand, this leads to a skipped
> (re)initialization to NULL from the second call onwards and the memory
> handling breaks.
Thanks for the explanation, fixed.
> > + if ( expfn == NULL || mkfifo(expfn, 0666) < 0) {
>
> Whitespace error.
> if (expfn ...
>
> > + if ( actfn == NULL || mkfifo(actfn, 0666) < 0) {
>
> Same here.
I had fixed this and looked at the old and updated attachment in gedit
under evolution. Obviously evolution does not update the attachment,
while gedit sees the changes and reloads the patch.
--- a/test/clittool.c
+++ b/test/clittool.c
@@ -619,11 +619,15 @@ xclosefrom(int fd)
return;
}
-static void
-mkfifofn(char *restrict buf, size_t bsz, const char *key, unsigned int tid)
+static char*
+mkfifofn(const char *key, unsigned int tid)
{
- snprintf(buf, bsz, "%s output %x", key, tid);
- return;
+ char *buf = NULL;
+ size_t len = strlen(key) + 9 + 8 + 1;
+ buf = malloc(len);
+ if (buf)
+ snprintf(buf, len, "%s output %x", key, tid);
+ return buf;
}
static pid_t
@@ -670,23 +674,27 @@ differ(struct clit_chld_s ctx[static 1],
#if !defined L_tmpnam
# define L_tmpnam (PATH_MAX)
#endif /* !L_tmpnam */
- static char expfn[PATH_MAX];
- static char actfn[PATH_MAX];
+ char *expfn = NULL;
+ char *actfn = NULL;
pid_t difftool = -1;
assert(!clit_bit_fd_p(exp));
- if (clit_bit_fn_p(exp) &&
- (strlen(exp.d) >= sizeof(expfn) || strcpy(expfn, exp.d) == NULL)) {
- error("cannot prepare in file `%s'", exp.d);
- goto out;
- } else if (!clit_bit_fn_p(exp) &&
- (mkfifofn(expfn, sizeof(expfn), "expected", ctx->test_id),
- mkfifo(expfn, 0666) < 0)) {
- error("cannot create fifo `%s'", expfn);
- goto out;
- } else if (mkfifofn(actfn, sizeof(actfn), "actual", ctx->test_id),
- mkfifo(actfn, 0666) < 0) {
+ if (clit_bit_fn_p(exp)) {
+ expfn = malloc(strlen(exp.d) + 1);
+ if (expfn == NULL || strcpy(expfn, exp.d) == NULL) {
+ error("cannot prepare in file `%s'", exp.d);
+ goto out;
+ }
+ } else {
+ expfn = mkfifofn("expected", ctx->test_id);
+ if (expfn == NULL || mkfifo(expfn, 0666) < 0) {
+ error("cannot create fifo `%s'", expfn);
+ goto out;
+ }
+ }
+ actfn = mkfifofn("actual", ctx->test_id);
+ if (actfn == NULL || mkfifo(actfn, 0666) < 0) {
error("cannot create fifo `%s'", actfn);
goto out;
}
@@ -701,7 +709,7 @@ differ(struct clit_chld_s ctx[static 1],
case 0:;
/* i am the child */
- static char *const diff_opt[] = {
+ char *const diff_opt[] = {
"diff",
"-u",
expfn, actfn, NULL,
@@ -761,11 +769,14 @@ differ(struct clit_chld_s ctx[static 1],
unblock_sigs();
out:
- if (*expfn && !clit_bit_fn_p(exp)) {
- unlink(expfn);
+ if (expfn) {
+ if (!clit_bit_fn_p(exp))
+ unlink(expfn);
+ free(expfn);
}
- if (*actfn) {
+ if (actfn) {
unlink(actfn);
+ free(actfn);
}
return difftool;
}
Reply to: