[Spice-devel] [PATCH spice-server 2/3] test-gst: Remove options parsing leaks

[Frediano Ziglio]

> 
> On Tue, Sep 12, 2017 at 04:11:57AM -0400, Frediano Ziglio wrote:
> > > 
> > > On Tue, Sep 12, 2017 at 03:41:15AM -0400, Frediano Ziglio wrote:
> > > > > 
> > > > > On Tue, Sep 12, 2017 at 03:19:12AM -0400, Frediano Ziglio wrote:
> > > > > > > > -    const EncoderInfo *encoder =
> > > > > > > > get_encoder_info(encoder_name);
> > > > > > > > +    const EncoderInfo *encoder = get_encoder_info(encoder_name
> > > > > > > > ?
> > > > > > > > encoder_name : "mjpeg");
> > > > > > > >      if (!encoder) {
> > > > > > > >          g_printerr("Encoder name unsupported: %s\n",
> > > > > > > >          encoder_name);
> > > > > > > 
> > > > > > > This is going to be "Encoder name unsupported: (null)" when the
> > > > > > > corresponding option is not given, ditto for the other options
> > > > > > > that
> > > > > > > you
> > > > > > > changed.
> > > > > > > 
> > > > > > 
> > > > > > No, the defaults are present so encoder won't be NULL... but I can
> > > > > > change
> > > > > > the code to make this more clear.
> > > > > 
> > > > > I'm missing something then, the code I quoted is:
> > > > > 
> > > > >      const EncoderInfo *encoder = get_encoder_info(encoder_name ?
> > > > >      encoder_name : "mjpeg");
> > > > >      if (!encoder) {
> > > > >           g_printerr("Encoder name unsupported: %s\n", encoder_name);
> > > > > 
> > > > > so it looks like encoder_name can be NULL in the call to
> > > > > get_encoder_info(), in which case we call get_encoder_info("mjpeg")
> > > > > instead of get_encoder_info(encoder_name), but right after that, in
> > > > > the
> > > > > g_printerr call, encoder_name cannot be NULL?
> > > > > 
> > > > 
> > > > No, get_encoder_info("mjpeg") is not supposed to return NULL... but I
> > > > changed
> > > > the code to understand it even without reading get_encoder_info,
> > > > better.
> > > 
> > > But you are printing "encoder_name" (the input parameter), not "encoder"
> > > (the
> > > output) !
> > > 
> > 
> > Yes, the name is encoder_name, the print is referring to name.
> > By the way, changed code in last version I sent, should be clear, now is
> > 
> >     const EncoderInfo *encoder =
> >         encoder_name ? get_encoder_info(encoder_name) : encoder_info_mjpeg;
> >     if (!encoder) {
> >         g_printerr("Encoder name unsupported: %s\n", encoder_name);
> >         exit(1);
> >     }
> 
> Which is still awful to read..
> 

This is a personal opinion, I don't see any opposite advice in the style
document, not any esoteric feature used

> if (encoder_name != NULL) {
>     encoder = get_encoder_info(encoder_name);
>     if (!encoder) {
>          g_printerr("Encoder name unsupported: %s\n", encoder_name);
>          exit(1);
>     }
> } else {
>     encoder = encoder_info_mjpeg;
> }
> 

I'll update it.

> 
> 
> > 
> > > Christophe
> > > 
> > 

Frediano