On Tue, Apr 19, 2016 at 10:59:20AM +0200, Pavel Březina wrote:
On 04/19/2016 09:59 AM, Lukas Slebodnik wrote:
> On (01/03/16 13:53), Pavel Březina wrote:
> > On 02/25/2016 02:08 PM, Jakub Hrozek wrote:
> > > On Tue, Feb 09, 2016 at 02:07:21PM +0100, Pavel Březina wrote:
> > > > First of the responders is converted -)
> > >
> > > Sorry for the first delay in review. Before reading the code, I
> > > submitted the patches to automated tests, so far I found:
> > >
> > > Error: COMPILER_WARNING:
> > > sssd-1.13.90/src/responder/sudo/sudosrv_get_sudorules.c:29: included_from:
Included from here.
> > > sssd-1.13.90/src/responder/sudo/sudosrv_get_sudorules.c: scope_hint: In
function 'sudosrv_get_rules_done'
> > > sssd-1.13.90/src/util/util.h:144:9: warning: 'debug_name' may be
used uninitialized in this function [-Wmaybe-uninitialized]
> > > # sss_debug_fn(__FILE__, __LINE__, __FUNCTION__, \
> > > # ^
> > > sssd-1.13.90/src/responder/sudo/sudosrv_get_sudorules.c:225:17: note:
'debug_name' was declared here
> > > # const char *debug_name;
> > > # ^
> > > # 142| int __debug_macro_level = level; \
> > > # 143| if (DEBUG_IS_SET(__debug_macro_level)) { \
> > > # 144|-> sss_debug_fn(__FILE__, __LINE__, __FUNCTION__, \
> > > # 145| __debug_macro_level, \
> > > # 146| format, ##__VA_ARGS__); \
> > >
> > > But feel free to only fix the issue in your branch for now, at least
until
> > > all the automated downstream tests finish.
> >
> // snip
>
> > +
> > +static errno_t sudosrv_query_cache(TALLOC_CTX *mem_ctx,
> > + struct sss_domain_info *domain,
> > + const char **attrs,
> > + unsigned int flags,
> > + const char *username,
> > + uid_t uid,
> > + char **groupnames,
> > + bool inverse_order,
> > + struct sysdb_attrs ***_rules,
> > + uint32_t *_count)
> > +{
> > + TALLOC_CTX *tmp_ctx;
> > + char *filter;
> > + errno_t ret;
> > + size_t count;
> > + struct sysdb_attrs **rules;
> > + struct ldb_message **msgs;
> > +
> > + tmp_ctx = talloc_new(NULL);
> > + if (tmp_ctx == NULL) {
> > + return ENOMEM;
> > + }
> > +
> > + ret = sysdb_get_sudo_filter(tmp_ctx, username, uid, groupnames,
> > + flags, &filter);
> > + if (ret != EOK) {
> > + DEBUG(SSSDBG_CRIT_FAILURE, "Could not construct the search filter
"
> > + "[%d]: %s\n", ret, sss_strerror(ret));
> > + goto done;
> > + }
> > +
> > + DEBUG(SSSDBG_FUNC_DATA, "Searching sysdb with [%s]\n", filter);
> > +
> > + if (IS_SUBDOMAIN(domain)) {
> > + /* rules are stored inside parent domain tree */
> > + domain = domain->parent;
> > + }
> > +
> > + ret = sysdb_search_custom(tmp_ctx, domain, filter, SUDORULE_SUBDIR,
> > + attrs, &count, &msgs);
> > + if (ret != EOK && ret != ENOENT) {
> > + DEBUG(SSSDBG_CRIT_FAILURE, "Error looking up SUDO
rules\n");
> > + goto done;
> > + } if (ret == ENOENT) {
> ^^^^^^^^^^^^^^^^^^^^^^
> I wanted to push these ACK-ed patches but I noticed
> this non-standard change :-)
>
> I think you wanted here "else if" or "if" on next line.
> I can change it before push If you let me which way you like more.
ret = sysdb_search_custom(tmp_ctx, domain, filter, SUDORULE_SUBDIR,
attrs, &count, &msgs);
if (ret == ENOENT) {
*_rules = NULL;
*_count = 0;
ret = EOK;
goto done;
} else if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, "Error looking up SUDO rules\n");
goto done;
}
This is how it's supposed to be :-) Thanks.
Thank you for catching this Lukas, please don't forget to add yourself
as a reviewer to the patches.