[dpdk-dev] [PATCH v1 05/32] eal/trace: add internal trace init and fini interface

[Jerin Jacob]

On Thu, Mar 19, 2020 at 4:05 PM Mattias Rönnblom
<mattias.ronnblom at ericsson.com> wrote:
>
> On 2020-03-18 20:02, jerinj at marvell.com wrote:
> > From: Jerin Jacob <jerinj at marvell.com>

> > +static bool
> > +trace_entry_compare(const char *name)
> > +{
> > +     struct trace_point_head *tp_list = trace_list_head_get();
> > +     struct trace_point *tp;
> > +     int count = 0;
> > +
> > +     STAILQ_FOREACH(tp, tp_list, next) {
> > +             if (strncmp(tp->name, name, TRACE_POINT_NAME_SIZE) == 0)
> > +                     count++;
> > +             if (count > 1) {
> > +                     trace_err("found duplicate entry %s", name);
> > +                     rte_errno = EEXIST;
> > +                     return 1;
>
> Maybe an assertion would be better here, especially since the caller
> doesn't seem to care about the fact the list is inconsistent.

assertion/panic are getting removed from the libraries.
This error propagates up to rte_eal_init() failure. So, the current
scheme is OK.

>
> Change 1 -> true.

Will fix it in v2.

>
> > +             }
> > +     }
> > +     return 0;
> return false;


Will fix it in v2.

> > +}
> > +

> > +static int
> > +trace_dir_default_path_get(char *dir_path)
> > +{
> > +     struct trace *trace = trace_obj_get();
> > +     uint32_t size = sizeof(trace->dir);
> > +     struct passwd *pwd;
> > +     char *home_dir;
> > +
> > +     /* First check for shell environment variable */
> > +     home_dir = getenv("HOME");
> In case of the application being started with 'sudo', $HOME will not be
> "/root", but rather the original user. This sounds like a bug to me.

When the application started with sudo, the trace will be written
by the "root" user. Probably it is not good to have some file whose
ownership as root in /home/user/dpdk-trace/ where the user can
not delete it.

A similar scheme followed in LTTng for trace directory.
I am open to a better scheme if any.


> > +     if (home_dir == NULL) {
> > +             /* Fallback to password file entry */
> > +             pwd = getpwuid(getuid());
> > +             if (pwd == NULL)
> > +                     return -EINVAL;
> > +
> > +             home_dir = pwd->pw_dir;
> > +     }
> > +
> > +     /* Append dpdk-traces to directory */
> > +     if (snprintf(dir_path, size, "%s/dpdk-traces/", home_dir) < 0)
> > +             return -ENAMETOOLONG;
> > +
> > +     return 0;
> > +}
> > +
> > +int
> > +trace_mkdir(void)
> > +{
> > +     struct trace *trace = trace_obj_get();
> > +     char session[TRACE_DIR_STR_LEN];
> > +     char *dir_path;
> > +     int rc;
> > +
> > +     if (!trace->dir_offset) {
> > +             dir_path = (char *)calloc(1, sizeof(trace->dir));
> calloc() returns a void pointer, so no need to cast it.

Will fix it in v2.

> > +             if (dir_path == NULL) {
> > +                     trace_err("fail to allocate memory\n");
> > +                     return -ENOMEM;
> > +             }
> > +
> > +             rc = trace_dir_default_path_get(dir_path);
> > +             if (rc < 0) {
> > +                     trace_err("fail to get default path\n");
> > +                     free(dir_path);
> > +                     return rc;
> > +             }
> > +
> > +     }
> > +
> > +     /* Create the path if it t exist, no "mkdir -p" available here */
> > +     rc = mkdir(trace->dir, 0700);
> > +     if (rc < 0 && errno != EEXIST) {
> > +             trace_err("mkdir %s failed [%s]", trace->dir, strerror(errno));
> > +             rte_errno = errno;
> > +             return -rte_errno;
> > +     }
> > +
> > +     rc = trace_session_name_generate(session);
> > +     if (rc < 0)
> > +             return rc;
> > +
> > +     rc = mkdir(trace->dir, 0700);
> > +     if (rc < 0) {
> > +             trace_err("mkdir %s failed [%s]", trace->dir, strerror(errno));
> > +             rte_errno = errno;
> > +             return -rte_errno;
> > +     }
> > +
> > +     RTE_LOG(INFO, EAL, "Trace dir: %s\n", trace->dir);
> > +     return 0;
>
> Is dir_path leaked? I can't find a free(), except in one of the error cases.

Not is in "struct trace" as memory.

struct trace {
        char dir[PATH_MAX];
..
}