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.

Sign up for GitHub

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

Jump to bottom

Add base64 encoding and decoding APIs that accept az_span. #1816

Merged
merged 4 commits into from
Jul 8, 2021
Merged

Add base64 encoding and decoding APIs that accept az_span. #1816

merged 4 commits into from
Jul 8, 2021

Conversation

ahsonkhan
Copy link
Contributor

Fixes #1618

cc @danewalton

@ahsonkhan ahsonkhan added this to the [2021] July milestone Jul 8, 2021
@ahsonkhan ahsonkhan self-assigned this Jul 8, 2021
@ahsonkhan ahsonkhan mentioned this pull request Jul 8, 2021
Closed
antkmsft
antkmsft approved these changes Jul 8, 2021
View reviewed changes
sdk/src/azure/core/az_base64.c Outdated
static const char _az_base64_encode_array[65]
= "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/";

static const int8_t _az_base64_decode_array[256] = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would try save some of that 256 bytes and try to implement it with ifs/switch-cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an implementation detail, and not sure if it is worth it from a total code size perspective. I will investigate this in a follow-up:
#1817

sdk/src/azure/core/az_base64.c
// into an az_span which has an int32_t length, i.e. (INT32_MAX / 4) * 3;
#define _az_MAX_SAFE_ENCODED_LENGTH 1610612733

#define _az_ENCODING_PAD '='
Copy link
Member

@antkmsft antkmsft Jul 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace #defines with unnamed enum?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw this pattern being used in IoT already, so decided to follow it. We can change it across the board later if there is a coding guideline :)

antkmsft
antkmsft reviewed Jul 8, 2021
View reviewed changes
sdk/src/azure/core/az_base64.c
-1,
};

static AZ_NODISCARD int32_t _az_base64_encode(uint8_t* three_bytes)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mark a bunch of these statics as AZ_INLINE, especially if they are being used once?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an implementation detail/compiler optimization, I can do that separately.

danewalton-msft
danewalton-msft approved these changes Jul 8, 2021
View reviewed changes
sdk/inc/azure/core/az_base64.h
// SPDX-License-Identifier: MIT

