Re: [PATCH v3 2/9] add vnc h264 encoder

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Apr 24, 2025 at 11:28:34AM +0200, Dietmar Maurer wrote:
> > > > > +void vnc_h264_clear(VncState *vs)
> > > > > +{
> > > > > +    if (!vs->h264) {
> > > > > +        return;
> > > > > +    }
> > > >
> > > > unnecessary
> > >
> > > This is required. For example if you disable h264, vs->h264 is
> > > always NULL, and we unconditionally call vnc_h264_clear().
> > >
> > > Why do you think this is unnecessary?
> > 
> > and there are already checks for NULL, no need to do it twice, do it
> > where it is actually necessary.
> 
> There is no check in destroy_encoder_context(), so this will generate a core dump.
> 
> So what do you mean by "where it is actually necessary"?
> 
> The final code looks like:
> 
> void vnc_h264_clear(VncState *vs)
> {
>     // Assume we remove this check ...
>     // if (!vs->h264) {
>     //    return;
>     //}
> 
>     // will trigger a core dump
>     notifier_remove(&vs->h264->shutdown_notifier);
> 
>     // will trigger a core dump
>     destroy_encoder_context(vs->h264);
>     // will trigger a core dump
>     g_free(vs->h264->encoder_name);
> 
>     g_clear_pointer(&vs->h264, g_free);
> }
> 
> Where do you want the check for NULL exactly? At the call site?

IMHO checking in vnc_h264_clear is correct - it is the expected pattern
that deallocation functions accept NULL and act as a no-op in that case.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|