Re: [Qemu-devel] [PATCH 1/1 v4] Allow machines to configure the QEMU_VERSION that's exposed via hardware

[Peter Maydell]

On 22 May 2012 22:10, Crístian Viana <address@hidden> wrote:
> Based on the following conversation:
>
> http://mid.gmane.org/address@hidden
>
>> Which reminds me - qemu sticks the release version in
>> guest visible places like CPU version.
>> This is wrong and causes windows guests to print messages
>> about driver updates when you switch.
>> We should find all these places and stop doing this.
>
> There is a new field on the struct QEmuMachine, hw_version, which may
> contain the version that the specific machine should report. If that
> field is set, then that machine will report that version to the virtual
> machine.

OK, this has been bugging me for the last three versions, and
since I'm complaining about other things anyway: can you reword
this commit message, please, so that it is a standalone paragraph
explaining (a) what the commit does and (b) why it is doing it, rather
than being a combination of an unattributed quote from Anthony and some
new text from you. Commit messages aren't emails.
Explanatory remarks like the URL to the previous discussion can go
below the '---' line where they won't appear in the formal git commit
message.

> Signed-off-by: Crístian Viana <address@hidden>
> ---

When you're posting a new version of a patch please include a
summary of changes since the previous version below the '---'
line.

> diff --git a/hw/nseries.c b/hw/nseries.c
> index a5cfa8c..9d07cb8 100644
> --- a/hw/nseries.c
> +++ b/hw/nseries.c
> @@ -1247,7 +1247,7 @@ static int n8x0_atag_setup(void *p, int model)
>     stw_raw(w ++, 24);                         /* u16 len */
>     strcpy((void *) w, "hw-build");            /* char component[12] */
>     w += 6;
> -    strcpy((void *) w, "QEMU " QEMU_VERSION);  /* char version[12] */
> +    sprintf((void *) w, "QEMU %s", qemu_get_version()); /* char version[12] 
> */

So when you posted the previous version of your patch it was pointed
out that this is a buffer overflow:
http://lists.gnu.org/archive/html/qemu-devel/2012-04/msg01657.html

You need to fix this.

> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> index 51c27b4..28c2b05 100644
> --- a/hw/usb/redirect.c
> +++ b/hw/usb/redirect.c
> @@ -143,8 +143,6 @@ static void usbredir_interrupt_packet(void *priv, 
> uint32_t id,
>  static int usbredir_handle_status(USBRedirDevice *dev,
>                                        int status, int actual_len);
>
> -#define VERSION "qemu usb-redir guest " QEMU_VERSION
> -
>  /*
>  * Logging stuff
>  */
> @@ -794,6 +792,9 @@ static void usbredir_open_close_bh(void *opaque)
>  {
>     USBRedirDevice *dev = opaque;
>     uint32_t caps[USB_REDIR_CAPS_SIZE] = { 0, };
> +    char version[32];
> +
> +    snprintf(version, sizeof(version), "qemu usb-redir guest %s", 
> qemu_get_version());

The questions about the usb-redir version still apply too:
 http://lists.gnu.org/archive/html/qemu-devel/2012-04/msg01700.html

Don't just leave things unchanged from previous versions that were objected
to without explicitly mentioning them in the below-the-'---' commentary (ie
explaining why you decided not to change them), please. Otherwise reviewers
have to go through the whole thing with a fine tooth comb rechecking whether
you've actually fixed anything.

thanks
-- PMM