Re: [PATCH v5 1/3] hw/i386/pc: Refactor logic to set SMBIOS defaults

[Suthikulpanit, Suravee]

On 6/7/2023 3:11 PM, Daniel P. Berrangé wrote:
> On Tue, Jun 06, 2023 at 09:49:37PM -0500, Suravee Suthikulpanit wrote:
> > Into a helper function pc_machine_init_smbios() in preparation for
> > subsequent code to upgrade default SMBIOS entry point type.
> >
> > Then, call the helper function from the pc_machine_initfn() to eliminate
> > duplicate code in pc_q35.c and pc_pixx.c. However, this changes the
> > ordering of when the smbios_set_defaults() is called to before
> > pc_machine_set_smbios_ep() (i.e. before handling the user specified
> > QEMU option "-M ...,smbios-entry-point-type=[32|64]" to override
> > the default type.)
> >
> > Therefore, also call the helper function in pc_machine_set_smbios_ep()
> > to update the defaults.
>
> This is unsafe - smbios_set_defaults is only intended to be called
> once. Calling it twice leads to a SEGV due to double-free
>
> $  ./build/qemu-system-x86_64 -machine pc,smbios-entry-point-type=64 -smbios 
> file=/tmp/smbios_entry_point
> Segmentation fault (core dumped)

Thanks for pointing this out. I missed this

> IMHO we should just not do this refactoring. The existing duplicated
> code is not a significant burden, and thus is better than having to
> workaround calling pc_machine_set_smbios_ep too early in startup.

Ok

Thanks,
Suravee