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

[Thomas Huth]

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