389-ds-base

Clone
Source Code
GIT

#50844 Issue 49254 - Fix compiler failures and warnings
Closed 3 years ago by spichugi. Opened 4 years ago by mreynolds.
mreynolds/389-ds-base fedNoCommon  into  master

file modified
-2 
@@ -760,7 +760,6 @@ 
 

  

  INPUT                  = src/libsds/include/sds.h \
 

                           docs/job-safety.md \
 

-                          src/nunc-stans/include/nunc-stans.h
 

                           # ldap/servers/slapd/slapi-plugin.h \
 

  

  # This tag can be used to specify the character encoding of the source files
 
@@ -1101,7 +1100,6 @@ 
 

  

  # HTML_EXTRA_FILES       = docs/nunc-stans-intro.png \
 

  # 						 docs/nunc-stans-job-states.png
 

- HTML_EXTRA_FILES = docs/nunc-stans-job-states.png
 

  

  # The HTML_COLORSTYLE_HUE tag controls the color of the HTML output. Doxygen
 

  # will adjust the colors in the style sheet and background images according to

file modified
+1 
@@ -13,6 +13,7 @@ 
 

  

  #include "acl.h"
 

  

+ 

  /****************************************************************************
 

  *
 

  * acl.c

file modified
+2 -2 
@@ -311,8 +311,8 @@ 
 

  #define ATTR_ACLPB_MAX_SELECTED_ACLS    "nsslapd-aclpb-max-selected-acls"
 

  #define DEFAULT_ACLPB_MAX_SELECTED_ACLS 200
 

  

- int aclpb_max_selected_acls; /* initialized from plugin config entry */
 

- int aclpb_max_cache_results; /* initialized from plugin config entry */
 

+ extern int aclpb_max_selected_acls; /* initialized from plugin config entry */

If I understand correctly, extern will only declare the variable but it still needs to be defined somewhere otherwise it can't be used

A possibility is to declare the variables in header file (extern) like you did. Then declare the effective variable in the acl_ext.c where it is set, optionally with default value 'int aclpb_max_selected_acls = DEFAULT_ACLPB_MAX_SELECTED_ACLS;'

 
+ extern int aclpb_max_cache_results; /* initialized from plugin config entry */
 

  

  typedef struct result_cache
 

  {

file modified
+2 
@@ -23,6 +23,8 @@ 
 

  static Acl_PBlock *acl__malloc_aclpb(void);
 

  static void acl__free_aclpb(Acl_PBlock **aclpb_ptr);
 

  

+ int aclpb_max_selected_acls = DEFAULT_ACLPB_MAX_SELECTED_ACLS;
 

+ int aclpb_max_cache_results = DEFAULT_ACLPB_MAX_SELECTED_ACLS;
 

  

  struct acl_pbqueue
 

  {

file modified
+5 -3 
@@ -1954,9 +1954,11 @@ 
 

               */
 

              buflen -= len;
 

              p += len;
 

-             /* Put in the end quote, then back track p. */
 

-             *p++ = '"';
 

-             *p--;
 

+             /*
 

+              * Put in the end quote. If another snp_detail is append a comma
 

+              * will overwrite the quote.
 

+              */
 

+             *(p + 1) = '"';
 

          }
 

      }

file modified
+2 -2 
@@ -935,7 +935,7 @@ 
 

  };
 

  

  /* DataList definition */
 

- struct datalist
 

+ typedef struct datalist
 

  {
 

      void **elements;   /* array of elements */
 

      int element_count; /* number of elements in the array */
 
@@ -1737,7 +1737,7 @@ 
 

   *  * Online tasks interface (to support import, export, etc)
 

   *   * After some cleanup, we could consider making these public.
 

   *    */
 

- struct slapi_task
 

+ typedef struct slapi_task
 

  {
 

      struct slapi_task *next;
 

      char *task_dn;

file modified
+1 -1 
@@ -698,7 +698,7 @@ 
 

              }
 

              if (mode & VERY_VERBOSE)
 

                  printf("ldclt[%d]: T%03d: Before ldap_simple_bind_s (%s, %s)\n",
 

-                        mctx.pid, thrdNum, binddn,
 

+                        mctx.pid, thrdNum, binddn ? binddn : "Anonymous",
 

                         passwd ? passwd : "NO PASSWORD PROVIDED");
 

              ret = ldap_sasl_bind_s(ld, binddn,
 

                                     LDAP_SASL_SIMPLE, &cred, NULL, NULL, &servercredp); /*JLS 05-01-01*/

file modified
+34 -22 
@@ -221,13 +221,13 @@ 
 

   * sds_crc32c uses the crc32c algorithm to create a verification checksum of data.
 

   * This checksum is for data verification, not cryptographic purposes. It is used
 

   * largely in debugging to find cases when bytes in structures are updated incorrectly,
 

-  * or to find memory bit flips during operation. If avaliable, this will use the
 

+  * or to find memory bit flips during operation. If available, this will use the
 

   * intel sse4 crc32c hardware acceleration.
 

   *
 

   * \param crc The running CRC value. Initially should be 0. If in doubt, use 0.
 

   * \param data Pointer to the data to checksum.
 

   * \param length number of bytes to validate.
 

-  * \retval crc The crc of this data. May be re-used in subsequent sds_crc32c calls
 

+  * \retval rcrc The crc of this data. May be re-used in subsequent sds_crc32c calls
 

   * for certain datatypes.
 

   */
 

  uint32_t sds_crc32c(uint32_t crc, const unsigned char *data, size_t length);
 
@@ -1356,48 +1356,60 @@ 
 

      SDS_HT_BRANCH = 2,
 

  } sds_ht_slot_state;
 

  

