[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [RFC] ide: Don't use qemu_hw_version() for
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-block] [Qemu-devel] [RFC] ide: Don't use qemu_hw_version() for firmware revision |
Date: |
Thu, 12 Nov 2015 09:27:56 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eduardo Habkost <address@hidden> writes:
> The IDEState.version field is used for firmware version
> information returned to the guest. Updating firmware information
> on QEMU upgrade is supposed to be acceptable, so IDE doesn't need
> the version compatibility magic of qemu_hw_version() and can use
> QEMU_VERSION directly.
>
> Signed-off-by: Eduardo Habkost <address@hidden>
> ---
> I'm sending this just to start a discussion about what exactly we
> are supposed to return to the guest on those IDE fields. Should
> we return:
>
> 1) Something that never changes and don't reveal QEMU version
> information (e.g. "QEMU")?
> 2) Something that is always the same depending on the
> machine-type (machine-type name? MachineClass.hw_version?)
> 3) Something that change every time QEMU is upgraded (QEMU_VERSION)?
> 4) Something else?
>
> This patch implements option (3).
>
> ---
> hw/ide/core.c | 2 +-
> hw/ide/internal.h | 2 ++
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 364ba21..1602707 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -2312,7 +2312,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk,
> IDEDriveKind kind,
> if (version) {
> pstrcpy(s->version, sizeof(s->version), version);
> } else {
> - pstrcpy(s->version, sizeof(s->version), qemu_hw_version());
> + pstrcpy(s->version, sizeof(s->version), QEMU_VERSION);
Is s->version migrated?
If no, live migration to a newer QEMU changes the version, doesn't it?
The "firmware upgrade is acceptable" argument doesn't apply there. I
guess a spontaneous version change is relatively unlikely to cause
trouble, but why risk it?
> }
>
> ide_reset(s);
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index e4629b0..a4277ce 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -378,6 +378,7 @@ struct IDEState {
> /* set for lba48 access */
> uint8_t lba48;
> BlockBackend *blk;
> + /* Firmware revision/version */
> char version[9];
> /* ATAPI specific */
> struct unreported_events events;
> @@ -488,6 +489,7 @@ struct IDEDevice {
> uint32_t unit;
> BlockConf conf;
> int chs_trans;
> + /* Firmware revision/version */
> char *version;
> char *serial;
> char *model;
I'd put the comment to the right of version, to make it immediately
obvious it applies to just version.