Re: [Qemu-devel] [PATCH 0/3] scsi: remove lsi53c895a_create() and lsi53c810_create() functions

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

On 07/09/18 07:27, Thomas Huth wrote:

> 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.

Well your modified function above certainly fixes my particular use case
- I'm just in the process of testing a v2 patchset that implements this
approach which I hope to post shortly.

At the end of the day I suspect that regardless of what is finally
committed, it won't really matter that much given the enormity of the
task to change the way that structs and scope are handled throughout the
codebase. So I'm happy either way :)


ATB,

Mark.