httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <wr...@lnd.com>
Subject RE: Global data is evil :)
Date Wed, 09 Aug 2000 05:06:16 GMT
> From: rbb@covalent.net [mailto:rbb@covalent.net]
> Sent: Monday, August 07, 2000 9:23 AM
> 
> On Mon, 7 Aug 2000, William A. Rowe, Jr. wrote:
> 
> > Does anyone object to this untested (but compiling) revision?
> > Assuming the answer is no, I will commit it Monday eve, with the
> > appropriate changes to http_main.c and mpm/win32/service.c
> 
> Yes, I object.  That getopt code was being used because it was from
> FreeBSD.  We were trying to avoid using a standard function name and then
> having it behave in a completely unexpected way.  By using the code from
> FreeBSD, we know the code works, and we know that it looks like other
> getopt code.  The only reason we replaced getopt was that it wasn't
> standard across all platforms.
> 
> What problem are you trying to solve with this?  The only problem with
> this code, is that it uses globabl variable, which makes it
> non-threadsafe, but we are using this function to parse command line
> options.  We shouldn't need to do that after we have spawned threads.

Based on the patch, and your objection, I've created the following (tested)
patch to http_main.c to illustrated the proposed apr_initopt/apr_getopt
processing without globals.  To my eyes, this looks far cleaner, but you
be the judge.  I haven't removed any functionallity from the original getopt,
in that you have the entire apr_getopt_t structure publicly defined, and are
welcome to flip arguments around, as appropriate.

Any further comments before I scrap this proposal?

