Forgot to check the repl agreement port number, new patch attached

I'm happy with this, but I think @lkrispen may have some input too ...

I think @lkrispen is away this week.

I'm going to nitpick your schema changes, can we make the DESC field actually describe what the attribute does?

Should repl_config_valid_num take int32_t not int? Actually, int64_t, because strtol is returing an int64_t not an int32_t, so the bounds check may not be complete there, and we can always extend int32_t to int64_t in cases we only accept the smaller value.

Sorry that I didn't review it closer on first pass,

I think @lkrispen is away this week.
I'm going to nitpick your schema changes, can we make the DESC field actually describe what the attribute does?
Should repl_config_valid_num take int32_t not int? Actually, int64_t, because strtol is returing an int64_t not an int32_t, so the bounds check may not be complete there, and we can always extend int32_t to int64_t in cases we only accept the smaller value.

I think the function is fine the way it is. Inside the function we use strtol, and set a int64_t variable (val). So the range is correctly checked. Then we use the min and max params to fine tune the range checking.

I do not see a "need" to change the int "return val" to int64_t, it would also require a lot of other changes in "a lot" of replication configuration code. It's a can of worms. Perhaps we can look into a change like this via a different ticket? I would really like to get this into 7.5 before the dev deadline.

Also there are already plans to redo all the schema descriptions, so I would also like to skip that for now as well.

Okay I just made all the changes, but it's more to review now

Everything looks great mate, except:

1689 +
1690 +int
1691 +repl_config_valid_num(const char *config_attr, char *config_attr_value, int min, int max,
1692 +                      int *returncode, char *errortext, int64_t *retval)
1693 +{

Because int min/max are int32_t, but val is int64_t, we can't express the range properly. :(

Everything looks great mate, except:
1689 +
1690 +int
1691 +repl_config_valid_num(const char config_attr, char config_attr_value, int min, int max,
1692 + int returncode, char errortext, int64_t *retval)
1693 +{

Because int min/max are int32_t, but val is int64_t, we can't express the range properly. :(

The CI test says otherwise ;-) If " val > max", even if max is a 32bit int, it still evaluates correctly, no?

On Fri, 2017-11-03 at 01:52 +0000, Mark Reynolds wrote:

mreynolds added a new comment to an issue you are following:
``

Everything looks great mate, except:
1689 +
1690 +int
1691 +repl_config_valid_num(const char config_attr, char
config_attr_value, int min, int max,
1692 +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int r=
eturncode, char errortext,
int64_t *retval)
1693 +{
=20
=20
=20
Because int min/max are int32_t, but val is int64_t, we can't
express the range properly. :(
=20
The CI test says otherwise ;-)=C2=A0=C2=A0If " val > max", even if max is=
a
32bit int, it still evaluates correctly, no?
=20
=20

Yes, but if val > int32_t max, and we want a max of say ... greater
than that? It means we can't express the range int32_t max -> int64_t
max.=20

That's my point :)=20

IE 4294967295 -> 18446744073709551615L

:)=20

--=20
Sincerely,

William Brown
Software Engineer
Red Hat, Australia/Brisbane

Ps: email reply formatting does not look good on pagure .... :(

Anyway, if you change:

1690 +int
1691 +repl_config_valid_num(const char *config_attr, char *config_attr_value, int min, int max,
1692 +                      int *returncode, char *errortext, int64_t *retval)
1693 +{

to:

1690 +int
1691 +repl_config_valid_num(const char *config_attr, char *config_attr_value, int64_t min, int64_
t max,
1692 +                      int *returncode, char *errortext, int64_t *retval)
1693 +{

Then you have my ack :)

On Fri, 2017-11-03 at 01:52 +0000, Mark Reynolds wrote:

mreynolds added a new comment to an issue you are following:
``

Everything looks great mate, except:
1689 +
1690 +int
1691 +repl_config_valid_num(const char config_attr, char
config_attr_value, int min, int max,
1692 +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int r=
eturncode, char errortext,
int64_t *retval)
1693 +{
=20
=20
=20
Because int min/max are int32_t, but val is int64_t, we can't
express the range properly. :(
=20
The CI test says otherwise ;-)=C2=A0=C2=A0If " val > max", even if max is=
a
32bit int, it still evaluates correctly, no?
=20
=20

Yes, but if val > int32_t max, and we want a max of say ... greater
than that?

Ahh, I should of mentioned this sooner. Technically max is not supposed to be greater than an 32bit int according to the official docs. All our config values are not supposed to exceed a 32 bit int. So I was originally trying to be compliant with the docs.

Note the CI test does check "all" of this and does pass. Anyway I'll change it...

Revised patch

acking for William..

fe85945..f6b0e18 master -> master

8ca21dd..1b04a2d 389-ds-base-1.3.7 -> 389-ds-base-1.3.7

Copy and paste error detected by covscan:

e1f866a..4316470 master -> master

b94a648..578e8a1 389-ds-base-1.3.7 -> 389-ds-base-1.3.7

A 64bit int can hold a 32bit one :) And we may choose to expand that in the future :)

Anyway ,thank you so much for listening to my comments!

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 issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/1724

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.