On 2018-09-06 19:15, Mark Cave-Ayland wrote:
> On 06/09/18 17:40, Thomas Huth wrote:
[...]
> Amusingly the main reason I need to expose the LSIState at all is to be
> able to call scsi_bus_legacy_handle_cmdline() on the SCSI bus object
> itself. I guess you could say that this is an argument in favour of the
> existing approach, but then you're effectively moving back to the
> equivalent of _init() functions for this one particular case which these
> days are considered to be bad.
If you mind that the "init" function creates the object, too, then
simply remove the two "lsi53c*_create" functions and provide a function
that looks like this instead:
void lsi53c8xx_handle_legacy_cmdline(DeviceState *lsi_dev)
{
LSIState *s = LSI53C895A(lsi_dev);
scsi_bus_legacy_handle_cmdline(&s->bus);
}
Then you can create the device from another .c file and simply call this
new function instead of scsi_bus_legacy_handle_cmdline() in that other
.c file.
In the long run, I think we should maybe also rather get rid of
scsi_bus_legacy_handle_cmdline() and either remove -drive if=scsi or
provide another more generic solution instead (e.g. some code that scans
the qdev tree for SCSI controllers and attaches the devices declared
with -drive if=scsi automatically there).
> My feeling is that since the pattern of a separate header with struct
> and QOM macros (or "modern" style) is used throughout the rest of the
> codebase, why should we make an exception for this one particular case?
I don't mind too much, so if you post your series again, I won't object
again. But still, even if it's a little bit more work for you now, if we
reconsider in a couple of years that information hiding would be a good
idea, it very likely way more work to make the struct private again, so
I'd really prefer if you could keep it private now if possible.
Thomas
