Re: [PATCH v2 09/10] trace-cmd: Use the new flow when creating output handler

[Tzvetomir Stoyanov <tz.stoyanov@gmail.com>]

On Wed, Nov 24, 2021 at 6:08 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Thu, 11 Nov 2021 17:07:30 +0200
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
>
> > @@ -4437,6 +4456,30 @@ static void write_guest_file(struct buffer_instance *instance)
> >       free(temp_files);
> >  }
> >
> > +static struct tracecmd_output *create_output(struct common_record_context *ctx)
> > +{
> > +     struct tracecmd_output *out;
> > +     int fd;
> > +
> > +     fd = open(ctx->output, O_RDWR | O_CREAT | O_TRUNC | O_LARGEFILE, 0644);
>
> I stopped at this patch because I really dislike the above.
>
> Why don't we have:
>
>         tracecmd_output_allocate(file);
>
> and
>         tracecmd_output_allocate_fd(fd);
>
> Where tracecmd_output_allocate(file) does:
>
> struct tracecmd_output *tracecmd_output_allocate(const char *file)
> {
>         int fd;
>
>         fd = open(file, O_RDWR | O_CREAT | O_TRUNC | O_LARGEFILE, 0644);
>         if (fd < 0)
>                 return NULL;
>         return tracecmd_output_allocate_fd(fd);
> }
>
> ?
>
> Then we could remove a lot of these duplicate opens all over the place.
>
> Although, I'm not sure I like the name allocate. It probably should be called:
>
> tracecmd_output_create();
>
> and we keep tracecmd_output_allocate() as is?
>

 There is already such API, I'll replace that pattern with a call to this:
   struct tracecmd_output *tracecmd_create_init_file(const char *output_file)

> -- Steve
>
>
> > +     if (fd < 0)
> > +             return NULL;
> > +
> > +     out = tracecmd_output_allocate(fd);
> > +     if (!out)
> > +             goto error;
> > +     if (tracecmd_output_write_headers(out, listed_events))
> > +             goto error;
> > +     return out;
> > +error:
> > +     if (out)



-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center