Thanks for the feedback, I'll respond to both your replies at once. On Wed, Jun 13, 2001 at 08:24:32PM +0400, Daniel Ginsburg <[EMAIL PROTECTED]> wrote: > On Wed, Jun 13, 2001 at 10:57:08AM -0500, Steve Greenland wrote: > > Tim, good fixups, a few C coding/style nitpicks: > > > > On 12-Jun-01, 17:57 (CDT), Tim van Erven <[EMAIL PROTECTED]> wrote: > > > #include <stdio.h> > > > > #include <unistd.h> /* For execlp */ > > #include <stdlib.h> /* For exit */ > > > > > int main() > > > > int main(void) /* () != (void) in C */
The comp.lang.c faq (http://www.faqs.org/faqs/C-faq/faq/) says it's ok. > > > { > > > char name[21]; /* Should be macro (#define NAMELEN 21) */ Possibly, but the name that can be entered is at most 20 chars long, so NAMELEN should arguably be defined to 20 and the declaration for name changed to char name[NAMELEN + 1];. I don't think it adds anything to readability in this case, though. > > > printf("Login as: "); > > > fflush(stdout); > > > > > > if(fgets(name, 21, stdin)) { > > > /* if(name[strlen(name) - 1] != '\n') */ > > > > if(name[strlen(name) - 1] != '\n') { Yes, this does look slightly better. > > > > Possible access to unallocated memory if "\0\n" supplied as input. Only if strlen(name) = 0 and besides from being hard to achieve when entering data on stdin, fgets will return 0 if that happens. > > > fprintf(stderr, "Username to long.\n"); > > > /* else { */ > > > > } else { > > > name[strlen(name) - 1] = '\0'; > > > execlp("/usr/bin/ssh", "ssh", "-l", name, "foo.foo.es", (char *)0); > > > } > > > } > > > > > > /* return 0; */ > > > > exit(EXIT_SUCCESS); /* return doesn't call atexit() registered > > functions, > > which doesn't apply in this case, but it's a good > > habit to get into */ > > > > Wrong comment. Returning from main _does_ call atexit() registered > functions. > > > > } > > > > > > You also should should make sure name doesn't contain any spaces: as > > written I can pass additional options to ssh. Also, for this kind of > > application you really ought to be checking the error conditions for > > *every* library call. > > > > Spaces and other shell metacharecters are irrelevant in this case, since > executed command won't undergo shell interpretation. No comments about my spelling mistake ('to long')? Personally I considered that the worst offence. Btw, I should start posting all my code to this list. That would *really* fix-up my coding habbits. :) Miquel, if you're not comfortable integrating all this feedback, mail me directly (off the list) and I'd be happy to do it for you. Regards, -- Tim van Erven [EMAIL PROTECTED] [EMAIL PROTECTED]