[SSSD] [PATCH 3/4] Add authtok utility functions.

Simo Sorce simo at redhat.com
Mon Dec 3 20:10:53 UTC 2012


On Mon, 2012-12-03 at 15:34 +0100, Pavel Březina wrote:
> On 12/02/2012 05:59 AM, Simo Sorce wrote:
> > These functions allow handling of auth tokens in a completely opaque way,
> > with clear semantics and accessor fucntions that guarantee consistency,
> > proper access to data and error conditions.
> > ---
> 
> /home/pbrezina/workspace/sssd/.git/rebase-apply/patch:229: new blank 
> line at EOF.
> +
> warning: 1 line adds whitespace errors.
> 
> Can we move enum sss_authtok_type to authtok.h?

Yes, I thought I did it, I had the same intention, will check if there
is a reason why I did not, otherwise I'll move.

> > +size_t sss_authtok_get_size(struct sss_auth_token *tok)
> > +{
> > +    switch (tok->type) {
> > +    case SSS_AUTHTOK_TYPE_PASSWORD:
> > +    case SSS_AUTHTOK_TYPE_CCFILE:
> > +        return tok->length;
> > +    default:
> > +        return 0;
> > +    }
> > +}
> 
> Return 0 for SSS_AUTHTOK_TYPE_CCFILE and print error otherwise?
> Also not using default branch for enum would be nice, because we'll get 
> compiler error if we add new enum value and not handle it in switch.

Oh, yes, I actually liked they were enum's exactly because then I could
avoid 'default' ... thanks for catching this.

> > +
> > +uint8_t *sss_authtok_get_data(struct sss_auth_token *tok)
> > +{
> > +    return (void *)tok->data;
> > +}
> 
> Why are you typecasting to void* when you are returning uint8_t*? And 
> tok->data is already uint8_t*.

bug, initially the interface was returning void *, then I change it,
thanks for catching.

> > +static errno_t sss_authtok_set_string(TALLOC_CTX *mem_ctx,
> > +                                      struct sss_auth_token *tok,
> > +                                      enum sss_authtok_type type,
> > +                                      const char *context_name,
> > +                                      const char *str, size_t len)
> > +{
> > +    size_t size;
> > +
> > +    if (len == 0) {
> > +        len = strlen(str);
> > +    } else {
> > +        while (len > 0 && str[len - 1] == '\0') len--;
> > +    }
> > +
> > +    if (len == 0) {
> 
> You should check len <= 0, because if negative len was passed in 
> parameter list, it will get here unchanged.

size_t is unsigned, it can't represent negative numbers :-)

> > +        /* we do not allow zero length ttyped tokens */
> 
>                                            ^ typo

will fix

> > +/* Auth token structure,
> > + * please never use directly.
> > + * Use ss_authtok_* accesor functions instead
>            ^typo

as well.

Thanks,
Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list