Dear Erez,
Dear All,
I'm sorry, but I tried to reply using my mobile.
I will prefer to give a better rationale.
Suppose to have the PortNumber_a=0x1234 and PortNumber_b=4512. On a
LittleEndian CPU, you will have the following memory streams:
a) 0x34 0x12
b) 0x12 0x45
Using a raw memory compare the a) will be erroneously evaluated great
than b).
Comparing, instead the PortNumber considering the HOST endianness you
will have the expected result.
The Network order is not meaningful because the struct has been already
translated to HOST's network order.
I'm sorry but I still think that we have bug here.
ciao
luigi
Il giorno gio, 17/11/2022 alle 18.04 +0000, Geva, Erez ha scritto:
> On Mon, 2022-11-14 at 14:25 +0100, Luigi Mantellini wrote:
> > The PortId is defined as a couple of ClockId (an 8-bytes opaque)
> > and
> > the PortNumber (UInterger16).
> > In order to correctly handle the PortId endianess, the comparison
> > has
> > been split in the ClockIds comparison (using memcmp) and PortNumber
> > comparison.
>
> You code do not solve any endianess issue.
> The clock ID is byte stream.
> And port number is 16 bits, it require converting from network order
> to
> host order, which you do not need here. Becuase port ID are already
> in
> the same endian here.
>
> As struct PortIdentity is defined as "PACKED", it should not have any
> pad that may cause the memory compare to fail. Of having different
> values in the pad.
>
>
> So memcmp and your "portid_cmp" should give the same result.
>
> I would suggest to replace the memcmp and "portid_cmp" with existing
> pid_eq(const struct PortIdentity *a, const struct PortIdentity *b)
>
>
> And debug and find the real reason you have the bug.
> And than send a real bug fix :-)
>
>
> Erez
>
>
>
>
> > ---
> > bmc.c | 17 ++++++++++++++---
> > 1 file changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/bmc.c b/bmc.c
> > index 6ac7aa0..ebc0789 100644
> > --- a/bmc.c
> > +++ b/bmc.c
> > @@ -21,6 +21,17 @@
> > #include "bmc.h"
> > #include "ds.h"
> >
> > +static int portid_cmp(struct PortIdentity *a, struct PortIdentity
> > *b)
> > +{
> > + int diff = memcmp(&a->clockIdentity, &b->clockIdentity,
> > sizeof(a->clockIdentity));
> > +
> > + if (diff == 0) {
> > + diff = a->portNumber - b->portNumber;
> > + }
> > +
> > + return diff;
> > +}
> > +
> > int dscmp2(struct dataset *a, struct dataset *b)
> > {
> > int diff;
> > @@ -35,7 +46,7 @@ int dscmp2(struct dataset *a, struct dataset *b)
> > * standard, since there is nothing we can do about it
> > anyway.
> > */
> > if (A < B) {
> > - diff = memcmp(&b->receiver, &b->sender, sizeof(b-
> > > receiver));
> > + diff = portid_cmp(&b->receiver, &b->sender);
> > if (diff < 0)
> > return A_BETTER;
> > if (diff > 0)
> > @@ -44,7 +55,7 @@ int dscmp2(struct dataset *a, struct dataset *b)
> > return 0;
> > }
> > if (A > B) {
> > - diff = memcmp(&a->receiver, &a->sender, sizeof(a-
> > > receiver));
> > + diff = portid_cmp(&a->receiver, &a->sender);
> > if (diff < 0)
> > return B_BETTER;
> > if (diff > 0)
> > @@ -53,7 +64,7 @@ int dscmp2(struct dataset *a, struct dataset *b)
> > return 0;
> > }
> >
> > - diff = memcmp(&a->sender, &b->sender, sizeof(a->sender));
> > + diff = portid_cmp(&a->sender, &b->sender);
> > if (diff < 0)
> > return A_BETTER_TOPO;
> > if (diff > 0)
>
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
