[PATCH] - SIGUSR2 should force SSSD to reread resolv.conf as well

[Ariel Barria]

https://fedorahosted.org/sssd/ticket/1045

[Dmitri Pal]

On 06/15/2012 01:29 PM, Ariel Barria wrote:
> https://fedorahosted.org/sssd/ticket/1045
>
> sssd-devel mailing list
> [e-mail]
> https://fedorahosted.org/mailman/listinfo/sssd-devel

Just a quick glance at the patch suggest that you have some issues with
spaces/tabs. Please correct.

[Stephen Gallagher]

On Fri, 2012-06-15 at 12:29 -0500, Ariel Barria wrote:
> https://fedorahosted.org/sssd/ticket/1045

Nack. Did you test this?

The function monitor_config_file() just adds a file to the list of files
that are being monitored. What you need to do is call to signal all of
the monitor's children to perform a res_init() with
service_signal_res_init().

Also, as Dmitri mentioned, *please* fix the whitespace issues. We use
four spaces for indentation. No tabs, please.

[Ariel Barria]

sorry if I made ​​them waste time.
 Well after some drawbacks to create the test environment seems to be already.
> Nack. Did you test this?
> The function monitor_config_file() just adds a file to the list of files
> that are being monitored. What you need to do is call to signal all of
> the monitor's children to perform a res_init() with
> service_signal_res_init().

yes, I not knew some features in vimrc.
I added these revisions :)
> Also, as Dmitri mentioned, *please* fix the whitespace issues. We use
> four spaces for indentation. No tabs, please.

_______________________________________________
sssd-devel mailing list
[e-mail]
https://fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Mon, Sep 03, 2012 at 12:14:53AM -0500, Ariel Barria wrote:
> sorry if I made ​​them waste time.
>  Well after some drawbacks to create the test environment seems to be already.
> > Nack. Did you test this?
> > The function monitor_config_file() just adds a file to the list of files
> > that are being monitored. What you need to do is call to signal all of
> > the monitor's children to perform a res_init() with
> > service_signal_res_init().
>
> yes, I not knew some features in vimrc.
> I added these revisions :)
> > Also, as Dmitri mentioned, *please* fix the whitespace issues. We use
> > four spaces for indentation. No tabs, please.

Much better, Ariel, thank you!

I have two very minor requests to make the patch more readable:

1) I think you can reuse service_signal_dns_reload instead of the
       new function service_signal_res_init, the two functions are
       identical. It is good practice to keep the code duplication as low
       as possible.

2) I don't understand why you pass the private void pointer to
       signal_res_init() instead of passing struct mt_ctx * directly?
       Can you either pass the monitor context directly or fold
       signal_res_init() into signal_offline_reset()

Functionally, the patch is fine. Thanks again.

[Ariel Barria]

> Date: Mon, 3 Sep 2012 17:16:53 +0200
> From: [e-mail]
> To: [e-mail]
> Subject: Re: [SSSD] [PATCH] - SIGUSR2 should force SSSD to reread resolv.conf as well
>
> On Mon, Sep 03, 2012 at 12:14:53AM -0500, Ariel Barria wrote:
> > sorry if I made ​​them waste time.
> >  Well after some drawbacks to create the test environment seems to be already.
> > > Nack. Did you test this?
> > > The function monitor_config_file() just adds a file to the list of files
> > > that are being monitored. What you need to do is call to signal all of
> > > the monitor's children to perform a res_init() with
> > > service_signal_res_init().
> >
> > yes, I not knew some features in vimrc.
> > I added these revisions :)
> > > Also, as Dmitri mentioned, *please* fix the whitespace issues. We use
> > > four spaces for indentation. No tabs, please.
>
> Much better, Ariel, thank you!
>
> I have two very minor requests to make the patch more readable:
> 1) I think you can reuse service_signal_dns_reload instead of the
>    new function service_signal_res_init, the two functions are
>    identical. It is good practice to keep the code duplication as low
>    as possible.

