Re: [PATCH] hw/ide: Add the possibility to disable the CompactFlash device in the build

[Thomas Huth <thuth@redhat.com>]

On 01/02/2024 13.54, BALATON Zoltan wrote:
> On Thu, 1 Feb 2024, Thomas Huth wrote:
>> On 01/02/2024 13.39, BALATON Zoltan wrote:
>>> On Thu, 1 Feb 2024, Thomas Huth wrote:
>>>> For distros like downstream RHEL, it would be helpful to allow to disable
>>>> the CompactFlash device. For making this possible, we need a separate
>>>> Kconfig switch for this device, and the code should reside in a separate
>>>> file.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>> hw/ide/qdev-ide.h  | 41 ++++++++++++++++++++++++++++++++
>>>> hw/ide/cf.c        | 58 ++++++++++++++++++++++++++++++++++++++++++++++
>>>> hw/ide/qdev.c      | 51 ++--------------------------------------
>>>> hw/ide/Kconfig     |  4 ++++
>>>> hw/ide/meson.build |  1 +
>>>> 5 files changed, 106 insertions(+), 49 deletions(-)
>>>> create mode 100644 hw/ide/qdev-ide.h
>>>> create mode 100644 hw/ide/cf.c
>>>>
>>>> diff --git a/hw/ide/qdev-ide.h b/hw/ide/qdev-ide.h
>>>> new file mode 100644
>>>> index 0000000000..3dd977466c
>>>> --- /dev/null
>>>> +++ b/hw/ide/qdev-ide.h
>>>
>>> This may be unrelated to this patch but we already have 
>>> include/hw/ide/internal.h which may be a place these should go in but 
>>> that header is in inlcude because some files outside hw/ide include it. 
>>> I've found three places that include ide/internal.h: hw/arm/sbsa-ref.c, 
>>> hw/i386/pc.c and hw/misc/macio.h. Only macio is really needing internal 
>>> IDE parts the other two just uses some functions so macio is probably the 
>>> reason this wasn't cleaned up yet. In any case, maybe this could go in 
>>> include/hw/ide/internal.h to avoid introducing a new header or somehow 
>>> make this a local header where non-public parts of hw/ide/internal.h 
>>> could be moved in the future. Such as rename include/hw/ide/internal.h to 
>>> ide.h and name this one internal.h maybe?
>>
>> I don't like headers that much that just collect a lot of only slightly 
>> related things. That only causes problems again when you have to 
>> unentangle the stuff one day. So what's wrong with having a dedicated 
>> header for the stuff in hw/ide/qdev.c ?
> 
> Maybe that it's not obvious from the name that it belongs to qdev.c as the 
> names are not the same.

I didn't want to just name the header "qdev.h" since that could easily be 
confused in #include statements...
IMHO qdev.c is already a very bad idea for a file name here... maybe 
something like ide-dev.c and ide-dev.h would be better?

> Also some of the qdev stuff that should be in this 
> header are in include/hw/ide/internal.h so these will still be split 
> arbitrarily.

Oh, well, it seems to be a mess already... hw/ide/pci.h includes the 
internal.h header and thus opens the internal definitions to all PCI-based 
IDE devices :-/

If we can agree on a better name for qdev-ide.h, I can try to clean that 
mess up a little bit...

  Thomas