Re: [PATCH v2 1/4] xen/arm: optimize the size of struct vcpu

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 12/19/25 3:16 PM, Jan Beulich wrote:
> On 19.12.2025 14:32, Oleksii Kurochko wrote:
> > On 12/18/25 7:15 PM, Andrew Cooper wrote:
> > > >    int vcpu_vgic_free(struct vcpu *v)
> > > >    {
> > > > -    xfree(v->arch.vgic.private_irqs);
> > > > +    xfree(v->arch.vgic->private_irqs);
> > > > +    xfree(v->arch.vgic);
> > > > +
> > > >        return 0;
> > > >    }
> > > Free functions should be idempotent.  This was buggy before, even moreso
> > > now.
> > Was it really buggy before in terms of idempotent.
> >
> > It seems like xfree() can handle the case when v->arch.vgic.private_irqs is 
> > NULL.
> > Should I still have:
> >    if ( v->arch.vgic->private_irqs )
> >       XFREE(v->arch.vgic->private_irqs);
> > ?
> No, and iirc Andrew also didn't ask for this.
Andrew wrote:
```
Free functions should be idempotent.  This was buggy before, even moreso
now.  It wants to be:

void vcpu_vgic_free(struct vcpu *v)
{
     if ( v->arch.vgic )
     {
         XFREE(v->arch.vgic->private_irqs);
         XFREE(v->arch.vgic);
     }
}

Given the type change, this probably wants splitting out into an earlier
patch.

Given the fact that the single caller doesn't even check the return
value, you're fixing a MISRA violation by making it void.
```

It isn't clear what "This was buggy before" means. In my changes
it will be an issue if vcpu_vgic_free() would be called twice because
without "if ( v->arch.vgic )" we will have NULL pointer dereference
XFREE(v->arch.vgic->private_irqs) here. But considering that before
it was just:
  int vcpu_vgic_free(struct vcpu *v)
  {
        xfree(v->arch.vgic.private_irqs);
        return 0;
  }
In that case, it seems that calling|vcpu_vgic_free()| twice would not cause
anything serious to happen. Am I misunderstanding what was buggy in the original
code, or probably I'm misunderstanding what is idempotent?

~ Oleksii