done.
> 2) I don't understand why you pass the private void pointer to
>    signal_res_init() instead of passing struct mt_ctx * directly?
>    Can you either pass the monitor context directly or fold
>    signal_res_init() into signal_offline_reset()

I spent monitor context directly.
> Functionally, the patch is fine. Thanks again.

thank to you for your comment.
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Michal Židek]

On 09/05/2012 03:16 AM, Ariel Barria wrote:
> > Date: Mon, 3 Sep 2012 17:16:53 +0200
> > From: [e-mail]
> > To: [e-mail]
> > Subject: Re: [SSSD] [PATCH] - SIGUSR2 should force SSSD to reread
>
> resolv.conf as well
> > On Mon, Sep 03, 2012 at 12:14:53AM -0500, Ariel Barria wrote:
> > > sorry if I made ​​them waste time.
> > > Well after some drawbacks to create the test environment seems to
>
> be already.
> > > > Nack. Did you test this?
> > > > The function monitor_config_file() just adds a file to the list
>
> of files
> > > > that are being monitored. What you need to do is call to signal
>
> all of
> > > > the monitor's children to perform a res_init() with
> > > > service_signal_res_init().
> > >
> > > yes, I not knew some features in vimrc.
> > > I added these revisions :)
> > > > Also, as Dmitri mentioned, *please* fix the whitespace issues. We use
> > > > four spaces for indentation. No tabs, please.
> >
> > Much better, Ariel, thank you!
> >
> > I have two very minor requests to make the patch more readable:
> > I think you can reuse service_signal_dns_reload instead of the
> >
> > new function service_signal_res_init, the two functions are
> > identical. It is good practice to keep the code duplication as low
> > as possible.
>
> done.
> > I don't understand why you pass the private void pointer to
> >
> > signal_res_init() instead of passing struct mt_ctx * directly?
> > Can you either pass the monitor context directly or fold
> > signal_res_init() into signal_offline_reset()
>
> I spent monitor context directly.
> > Functionally, the patch is fine. Thanks again.
>
> thank to you for your comment.
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Looks good to me.
Ack.

Michal

[Jakub Hrozek]

On Wed, Sep 05, 2012 at 01:21:31PM +0200, Michal Židek wrote:
> On 09/05/2012 03:16 AM, Ariel Barria wrote:
> > > Date: Mon, 3 Sep 2012 17:16:53 +0200
> > > From: [e-mail]
> > > To: [e-mail]
> > > Subject: Re: [SSSD] [PATCH] - SIGUSR2 should force SSSD to
> >
> > reread resolv.conf as well
> > > On Mon, Sep 03, 2012 at 12:14:53AM -0500, Ariel Barria wrote:
> > > > sorry if I made ​​them waste time.
> > > > Well after some drawbacks to create the test environment seems
> >
> > to be already.
> > > > > Nack. Did you test this?
> > > > > The function monitor_config_file() just adds a file to the
> >
> > list of files
> > > > > that are being monitored. What you need to do is call to
> >
> > signal all of
> > > > > the monitor's children to perform a res_init() with
> > > > > service_signal_res_init().
> > > >
> > > > yes, I not knew some features in vimrc.
> > > > I added these revisions :)
> > > > > Also, as Dmitri mentioned, *please* fix the whitespace issues. We use
> > > > > four spaces for indentation. No tabs, please.
> > >
> > > Much better, Ariel, thank you!
> > >
> > > I have two very minor requests to make the patch more readable:
> > > I think you can reuse service_signal_dns_reload instead of the
> > >
> > > new function service_signal_res_init, the two functions are
> > > identical. It is good practice to keep the code duplication as low
> > > as possible.
> >
> > done.
> > > I don't understand why you pass the private void pointer to
> > >
> > > signal_res_init() instead of passing struct mt_ctx * directly?
> > > Can you either pass the monitor context directly or fold
> > > signal_res_init() into signal_offline_reset()
> >
> > I spent monitor context directly.
> > > Functionally, the patch is fine. Thanks again.
> >
> > thank to you for your comment.
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> Looks good to me.
> Ack.
>
> Michal

Pushed to master.