[SSSD] [PATCH] Read and validate configuration before daemonizing
Stephen Gallagher
sgallagh at redhat.com
Thu Sep 10 20:18:07 UTC 2009
New patch attached, comments inline.
On 09/10/2009 11:39 AM, Simo Sorce wrote:
> On Thu, 2009-09-10 at 11:11 -0400, Stephen Gallagher wrote:
>> Attached is a new approach to reading the configuration and exiting
>> with
>> an error code if the configuration is invalid. There should be no
>> situation where this will affect signal/process setup anymore.
>>
>> Patch 0001: Simple cleanup. We're now using a private event context
>> for
>> the confdb and ignoring the event_ctx argument to confdb_init, so I'm
>> removing it to eliminate confusion.
>
> Ack this one.
>
>> Patch 0002: Create the confdb and read in the configuration options
>> before daemonizing so that failures can be reported immediately.
>
> Patche is going in the right direction.
> Comments inline.
>
>
>
>> +static errno_t load_configuration(const char *config_file,
>> + struct mt_ctx **monitor)
>> +{
>> + errno_t ret;
>> + struct mt_ctx *ctx;
>> + char *cdb_file = NULL;
>> +
>> + ctx = talloc_zero(NULL, struct mt_ctx);
>
> Please pass in a memory context to this function, don't use NULL.
>
I added a tmp_ctx to the main() function that will act as the parent of
this (and the config_file string) until it's stolen.
>> int monitor_process_init(TALLOC_CTX *mem_ctx,
>> - struct tevent_context *event_ctx,
>> - struct confdb_ctx *cdb,
>> + struct mt_ctx *ctx,
>> const char *config_file)
>> {
>
> mem_ctx is not used anymore in this function as far as I can see,
> please remove it.
It is still used for sysdb_init, so I left it there.
>
>
>> - struct mt_ctx *ctx;
>> @@ -2332,6 +2343,7 @@ int main(int argc, const char *argv[])
>> char *config_file = NULL;
>> int flags = 0;
>> struct main_context *main_ctx;
>> + struct mt_ctx *monitor;
>> int ret;
>>
>> struct poptOption long_options[] = {
>> @@ -2379,16 +2391,18 @@ int main(int argc, const char *argv[])
>> flags |= FLAGS_PID_FILE;
>>
>> /* Parse config file, fail if cannot be done */
>> - ret = read_config_file(config_file);
>> + ret = load_configuration(config_file, &monitor);
>> if (ret != EOK) return 4;
>>
>> /* set up things like debug , signals, daemonization, etc... */
>> ret = server_setup("sssd", flags, MONITOR_CONF_ENTRY, &main_ctx);
>> if (ret != EOK) return 2;
>>
>> + monitor->ev = main_ctx->event_ctx;
>> + talloc_steal(main_ctx, monitor);
>> +
>
> It would be nice to refactor the code to avoid this steal here, although
> that may prove to be a bit complicated.
> One way could be to allocate main_ctx outside of server_setup() before
> load_configuration() is called, and make it not be a child of the event
> context.
> Although I wouldn't consider a decision not to to this as reason to
> nack.
It's a fairly complicated change to do that. I think it deserves its own
patch somewhere down the road.
>
> The rest looks good.
>
> Simo.
>
--
Stephen Gallagher
RHCE 804006346421761
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-Remove-unused-event-context-argument-from-confdb_ini.patch
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20090910/7e5b5d4b/attachment.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0002-Read-the-configuration-parsing-before-daemonization.patch
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20090910/7e5b5d4b/attachment-0001.ksh>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 260 bytes
Desc: OpenPGP digital signature
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20090910/7e5b5d4b/attachment.sig>
More information about the sssd-devel
mailing list