+ /**
 

+  * ht values
 

+  */
 

  typedef struct _sds_ht_value
 

  {
 

-     uint32_t checksum;
 

-     void *key;
 

-     void *value;
 

+     uint32_t checksum;  /**< the checksum */
 

+     void *key;  /**< the key */
 

+     void *value;  /**< the key value */
 

      // may make this a LL of values later for collisions
 

  } sds_ht_value;
 

  

+ /**
 

+  * ht slot
 

+  */
 

  typedef struct _sds_ht_slot
 

  {
 

-     sds_ht_slot_state state;
 

+     sds_ht_slot_state state; /**< the checksum */
 

      union
 

      {
 

          sds_ht_value *value;
 

          struct _sds_ht_node *node;
 

-     } slot;
 

+     } slot;  /**< slot union */
 

  } sds_ht_slot;
 

  

+ /**
 

+  *  ht node
 

+  */
 

  typedef struct _sds_ht_node
 

  {
 

-     uint32_t checksum;
 

-     uint64_t txn_id;
 

-     uint_fast32_t count;
 

+     uint32_t checksum; /**< the checksum */
 

+     uint64_t txn_id; /**< transaction id */
 

+     uint_fast32_t count; /**< the count */
 

  #ifdef SDS_DEBUG
 

      uint64_t depth;
 

  #endif
 

-     struct _sds_ht_node *parent;
 

-     size_t parent_slot;
 

-     sds_ht_slot slots[HT_SLOTS];
 

+     struct _sds_ht_node *parent; /**< the parent */
 

+     size_t parent_slot; /**< the parent slot */
 

+     sds_ht_slot slots[HT_SLOTS]; /**< the slots */
 

  } sds_ht_node;
 

  

+ /**
 

+  *  ht instance
 

+  */
 

  typedef struct _sds_ht_instance
 

  {
 

-     uint32_t checksum;
 

-     char hkey[16];
 

-     sds_ht_node *root;
 

-     int64_t (*key_cmp_fn)(void *a, void *b);
 

-     uint64_t (*key_size_fn)(void *key);
 

-     void *(*key_dup_fn)(void *key);
 

-     void (*key_free_fn)(void *key);
 

-     void *(*value_dup_fn)(void *value);
 

-     void (*value_free_fn)(void *value);
 

+     uint32_t checksum; /**< the checksum */
 

+     char hkey[16]; /**< the key */
 

+     sds_ht_node *root; /**< the root */
 

+     int64_t (*key_cmp_fn)(void *a, void *b); /**< the keycompare function */
 

+     uint64_t (*key_size_fn)(void *key); /**< the key size function */
 

+     void *(*key_dup_fn)(void *key); /**< the key dup function */
 

+     void (*key_free_fn)(void *key); /**< the key free function */
 

+     void *(*value_dup_fn)(void *value); /**< the value dup function */
 

+     void (*value_free_fn)(void *value); /**< the value free function */
 

  } sds_ht_instance;
 

  

  uint64_t sds_uint64_t_size(void *key);

Description:

Fix issues with new gcc compiler flag "-fno-common", and clean up doxygen warnings around libsds

relates: https://pagure.io/389-ds-base/issue/49254

If I understand correctly, extern will only declare the variable but it still needs to be defined somewhere otherwise it can't be used

Yeah, I think @spichugi is right here, the change to these two values could be an issue. Looking at the code I see:

