----- Original Message -----
> Hi All,
>
> I've just pushed the following to the brolley/dev branch in the pcpfans
> repository:
Lookin' good!
> - Added -p option to pminfo for prototype/testing purposes
pmfind(1) sounds good; even just with a main() and that snippet of pminfo
code initially. It would be a good idea to make pmwebd advertise soon, to
start testing the multi-server aspects of this code.
> I just want to add a few notes to the information in the commit message and
> to solicit review and comments.
>
> * The new __pmDiscoverServices() API is as was discussed on this list and
> on IRC. Not much to add in the way of notes for that one.
As Frank suggested, I tend to agree this would be a good pmapi.h candidate with
no double-underscore. Man pages, book sections and navel-gazing about soname's
and symbol-files is warranted too as a result.
> * I exposed the data structure __pmServiceInfo along with the functions
> __pmServiceInfoAlloc(), __pmServiceInfoFree() and
> __pmAddDiscoveredService() in case there is a need for clients to
> manually add services which are known to them by other means than the
> supported discovery mechanisms (currently only PMCD via Avahi). If the
> consensus is that this is not needed, then we can easily hide them
> again.
I don't think clients would ever do this (so, not PMAPI). Others servers may,
but double-underscore level of stuff for sure. Its likely that just the calls
that pmcd makes will be enough, so not sure this needs to be exposed?
At this stage, these routines could perhaps live in libpcp/src/internal.h and
not <pcp/impl.h>?
> * Although __pmDiscoverServices() returns an array of urls as strings,
> __pmServiceInfo maintains the information as __pmSockAddr which I feel
> will be easier to work with internally once we start adding filters and
> other manipulations. __pmSockAddr is easily converted to a string at the
> end of it all using __pmSockAddrToString.
Hmm, OK, we'll need to see how that plays out I guess.
> * The new API function __pmSockAddrGetPort() was required in order to
> construct the urls.
*nod*, thats possibly all that should be left in impl.h then, I think?
> * I added a prototype option (-p) to pminfo just so that I could
> demonstrate that the new API is working. It can be removed at any time.
> * The urls returned for IPv6 PMCDs are similar to
> pcp://[fe80::5eff:35ff:fe07:55ca]:44321. That is, the address is
> enclosed in [] in order to separate the address from the port. This is a
> common notation among IPv6-enabled applications. Currently, PCP's url
> parser does not accept this notation. It should be an easy fix which I
> can have ready quickly.
Sounds good & useful & needed to me. Misc other notes follow...
diff --git a/src/include/pcp/impl.h b/src/include/pcp/impl.h
index 6c9023e..1394a49 100644
--- a/src/include/pcp/impl.h
+++ b/src/include/pcp/impl.h
@@ -618,6 +618,7 @@ extern __pmSockAddr *__pmSockAddrMask(__pmSockAddr *, const
__pmSockAddr *);
extern void __pmSockAddrSetFamily(__pmSockAddr *, int);
extern int __pmSockAddrGetFamily(const __pmSockAddr *);
extern void __pmSockAddrSetPort(__pmSockAddr *, int);
+extern int __pmSockAddrGetPort(const __pmSockAddr *);
extern void __pmSockAddrSetPath(__pmSockAddr *, const char *);
extern int __pmSockAddrIsLoopBack(const __pmSockAddr *);
extern int __pmSockAddrIsInet(const __pmSockAddr *);
@@ -665,10 +666,25 @@ extern void __pmServerCloseRequestPorts(void);
extern void __pmServerDumpRequestPorts(FILE *);
extern char *__pmServerRequestPortString(int, char *, size_t);
+/* Service broadcasting, for servers. */
typedef struct __pmServerPresence __pmServerPresence;
extern __pmServerPresence *__pmServerAdvertisePresence(const char *, int);
extern void __pmServerUnadvertisePresence(__pmServerPresence *);
+/* Service discovery, for clients. */
+typedef struct {
+ const char *spec;
+ __pmSockAddr *address;
+} __pmServiceInfo;
+
Can this live in internal.h? Not clear we should be exposing this
outside of libpcp at this stage.
+
+extern int __pmDiscoverServices(char ***, const char *, const char *);
+
(into pmapi.h without double-underscore?)
+extern __pmServiceInfo *__pmServiceInfoAlloc();
+extern void __pmServiceInfoFree(__pmServiceInfo *);
+extern void __pmAddDiscoveredService(char ***, __pmServiceInfo *);
+extern void __pmServiceListFree(char **);
+
So far, all of the above feels like libpcp internal.h fodder.
diff --git a/src/libpcp/src/auxserver.c b/src/libpcp/src/auxserver.c
index 69207e1..f13388a 100644
--- a/src/libpcp/src/auxserver.c
+++ b/src/libpcp/src/auxserver.c
@@ -18,9 +18,7 @@
#include "impl.h"
#define SOCKET_INTERNAL
#include "internal.h"
-#if HAVE_AVAHI
#include "avahi.h"
-#endif
Nice.
diff --git a/src/libpcp/src/avahi.c b/src/libpcp/src/avahi.c
index f74ec32..450c762 100644
--- a/src/libpcp/src/avahi.c
+++ b/src/libpcp/src/avahi.c
@@ -13,12 +13,21 @@
*/
#include <assert.h>
+#include <avahi-client/publish.h>
+#include <avahi-client/lookup.h>
+#include <avahi-common/alternative.h>
+#include <avahi-common/simple-watch.h>
+#include <avahi-common/thread-watch.h>
+#include <avahi-common/timeval.h>
+#include <avahi-common/malloc.h>
+#include <avahi-common/error.h>
*nod* - good stuff.
@@ -351,3 +360,233 @@ __pmServerAvahiUnadvertisePresence(__pmServerPresence *s)
s->avahi = NULL;
}
}
+
+/* Support for clients searching for services. */
+typedef struct browsingContext {
+ AvahiSimplePoll *simplePoll;
+ AvahiClient *client;
+ char ***urls;
+} browsingContext;
+
+/* Called whenever a service has been resolved successfully or timed out. */
+static void
+resolveCallback(
+ AvahiServiceResolver *r,
+ AvahiIfIndex interface,
+ AvahiProtocol protocol,
+ AvahiResolverEvent event,
+ const char *name,
+ const char *type,
+ const char *domain,
+ const char *hostName,
+ const AvahiAddress *address,
+ uint16_t port,
+ AvahiStringList *txt,
+ AvahiLookupResultFlags flags,
+ void *userdata
+)
+{
+ char addressString[AVAHI_ADDRESS_STR_MAX];
+ const browsingContext *context = (browsingContext *)userdata;
+ char ***urls = context->urls;
+ __pmServiceInfo serviceInfo;
+
+ /* Unused arguments. */
+ (void)interface;
+ (void)protocol;
+ (void)hostName;
+ (void)txt;
+ (void)flags;
+ assert(r);
+
+ switch (event) {
+ case AVAHI_RESOLVER_FAILURE:
+ __pmNotifyErr(LOG_ERR,
+ "Failed to resolve service '%s' of type '%s' in
domain '%s': %s",
+ name, type, domain,
+
avahi_strerror(avahi_client_errno(avahi_service_resolver_get_client(r))));
This is visible to PMAPI client (monitor) tools isn't it?
If they are GUI tools, __pmNotifyErr is not ideal - can we
return an errno here? Or just ignore it? And/or diagnostic
guarded by PM_TRACE_DISCOVERY?
+ break;
+
+ case AVAHI_RESOLVER_FOUND:
+ /* Currently, only pmcd is supported. */
pmwebd in here will help us to exercise more cases, and remove
the need for these kinds of comments :)
+ if (strcmp(type, "_pmcd._tcp") == 0) {
+ serviceInfo.spec = SERVER_SERVICE_SPEC;
+ avahi_address_snprint(addressString, sizeof(addressString),
address);
+ serviceInfo.address = __pmStringToSockAddr(addressString);
+ if (serviceInfo.address == NULL) {
+ __pmNoMem("resolveCallback", __pmSockAddrSize(),
PM_FATAL_ERR);
As above, return ENOMEM?
+ }
+ __pmSockAddrSetPort(serviceInfo.address, port);
+ __pmAddDiscoveredService(urls, &serviceInfo);
+ __pmSockAddrFree(serviceInfo.address);
+ }
+ else {
+ __pmNotifyErr(LOG_ERR, "Unsupported Avahi service type '%s'",
type);
+ }
Ignore or EINVAL or...?
+/*
+ * Called whenever a new service becomes available on the LAN
+ * or is removed from the LAN.
+ */
+static void
+browseCallback(
+ AvahiServiceBrowser *b,
+ AvahiIfIndex interface,
+ AvahiProtocol protocol,
+ AvahiBrowserEvent event,
+ const char *name,
+ const char *type,
+ const char *domain,
+ AvahiLookupResultFlags flags,
+ void *userdata
+)
+{
+ browsingContext *context = (browsingContext *)userdata;
+ AvahiClient *c = context->client;
+ AvahiSimplePoll *simplePoll = context->simplePoll;
+ assert(b);
+
+ /* Unused argument. */
+ (void)flags;
+
+ switch (event) {
+ case AVAHI_BROWSER_FAILURE:
+ __pmNotifyErr(LOG_ERR,
+ "Avahi browse failed: %s",
+
avahi_strerror(avahi_client_errno(avahi_service_browser_get_client(b))));
+ avahi_simple_poll_quit(simplePoll);
Ignore, or trace diagnostic? (few more similar cases later as well)
+int __pmAvahiDiscoverServices(char ***urls, const char *service)
(code style/consistency - separate line for return type)
Oh - also, the arguments would usually be the other way around -
inputs, then outputs.
+
+ /* Create the service browser. */
+ size = sizeof("_._tcp") + strlen(service); /* includes room for the nul */
+ if ((serviceTag = malloc(size)) == NULL) {
+ __pmNoMem("__pmAvahiDiscoverServices: can't allocate service tag",
+ size, PM_FATAL_ERR);
+ }
+ sprintf(serviceTag, "_%s._tcp", service);
+ sb = avahi_service_browser_new(client, AVAHI_IF_UNSPEC,
+ AVAHI_PROTO_UNSPEC, serviceTag,
+ NULL, (AvahiLookupFlags)0,
+ browseCallback, & context);
+ free(serviceTag);
+ if (sb == NULL) {
+ __pmNotifyErr(LOG_ERR, "__pmAvahiDiscoverServices: Failed to create
Avahi service browser: %s",
+ avahi_strerror(avahi_client_errno(client)));
+ sts = -1;
+ goto done;
+ }
+
+ /* Timeout after 2 seconds. */
Hmm, seems ... arbitrary. Should be env var controllable - maybe use
__pmConvertTimeout(TIMEOUT_DEFAULT)?
+ avahi_simple_poll_get(simplePoll)->timeout_new(
+ avahi_simple_poll_get(simplePoll),
+ avahi_elapse_time(&tv, 1000*2, 0),
+ timeoutCallback, &context);
diff --git a/src/libpcp/src/avahi.h b/src/libpcp/src/avahi.h
index 78d4372..92fe5d4 100644
--- a/src/libpcp/src/avahi.h
+++ b/src/libpcp/src/avahi.h
@@ -13,13 +13,11 @@
* or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* for more details.
*/
-#include <avahi-client/publish.h>
-#include <avahi-common/alternative.h>
-#include <avahi-common/thread-watch.h>
-#include <avahi-common/malloc.h>
-#include <avahi-common/error.h>
Liking this change - good catch!
diff --git a/src/libpcp/src/check-statics b/src/libpcp/src/check-statics
index 35f3f24..b2512d3 100755
--- a/src/libpcp/src/check-statics
+++ b/src/libpcp/src/check-statics
@@ -100,6 +100,7 @@ auxserver.o
b serviceSpec # single-threaded server scope
d localSocketFd # single-threaded server scope
b server_features # single-threaded server scope
+discovery.o
I like this too - we probably should have done this for the advertise
code too.
diff --git a/src/libpcp/src/discovery.c b/src/libpcp/src/discovery.c
new file mode 100644
index 0000000..3e479cc
--- /dev/null
+++ b/src/libpcp/src/discovery.c
@@ -0,0 +1,155 @@
+/*
+ * Copyright (c) 2013 Red Hat.
+ *
+ * This library is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; either version 2.1 of the License, or
+ * (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
+ * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public
+ * License for more details.
+ */
+#include "pmapi.h"
+#include "impl.h"
+#include "avahi.h"
+
+#if defined(HAVE_SERVICE_DISCOVERY)
+
+int __pmDiscoverServices(char ***urls, const char *service,
+ const char *discovery_domain)
+{
(as discussed already - input, then output args, + fewer underscores)
+ int sts;
+ /*
+ * Attempt to discover the requested service(s) using the
requested/available means.
+ * If a particular method is not available or not configured, then the
+ * respective call will have no effect.
+ *
+ * Currently, only Avahi is supported, so ignore the discovery_domain
+ */
+ (void)discovery_domain;
Hmm - shouldn't we still check that its "avahi" or NULL? Else, this
argument can never be reliably used by a caller?
+ *urls = NULL;
+ sts = __pmAvahiDiscoverServices(urls, service);
+ return sts;
I think this API is also inconsistent with other PMAPI routines - it
should return the number of service urls discovered on success, and
a urls list allocated in a single allocation call, suitable for the
client to pass to free().
See pmNameAll(3), pmGetChildren(3), pmGetChildrenStatus(3) as other
examples that pass back arrays of strings which can be freed that way.
+}
+
+#else /* !HAVE_SERVICE_DISCOVERY */
+
+int __pmDiscoverServices(char ***urls, const char *service,
+ const char *discovery_domain)
+{
+ /* No services to discover. */
+ (void)service;
+ (void)discovery_domain;
+ *urls = NULL;
+ return 0;
This should return EOPNOTSUPP, and can ignore urls.
+}
+
+#endif /* !HAVE_SERVICE_DISCOVERY */
+
+/* For manually adding a service. Also used by __pmDiscoverServices(). */
+void __pmAddDiscoveredService(char ***urls, __pmServiceInfo *info)
If this is indeed all internal(.h), can it be within the above ifdef?
+{
+ const char* prefix;
+ char *addressString;
+ char *url;
+ size_t next;
+ size_t size;
+ int isIPv6;
+ int port;
+ /*
+ * Add the given address and port to the given list of urls.
+ * Build the new entry first, so that we can filter out duplicates.
+ * Currently, only "pmcd" is supported.
+ */
+ if (strcmp(info->spec, SERVER_SERVICE_SPEC) == 0) {
+ prefix = "pcp://";
+ }
+ else {
+ __pmNotifyErr(LOG_ERR,
+ "__pmAddDiscoveredService: Unsupported service: '%s'",
+ info->spec);
+ return;
+ }
+
+ /*
+ * Allocate the new entry. We need room for the url prefix, the address
+ * and the port. IPv6 addresses require a set of [] surrounding the
+ * address in order to distinguish the port.
+ */
+ port = __pmSockAddrGetPort(info->address);
+ addressString = __pmSockAddrToString(info->address);
+ if (addressString == NULL) {
+ __pmNoMem("__pmAddDiscoveredService: can't allocate address buffer",
+ 0, PM_FATAL_ERR);
+ }
+ size = strlen(prefix) + strlen(addressString) + sizeof(":65535");
+ if ((isIPv6 = (__pmSockAddrGetFamily(info->address) == AF_INET6)))
+ size += 2;
+ url = malloc(size);
+ if (url == NULL) {
+ __pmNoMem("__pmAddDiscoveredService: can't allocate new entry",
+ size, PM_FATAL_ERR);
+ }
+ if (isIPv6)
+ snprintf(url, size, "%s[%s]:%u", prefix, addressString, (uint16_t)port);
+ else
+ snprintf(url, size, "%s%s:%u", prefix, addressString, (uint16_t)port);
+ free(addressString);
+
+ /*
+ * Now search the current list for the new entry.
+ */
+ if (*urls == NULL)
+ next = 0; /* no list to search */
+ else {
+ for (next = 0; (*urls)[next] != NULL; ++next) {
+ if (strcmp(url, (*urls)[next]) == 0) {
+ /* Found a duplicate. */
+ free(url);
+ return;
+ }
+ }
+ }
+
+ /*
+ * It's not a duplicate, so add it to the end of the list.
+ * We grow the list each time, since it is likely to be
+ * small.
+ */
+ size = (next + 2) * sizeof(**urls);
+ *urls = realloc(*urls, size);
+ if (*urls == NULL) {
+ __pmNoMem("__pmAddDiscoveredService: can't allocate service table",
+ size, PM_FATAL_ERR);
+ }
+ (*urls)[next] = url;
+
+ /* Terminate the list. */
+ (*urls)[next + 1] = NULL;
NULL-termination is inconsistent with other PMAPI interfaces which
return counts - could we switch it to that existing style?
+/* For freeing a service list. */
+void __pmServiceListFree(char **urls)
(then, I don't think we need this anymore? - its just a free - if
we do need this "__pmFreeServiceList" would match "pmFreeResult"
naming convention a bit more).
diff --git a/src/pminfo/pminfo.c b/src/pminfo/pminfo.c
index 112cfc1..3f0ec81 100644
--- a/src/pminfo/pminfo.c
+++ b/src/pminfo/pminfo.c
@@ -763,6 +769,21 @@ main(int argc, char **argv)
}
}
+ if (discover_pmcd) {
+ char **urls;
+ if ((sts = __pmDiscoverServices(&urls, SERVER_SERVICE_SPEC, NULL)) ==
0) {
+ if (urls != NULL) {
+ char **p;
+ printf("Local PMCD servers:\n");
("local" can possibly be misinterpreted here)
+ for (p = urls; *p != NULL; ++p)
+ printf(" %s\n", *p);
+ __pmServiceListFree(urls);
+ }
+ else
+ printf("No local PMCD servers found\n");
+ }
+ }
+
Yeah, new tool - and start loading it up with search options :) -
like pmcd vs pmwebd server searches and so on - this is all really
cool!
cheers.
--
Nathan
|