[PATCH] AD SRV plugin

[Pavel Březina]

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

Here it is.

[Pavel Březina]

On 04/17/2013 02:44 PM, Pavel Březina wrote:
> https://fedorahosted.org/sssd/ticket/1032
>
> Here it is.

This is new patch set that addresses few issues raised by Jakub via IRC.

The most important was that I should treat domain names as case
insensitive. So I changed strcmp() to strcasecmp() in
fo_discover_servers_primary_done() which was part of the the original
patch set and also included a new patch that makes the change in dns srv
plugin.

[Jakub Hrozek]

On Mon, Apr 29, 2013 at 06:58:41PM +0200, Pavel Březina wrote:
> On 04/17/2013 02:44 PM, Pavel Březina wrote:
> > https://fedorahosted.org/sssd/ticket/1032
> >
> > Here it is.
>
> This is new patch set that addresses few issues raised by Jakub via IRC.
>
> The most important was that I should treat domain names as case
> insensitive. So I changed strcmp() to strcasecmp() in
> fo_discover_servers_primary_done() which was part of the the original
> patch set and also included a new patch that makes the change in dns srv
> plugin.

tl;dr - looking mostly good. Let's fix some nitpicks and push the work
to master.

I performed testing using the dssite.msc MMC -- without any specific site, I
got redirected to the Default-First-Site-Name site, defining the site
got me redirected to the proper site.

[PATCH 1/7] add fo_discover_servers request
Mostly good, but I'd like to discuss returning backup servers as primary
servers. I haven't tested that particular case, but can you confirm that
if any records were added on the server that would resolve to primary
servers, the client would pick them up after the SRV query expires
without restarting the client completely?

Otherwise ack.

[PATCH 2/7] IPA SRV plugin: use fo_discover_servers request
Ack. I did a quick sanity test of the IPA DNS discovery and it still
works.

[PATCH 3/7] IPA SRV plugin: improve debugging
Ack

[PATCH 4/7] sdap: add sdap_connect_host request
I tested this patch by checking with netstat that there was only one
connection after the discovery used the request.

I just wonder if using "struct be_resolv_ctx" would make more sense rather
than using both "struct resolv_ctx" and "enum restrict_family". It would also
make sense to cancel the resolv request after "dns_resolver_timeout" passes
(and read its value from "struct be_resolv_ctx"), although I would be OK
with filing a ticket for that. This functionality is not needed for the beta.

[PATCH 5/7] add sss_ldap_filter_binary_value
Have you looked at ldap_encode_ndr_uint32() from Samba? At the very least
I would prefer if the name made it clear that the function is converting
uint32_t to char. FTR the current implementation of ldap_encode_ndr_uint32()
looks like:

char *ldap_encode_ndr_uint32(TALLOC_CTX *mem_ctx, uint32_t value)
{
    uint8_t buf[4];
    struct ldb_val val;
    SIVAL(buf, 0, value);
    val.data = buf;
    val.length = 4;
    return ldb_binary_encode(mem_ctx, val);
}

So what about a very similar sss_ldap_encode_ndr_uint32() ?

[PATCH 6/7] DNS sites support - add AD SRV plugin
This patch makes samba4-devel a hard requirement of the AD provider.
Given that we're considering dropping support of RHEL5 upstream now, I
don't think this should be a blocker in accepting these patches, but I
would prefer to file a ticket to discover if we can #ifdef the support
out to keep the support of older releases upstream.

It might not be practical after all given the other AD provider features
coming up in 1.10, but still, I think we should track the samba4-devel
requirement and decide on implementing or dropping the ifdefs later during
1.10.0 cycle when all the features are in.
> --- a/src/man/sssd-ad.5.xml
> +++ b/src/man/sssd-ad.5.xml
> @@ -130,6 +130,26 @@ ldap_id_mapping = False
>                          </para>
>                      </listitem>
>                  </varlistentry>
>            <varlistentry>
>                <term>ad_enable_dns_sites (boolean)</term>
>                <listitem>
>                    <para>
>                        Enables DNS sites - location based
>                        service discovery.
>                    </para>
>                    <para>
>                        If true and service discovery (see Service
>                        Discovery paragraph at the bottom of the man page)
>                        is enabled, then the SSSD will first attempt
>                        location based discovery using an Active Directory
>                        mechanism called Sites.

