sssctl: Flags for commadn initialization

[mzidek-gh]

Allow passing flags for command specific
initialization. Currently only one flag is
available to skip the confdb initialization
which is required to improce config-check command.

Resolves:
https://fedorahosted.org/sssd/ticket/3209

[mzidek-gh]

Hi, connecting to confdb makes some sssctl commands less useful (config-check and in the future, config-show). This adds flag to skip confdb init for sssctl commands.

[fidencio]

There are a few typos in the commit message:

commadn -> command
improce -> improve

Also, the template is not followed as you're breaking the lines with way less than 72 chars.

[fidencio]

Just a minor comment (and I know it was like this before ...), but could you align all parameters to the first one in case of new line?
I'd just go for:

    DEBUG(SSSDBG_CRIT_FAILURE, "Unable to open confdb [%d]: %s\n",
          ret, sss_strerror(ret));

(OTOH, as it was there before, feel free to ignore my comment ;-))

[fidencio]

Same comment here ...

[fidencio]

And here as well ...

[fidencio]

No need to add the extra (blank) line here.

[mzidek-gh]

Updated patch according to Fabiano's comments.

[fidencio]

Really a nitpick here and sorry for not catching this one in the first review.
Please, compare against EOK instead of 0 (this change can be done before pushing, actually, no need to re-send the patch just because of this).

I'll run a round of tests manually and run the CI just to be sure it doesn't break anything before officially giving the ACK.

[mzidek-gh]

updated

[lslebodn]

IMHO, it would be nicer if you add new macro SSS_TOOL_COMMAND_FLAGS (or different name)
rather then changing usage of SSS_TOOL_COMMAND and SSS_TOOL_COMMAND_NOMSG on all places.

[mzidek-gh]

Ok, new patch uploaded.

[lslebodn]

Sorry I should have notice it earlier, but this flag might be a little bit confusing.

The function tool_cmd_init does not initialize only confdb but also tool_ctx->domains and tool_ctx->default_domain. I know that we cannot initialize domains without confdb but
the name of flag should be crystal clear and should not any cause confusion.

[mzidek-gh]

Sorry, I am out of ideas here. What name do you propose? Or will it be enough if I just add a comment to the flag that it will not initialize the domain context?

[mzidek-gh]

I see the comment did not get forwarded to the devel list, so pasting again:

Sorry, I am out of ideas here. What name do you propose? Or will it be enough if I just add a comment to the flag that it will not initialize the domain context?

[jhrozek]

On Fri, Oct 14, 2016 at 08:03:20AM -0700, mzidek-rh wrote:

I see the comment did not get forwarded to the devel list, so pasting again:

Sorry, I am out of ideas here. What name do you propose? Or will it be enough if I just add a comment to the flag that it will not initialize the domain context?

The review comments added to the github review tool do not make it to
the list. Unfortunately github doesn't offer an API to catch the review
comments..

[lslebodn]

On (14/10/16 08:03), mzidek-rh wrote:

I see the comment did not get forwarded to the devel list, so pasting again:

Sorry, I am out of ideas here. What name do you propose? Or will it be enough if I just add a comment to the flag that it will not initialize the domain context? I am out of ideas too but SSS_TOOL_CMD_FLAG_NO_CONFDB is confusing atm. I am preatty sure you will come up with a right name.

LS

[jhrozek]

I don't know how to unstuck this PR except providing some ideas

• SSS_TOOL_FLAG_NOCONF
• SSS_TOOL_FLAG_STATIC
• SSS_TOOL_FLAG_CONFPARSE_FALSE

[mzidek-gh]

I used the jhrozek's proposed *_NOCONF name. I also added comment to make it clear that it will not initialize both confdb and domain context.

[lslebodn]

On (17/10/16 09:33), Jakub Hrozek wrote:

I don't know how to unstuck this PR except providing some ideas

• SSS_TOOL_FLAG_NOCONF
• SSS_TOOL_FLAG_STATIC
• SSS_TOOL_FLAG_CONFPARSE_FALSE

Let me explain how I read code of the function tool_cmd_init

+static int tool_cmd_init(struct sss_tool_ctx *tool_ctx,

•                     struct sss_route_cmd *command)
  

  +{
• int ret;

• if (command->flags & SSS_TOOL_FLAG_NOCONF) {

•    /\* This tool does not need to connect to confdb or
  

•     \* initialize the domain contexts. Nothing to do. */
  

•    return EOK;
  

The function tool_cmd_init has "init" in a name so I assume
it want to do some initialisation. The first action in the function
is checking "some flag". If the flag is enabled then the execution
is immediatelly finished with success.

Based on the name of flag SSS_TOOL_FLAG_NOCONF. It seems that
there should be some configuration done in this function which
should be skipped. However, this function does not do any configuration.

The previous name was SSS_TOOL_CMD_FLAG_NO_CONFDB which looks like
just confd shoudl not be initialized. But tool_cmd_init initialize two things
and can initialize more things in future. And it isn't important
that initialization of domain depends on confdb because it can change
in future.

If you would like to use the name of option SSS_TOOL_CMD_FLAG_NO_CONFDB
The code should look:

if ((command->flags & SSS_TOOL_FLAG_NOCONF) == 0) {
//initialisation of confdb
}

//further initialisation.

But your code loks like
if (command->flags & SSS_TOOL_FLAG_WITH_CONFUSING_NAME) {
//immediatelly finish
}

//initialisation of confdb

//initialisation of domains

http://martinfowler.com/bliki/TwoHardThings.html

[mzidek-gh]

Is it really that confusing even with the comments added?
Well, I immediately return from the function when the flag is set, because the function currently only initializes the confdb and the domain context. Of course it is possible that the function will do also something else in the future, but is not doing so now, so I do not see reason to put more code to the if-statement.
I can change name of the function or flag (or any name) if you believe it will help readability, but please provide suggestions.

[lslebodn]

We(mzidek, lslebodn) tried some feature for github. "Allow edits from maintainers."

When you are creating a new pull request, you'll see a checkbox
labelled "Allow edits from maintainers". This is enabled by default.

With this in place, anyone with commit access to the repository that
is the target of the pull request will also be able to push changes
to the branch of the repository that is the origin of the pull
request.

But information about this changes does not appear in the conversation itself.

Anyway we renamed the flag into SSS_TOOL_FLAG_SKIP_CMD_INIT

[lslebodn]

master:

• cbee11e

sssd-1-14:

• ec1829d

LS

[lslebodn]

ACK

[lslebodn]

master:

• cbee11e

sssd-1-14:

• ec1829d

LS