./ldap/servers/plugins/acl/acllist.c:662:        if (index >= aclpb_max_selected_acls - 2) {
./ldap/servers/plugins/acl/acllist.c:722:                        (index < aclpb_max_selected_acls - 2);
./ldap/servers/plugins/acl/acllist.c:758:            if (index >= aclpb_max_selected_acls - 2) {
./ldap/servers/plugins/acl/acllist.c:848:    if ((!scan_entire_list && (*cookie >= (aclpb_max_selected_acls - 1))) ||
./ldap/servers/plugins/acl/acl.h:308: * of aclpb_max_selected_acls and aclpb_max_cache_results.
./ldap/servers/plugins/acl/acl.h:309: * If not set, DEFAULT_ACLPB_MAX_SELECTED_ACLS will be used.
./ldap/servers/plugins/acl/acl.h:311:#define ATTR_ACLPB_MAX_SELECTED_ACLS    "nsslapd-aclpb-max-selected-acls"
./ldap/servers/plugins/acl/acl.h:312:#define DEFAULT_ACLPB_MAX_SELECTED_ACLS 200
./ldap/servers/plugins/acl/acl.h:314:int aclpb_max_selected_acls; /* initialized from plugin config entry */
./ldap/servers/plugins/acl/acl_ext.c:141:        slapi_ch_calloc(aclpb_max_selected_acls, sizeof(int));
./ldap/servers/plugins/acl/acl_ext.c:366:    int value = slapi_entry_attr_get_int(e, ATTR_ACLPB_MAX_SELECTED_ACLS);
./ldap/servers/plugins/acl/acl_ext.c:368:        aclpb_max_selected_acls = value;
./ldap/servers/plugins/acl/acl_ext.c:371:        aclpb_max_selected_acls = DEFAULT_ACLPB_MAX_SELECTED_ACLS;
./ldap/servers/plugins/acl/acl_ext.c:372:        aclpb_max_cache_results = DEFAULT_ACLPB_MAX_SELECTED_ACLS;
./ldap/servers/plugins/acl/acl_ext.c:662:        slapi_ch_calloc(aclpb_max_selected_acls, sizeof(int));
./ldap/servers/plugins/acl/acl_ext.c:664:        slapi_ch_calloc(aclpb_max_selected_acls, sizeof(int));
./ldap/servers/plugins/acl/acl_ext.c:673:        slapi_ch_calloc(aclpb_max_selected_acls, sizeof(int));
./ldap/servers/plugins/acl/acl_ext.c:675:        slapi_ch_calloc(aclpb_max_selected_acls, sizeof(int));
./ldap/servers/plugins/acl/acl_ext.c:677:        slapi_ch_calloc(aclpb_max_selected_acls, sizeof(int));
./ldap/servers/plugins/acl/acl.c:1947:        while (kk < aclpb_max_selected_acls && aclpb->aclpb_handles_index[kk] != -1) {
./ldap/servers/plugins/acl/acl.c:2672:        while (kk < aclpb_max_selected_acls && aclpb->aclpb_handles_index[kk] >= 0)
./ldap/servers/plugins/acl/acl.c:2674:        if (kk >= aclpb_max_selected_acls) {
./ldap/servers/plugins/acl/acl.c:2689:        if (nhandle < aclpb_max_selected_acls) {
./ldap/servers/plugins/acl/acl.c:3105:                                  j, aclpb_max_cache_results, ATTR_ACLPB_MAX_SELECTED_ACLS);
./ldap/servers/plugins/acl/acl.c:3328:                                  j, aclpb_max_cache_results, ATTR_ACLPB_MAX_SELECTED_ACLS);

Which means the int is never defined as having a location in the programs memory as extern is only to provide a shared name I think. I think to make this valid the extern int lines need to be:

extern int aclpb_max_... = 0;

In the .h, because that will cause the compiler to allocate memory to them. How this works when it's included in multiple files though. ..... I'm not sure. Either way, I sense something going on here ...

I'm curious if we still don't need to backtrack since we are in a loop and the p is defined outside of it. I'd maybe instead of these two lines do something like *(p+1) = '"';.

Which means the int is never defined as having a location in the programs memory as extern is only to provide a shared name I think. I think to make this valid the extern int lines need to be:
extern int aclpb_max_... = 0;

This causes the build to fail with the same errors I'm trying to clean up. I'll run an ACI test or two tomorrow and see if anything arises with the current patch.

I agree with @mhonek suggestion those two lines sets '"' after p (and we are sure to have the enough room to do so).
Comments are precious to understand the loop but this one could be improved with something like

 /* Put in the end quote. I another snp_detail is append,  a comma will overwrite the quote*/

A possibility is to declare the variables in header file (extern) like you did. Then declare the effective variable in the acl_ext.c where it is set, optionally with default value 'int aclpb_max_selected_acls = DEFAULT_ACLPB_MAX_SELECTED_ACLS;'

rebased onto 6f3ae5e08f0fc759ea919185080e5985e426d4b5
4 years ago

Changes applied, please review...

Actually both variables have the same default DEFAULT_ACLPB_MAX_SELECTED_ACLS (according to acl__handle_plugin_config_entry)

Actually both variables have the same default DEFAULT_ACLPB_MAX_SELECTED_ACLS (according to acl__handle_plugin_config_entry)

I agree. Other than that, LGTM!

rebased onto 79f8351fc156807e836b5f3c9e1028007a078384
4 years ago

rebased onto 3c38d33
4 years ago

Pull-Request has been merged by mreynolds
4 years ago

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3898

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi
3 years ago
Metadata
None
No Tags
Changes Summary 8
+0 -2
file changed
docs/slapi.doxy.in
+1 -0
file changed
ldap/servers/plugins/acl/acl.c
+2 -2
file changed
ldap/servers/plugins/acl/acl.h
+2 -0
file changed
ldap/servers/plugins/acl/acl_ext.c
+5 -3
file changed
ldap/servers/slapd/result.c
+2 -2
file changed
ldap/servers/slapd/slap.h
+1 -1
file changed
ldap/servers/slapd/tools/ldclt/ldapfct.c
+34 -22
file changed
src/libsds/include/sds.h
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.