On Mon, Jan 12, 2026 at 10:56:54AM +0000, Hector CAO via Devel wrote:
> Thanks Andreas,
>
> Really appreciate your feedback and suggestions,
>
> I took a look at your revised patch, it looks great,
> I have however a question:
>
> +if not get_option('msr_module_load').disabled()
> + if host_machine.system() != 'linux'
> + if get_option('msr_module_load').enabled()
> + error('msr module loading can be enabled on Linux only')
> + endif
> + endif
> + if host_machine.cpu_family() not in [ 'x86', 'x86_64' ]
> + if get_option('msr_module_load').enabled()
> + error('msr module loading can be enabled on x86 only')
> + endif
> + endif
> + conf.set('WITH_MSR_MODULE_LOAD', 1)
> +endif
>
> Do we need the second "if get_option('msr_module_load').enabled()" since we
> already check it in the outer if ?
Yes. The outer check is there to skip the entire block if the user
explicitly disabled the feature, while the inner ones are to ensure
that we error out if the user explicitly asked for it to be enabled
but it's not possible to do so (wrong OS or architecture).
But I have actually gotten it wrong, because as it is it doesn't
ensure that installation of the file is automatically disabled if the
OS is not Linux or the architecture is not x86. This is what it
should look like instead:
if not get_option('msr_module_load').disabled()
msr_module_load = true
if host_machine.system() != 'linux'
if get_option('msr_module_load').enabled()
error('msr module loading can be enabled on Linux only')
else
msr_module_load = false
endif
endif
if host_machine.cpu_family() not in [ 'x86', 'x86_64' ]
if get_option('msr_module_load').enabled()
error('msr module loading can be enabled on x86 only')
else
msr_module_load = false
endif
endif
if msr_module_load
conf.set('WITH_MSR_MODULE_LOAD', 1)
endif
endif
I believe it's correct this time around O:-)
--
Andrea Bolognani / Red Hat / Virtualization
