You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@httpd.apache.org by GitBox <gi...@apache.org> on 2021/05/04 12:11:35 UTC

[GitHub] [httpd] atlesn opened a new pull request #185: Rewrite of mod_unique_id:

atlesn opened a new pull request #185:
URL: https://github.com/apache/httpd/pull/185


   The old implementation has multiple issues also when the latest counter-modification is taken into account.
   
   The old module was a bit difficult to understand with a lot of extra logic doing byte per byte operations "manually". I'm proposing a new structure to the whole module which is easier to modify.
   
   The actual problem with uniqueness cannot be solved as intended by the original authors 20+ years ago. This implementation used IP-addresses inside the unique ID, this has since been removed.
   
   Maybe a configuration parameter could provide a server ID (set manually) which can be used inside the string.
   
   I'm also unsure about this "Thread ID" parameter, I haven't found any reliable source for this parameter. Currently a truncated pointer is used.
   
   - Use Process ID, Thread ID, Timestamp, 16-bit per thread counter
     and 16 random bits
   - Use provided library function for encoding base64
   - Use packed struct instead of addressing each member manually in
     and index
   - Use global counter /only/ if threads are not being used
   - General re-structuring of functions
   - Initialize counters on demand, remove two init functions
   - Straight forward to change the build-up of the unique ID parameters
     after discussion.
   - TODO : Add endian conversion, find better thread ID


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org

[GitHub] [httpd] atlesn commented on pull request #185: Rewrite of mod_unique_id:

Posted by GitBox <gi...@apache.org>.
atlesn commented on pull request #185:
URL: https://github.com/apache/httpd/pull/185#issuecomment-841325993


   I've added the configuration parameter **UniqueIdServerID** which is used to ensure uniqueness across different servers. I've also implemented byte swapping and changed the method of retrieving a thread ID.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org

[GitHub] [httpd] atlesn commented on pull request #185: Rewrite of mod_unique_id:

Posted by GitBox <gi...@apache.org>.
atlesn commented on pull request #185:
URL: https://github.com/apache/httpd/pull/185#issuecomment-832597109


   Which byte swap functions should be used for the fixed width types that work on all platforms and that preferably are used elsewhere in the code?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org

[GitHub] [httpd] atlesn edited a comment on pull request #185: Rewrite of mod_unique_id:

Posted by GitBox <gi...@apache.org>.
atlesn edited a comment on pull request #185:
URL: https://github.com/apache/httpd/pull/185#issuecomment-841325993


   I've added the configuration parameter **UniqueIdServerId** which is used to ensure uniqueness across different servers. I've also implemented byte swapping and changed the method of retrieving a thread ID.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org

[GitHub] [httpd] atlesn edited a comment on pull request #185: Rewrite of mod_unique_id:

Posted by GitBox <gi...@apache.org>.
atlesn edited a comment on pull request #185:
URL: https://github.com/apache/httpd/pull/185#issuecomment-902943065


   @wjcarpenter Thanks, I can see that somebody linked in this PR there as well. I've also posted this diff on bugzilla https://bz.apache.org/bugzilla/show_bug.cgi?id=65307


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org

[GitHub] [httpd] wjcarpenter commented on pull request #185: Rewrite of mod_unique_id:

Posted by GitBox <gi...@apache.org>.
wjcarpenter commented on pull request #185:
URL: https://github.com/apache/httpd/pull/185#issuecomment-902083965


   @atlesn You might be interested in some of the conversation taking place on this bugid: https://bz.apache.org/bugzilla/show_bug.cgi?id=65159


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org

[GitHub] [httpd] ylavic commented on a change in pull request #185: Rewrite of mod_unique_id:

Posted by GitBox <gi...@apache.org>.
ylavic commented on a change in pull request #185:
URL: https://github.com/apache/httpd/pull/185#discussion_r692955246



