On (19/04/16 11:24), Jakub Hrozek wrote:
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.
I verified that change was only coding style related.
Therefore Jakub's ACK still apply :-)
master:
* b3ca35780617b2e5a7637f9888b089e8e26a4e8c
* 15d41c8f28259061e39715acdbbbaea778b6ecc8
* 52300e30a0ec0bbfa1b0918ee0b495f06f5d142a
* 536dcc7fb975acfc126846a889d90332304e88ba
LS