Re: [Linuxptp-devel] [PATCH v2 1/4] unicast: Add support to check if message was received from an entry in the unicast master table.

2022-03-21 Thread Keller, Jacob E 



> -Original Message-
> From: vincent.cheng...@renesas.com 
> Sent: Saturday, March 19, 2022 7:04 PM
> To: Keller, Jacob E 
> Cc: linuxptp-devel@lists.sourceforge.net
> Subject: Re: [Linuxptp-devel] [PATCH v2 1/4] unicast: Add support to check if
> message was received from an entry in the unicast master table.
> 
> >I'm ok with the name if Richard is, but thought I'd voice my bikeshed 
> >opinion.
> 
> Changed to unicast_client_msg_is_from_master_table_entry() in v3 patch.
> The unicast_client prefix is to keep with existing naming in the file.
> 
> Thanks,
> Vincent

Yep that seems reasonable to me.

Thanks,
Jake

___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

Re: [Linuxptp-devel] [PATCH v2 1/4] unicast: Add support to check if message was received from an entry in the unicast master table.

2022-03-19 Thread vincent.cheng...@renesas.com 
On Tue, Mar 15, 2022 at 06:45:50PM EDT, Keller, Jacob E wrote:

>> +
>> +int unicast_client_unicast_master_table_received(struct port *p, struct
>> ptp_message *m)
>> +{
>> +struct unicast_master_address *ucma;
>> +
>> +if (!unicast_client_enabled(p)) {
>> +return 0;
>> +}
>> +STAILQ_FOREACH(ucma, >unicast_master_table->addrs, list) {
>> +if (addreq(transport_type(p->trp), >address, 
>> >address)) {
>> +break;
>> +}
>> +}
>
>Does STALQ_FOREACH guarantee that ucma is NULL after exiting?
>
>For code clarity I'd rather have a separate variable set set to 1 when addreq 
>returns true. That is far easier to read, since not every for each iterator 
>works this way.
>
>> +return ucma ? 1 : 0;
>> +}

Thanks. Fixed in next v3 patch.

>> +
>> diff --git a/unicast_client.h b/unicast_client.h
>> index 16e291f..b24c4eb 100644
>> --- a/unicast_client.h
>> +++ b/unicast_client.h
>> @@ -82,4 +82,15 @@ void unicast_client_state_changed(struct port *p);
>>   */
>>  int unicast_client_timer(struct port *p);
>> 
>> +/**
>> + * Check whether a message was received from an entry in the unicast
>> + * master table.
>> + * @param p  The port in question.
>> + * @param m  The message in question.
>> + * @return   One (1) if the message is from an entry in the unicast
>> + *   master table, or zero otherwise.
>> + */
>> +int unicast_client_unicast_master_table_received(struct port *p,
>> + struct ptp_message *m);
>> +
>>  #endif
>
>The name of the function doesn't quite capture its meaning to me.
>
>Perhaps something like "unicast_message_is_from_master_table_entry"?
>
>I'm ok with the name if Richard is, but thought I'd voice my bikeshed opinion.

Changed to unicast_client_msg_is_from_master_table_entry() in v3 patch.
The unicast_client prefix is to keep with existing naming in the file.

Thanks,
Vincent


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

Re: [Linuxptp-devel] [PATCH v2 1/4] unicast: Add support to check if message was received from an entry in the unicast master table.

2022-03-15 Thread Keller, Jacob E 



> -Original Message-
> From: vincent.cheng...@renesas.com 
> Sent: Sunday, March 13, 2022 10:01 PM
> To: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [PATCH v2 1/4] unicast: Add support to check if
> message was received from an entry in the unicast master table.
> 
> From: Vincent Cheng 
> 
> Signed-off-by: Vincent Cheng 
> ---
>  unicast_client.c | 16 
>  unicast_client.h | 11 +++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/unicast_client.c b/unicast_client.c
> index 87d8471..4d6386e 100644
> --- a/unicast_client.c
> +++ b/unicast_client.c
> @@ -547,3 +547,19 @@ int unicast_client_timer(struct port *p)
>   unicast_client_set_tmo(p);
>   return err;
>  }
> +
> +int unicast_client_unicast_master_table_received(struct port *p, struct
> ptp_message *m)
> +{
> + struct unicast_master_address *ucma;
> +
> + if (!unicast_client_enabled(p)) {
> + return 0;
> + }
> + STAILQ_FOREACH(ucma, >unicast_master_table->addrs, list) {
> + if (addreq(transport_type(p->trp), >address, 
> >address)) {
> + break;
> + }
> + }

Does STALQ_FOREACH guarantee that ucma is NULL after exiting?

For code clarity I'd rather have a separate variable set set to 1 when addreq 
returns true. That is far easier to read, since not every for each iterator 
works this way.

> + return ucma ? 1 : 0;
> +}
> +
> diff --git a/unicast_client.h b/unicast_client.h
> index 16e291f..b24c4eb 100644
> --- a/unicast_client.h
> +++ b/unicast_client.h
> @@ -82,4 +82,15 @@ void unicast_client_state_changed(struct port *p);
>   */
>  int unicast_client_timer(struct port *p);
> 
> +/**
> + * Check whether a message was received from an entry in the unicast
> + * master table.
> + * @param p  The port in question.
> + * @param m  The message in question.
> + * @return   One (1) if the message is from an entry in the unicast
> + *   master table, or zero otherwise.
> + */
> +int unicast_client_unicast_master_table_received(struct port *p,
> +  struct ptp_message *m);
> +
>  #endif

The name of the function doesn't quite capture its meaning to me.

Perhaps something like "unicast_message_is_from_master_table_entry"?

I'm ok with the name if Richard is, but thought I'd voice my bikeshed opinion.

> --
> 2.34.1
> 
> 
> 
> ___
> Linuxptp-devel mailing list
> Linuxptp-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel