Re: [PATCH 04/12] hw/block/fdc: Expose internal header

[BALATON Zoltan]

On Sun, 17 Dec 2023, Bernhard Beschow wrote:
> Exposing the internal header allows for exposing struct FDCtrlISABus which is
> encuraged by qdev guidelines.

Hopefully the guidelines don't encourage this as object orientation indeed 
encourages object encapsulation so only the object itseld should poke its 
internals and other objects should use methods the change object state. In 
QOM some object states were exposed in public headers to allow embedding 
those objects in other objects becuase C needs the struct size to allow 
that. This was to simplify memory management so the embedded objects don't 
need to be tracked and freed but would be created and freed with the other 
object embedding it but this does not mean the other object should poke 
into these object or that this is a general guideline to expose internal 
object state. I'd say the exposed objects are an exception instead of 
recommended guideline and only allowed for objects that need to be embeded 
in others but generally object encapsulation would be better to preserve 
where possible. This patch exposes objects so others can poke into them 
which would make those other objects dependent on the implementation of 
these objects making these harder to chnage in the future so a better way 
may be to add methods to fdc and serial to allow changing their base 
address and map/unmap their ports and keep their internals unexposed.

Regards,
BALATON Zoltan

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> MAINTAINERS                                       | 2 +-
> hw/block/fdc-internal.h => include/hw/block/fdc.h | 4 ++--
> hw/block/fdc-isa.c                                | 2 +-
> hw/block/fdc-sysbus.c                             | 2 +-
> hw/block/fdc.c                                    | 2 +-
> 5 files changed, 6 insertions(+), 6 deletions(-)
> rename hw/block/fdc-internal.h => include/hw/block/fdc.h (98%)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b4718fcf59..939f518701 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1945,9 +1945,9 @@ M: John Snow <jsnow@redhat.com>
> L: qemu-block@nongnu.org
> S: Odd Fixes
> F: hw/block/fdc.c
> -F: hw/block/fdc-internal.h
> F: hw/block/fdc-isa.c
> F: hw/block/fdc-sysbus.c
> +F: include/hw/block/fdc.h
> F: include/hw/block/fdc-isa.h
> F: tests/qtest/fdc-test.c
> T: git https://gitlab.com/jsnow/qemu.git ide
> diff --git a/hw/block/fdc-internal.h b/include/hw/block/fdc.h
> similarity index 98%
> rename from hw/block/fdc-internal.h
> rename to include/hw/block/fdc.h
> index 1728231a26..acca7e0d0e 100644
> --- a/hw/block/fdc-internal.h
> +++ b/include/hw/block/fdc.h
> @@ -22,8 +22,8 @@
>  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>  * THE SOFTWARE.
>  */
> -#ifndef HW_BLOCK_FDC_INTERNAL_H
> -#define HW_BLOCK_FDC_INTERNAL_H
> +#ifndef HW_BLOCK_FDC_H
> +#define HW_BLOCK_FDC_H
>
> #include "exec/memory.h"
> #include "exec/ioport.h"
> diff --git a/hw/block/fdc-isa.c b/hw/block/fdc-isa.c
> index 6387dc94fa..7058d4118f 100644
> --- a/hw/block/fdc-isa.c
> +++ b/hw/block/fdc-isa.c
> @@ -39,6 +39,7 @@
> #include "hw/qdev-properties-system.h"
> #include "migration/vmstate.h"
> #include "hw/block/block.h"
> +#include "hw/block/fdc.h"
> #include "sysemu/block-backend.h"
> #include "sysemu/blockdev.h"
> #include "sysemu/sysemu.h"
> @@ -47,7 +48,6 @@
> #include "qemu/module.h"
> #include "trace.h"
> #include "qom/object.h"
> -#include "fdc-internal.h"
>
> OBJECT_DECLARE_SIMPLE_TYPE(FDCtrlISABus, ISA_FDC)
>
> diff --git a/hw/block/fdc-sysbus.c b/hw/block/fdc-sysbus.c
> index f18f0d19b0..cff21c02b3 100644
> --- a/hw/block/fdc-sysbus.c
> +++ b/hw/block/fdc-sysbus.c
> @@ -28,8 +28,8 @@
> #include "qom/object.h"
> #include "hw/sysbus.h"
> #include "hw/block/fdc-isa.h"
> +#include "hw/block/fdc.h"
> #include "migration/vmstate.h"
> -#include "fdc-internal.h"
> #include "trace.h"
>
> #define TYPE_SYSBUS_FDC "base-sysbus-fdc"
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 2bd6d925b5..0e2fa527f9 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -39,6 +39,7 @@
> #include "hw/qdev-properties-system.h"
> #include "migration/vmstate.h"
> #include "hw/block/block.h"
> +#include "hw/block/fdc.h"
> #include "sysemu/block-backend.h"
> #include "sysemu/blockdev.h"
> #include "sysemu/sysemu.h"
> @@ -47,7 +48,6 @@
> #include "qemu/module.h"
> #include "trace.h"
> #include "qom/object.h"
> -#include "fdc-internal.h"
>
> /********************************************************/
> /* debug Floppy devices */