Index: http_main.c
===================================================================
RCS file: /home/cvs/apache-2.0/src/main/http_main.c,v
retrieving revision 1.63
diff -u -r1.63 http_main.c
--- http_main.c	2000/08/06 06:07:34	1.63
+++ http_main.c	2000/08/09 04:58:48
@@ -276,7 +276,7 @@
 
 int main(int argc, char *argv[])
 {
-    int c;
+    char c;
     int configtestonly = 0;
     const char *confname = SERVER_CONFIG_FILE;
     const char *def_server_root = HTTPD_ROOT;
@@ -287,8 +287,10 @@
     apr_pool_t *plog; /* Pool of log streams, reset _after_ each read of conf */
     apr_pool_t *ptemp; /* Pool for temporary config stuff, reset often */
     apr_pool_t *pcommands; /* Pool for -D, -C and -c switches */
+    apr_getopt_t *opt;
     module **mod;
     ap_directive_t *conftree = NULL;
+    const char *optarg;
 
     apr_initialize();
     process = create_process(argc, (char *const *)argv);
@@ -316,28 +318,29 @@
     /* Maintain AP_SERVER_BASEARGS list in http_main.h to allow the MPM 
      * to safely pass on our args from its rewrite_args() handler.
      */
-    while (apr_getopt(process->argc, process->argv, 
-                     AP_SERVER_BASEARGS, &c, pcommands) 
+    apr_initopt(&opt, pcommands, process->argc, process->argv);
+
+    while (apr_getopt(opt, AP_SERVER_BASEARGS, &c, &optarg) 
             == APR_SUCCESS) {
         char **new;
         switch (c) {
  	case 'c':
 	    new = (char **)apr_push_array(ap_server_post_read_config);
-	    *new = apr_pstrdup(pcommands, apr_optarg);
+	    *new = apr_pstrdup(pcommands, optarg);
 	    break;
 	case 'C':
 	    new = (char **)apr_push_array(ap_server_pre_read_config);
-	    *new = apr_pstrdup(pcommands, apr_optarg);
+	    *new = apr_pstrdup(pcommands, optarg);
 	    break;
 	case 'd':
-	    def_server_root = apr_optarg;
+	    def_server_root = optarg;
 	    break;
 	case 'D':
 	    new = (char **)apr_push_array(ap_server_config_defines);
-	    *new = apr_pstrdup(pcommands, apr_optarg);
+	    *new = apr_pstrdup(pcommands, optarg);
 	    break;
 	case 'f':
-	    confname = apr_optarg;
+	    confname = optarg;
 	    break;
 	case 'v':
 	    printf("Server version: %s\n", ap_get_server_version());


And here are the changes to testargs.c in lib/apr/test

Index: testargs.c
===================================================================
RCS file: /home/cvs/apache-2.0/src/lib/apr/test/testargs.c,v
retrieving revision 1.14
diff -u -r1.14 testargs.c
--- testargs.c	2000/08/06 06:07:28	1.14
+++ testargs.c	2000/08/09 05:00:53
@@ -66,25 +66,32 @@
 int main(int argc, char * const argv[])
 {
     apr_pool_t *context;
-    apr_int32_t data;
+    apr_getopt_t *opt;
+    char data;
+    const char *optarg;
 
     apr_initialize();
     atexit(apr_terminate);
     apr_create_pool(&context, NULL);
 
-    while (apr_getopt(argc, argv, "abc:d::", &data, context) == APR_SUCCESS) {
+    if (apr_initopt(&opt, context, argc, argv))
+    {
+        printf("failed to initialize opts");
+        exit(1);
+    }
+    while (apr_getopt(opt, "abc:d::", &data, &optarg) == APR_SUCCESS) {
         switch(data) {
             case 'a':
             case 'b':
                 printf("option %c\n", data);
                 break;
             case 'c':
-                printf("option %c with %s\n", data, apr_optarg);
+                printf("option %c with %s\n", data, optarg);
                 break;
             case 'd':
                 printf("option %c", data);
                 if (apr_optarg) {
-                    printf(" with %s\n", apr_optarg);
+                    printf(" with %s\n", optarg);
                 }
                 else {
                     printf("\n");

And as a final illustration, here are the changes to ab.c in support:

Index: ab.c
===================================================================
RCS file: /home/cvs/apache-2.0/src/support/ab.c,v
retrieving revision 1.22
diff -u -r1.22 ab.c
--- ab.c	2000/08/06 06:07:53	1.22
+++ ab.c	2000/08/09 05:05:15
@@ -970,11 +970,12 @@
 /* sort out command-line args and call test */
 int main(int argc, char **argv)
 {
-    int c, r, l;
+    int r, l;
     char tmp[1024];
-#ifdef NOT_ASCII
     apr_status_t status;
-#endif
+    apr_getopt_t *opt;
+    char *optarg;
+    char c;
 
     /* table defaults  */
     tablestring = "";
@@ -1006,11 +1007,11 @@
     }
 #endif
 
-    apr_optind = 1;
-    while (apr_getopt(argc, argv, "n:c:t:T:p:v:kVhwix:y:z:C:H:P:A:", &c, cntxt) == APR_SUCCESS)
{
+    apr_initopt(&opt, cntxt, argc, argv);
+    while ((status = apr_getopt(opt, "n:c:t:T:p:v:kVhwix:y:z:C:H:P:A:", &c, &optarg))
== APR_SUCCESS) {
         switch (c) {
         case 'n':
-            requests = atoi(apr_optarg);
+            requests = atoi(optarg);
             if (!requests) {
                err("Invalid number of requests\n");
             }
@@ -1019,7 +1020,7 @@
             keepalive = 1;
             break;
         case 'c':
-            concurrency = atoi(apr_optarg);
+            concurrency = atoi(optarg);
             break;
         case 'i':
             if (posting == 1)
@@ -1030,7 +1031,7 @@
             if (posting != 0)
                 err("Cannot mix POST and HEAD\n");
 
-            if (0 == (r = open_postfile(apr_optarg))) {
+            if (0 == (r = open_postfile(optarg))) {
                posting = 1;
             }
             else if (postdata) {
@@ -1038,27 +1039,27 @@
             }
             break;
         case 'v':
-            verbosity = atoi(apr_optarg);
+            verbosity = atoi(optarg);
             break;
         case 't':
-            tlimit = atoi(apr_optarg);
+            tlimit = atoi(optarg);
             requests = MAX_REQUESTS;  /* need to size data array on something */
             break;
         case 'T':
-            strcpy(content_type, apr_optarg);
+            strcpy(content_type, optarg);
             break;
         case 'C':
             strncat(cookie, "Cookie: ", sizeof(cookie));
-            strncat(cookie, apr_optarg, sizeof(cookie));
+            strncat(cookie, optarg, sizeof(cookie));
             strncat(cookie, "\r\n", sizeof(cookie));
             break;
         case 'A':
             /* assume username passwd already to be in colon separated form. 
              * Ready to be uu-encoded.
              */
-            while(isspace(*apr_optarg))
-                apr_optarg++;
-            l=ap_base64encode(tmp, apr_optarg, strlen(apr_optarg));
+            while(isspace(*optarg))
+                optarg++;
+            l=ap_base64encode(tmp, optarg, strlen(optarg));
             tmp[l]='\0';
  
             strncat(auth, "Authorization: basic ", sizeof(auth));
@@ -1069,9 +1070,9 @@
             /*
              * assume username passwd already to be in colon separated form.
              */
-            while(isspace(*apr_optarg))
-                apr_optarg++;
-            l=ap_base64encode(tmp, apr_optarg, strlen(apr_optarg));
+            while(isspace(*optarg))
+                optarg++;
+            l=ap_base64encode(tmp, optarg, strlen(optarg));
             tmp[l]='\0';
  
             strncat(auth, "Proxy-Authorization: basic ", sizeof(auth));
@@ -1079,7 +1080,7 @@
             strncat(auth, "\r\n", sizeof(auth));
             break;
         case 'H':
-            strncat(hdrs, apr_optarg, sizeof(hdrs));
+            strncat(hdrs, optarg, sizeof(hdrs));
             strncat(hdrs, "\r\n", sizeof(hdrs));
             break;
 	case 'w':
@@ -1091,15 +1092,15 @@
 	     */
 	case 'x':
 	    use_html = 1;
-	    tablestring = apr_optarg;
+	    tablestring = optarg;
 	    break;
 	case 'y':
 	    use_html = 1;
-	    trstring = apr_optarg;
+	    trstring = optarg;
 	    break;
 	case 'z':
 	    use_html = 1;
-	    tdstring = apr_optarg;
+	    tdstring = optarg;
 	    break;
 	case 'h':
 	    usage(argv[0]);
@@ -1110,12 +1111,12 @@
         }
     }
 
-    if (apr_optind != argc - 1) {
+    if (opt->ind != argc - 1) {
         fprintf(stderr, "%s: wrong number of arguments\n", argv[0]);
         usage(argv[0]);
     }
 
-    if (parse_url(argv[apr_optind++])) {
+    if (parse_url((char*)opt->argv[opt->ind++])) {
         fprintf(stderr, "%s: invalid URL\n", argv[0]);
         usage(argv[0]);
     }

The oddness in the last change is due to the constness of parse_url.

Bill



Mime
View raw message