Opened 8 months ago

Closed 5 months ago

#5525 closed enhancement (complete)

Radius: configuration storage and parsers

Reported by: tomek Owned by: fdupont
Priority: medium Milestone: Kea1.4
Component: hook-radius Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Once #5524 is implemented, the next step required to implement RadiusDesign is to write code that will be able to parse Radius configuration and store it in the configuration structures.

For details, see RadiusDesign.

Subtickets

Change History (14)

comment:1 Changed 7 months ago by fdupont

  • Owner set to fdupont
  • Status changed from new to accepted

comment:2 Changed 7 months ago by fdupont

  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing

Done. Ready for review.

comment:3 Changed 7 months ago by fdupont

  • Owner changed from UnAssigned to fdupont

Take it to simplify things.

Last edited 7 months ago by fdupont (previous) (diff)

comment:4 Changed 7 months ago by fdupont

  • Owner changed from fdupont to UnAssigned

Reported many improvements I did in trac5526.
Again available for review.

comment:5 Changed 6 months ago by tomek

  • Owner changed from UnAssigned to tomek

comment:6 follow-up: Changed 6 months ago by tomek

I recently installed fresh Ubuntu 17.10 Server (wanted to migrate to a new ssd disk for a while and broken ubuntu upgrade gave me a good reason to start from scratch). This new ubuntu has g++ 7.2.0, which may explain some of the issues reported below.

I compiled the code (kea trac5525, premium trac5525) without any problems.

The unit-tests in kea passed, but those in premium failed miserably:

$ export KEA_LOGGER_DESTINATION=stdout
$ cd premium/src/hooks/dhcp/radius/
$ make check
[==========] Running 2 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 2 tests from LibLoadTest
[ RUN      ] LibLoadTest.validLoad
2018-03-30 17:48:39.323 DEBUG [kea.hooks/8001] HOOKS_LIBRARY_LOADING loading hooks library /home/thomson/devel/kea-premium/premium/src/hooks/dhcp/radius/.libs/libdhcp_radius.so
2018-03-30 17:48:39.330 DEBUG [kea.hooks/8001] HOOKS_LIBRARY_VERSION hooks library /home/thomson/devel/kea-premium/premium/src/hooks/dhcp/radius/.libs/libdhcp_radius.so reports its version as 5
2018-03-30 17:48:39.330 ERROR [kea.hooks/8001] HOOKS_LOAD_FRAMEWORK_EXCEPTION 'load' function in hook library /home/thomson/devel/kea-premium/premium/src/hooks/dhcp/radius/.libs/libdhcp_radius.so threw an exception: reason attempt to access logging function before logging has been initialized
2018-03-30 17:48:39.330 DEBUG [kea.hooks/8001] HOOKS_LIBRARY_UNLOADING unloading library /home/thomson/devel/kea-premium/premium/src/hooks/dhcp/radius/.libs/libdhcp_radius.so
2018-03-30 17:48:39.330 ERROR [kea.hooks/8001] HOOKS_UNLOAD_FRAMEWORK_EXCEPTION 'unload' function in hook library /home/thomson/devel/kea-premium/premium/src/hooks/dhcp/radius/.libs/libdhcp_radius.so threw an exception, reason attempt to access logging function before logging has been initialized
load_unload_unittests.cc:50: Failure
Value of: HooksManager::loadLibraries(libraries_)
  Actual: false
Expected: true

I tried to run other hook libraries and flex-id, host_cmds, and subnet_cmds all worked.

The only hook that failed was legal_log with the following error:

[ RUN      ] LegalLibLoadTest.validLoad
2018-03-30 17:50:50.143 DEBUG [kea.hooks/10031] HOOKS_LIBRARY_LOADING loading hooks library /home/thomson/devel/kea-premium/premium/src/hooks/dhcp/legal_log/.libs/libdhcp_legal_log.so
2018-03-30 17:50:50.148 ERROR [kea.hooks/10031] HOOKS_OPEN_ERROR failed to open hook library /home/thomson/devel/kea-premium/premium/src/hooks/dhcp/legal_log/.libs/libdhcp_legal_log.so: /home/thomson/devel/kea-premium/premium/src/hooks/dhcp/legal_log/.libs/libdhcp_legal_log.so: undefined symbol: _ZN3isc4dhcp6CfgMgr8instanceEv
load_unload_unittests.cc:119: Failure
Value of: HooksManager::loadLibraries(libraries)
  Actual: false
