On Thu, 2010-07-29 at 17:44 +0200, Karel Zak wrote:
> Hi,
>
> it would be nice to have a regression test (see tests/) before you
> start to rewrite any tool.
>
> On Wed, Jul 28, 2010 at 06:32:32PM -0400, Davidlohr Bueso wrote:
> > +/* if not global, we cannot free it when SIGINTs occur */
> > +wchar_t *p;
>
> horrible name for a global variable :-)
I agree, I didn't want to change too many stuff.
> >
> > void
> > usage(void)
> > @@ -143,3 +69,105 @@ usage(void)
> > (void)fprintf(stderr, _("usage: rev [file ...]\n"));
> > exit(1);
> > }
>
> Please, try to use usage() from (for example) sys-utils/fallocate.c.
> I think that "rev -?" should not print the output to stderr.
>
> > +static void
> > +sig_handler(int signo)
> > +{
> > + switch(signo) {
> > + case SIGINT: /* very used Ctrl-C when no file is passed as argv */
> > + case SIGTERM:
> > + if(p)
> > + free(p);
>
> please, don't use "if" before free.
>
> > + }
> > +
> > + exit (EXIT_SUCCESS);
> > +}
> > +
> > +int
> > +main(int argc, char *argv[])
> > +{
> > + register char *filename;
> > + register wchar_t *t;
>
> I don't think that "register" is necessary here.
>
> > + size_t len, buflen = 512;
>
> what about BUFSIZ instead 512 bytes?
>
> > + FILE *fp;
> > + int ch, rval;
> > +
> > + setlocale(LC_ALL, "");
> > + bindtextdomain(PACKAGE, LOCALEDIR);
> > + textdomain(PACKAGE);
> > +
> > + signal(SIGINT, sig_handler);
> > + signal(SIGTERM, sig_handler);
> > +
> > + /* if no mem left, no bother continuing */
> > + p = malloc(buflen*sizeof(wchar_t));
>
> do we really need to allocate the buffer before we parse argv[] ?
Not at all, again, didn't intend to change the basic logic of the
program.
If you want I can make all the suggested changes and resend. Let me
know, otherwise is the regression test enough?
Thanks,
Davidlohr
> > + if (p == NULL)
> > + err(1, _("unable to allocate bufferspace"));
>
> err(EXIT_FAILURE, ....)
>
> > + while ((ch = getopt(argc, argv, "")) != -1)
> > + switch(ch) {
> > + case '?':
>
> what about "-h" (and ideally "--help" too) ?
>
> > + default:
> > + free(p);
> > + usage();
> > + }
> > +
> > + argc -= optind;
> > + argv += optind;
> > +
> > + fp = stdin;
> > + filename = "stdin";
> > + rval = 0;
>
> rval = EXIT_SUCCESS;
>
> > + do {
> > + if (*argv) {
> > + if ((fp = fopen(*argv, "r")) == NULL) {
> > + warn("cannot open %s", *argv );
> > + rval = 1;
> > + ++argv;
> > + continue;
> > + }
> > + filename = *argv++;
> > + }
> > +
> > + while (fgetws(p, buflen, fp)) {
> > +
> > + len = wcslen(p);
> > +
> > + /* This is my hack from setpwnam.c -janl */
> > + while (p[len-1] != '\n' && !feof(fp)) {
> > + /* Extend input buffer if it failed getting the whole line */
> > +
> > + /* So now we double the buffer size */
> > + buflen *= 2;
> > +
> > + p = realloc(p, buflen*sizeof(wchar_t));
> > + if (p == NULL)
> > + err(1, _("unable to allocate bufferspace"));
> > +
> > + /* And fill the rest of the buffer */
> > + if (fgetws(&p[len], buflen/2, fp) == NULL) break;
> > +
> > + len = wcslen(p);
> > +
> > + /* That was a lot of work for nothing. Gimme perl! */
> > + }
> > +
> > + t = p + len - 1 - (*(p+len-1)=='\r' || *(p+len-1)=='\n');
> > + for ( ; t >= p; --t)
> > + if (*t != 0)
> > + putwchar(*t);
> > + putwchar('\n');
> > + }
> > + fflush(fp);
> > + if (ferror(fp)) {
> > + warn("%s", filename);
> > + rval = 1;
>
> rval = EXIT_FAILURE;
>
> > + }
> > + if (fclose(fp))
> > + rval = 1;
> > + } while(*argv);
> > +
> > + free(p);
> > +
> > + exit(rval);
>
> return rval
>
> > +}
>
> Karel
>
--
To unsubscribe from this list: send the line "unsubscribe util-linux-ng" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
