Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mdsn] handling of txt record and boolean attributes #1987

Closed
abtink opened this issue Sep 1, 2023 · 6 comments
Closed

[mdsn] handling of txt record and boolean attributes #1987

abtink opened this issue Sep 1, 2023 · 6 comments

Comments

@abtink
Copy link
Member

abtink commented Sep 1, 2023

I have been looking into the Mdns::Publisher related to TXT data record in published services.

Per RFC 6763, section 6.4,

"If there is no '=' in a DNS-SD TXT record string, then it is a boolean attribute, simply identified as being present, with no value.".

We can also have:

"Attribute present, with empty value (e.g., "PlugIns=" -- the server supports plugins, but none are presently installed)"

But the Publisher API won't allow us to define/distinguish such entries since we use:

    struct TxtEntry
    {
        std::string          mName;  ///< The name of the TXT entry.
        std::vector<uint8_t> mValue; ///< The value of the TXT entry.

And the EncodeTxtData() always inserts = between key and value when constructing the data.

In AdvertisingProxy::MakeTxtList() we construct array of TxtEntry using otDnsTxtEntryIterator and otDnsGetNextTxtEntry() to parse the encoded TXT data.

  • The otDnsGetNextTxtEntry() will indicate boolean attribute by setting mValue in otDnsTxtEntry to NULL
    • (in contrast with an empty value where mValue is non-null but mValueLength is zero).
  • I think with the current code we would incorrectly encode a "boolean attribute" as a empty value key "key=" format when advertising it on mDNS.

One ides is to change the Publisher APIs to directly get the encoded TXT data instead of a TxtEntry list.

  • For mDNSResponder we can directly use the encoded data (so avoid converting it from encoded format to TxtEntry array and back to encoded format)
  • For Avahi, it may be easier to convert from encoded TXT data to AvahiStringList.

@wgtdkp what do you think? do you see any issues with this?

The one thing I noticed in the code, is that we do SortTxtList and keep track of the sorted list in Registeration entries so that we can detect duplicate publish request. I think we can simplfit this and treat TXT data as data blob (seq of bytes) and compare the bytes directly (if there is a change to data blob, it is a change and need to updated/re-registered on mDNS).

@wgtdkp
Copy link
Member

wgtdkp commented Sep 1, 2023

The one thing I noticed in the code, is that we do SortTxtList and keep track of the sorted list in Registeration entries so that we can detect duplicate publish request. I think we can simplfit this and treat TXT data as data blob (seq of bytes) and compare the bytes directly (if there is a change to data blob, it is a change and need to updated/re-registered on mDNS).

Practically, I think it should work because an end device likely won't change the order of the TXT entries when it updates one or more entries. But conceptually, we can't guarantee that, though it's also okay that we treat two TXT blob as different values even when there are just order changes (we simply publish the same service for one more time).

One ides is to change the Publisher APIs to directly get the encoded TXT data instead of a TxtEntry list.

  • For mDNSResponder we can directly use the encoded data (so avoid converting it from encoded format to TxtEntry array and back to encoded format)
  • For Avahi, it may be easier to convert from encoded TXT data to AvahiStringList.

@wgtdkp what do you think? do you see any issues with this?

It sounds okay to me. But if "Attribute present, with empty value (e.g., "PlugIns=" -- the server supports plugins, but none are presently installed)" is the only problem we are trying to solve, the simplest solution is probably add a boolean flag to TxtEntry. Thoughts?

// cc @superwhd

@superwhd
Copy link
Contributor

superwhd commented Sep 1, 2023

But if "Attribute present, with empty value (e.g., "PlugIns=" -- the server supports plugins, but none are presently installed)" is the only problem we are trying to solve, the simplest solution is probably add a boolean flag to TxtEntry.

+1 on this. For the code who uses Publisher in ot-br-posix I think TxtEntry may be slightly more convenient than otDnsGetNextTxtEntry (probably due to STL).

@abtink
Copy link
Member Author

abtink commented Sep 1, 2023

Thanks guys.

I agree that we need to update/enhance the TxtEntry and related EncodeTxtData() and DecodeTxtData() to be able to handle both "boolean attributes" (key only without any =) and empty attributes (key with = and no value).

Regarding the PublishService() method, I think it would be better to have it take encoded TXT data as its input. We currently use the PublishService() method in three places in otbr: (1) in advertising_proxy.cpp, (2) in trel_dnssd, and (3) in border_agent.cpp (to publish the "meshcop" service). In the future, we may also use it for the otPlatDnssd API and SRPL.

In all cases except for the border_agent case, we already have the encoded TXT data and currently need to do the extra step of constructing a TxEntry list from it. It would be better to make it easier for the more common case. It would also make the PublishService() API more similar to the Subscribe() API, which uses std::vector<uint8_t> mTxtData as encoded data in the discovered service instance struct DiscoveredInstanceInfo.

For the code who uses Publisher in ot-br-posix I think TxtEntry may be slightly more convenient than otDnsGetNextTxtEntry (probably due to STL).

Since we already have Mdsn::EncodeTxtData() and DecodeTxtData(), users can still use the more convenient TxEntry list to generate/parse the encoded TXT data. Users do not need to use ot APIs like otDnsGetNextTxtEntry().

@abtink
Copy link
Member Author

abtink commented Sep 4, 2023

Submitted first PR -> #1992 on this for enhancing Encode/DecodeTxtData()

@abtink
Copy link
Member Author

abtink commented Sep 6, 2023

@abtink
Copy link
Member Author

abtink commented Sep 20, 2023

All PRs address this are merged. We can close this.

@abtink abtink closed this as completed Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants