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 :|