Expected: true

After adding libdhcpsrv to the dependencies (commited, please pull), it started failing load with the same error as radius:

[==========] Running 2 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 2 tests from LegalLibLoadTest
[ RUN      ] LegalLibLoadTest.validLoad
2018-03-30 17:52:28.470 DEBUG [kea.hooks/12253] HOOKS_LIBRARY_LOADING loading hooks library /home/thomson/devel/kea-premium/premium/src/hooks/dhcp/legal_log/.libs/libdhcp_legal_log.so
2018-03-30 17:52:28.479 DEBUG [kea.hooks/12253] HOOKS_LIBRARY_VERSION hooks library /home/thomson/devel/kea-premium/premium/src/hooks/dhcp/legal_log/.libs/libdhcp_legal_log.so reports its version as 5
2018-03-30 17:52:28.480 ERROR [kea.hooks/12253] HOOKS_LOAD_FRAMEWORK_EXCEPTION 'load' function in hook library /home/thomson/devel/kea-premium/premium/src/hooks/dhcp/legal_log/.libs/libdhcp_legal_log.so threw an exception: reason attempt to access logging function before logging has been initialized
2018-03-30 17:52:28.480 DEBUG [kea.hooks/12253] HOOKS_LIBRARY_UNLOADING unloading library /home/thomson/devel/kea-premium/premium/src/hooks/dhcp/legal_log/.libs/libdhcp_legal_log.so
2018-03-30 17:52:28.480 DEBUG [kea.hooks/12253] HOOKS_UNLOAD_SUCCESS 'unload' function in hook library /home/thomson/devel/kea-premium/premium/src/hooks/dhcp/legal_log/.libs/libdhcp_legal_log.so returned success
2018-03-30 17:52:28.480 INFO  [kea.hooks/12253] HOOKS_LIBRARY_UNLOADED hooks library /home/thomson/devel/kea-premium/premium/src/hooks/dhcp/legal_log/.libs/libdhcp_legal_log.so successfully unloaded
load_unload_unittests.cc:119: Failure
Value of: HooksManager::loadLibraries(libraries)
  Actual: false
Expected: true
load_unload_unittests.cc:127: Failure
Value of: fileExists(expected_filename)
  Actual: false
Expected: true
load_unload_unittests.cc:134: Failure
Value of: fileExists(expected_filename)
  Actual: false
Expected: true
[  FAILED  ] LegalLibLoadTest.validLoad (13 ms)

I do not understand what is the reason of this. The liblog library is correctly listed in the dependencies.

I have reviewed the changes in kea repo and they look good. I started reviewing changes on trac5525 in premium repo, but haven't completed the review yet.

There is one failing test in radius/tests:

cd premium/src/hooks/dhcp/radius/tests
make check
[ RUN      ] ConfigTest.services
../../../../../../src/lib/testutils/test_to_element.h:54: Failure
Failed
Expected:
{
  "attributes": [ ],
  "servers": [
    {
      "name": "one",
      "port": 16460,
      "secret": "1111"
    },
    {
      "name": "two",
      "port": 1646,
      "secret": "22222"
    }
  ]
}
Actual:
{
  "attributes": [ ],
  "servers": [
    {
      "name": "one",
      "port": 16460,
      "secret": "1111"
    },
    {
      "name": "two",
      "port": 1813,
      "secret": "22222"
    }
  ]
}
Diff:
@@ -9,5 +9,5 @@
     {
       "name": "two",
-      "port": 1646,
+      "port": 1813,
       "secret": "22222"
     }


config_unittests.cc:234: Failure
      Expected: 1646
To be equal to: srv->port[1]
      Which is: 1813
[  FAILED  ] ConfigTest.services (2 ms)

comment:7 in reply to: ↑ 6 Changed 6 months ago by fdupont

Replying to tomek:

I recently installed fresh Ubuntu 17.10 Server (wanted to migrate to a new ssd disk for a while and broken ubuntu upgrade gave me a good reason to start from scratch). This new ubuntu has g++ 7.2.0, which may explain some of the issues reported below.

=> I have a workstation suspended with the same system and compiler.

I compiled the code (kea trac5525, premium trac5525) without any problems.

