Make fr_radius_encode_pair() a one-line wrapper around a dbuff version. #3463

jejones3141 · 2020-05-20T15:49:00Z

No description provided.

arr2036 · 2020-05-20T15:58:29Z

I'll hold off merging this until the rest of the encoder elements are done. Maybe use this PR to push up commits for each area of the encoder, and then squash it all down at the end.

jejones3141 · 2020-05-20T16:18:39Z

Will do; thanks.

…

On Wed, May 20, 2020 at 10:58 AM Arran Cudbard-Bell < ***@***.***> wrote: I'll hold off merging this until the rest of the encoder elements are done. Maybe use this PR to push up commits for each area of the encoder, and then squash it all down at the end. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3463 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABV2HFCLWFF2IEHQVPFNIMTRSP43LANCNFSM4NGBYCHA> .

lgtm-com · 2020-05-22T14:19:20Z

This pull request introduces 1 alert when merging 58006cb into 12501cf - view on LGTM.com

new alerts:

1 for Multiplication result converted to larger type

lgtm-com · 2020-05-22T14:59:03Z

This pull request introduces 1 alert when merging 3f03237 into a02e21b - view on LGTM.com

new alerts:

1 for Multiplication result converted to larger type

arr2036 · 2020-05-22T15:15:08Z

@jejones3141 looks like you tripped LGTM CI. Can you take a look and see what it's complaining about.

jejones3141 · 2020-05-22T15:19:48Z

Found it, and will send up a correction. (Also corrected a doxygen mistake.)

…

On Fri, May 22, 2020 at 10:15 AM Arran Cudbard-Bell < ***@***.***> wrote: @jejones3141 <https://github.com/jejones3141> looks like you tripped LGTM CI. Can you take a look and see what it's complaining about. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3463 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABV2HFAHG7JHOVZ7GZIDJS3RS2JI3ANCNFSM4NGBYCHA> .

arr2036 · 2020-05-28T17:21:05Z

