On 06/19/15 09:01, Markus Armbruster wrote:
> "Michael S. Tsirkin" <[email protected]> writes:
>
>> From: Laszlo Ersek <[email protected]>
>>
>> It is Very annoying to carry forward an outdatEd coNtroller with a mOdern
>> Machine type.
>>
>> Hence, let us not instantiate the FDC when all of the following apply:
>> - the machine type is pc-q35-2.4 or later,
>> - "-device isa-fdc" is not passed on the command line (nor in the config
>> file),
>> - no "-drive if=floppy,..." is requested.
>>
>> Cc: Markus Armbruster <[email protected]>
>> Cc: Paolo Bonzini <[email protected]>
>> Cc: Gerd Hoffmann <[email protected]>
>> Cc: John Snow <[email protected]>
>> Cc: "Gabriel L. Somlo" <[email protected]>
>> Cc: "Michael S. Tsirkin" <[email protected]>
>> Cc: Kevin Wolf <[email protected]>
>> Cc: [email protected]
>> Suggested-by: Markus Armbruster <[email protected]>
>> Signed-off-by: Laszlo Ersek <[email protected]>
>> Acked-by: Paolo Bonzini <[email protected]>
>> Reviewed-by: Michael S. Tsirkin <[email protected]>
>> Signed-off-by: Michael S. Tsirkin <[email protected]>
>> Reviewed-by: Markus Armbruster <[email protected]>
>> ---
>> hw/i386/pc_q35.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index 9f036c8..66220b3 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -392,6 +392,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
>> pc_q35_machine_options(m);
>> m->default_machine_opts = "firmware=bios-256k.bin";
>> m->default_display = "std";
>> + m->no_floppy = 1;
>> m->alias = "q35";
>> }
>
> Botched merge, see [PATCH] q35: Re-enable FDC on pc-q35-2.3 and older.
>
> Your merge is closer to a rewrite than to conflict resolution, rendering
> my R-by totally meaningless.
>
> In the future, please either fully test such a merge, or ask the
> submitter / reviewers to review and test.
Yes, please. While I *was* Cc'd on this patch in the pull request, I
didn't review it. I never expect PULLs to amount to such intrusive
changes -- I treat them only as a hint that my patch is going in -- so
if such changes are necessary (because the original patch doesn't apply
any longer), then please ask me to rebase, or give me an explicit
heads-up that my verification / testing is needed.
Thanks!
Laszlo
> For comparison, this is the patch that got actually reviewed and tested:
>
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index ad014e7..671ae69 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -424,7 +424,8 @@ static void pc_q35_init_1_4(MachineState *machine)
> #define PC_Q35_2_4_MACHINE_OPTIONS \
> PC_Q35_MACHINE_OPTIONS, \
> .default_machine_opts = "firmware=bios-256k.bin", \
> - .default_display = "std"
> + .default_display = "std", \
> + .no_floppy = 1
>
> static QEMUMachine pc_q35_machine_v2_4 = {
> PC_Q35_2_4_MACHINE_OPTIONS,
> @@ -433,7 +434,10 @@ static QEMUMachine pc_q35_machine_v2_4 = {
> .init = pc_q35_init,
> };
>
> -#define PC_Q35_2_3_MACHINE_OPTIONS PC_Q35_2_4_MACHINE_OPTIONS
> +#define PC_Q35_2_3_MACHINE_OPTIONS \
> + PC_Q35_MACHINE_OPTIONS, \
> + .default_machine_opts = "firmware=bios-256k.bin", \
> + .default_display = "std"
>
> static QEMUMachine pc_q35_machine_v2_3 = {
> PC_Q35_2_3_MACHINE_OPTIONS,
>
