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

at_c: remove atclient_atstr/atbytes and use native char * C strings #284

Closed
Tracked by #155 ...
JeremyTubongbanua opened this issue May 28, 2024 · 5 comments
Closed
Tracked by #155 ...
Assignees

Comments

@JeremyTubongbanua
Copy link
Member

JeremyTubongbanua commented May 28, 2024

Would like to bring up discussion on getting rid of atclient_atstr

For reasons:

  • Creating our own string library could be overwhelming. We don't want to have to teach the programmer using the C SDK how to use our special string library when we should just be using regular C strings.
  • Just gets rid of lots of confusion

How Functions That Have atclient_atstr As A Parameter Will Change

The initial reason why atclient_atstr was created, was because of the constant pattern of having 3 things bundled together: 1. a buffer, 2. a buffer size, and 3. length of the value inside of the buffer (the bytes we are concerned about).

For functions that return strings, instead of populating an atclient_atstr, we should adopt the technique of passing a double pointer, where the function is in charge of allocating the memory of the string it wants to return.

So instead of a function being called similarly to:

atclient_atstr result;
atclient_atstr_init(&result, 256);

atclient_atkey_to_string(&atkey, result.str, result.size, &result.len);

// ...

atclient_atstr_free(&result);

We would call it like:

char *result = NULL;

atclient_atkey_to_string(&atkey, &result);

// ...

free(result);

The first way is inefficient because 256 bytes is always allocated, despite maybe only 100 (for example) is being actually useful.

The second way is more efficient because only enough memory is allocated as needed (100 for example, if an atkey string resulted in a length of 100).

How Structs That Use atclient_atstr As A Field/Variable Will Change

This is the atclient_atkey struct

typedef struct atclient_atkey {
  // TODO: remove atkey_type and replace it with a policy function that infers atkeytype given atclient_atkey & atclient
  atclient_atkey_type atkeytype;

  // TODO: this should be called atkey.key to be consistent with dart
  atclient_atstr name;
  atclient_atstr namespacestr;
  atclient_atstr sharedby;
  atclient_atstr sharedwith;

  atclient_atkey_metadata metadata;

} atclient_atkey;

The struct would be changed to something like

typedef struct atclient_atkey {
  // TODO: remove atkey_type and replace it with a policy function that infers atkeytype given atclient_atkey & atclient
  atclient_atkey_type atkeytype;

  char *key;
  char *namespacestr;
  char *sharedby;
  char *sharedwith;

  atclient_atkey_metadata metadata;

  uint8_t initializedfields[2];


} atclient_atkey;

Where in the init function, these will be set to NULL

There will also be initializedfields which will dictate which fields are initialized and are safe to be read

@JeremyTubongbanua JeremyTubongbanua self-assigned this May 28, 2024
@JeremyTubongbanua JeremyTubongbanua changed the title at_c: discussion to get rid of atstr at_c: discussion to get rid of atstr/atbytes May 28, 2024
@JeremyTubongbanua JeremyTubongbanua changed the title at_c: discussion to get rid of atstr/atbytes at_c: discussion to remove of atstr/atbytes Jun 10, 2024
@JeremyTubongbanua JeremyTubongbanua changed the title at_c: discussion to remove of atstr/atbytes at_c: discussion to remove atclient_atstr/atbytes Jun 10, 2024
@JeremyTubongbanua
Copy link
Member Author

@XavierChanth agrees that we should use char * wherever we expect a null-terminated string. Only time that a length should be passed around is when we are doing cryptographic operations

@JeremyTubongbanua
Copy link
Member Author

@realvarx Would like your thoughts on this

@realvarx
Copy link
Contributor

realvarx commented Jun 12, 2024

@realvarx Would like your thoughts on this

I believe that using char* instead of atclient_atstr is the right approach when we want to define structs like atclient_atkey. We are already using parameters like olen in many atclient functions to determine the bytes we have used in a buffer. Although it may be slightly less convenient for us, it will also allow us greater control over how much space we use, and users will more easily understand how to use some features of our sdk.

On the other hand, I think it can still be useful in some contexts of less relevant internal functions, which the user should never need to call or use directly.

@JeremyTubongbanua
Copy link
Member Author

After discussion with XC and AM, we would like to go through and eliminate atstr and go through with native char * C strings.

I will be renaming the title and allocating SP to conduct the changes. Also adding to the breaking changes tracker

@JeremyTubongbanua JeremyTubongbanua changed the title at_c: discussion to remove atclient_atstr/atbytes at_c: remove atclient_atstr/atbytes and use native char * C strings Jun 24, 2024
@JeremyTubongbanua
Copy link
Member Author

Closed by #337

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

2 participants