[SSSD] IFP: Implement methods to get and list domain objects

Jakub Hrozek jhrozek at redhat.com
Tue May 27 12:17:47 UTC 2014


On Mon, May 26, 2014 at 10:33:22PM +0200, Pavel Březina wrote:
> From f9be8417fefd5e336a85fece276e5601b75f3ccf Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> Date: Fri, 2 May 2014 13:08:21 +0200
> Subject: [PATCH 4/4] IFP: Implement SSSD components

Some general comments while you're rebasing the patch..

> +int ifp_find_backend_by_name(struct sbus_request *req,
> +                             void *data,
> +                             const char *arg_name)
> +{
> +    struct ifp_ctx *ctx = NULL;
> +    DBusError *error = NULL;
> +    const char *result = NULL;
> +
> +    ctx = talloc_get_type(data, struct ifp_ctx);
> +    if (ctx == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "Invalid ifp context!\n");
> +        error = sbus_error_new(req, DBUS_ERROR_FAILED, "Invalid ifp context!");
> +        return sbus_request_fail_and_finish(req, error);
> +    }
> +
> +    if (backend_exists(ctx->rctx->cdb, arg_name)) {
> +        result = ifp_reply_objpath(req, PATH_BACKENDS, arg_name);
> +        if (result == NULL) {
> +            error = sbus_error_new(req, DBUS_ERROR_NO_MEMORY,
> +                                   "%s", strerror(ENOMEM));
> +            return sbus_request_fail_and_finish(req, error);

We now have two ways to handle ENOMEM in IFP. The one above, and the one
is that I used previously as suggested by Stef:
    if (ptr == NULL) {
        return sbus_request_finish(dbus_req, NULL);
    }

It's simpler because if the system is really OOM, you won't be able to
construct the error and in most cases even send the reply back..

(In general we have different opinion on handling OOM with Stef, but
this seemed like a common ground)

I would prefer if the responder codebase used one way to handle ENOMEM
consistently.

> +        }
> +    } else {
> +        error = sbus_error_new(req, DBUS_ERROR_FAILED,
> +                               "Backend \"%s\" does not exist", arg_name);
> +        return sbus_request_fail_and_finish(req, error);
> +    }
> +
> +    return infopipe_iface_FindBackendByName_finish(req, result);
> +}
> +
> +#ifndef HAVE_CONFIG_LIB
> +int ifp_component_enable(struct sbus_request *req, void *data)

I wonder if it would be safer to have the #ifndef inside the function to
only keep one declaration of each function instead of two?

Maybe:
int ifp_component_enable(struct sbus_request *req, void *data)
{
#ifdef HAVE_CONFIG_LIB
    /* Do  stuff */
    retun infopipe_component_Enable_finish();
#else
    return sbus_request_fail_and_finish(req,
                sbus_error_new(req, DBUS_ERROR_NOT_SUPPORTED, NULL));
#endif
}

> +{
> +    return sbus_request_fail_and_finish(req,
> +                sbus_error_new(req, DBUS_ERROR_NOT_SUPPORTED, NULL));
> +}
> +#else
> +int ifp_component_enable(struct sbus_request *req, void *data)

It would be nice to use dbus_req, not req as the variable name. I know
that's how codegen currently generates the code and I have a TODO item
to change the name there as well. Everytime I see a variable names 'req'
I think of it as a tevent_req..

We can rename the variable post-beta, if that's too much work, I don't
want to delay the patches any longer, but maybe it's just one click with
your mighty IDE?

> +{
> +    struct ifp_ctx *ctx = NULL;
> +    DBusError *error = NULL;
> +    const char *path = dbus_message_get_path(req->message);
> +    char *name = NULL;
> +    enum component_type type;
> +    struct sss_config_ctx *config_ctx = NULL;
> +    errno_t ret;
> +
> +    ctx = talloc_get_type(data, struct ifp_ctx);
> +    if (ctx == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "Invalid ifp context!\n");
> +        error = sbus_error_new(req, DBUS_ERROR_FAILED, "Invalid ifp context!");
> +        goto done;
> +    }
> +
> +    ret = check_and_get_component_from_path(req, ctx->rctx->cdb,
> +                                            path, &type, &name);
> +    if (ret != EOK) {
> +        error = sbus_error_new(req, DBUS_ERROR_FAILED, "%s", strerror(ret));
> +        goto done;
> +    }
> +
> +    config_ctx = sss_config_open(NULL, NULL, CONFDB_DEFAULT_CONFIG_FILE);

Can you use req/dbus_req here as the talloc context?

> +    if (config_ctx == NULL) {
> +        error = sbus_error_new(req, DBUS_ERROR_NO_MEMORY,
> +                               "%s", strerror(ENOMEM));
> +        goto done;
> +    }
> +
> +    switch (type) {
> +    case COMPONENT_MONITOR:
> +        error = sbus_error_new(req, DBUS_ERROR_NOT_SUPPORTED, NULL);
> +        goto done;
> +        break;
> +    case COMPONENT_RESPONDER:
> +        ret = sss_config_service_enable(config_ctx, name);
> +        break;
> +    case COMPONENT_BACKEND:
> +        ret = sss_config_domain_enable(config_ctx, name);
> +        break;
> +    }
> +
> +    if (ret != EOK) {
> +        error = sbus_error_new(req, DBUS_ERROR_FAILED, "%s", strerror(ret));
> +        goto done;
> +    }
> +
> +    ret = sss_config_save(config_ctx);
> +    if (ret != EOK) {
> +        error = sbus_error_new(req, DBUS_ERROR_FAILED, "%s", strerror(ret));
> +        goto done;
> +    }
> +
> +done:
> +    sss_config_close(&config_ctx);
> +
> +    if (error != NULL) {
> +        return sbus_request_fail_and_finish(req, error);
> +    }
> +
> +    return infopipe_component_Enable_finish(req);
> +}
> +#endif

In general the patches look good to me.



More information about the sssd-devel mailing list