Re: [Qemu-devel] [Patch][Bug 618533]OpenSolaris guest fails to see the Solaris partitions of a physical disk

[Kevin Wolf <kwolf@redhat.com>]

Am 28.08.2010 09:17, schrieb devsk:
> Attached is the patch for fixing the issue reported in bug 618533.
> 
> The patch applies clean against 0.12.5 as well as git version.
> 
> -devsk
> PS: I am not on the list. Please CC me.
> 

> Signed Off by: devsk <funtoos@yahoo.com>
> =============================================

Please include a patch description that can be used as a commit message.
If you use git format-patch, you make it easiest for maintainers to
apply your patch.

Also, the tag is spelled "Signed-off-by:" and you should use your real
name there.

> --- hw/ide/core.c.orig  2010-08-16 20:16:11.168843003 -0700
> +++ hw/ide/core.c   2010-08-16 20:37:48.979843000 -0700
> @@ -109,7 +109,6 @@
>      put_le16(p + 0, 0x0040);
>      put_le16(p + 1, s->cylinders);
>      put_le16(p + 3, s->heads);
> -    put_le16(p + 4, 512 * s->sectors); /* XXX: retired, remove ? */

Just curious, is removing this field actually required?

>      put_le16(p + 5, 512); /* XXX: retired, remove ? */
>      put_le16(p + 6, s->sectors);
>      padstr((char *)(p + 10), s->drive_serial_str, 20); /* serial number */
> @@ -134,8 +133,16 @@
>      put_le16(p + 58, oldsize >> 16);
>      if (s->mult_sectors)
>          put_le16(p + 59, 0x100 | s->mult_sectors);
> -    put_le16(p + 60, s->nb_sectors);
> -    put_le16(p + 61, s->nb_sectors >> 16);
> +    if (s->nb_sectors <= (1 << 28) - 1)
> +    {

You may write the same condition a bit simpler as (s->nb_sectors < (1 <<
28)).

Please put the brace on the same line as the if, see the CODING_STYLE
file in the source. Also, indentation should be four spaces.

> +      put_le16(p + 60, s->nb_sectors);
> +      put_le16(p + 61, s->nb_sectors >> 16);
> +    }
> +    else
> +    {
> +      put_le16(p + 60, ((1 << 28) - 1) & 0xffff);
> +      put_le16(p + 61, ((1 << 28) - 1) >> 16);

Hm, I guess it's more readable like this:

    ....
} else {
    put_le16(p + 60, 0xffff);
    put_le16(p + 61, 0xffff);
}

> +    }
>      put_le16(p + 62, 0x07); /* single word dma0-2 supported */
>      put_le16(p + 63, 0x07); /* mdma0-2 supported */
>      put_le16(p + 65, 120);
> @@ -156,7 +163,7 @@
>      else
>           put_le16(p + 85, (1 << 14) | 1);
>      /* 13=flush_cache_ext,12=flush_cache,10=lba48 */
> -    put_le16(p + 86, (1 << 14) | (1 << 13) | (1 <<12) | (1 << 10));
> +    put_le16(p + 86, (1 << 13) | (1 <<12) | (1 << 10));
>      /* 14=set to 1, 1=smart self test, 0=smart error logging */
>      put_le16(p + 87, (1 << 14) | 0);
>      put_le16(p + 88, 0x3f | (1 << 13)); /* udma5 set and supported */

The logic looks correct. Please CC me when you submit v2 of the patch.

Kevin