[PATCH] dyndns: do not modify global family_order

[Sumit Bose]

Hi,

this issue was discovered by a user and as far as I can see it was
always there. Feel free to apply it to older branches as well.

bye,
Sumit

[Pavel Březina]

On 09/03/2013 12:57 PM, Sumit Bose wrote:
> Hi,
>
> this issue was discovered by a user and as far as I can see it was
> always there. Feel free to apply it to older branches as well.
>
> bye,
> Sumit

Hi,
> /* If the resolver is set to honor both address families
>  * and the first one matched, retry the second one to
>  * get the complete list.
>  */

I haven't tried it, but since you don't change the global family, 
wouldn't this always (if not found) hit the condition and invoke new lookup?
> /* If the resolver is set to honor both address families
>  * and the first one matched, retry the second one to
>  * get the complete list.
>  */
> if (((state->be_res->family_order == IPV4_FIRST &&
>       rhostent->family == AF_INET) ||
>     (state->be_res->family_order == IPV6_FIRST &&
>      rhostent->family == AF_INET6))) {
>
>     retry_family_order = (state->be_res->family_order == IPV4_FIRST) ? \
>                          IPV6_ONLY : \
>                          IPV4_ONLY;
>
>     subreq = resolv_gethostbyname_send(state, state->ev,
>                                        state->be_res->resolv,
>                                        state->hostname,
>                                        retry_family_order,
>                                        state->db);
>     if (!subreq) {
>         ret = ENOMEM;
>         goto done;
>     }
>     tevent_req_set_callback(subreq, nsupdate_get_addrs_done, req);
>     return;
> }

[Sumit Bose]

On Tue, Sep 03, 2013 at 01:22:22PM +0200, Pavel Březina wrote:
> On 09/03/2013 12:57 PM, Sumit Bose wrote:
> > Hi,
> >
> > this issue was discovered by a user and as far as I can see it was
> > always there. Feel free to apply it to older branches as well.
> >
> > bye,
> > Sumit
>
> Hi,
> > /* If the resolver is set to honor both address families
> >     * and the first one matched, retry the second one to
> >     * get the complete list.
> >     */
>
> I haven't tried it, but since you don't change the global family,
> wouldn't this always (if not found) hit the condition and invoke new
> lookup?

Thank you for the comment, it reminded me to check if the setup where I
tested the patch really uses both families, which it doesn't. I will
recheck in a more suitable environment and send a comment.

But nevertheless it should work, because not only the value of the
family_order is checked, but the family of the last result as well. So
if I read it correctly the if statement below can never be true for the
second run.

bye,
Sumit
> > /* If the resolver is set to honor both address families
> >     * and the first one matched, retry the second one to
> >     * get the complete list.
> >     */
> >    if (((state->be_res->family_order == IPV4_FIRST &&
> >          rhostent->family == AF_INET) ||
> >        (state->be_res->family_order == IPV6_FIRST &&
> >         rhostent->family == AF_INET6))) {
> >    retry_family_order = (state->be_res->family_order == IPV4_FIRST) ? \
> >                         IPV6_ONLY : \
> >                         IPV4_ONLY;
> >
> >    subreq = resolv_gethostbyname_send(state, state->ev,
> >                                       state->be_res->resolv,
> >                                       state->hostname,
> >                                       retry_family_order,
> >                                       state->db);
> >    if (!subreq) {
> >        ret = ENOMEM;
> >        goto done;
> >    }
> >    tevent_req_set_callback(subreq, nsupdate_get_addrs_done, req);
> >    return;
> >
> > }
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Pavel Březina]

On 09/03/2013 01:36 PM, Sumit Bose wrote:
> On Tue, Sep 03, 2013 at 01:22:22PM +0200, Pavel Březina wrote:
> > On 09/03/2013 12:57 PM, Sumit Bose wrote:
> > > Hi,
> > >
> > > this issue was discovered by a user and as far as I can see it was
> > > always there. Feel free to apply it to older branches as well.
> > >
> > > bye,
> > > Sumit
> >
> > Hi,
> > > /* If the resolver is set to honor both address families
> > >  * and the first one matched, retry the second one to
> > >  * get the complete list.
> > >  */
> >
> > I haven't tried it, but since you don't change the global family,
> > wouldn't this always (if not found) hit the condition and invoke new
> > lookup?
>
> Thank you for the comment, it reminded me to check if the setup where I
> tested the patch really uses both families, which it doesn't. I will
> recheck in a more suitable environment and send a comment.
>
> But nevertheless it should work, because not only the value of the
> family_order is checked, but the family of the last result as well. So
> if I read it correctly the if statement below can never be true for the
> second run.

Ah, you are right.
> bye,
> Sumit
> > > /* If the resolver is set to honor both address families
> > >  * and the first one matched, retry the second one to
> > >  * get the complete list.
> > >  */
> > > if (((state->be_res->family_order == IPV4_FIRST &&
> > >       rhostent->family == AF_INET) ||
> > >     (state->be_res->family_order == IPV6_FIRST &&
> > >      rhostent->family == AF_INET6))) {
> > >
> > >     retry_family_order = (state->be_res->family_order == IPV4_FIRST) ? \
> > >                          IPV6_ONLY : \
> > >                          IPV4_ONLY;
> > >
> > >     subreq = resolv_gethostbyname_send(state, state->ev,
> > >                                        state->be_res->resolv,
> > >                                        state->hostname,
> > >                                        retry_family_order,
> > >                                        state->db);
> > >     if (!subreq) {
> > >         ret = ENOMEM;
> > >         goto done;
> > >     }
> > >     tevent_req_set_callback(subreq, nsupdate_get_addrs_done, req);
> > >     return;
> > > }

[Pavel Březina]

On 09/03/2013 01:36 PM, Sumit Bose wrote:
> On Tue, Sep 03, 2013 at 01:22:22PM +0200, Pavel Březina wrote:
> > On 09/03/2013 12:57 PM, Sumit Bose wrote:
> > > Hi,
> > >
> > > this issue was discovered by a user and as far as I can see it was
> > > always there. Feel free to apply it to older branches as well.
> > >
> > > bye,
> > > Sumit
> >
> > Hi,
> > > /* If the resolver is set to honor both address families
> > >  * and the first one matched, retry the second one to
> > >  * get the complete list.
> > >  */
> >
> > I haven't tried it, but since you don't change the global family,
> > wouldn't this always (if not found) hit the condition and invoke new
> > lookup?
>
> Thank you for the comment, it reminded me to check if the setup where I
> tested the patch really uses both families, which it doesn't. I will
> recheck in a more suitable environment and send a comment.
>
> But nevertheless it should work, because not only the value of the
> family_order is checked, but the family of the last result as well. So
> if I read it correctly the if statement below can never be true for the
> second run.
>
> bye,
> Sumit
> > > /* If the resolver is set to honor both address families
> > >  * and the first one matched, retry the second one to
> > >  * get the complete list.
> > >  */
> > > if (((state->be_res->family_order == IPV4_FIRST &&
> > >       rhostent->family == AF_INET) ||
> > >     (state->be_res->family_order == IPV6_FIRST &&
> > >      rhostent->family == AF_INET6))) {
> > >
> > >     retry_family_order = (state->be_res->family_order == IPV4_FIRST) ? \
> > >                          IPV6_ONLY : \
> > >                          IPV4_ONLY;
> > >
> > >     subreq = resolv_gethostbyname_send(state, state->ev,
> > >                                        state->be_res->resolv,
> > >                                        state->hostname,
> > >                                        retry_family_order,
> > >                                        state->db);
> > >     if (!subreq) {
> > >         ret = ENOMEM;
> > >         goto done;
> > >     }
> > >     tevent_req_set_callback(subreq, nsupdate_get_addrs_done, req);
> > >     return;
> > > }

Hi,
I've just tried it and it works as expected.

ACK

[Jakub Hrozek]

On Wed, Sep 04, 2013 at 10:16:01AM +0200, Pavel Březina wrote:
> On 09/03/2013 01:36 PM, Sumit Bose wrote:
> > On Tue, Sep 03, 2013 at 01:22:22PM +0200, Pavel Březina wrote:
> > > On 09/03/2013 12:57 PM, Sumit Bose wrote:
> > > > Hi,
> > > >
> > > > this issue was discovered by a user and as far as I can see it was
> > > > always there. Feel free to apply it to older branches as well.
> > > >
> > > > bye,
> > > > Sumit
> > >
> > > Hi,
> > > > /* If the resolver is set to honor both address families
> > > >     * and the first one matched, retry the second one to
> > > >     * get the complete list.
> > > >     */
> > >
> > > I haven't tried it, but since you don't change the global family,
> > > wouldn't this always (if not found) hit the condition and invoke new
> > > lookup?
> >
> > Thank you for the comment, it reminded me to check if the setup where I
> > tested the patch really uses both families, which it doesn't. I will
> > recheck in a more suitable environment and send a comment.
> >
> > But nevertheless it should work, because not only the value of the
> > family_order is checked, but the family of the last result as well. So
> > if I read it correctly the if statement below can never be true for the
> > second run.
> >
> > bye,
> > Sumit
> > > > /* If the resolver is set to honor both address families
> > > >     * and the first one matched, retry the second one to
> > > >     * get the complete list.
> > > >     */
> > > >    if (((state->be_res->family_order == IPV4_FIRST &&
> > > >          rhostent->family == AF_INET) ||
> > > >        (state->be_res->family_order == IPV6_FIRST &&
> > > >         rhostent->family == AF_INET6))) {
> > > >    retry_family_order = (state->be_res->family_order == IPV4_FIRST) ? \
> > > >                         IPV6_ONLY : \
> > > >                         IPV4_ONLY;
> > > >
> > > >    subreq = resolv_gethostbyname_send(state, state->ev,
> > > >                                       state->be_res->resolv,
> > > >                                       state->hostname,
> > > >                                       retry_family_order,
> > > >                                       state->db);
> > > >    if (!subreq) {
> > > >        ret = ENOMEM;
> > > >        goto done;
> > > >    }
> > > >    tevent_req_set_callback(subreq, nsupdate_get_addrs_done, req);
> > > >    return;
> > > >
> > > > }
>
> Hi,
> I've just tried it and it works as expected.
>
> ACK

Pushed to master and sssd-1-10