[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: BALATON Zoltan
Subject: Re: [PATCH 04/12] hw/block/fdc: Expose internal header
Date: Tue, 19 Dec 2023 00:44:33 +0100 (CET)

On Mon, 18 Dec 2023, Bernhard Beschow wrote:
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 de
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 

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.

I think this version looks better. I only have time for somw preliminary comments after a quick look now, I'll plan to give it more testing during Xmas holiday.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]