> On Tue, 2012-01-24 at 16:48 -0500, Stephen Gallagher wrote:
> > On Tue, 2012-01-24 at 11:19 -0500, Stephen Gallagher wrote:
> > > On Tue, 2012-01-24 at 16:03 +0100, Jakub Hrozek wrote:
> > > > On Mon, Jan 23, 2012 at 10:12:15PM -0500, Stephen Gallagher wrote:
> > > > > On Sat, 2012-01-21 at 16:54 -0500, Stephen Gallagher wrote:
> > > > > > These patches add support for getservbyname() and
getservbyport()
> > > > > > to the NSS responder. They do not yet support enumeration
of
> > > > > > services through setservent(), getservent() and
endservent().
> > > > > > These patches also include a reference implementation using
the
> > > > > > proxy ID provider (which I used to test and validate the
> > > > > > functionality).
> > > > > >
> > > > > > Work still to come: enumeration support and LDAP provider
> > > > > > implementation.
> > > > > >
> > > > > > Patch 0001: Make the sized_string structure available
elsewhere
> > > > > > in the source.
> > > > > >
> > > > > > Patch 0002: Add support for strtouint16 (needed for
managing the
> > > > > > port number)
> > > > > >
> > > > > > Patch 0003: Make add_string() and add_ulong() available to
all
> > > > > > sysdb files.
> > > > > >
> > > > > > Patch 0004: Add routines to save, delete and modify
service
> > > > > > entries in the sysdb. (Also includes some very rudimentary
unit
> > > > > > tests)
> > > > > >
> > > > > > Patch 0005: Add LDB indexes for the port and protocol
arguments,
> > > > > > since we'll be searching on them.
> > > > > >
> > > > > > Patch 0006: Add support to the NSS sss_client to look up
> > > > > > getservbyname() and getservbyport().
> > > > > >
> > > > > > Patch 0007: Add support for services in dp requests
> > > > > >
> > > > > > Patch 0008: Add routines to the negative cache to handle
services
> > > > > >
> > > > > > Patch 0009: The big one. This patch adds a new feature to
the NSS
> > > > > > responder to handle looking up services by name and by
port
> > > > > > (optionally filtering by protocol).
> > > > > >
> > > > > > Important note: I did not use the existing check_cache()
> > > > > > functionality here. I changed the cache check and lookup
logic
> > > > > > to match that which was proposed in
> > > > > >
https://fedorahosted.org/sssd/ticket/1126 (to deal with
multiple
> > > > > > domains more efficiently). I would like this very
carefully
> > > > > > reviewed (by two engineers) because I think our long-term
plan
> > > > > > is going to be to migrate the existing NSS responders over
to
> > > > > > this style.
> > > > > >
> > > > > > /* Provider Lookup Logic:
> > > > > > * Iterate through the available caches. If the cached
entry is
> > > > > > * present and not expired, return it immediately(*). If it
is
> > > > > > * present and expired, add it to a list of domains
eligible to
> > > > > > * be checked. If it is in the negative cache, skip over it
and
> > > > > > * do not add it to the eligible domain list.
> > > > > > *
> > > > > > * Once we have searched all of the caches, if the entry
has not
> > > > > > * been determined to be available, search all domains in
order
> > > > > > * to see if any of them contain the requested entry.
> > > > > > *
> > > > > > * (*) Optionally perform a midpoint cache refresh if
> > > > > > appropriate. */
> > > > > >
> > > > > > See the functions getserv_send() and lookup_service_*() for
more
> > > > > > details.
> > > > > >
> > > > > >
> > > > > > Patch 0010: This is the reference implementation I used
for
> > > > > > testing purposes. I tested against proxy_lib_name = files
(and
> > > > > > changed the 'services' line in my
/etc/nsswitch.conf to contain
> > > > > > only 'sss')
> > > > >
> > > > > New round of patches. The first seven patches are unchanged
from
> > > > > the first pass (though offset by three, since the first three
> > > > > patches have been pushed). The eighth patch is just Jakub's
acked
> > > > > patch for refactoring the dp_req routines from "[PATCH]
DP:
> > > > > Refactor responder_dp_req so it's reusable by other
responders" in
> > > > > the proper order to ensure it doesn't get lost/put in the
wrong
> > > > > place. (Though I'm pretty sure it will apply cleanly
anywhere
> > > > > after 0006).
> > > > >
> > > > > Patch 0009: Add support to sss_client to make enumeration
requests
> > > > > for services
> > > > >
> > > > > Patch 0010: Add sysdb routines for requesting the complete list
of
> > > > > cached services.
> > > > >
> > > > > Patch 0011: Another big one. Adds support for enumerating
services
> > > > > to the NSS provider.
> > > > >
> > > > > Patch 0012: The proxy provider implementation of the
enumeration
> > > > > logic. Useful for testing purposes (I tested against the
'files'
> > > > > NSS library).
> > > > >
> > > > > There is a known issue with the proxy provider support that
I'm
> > > > > going to work on in a later patch. Specifically, when we get
back
> > > > > the list of available services from the proxy provider, we get
> > > > > services that have multiple available protocols back as
separate
> > > > > entries. This causes the second value to overwrite the primary
> > > > > value in sysdb_store_service(). I'm going to add the ability
to
> > > > > merge them, but since the LDAP provider (when finished)
won't
> > > > > suffer this problem (since LDAP lookups will return all
protocols
> > > > > in the same request), I'm not stressing over it right now.
Once
> > > > > the code is in, I'll file a bug so we don't forget about
it.
> > > >
> > > > Patch 0001: SYSDB: Add sysdb routines for manipulating service
> > > > entries
> > > >
> > > > I agree with Simo that proto?sanitized_proto:"*" is
confusing. I
> > > > think it would be more readable to set sanitized_proto to NULL on
> > > > definition and just use sanitized_proto in the ternary operator.
> > >
> > > Whoops. I forgot to include Simo's review comment. Fixed.
> > >
> > > > Why the cast of "int proto" in sysdb_getservbyport() to
"unsigned
> > > > int" ? Would it make sense to add a check if proto >= 0 ? (I
can't
> > > > help wondering why the glibc interface uses int and not unsigned int
> > > > in the first place..)
> > >
> > > I assume you meant "int port". I cast it because (as you
observe) glibc
> > > uses 'int' instead of 'uint16_t' which is what it SHOULD
be doing.
> > >
> > > I added a check.
> > >
> > > > I noticed we use the same construct with sysdb_transaction_start()
> > > > and bool in_transaction all around the code. I think a macro that
> > > > would wrap the start, commit and cancel might be nicer and save us
> > > > from some bugs in the future. Not as part of this patch, of course.
> > >
> > > We can think about it. Please file a ticket.
> > >
> > > > Can you add a DEBUG() message fot the case sysdb_store_service() is
> > > > deleting a corrupt entry?
> > >
> > > Yeah, good idea.
> > >
> > > > The loop after sysdb_getservbyname() in sysdb_store_service() always
> > > > references res->msgs[0], shouldn't that be res->msgs[i] ?
> > >
> > > Thanks, bad copy-paste
> > >
> > > > In sysdb_svc_add() isn't it safer to use "if (aliases
&&
> > > > aliases[0])"? I think the caller might appreciate the simplicity
of
> > > > passing NULL for no
> > >
> > > Good idea. Fixed
> > >
> > > > aliases. The same applies for aliases and protocols in
> > > > sysdb_svc_update().
> > >
> > > For aliases, yes. There MUST be at least one protocol. I've added an
> > > EINVAL check at the start of the function.
> > >
> > > > In sysdb_svc_delete() you forgot to set in_transaction to false
after
> > > > sysdb_transaction_commit().
> > >
> > > Good catch. That macro is sounding better :)
> > >
> > > > Patch 0002: NSS: Add client support for services (non-enumeration)
> > > > Ack
> > > >
> > > > Patch 0003: NSS: Add client support for services (non-enumeration)
> > > > Can you #define some SERVICES_METADATA_COUNT to avoid using the
> > > > constant 8 in the functions?
> > > > Otherwise looks OK
> > >
> > > Done. I also made the same change to patch 0009
> > >
> > > > Patch 0004: DP: Add support for services in dp requests
> > > > Ack
> > > >
> > > > Patch 0005: NSS: Add negative cache routines for services
> > > > Ack
> > > >
> > > > The rest of the patches is TBR, it might take time to review the big
> > > > ones, so I decided to send the review in parts.
> > >
> > > Since the full patchset is enormous, I'm only attaching the current
> > > updates. Anyone who wants the full set of patches can get them from my
> > > personal git repo with the following commands:
> > >
> > > git remote add sgallagh \
> > >
git://fedorapeople.org/home/fedora/jhrozek/public_git/sssd.git
> > >
> > > git remote update
> > >
> > > git format-patch -M -C --patience --full-index \
> > > 6e4efa5a0960a83fef6a654152edfc618bcebece..HEAD
> > >
> > >
> > > I'll continue to update the format-patch command in subsequent
> > > revisions.
> >
> > Just a heads-up. I'm reworking the "extra" field in the backend
so that
> > it's parsed by be_get_account_info() instead of in the providers itself
> > (so they'll be able to get breq->extra_value instead of parsing
> > breq->filter_value).
> >
> > This is going to require adjustment for the proxy provider.
>
> Ok, resending all patches (tarballed this time) just to keep things
> clean.
>
> I added a new patch to the series (now Patch 0001) that adds support for
> splitting the extra data in the be_get_account_info() routine instead of
> requiring the provider plugins to do it themselves. This adds a new
> attribute to the be_acct_req structure for the extra data.
>
> Patches 0002-0006 are unchanged this round.
>
> Patch 0007 was updated to correct some DEBUG log glitches:
> * %lu instead of %ul
> * Wrap the SVC_*_CASED macros in parens so they will work inside other
> ternary operations.
>
> It also fixes a much more serious issue where it was not properly
> detecting the length of the protocol and was thus failing.
>
> Patch 0008 was updated to properly convert the port value back to the
> network format which is expected by the loaded NSS library. (The NSS
> Responder serves the data provider a port in the native format).
>
> All the rest are unchanged from the last submission. Sorry for the
> churn.
#0001:
Ack.
#0002:
- make sysdb_svc_add and sysdb_svc_update static -> there is a
sysdb_store_service as an interface for them. Also consider making
sysdb_svc_dn and sysdb_svc_remove_alias making static - I'm not sure about
those.
- if we are to be really defensive (as discussed on IRC), I'd change the
approach if detecting more services with the same name being already stored in
sysdb - perhaps delete them all and store the new one instead of returning
EINVAL?
- in sysdb_svc_delete: just in case both port and name is given, I'd try to
delete service by name and in case of failure fall back to the port (i.e. not
making them mutually exclusive).
#0003:
Ack
#0004:
- just a reminder that there is that segfault inducing error, other than that
the patch is fine, so conditional Ack
#0005:
Ack
#0006:
- I think you should be freeing service_and_protocol variables after they are
used. They are allocated on top of negative cache, so they could potentially
live pretty long and take a lot of space
- I also think you should be checking the above mentioned variable for NULL
and returning ENOMEM if the operation failed. Otherwise I see potential
problems in utf8 library which would could receive NULL on input. And if
nothing else, you would be returning EINVAL from sss_ncache_set_service_int
instead of ENOMEM ;-)
That's all I've got right now. I will continue with the review tomorrow.
I forgot to include the patches. Fixed.