feature/implement-PID_PRODUCT_VERSION

[trittsv]

This PR implements PID_PRODUCT_VERSION to be able to see the version on wireshark.

@eboasson could you have a look?

[eboasson]

Thank you @trittsv ! I think it makes sense to put proper version information in the discovery data, but there are a few ways in which I think this can be improved upon.

I didn't dig deep enough in the sources of Cyclone (and of old versions) to be able to say exactly which parts of the old "junk" we need to keep, or whether it really would make more sense to keep a tweaked version of what is there today. A bit of both probably will be best.

I expect it needs another look for you and me before it is clear what we should do.

[eboasson]

The vendor-specific flags are defined below, e.g., line 349 where it defines DDSI_PID_ADLINK_PARTICIPANT_VERSION_INFO. 0x8000 is already defined in this file, inherited from OpenSplice but not used by Cyclone (see

cyclonedds/src/core/ddsi/src/ddsi_plist.c

Line 2110 in d8cef89

static const struct piddesc piddesc_eclipse[] = {

and the table below it: it doesn't occur in either). There are a lot of parameters there that it doesn't grok, come to think of it.

I suspect 0x8000 has never been used by Cyclone, so I think it can be reclaimed.

[eboasson]

I wouldn't add it to endpoint discovery: the version is constant for all endpoints in a participant, endpoint discovery only runs after participant discovery and participant discovery messages are periodic. So putting in participant discovery both ensures you eventually will find out, and avoids sending it for potentially thousands of endpoints.

[eboasson]

If you look a bit further down in the function can also see that it already puts something called "adlink version info" in. I am not sure the format is ideal, nor that we should keep it at all, but the name is irrelevant for compatibility and we could clean it up a bit.

For example, we could also redefine struct ddsi_adlink_participant_version_info as

typedef struct ddsi_cyclonedds_version
{
  uint8_t major;
  uint8_t minor;
  uint8_t release;
  uint8_t revision;
  uint32_t flags;
  uint32_t unused[3];
  char *internals;
} ddsi_cyclonedds_version_t;

It has proven useful to have a bunch of flags ready for use. The string can probably be replaced by something more useful, but if we really replace it by some other type (and not just change the contents) then we would need to change the parameter id to avoid upsetting "old" Cyclone versions.

OpenSplice doesn't interpret this if it comes from Cyclone, so we really have some freedom.

[eboasson]

Like with the endpoints, I wouldn't add it for the topics either. I think we should just clean up/improve version information in SPDP.

[eboasson]

Almost right! Xox2 means o times 2, so two octets. You really want four, which means Xox4 but that one doesn't exist. It could be added, but it would be equivalent to Xox2, Xox2, or even to Xo, Xo, Xo, Xo. I probably would go for the latter (out of laziness? who knows ...)

This would be a vendor-specific code, and so it would need to be moved to the vendor-specific code tables, though. And as I remarked before, I think I would first see if it wouldn't make sense to clean up the adlink_participant_info in a (sufficiently) backwards compatible manner.

[eboasson]

As the product version is only informational now, I don't think anything is to be gained by also burdening the serializer used for the DDS Security with it. The clear text SPDP message will be good enough for the purpose.

Note that we shouldn't even have a separate "DDS Security" serializer. It only exists because the in-memory representation in the spec'd plug-in interface is different, and the dice landed the wrong way when a choice had to be made between adding a second serializer or having only a single serializer and converting between in-memory representations. Given the security sensitivity of (in particular de-)serializers, we should get rid of the second one.

In short, I wouldn't add it here.

[eboasson]

Right now it is major, minor, "something" ("patch"?) and I don't know that having four fields won't make life more complicated. What I would find really useful is to also have a commit hash in there (or perhaps just the first few bytes to keep the size in check).