[SSSD] [PATCH 4/5] Streamline ipa_account_info handler

Pavel Březina pbrezina at redhat.com
Tue Nov 27 11:34:08 UTC 2012


On 11/24/2012 05:33 AM, Simo Sorce wrote:
> In particular note that we merge ipa_account_info_netgroups_done()
> and ipa_account_info_users_done() into a single fucntion called
> ipa_account_info_done() that handles both cases
>
> We also remove the auxiliary function ipa_account_info_complete() that
> unnecessarily violates the tevent_req style and instead use a new function
> named ipa_account_info_error_text() to generate error text.
> ---

Nack. Comments inline.

> +static const char *ipa_account_info_error_text(int ret, int *dp_error,
> +                                               const char *default_text)
> +{
> +    switch (*dp_error) {
> +    case DP_ERR_OK:
> +        if (ret == EOK) {
> +            return NULL;
> +        }
> +        DEBUG(SSSDBG_CRIT_FAILURE, ("Bug: dp_error is OK on failed request"));

Missing \n in debug message.

>   void ipa_account_info_handler(struct be_req *breq)
>   {
> @@ -46,6 +70,7 @@ void ipa_account_info_handler(struct be_req *breq)
>       struct be_acct_req *ar;
>       struct tevent_req *req;
>       const char *err = "Unknown Error";
> +    bool is_subdom_req = false;
>       int ret = EOK;
>
>       ipa_ctx = talloc_get_type(breq->be_ctx->bet_info[BET_ID].pvt_bet_data, struct ipa_id_ctx);
> @@ -58,19 +83,31 @@ void ipa_account_info_handler(struct be_req *breq)
>       ar = talloc_get_type(breq->req_data, struct be_acct_req);
>
>       if (strcasecmp(ar->domain, breq->be_ctx->domain->name) != 0) {
> -        req = ipa_get_subdom_acct_send(breq, breq->be_ctx->ev, ctx, ar);
> -        if (!req) {
> -            return sdap_handler_done(breq, DP_ERR_FATAL, ENOMEM, "Out of memory");
> -        }
> +        is_subdom_req = true;
> +    }
>
> -        tevent_req_set_callback(req, ipa_account_info_users_done, breq);
> -
> -        return;
> +    if (is_subdom_req && ((ar->entry_type & (BE_REQ_USER|BE_REQ_GROUP)) == 0)) {
> +        ret = EINVAL;
> +        err = "Invalid sub-domain request type";
> +        goto done;
>       }

This way you allow BE_REQ_INITGROUPS. BE_REQ* constants are not bit 
flags so you can't use & and |. BE_REQ_USER|BE_REQ_GROUP == 
BE_REQ_INITGROUPS

Also you are introducing again another place where we are testing if 
request is allowed in subdomains (which basically reverts my patch) and
hides detailed initgroups error message (reverts Sumit's patch). Both 
explained in:

https://lists.fedorahosted.org/pipermail/sssd-devel/2012-November/012230.html

In other words, I like it better how it was. This change adds more 
complexity than clarity.

>
>       switch (ar->entry_type & 0xFFF) {
>       case BE_REQ_USER: /* user */
>       case BE_REQ_GROUP: /* group */
> +
> +        if (is_subdom_req) {
> +            req = ipa_get_subdom_acct_send(breq, breq->be_ctx->ev, ctx, ar);
> +            if (!req) {
> +                ret = ENOMEM;
> +                err = "Out of memory";
> +                goto done;
> +            }
> +            tevent_req_set_callback(req, ipa_account_info_done, breq);
> +            return;
> +        }
> +        /* intentional fall-through */
> +
>       case BE_REQ_INITGROUPS: /* init groups for user */
>       case BE_REQ_SERVICES: /* Services. Not natively supported by IPA */
>           return sdap_handle_account_info(breq, ctx);
> @@ -79,15 +116,17 @@ void ipa_account_info_handler(struct be_req *breq)
>           if (ar->filter_type != BE_FILTER_NAME) {
>               ret = EINVAL;
>               err = "Invalid filter type";
> -            break;
> +            goto done;
>           }
>
> -        req = ipa_id_get_netgroup_send(breq, breq->be_ctx->ev, ipa_ctx, ar->filter_value);
> +        req = ipa_id_get_netgroup_send(breq, breq->be_ctx->ev,
> +                                       ipa_ctx, ar->filter_value);
>           if (!req) {
> -            return sdap_handler_done(breq, DP_ERR_FATAL, ENOMEM, "Out of memory");
> +            ret = ENOMEM;
> +            err = "Out of memory";
> +            goto done;
>           }
> -
> -        tevent_req_set_callback(req, ipa_account_info_netgroups_done, breq);
> +        tevent_req_set_callback(req, ipa_account_info_done, breq);
>           break;
>
>       default: /*fail*/
> @@ -95,55 +134,32 @@ void ipa_account_info_handler(struct be_req *breq)
>           err = "Invalid request type";
>       }
>
> -    if (ret != EOK) return sdap_handler_done(breq, DP_ERR_FATAL, ret, err);
> +done:
> +    if (ret != EOK) {
> +        return sdap_handler_done(breq, DP_ERR_FATAL, ret, err);
> +    }
>   }
>
> -static void ipa_account_info_complete(struct be_req *breq, int dp_error,
> -                                       int ret, const char *default_error_text)
> +static void ipa_account_info_done(struct tevent_req *req)
>   {
> -    const char* error_text;
> +    struct be_req *breq = tevent_req_callback_data(req, struct be_req);
> +    struct be_acct_req *ar = talloc_get_type(breq->req_data,
> +                                             struct be_acct_req);
> +    const char *error_text;
> +    int ret, dp_error;
>
> -    if (dp_error == DP_ERR_OK) {
> -        if (ret == EOK) {
> -            error_text = NULL;
> -        } else {
> -            DEBUG(1, ("Bug: dp_error is OK on failed request"));
> -            dp_error = DP_ERR_FATAL;
> -            error_text = default_error_text;
> -        }
> -    } else if (dp_error == DP_ERR_OFFLINE) {
> -        error_text = "Offline";
> -    } else if (dp_error == DP_ERR_FATAL && ret == ENOMEM) {
> -        error_text = "Out of memory";
> +    if (ar->entry_type & BE_REQ_NETGROUP) {

Again, you can't use &.




More information about the sssd-devel mailing list