On Thu, Feb 14, 2019 at 08:32:11AM +0100, Martijn van Duren wrote:
> I would like to see some more functionality in ldap(1), so I started of 
> with delete, because that's seems to be the easiest/shortest to
> implement and the diffs are big enough as is.
> 
> I split it up in 2 diffs. This is the first one, which restructures
> ldap(1) to make use of a per command environment.
> 
> Thoughts? OK?

I'm not thrilled by the way you do the getopt handling.
Having dynamic getopt strings is intransparent. It will also fail the
moment a flag is reused with a different meaning.
I would prefer a more straight forward solution with one single getopt
option string and conflict resolution during or after option parsing.
 
Also I would use some kind of enum or define for the mode instead of using
strcmp to know the mode.

> martijn@
> 
> diff --git a/ldapclient.c b/ldapclient.c
> index 9763b8e..9f29997 100644
> --- a/ldapclient.c
> +++ b/ldapclient.c
> @@ -52,6 +52,8 @@
>  #define F_NEEDAUTH   0x04
>  #define F_LDIF               0x08
>  
> +#define GETOPT_COMMON        "c:D:H:vWw:xy:Z"
> +
>  #define LDAPHOST     "localhost"
>  #define LDAPFILTER   "(objectClass=*)"
>  #define LDIF_LINELENGTH      79
> @@ -79,9 +81,21 @@ struct ldapc_search {
>       char                    **ls_attr;
>  };
>  
> +struct ldapc_app {
> +     const char              *name;
> +     const char              *optstring;
> +     const char              *usage;
> +     const char              *pledge;
> +     int                     (*exec)(int, char *[]);
> +     struct ldapc             ldap;
> +     union {
> +             struct ldapc_search ls;
> +     };
> +};
> +
>  __dead void   usage(void);
>  int           ldapc_connect(struct ldapc *);
> -int           ldapc_search(struct ldapc *, struct ldapc_search *);
> +int           ldapc_search(int, char *[]);
>  int           ldapc_printattr(struct ldapc *, const char *,
>                   const struct ber_octetstring *);
>  void          ldapc_disconnect(struct ldapc *);
> @@ -90,115 +104,132 @@ int             ldapc_parseurl(struct ldapc *, struct 
> ldapc_search *,
>  const char   *ldapc_resultcode(enum result_code);
>  const char   *url_decode(char *);
>  
> -__dead void
> -usage(void)
> -{
> -     extern char     *__progname;
> -
> -     fprintf(stderr,
> -"usage: %s search [-LvWxZ] [-b basedn] [-c CAfile] [-D binddn] [-H host]\n"
> -"        [-l timelimit] [-s scope] [-w secret] [-y secretfile] [-z 
> sizelimit]\n"
> -"        [filter] [attributes ...]\n",
> -         __progname);
> -
> -     exit(1);
> -}
> +struct ldapc_app ldapc_apps[] = {
> +     {"search", "Lb:s:l:z:", "[-L] [-b basedn] [-s scope] [-l timelimit] "
> +         "[-z sizelimit] [filter] [attributes ...]", "stdio", ldapc_search,
> +         {0}, {0}}
> +};
> +struct ldapc_app *ldapc_app = NULL;
>  
>  int
>  main(int argc, char *argv[])
>  {
> +     char                     optstr[BUFSIZ];
>       char                     passbuf[LDAPPASSMAX];
>       const char              *errstr, *url = NULL, *secretfile = NULL;
>       struct stat              st;
> -     struct ldapc             ldap;
> -     struct ldapc_search      ls;
>       int                      ch;
>       int                      verbose = 1;
> +     size_t                   i;
>       FILE                    *fp;
>  
>       if (pledge("stdio inet unix tty rpath dns", NULL) == -1)
>               err(1, "pledge");
>  
> -     log_init(verbose, 0);
> +     if (strlcpy(optstr, GETOPT_COMMON, sizeof(optstr)) >= sizeof(optstr))
> +             errx(1, "strlcpy");
>  
> -     memset(&ldap, 0, sizeof(ldap));
> -     memset(&ls, 0, sizeof(ls));
> -     ls.ls_scope = -1;
> -     ldap.ldap_port = -1;
> +     log_init(verbose, 0);
>  
> -     /*
> -      * Check the command.  Currently only "search" is supported but
> -      * it could be extended with others such as add, modify, or delete.
> -      */
>       if (argc < 2)
>               usage();
> -     else if (strcmp("search", argv[1]) == 0)
> -             ldap.ldap_req = LDAP_REQ_SEARCH;
> -     else
> +     for (i = 0; i < sizeof(ldapc_apps)/sizeof(*ldapc_apps); i++) {
> +             if (strcmp(ldapc_apps[i].name, argv[1]) == 0) {
> +                     ldapc_app = &ldapc_apps[i];
> +                     break;
> +             }
> +     }
> +     if (ldapc_app == NULL)
>               usage();
>       argc--;
>       argv++;
>  
> -     while ((ch = getopt(argc, argv, "b:c:D:H:Ll:s:vWw:xy:Zz:")) != -1) {
> +     if (strcmp(ldapc_app->name, "search") == 0)
> +             ldapc_app->ls.ls_scope = -1;
> +     ldapc_app->ldap.ldap_port = -1;
> +
> +
> +     if (strlcat(optstr, ldapc_app->optstring, sizeof(optstr)) >=
> +         sizeof(optstr))
> +             errx(1, "strlcat optstr");
> +     while ((ch = getopt(argc, argv, optstr)) != -1) {
>               switch (ch) {
>               case 'b':
> -                     ls.ls_basedn = optarg;
> -                     break;
> +                     if (strcmp(ldapc_app->name, "search") == 0) {
> +                             ldapc_app->ls.ls_basedn = optarg;
> +                             break;
> +                     }
> +                     usage();
>               case 'c':
> -                     ldap.ldap_capath = optarg;
> +                     ldapc_app->ldap.ldap_capath = optarg;
>                       break;
>               case 'D':
> -                     ldap.ldap_binddn = optarg;
> -                     ldap.ldap_flags |= F_NEEDAUTH;
> +                     ldapc_app->ldap.ldap_binddn = optarg;
> +                     ldapc_app->ldap.ldap_flags |= F_NEEDAUTH;
>                       break;
>               case 'H':
>                       url = optarg;
>                       break;
>               case 'L':
> -                     ldap.ldap_flags |= F_LDIF;
> -                     break;
> +                     if (strcmp(ldapc_app->name, "search") == 0) {
> +                             ldapc_app->ldap.ldap_flags |= F_LDIF;
> +                             break;
> +                     }
> +                     usage();
>               case 'l':
> -                     ls.ls_timelimit = strtonum(optarg, 0, INT_MAX,
> -                         &errstr);
> -                     if (errstr != NULL)
> -                             errx(1, "timelimit %s", errstr);
> -                     break;
> +                     if (strcmp(ldapc_app->name, "search") == 0) {
> +                             ldapc_app->ls.ls_timelimit = strtonum(optarg, 0,
> +                                 INT_MAX, &errstr);
> +                             if (errstr != NULL)
> +                                     errx(1, "timelimit %s", errstr);
> +                             break;
> +                     }
> +                     usage();
>               case 's':
> -                     if (strcasecmp("base", optarg) == 0)
> -                             ls.ls_scope = LDAP_SCOPE_BASE;
> -                     else if (strcasecmp("one", optarg) == 0)
> -                             ls.ls_scope = LDAP_SCOPE_ONELEVEL;
> -                     else if (strcasecmp("sub", optarg) == 0)
> -                             ls.ls_scope = LDAP_SCOPE_SUBTREE;
> -                     else
> -                             errx(1, "invalid scope: %s", optarg);
> -                     break;
> +                     if (strcmp(ldapc_app->name, "search") == 0) {
> +                             if (strcasecmp("base", optarg) == 0)
> +                                     ldapc_app->ls.ls_scope =
> +                                         LDAP_SCOPE_BASE;
> +                             else if (strcasecmp("one", optarg) == 0)
> +                                     ldapc_app->ls.ls_scope =
> +                                         LDAP_SCOPE_ONELEVEL;
> +                             else if (strcasecmp("sub", optarg) == 0)
> +                                     ldapc_app->ls.ls_scope =
> +                                         LDAP_SCOPE_SUBTREE;
> +                             else
> +                                     errx(1, "invalid scope: %s", optarg);
> +                             break;
> +                     }
> +                     usage();
>               case 'v':
>                       verbose++;
>                       break;
>               case 'w':
> -                     ldap.ldap_secret = optarg;
> -                     ldap.ldap_flags |= F_NEEDAUTH;
> +                     ldapc_app->ldap.ldap_secret = optarg;
> +                     ldapc_app->ldap.ldap_flags |= F_NEEDAUTH;
>                       break;
>               case 'W':
> -                     ldap.ldap_flags |= F_NEEDAUTH;
> +                     ldapc_app->ldap.ldap_flags |= F_NEEDAUTH;
>                       break;
>               case 'x':
>                       /* provided for compatibility */
>                       break;
>               case 'y':
>                       secretfile = optarg;
> -                     ldap.ldap_flags |= F_NEEDAUTH;
> +                     ldapc_app->ldap.ldap_flags |= F_NEEDAUTH;
>                       break;
>               case 'Z':
> -                     ldap.ldap_flags |= F_STARTTLS;
> +                     ldapc_app->ldap.ldap_flags |= F_STARTTLS;
>                       break;
>               case 'z':
> -                     ls.ls_sizelimit = strtonum(optarg, 0, INT_MAX,
> -                         &errstr);
> -                     if (errstr != NULL)
> -                             errx(1, "sizelimit %s", errstr);
> -                     break;
> +                     if (strcmp(ldapc_app->name, "search") == 0) {
> +                             ldapc_app->ls.ls_sizelimit = strtonum(optarg, 0,
> +                                 INT_MAX, &errstr);
> +                             if (errstr != NULL)
> +                                     errx(1, "sizelimit %s", errstr);
> +                             break;
> +                     }
> +                     usage();
>               default:
>                       usage();
>               }
> @@ -208,33 +239,41 @@ main(int argc, char *argv[])
>  
>       log_setverbose(verbose);
>  
> -     if (url != NULL && ldapc_parseurl(&ldap, &ls, url) == -1)
> -             errx(1, "ldapurl");
> +     if (url != NULL && strcmp(ldapc_app->name, "search") == 0) {
> +             if (ldapc_parseurl(&ldapc_app->ldap, &ldapc_app->ls, url) == -1)
> +                     errx(1, "ldapurl");
> +     } else if (url != NULL && strcmp(ldapc_app->name, "search") != 0) {
> +             if (ldapc_parseurl(&ldapc_app->ldap, NULL, url) == -1)
> +                     errx(1, "ldapurl");
> +     }
>  
>       /* Set the default after parsing URL and/or options */
> -     if (ldap.ldap_host == NULL)
> -             ldap.ldap_host = LDAPHOST;
> -     if (ldap.ldap_port == -1)
> -             ldap.ldap_port = ldap.ldap_protocol == LDAPS ?
> -                 LDAPS_PORT : LDAP_PORT;
> -     if (ldap.ldap_protocol == LDAP && (ldap.ldap_flags & F_STARTTLS))
> -             ldap.ldap_protocol = LDAPTLS;
> -     if (ldap.ldap_capath == NULL)
> -             ldap.ldap_capath = tls_default_ca_cert_file();
> -     if (ls.ls_basedn == NULL)
> -             ls.ls_basedn = "";
> -     if (ls.ls_scope == -1)
> -             ls.ls_scope = LDAP_SCOPE_SUBTREE;
> -     if (ls.ls_filter == NULL)
> -             ls.ls_filter = LDAPFILTER;
> -
> -     if (ldap.ldap_flags & F_NEEDAUTH) {
> -             if (ldap.ldap_binddn == NULL) {
> +     if (ldapc_app->ldap.ldap_host == NULL)
> +             ldapc_app->ldap.ldap_host = LDAPHOST;
> +     if (ldapc_app->ldap.ldap_port == -1)
> +             ldapc_app->ldap.ldap_port = ldapc_app->ldap.ldap_protocol ==
> +                 LDAPS ? LDAPS_PORT : LDAP_PORT;
> +     if (ldapc_app->ldap.ldap_protocol == LDAP &&
> +         (ldapc_app->ldap.ldap_flags & F_STARTTLS))
> +             ldapc_app->ldap.ldap_protocol = LDAPTLS;
> +     if (ldapc_app->ldap.ldap_capath == NULL)
> +             ldapc_app->ldap.ldap_capath = tls_default_ca_cert_file();
> +     if (strcmp(ldapc_app->name, "search") == 0) {
> +             if (ldapc_app->ls.ls_basedn == NULL)
> +                     ldapc_app->ls.ls_basedn = "";
> +             if (ldapc_app->ls.ls_scope == -1)
> +                     ldapc_app->ls.ls_scope = LDAP_SCOPE_SUBTREE;
> +             if (ldapc_app->ls.ls_filter == NULL)
> +                     ldapc_app->ls.ls_filter = LDAPFILTER;
> +     }
> +
> +     if (ldapc_app->ldap.ldap_flags & F_NEEDAUTH) {
> +             if (ldapc_app->ldap.ldap_binddn == NULL) {
>                       log_warnx("missing -D binddn");
>                       usage();
>               }
>               if (secretfile != NULL) {
> -                     if (ldap.ldap_secret != NULL)
> +                     if (ldapc_app->ldap.ldap_secret != NULL)
>                               errx(1, "conflicting -w/-y options");
>  
>                       /* read password from stdin or file (first line) */
> @@ -252,47 +291,39 @@ main(int argc, char *argv[])
>                               fclose(fp);
>  
>                       passbuf[strcspn(passbuf, "\n")] = '\0';
> -                     ldap.ldap_secret = passbuf;
> +                     ldapc_app->ldap.ldap_secret = passbuf;
>               }
> -             if (ldap.ldap_secret == NULL) {
> +             if (ldapc_app->ldap.ldap_secret == NULL) {
>                       if (readpassphrase("Password: ",
>                           passbuf, sizeof(passbuf), RPP_REQUIRE_TTY) == NULL)
>                               errx(1, "failed to read LDAP password");
> -                     ldap.ldap_secret = passbuf;
> +                     ldapc_app->ldap.ldap_secret = passbuf;
>               }
>       }
>  
>       if (pledge("stdio inet unix rpath dns", NULL) == -1)
>               err(1, "pledge");
>  
> -     /* optional search filter */
> -     if (argc && strchr(argv[0], '=') != NULL) {
> -             ls.ls_filter = argv[0];
> -             argc--;
> -             argv++;
> -     }
> -     /* search attributes */
> -     if (argc)
> -             ls.ls_attr = argv;
> -
> -     if (ldapc_connect(&ldap) == -1)
> +     if (ldapc_connect(&ldapc_app->ldap) == -1)
>               errx(1, "LDAP connection failed");
>  
> -     if (pledge("stdio", NULL) == -1)
> +     if (pledge(ldapc_app->pledge, NULL) == -1)
>               err(1, "pledge");
>  
> -     if (ldapc_search(&ldap, &ls) == -1)
> -             errx(1, "LDAP search failed");
> +     if (ldapc_app->exec(argc, argv) == -1)
> +             errx(1, "LDAP %s failed", ldapc_app->name);
>  
> -     ldapc_disconnect(&ldap);
> -     aldap_free_url(&ldap.ldap_url);
> +     ldapc_disconnect(&ldapc_app->ldap);
> +     aldap_free_url(&ldapc_app->ldap.ldap_url);
>  
>       return (0);
>  }
>  
>  int
> -ldapc_search(struct ldapc *ldap, struct ldapc_search *ls)
> +ldapc_search(int argc, char *argv[])
>  {
> +     struct ldapc                    *ldap;
> +     struct ldapc_search             *ls;
>       struct aldap_page_control       *pg = NULL;
>       struct aldap_message            *m;
>       const char                      *errstr;
> @@ -302,6 +333,19 @@ ldapc_search(struct ldapc *ldap, struct ldapc_search *ls)
>       int                              ret, code, fail = 0;
>       size_t                           i;
>  
> +     ldap = &ldapc_app->ldap;
> +     ls = &ldapc_app->ls;
> +
> +     /* optional search filter */
> +     if (argc && strchr(argv[0], '=') != NULL) {
> +             ls->ls_filter = argv[0];
> +             argc--;
> +             argv++;
> +     }
> +     /* search attributes */
> +     if (argc)
> +             ls->ls_attr = argv;
> +
>       if (ldap->ldap_flags & F_LDIF)
>               printf("version: 1\n");
>       do {
> @@ -675,7 +719,7 @@ ldapc_resultcode(enum result_code code)
>       default:
>               return ("UNKNOWN_ERROR");
>       }
> -};
> +}
>  
>  int
>  ldapc_parseurl(struct ldapc *ldap, struct ldapc_search *ls, const char *url)
> @@ -713,6 +757,9 @@ ldapc_parseurl(struct ldapc *ldap, struct ldapc_search 
> *ls, const char *url)
>       ldap->ldap_host = lu->host;
>       if (lu->port)
>               ldap->ldap_port = lu->port;
> +     /* Ignore search elements if we're not searching */
> +     if (ls == NULL)
> +             return (0);
>  
>       /* The distinguished name has to be URL-encoded */
>       if (lu->dn != NULL && ls->ls_basedn != NULL &&
> @@ -791,3 +838,29 @@ url_decode(char *url)
>  
>       return (url);
>  }
> +
> +__dead void
> +usage(void)
> +{
> +     size_t i;
> +     extern char     *__progname;
> +
> +     if (ldapc_app != NULL) {
> +             fprintf(stderr, "usage: %s %s %s %s %s\n",
> +                 __progname, ldapc_app->name,
> +                 "[-vWxZ] [-c CAfile] [-D binddn] [-H host] [-w secret]",
> +                 "[-y secretfile]",
> +                 ldapc_app->usage == NULL ? "" : ldapc_app->usage);
> +             exit(1);
> +     }
> +     fprintf(stderr, "usage:\n");
> +     for (i = 0; i < (sizeof(ldapc_apps)/sizeof(*ldapc_apps)); i++) {
> +             fprintf(stderr, "%*s %s %s %s %s\n",
> +                 (int) (sizeof("usage:") + strlen(__progname)), __progname,
> +                 ldapc_apps[i].name,
> +                 "[-vWxZ] [-c CAfile] [-D binddn] [-H host] [-w secret]",
> +                 "[-y secretfile]",
> +                 ldapc_apps[i].usage == NULL ? "" : ldapc_apps[i].usage);
> +     }
> +     exit(1);
> +}
> 

-- 
:wq Claudio

Reply via email to