Skip to content

Commit

Permalink
Build FlatAllocatedQName from array, use for TXT. (#7364)
Browse files Browse the repository at this point in the history 
* Build FlatAllocatedQName from array, use for TXT.

The argument version of the Build command allows the name parts to
be easily specified within the build function, but does not allow
a run-time change of the number of args. This is fine for the
service names, but causes problems for the variable-length txt
fields. Previously we were sending 0-length txt fields, but this
causes problems on some impementations. It's better to just not send
the fields we don't need.

* Add back vendor, add empty text entries.

Also use const value for sizing so it's clear where that number is
from.
  • Loading branch information
@cecille @pull
cecille authored and pull[bot] committed Jul 2, 2021
1 parent 48a562e commit 1217222
Show file tree
Hide file tree
Showing 3 changed files with 139 additions and 40 deletions.
82 changes: 44 additions & 38 deletions src/lib/mdns/Advertiser_ImplMinimalMdns.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,7 @@ class AdvertiserMinMdns : public ServiceAdvertiser,
return AddAllocatedResponder(chip::Platform::New<ResponderType>(std::forward<Args>(args)...));
}

template <typename... Args>
FullQName AllocateQName(Args &&... names)
void * AllocateQNameSpace(size_t size)
{
for (size_t i = 0; i < kMaxAllocatedQNameData; i++)
{
Expand All @@ -185,19 +184,36 @@ class AdvertiserMinMdns : public ServiceAdvertiser,
continue;
}

mAllocatedQNameParts[i] =
chip::Platform::MemoryAlloc(FlatAllocatedQName::RequiredStorageSize(std::forward<Args>(names)...));

mAllocatedQNameParts[i] = chip::Platform::MemoryAlloc(size);
if (mAllocatedQNameParts[i] == nullptr)
{
ChipLogError(Discovery, "QName memory allocation failed");
return FullQName();
}
return FlatAllocatedQName::Build(mAllocatedQNameParts[i], std::forward<Args>(names)...);
return mAllocatedQNameParts[i];
}

ChipLogError(Discovery, "Failed to find free slot for adding a qname");
return FullQName();
return nullptr;
}

template <typename... Args>
FullQName AllocateQName(Args &&... names)
{
void * storage = AllocateQNameSpace(FlatAllocatedQName::RequiredStorageSize(std::forward<Args>(names)...));
if (storage == nullptr)
{
return FullQName();
}
return FlatAllocatedQName::Build(storage, std::forward<Args>(names)...);
}

FullQName AllocateQNameFromArray(char const * const * names, size_t num)
{
void * storage = AllocateQNameSpace(FlatAllocatedQName::RequiredStorageSizeFromArray(names, num));
if (storage == nullptr)
{
return FullQName();
}
return FlatAllocatedQName::BuildFromArray(storage, names, num);
}