src/protocols/radius/encode.c

 				       fr_da_stack_t *da_stack, unsigned int depth,
 				       fr_cursor_t *cursor, void *encoder_ctx)
 {
 	ssize_t			slen;
-	uint8_t			*p = out;
+	size_t			init_remaining = fr_dbuff_remaining(dbuff);


I added FR_DBUFF_MAX_NO_ADVANCE. Maybe it'd be better to use that here to initialise a fr_dbuff_t on the stack, then we don't have to initialise a new dbuff for every call for encode_tlv_hdr[_internal].

You can then return at the end with:

return fr_dbuff_advance(dbuff, fr_dbuff_used(tlv))

Thank you. I'll look for places I can use FR_DBUFF_MAX_NO_ADVANCE to advantage.

It's been 4+ weeks. Can these commits be updated to use the new API?

The idea behind DBUFF was to avoid these manual length calculations.

Yes, they can, once FR_DBUFF_MAX_NO_ADVANCE() gives you a dbuff with matching start and p.

arr2036 · 2020-05-28T17:23:45Z

src/protocols/radius/encode.c

+ * Fully migrating away from the original encode_value() will require a revision of
+ * fr_struct_to_network() and revising the fr_encode_value_t type.
+ */
+static ssize_t encode_value_dbuff(fr_dbuff_t *dbuff,


I don't think encode_value is used outside of this compilation unit, so there's no reason to wrap it?

Sorry; I thought replying to the email would show up here. The ptr/len version of encode_value() has to stay around because it's passed as a parameter to fr_struct_to_network(), which others use (and which, because it takes function pointer parameters, isn't easy to wrap); I wrote the one layer around it rather than doing it in each caller in src/protocols/radius/encode.c.

jejones3141 · 2020-05-28T19:49:35Z

I wrapped it so that dbuff-using functions wouldn't have to switch back to ptr/len to call encode_value(). encode_value() itself has to keep using ptr/len for now because it is passed to fr_struct_to_network() which expects that--one can't write a layer around it as one can for fr_value_box_to_network(). It will have to wait until all callers of fr_struct_to_network() use dbuffs.

…

On Thu, May 28, 2020 at 12:24 PM Arran Cudbard-Bell < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/protocols/radius/encode.c <#3463 (comment)> : > @@ -710,12 +709,46 @@ static ssize_t encode_value(uint8_t *out, size_t outlen, return out_p - out; } -static ssize_t attr_shift(uint8_t const *start, uint8_t const *end, +/** Encodes the data portion of an attribute into a dbuff + * + * Fully migrating away from the original encode_value() will require a revision of + * fr_struct_to_network() and revising the fr_encode_value_t type. + */ +static ssize_t encode_value_dbuff(fr_dbuff_t *dbuff, I don't think encode_value is used outside of this compilation unit, so there's no reason to wrap it? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3463 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABV2HFBFSQR5LRKK4ILXP3LRT2M3BANCNFSM4NGBYCHA> .

jejones3141 · 2020-05-28T19:52:54Z

Thank you. I will look into where I could use FR_DBUFF_MAX_NO_ADVANCE(), and make sure that line in src/protocols/radius/encode.c is right.

…

On Thu, May 28, 2020 at 12:21 PM Arran Cudbard-Bell < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/protocols/radius/encode.c <#3463 (comment)> : > fr_da_stack_t *da_stack, unsigned int depth, fr_cursor_t *cursor, void *encoder_ctx) { ssize_t slen; - uint8_t *p = out; + size_t init_remaining = fr_dbuff_remaining(dbuff); I added FR_DBUFF_MAX_NO_ADVANCE. Maybe it'd be better to use that here to initialise a fr_dbuff_t on the stack, then we don't have to initialise a new dbuff for every call for encode_tlv_hdr[_internal]. You can then return at the end with: return fr_dbuff_advance(dbuff, fr_dbuff_used(tlv)) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3463 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABV2HFBVQQJ2I2G4YGBQLWDRT2MQ7ANCNFSM4NGBYCHA> .

alandekok · 2020-07-02T17:33:49Z

src/protocols/radius/encode.c

+	FR_DBUFF_RETURN(encode_vsa_hdr, _dbuff, _da_stack, _depth, _cursor, _ctx)
+#define ENCODE_TLV_HDR_RETURN(_dbuff, _da_stack, _depth, _cursor, _ctx) \
+	FR_DBUFF_RETURN(encode_tlv_hdr, _dbuff, _da_stack, _depth, _cursor, _ctx)
+#define ENCODE_EXTENDED_HDR_RETURN(_dbuff, _da_stack, _depth, _cursor, _ctx) \


If these macros are only used once, there's no need to have them as macros.

alandekok · 2020-07-02T17:35:36Z

src/protocols/radius/encode.c

@@ -303,14 +319,16 @@ static ssize_t encode_tlv_hdr_internal(uint8_t *out, size_t outlen,
 		vp = fr_cursor_current(cursor);
 	}

-	return p - out;
+	return init_remaining - fr_dbuff_remaining(dbuff);


This should just be return fr_dbuff_used(dbuff)

here, and other places.

fr_dbuff_used(dbuff) includes bytes already written to dbuff, so just doing that doesn't work. What is needed is no-advance dbuffs having start set to the parent's p (or more accurately the same thing as their p is set to; the FR_DBUFF_NO_ADVANCE_RESERVE() macro has an edge case); actually, I'm not sure why that's not done for all children. Then you can write

fr_dbuff_t work_dbuff = FR_DBUFF_NO_ADVANCE(dbuff); /* Do what's needed using work_dbuff */ return fr_dbuff_advance(dbuff, fr_dbuff_used(&work_dbuff));

Yeah, that's the pattern which should be used everywhere.

alandekok · 2020-07-02T17:36:52Z

src/protocols/radius/encode.c

 			      fr_da_stack_t *da_stack, unsigned int depth,
 			      fr_cursor_t *cursor, void *encoder_ctx)
 {
-	ssize_t			slen;
+	ssize_t	slen;
+	uint8_t	*hdr_field = dbuff->p;


this is a nitpick: we typically would use attr here for radius. We don't generally use field suffixes, as they're pretty much redundant.

Here, and other places in the commit.

alandekok · 2020-07-02T17:58:44Z

src/protocols/radius/encode.c

@@ -724,7 +772,7 @@ static ssize_t attr_shift(uint8_t const *start, uint8_t const *end,
 	 */

 	while (check_len > (255 - hdr_len)) {
-		total += hdr_len;
+		new_hdr_count++;


I'm not sure why total was changed to new_hdr_count and then a later multiplication. It should just use total as before

Agreed; I will change it back.

alandekok · 2020-07-02T18:01:34Z

src/protocols/radius/encode.c

@@ -735,7 +783,7 @@ static ssize_t attr_shift(uint8_t const *start, uint8_t const *end,
 	 *	"encode_value" function to take into account the header
 	 *	lengths.
 	 */
-	if ((ptr + ptr[1] + total) > end) return (ptr + ptr[1]) - start;
+	if (fr_dbuff_advance(dbuff, new_hdr_count * hdr_len) < 0) return ptr[1];


returning ptr[1] changes the meaning of this return code. It should return the number of bytes necessary to encode this data.

The previous code returned the total amount of data encoded since start. this change just returns the total amount of data in this particular attribute. So at the minimum, returning ptr[1] is different from what it was before.

alandekok · 2020-07-02T18:04:11Z

src/protocols/radius/encode.c

@@ -1387,10 +1419,10 @@ ssize_t fr_radius_encode_pair(uint8_t *out, size_t outlen, fr_cursor_t *cursor,
 			 *	using a different scheme than the "long
 			 *	extended" one.
 			 */
-			ret = encode_concat(out, outlen, &da_stack, 0, cursor, encoder_ctx);
+			ENCODE_CONCAT_RETURN(dbuff, &da_stack, 0, cursor, encoder_ctx);


It's not clear why macros are useful here.

The conversion is not complete; encode_value() must wait until fr_struct_to_network(), which radius, dhcpv6, and arp use, can be converted to use dbuff.

jejones3141 force-pushed the radius_encode_dbuff branch from e48ffd5 to 58006cb Compare May 22, 2020 13:58

jejones3141 force-pushed the radius_encode_dbuff branch from 58006cb to 3f03237 Compare May 22, 2020 14:37

jejones3141 force-pushed the radius_encode_dbuff branch from 9e87cdc to e4f83fb Compare May 22, 2020 15:24

arr2036 reviewed May 28, 2020

View reviewed changes

jejones3141 force-pushed the radius_encode_dbuff branch 2 times, most recently from f1587e9 to a71fdd1 Compare June 11, 2020 21:04

jejones3141 force-pushed the radius_encode_dbuff branch from a71fdd1 to b83b323 Compare June 18, 2020 17:50

alandekok reviewed Jul 2, 2020

View reviewed changes

jejones3141 force-pushed the radius_encode_dbuff branch 2 times, most recently from d8ea999 to 66725a2 Compare July 6, 2020 21:21

Convert radius encode functions to use dbuff

687886b

The conversion is not complete; encode_value() must wait until fr_struct_to_network(), which radius, dhcpv6, and arp use, can be converted to use dbuff.

jejones3141 force-pushed the radius_encode_dbuff branch from 66725a2 to 687886b Compare July 9, 2020 02:28

arr2036 merged commit c376be5 into FreeRADIUS:master Jul 15, 2020

jejones3141 deleted the radius_encode_dbuff branch October 5, 2020 16:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make fr_radius_encode_pair() a one-line wrapper around a dbuff version. #3463

Make fr_radius_encode_pair() a one-line wrapper around a dbuff version. #3463

jejones3141 commented May 20, 2020

arr2036 commented May 20, 2020

jejones3141 commented May 20, 2020 via email

lgtm-com bot commented May 22, 2020

lgtm-com bot commented May 22, 2020

arr2036 commented May 22, 2020

jejones3141 commented May 22, 2020 via email

arr2036 May 28, 2020 •

edited

jejones3141 May 28, 2020

alandekok Jul 2, 2020

jejones3141 Jul 2, 2020

arr2036 May 28, 2020

jejones3141 May 28, 2020

jejones3141 commented May 28, 2020 via email

jejones3141 commented May 28, 2020 via email

alandekok Jul 2, 2020

alandekok Jul 2, 2020

jejones3141 Jul 2, 2020

alandekok Jul 2, 2020

alandekok Jul 2, 2020 •

edited

alandekok Jul 2, 2020

jejones3141 Jul 2, 2020

alandekok Jul 2, 2020

alandekok Jul 2, 2020

Make fr_radius_encode_pair() a one-line wrapper around a dbuff version. #3463

Make fr_radius_encode_pair() a one-line wrapper around a dbuff version. #3463

Conversation

jejones3141 commented May 20, 2020

arr2036 commented May 20, 2020

jejones3141 commented May 20, 2020 via email

lgtm-com bot commented May 22, 2020

lgtm-com bot commented May 22, 2020

arr2036 commented May 22, 2020

jejones3141 commented May 22, 2020 via email

arr2036 May 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jejones3141 commented May 28, 2020 via email

jejones3141 commented May 28, 2020 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alandekok Jul 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arr2036 May 28, 2020 •

edited

alandekok Jul 2, 2020 •

edited