Did anyone have any comments on this approach?
On Tue, Jun 16, 2009 at 01:22:24PM -0500, Martin Hicks wrote:
>
> On Tue, Jun 09, 2009 at 11:12:48PM +1000, Max Matveev wrote:
> >
> > Perhaps a better way to deal with this would be to add some keywords
> > to the config file to indicate local HCAs and to complain bitterly if
> > they're not found.
>
> In the end, I still don't love this solution because it requires manual
> intervention from the user to change or remove the config file when they
> change an HCA (or in my case when a diskless blade gets replaced in a
> cluster).
>
> How about this patch. It rejigs things a lot, but essentially:
>
> - No config file will be created unless the '-w' flag is passed to
> pmdaib. (this is a change)
>
> - By default, if no config file is found, only local ports are monitored.
> (before we would always write out a config file containing only local
> ports)
>
> - If a config file exists then it is the preferred source of information
> and perhaps not even local ports will be monitored. (before you could
> have edited the automatically created file to remove the local ports)
>
>
> So in the case of a user who is using a custom config file (either to
> name the instances differently, or to monitor remote ports) who replaces
> an HCA, it will be up to them to edit the config file that they created.
>
> For those who are using the ib pmda in its simplest mode with no config
> file, no changes will be necessary when an HCA is replaced but they have
> no guarantee of static device naming if they add an additional HCA.
>
>
>
> diff --git a/src/pmdas/ib/daemon.c b/src/pmdas/ib/daemon.c
> index 2a71151..12540cc 100644
> --- a/src/pmdas/ib/daemon.c
> +++ b/src/pmdas/ib/daemon.c
> @@ -33,7 +33,8 @@ usage(void)
> fputs("Options:\n"
> " -d domain use domain (numeric) for metrics domain of PMDA\n"
> " -l logfile write log into logfile rather than using default log
> name\n"
> - " -c path to configuration file\n",
> + " -c path to configuration file\n"
> + " -w write the basic configuration file\n",
> stderr);
> exit(1);
> }
> @@ -46,19 +47,22 @@ main(int argc, char **argv)
> pmdaInterface dispatch;
> char helppath[MAXPATHLEN];
> char *confpath = NULL;
> - char *p;
> int opt;
> + int writeconf = 0;
>
> __pmSetProgname(argv[0]);
> snprintf(helppath, sizeof(helppath), "%s%c" "ib" "%c" "help",
> pmGetConfig("PCP_PMDAS_DIR"), sep, sep);
> pmdaDaemon(&dispatch, PMDA_INTERFACE_3, pmProgname, IB, "ib.log",
> helppath);
>
> - while ((opt = pmdaGetOpt(argc, argv, "D:c:d:l:?", &dispatch, &err)) !=
> EOF) {
> + while ((opt = pmdaGetOpt(argc, argv, "D:c:d:l:w?", &dispatch, &err)) !=
> EOF) {
> switch (opt) {
> case 'c':
> confpath = optarg;
> break;
> + case 'w':
> + writeconf = 1;
> + break;
> default:
> err++;
> }
> @@ -69,7 +73,7 @@ main(int argc, char **argv)
> }
>
> pmdaOpenLog(&dispatch);
> - ibpmda_init(confpath, &dispatch);
> + ibpmda_init(confpath, writeconf, &dispatch);
> pmdaConnect(&dispatch);
> pmdaMain(&dispatch);
>
> diff --git a/src/pmdas/ib/dso.c b/src/pmdas/ib/dso.c
> index 33f10a8..4e506f8 100644
> --- a/src/pmdas/ib/dso.c
> +++ b/src/pmdas/ib/dso.c
> @@ -37,5 +37,5 @@ ib_init (pmdaInterface * dispatch)
> pmGetConfig("PCP_PMDAS_DIR"), sep, sep);
> pmdaDSO(dispatch, PMDA_INTERFACE_3, "ibpmda", helppath);
>
> - ibpmda_init(NULL, dispatch);
> + ibpmda_init(NULL, 0, dispatch);
> }
> diff --git a/src/pmdas/ib/ib.c b/src/pmdas/ib/ib.c
> index ccfb079..c027f77 100644
> --- a/src/pmdas/ib/ib.c
> +++ b/src/pmdas/ib/ib.c
> @@ -141,6 +141,70 @@ typedef struct port_state_s {
> char pcap[IB_ALLPORTCAPSTRLEN];
> } port_state_t;
>
> +static char confpath[MAXPATHLEN];
> +static int portcount;
> +/* Line number while parsing the config file */
> +static FILE *fconf;
> +static int lcnt;
> +
> +#define print_parse_err(loglevel, fmt, args...) \
> + if (fconf) { \
> + __pmNotifyErr(loglevel, "%s(%d): " fmt, fconf, lcnt, args); \
> + } else { \
> + __pmNotifyErr(loglevel, fmt, args); \
> + }
> +
> +static void
> +monitor_guid(pmdaIndom *itab, char *name, long long guid, int rport,
> + char *local, int lport)
> +{
> + int inst;
> + hca_state_t *hca = NULL;
> + port_state_t *ps;
> +
> + if (pmdaCacheLookupName(itab[IB_HCA_INDOM].it_indom, local, NULL,
> + (void**)&hca) != PMDA_CACHE_ACTIVE) {
> + print_parse_err(LOG_ERR, "unknown HCA '%s' in 'via' clause\n", local);
> + return;
> + }
> +
> + if ((lport >= UMAD_CA_MAX_PORTS) || (lport < 0)) {
> + print_parse_err(LOG_ERR,
> + "port number %d is out of bounds for HCA %s\n",
> + lport, local);
> + return;
> + }
> +
> + if (hca->lports[lport].hndl == NULL) {
> + print_parse_err(LOG_ERR,
> + "port %s:%d has failed initialization\n",
> + local, lport);
> + return;
> + }
> +
> + if ((ps = (port_state_t *)calloc(1, sizeof(port_state_t))) == NULL) {
> + __pmNotifyErr (LOG_ERR, "Out of memory to save state for %s\n", name);
> + return;
> + }
> +
> + ps->guid = guid;
> + ps->remport = rport;
> + ps->lport = hca->lports + lport;
> + ps->portid.lid = -1;
> + ps->timeout = 1000;
> +
> + if ((inst = pmdaCacheStore(itab[IB_PORT_INDOM].it_indom,
> + PMDA_CACHE_ADD, name, ps)) < 0) {
> + __pmNotifyErr(LOG_ERR, "Cannot add %s to the cache - %s\n",
> + name, pmErrStr(inst));
> + free (ps);
> + return;
> + }
> +
> + portcount++;
> +}
> +
> +
> static int
> foreachport(hca_state_t *hst, void (*cb)(hca_state_t *, umad_port_t *, void
> *),
> void *closure)
> @@ -166,7 +230,6 @@ foreachport(hca_state_t *hst, void (*cb)(hca_state_t *,
> umad_port_t *, void *),
> static void
> printportconfig (hca_state_t *hst, umad_port_t *port, void *arg)
> {
> - FILE *fconf = arg;
> uint64_t hguid = port->port_guid;
>
> __ntohll((char *)&hguid);
> @@ -176,6 +239,20 @@ printportconfig (hca_state_t *hst, umad_port_t *port,
> void *arg)
> hst->ca.ca_name, port->portnum);
> }
>
> +static void
> +monitorport(hca_state_t *hst, umad_port_t *port, void *arg)
> +{
> + pmdaIndom *itab = arg;
> + uint64_t hguid = port->port_guid;
> + char name[128];
> +
> + __ntohll((char *)&hguid);
> + sprintf(name, "%s:%d", port->ca_name, port->portnum);
> +
> + monitor_guid(itab, name, hguid, port->portnum, port->ca_name,
> port->portnum);
> +}
> +
> +
> static int mgmt_classes[] = {IB_SMI_CLASS, IB_SMI_DIRECT_CLASS,
> IB_SA_CLASS, IB_PERFORMANCE_CLASS};
> static void
> @@ -196,74 +273,10 @@ openumadport (hca_state_t *hst, umad_port_t *port, void
> *arg)
> lp->hndl = hndl;
> }
>
> -int
> -ib_load_config(const char *confpath, pmdaIndom *itab, unsigned int nindoms)
> +static void
> +parse_config(pmdaIndom *itab)
> {
> - char hcas[IBPMDA_MAX_HCAS][UMAD_CA_NAME_LEN];
> - hca_state_t *st = NULL;
> - int i, n;
> - int lcnt = 0;
> char buffer[2048];
> - FILE *fconf;
> - int (*closef)(FILE *) = fclose;
> - int portcount = 0;
> -
> - if (nindoms <= IB_CNT_INDOM) {
> - return (-EINVAL);
> - }
> -
> - if (umad_init()) {
> - return (-EIO);
> - }
> -
> - if ((n = umad_get_cas_names(hcas, ARRAYSZ(hcas)))) {
> - if ((st = calloc (n, sizeof(hca_state_t))) == NULL) {
> - return (-ENOMEM);
> - }
> - } else {
> - return (0);
> - }
> -
> - for (i=0; i < n; i++) {
> - if (umad_get_ca(hcas[i], &st[i].ca) == 0) {
> - int e = pmdaCacheStore (itab[IB_HCA_INDOM].it_indom, PMDA_CACHE_ADD,
> - st[i].ca.ca_name, &st[i].ca);
> -
> - if (e >= 0) {
> - foreachport (st+i, openumadport, NULL);
> - } else {
> - __pmNotifyErr (LOG_ERR,
> - "Cannot add instance for %s to the "
> - "cache - %s\n",
> - st[i].ca.ca_name, pmErrStr(e));
> - }
> - }
> - }
> -
> - /* Process config file - if the executable bit is set then assume
> - * that user wants it to be a script and run it, otherwise try loading
> - * it. If it cannot be loaded then create a default one and
> - * process it again */
> - if (access(confpath, X_OK)) {
> - /* Not an executable, just read it */
> - if ((fconf = fopen (confpath, "r")) == NULL) {
> - if ((fconf = fopen (confpath, "w+")) != NULL) {
> - for (i=0; i < n; i++) {
> - foreachport (st+i, printportconfig, fconf);
> - }
> - fseek (fconf, 0, SEEK_SET);
> - }
> - }
> - } else {
> - fconf = popen(confpath, "r");
> - closef = pclose;
> - }
> -
> - if (fconf == NULL) {
> - __pmNotifyErr (LOG_CRIT, "Cannot open configuration file %s\n",
> - confpath);
> - return (-ENOENT);
> - }
>
> while ((fgets(buffer, sizeof(buffer)-1, fconf)) != NULL) {
> char *p;
> @@ -285,64 +298,98 @@ ib_load_config(const char *confpath, pmdaIndom *itab,
> unsigned int nindoms)
> int rport;
> char local[128];
> int lport;
> - hca_state_t *hca = NULL;
> - port_state_t *ps;
>
> if (sscanf(p, "%[^ \t]%llx%d via %[^:]:%d",
> name, &guid, &rport, local, &lport) != 5) {
> __pmNotifyErr (LOG_ERR, "%s(%d): cannot parse the line\n",
> confpath, lcnt);
> - } else if (pmdaCacheLookupName(itab[IB_HCA_INDOM].it_indom,
> - local, NULL,
> - (void**)&hca) != PMDA_CACHE_ACTIVE) {
> - __pmNotifyErr (LOG_ERR,
> - "%s(%d): unknown HCA '%s' in 'via' clause\n",
> - confpath, lcnt, local);
> - } else if ((lport >= UMAD_CA_MAX_PORTS) || (lport < 0)) {
> - __pmNotifyErr (LOG_ERR,
> - "%s(%d): port number %d is out of bounds for "
> - "HCA %s\n",
> - confpath, lcnt, lport, local);
> - } else if (hca->lports[lport].hndl == NULL) {
> - __pmNotifyErr (LOG_ERR,
> - "%s(%d): port %s:%d has failed initialization\n",
> - confpath, lcnt, local, lport);
> - } else if ((ps = (port_state_t *)calloc(1, sizeof(port_state_t)))
> == NULL) {
> - __pmNotifyErr (LOG_ERR, "Out of memory to save state for %s\n",
> - name);
> - } else {
> - int inst;
> - ps->guid = guid;
> - ps->remport = rport;
> - ps->lport = hca->lports + lport;
> - ps->portid.lid = -1;
> - ps->timeout = 1000;
> -
> - if ((inst = pmdaCacheStore (itab[IB_PORT_INDOM].it_indom,
> - PMDA_CACHE_ADD, name, ps)) < 0) {
> - __pmNotifyErr (LOG_ERR,
> - "Cannot add %s to the cache - %s\n",
> - name, pmErrStr(inst));
> - free (ps);
> - } else {
> - portcount++;
> - }
> + continue;
> }
> +
> + monitor_guid(itab, name, guid, rport, local, lport);
> }
> }
> - (*closef) (fconf);
> +}
> +
> +int
> +ib_load_config(const char *cp, int writeconf, pmdaIndom *itab, unsigned int
> nindoms)
> +{
> + char hcas[IBPMDA_MAX_HCAS][UMAD_CA_NAME_LEN];
> + hca_state_t *st = NULL;
> + int i, n;
> + int (*closef)(FILE *) = fclose;
> +
> + if (nindoms <= IB_CNT_INDOM)
> + return -EINVAL;
> +
> + if (umad_init())
> + return -EIO;
> +
> + if ((n = umad_get_cas_names(hcas, ARRAYSZ(hcas)))) {
> + if ((st = calloc (n, sizeof(hca_state_t))) == NULL)
> + return -ENOMEM;
> + } else
> + /* No HCAs */
> + return 0;
> +
> + /* Open config file - if the executable bit is set then assume that
> + * user wants it to be a script and run it, otherwise try loading it.
> + */
> + strcpy(confpath, cp);
> + if (access(confpath, F_OK) == 0) {
> + if (writeconf) {
> + __pmNotifyErr(LOG_ERR,
> + "Config file exists and writeconf arg was given to pmdaib.
> "
> + "Using existing config file");
> + writeconf = 0;
> + }
> +
> + if (access(confpath, X_OK)) {
> + /* Not an executable, just read it */
> + fconf = fopen (confpath, "r");
> + } else if (!writeconf) {
> + fconf = popen(confpath, "r");
> + closef = pclose;
> + }
> + } else if (writeconf) {
> + fconf = fopen(confpath, "w");
> + }
> +
> + for (i=0; i < n; i++) {
> + if (umad_get_ca(hcas[i], &st[i].ca) == 0) {
> + int e = pmdaCacheStore(itab[IB_HCA_INDOM].it_indom, PMDA_CACHE_ADD,
> + st[i].ca.ca_name, &st[i].ca);
> +
> + if (e < 0) {
> + __pmNotifyErr(LOG_ERR,
> + "Cannot add instance for %s to the cache - %s\n",
> + st[i].ca.ca_name, pmErrStr(e));
> + continue;
> + }
> +
> + foreachport(st+i, openumadport, NULL);
> + if (fconf == NULL || writeconf)
> + /* No config file - monitor local ports */
> + foreachport(st+i, monitorport, itab);
> + if (writeconf)
> + foreachport(st+i, printportconfig, fconf);
> + }
> + }
> +
> + if (fconf) {
> + parse_config(itab);
> + (*closef)(fconf);
> + }
>
> if (!portcount) {
> - __pmNotifyErr (LOG_INFO,
> - "No ports found in configuration file %s\n",
> - confpath);
> + __pmNotifyErr(LOG_INFO, "No IB ports found to monitor");
> }
> -
> +
> itab[IB_CNT_INDOM].it_set = (pmdaInstid
> *)calloc(ARRAYSZ(mad_cnt_descriptors),
> sizeof(pmdaInstid));
>
> if (itab[IB_CNT_INDOM].it_set == NULL) {
> - return (-ENOMEM);
> + return -ENOMEM;
> }
>
> itab[IB_CNT_INDOM].it_numinst = ARRAYSZ(mad_cnt_descriptors);
> @@ -351,8 +398,8 @@ ib_load_config(const char *confpath, pmdaIndom *itab,
> unsigned int nindoms)
> itab[IB_CNT_INDOM].it_set[i].i_name = mad_cnt_descriptors[i].name;
>
> }
> -
> - return (0);
> +
> + return 0;
> }
>
> static char *
> @@ -869,7 +916,7 @@ ib_store(pmResult *result, pmdaExt *pmda)
> return (PM_ERR_INST);
> }
> break;
> -
> +
> case METRIC_ib_control_hiwat:
> if ((id < 0) ||
> (id > pmda->e_indoms[IB_CNT_INDOM].it_numinst)) {
> @@ -884,7 +931,7 @@ ib_store(pmResult *result, pmdaExt *pmda)
> }
> }
> }
> - return (0);
> + return 0;
> }
>
>
> diff --git a/src/pmdas/ib/ibpmda.h b/src/pmdas/ib/ibpmda.h
> index 153743b..fae2c3c 100644
> --- a/src/pmdas/ib/ibpmda.h
> +++ b/src/pmdas/ib/ibpmda.h
> @@ -22,10 +22,10 @@
> #include "domain.h"
>
>
> -void ibpmda_init (const char *configpath, pmdaInterface *);
> +void ibpmda_init (const char *configpath, int, pmdaInterface *);
>
> int ib_fetch_val(pmdaMetric *, unsigned int, pmAtomValue *);
> -int ib_load_config(const char *, pmdaIndom *, unsigned int);
> +int ib_load_config(const char *, int, pmdaIndom *, unsigned int);
> void ib_rearm_for_update(void *);
> void ib_reset_perfcounters (void *);
> int ib_store(pmResult *, pmdaExt *);
> diff --git a/src/pmdas/ib/pmda.c b/src/pmdas/ib/pmda.c
> index 6a8e02d..d5557b3 100644
> --- a/src/pmdas/ib/pmda.c
> +++ b/src/pmdas/ib/pmda.c
> @@ -297,7 +297,7 @@ ib_fetch(int numpmid, pmID pmidlist[], pmResult **resp,
> pmdaExt *pmda)
> }
>
> void
> -ibpmda_init(const char *confpath, pmdaInterface *dp)
> +ibpmda_init(const char *confpath, int writeconf, pmdaInterface *dp)
> {
> char defconf[MAXPATHLEN];
> int sep = __pmPathSeparator();
> @@ -320,7 +320,7 @@ ibpmda_init(const char *confpath, pmdaInterface *dp)
> }
>
>
> - if ((dp->status = ib_load_config (confpath, indomtab,
> ARRAYSZ(indomtab))))
> + if ((dp->status = ib_load_config(confpath, writeconf, indomtab,
> ARRAYSZ(indomtab))))
> return;
>
> for (i=0; i < ARRAYSZ(indomtab); i++) {
>
>
> _______________________________________________
> pcp mailing list
> pcp@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/pcp
|