FullQName GetCommisioningTextEntries(const CommissionAdvertisingParameters & params);
Expand Down Expand Up @@ -546,6 +562,11 @@ CHIP_ERROR AdvertiserMinMdns::Advertise(const CommissionAdvertisingParameters &

FullQName AdvertiserMinMdns::GetCommisioningTextEntries(const CommissionAdvertisingParameters & params)
{
// Max number of TXT fields from the spec is 9: D, VP, AP, CM, DT, DN, RI, PI, PH.
constexpr size_t kMaxTxtFields = 9;
const char * txtFields[kMaxTxtFields];
size_t numTxtFields = 0;

char txtVidPid[chip::Mdns::kKeyVendorProductMaxLength + 4];
if (params.GetProductId().HasValue())
{
Expand All @@ -560,20 +581,14 @@ FullQName AdvertiserMinMdns::GetCommisioningTextEntries(const CommissionAdvertis
if (params.GetDeviceType().HasValue())
{
sprintf(txtDeviceType, "DT=%d", params.GetDeviceType().Value());
}
else
{
txtDeviceType[0] = '\0';
txtFields[numTxtFields++] = txtDeviceType;
}

char txtDeviceName[chip::Mdns::kKeyDeviceNameMaxLength + 4];
if (params.GetDeviceName().HasValue())
{
sprintf(txtDeviceName, "DN=%s", params.GetDeviceName().Value());
}
else
{
txtDeviceName[0] = '\0';
txtFields[numTxtFields++] = txtDeviceName;
}

// the following sub types only apply to commissionable node advertisements
Expand All @@ -582,6 +597,7 @@ FullQName AdvertiserMinMdns::GetCommisioningTextEntries(const CommissionAdvertis
// a discriminator always exists
char txtDiscriminator[chip::Mdns::kKeyDiscriminatorMaxLength + 3];
sprintf(txtDiscriminator, "D=%d", params.GetLongDiscriminator());
txtFields[numTxtFields++] = txtDiscriminator;

if (!params.GetVendorId().HasValue())
{
Expand All @@ -590,55 +606,45 @@ FullQName AdvertiserMinMdns::GetCommisioningTextEntries(const CommissionAdvertis

char txtCommissioningMode[chip::Mdns::kKeyCommissioningModeMaxLength + 4];
sprintf(txtCommissioningMode, "CM=%d", params.GetCommissioningMode() ? 1 : 0);
txtFields[numTxtFields++] = txtCommissioningMode;

char txtOpenWindowCommissioningMode[chip::Mdns::kKeyAdditionalPairingMaxLength + 4];
if (params.GetCommissioningMode() && params.GetOpenWindowCommissioningMode())
{
sprintf(txtOpenWindowCommissioningMode, "AP=1");
}
else
{
txtOpenWindowCommissioningMode[0] = '\0';
txtFields[numTxtFields++] = txtOpenWindowCommissioningMode;
}

char txtRotatingDeviceId[chip::Mdns::kKeyRotatingIdMaxLength + 4];
if (params.GetRotatingId().HasValue())
{
sprintf(txtRotatingDeviceId, "RI=%s", params.GetRotatingId().Value());
}
else
{
txtRotatingDeviceId[0] = '\0';
txtFields[numTxtFields++] = txtRotatingDeviceId;
}

char txtPairingHint[chip::Mdns::kKeyPairingInstructionMaxLength + 4];
if (params.GetPairingHint().HasValue())
{
sprintf(txtPairingHint, "PH=%d", params.GetPairingHint().Value());
}
else
{
txtPairingHint[0] = '\0';
txtFields[numTxtFields++] = txtPairingHint;
}

char txtPairingInstr[chip::Mdns::kKeyPairingInstructionMaxLength + 4];
if (params.GetPairingInstr().HasValue())
{
sprintf(txtPairingInstr, "PI=%s", params.GetPairingInstr().Value());
txtFields[numTxtFields++] = txtPairingInstr;
}
else
{
txtPairingInstr[0] = '\0';
}

return AllocateQName(txtDiscriminator, txtVidPid, txtCommissioningMode, txtOpenWindowCommissioningMode, txtDeviceType,
txtDeviceName, txtRotatingDeviceId, txtPairingHint, txtPairingInstr);
}
if (numTxtFields == 0)
{
return AllocateQNameFromArray(mEmptyTextEntries, 1);
}
else
{
return AllocateQName(txtVidPid, txtDeviceType, txtDeviceName);
return AllocateQNameFromArray(txtFields, numTxtFields);
}
}
} // namespace

bool AdvertiserMinMdns::ShouldAdvertiseOn(const chip::Inet::InterfaceId id, const chip::Inet::IPAddress & addr)
{
Expand Down
26 changes: 26 additions & 0 deletions src/lib/mdns/minimal/core/FlatAllocatedQName.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,15 @@ inline size_t RequiredStorageSize(QNamePart name, Args &&... rest)
return sizeof(QNamePart) + strlen(name) + 1 + RequiredStorageSize(std::forward<Args>(rest)...);
}

inline size_t RequiredStorageSizeFromArray(char const * const * names, size_t num)
{
size_t ret = 0;
for (size_t i = 0; i < num; ++i)
{
ret += sizeof(QNamePart) + strlen(names[i]) + 1;
}
return ret;
}
namespace Internal {

// nothing left to initialize
Expand Down Expand Up @@ -87,6 +96,23 @@ inline FullQName Build(void * storage, Args &&... args)
return result;
}

inline FullQName BuildFromArray(void * storage, char const * const * parts, size_t num)
{
// Storage memory holds pointers to each name, then copies of the names after
QNamePart * names = reinterpret_cast<QNamePart *>(storage);
char * nameOut = reinterpret_cast<char *>(names + num);
for (size_t i = 0; i < num; ++i)
{
QNamePart * ptrLocation = names + i;
Internal::Initialize(ptrLocation, nameOut, parts[i]);
nameOut += strlen(parts[i]) + 1;
}
FullQName result;
result.names = names;
result.nameCount = num;
return result;
}

} // namespace FlatAllocatedQName

} // namespace Minimal
Expand Down
71 changes: 69 additions & 2 deletions src/lib/mdns/minimal/core/tests/TestFlatAllocatedQName.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,76 @@ void TestFlatAllocatedQName(nlTestSuite * inSuite, void * inContext)
}
}