I was wondering for a while whether to explain a little about the AD
Sites, but the sssd-ad man page is probably not the right place..

Still, what about "the SSSD will first attempt to discover the Active
Directory server to connect to using the Active Directory Site
Discovery and fall back to the DNS SRV records if no AD site is found".

Defaulting to True is correct give the presence of the
Default-First-Site-Name site.

The ad_get_client_site_send() function could accept "struct
be_resolv_ctx".

Nitpick:
> +static void ad_get_client_site_done(struct tevent_req *subreq)
> +{
> struct ad_get_client_site_state *state = NULL;
> struct tevent_req *req = NULL;
> struct ldb_message_element *el = NULL;
> struct sysdb_attrs **reply = NULL;
> size_t reply_count;
> errno_t ret;
> req = tevent_req_callback_data(subreq, struct tevent_req);
> state = tevent_req_data(req, struct ad_get_client_site_state);
> ret = sdap_get_generic_recv(subreq, state, &reply_count, &reply);
> talloc_zfree(subreq);

Can you put a comment here stating that this is where the LDAP
connection that was used for the location discovery is closed?
> talloc_zfree(state->sh);
> if (ret != EOK) {

The rest looks good to me.

[PATCH 7/7] dns srv plugin: compare domain names case insensitive
Ack

[Pavel Březina]

On 04/30/2013 12:37 AM, Jakub Hrozek wrote:
> On Mon, Apr 29, 2013 at 06:58:41PM +0200, Pavel Březina wrote:
> > On 04/17/2013 02:44 PM, Pavel Březina wrote:
> > > https://fedorahosted.org/sssd/ticket/1032
> > >
> > > Here it is.
> >
> > This is new patch set that addresses few issues raised by Jakub via IRC.
> >
> > The most important was that I should treat domain names as case
> > insensitive. So I changed strcmp() to strcasecmp() in
> > fo_discover_servers_primary_done() which was part of the the original
> > patch set and also included a new patch that makes the change in dns srv
> > plugin.
>
> tl;dr - looking mostly good. Let's fix some nitpicks and push the work
> to master.
>
> I performed testing using the dssite.msc MMC -- without any specific site, I
> got redirected to the Default-First-Site-Name site, defining the site
> got me redirected to the proper site.
>
> [PATCH 1/7] add fo_discover_servers request
> Mostly good, but I'd like to discuss returning backup servers as primary
> servers. I haven't tested that particular case, but can you confirm that
> if any records were added on the server that would resolve to primary
> servers, the client would pick them up after the SRV query expires
> without restarting the client completely?

When the SRV fo_server collapse, all associated primary and backup 
servers are removed from the list. The plugin doesn't have any state 
from the previous run, so it will contact both primary and backup domain 
again.
> Otherwise ack.
>
> [PATCH 2/7] IPA SRV plugin: use fo_discover_servers request
> Ack. I did a quick sanity test of the IPA DNS discovery and it still
> works.
>
> [PATCH 3/7] IPA SRV plugin: improve debugging
> Ack
>
> [PATCH 4/7] sdap: add sdap_connect_host request
> I tested this patch by checking with netstat that there was only one
> connection after the discovery used the request.
>
> I just wonder if using "struct be_resolv_ctx" would make more sense rather
> than using both "struct resolv_ctx" and "enum restrict_family".

Hmm, the whole sdap async code doesn't have any knowledge of be what so 
ever and I'd stick with that.

It would also
> make sense to cancel the resolv request after "dns_resolver_timeout" passes
> (and read its value from "struct be_resolv_ctx"), although I would be OK
> with filing a ticket for that. This functionality is not needed for the beta.

If it takes too long then it will hit a timeout for service resolver in 
fail over, which already is dns_resolver_timeout.
> [PATCH 5/7] add sss_ldap_filter_binary_value
> Have you looked at ldap_encode_ndr_uint32() from Samba? At the very least
> I would prefer if the name made it clear that the function is converting
> uint32_t to char. FTR the current implementation of ldap_encode_ndr_uint32()
> looks like:
>
> char *ldap_encode_ndr_uint32(TALLOC_CTX *mem_ctx, uint32_t value)
> {
>      uint8_t buf[4];
>      struct ldb_val val;
>      SIVAL(buf, 0, value);
>      val.data = buf;
>      val.length = 4;
>      return ldb_binary_encode(mem_ctx, val);
> }
>
> So what about a very similar sss_ldap_encode_ndr_uint32() ?

Done.
> [PATCH 6/7] DNS sites support - add AD SRV plugin
> This patch makes samba4-devel a hard requirement of the AD provider.
> Given that we're considering dropping support of RHEL5 upstream now, I
> don't think this should be a blocker in accepting these patches, but I
> would prefer to file a ticket to discover if we can #ifdef the support
> out to keep the support of older releases upstream.
> It might not be practical after all given the other AD provider features
> coming up in 1.10, but still, I think we should track the samba4-devel
> requirement and decide on implementing or dropping the ifdefs later during
> 1.10.0 cycle when all the features are in.

If we decide to #ifdef it, it will be quite simple, I think.
> > --- a/src/man/sssd-ad.5.xml
> > +++ b/src/man/sssd-ad.5.xml
> > @@ -130,6 +130,26 @@ ldap_id_mapping = False
> >                           </para>
> >                       </listitem>
> >                   </varlistentry>
> >            <varlistentry>
> >                <term>ad_enable_dns_sites (boolean)</term>
> >                <listitem>
> >                    <para>
> >                        Enables DNS sites - location based
> >                        service discovery.
> >                    </para>
> >                    <para>
> >                        If true and service discovery (see Service
> >                        Discovery paragraph at the bottom of the man page)
> >                        is enabled, then the SSSD will first attempt
> >                        location based discovery using an Active Directory
> >                        mechanism called Sites.
>
> I was wondering for a while whether to explain a little about the AD
> Sites, but the sssd-ad man page is probably not the right place..

This is a place for google and MSDN IMHO.
> Still, what about "the SSSD will first attempt to discover the Active
> Directory server to connect to using the Active Directory Site
> Discovery and fall back to the DNS SRV records if no AD site is found".

Sounds good.
> Defaulting to True is correct give the presence of the
> Default-First-Site-Name site.
>
> The ad_get_client_site_send() function could accept "struct
> be_resolv_ctx".

Done.
> Nitpick:
> > +static void ad_get_client_site_done(struct tevent_req *subreq)
> > +{
> > struct ad_get_client_site_state *state = NULL;
> > struct tevent_req *req = NULL;
> > struct ldb_message_element *el = NULL;
> > struct sysdb_attrs **reply = NULL;
> > size_t reply_count;
> > errno_t ret;
> > req = tevent_req_callback_data(subreq, struct tevent_req);
> > state = tevent_req_data(req, struct ad_get_client_site_state);
> > ret = sdap_get_generic_recv(subreq, state, &reply_count, &reply);
> > talloc_zfree(subreq);
>
> Can you put a comment here stating that this is where the LDAP
> connection that was used for the location discovery is closed?

Done.
> > talloc_zfree(state->sh);
> > if (ret != EOK) {
>
> The rest looks good to me.
>
> [PATCH 7/7] dns srv plugin: compare domain names case insensitive
> Ack

Thank you for the review.

[Pavel Březina]

On 04/30/2013 04:00 PM, Pavel Březina wrote:
> On 04/30/2013 12:37 AM, Jakub Hrozek wrote:
> > On Mon, Apr 29, 2013 at 06:58:41PM +0200, Pavel Březina wrote:
> > > On 04/17/2013 02:44 PM, Pavel Březina wrote:
> > > > https://fedorahosted.org/sssd/ticket/1032
> > > >
> > > > Here it is.
> > >
> > > This is new patch set that addresses few issues raised by Jakub via IRC.
> > >
> > > The most important was that I should treat domain names as case
> > > insensitive. So I changed strcmp() to strcasecmp() in
> > > fo_discover_servers_primary_done() which was part of the the original
> > > patch set and also included a new patch that makes the change in dns srv
> > > plugin.
> >
> > tl;dr - looking mostly good. Let's fix some nitpicks and push the work
> > to master.
> >
> > I performed testing using the dssite.msc MMC -- without any specific
> > site, I
> > got redirected to the Default-First-Site-Name site, defining the site
> > got me redirected to the proper site.
> >
> > [PATCH 1/7] add fo_discover_servers request
> > Mostly good, but I'd like to discuss returning backup servers as primary
> > servers. I haven't tested that particular case, but can you confirm that
> > if any records were added on the server that would resolve to primary
> > servers, the client would pick them up after the SRV query expires
> > without restarting the client completely?
>
> When the SRV fo_server collapse, all associated primary and backup
> servers are removed from the list. The plugin doesn't have any state
> from the previous run, so it will contact both primary and backup domain
> again.
> > Otherwise ack.
> >
> > [PATCH 2/7] IPA SRV plugin: use fo_discover_servers request
> > Ack. I did a quick sanity test of the IPA DNS discovery and it still
> > works.
> >
> > [PATCH 3/7] IPA SRV plugin: improve debugging
> > Ack
> >
> > [PATCH 4/7] sdap: add sdap_connect_host request
> > I tested this patch by checking with netstat that there was only one
> > connection after the discovery used the request.
> >
> > I just wonder if using "struct be_resolv_ctx" would make more sense
> > rather
> > than using both "struct resolv_ctx" and "enum restrict_family".
>
> Hmm, the whole sdap async code doesn't have any knowledge of be what so
> ever and I'd stick with that.
>
> It would also
> > make sense to cancel the resolv request after "dns_resolver_timeout"
> > passes
> > (and read its value from "struct be_resolv_ctx"), although I would be OK
> > with filing a ticket for that. This functionality is not needed for
> > the beta.
>
> If it takes too long then it will hit a timeout for service resolver in
> fail over, which already is dns_resolver_timeout.
> > [PATCH 5/7] add sss_ldap_filter_binary_value
> > Have you looked at ldap_encode_ndr_uint32() from Samba? At the very least
> > I would prefer if the name made it clear that the function is converting
> > uint32_t to char. FTR the current implementation of
> > ldap_encode_ndr_uint32()
> > looks like:
> >
> > char *ldap_encode_ndr_uint32(TALLOC_CTX *mem_ctx, uint32_t value)
> > {
> >      uint8_t buf[4];
> >      struct ldb_val val;
> >      SIVAL(buf, 0, value);
> >      val.data = buf;
> >      val.length = 4;
> >      return ldb_binary_encode(mem_ctx, val);
> > }
> >
> > So what about a very similar sss_ldap_encode_ndr_uint32() ?
>
> Done.
> > [PATCH 6/7] DNS sites support - add AD SRV plugin
> > This patch makes samba4-devel a hard requirement of the AD provider.
> > Given that we're considering dropping support of RHEL5 upstream now, I
> > don't think this should be a blocker in accepting these patches, but I
> > would prefer to file a ticket to discover if we can #ifdef the support
> > out to keep the support of older releases upstream.
> > It might not be practical after all given the other AD provider features
> > coming up in 1.10, but still, I think we should track the samba4-devel
> > requirement and decide on implementing or dropping the ifdefs later
> > during
> > 1.10.0 cycle when all the features are in.
>
> If we decide to #ifdef it, it will be quite simple, I think.
> > > --- a/src/man/sssd-ad.5.xml
> > > +++ b/src/man/sssd-ad.5.xml
> > > @@ -130,6 +130,26 @@ ldap_id_mapping = False
> > >                           </para>
> > >                       </listitem>
> > >                   </varlistentry>
> > >            <varlistentry>
> > >                <term>ad_enable_dns_sites (boolean)</term>
> > >                <listitem>
> > >                    <para>
> > >                        Enables DNS sites - location based
> > >                        service discovery.
> > >                    </para>
> > >                    <para>
> > >                        If true and service discovery (see Service
> > >                        Discovery paragraph at the bottom of the
> > >
> > > man page)
> > >                        is enabled, then the SSSD will first
> > >
> > > attempt
> > >                        location based discovery using an Active
> > >
> > > Directory
> > >                        mechanism called Sites.
> >
> > I was wondering for a while whether to explain a little about the AD
> > Sites, but the sssd-ad man page is probably not the right place..
>
> This is a place for google and MSDN IMHO.
> > Still, what about "the SSSD will first attempt to discover the Active
> > Directory server to connect to using the Active Directory Site
> > Discovery and fall back to the DNS SRV records if no AD site is found".
>
> Sounds good.
> > Defaulting to True is correct give the presence of the
> > Default-First-Site-Name site.
> >
> > The ad_get_client_site_send() function could accept "struct
> > be_resolv_ctx".
>
> Done.
> > Nitpick:
> > > +static void ad_get_client_site_done(struct tevent_req *subreq)
> > > +{
> > > struct ad_get_client_site_state *state = NULL;
> > > struct tevent_req *req = NULL;
> > > struct ldb_message_element *el = NULL;
> > > struct sysdb_attrs **reply = NULL;
> > > size_t reply_count;
> > > errno_t ret;
> > > req = tevent_req_callback_data(subreq, struct tevent_req);
> > > state = tevent_req_data(req, struct ad_get_client_site_state);
> > > ret = sdap_get_generic_recv(subreq, state, &reply_count, &reply);
> > > talloc_zfree(subreq);
> >
> > Can you put a comment here stating that this is where the LDAP
> > connection that was used for the location discovery is closed?
>
> Done.
> > > talloc_zfree(state->sh);
> > > if (ret != EOK) {
> >
> > The rest looks good to me.
> >
> > [PATCH 7/7] dns srv plugin: compare domain names case insensitive
> > Ack
>
> Thank you for the review.

Ups, I forgot to commit last changes.

[Jakub Hrozek]

On Tue, Apr 30, 2013 at 04:21:32PM +0200, Pavel Březina wrote:
> On 04/30/2013 04:00 PM, Pavel Březina wrote:
> > On 04/30/2013 12:37 AM, Jakub Hrozek wrote:
> > > On Mon, Apr 29, 2013 at 06:58:41PM +0200, Pavel Březina wrote:
> > > > On 04/17/2013 02:44 PM, Pavel Březina wrote:
> > > > > https://fedorahosted.org/sssd/ticket/1032
> > > > >
> > > > > Here it is.
> > > >
> > > > This is new patch set that addresses few issues raised by Jakub via IRC.
> > > >
> > > > The most important was that I should treat domain names as case
> > > > insensitive. So I changed strcmp() to strcasecmp() in
> > > > fo_discover_servers_primary_done() which was part of the the original
> > > > patch set and also included a new patch that makes the change in dns srv
> > > > plugin.
> > >
> > > tl;dr - looking mostly good. Let's fix some nitpicks and push the work
> > > to master.
> > >
> > > I performed testing using the dssite.msc MMC -- without any specific
> > > site, I
> > > got redirected to the Default-First-Site-Name site, defining the site
> > > got me redirected to the proper site.
> > >
> > > [PATCH 1/7] add fo_discover_servers request
> > > Mostly good, but I'd like to discuss returning backup servers as primary
> > > servers. I haven't tested that particular case, but can you confirm that
> > > if any records were added on the server that would resolve to primary
> > > servers, the client would pick them up after the SRV query expires
> > > without restarting the client completely?
> >
> > When the SRV fo_server collapse, all associated primary and backup
> > servers are removed from the list. The plugin doesn't have any state
> > from the previous run, so it will contact both primary and backup domain
> > again.

Thanks for checking
> > > Otherwise ack.
> > >
> > > [PATCH 2/7] IPA SRV plugin: use fo_discover_servers request
> > > Ack. I did a quick sanity test of the IPA DNS discovery and it still
> > > works.
> > >
> > > [PATCH 3/7] IPA SRV plugin: improve debugging
> > > Ack
> > >
> > > [PATCH 4/7] sdap: add sdap_connect_host request
> > > I tested this patch by checking with netstat that there was only one
> > > connection after the discovery used the request.
> > >
> > > I just wonder if using "struct be_resolv_ctx" would make more sense
> > > rather
> > > than using both "struct resolv_ctx" and "enum restrict_family".
> >
> > Hmm, the whole sdap async code doesn't have any knowledge of be what so
> > ever and I'd stick with that.

OK
> > It would also
> > > make sense to cancel the resolv request after "dns_resolver_timeout"
> > > passes
> > > (and read its value from "struct be_resolv_ctx"), although I would be OK
> > > with filing a ticket for that. This functionality is not needed for
> > > the beta.
> >
> > If it takes too long then it will hit a timeout for service resolver in
> > fail over, which already is dns_resolver_timeout.

Right, I didn't realize we were in the resolve_service code already when
the plugin is called. Sorry.
> > > [PATCH 5/7] add sss_ldap_filter_binary_value
> > > Have you looked at ldap_encode_ndr_uint32() from Samba? At the very least
> > > I would prefer if the name made it clear that the function is converting
> > > uint32_t to char. FTR the current implementation of
> > > ldap_encode_ndr_uint32()
> > > looks like:
> > >
> > > char *ldap_encode_ndr_uint32(TALLOC_CTX *mem_ctx, uint32_t value)
> > > {
> > >     uint8_t buf[4];
> > >     struct ldb_val val;
> > >     SIVAL(buf, 0, value);
> > >     val.data = buf;
> > >     val.length = 4;
> > >     return ldb_binary_encode(mem_ctx, val);
> > > }
> > >
> > > So what about a very similar sss_ldap_encode_ndr_uint32() ?
> >
> > Done.
> > > [PATCH 6/7] DNS sites support - add AD SRV plugin
> > > This patch makes samba4-devel a hard requirement of the AD provider.
> > > Given that we're considering dropping support of RHEL5 upstream now, I
> > > don't think this should be a blocker in accepting these patches, but I
> > > would prefer to file a ticket to discover if we can #ifdef the support
> > > out to keep the support of older releases upstream.
> > > It might not be practical after all given the other AD provider features
> > > coming up in 1.10, but still, I think we should track the samba4-devel
> > > requirement and decide on implementing or dropping the ifdefs later
> > > during
> > > 1.10.0 cycle when all the features are in.
> >
> > If we decide to #ifdef it, it will be quite simple, I think.

Yes, since we dropped RHEL5 support in upstream, I no longer think it's
needed. We can ifdef it later if someone really needs it.
> > > > --- a/src/man/sssd-ad.5.xml
> > > > +++ b/src/man/sssd-ad.5.xml
> > > > @@ -130,6 +130,26 @@ ldap_id_mapping = False
> > > >                          </para>
> > > >                      </listitem>
> > > >                  </varlistentry>
> > > >            <varlistentry>
> > > >                <term>ad_enable_dns_sites (boolean)</term>
> > > >                <listitem>
> > > >                    <para>
> > > >                        Enables DNS sites - location based
> > > >                        service discovery.
> > > >                    </para>
> > > >                    <para>
> > > >                        If true and service discovery (see Service
> > > >                        Discovery paragraph at the bottom of the
> > > >
> > > > man page)
> > > >                        is enabled, then the SSSD will first
> > > >
> > > > attempt
> > > >                        location based discovery using an Active
> > > >
> > > > Directory
> > > >                        mechanism called Sites.
> > >
> > > I was wondering for a while whether to explain a little about the AD
> > > Sites, but the sssd-ad man page is probably not the right place..
> >
> > This is a place for google and MSDN IMHO.

Yes, I just wanted to explain what the option does.
> > > Still, what about "the SSSD will first attempt to discover the Active
> > > Directory server to connect to using the Active Directory Site
> > > Discovery and fall back to the DNS SRV records if no AD site is found".
> >
> > Sounds good.

This is good enough for me.
> > > Defaulting to True is correct give the presence of the
> > > Default-First-Site-Name site.
> > >
> > > The ad_get_client_site_send() function could accept "struct
> > > be_resolv_ctx".
> >
> > Done.
> > > Nitpick:
> > > > +static void ad_get_client_site_done(struct tevent_req *subreq)
> > > > +{
> > > > struct ad_get_client_site_state *state = NULL;
> > > > struct tevent_req *req = NULL;
> > > > struct ldb_message_element *el = NULL;
> > > > struct sysdb_attrs **reply = NULL;
> > > > size_t reply_count;
> > > > errno_t ret;
> > > > req = tevent_req_callback_data(subreq, struct tevent_req);
> > > > state = tevent_req_data(req, struct ad_get_client_site_state);
> > > > ret = sdap_get_generic_recv(subreq, state, &reply_count, &reply);
> > > > talloc_zfree(subreq);
> > >
> > > Can you put a comment here stating that this is where the LDAP
> > > connection that was used for the location discovery is closed?
> >
> > Done.
> > > > talloc_zfree(state->sh);
> > > > if (ret != EOK) {
> > >
> > > The rest looks good to me.
> > >
> > > [PATCH 7/7] dns srv plugin: compare domain names case insensitive
> > > Ack
> >
> > Thank you for the review.
>
> Ups, I forgot to commit last changes.

The latest changes work for me. Ack!

[Jakub Hrozek]

On Thu, May 02, 2013 at 02:25:12PM +0200, Jakub Hrozek wrote:
> The latest changes work for me. Ack!

All patches pushed to master.