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

[Bernhard Beschow]

Am 18. Dezember 2023 10:54:56 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sun, 17 Dec 2023, Bernhard Beschow wrote:
>> Am 17. Dezember 2023 15:47:33 UTC schrieb BALATON Zoltan 
>> <balaton@eik.bme.hu>:
>>> 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 depe
>ndent 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.
>> 
>> Each ISADevice sub class would need concenience methods as well as each 
>> state class. This series touches three of each: fdc, parallel, serial. And 
>> each of those need two convenience methods: set_enabled() and set_address(). 
>> This would add another 12 functions on top of the current ones.
>
>If all ISA devices need this then these should really be methods of ISADevice 
>but since that's just an empty wrapper over devices each of which handles its 
>own ports, the ISADevice does not know about those and since each device may 
>have different ports and not all of them uses portio lists for this, moving 
>port handling to ISADevice might be too big refactoring to do for this. 
>Keeping these functions with the superio component devices so their 
>implementation is kept private still worth it in my opinion so even if that 
>adds 2 functions to superio component devices (which is not all ISA devices 
>just a limited set) seems to be a better approach to me than breaking 
>encapsulation of objects. These are simple access methods for internal object 
>state which are common in object otiented programming.
>
>> Then ISASuperIODevice would require at least 6 more such methods (not 
>> counting the unneeded ones for IDE which might be desirable for 
>> consistency). So in the end we'd have at least 18 more methods. Is this 
>> really worth it?
>
>We may do without these if we say superio is just a container of components so 
>don't add forwarding methods but we can call the accessor methods of component 
>objects from vt82c686.c. That's still better than reaching into object 
>internals from foreign objects.

Version 2 is out which should address all of your comments.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> I didn't feel very comfortable going this route, so ended up with the 
>> current solution poking the states directly. I'm open to different 
>> approaches including the one above but I'd really like to know the opinion 
>> of the maintainers, too.
>> 
>> Best regards,
>> Bernhard
>> 
>>> 
>>> 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 */
>>>> 
>> 
>>