/**
* @file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this need:

Suggested change
* @file
* @file az_base64.h

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we got rid of that in most places already (probably the remaining IoT files can remove it too). Let me dig up the rational as to why.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here: #1057

From @antkmsft

  • Remove file name from the @file section - Doxygen still adds it to documentation perfectly, only that now if you rename a file, you don't need to update the comment, the name will get picked up automatically.

@ahsonkhan ahsonkhan enabled auto-merge (squash) July 8, 2021 21:21
@ahsonkhan ahsonkhan merged commit ec4df09 into Azure:main Jul 8, 2021
ahsonkhan added a commit that referenced this pull request Jul 9, 2021
@ahsonkhan @azure-sdk
@mitchdenny @chidozieononiwu @weshaggard @momuno @hyoshioka0128 @benbp @scbedd @danieljurek @danewalton-msft @CIPop @praveenkuttappan @mikexmas @JoshLove-msft @RickWinter
Pull in main into the feature/iot pnp branch. (#1821)
2a4b93e 
* Sync eng/common directory with azure-sdk-tools for PR 1579 (#1724)

* Allow propogation of SetDevVersion.

* Allow use of SetDevVersion locally and across stages.

Co-authored-by: Mitch Denny <[email protected]>

* Sync eng/common directory with azure-sdk-tools for PR 1524 (#1728)

* Make RELEASE_TITLE_REGEX more specific

* Update eng/common/scripts/ChangeLog-Operations.ps1

Co-authored-by: Wes Haggard <[email protected]>

* Update release REGEX

* tighten up changelog verification

* Ensure releaseDate is in the appriopriate format.

* Update error messages.

* Retrun false after error

Co-authored-by: Chidozie Ononiwu <[email protected]>
Co-authored-by: Chidozie Ononiwu <[email protected]>
Co-authored-by: Wes Haggard <[email protected]>

* Update samples to include log of mqtt client username (#1731)

* Typo "Github"→"GitHub" (#1738)

* Revert changes to SetDevVersion. (#1740)

Co-authored-by: Mitch Denny <[email protected]>

* Add better error handling in Prepare-Release (#1746)

Co-authored-by: Wes Haggard <[email protected]>

* Sync eng/common directory with azure-sdk-tools for PR 1594 (#1747)

* Update sparse checkout create directory step name

* plumb BaseBranchName to create pull request

Co-authored-by: Ben Broderick Phillips <[email protected]>

* Fix sample readme urls (#1752)

* allow multiple invocations of archetype-sdk-tests-generate (#1754)

Co-authored-by: scbedd <[email protected]>

* Sync eng/common directory with azure-sdk-tools for PR 1585 (#1758)

* Sparse checkout docs repository in release step

* Detect default branch for custom sparse checkout repository

* Use sparse checkout paths parameter in docs metadata release

Co-authored-by: Ben Broderick Phillips <[email protected]>

* Sync eng/common directory with azure-sdk-tools for PR 1615 (#1761)

* Move docindex.yml from steps -> jobs

* Add docindex.yml

Co-authored-by: Daniel Jurek <[email protected]>

* add clang format version (#1755)

* Sync eng/common directory with azure-sdk-tools for PR 1627 (#1763)

* Refactor sparse checkout template: idempotency and no yaml loops

* Add logging, re-organize sparse checkout script

Co-authored-by: Ben Broderick Phillips <[email protected]>

* Add Get-ChangeLogEntriesFromContent (#1764)

Co-authored-by: Chidozie Ononiwu <[email protected]>

* Sync eng/common directory with azure-sdk-tools for PR 1635 (#1767)

* Update common engineering system for docs onboarding

* Add updates from review feedback

* Space nit

Co-authored-by: Daniel Jurek <[email protected]>

* add empty section for compile options (#1741)

* Enable Caching of PS Modules (#1769)

- Remove copied AzPowershell utilities
- Add latest AZ module path already on hosted agents to PSModulePath
- Rename setup-az-modules template setup-environments to reflect what is is doing
- Add support for Caching the current user PS Module folder
- Add support for install-module if not already present in module folder
- Organize the live test clean-up script to be in the standard location

Co-authored-by: Wes Haggard <[email protected]>

* Adding new CA trusted certificates (#1765)

Adding new CA certificates and updating the documentation to create a store when testing in Windows or OSX.

* Sync eng/common directory with azure-sdk-tools for PR 1611 (#1766)

* Add API status check

* Revert temp change

* Update as per review comments

* Removed blank line

* Review comments to use az cli

* Changes to move az cli commands to caller script and other review comments

* Skip languages that's not supported in APIView

* Fix bug in language mapping

Co-authored-by: praveenkuttappan <[email protected]>
Co-authored-by: praveenkuttappan <[email protected]>

* Fix for Issue #1768  - Response Types (#1770)

* Add the ability to check for open pull request to a different repo. (#1774)

Co-authored-by: Chidozie Ononiwu <[email protected]>

* Fetch specific branch name only in git-branch-push script (#1771)

Co-authored-by: Ben Broderick Phillips <[email protected]>

* Use generate matrix job name parameter as display name (#1776)

Co-authored-by: Ben Broderick Phillips <[email protected]>

* Updating documentation for ECC server certificate chain support. (#1773)

* Sync eng/common directory with azure-sdk-tools for PR 1633 (#1778)

* Update format of new Changelog Entry

* Add parsing of changelog sections

* Update ChangeLog Logic

Co-authored-by: Chidozie Ononiwu <[email protected]>
Co-authored-by: Chidozie Ononiwu <[email protected]>

* Strict mode needs the variables initialized (#1779)

Co-authored-by: Wes Haggard <[email protected]>

* Fix of depricated BearSSL method call of WiFiClientSecure class in sample for ESP8266. (#1780)

* Fix of depricated BearSSL method call in sample for ESP8266.

* Fix of depricated BearSSL method call of WiFiClientSecure class in sample for ESP8266.

Co-authored-by: ts\bozicekm <[email protected]>

* Sync eng/common directory with azure-sdk-tools for PR 1688 (#1782)

* Add verification of changelog sections

* Update eng/common/scripts/ChangeLog-Operations.ps1

Co-authored-by: Wes Haggard <[email protected]>

Co-authored-by: Chidozie Ononiwu <[email protected]>
Co-authored-by: Chidozie Ononiwu <[email protected]>
Co-authored-by: Wes Haggard <[email protected]>

* Update for master -> main branch rename (#1783)

* ci.yml -- Add forward compatability with "main" to triggering branches

* Fix hard-coded master (#1788)

Co-authored-by: Chidozie Ononiwu <[email protected]>

* Sync eng/common directory with azure-sdk-tools for PR 1716 (#1790)

* Update links with master to use main

* Update ci.yml files

Co-authored-by: Chidozie Ononiwu <[email protected]>

* Sync eng/common directory with azure-sdk-tools for PR 1719 (#1793)

* Update pipeline generation tool version

Consume latest changes from pipeline generation tool in Azure/azure-sdk-tools#1708

* Update tool version to include fix for public ci

Co-authored-by: Wes Haggard <[email protected]>

* Sync eng/common directory with azure-sdk-tools for PR 1729 (#1794)

* Check for API review status only if release date is set in changelog

* Change property name to ReleaseStatus

Co-authored-by: Praveen Kuttappan <[email protected]>

* Add Ubuntu 20 to local dns bypass template (#1795)

Co-authored-by: Praveen Kuttappan <[email protected]>
Co-authored-by: praveenkuttappan <[email protected]>

* Update master to main (#1798)

* Update references from master to main (#1799)

Co-authored-by: Wes Haggard <[email protected]>

* Sync eng/common directory with azure-sdk-tools for PR 1725 (#1805)

* Bring changes from JS docs metadata

* Move business logic inside Update-DocsMsMetadata.ps1

* Update with the latest changes in JS PR

* Update from latest PR feedback

* Add check for empty path to prevent crashes when creating relative paths

Co-authored-by: Daniel Jurek <[email protected]>

* Update devops workitem helpers (#1806)

- Switch to using rest instead of cli for querying work items
  as cli is limited to 1000 items only.
- Fix issues with Generated fields not being set.
- Correctly sort older workitems by version isntead of string.

Co-authored-by: Wes Haggard <[email protected]>

* Sync eng/common directory with azure-sdk-tools for PR 1763 (#1807)

* Disable release date check

* Release check is not finding release date

Co-authored-by: Praveen Kuttappan <[email protected]>
Co-authored-by: praveenkuttappan <[email protected]>

* only add compiler flags for tests (#1791)

* Update docs metadata mutation logic (#1810)

Co-authored-by: Daniel Jurek <[email protected]>

* Sync eng/common directory with azure-sdk-tools for PR 1772 (#1812)

* Update change log headers based on guideline update

Updates based on Azure/azure-sdk#3103

- Renamed "Key Bugs Fixed" to "Bugs Fixed"
- Renamed "Fixed" to "Other Changes"

Added a warning in validation if at lease one of the recommended
headers aren't used.

* Update eng/common/scripts/ChangeLog-Operations.ps1

Co-authored-by: JoshLove-msft <[email protected]>

Co-authored-by: Wes Haggard <[email protected]>
Co-authored-by: Wes Haggard <[email protected]>
Co-authored-by: JoshLove-msft <[email protected]>

* Add public current depth field to JSON reader. (#1802)

* Add public current depth field to JSON reader.

* Add a CL entry.

* Fix typo in src comment.

* Update codeowners file (#1809)

* Switch ubuntu 18 to ubuntu 20 (#1814)

Co-authored-by: Chidozie Ononiwu <[email protected]>

* Sync eng/common directory with azure-sdk-tools for PR 1767 (#1813)

* Support building and deploying bicep templates

* Add bicep powershell install aka link to deployment error message

* Write bicep compiled arm templates to temp directory

* Simplify bicep building code/function usage

* Use bicep location for compiled arm templates, and remove them on success

Co-authored-by: Ben Broderick Phillips <[email protected]>

* Add base64 encoding and decoding APIs that accept az_span. (#1816)

* Add base64 encoding and decoding APIs that accept az_span.

* Add entry to the changelog.

* Address PR feedback - move const to r.h.s.

* Fix link substitution (#1820)

* Fix doc publishing to support basic link substitution

* Remove publishing for az_storage_blobs

Co-authored-by: Azure SDK Bot <[email protected]>
Co-authored-by: Mitch Denny <[email protected]>
Co-authored-by: Chidozie Ononiwu <[email protected]>
Co-authored-by: Chidozie Ononiwu <[email protected]>
Co-authored-by: Wes Haggard <[email protected]>
Co-authored-by: Mollie Munoz <[email protected]>
Co-authored-by: Hiroshi Yoshioka <[email protected]>
Co-authored-by: Wes Haggard <[email protected]>
Co-authored-by: Ben Broderick Phillips <[email protected]>
Co-authored-by: scbedd <[email protected]>
Co-authored-by: Daniel Jurek <[email protected]>
Co-authored-by: Dane Walton <[email protected]>
Co-authored-by: Cristian Pop <[email protected]>
Co-authored-by: praveenkuttappan <[email protected]>
Co-authored-by: praveenkuttappan <[email protected]>
Co-authored-by: Miha Božiček <[email protected]>
Co-authored-by: ts\bozicekm <[email protected]>
Co-authored-by: Praveen Kuttappan <[email protected]>
Co-authored-by: JoshLove-msft <[email protected]>
Co-authored-by: Rick Winter <[email protected]>
@ahsonkhan ahsonkhan deleted the AddBase64 branch July 10, 2021 00:42
@ahsonkhan
Copy link
Contributor Author

Code coverage looks good (almost 100% line coverage):
https://dev.azure.com/azure-sdk/public/_build/results?buildId=990064&_a=summary&tab=ms.vss-codecoverage-web.code-coverage-details&view=codecoverage-tab
image

The only branches missing is precondition testing:
#1836

@ahsonkhan ahsonkhan mentioned this pull request Sep 9, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antkmsft antkmsft antkmsft approved these changes

@danewalton-msft danewalton-msft danewalton-msft approved these changes

@JeffreyRichter JeffreyRichter Awaiting requested review from JeffreyRichter

@gearama gearama Awaiting requested review from gearama gearama is a code owner

@RickWinter RickWinter Awaiting requested review from RickWinter RickWinter is a code owner

@vhvb1989 vhvb1989 Awaiting requested review from vhvb1989

Assignees

@ahsonkhan ahsonkhan

Labels
Azure.Core
Projects
None yet
Milestone
[2021] July
Development

Successfully merging this pull request may close these issues.

Add base64 encoding and decoding to az_span
3 participants
@ahsonkhan @danewalton-msft @antkmsft