void SizeCompare(nlTestSuite * inSuite, void * inContext)
{
const char kThis[] = "this";
const char kIs[] = "is";
const char kA[] = "a";
const char kTest[] = "test";
const char kVest[] = "vest";
const char kBee[] = "bee";
const char kRobbery[] = "robbery";
const char kExtra[] = "extra";

const char * kSameArraySameSize[4] = { kThis, kIs, kA, kTest };
const char * kDifferentArraySameSize[4] = { kThis, kIs, kA, kVest };
const char * kDifferenArrayLongerWord[4] = { kThis, kIs, kA, kRobbery };
const char * kDifferenArrayShorterWord[4] = { kThis, kIs, kA, kBee };
const char * kSameArrayExtraWord[5] = { kThis, kIs, kA, kTest, kExtra };
const char * kShorterArray[3] = { kThis, kIs, kA };

const size_t kTestStorageSize = FlatAllocatedQName::RequiredStorageSize(kThis, kIs, kA, kTest);

NL_TEST_ASSERT(inSuite, kTestStorageSize == FlatAllocatedQName::RequiredStorageSizeFromArray(kSameArraySameSize, 4));
NL_TEST_ASSERT(inSuite, kTestStorageSize == FlatAllocatedQName::RequiredStorageSizeFromArray(kDifferentArraySameSize, 4));
NL_TEST_ASSERT(inSuite, kTestStorageSize < FlatAllocatedQName::RequiredStorageSizeFromArray(kDifferenArrayLongerWord, 4));
NL_TEST_ASSERT(inSuite, kTestStorageSize > FlatAllocatedQName::RequiredStorageSizeFromArray(kDifferenArrayShorterWord, 4));

// Although the size of the array is larger, if we tell the function there are only 4 words, it should still work.
NL_TEST_ASSERT(inSuite, kTestStorageSize == FlatAllocatedQName::RequiredStorageSizeFromArray(kSameArrayExtraWord, 4));
// If we add the extra word, the sizes should not match
NL_TEST_ASSERT(inSuite, kTestStorageSize < FlatAllocatedQName::RequiredStorageSizeFromArray(kSameArrayExtraWord, 5));

NL_TEST_ASSERT(inSuite, kTestStorageSize > FlatAllocatedQName::RequiredStorageSizeFromArray(kShorterArray, 3));
NL_TEST_ASSERT(inSuite,
FlatAllocatedQName::RequiredStorageSizeFromArray(kSameArraySameSize, 3) ==
FlatAllocatedQName::RequiredStorageSizeFromArray(kShorterArray, 3));
}

void BuildCompare(nlTestSuite * inSuite, void * inContext)
{
const char kThis[] = "this";
const char kIs[] = "is";
const char kA[] = "a";
const char kTest[] = "test";
const char kExtra[] = "extra";

const char * kSameArraySameSize[4] = { kThis, kIs, kA, kTest };
const char * kSameArrayExtraWord[5] = { kThis, kIs, kA, kTest, kExtra };
const char * kShorterArray[3] = { kThis, kIs, kA };

uint8_t storage[256];

const FullQName kTestQName = FlatAllocatedQName::Build(storage, kThis, kIs, kA, kTest);

NL_TEST_ASSERT(inSuite, kTestQName == FlatAllocatedQName::BuildFromArray(storage, kSameArraySameSize, 4));

// Although the size of the array is larger, if we tell the function there are only 4 words, it should still work.
NL_TEST_ASSERT(inSuite, kTestQName == FlatAllocatedQName::BuildFromArray(storage, kSameArrayExtraWord, 4));
// If we add the extra word, the names
NL_TEST_ASSERT(inSuite, kTestQName != FlatAllocatedQName::BuildFromArray(storage, kSameArrayExtraWord, 5));

NL_TEST_ASSERT(inSuite, kTestQName != FlatAllocatedQName::BuildFromArray(storage, kShorterArray, 3));
NL_TEST_ASSERT(inSuite,
FlatAllocatedQName::BuildFromArray(storage, kSameArraySameSize, 3) ==
FlatAllocatedQName::BuildFromArray(storage, kShorterArray, 3));
}

const nlTest sTests[] = {
NL_TEST_DEF("TestFlatAllocatedQName", TestFlatAllocatedQName), //
NL_TEST_SENTINEL() //
NL_TEST_DEF("TestFlatAllocatedQName", TestFlatAllocatedQName), //
NL_TEST_DEF("TestFlatAllocatedQNameRequiredSizes", SizeCompare), //
NL_TEST_DEF("TestFlatAllocatedQNameBuild", BuildCompare), //
NL_TEST_SENTINEL() //
};

} // namespace
Expand Down

0 comments on commit 1217222

Please sign in to comment.