The unit-tests in kea passed, but those in premium failed miserably:

$ export KEA_LOGGER_DESTINATION=stdout

=> I use this too with this test because it gives the name of any missing symbols in dlopen.

$ cd premium/src/hooks/dhcp/radius/
$ make check
[==========] Running 2 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 2 tests from LibLoadTest?

=> according to the name it is the libloadtests (using the DSO when tests uses the convenience archive).

[ RUN ] LibLoadTest?.validLoad
2018-03-30 17:48:39.323 DEBUG [kea.hooks/8001] HOOKS_LIBRARY_LOADING loading hooks library /home/thomson/devel/kea-premium/premium/src/hooks/dhcp/radius/.libs/libdhcp_radius.so
2018-03-30 17:48:39.330 DEBUG [kea.hooks/8001] HOOKS_LIBRARY_VERSION hooks library /home/thomson/devel/kea-premium/premium/src/hooks/dhcp/radius/.libs/libdhcp_radius.so reports its version as 5
2018-03-30 17:48:39.330 ERROR [kea.hooks/8001] HOOKS_LOAD_FRAMEWORK_EXCEPTION 'load' function in hook library /home/thomson/devel/kea-premium/premium/src/hooks/dhcp/radius/.libs/libdhcp_radius.so threw an exception: reason attempt to access logging function before logging has been initialized
2018-03-30 17:48:39.330 DEBUG [kea.hooks/8001] HOOKS_LIBRARY_UNLOADING unloading library /home/thomson/devel/kea-premium/premium/src/hooks/dhcp/radius/.libs/libdhcp_radius.so
2018-03-30 17:48:39.330 ERROR [kea.hooks/8001] HOOKS_UNLOAD_FRAMEWORK_EXCEPTION 'unload' function in hook library /home/thomson/devel/kea-premium/premium/src/hooks/dhcp/radius/.libs/libdhcp_radius.so threw an exception, reason attempt to access logging function before logging has been initialized
load_unload_unittests.cc:50: Failure
Value of: HooksManager::loadLibraries(libraries_)

Actual: false

Expected: true
}}}

=> your system is crazy: there is a initLogger in run_unittests.cc to not get that!!!

I tried to run other hook libraries and flex-id, host_cmds, and subnet_cmds all worked.

=> flex-id and others have no equivalent tests: they only have the "tests" version (even
if flex-id fails to link the convenience archive, I opened a Kea 1.4-final ticket with
the one line fix for this).

The only hook that failed was legal_log with the following error:

=> legal_log has a similar test which crashes on Ubuntu with an out-of-bound vector access
in the callout core code (details with a proposed fix are in 5577).

[ RUN      ] LegalLibLoadTest.validLoad
2018-03-30 17:50:50.143 DEBUG [kea.hooks/10031] HOOKS_LIBRARY_LOADING loading hooks library /home/thomson/devel/kea-premium/premium/src/hooks/dhcp/legal_log/.libs/libdhcp_legal_log.so
2018-03-30 17:50:50.148 ERROR [kea.hooks/10031] HOOKS_OPEN_ERROR failed to open hook library /home/thomson/devel/kea-premium/premium/src/hooks/dhcp/legal_log/.libs/libdhcp_legal_log.so: /home/thomson/devel/kea-premium/premium/src/hooks/dhcp/legal_log/.libs/libdhcp_legal_log.so: undefined symbol: _ZN3isc4dhcp6CfgMgr8instanceEv
load_unload_unittests.cc:119: Failure
Value of: HooksManager::loadLibraries(libraries)
  Actual: false
Expected: true

=> you are lucky: on my Ubuntu it crashed. Note the legal_log in trac5525 is from the version
when the branch was created and at least one bug was fixed in it (cf the merge logs of trac5566).

After adding libdhcpsrv to the dependencies (commited, please pull), it started failing load with the same error as radius:

[==========] Running 2 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 2 tests from LegalLibLoadTest
[ RUN      ] LegalLibLoadTest.validLoad
2018-03-30 17:52:28.470 DEBUG [kea.hooks/12253] HOOKS_LIBRARY_LOADING loading hooks library /home/thomson/devel/kea-premium/premium/src/hooks/dhcp/legal_log/.libs/libdhcp_legal_log.so
2018-03-30 17:52:28.479 DEBUG [kea.hooks/12253] HOOKS_LIBRARY_VERSION hooks library /home/thomson/devel/kea-premium/premium/src/hooks/dhcp/legal_log/.libs/libdhcp_legal_log.so reports its version as 5
2018-03-30 17:52:28.480 ERROR [kea.hooks/12253] HOOKS_LOAD_FRAMEWORK_EXCEPTION 'load' function in hook library /home/thomson/devel/kea-premium/premium/src/hooks/dhcp/legal_log/.libs/libdhcp_legal_log.so threw an exception: reason attempt to access logging function before logging has been initialized
2018-03-30 17:52:28.480 DEBUG [kea.hooks/12253] HOOKS_LIBRARY_UNLOADING unloading library /home/thomson/devel/kea-premium/premium/src/hooks/dhcp/legal_log/.libs/libdhcp_legal_log.so
2018-03-30 17:52:28.480 DEBUG [kea.hooks/12253] HOOKS_UNLOAD_SUCCESS 'unload' function in hook library /home/thomson/devel/kea-premium/premium/src/hooks/dhcp/legal_log/.libs/libdhcp_legal_log.so returned success
2018-03-30 17:52:28.480 INFO  [kea.hooks/12253] HOOKS_LIBRARY_UNLOADED hooks library /home/thomson/devel/kea-premium/premium/src/hooks/dhcp/legal_log/.libs/libdhcp_legal_log.so successfully unloaded
load_unload_unittests.cc:119: Failure
Value of: HooksManager::loadLibraries(libraries)
  Actual: false
Expected: true
load_unload_unittests.cc:127: Failure
Value of: fileExists(expected_filename)
  Actual: false
Expected: true
load_unload_unittests.cc:134: Failure
Value of: fileExists(expected_filename)
  Actual: false
Expected: true
[  FAILED  ] LegalLibLoadTest.validLoad (13 ms)

I do not understand what is the reason of this. The liblog library is correctly listed in the dependencies.

=> yes there is no obvious reason. The good thing is the problem is not in trac5525
(not a surprise because of course I copied the libloadtests code). I'll refresh my Ubuntu 17.10,
add the FreeRADIUS client and investigate a bit. BTW can you pass the trac5577 patch in
the case it is a side effect of the 5577 bug?

There is one failing test in radius/tests:

=> I don't get it. I fact I believed rc_getport always returns PW_ACCT_UDP_PORT but
it is not true if /etc/services defines radacct (false on macOS, true on Ubuntu).
I'll change the code to return a fixed value with a comment saving it is better
but system dependent to call rc_getport.

> Expected:
> {
>   "attributes": [ ],
>   "servers": [
> ...

to hack the test code to create the diff was a good investment...

Note I did a small change in the core code so Kea trac5525 should be reviewed too.

I pushed two small changes to premium trac5525. My Ubuntu VM is nearly ready:
I'll see if I can reproduce (and fix) problems you got...

comment:8 Changed 6 months ago by fdupont

Tried on my Ubuntu: got a crash (same basic cause than for legal log, cf #5577) but with a fix in callout code it works well.

comment:9 follow-up: Changed 6 months ago by tomek

  • Owner changed from tomek to fdupont

I managed to figure out what the problem with unitialized logging error was.
To load the tests into gdb I edited the makefile to link the unittests statically
and then forgot about that change. I modified the Makefile itself, so git diff
didn't show any changes.

Anyway, the code now builds and unit-tests pass on Ubuntu 17.10 x64.

I've made some small changes. Please pull trac5525.

Here are my comments for the premium code.

radius.h

There should be a better explanation what the RadiusService class is for.

I don't think the enabled_ flag is necessary. If you don't want to use
Radius, just don't load the library.

radius_attribute.cc
constructors call check(), but do nothing if it returns false.

The code will segfault if Attribute(avpair) is called with NULL value.

I think the PW_TYPE_IPV6PREFIX in Attribute::pack() is incorrect.
I looked at RFC3162, Section 2.3 and there is one extra field called
reserved.

radius_attribute.h
The CfgAttributes should better explain what it is for. I've
updated the description. Please pull.

The pda_ field should have more meaningful name.

To get name, there's getName. To get type, there's getType. Following
that convention, the method to get avpair should be getAvpair, not
avpair.

Is there a good rationale why avpairs() return naked pointer?
It would be better to return shared_ptr.

There are constant values uses in many places, especially 33 and 256.
They should be converted to constants and commented on where they
came from.

radius_callout.cc
The name intern is misleading. It should be called parseConfig or similar.
There should not be a space between the doxygen comment and the function.

radius_utils.h
Why does pop0 strips 2 leading zeros from the duid? There should be
some explanation why only one 0 is stripped from client-id and two 0s
from duid.

radius_parsers.h
There should be an enum defined for srv_type. That should be used in
every place that uses int srv_type now.

radius_parsers.cc
The RadiusConfigParser::parse should not set anything up in
Radius library. This will be a problem when running config-test
command. This parser should simply set the structures and
separate method should apply them using rc_add_*.

Wouldn't it be simpler to pass const ElementPtr& to
RadiusAttributeParser::parse? You could then omit the
const_pointer_cast...

attribute_unittests.cc
The comment starting in line 6 doesn't seem correct. It looks
like a copy paste error.

AttributeTest.fromAVP - the avp pointer is never released.
The avp should probably be a scoped_ptr.

The commented out code in line 163 is commented out. I was curious
and uncommented it and found out there seems to be a problem.
The strvalue contains actually a long (10 lines) text:

VALUE		Framed-Protocol		PPP			1
VALUE		Framed-Protocol		SLIP			2
VALUE		Framed-Protocol		ARAP			3
VALUE		Framed-Protocol		GANDALF-SLMLP		4
VALUE		Framed-Protocol		XYLOGICS-IPX-SLIP	5
VALUE		Framed-Protocol		X75			6

#	Framed Routing Values

VALUE		Framed-R

Which doesn't seem to be the correct value. Isn't it supposed to be set
to ""?

I see the same commented out check in line 185.

The checks around lines 221 to 240 should check more cases for ipv6 prefixes:
at least /0 and /128.

fromAVP, parse, toAVP, fill tests should be split into many small tests.

There should not be any #if 0 sections. Please move the code between
lines 771 and 795 to separate test and mark it as disabled.

AttributesTest.append was missing a description. Added. Please
pull and review.

The commented out code in lines 880 and 883 should be removed.

comment:10 in reply to: ↑ 9 ; follow-up: Changed 6 months ago by fdupont

Replying to tomek:

I've made some small changes. Please pull trac5525.

=> pulledl and looked at them.

Here are my comments for the premium code.

radius.h

There should be a better explanation what the RadiusService class is for.

=> done.

I don't think the enabled_ flag is necessary. If you don't want to use
Radius, just don't load the library.

=> first the enabling condition (have a server in the FRC library) is complex,
second for tests it is nice to have an easy way to force enabling.
So if nothing is really necessary it is very useful.

radius_attribute.cc
constructors call check(), but do nothing if it returns false.

=> not a problem as it is not a critical condition nor a condition
one can get in production. Do you want an explicit cast to ignore
the returned value?

The code will segfault if Attribute(avpair) is called with NULL value.

=> it does not make sense to call it with a NULL value and to raise
an exception is not really better. Do you what an assert?

I think the PW_TYPE_IPV6PREFIX in Attribute::pack() is incorrect.
I looked at RFC3162, Section 2.3 and there is one extra field called
reserved.

=> I'll remove pack/unpack as they are useless (and unused).

The pda_ field should have more meaningful name.

=> it is the name in the FRC library (without the underscore as the library
is in C) so changing the name you loose the connection.

To get name, there's getName. To get type, there's getType. Following
that convention, the method to get avpair should be getAvpair, not
avpair.

=> I used a "constructor" like name.

Is there a good rationale why avpairs() return naked pointer?
It would be better to return shared_ptr.

=> FRC library is in C so avpairs are C structures (no constructor
or destructor, and of course plain C pointers in chaining).
It does not make sense at all to use shared pointer without
rewriting the whole FRC library in C++ first.

There are constant values uses in many places, especially 33 and 256.
They should be converted to constants and commented on where they
came from.

=> done. They are the buffer sizes for name and value in avpair.c code.

radius_callout.cc
The name intern is misleading. It should be called parseConfig or similar.

=> parseConfig is not better: the function translates parameters to an element
map so it does not parse the config just changes its representation...

radius_utils.h
Why does pop0 strips 2 leading zeros from the duid? There should be
some explanation why only one 0 is stripped from client-id and two 0s
from duid.

=> DHCPv4 vs DHCPv6 (and RFC 4361 making things more confusing
it is one 0 in DHCPv4 client-id (which does not exist for DHCPv6) and
two 0s for DUID so DHCPv6 or RFC 4361).

radius_parsers.h
There should be an enum defined for srv_type. That should be used in
every place that uses int srv_type now.

=> no, FRC library does not use an enum (even they exists in C) so
it simply breaks compatibility.

radius_parsers.cc
The RadiusConfigParser::parse should not set anything up in
Radius library.

=> this is not possible because the dictionary must be loaded.

This will be a problem when running config-test
command. This parser should simply set the structures and
separate method should apply them using rc_add_*.

=> the "set the structures" is not compatible with the way
the FRC library handles its config. Either you postpone the
config parsing or you integrate FRC library config with it.

Wouldn't it be simpler to pass const ElementPtr& to
RadiusAttributeParser::parse? You could then omit the
const_pointer_cast...

=> I'll try: if it is called only from a list it should be possible.

attribute_unittests.cc
The comment starting in line 6 doesn't seem correct. It looks
like a copy paste error.

=> oops.

AttributeTest.fromAVP - the avp pointer is never released.
The avp should probably be a scoped_ptr.

=> not a scope_ptr which does not work with C but a call
to rc_avpair_free().

The commented out code in line 163 is commented out.

=> yes, the FRC library does not correctly initialize it.

Which doesn't seem to be the correct value. Isn't it supposed to be set
to ""?

=> yes but the FRC library does a malloc without a memset/bzero.

I see the same commented out check in line 185.

=> same cause, same effect.

The checks around lines 221 to 240 should check more cases for ipv6 prefixes:
at least /0 and /128.

=> it won't change something: the idea is to check if the FRC IPv6 prefix
is correctly handled from/to C avpair and C++ attribute and one
test is enough (there are no conner cases in FRC code for this).

fromAVP, parse, toAVP, fill tests should be split into many small tests.

=> not sure it will improve things but my editor should be able to do
that with a good macro.

There should not be any #if 0 sections. Please move the code between
lines 771 and 795 to separate test and mark it as disabled.

=> IMHO it is better to fix the bug and there is a ticket about it
as here it just postpones a test but someone shall get it in production
soon or late. I propose to add the reference of the ticket with a @todo.

The commented out code in lines 880 and 883 should be removed.

=> they are to help. IMHO you don't like the way they are: where I should
move them?

comment:11 Changed 5 months ago by fdupont

  • Owner changed from fdupont to tomek

Fixed things which were not bound to FRC library. BTW it seems the ticket about unicode escapes was not created?

comment:12 in reply to: ↑ 10 ; follow-up: Changed 5 months ago by tomek

  • Owner changed from tomek to fdupont

Replying to fdupont:

Replying to tomek:

I don't think the enabled_ flag is necessary. If you don't want to use
Radius, just don't load the library.

=> first the enabling condition (have a server in the FRC library) is complex,
second for tests it is nice to have an easy way to force enabling.
So if nothing is really necessary it is very useful.

Ok. Added a note that explains the reasons.

radius_attribute.cc
constructors call check(), but do nothing if it returns false.

=> not a problem as it is not a critical condition nor a condition
one can get in production. Do you want an explicit cast to ignore
the returned value?

No, I want the code to throw.

The code will segfault if Attribute(avpair) is called with NULL value.

=> it does not make sense to call it with a NULL value and to raise
an exception is not really better. Do you what an assert?

Of course it doesn't make sense. The point is to write a safer code that
will not segfault even if you try doing stupid things.

The pda_ field should have more meaningful name.

=> it is the name in the FRC library (without the underscore as the library
is in C) so changing the name you loose the connection.

Thanks for adding the explanation.

Is there a good rationale why avpairs() return naked pointer?
It would be better to return shared_ptr.

=> FRC library is in C so avpairs are C structures (no constructor
or destructor, and of course plain C pointers in chaining).
It does not make sense at all to use shared pointer without
rewriting the whole FRC library in C++ first.

So it should return a wrapper class that calls rc_avpair_free() in
its destructor. But this is a safety improvement, so we can live
without it for a while.

=> done. They are the buffer sizes for name and value in avpair.c code.

Thanks.

radius_callout.cc
The name intern is misleading. It should be called parseConfig or similar.

=> parseConfig is not better: the function translates parameters to an element
map so it does not parse the config just changes its representation...

thanks for renaming.

radius_parsers.h
There should be an enum defined for srv_type. That should be used in
every place that uses int srv_type now.

=> no, FRC library does not use an enum (even they exists in C) so
it simply breaks compatibility.

Fair enough. Added some pointers in the comments.

radius_parsers.cc
The RadiusConfigParser::parse should not set anything up in
Radius library.

=> this is not possible because the dictionary must be loaded.

This will be a problem when running config-test
command. This parser should simply set the structures and
separate method should apply them using rc_add_*.

=> the "set the structures" is not compatible with the way
the FRC library handles its config. Either you postpone the
config parsing or you integrate FRC library config with it.

It's doable and rather easy to do. But to not hold this work
back, I've added

=> not a scope_ptr which does not work with C but a call
to rc_avpair_free().

See my comment above about a wrapper class. But since this
is a small memory leak only if the test fails, that's ok.
If the premium code was covered by Coverity Scan, it would have
complained about this.

The commented out code in line 163 is commented out.

=> yes, the FRC library does not correctly initialize it.

There should be a comment explaining this.

There should not be any #if 0 sections. Please move the code between
lines 771 and 795 to separate test and mark it as disabled.

=> IMHO it is better to fix the bug and there is a ticket about it
as here it just postpones a test but someone shall get it in production
soon or late. I propose to add the reference of the ticket with a @todo.

Yes, good suggestion.

I've tweaked some of the comments with info from your responses.
Please pull and review. If you're ok with the changes, please merge.

Please create the ticket for unicode responses, change the #if 0 code
in attribute_unittests.cc into a disabled test and reference the ticket.
Don't work on that new ticket, though. As far as we can tell, our customer
is not using unicode, so we can postpone it for later.

This change requires a changelog entry in premium/ChangeLog. Here's my proposal:

2x.	[func]		fdupont
	The Radius library is now able to parse its configuration
	parameters.
	(Trac #5525, git tbd)

If you agree with my suggestions above, please do them and merge the code.

Thanks for writing this code.

comment:13 in reply to: ↑ 12 Changed 5 months ago by fdupont

Replying to tomek:

radius_attribute.cc
constructors call check(), but do nothing if it returns false.

=> not a problem as it is not a critical condition nor a condition
one can get in production. Do you want an explicit cast to ignore
the returned value?

No, I want the code to throw.

=> I am against this because it introduces a hard dependency
on the dictionary which is an external element. I am adding
something in the Radius document leaving it as an open question.

Is there a good rationale why avpairs() return naked pointer?
It would be better to return shared_ptr.

=> FRC library is in C so avpairs are C structures (no constructor
or destructor, and of course plain C pointers in chaining).
It does not make sense at all to use shared pointer without
rewriting the whole FRC library in C++ first.

So it should return a wrapper class that calls rc_avpair_free() in
its destructor. But this is a safety improvement, so we can live
without it for a while.

=> A/V pair pointers are used to chain them so a wrapper won't work.
The only possible thing is simply to rewrite the whole library...

=> not a scope_ptr which does not work with C but a call
to rc_avpair_free().

See my comment above about a wrapper class. But since this
is a small memory leak only if the test fails, that's ok.
If the premium code was covered by Coverity Scan, it would have
complained about this.

=> coverity should complain about many things in the FRC library too...
BTW I forgot to add some rc_avpair_free() when I split tests so
now you have one free per new (fixed after your review).

I've tweaked some of the comments with info from your responses.
Please pull and review. If you're ok with the changes, please merge.

=> I fixed rc_avpair_free too.

Please create the ticket for unicode responses, change the #if 0 code
in attribute_unittests.cc into a disabled test and reference the ticket.
Don't work on that new ticket, though. As far as we can tell, our customer
is not using unicode, so we can postpone it for later.

=> unfortunately the unicode escapes includes control ASCII characters
i.e. unicode is just the encoding...

comment:14 Changed 5 months ago by fdupont

  • Resolution set to complete
  • Status changed from reviewing to closed

Design updated and #5582 created. Merged. Closing.

Note: See TracTickets for help on using tickets.