##########
File path: modules/metadata/mod_unique_id.c
##########
@@ -19,238 +19,285 @@
  *
  * Original author: Dean Gaudet <dg...@arctic.org>
  * UUencoding modified by: Alvaro Martinez Echevarria <al...@lander.es>
+ * Complete rewrite by: Atle Solbakken <at...@goliathdns.no>, April 2021
  */
 
-#define APR_WANT_BYTEFUNC   /* for htons() et al */
-#include "apr_want.h"
-#include "apr_general.h"    /* for APR_OFFSETOF                */
-#include "apr_network_io.h"
+/* Enable when ready to use new library encoder
+ * #define WITH_APR_ENCODE
+ */
+
+/* Enable debug to error log
+ * #define UNIQUE_ID_DEBUG
+ */
 
+#define THREADED_COUNTER "unique_id_counter"
+
+#include "apr.h"
+
+#ifdef WITH_APR_ENCODE
+#    include "apr_encode.h"
+#endif
+#include "apr_thread_proc.h"
+#include "apr_cstr.h"
+
+#include "ap_mpm.h"
 #include "httpd.h"
 #include "http_config.h"
 #include "http_log.h"
-#include "http_protocol.h"  /* for ap_hook_post_read_request */
+#include "http_protocol.h"
+
+#ifdef HAVE_SYS_GETTID
+#    include <sys/syscall.h>
+#    include <sys/types.h>
+#endif
+
+#if APR_HAVE_UNISTD_H
+#    include <unistd.h>
+#endif
+#if APR_HAVE_PROCESS_H
+#    include <process.h>
+#endif
 
-#define ROOT_SIZE 10
+typedef apr_uint16_t unique_counter;
 
