| Message ID | 20231217144148.15511-5-shentey@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/isa/vt82c686: Implement relocation and toggling of SuperI/O functions | expand |
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 */ >
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 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. 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. 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? 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 */ >>
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. 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 */ >>> > >
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 */ >>>> >> >>
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 pe >> 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. 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. Regards, BALATON Zoltan
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 */
Exposing the internal header allows for exposing struct FDCtrlISABus which is encuraged by qdev guidelines. 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%)