Re: [Linuxptp-devel] [PATCH 6/7] bmc: Added dscmp_alternate

[Richard Cochran]

On Fri, Apr 20, 2018 at 12:14:27PM +0200, Anders Selhammer wrote:
> @@ -81,7 +81,7 @@ int dscmp2(struct dataset *a, struct dataset *b)
>   return 0;
>  }
>  
> -int dscmp_ieee1588(struct dataset *a, struct dataset *b)
> +static int dscmp(struct dataset *a, struct dataset *b, enum bmca_type type)

Let's think about where this is headed...

> @@ -96,15 +96,18 @@ int dscmp_ieee1588(struct dataset *a, struct dataset *b)
>   }
>  
>   diff = memcmp(&a->identity, &b->identity, sizeof(a->identity));
> - if (!diff) {
> - return dscmp2(a, b);
> - }
>  
> - if (a->priority1 < b->priority1) {
> - return A_BETTER;
> - }
> - if (a->priority1 > b->priority1) {
> - return B_BETTER;
> + if (IEEE1588_BMCA == type) {
> + if (!diff) {
> + return dscmp2(a, b);
> + }
> +
> + if (a->priority1 < b->priority1) {
> + return A_BETTER;
> + }
> + if (a->priority1 > b->priority1) {
> + return B_BETTER;
> + }

How will the code look after the tenth different BMCA is published?

if (type == AAA || type == BBB) {
...
} else if (type != FFF && type > DDD) {
...
} else if (...) {
...
}

We want the code to match the flow charts in the published standards.
That is why I have placed the alternate function into its own C
function.  Also, that is why it makes sense to make an exception to
the coding style and dispense with the extra braces here.

Thanks,
Richard

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

Re: [Linuxptp-devel] [PATCH 6/7] bmc: Added dscmp_alternate

[Keller, Jacob E]

> -Original Message-
> From: Anders Selhammer [mailto:anders.selham...@est.tech]
> Sent: Friday, April 20, 2018 3:14 AM
> To: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [PATCH 6/7] bmc: Added dscmp_alternate
> 
>  /**
>   * BMC state decision algorithm.
>   * @param c  The local clock.
> @@ -54,6 +59,16 @@ enum port_state bmc_state_decision(struct clock *c, struct
> port *r,
>  int dscmp_ieee1588(struct dataset *a, struct dataset *b);
> 
>  /**
> + * Compare two data sets using the algorithm defined in ITU-T G.8275.1.
> + * @param a A dataset to compare.
> + * @param b A dataset to compare.
> + * @return An integer less than, equal to, or greater than zero
> + * if the dataset @a a is found, respectively, to be
> + * less than, to match, or be greater than @a b.
> + */
> +int dscmp_alternate(struct dataset *a, struct dataset *b);
> +

Would it make more sense if the function included something indicating the 
standard here? Alternate seems a bit vague.

> +/**
>   * Second part of the data set comparison algorithm, not for general
>   * public use.
>   */
> --
> 1.8.3.1
> 


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel