On 27 May 2024, at 16:42, Ilya Maximets wrote:
> On 5/27/24 13:01, Eelco Chaudron wrote:
>> Instead of casting time_t to uint32_t for the 0xffffffff comparison,
>> define a TIME_T_MAX and use it for both setting and comparison.
>>
>> Fixes: c72e245a0e2c ("Add InMon's sFlow Agent library to the build system.")
>> Signed-off-by: Eelco Chaudron <echau...@redhat.com>
>> --
>> Note that this checkpatch reports an 'Improper whitespace
>> around control block' error on this patch. But I did not
>> want to change the code style in this entire file.
>> ---
>> include/openvswitch/types.h | 3 +++
>> lib/sflow_receiver.c | 3 ++-
>> ofproto/ofproto-dpif-sflow.c | 2 +-
>> 3 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/openvswitch/types.h b/include/openvswitch/types.h
>> index 8c5ec94a6..bfeed752c 100644
>> --- a/include/openvswitch/types.h
>> +++ b/include/openvswitch/types.h
>> @@ -33,6 +33,9 @@ extern "C" {
>> #define OVS_FORCE
>> #endif
>>
>> +/* Maximum time_t value. */
>> +#define TIME_T_MAX (time_t)((1UL << ((sizeof(time_t) << 3) - 1)) - 1)
>
> We should multiply by CHAR_BITS instead of '<< 3'.
>
> This also assumes that time_t is an integer type, which is true for POSIX,
> but isn't generally true for C.
>
> And the openvswitch/types.h is probably not a good place for the define.
> This header is for OVS-specific types and structures that include those
> specific types. We shouldn't export stuff as generic as TIME_T_MAX.
>
>
> We may probably just replace sFlowRcvrTimeout with a uint32_t value and
> compare agains UIN32_MAX. It doesn't have to be time_t. It's just a tick
> counter that should not be too high. And we're not even using it anyway.
I did this at first, but the TIME_T_MAX was a smaller change. But with all your
feedback above, I’ll do the uint32_t change instead for the v2.
> Best regards, Ilya Maximets.
>
>> +
>> /* The ovs_be<N> types indicate that an object is in big-endian, not
>> * native-endian, byte order. They are otherwise equivalent to uint<N>_t.
>> */
>> typedef uint16_t OVS_BITWISE ovs_be16;
>> diff --git a/lib/sflow_receiver.c b/lib/sflow_receiver.c
>> index 4162518e3..b5b02c060 100644
>> --- a/lib/sflow_receiver.c
>> +++ b/lib/sflow_receiver.c
>> @@ -146,7 +146,8 @@ void sfl_receiver_tick(SFLReceiver *receiver)
>> // if there are any samples to send, flush them now
>> if(receiver->sampleCollector.numSamples > 0) sendSample(receiver);
>> // check the timeout
>> - if(receiver->sFlowRcvrTimeout && (u_int32_t)receiver->sFlowRcvrTimeout
>> != 0xFFFFFFFF) {
>> + if(receiver->sFlowRcvrTimeout
>> + && receiver->sFlowRcvrTimeout != TIME_T_MAX) {
>> // count down one tick and reset if we reach 0
>> if(--receiver->sFlowRcvrTimeout == 0) reset(receiver);
>> }
>> diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
>> index 4a68e9b94..08c16f6f8 100644
>> --- a/ofproto/ofproto-dpif-sflow.c
>> +++ b/ofproto/ofproto-dpif-sflow.c
>> @@ -808,7 +808,7 @@ dpif_sflow_set_options(struct dpif_sflow *ds,
>>
>> receiver = sfl_agent_addReceiver(ds->sflow_agent);
>> sfl_receiver_set_sFlowRcvrOwner(receiver, "Open vSwitch sFlow");
>> - sfl_receiver_set_sFlowRcvrTimeout(receiver, 0xffffffff);
>> + sfl_receiver_set_sFlowRcvrTimeout(receiver, TIME_T_MAX);
>>
>> /* Set the sampling_rate down in the datapath. */
>> ds->probability = MAX(1, UINT32_MAX / ds->options->sampling_rate);
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
