#51206 Issue 50984 - Fix disk_mon_check_diskspace types
Closed 3 years ago by spichugi. Opened 3 years ago by spichugi.
spichugi/389-ds-base i50984  into  master

file modified
+3 -3
@@ -345,8 +345,8 @@ 

      uint64_t freeBytes = 0;

      uint64_t blockSize = 0;

      char *worst_dir = NULL;

-     int hit_threshold = 0;

-     int i = 0;

+     int32_t hit_threshold = 0;

+     int32_t i = 0;

  

      for (i = 0; dirs && dirs[i]; i++) {

          if (statvfs(dirs[i], &buf) != -1) {
@@ -396,7 +396,7 @@ 

      char *dirstr = NULL;

      uint64_t previous_mark = 0;

      uint64_t disk_space = 0;

-     int64_t threshold = 0;

+     uint64_t threshold = 0;

      uint64_t halfway = 0;

      time_t start = 0;

      time_t now = 0;

@@ -2031,7 +2031,7 @@ 

  

      if (apply) {

          CFG_LOCK_WRITE(slapdFrontendConfig);

-         slapdFrontendConfig->disk_threshold = threshold;

+         slapdFrontendConfig->disk_threshold = (uint64_t)threshold;

          CFG_UNLOCK_WRITE(slapdFrontendConfig);

      }

  
@@ -5164,11 +5164,11 @@ 

      return retVal;

  }

  

- PRInt64

+ uint64_t

  config_get_disk_threshold()

  {

      slapdFrontendConfig_t *slapdFrontendConfig = getFrontendConfig();

-     PRInt64 retVal;

+     uint64_t retVal;

  

      CFG_LOCK_READ(slapdFrontendConfig);

      retVal = slapdFrontendConfig->disk_threshold;

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

          char **dirs = NULL;

          char *dirstr = NULL;

          uint64_t disk_space = 0;

-         int64_t threshold = 0;

+         uint64_t threshold = 0;

          uint64_t halfway = 0;

          threshold = config_get_disk_threshold();

          halfway = threshold / 2;

@@ -545,7 +545,7 @@ 

  int config_get_accesslog_logging_enabled(void);

  int config_get_disk_monitoring(void);

  int config_get_disk_threshold_readonly(void);

- PRInt64 config_get_disk_threshold(void);

+ uint64_t config_get_disk_threshold(void);

  int config_get_disk_grace_period(void);

  int config_get_disk_logging_critical(void);

  int config_get_ndn_cache_count(void);

Description: Function parameters are inconsistence.
Documentation states that threshold should be 2^63 - 1
so we can use int64_t for that.

https://pagure.io/389-ds-base/issue/50984

Reviewed by: ?

Do we also need config get threshold to return a uint64_t not PRInt64 (aka int64_t)

And would that imply that threshold is valid to take -1 and that has some meaning?

And would that imply that threshold is valid to take -1 and that has some meaning?

Documentation states that it is 0 to the maximum 64-bit integer value (9223372036854775807) on 64-bit systems. 9223372036854775807 is the highest int64 and we have our config_set and config_get working with signed int.

So I think it makes more sense to stick with the docs and with the existing logic for this PR.

If you think we need to extend the range (by setting uint64), I'd say it is a small RFE (with documentation fix) and it should be addressed in a separate ticket.

As for this issue, I am fixing the static checks so it is consistent.

Okay cool. let's stick to the docs I agree 100%. It's just that in DS some values CAN take -1 so I wanted to check :)

I think the issue now would be to explicitly do the cast from int64_t to uint64_t in get threshold so that it can return a uint64_t, and we keep the logic the same. Is that reasonable?

Okay cool. let's stick to the docs I agree 100%. It's just that in DS some values CAN take -1 so I wanted to check :)
I think the issue now would be to explicitly do the cast from int64_t to uint64_t in get threshold so that it can return a uint64_t, and we keep the logic the same. Is that reasonable?

Yeah, makes sense) I'll finish up and push the change to PR tomorrow.

@spichugi Thanks so much! I thought about doing it myself but I figured since you had worked on it recently, you would still have in your mind all the tricks around the code. Thanks for your time, and sleep well mate,

rebased onto 654d0ff

3 years ago

Ack from me, thanks mate!

Just to be sure I'm going to run it through the staci checker too :)

Is disk_mon_check_diskspace missing from a header file too?

ds > grep -i -r -n -e 'disk_mon_check_diskspace' .
./ldap/servers/slapd/main.c:902:        dirstr = disk_mon_check_diskspace(dirs, threshold, &disk_space);
./ldap/servers/slapd/daemon.c:341:disk_mon_check_diskspace(char **dirs, uint64_t threshold, uint64_t *disk_space)
./ldap/servers/slapd/daemon.c:454:        dirstr = disk_mon_check_diskspace(dirs, threshold, &disk_space);
./ldap/servers/slapd/daemon.c:572:                dirstr = disk_mon_check_diskspace(dirs, threshold, &disk_space);

Okay, I think maybe that something weird happened, as in the 1.4.2 branch, the line:

./ldap/servers/slapd/proto-slap.h:1514:char *disk_mon_check_diskspace(char **dirs, uint64_t threshold, uint64_t *disk_space);

Is missing. So we'll need ot pay attention to that when/if we backport this.

So, ack still stands, but we should bring this back to 1.4.3 and 1.4.2 (if @mreynolds is okay with that), and the 1.4.2 branch needs proto-slap.h fixed. Is that okay?

Yeah, originally, the feature was developed for 1.4.3.
https://pagure.io/389-ds-base/issue/50960

But I think this PR can be backported to both 1.4.2 and 1.4.3. I'll leave @mreynolds to make a final decision though.

Pull-Request has been merged by spichugi

3 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/4259

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