-typedef struct {
-    unsigned int stamp;
-    char root[ROOT_SIZE];
-    unsigned short counter;
-    unsigned int thread_index;
+/* Unique ID structure members must be aligned on 1-byte boundaries */
+
+#pragma pack(push)
+#pragma pack(1)
+
+typedef struct unique_id_rec {
+    apr_uint32_t process_id;
+    apr_uint64_t thread_id;

Review comment:
       Since the struct is packed the field `thread_id` (uint64_t) is now unaligned, can we access it without faulting (SIGBUS) on platforms like sparc?
   Do we really need packing here, or e.g. `process_id` could be moved below `timestamp` to let the struct be naturally aligned (with no holes)?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org

[GitHub] [httpd] ylavic commented on a change in pull request #185: Rewrite of mod_unique_id:

Posted by GitBox <gi...@apache.org>.
ylavic commented on a change in pull request #185:
URL: https://github.com/apache/httpd/pull/185#discussion_r692955246



##########
File path: modules/metadata/mod_unique_id.c
##########
@@ -19,238 +19,285 @@
  *
  * Original author: Dean Gaudet <dg...@arctic.org>
  * UUencoding modified by: Alvaro Martinez Echevarria <al...@lander.es>
+ * Complete rewrite by: Atle Solbakken <at...@goliathdns.no>, April 2021
  */
 
-#define APR_WANT_BYTEFUNC   /* for htons() et al */
-#include "apr_want.h"
-#include "apr_general.h"    /* for APR_OFFSETOF                */
-#include "apr_network_io.h"
+/* Enable when ready to use new library encoder
+ * #define WITH_APR_ENCODE
+ */
+
+/* Enable debug to error log
+ * #define UNIQUE_ID_DEBUG
+ */
 
+#define THREADED_COUNTER "unique_id_counter"
+
+#include "apr.h"
+
+#ifdef WITH_APR_ENCODE
+#    include "apr_encode.h"
+#endif
+#include "apr_thread_proc.h"
+#include "apr_cstr.h"
+
+#include "ap_mpm.h"
 #include "httpd.h"
 #include "http_config.h"
 #include "http_log.h"
-#include "http_protocol.h"  /* for ap_hook_post_read_request */
+#include "http_protocol.h"
+
+#ifdef HAVE_SYS_GETTID
+#    include <sys/syscall.h>
+#    include <sys/types.h>
+#endif
+
+#if APR_HAVE_UNISTD_H
+#    include <unistd.h>
+#endif
+#if APR_HAVE_PROCESS_H
+#    include <process.h>
+#endif
 
-#define ROOT_SIZE 10
+typedef apr_uint16_t unique_counter;
 
-typedef struct {
-    unsigned int stamp;
-    char root[ROOT_SIZE];
-    unsigned short counter;
-    unsigned int thread_index;
+/* Unique ID structure members must be aligned on 1-byte boundaries */
+
+#pragma pack(push)
+#pragma pack(1)
+
+typedef struct unique_id_rec {
+    apr_uint32_t process_id;
+    apr_uint64_t thread_id;

Review comment:
       Since the struct is packed the field `thread_id` (uint64_t) is now unaligned, can we access it without faulting (SIGBUS) on platforms like sparc?
   Do we really need packing here, or e.g. `process_id` could be moved below `timestamp` to let the struct be naturally aligned?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org

[GitHub] [httpd] atlesn commented on a change in pull request #185: Rewrite of mod_unique_id:

Posted by GitBox <gi...@apache.org>.
atlesn commented on a change in pull request #185:
URL: https://github.com/apache/httpd/pull/185#discussion_r693206008



##########
File path: modules/metadata/mod_unique_id.c
##########
@@ -19,238 +19,285 @@
  *
  * Original author: Dean Gaudet <dg...@arctic.org>
  * UUencoding modified by: Alvaro Martinez Echevarria <al...@lander.es>
+ * Complete rewrite by: Atle Solbakken <at...@goliathdns.no>, April 2021
  */
 
-#define APR_WANT_BYTEFUNC   /* for htons() et al */
-#include "apr_want.h"
-#include "apr_general.h"    /* for APR_OFFSETOF                */
-#include "apr_network_io.h"
+/* Enable when ready to use new library encoder
+ * #define WITH_APR_ENCODE
+ */
+
+/* Enable debug to error log
+ * #define UNIQUE_ID_DEBUG
+ */
 
+#define THREADED_COUNTER "unique_id_counter"
+
+#include "apr.h"
+
+#ifdef WITH_APR_ENCODE
+#    include "apr_encode.h"
+#endif
+#include "apr_thread_proc.h"
+#include "apr_cstr.h"
+
+#include "ap_mpm.h"
 #include "httpd.h"
 #include "http_config.h"
 #include "http_log.h"
-#include "http_protocol.h"  /* for ap_hook_post_read_request */
+#include "http_protocol.h"
+
+#ifdef HAVE_SYS_GETTID
+#    include <sys/syscall.h>
+#    include <sys/types.h>
+#endif
+
+#if APR_HAVE_UNISTD_H
+#    include <unistd.h>
+#endif
+#if APR_HAVE_PROCESS_H
+#    include <process.h>
+#endif
 
-#define ROOT_SIZE 10
+typedef apr_uint16_t unique_counter;
 
-typedef struct {
-    unsigned int stamp;
-    char root[ROOT_SIZE];
-    unsigned short counter;
-    unsigned int thread_index;
+/* Unique ID structure members must be aligned on 1-byte boundaries */
+
+#pragma pack(push)
+#pragma pack(1)
+
+typedef struct unique_id_rec {
+    apr_uint32_t process_id;
+    apr_uint64_t thread_id;

Review comment:
       Yes, so basically order them by size? 64, 64, 32, 16, 16




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org

[GitHub] [httpd] atlesn commented on pull request #185: Rewrite of mod_unique_id:

Posted by GitBox <gi...@apache.org>.
atlesn commented on pull request #185:
URL: https://github.com/apache/httpd/pull/185#issuecomment-902943065


   @wjcarpenter Thanks, I can see somebody linked in this PR as well. I've also posted this diff on bugzilla https://bz.apache.org/bugzilla/show_bug.cgi?id=65307


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org