Re: [bug-inetutils] [PATCH] ftpd

[Alfred M. Szmidt]

   I saw the developer wanted advert and would like to help out. I have
   not contributed to an open source project previously.

Welcome, but please note that inetutils is not a open source project
it is a free software project--our goals are not of pragmatism but to
defend computer users freedom to run, study, modify and distribute
software.  Please see
http://gnu.org/philosophy/open-source-misses-the-point.html for a
longer explanation about the issue.

   The patch is to allow ftpd to take a port number. It was mentioned
   in the TODO file.

Thank you for your patch, and thank you for adding the feature!

You have a few cosmetic fixes that are incorrect.  Could you also
write a short NEWS entry, update the manual and write a ChangeLog
entry (see the ChangeLog for examples on how to do that).

The patch is very small so we won't need a copyright assignment right
now, but if you plan on contributing more, then we would need one.  

   --- a/ftpd/ftpd.c
   +++ b/ftpd/ftpd.c
   @@ -1,4 +1,4 @@
   -/* - Ftp Server
   + /* - Ftp Server
     * Copyright (c) 1985, 1988, 1990, 1992, 1993, 1994
     *  The Regents of the University of California.  All rights reserved.
     *
   @@ -166,6 +166,7 @@ static off_t file_size;
    static off_t byte_count;
    static sig_atomic_t transflag;      /* Flag where in a middle of transfer.  
*/
    static const char *pid_file = PATH_FTPDPID;
   +static int port_no=-1;          /* Listen on non standard port */

Spaces between the equal sign, and the comment is superfluous.

    #if !defined(CMASK) || CMASK == 0
    # undef CMASK
    # define CMASK 027
   @@ -294,6 +295,9 @@ static struct argp_option options[] = {
      { "  default", 0, NULL, OPTION_DOC|OPTION_NO_TRANS,
        "passwd authentication",
        GRID+3 },
   +  { "port", 'P', "PORT", 0,
   +    "listen on non standard port",
   +    GRID+1 },

A more descriptive string would be `listen for connections on port
PORT'.

   @@ -397,6 +401,12 @@ parse_opt (int key, char *arg, struct argp_state *state)
             defumask = val;
           break;
          }
   +
   +    case 'P':
   +      {
   +        port_no = atoi (arg);
   +        break;
   +      }

Please use strol instead since atoi is obsolete, and there should be a
range check here since the only valid port numbers are 0-65535.  Have
you checked what happens if the port number is 0?

I'd also just name the variable `port', a port is always a number.

   --- a/ftpd/server_mode.c
   +++ b/ftpd/server_mode.c
   @@ -75,13 +75,15 @@ reapchild (int signo ARG_UNUSED)
    }

    int
   -server_mode (const char *pidfile, struct sockaddr_in *phis_addr)
   +server_mode (const char *pidfile, int port_no, struct sockaddr_in 
*phis_addr)
    {
      int ctl_sock, fd;
      struct servent *sv;
      int port;
      static struct sockaddr_in server_addr;    /* Our address.  */

   +  port = port_no;

This isn't needed, you can just use the variable that is passed to
server_mode.