Re: snmpd_metrics: differentiate between hrSWRunName and hrSWRunPath

2023-10-24 Thread Theo Buehler
On Wed, Oct 18, 2023 at 01:49:00PM +0200, Martijn van Duren wrote:
> Right now we return the same value for both hrSWRunName and hrSWRunPath.
> hrSWRunPath should return the full path of the binary, and hrSWRunName
> a description of the running software.
> 
> Afaik there's no proper way to retrieve the full path of the running
> binary.

No there isn't. It's been discussed many times.

> However, in a lot of cases argv[0] can contain the full or
> relative path. But if argv[0] gets overwritten (like most of our
> daemons' children) it gives a more descriptive name, which is more in
> line with hrSWRunName.
> 
> netsnmp's snmpd uses argv[0] for hrSWRunPath and kinfo_proc's p_comm for
> hrSWRunName, and snmptop defaults to hrSWRunName. top(1) also defaults
> to p_comm, but contrary to top(1), snmptop allows us to switch between
> hrSWRunName, and hrSWRunPath and toggling of hrSWRunParameters
> independently, where top(1) toggles between p_comm and argv[].
> 
> So there's an argument to be made either way, but for this diff I stuck
> with netsnmp's choices.

I am fine with your choices and people feeling strongly about this had
week to object.

> 
> While here, change the buffer length from 128 to 129. HOST-RESOURCES-MIB
> allows up to 128 characters in the response, so make room for the
> terminating NUL.
> 
> Thoughts? OK?

ok tb



snmpd_metrics: differentiate between hrSWRunName and hrSWRunPath

2023-10-18 Thread Martijn van Duren
Right now we return the same value for both hrSWRunName and hrSWRunPath.
hrSWRunPath should return the full path of the binary, and hrSWRunName
a description of the running software.

Afaik there's no proper way to retrieve the full path of the running
binary, However, in a lot of cases argv[0] can contain the full or
relative path. But if argv[0] gets overwritten (like most of our
daemons' children) it gives a more descriptive name, which is more in
line with hrSWRunName.

netsnmp's snmpd uses argv[0] for hrSWRunPath and kinfo_proc's p_comm for
hrSWRunName, and snmptop defaults to hrSWRunName. top(1) also defaults
to p_comm, but contrary to top(1), snmptop allows us to switch between
hrSWRunName, and hrSWRunPath and toggling of hrSWRunParameters
independently, where top(1) toggles between p_comm and argv[].

So there's an argument to be made either way, but for this diff I stuck
with netsnmp's choices.

While here, change the buffer length from 128 to 129. HOST-RESOURCES-MIB
allows up to 128 characters in the response, so make room for the
terminating NUL.

Thoughts? OK?

martijn@

Index: mib.c
===
RCS file: /cvs/src/libexec/snmpd/snmpd_metrics/mib.c,v
retrieving revision 1.4
diff -u -p -r1.4 mib.c
--- mib.c   4 Jul 2023 11:34:19 -   1.4
+++ mib.c   18 Oct 2023 11:47:00 -
@@ -103,7 +103,9 @@ int  kinfo_proc_comp(const void *, const
 int kinfo_proc(u_int32_t, struct kinfo_proc **);
 voidkinfo_timer_cb(int, short, void *);
 voidkinfo_proc_free(void);
-int kinfo_args(struct kinfo_proc *, char **);
+int kinfo_args(struct kinfo_proc *, char ***);
+int kinfo_path(struct kinfo_proc *, char **);
+int kinfo_parameters(struct kinfo_proc *, char **);
 
 /* IF-MIB */
 struct agentx_index *ifIdx;
@@ -669,13 +671,21 @@ mib_hrswrun(struct agentx_varbind *vb)
 
if (obj == hrSWRunIndex)
agentx_varbind_integer(vb, kinfo->p_pid);
-   else if (obj == hrSWRunName || obj == hrSWRunPath)
+   else if (obj == hrSWRunName)
agentx_varbind_string(vb, kinfo->p_comm);
-   else if (obj == hrSWRunID)
+   else if (obj == hrSWRunPath) {
+   if (kinfo_path(kinfo, ) == -1) {
+   log_warn("kinfo_path");
+   agentx_varbind_error(vb);
+   return;
+   }
+   
+   agentx_varbind_string(vb, s);
+   } else if (obj == hrSWRunID)
agentx_varbind_oid(vb, AGENTX_OID(0, 0));
else if (obj == hrSWRunParameters) {
-   if (kinfo_args(kinfo, ) == -1) {
-   log_warn("kinfo_args");
+   if (kinfo_parameters(kinfo, ) == -1) {
+   log_warn("kinfo_parameters");
agentx_varbind_error(vb);
return;
}
@@ -811,25 +821,22 @@ kinfo_proc_free(void)
 }
 
 int
-kinfo_args(struct kinfo_proc *kinfo, char **s)
+kinfo_args(struct kinfo_proc *kinfo, char ***s)
 {
-   static char  str[128];
static char *buf = NULL;
static size_tbuflen = 128;
 
int  mib[] = { CTL_KERN, KERN_PROC_ARGS,
kinfo->p_pid, KERN_PROC_ARGV };
-   char*nbuf, **argv;
+   char*nbuf;
 
+   *s = NULL;
if (buf == NULL) {
buf = malloc(buflen);
if (buf == NULL)
return (-1);
}
 
-   str[0] = '\0';
-   *s = str;
-
while (sysctl(mib, nitems(mib), buf, , NULL, 0) == -1) {
if (errno != ENOMEM) {
/* some errors are expected, dont get too upset */
@@ -844,11 +851,41 @@ kinfo_args(struct kinfo_proc *kinfo, cha
buflen += 128;
}
 
-   argv = (char **)buf;
-   if (argv[0] == NULL)
-   return (0);
+   *s = (char **)buf;
+   return (0);
+}
+
+int
+kinfo_path(struct kinfo_proc *kinfo, char **s)
+{
+   static char  str[129];
+   char**argv;
+
+   if (kinfo_args(kinfo, ) == -1)
+   return (-1);
 
+   str[0] = '\0';
+   *s = str;
+   if (argv != NULL && argv[0] != NULL)
+   strlcpy(str, argv[0], sizeof(str));
+   return (0);
+}
+
+int
+kinfo_parameters(struct kinfo_proc *kinfo, char **s)
+{
+   static char  str[129];
+   char**argv;
+
+   if (kinfo_args(kinfo, ) == -1)
+   return (-1);
+
+   str[0] = '\0';
+   *s = str;
+   if (argv == NULL || argv[0] == NULL)
+   return (0);
argv++;
+
while (*argv != NULL) {
strlcat(str, *argv, sizeof(str));
argv++;