[lttng-dev] [PATCH lttng-tools 2/2] Fix: lttng-crash: remove tmp working directory

[Jérémie Galarneau]

Merged with modifications, see below.

Thanks!
Jérémie

On Thu, Sep 3, 2015 at 5:52 PM, Jonathan Rajotte
<jonathan.rajotte-julien at efficios.com> wrote:
> Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien at efficios.com>
> ---
>  src/bin/lttng-crash/lttng-crash.c | 65 +++++++++++++++++++++++++++++----------
>  1 file changed, 48 insertions(+), 17 deletions(-)
>
> diff --git a/src/bin/lttng-crash/lttng-crash.c b/src/bin/lttng-crash/lttng-crash.c
> index 8e57034..00a97df 100644
> --- a/src/bin/lttng-crash/lttng-crash.c
> +++ b/src/bin/lttng-crash/lttng-crash.c
> @@ -1053,35 +1053,62 @@ end:
>  }
>
>  static
> -int delete_trace(const char *trace_path)
> +int delete_recursive_dir(const char *path)

Renamed to "delete_dir_recursive".

>  {
> -       DIR *trace_dir;
> -       int trace_dir_fd, ret = 0, closeret;
> +       DIR *dir;
> +       int dir_fd, ret = 0, closeret;
>         struct dirent *entry;
>

>         /* Open trace directory */
> -       trace_dir = opendir(trace_path);
> -       if (!trace_dir) {
> -               PERROR("Cannot open '%s' path", trace_path);
> +       dir = opendir(path);
> +       if (!dir) {
> +               PERROR("Cannot open '%s' path", path);
>                 return -1;
>         }
> -       trace_dir_fd = dirfd(trace_dir);
> -       if (trace_dir_fd < 0) {
> +       dir_fd = dirfd(dir);
> +       if (dir_fd < 0) {
>                 PERROR("dirfd");
>                 return -1;
>         }

Not introduced by your patch, but this will leak dir. I have fixed the
problem and modified your patch so that it applies cleanly on top of
it.

>
> -       while ((entry = readdir(trace_dir))) {
> +       while ((entry = readdir(dir))) {
>                 if (!strcmp(entry->d_name, ".")
>                                 || !strcmp(entry->d_name, "..")) {
>                         continue;
>                 }
>                 switch (entry->d_type) {
>                 case DT_DIR:
> -                       unlinkat(trace_dir_fd, entry->d_name, AT_REMOVEDIR);
> +               {
> +                       char subpath[PATH_MAX];
>

^ I changed this for dynamic allocation since "PATH_MAX" can become
quite large on some system configurations and we may "recurse" rather
deeply in a trace hierarchy.

> +                       strncpy(subpath, path,
> +                               sizeof(subpath));
> +                       subpath[sizeof(subpath) - 1] = '\0';
> +                       strncat(subpath, "/",
> +                               sizeof(subpath) - strlen(subpath) - 1);
> +                       strncat(subpath, entry->d_name,
> +                               sizeof(subpath) - strlen(subpath) - 1);
> +
> +                       ret = delete_recursive_dir(subpath);
> +                       if (ret) {
> +                               goto end;
> +                       }
> +
> +                       ret = unlinkat(dir_fd, entry->d_name, AT_REMOVEDIR);
> +                       if (ret) {
> +                               PERROR("Unlinking '%s'", entry->d_name);
> +                               goto end;
> +                       }

I have removed this unlinkat() to perform an rmdir() at the end to
always remove the "root" on which we are called.
This simplifies the caller in main() and this "case".

> +
>                         break;
> +               }
>                 case DT_REG:
> -                       unlinkat(trace_dir_fd, entry->d_name, 0);
> +                       ret = unlinkat(dir_fd, entry->d_name, 0);
> +                       if (ret) {
> +                               PERROR("Unlinking '%s'", entry->d_name);
> +                               goto end;
> +                       }
> +
>                         break;
>                 default:
>                         ret = -EINVAL;
> @@ -1089,13 +1116,10 @@ int delete_trace(const char *trace_path)
>                 }
>         }
>  end:
> -       closeret = closedir(trace_dir);
> +       closeret = closedir(dir);
>         if (closeret) {
>                 PERROR("closedir");
>         }
> -       if (!ret) {
> -               ret = rmdir(trace_path);
> -       }
>         return ret;
>  }
>
> @@ -1178,9 +1202,16 @@ int main(int argc, char *argv[])
>                         has_warning = 1;
>
>                 /* unlink temporary trace */
> -               ret = delete_trace(output_path);
> -               if (ret)
> +               ret = delete_recursive_dir(output_path);
> +               if (ret){

Missing space here ^.

>                         has_warning = 1;
> +               }else{

Missing spaces around "else" here ^.

> +                       ret = rmdir(output_path);
> +                       if (ret) {
> +                               PERROR("Deleting '%s'", output_path);
> +                               has_warning = 1;
> +                       }
> +               }
>         }
>         if (has_warning)
>                 exit(EXIT_FAILURE);
> --
> 2.1.4
>



-- 
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com