Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf

Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf

[Stefan Hajnoczi]

On Fri, Jun 29, 2012 at 05:34:40PM +0200, Markus Armbruster wrote:
> Best to use the same type, to avoid unwanted truncation or sign
> extension.
> 
> BlockConf can't use plain int for cyls, heads and secs, because
> integer properties require an exact width.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  blockdev.h       |    2 +-
>  hw/hd-geometry.c |    4 ++--
>  hw/ide/core.c    |    2 +-
>  hw/scsi-disk.c   |    2 +-
>  hw/virtio-blk.c  |    2 +-
>  5 files changed, 6 insertions(+), 6 deletions(-)

It would be nice to update the hd_geometry_lchs_guess() trace event that
you added to use uint32_t + %u instead of int + %d.

Stefan

Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf

[Andreas Färber]

Am 02.07.2012 14:55, schrieb Stefan Hajnoczi:
> On Fri, Jun 29, 2012 at 05:34:40PM +0200, Markus Armbruster wrote:
>> Best to use the same type, to avoid unwanted truncation or sign
>> extension.
>>
>> BlockConf can't use plain int for cyls, heads and secs, because
>> integer properties require an exact width.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  blockdev.h       |    2 +-
>>  hw/hd-geometry.c |    4 ++--
>>  hw/ide/core.c    |    2 +-
>>  hw/scsi-disk.c   |    2 +-
>>  hw/virtio-blk.c  |    2 +-
>>  5 files changed, 6 insertions(+), 6 deletions(-)
> 
> It would be nice to update the hd_geometry_lchs_guess() trace event that
> you added to use uint32_t + %u instead of int + %d.

PRIu32?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf

[Markus Armbruster]

Andreas Färber <afaerber@suse.de> writes:

> Am 02.07.2012 14:55, schrieb Stefan Hajnoczi:
>> On Fri, Jun 29, 2012 at 05:34:40PM +0200, Markus Armbruster wrote:
>>> Best to use the same type, to avoid unwanted truncation or sign
>>> extension.
>>>
>>> BlockConf can't use plain int for cyls, heads and secs, because
>>> integer properties require an exact width.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  blockdev.h       |    2 +-
>>>  hw/hd-geometry.c |    4 ++--
>>>  hw/ide/core.c    |    2 +-
>>>  hw/scsi-disk.c   |    2 +-
>>>  hw/virtio-blk.c  |    2 +-
>>>  5 files changed, 6 insertions(+), 6 deletions(-)
>> 
>> It would be nice to update the hd_geometry_lchs_guess() trace event that
>> you added to use uint32_t + %u instead of int + %d.
>
> PRIu32?

uint32_t: Good point, will do.

Conversion specifier: trace-events routinely prints uint32_t with %d and
%x, never with PRI*32.  I'm happy to change from %d to %u, but reluctant
to add the first use of PRIu32.

Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf

[Stefan Hajnoczi]

On Mon, Jul 2, 2012 at 3:15 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Andreas Färber <afaerber@suse.de> writes:
>
>> Am 02.07.2012 14:55, schrieb Stefan Hajnoczi:
>>> On Fri, Jun 29, 2012 at 05:34:40PM +0200, Markus Armbruster wrote:
>>>> Best to use the same type, to avoid unwanted truncation or sign
>>>> extension.
>>>>
>>>> BlockConf can't use plain int for cyls, heads and secs, because
>>>> integer properties require an exact width.
>>>>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> ---
>>>>  blockdev.h       |    2 +-
>>>>  hw/hd-geometry.c |    4 ++--
>>>>  hw/ide/core.c    |    2 +-
>>>>  hw/scsi-disk.c   |    2 +-
>>>>  hw/virtio-blk.c  |    2 +-
>>>>  5 files changed, 6 insertions(+), 6 deletions(-)
>>>
>>> It would be nice to update the hd_geometry_lchs_guess() trace event that
>>> you added to use uint32_t + %u instead of int + %d.
>>
>> PRIu32?
>
> uint32_t: Good point, will do.
>
> Conversion specifier: trace-events routinely prints uint32_t with %d and
> %x, never with PRI*32.  I'm happy to change from %d to %u, but reluctant
> to add the first use of PRIu32.

Either is fine by me although we might as well continue to do %u.

Stefan

Re: [Qemu-devel] [PATCH 08/32] hd-geometry: Move disk geometry guessing back from block.c

[Blue Swirl]

On Sat, Jun 30, 2012 at 5:50 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Blue Swirl <blauwirbel@gmail.com> writes:
>
>> On Fri, Jun 29, 2012 at 3:34 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>> Commit f3d54fc4 factored it out of hw/ide.c for reuse.  Sensible,
>>> except it was put into block.c.  Device-specific functionality should
>>> be kept in device code, not the block layer.  Move it to
>>> hw/hd-geometry.c, and make stylistic changes required to keep
>>> checkpatch.pl happy.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  block.c          |  121 ------------------------------------------
>>>  block.h          |    1 -
>>>  blockdev.h       |    5 ++
>>>  hw/Makefile.objs |    2 +-
>>>  hw/hd-geometry.c |  153 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  hw/ide/core.c    |    2 +-
>>>  hw/scsi-disk.c   |    4 +-
>>>  hw/virtio-blk.c  |    2 +-
>>>  8 files changed, 163 insertions(+), 127 deletions(-)
>>>  create mode 100644 hw/hd-geometry.c
> [...]
>>> diff --git a/blockdev.h b/blockdev.h
>>> index 260e16b..7b05945 100644
>>> --- a/blockdev.h
>>> +++ b/blockdev.h
>>> @@ -62,4 +62,9 @@ void qmp_change_blockdev(const char *device, const char *filename,
>>>                          bool has_format, const char *format, Error **errp);
>>>  void do_commit(Monitor *mon, const QDict *qdict);
>>>  int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
>>> +
>>> +/* Hard disk geometry */
>>> +void hd_geometry_guess(BlockDriverState *bs,
>>> +                       int *pcyls, int *pheads, int *psecs);
>>
>> I'd move this to a separate header under hw/.
>
> hw/hd-geometry.h?  Or a header collecting shared block device model
> code, say hw/block-common.h?

I'd be happy with either.

>
>>> +
>>>  #endif
>>> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
>>> index 3d77259..5bf1d76 100644
>>> --- a/hw/Makefile.objs
>>> +++ b/hw/Makefile.objs
>>> @@ -137,7 +137,7 @@ common-obj-$(CONFIG_MAX111X) += max111x.o
>>>  common-obj-$(CONFIG_DS1338) += ds1338.o
>>>  common-obj-y += i2c.o smbus.o smbus_eeprom.o
>>>  common-obj-y += eeprom93xx.o
>>> -common-obj-y += scsi-disk.o cdrom.o
>>> +common-obj-y += scsi-disk.o cdrom.o hd-geometry.o
>>>  common-obj-y += scsi-generic.o scsi-bus.o
>>>  common-obj-y += hid.o
>>>  common-obj-$(CONFIG_SSI) += ssi.o
>>> diff --git a/hw/hd-geometry.c b/hw/hd-geometry.c
>>> new file mode 100644
>>> index 0000000..9b22e3f
>>> --- /dev/null
>>> +++ b/hw/hd-geometry.c
>>> @@ -0,0 +1,153 @@
>>> +/*
>>> + * Hard disk geometry utilities
>>> + *
>>> + * Copyright (c) 2003 Fabrice Bellard
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>>> + * of this software and associated documentation files (the "Software"), to deal
>>> + * in the Software without restriction, including without limitation the rights
>>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>>> + * copies of the Software, and to permit persons to whom the Software is
>>> + * furnished to do so, subject to the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice shall be included in
>>> + * all copies or substantial portions of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>>> + * THE SOFTWARE.
>>> + */
>
> By the way, X11 license.  I'd very much prefer to put my contributions
> under GPLv2+.  Is there an acceptable way to do that?

IANAL, but isn't MIT license compatible with GPL? Then maybe you could
license your contributions under GPLv2+, for example with the same
statement as GPLv2 to GPLv2+:
"Contributions after 2012-mm-dd are licensed under the terms of the
GNU GPL, version 2 or (at your option) any later version."

Though after that, the file would be dual licensed. Those who want to
use the MIT version can use the file before the change.

>>> +#include "blockdev.h"
>>> +
>>> +struct partition {
>>
>> Partition
>
> Okay.
>
> [...]

Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf

[Blue Swirl]

On Mon, Jul 2, 2012 at 2:34 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Mon, Jul 2, 2012 at 3:15 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> Andreas Färber <afaerber@suse.de> writes:
>>
>>> Am 02.07.2012 14:55, schrieb Stefan Hajnoczi:
>>>> On Fri, Jun 29, 2012 at 05:34:40PM +0200, Markus Armbruster wrote:
>>>>> Best to use the same type, to avoid unwanted truncation or sign
>>>>> extension.
>>>>>
>>>>> BlockConf can't use plain int for cyls, heads and secs, because
>>>>> integer properties require an exact width.
>>>>>
>>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>>> ---
>>>>>  blockdev.h       |    2 +-
>>>>>  hw/hd-geometry.c |    4 ++--
>>>>>  hw/ide/core.c    |    2 +-
>>>>>  hw/scsi-disk.c   |    2 +-
>>>>>  hw/virtio-blk.c  |    2 +-
>>>>>  5 files changed, 6 insertions(+), 6 deletions(-)
>>>>
>>>> It would be nice to update the hd_geometry_lchs_guess() trace event that
>>>> you added to use uint32_t + %u instead of int + %d.
>>>
>>> PRIu32?
>>
>> uint32_t: Good point, will do.
>>
>> Conversion specifier: trace-events routinely prints uint32_t with %d and
>> %x, never with PRI*32.  I'm happy to change from %d to %u, but reluctant
>> to add the first use of PRIu32.
>
> Either is fine by me although we might as well continue to do %u.

I'd also vote for %u. PRI*32 do not seem very useful compared to plain
int versions.

>
> Stefan
>

Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf

[Andreas Färber]

Am 03.07.2012 21:11, schrieb Blue Swirl:
> On Mon, Jul 2, 2012 at 2:34 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Mon, Jul 2, 2012 at 3:15 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>> Andreas Färber <afaerber@suse.de> writes:
>>>
>>>> Am 02.07.2012 14:55, schrieb Stefan Hajnoczi:
>>>>> On Fri, Jun 29, 2012 at 05:34:40PM +0200, Markus Armbruster wrote:
>>>>>> Best to use the same type, to avoid unwanted truncation or sign
>>>>>> extension.
>>>>>>
>>>>>> BlockConf can't use plain int for cyls, heads and secs, because
>>>>>> integer properties require an exact width.
>>>>>>
>>>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>>>> ---
>>>>>>  blockdev.h       |    2 +-
>>>>>>  hw/hd-geometry.c |    4 ++--
>>>>>>  hw/ide/core.c    |    2 +-
>>>>>>  hw/scsi-disk.c   |    2 +-
>>>>>>  hw/virtio-blk.c  |    2 +-
>>>>>>  5 files changed, 6 insertions(+), 6 deletions(-)
>>>>>
>>>>> It would be nice to update the hd_geometry_lchs_guess() trace event that
>>>>> you added to use uint32_t + %u instead of int + %d.
>>>>
>>>> PRIu32?
>>>
>>> uint32_t: Good point, will do.
>>>
>>> Conversion specifier: trace-events routinely prints uint32_t with %d and
>>> %x, never with PRI*32.  I'm happy to change from %d to %u, but reluctant
>>> to add the first use of PRIu32.
>>
>> Either is fine by me although we might as well continue to do %u.
> 
> I'd also vote for %u. PRI*32 do not seem very useful compared to plain
> int versions.

If it's not useful we should use unsigned int.

Mixing both is simply wrong. I'm okay with fixing it consistently in a
follow-up though. The Broken Window theory scores again (lack of const,
non-CamelCase type names, placement of {, wrong semicolons, etc.).

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 08/32] hd-geometry: Move disk geometry guessing back from block.c

[Kevin Wolf]

Am 03.07.2012 20:40, schrieb Blue Swirl:
> On Sat, Jun 30, 2012 at 5:50 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> Blue Swirl <blauwirbel@gmail.com> writes:
>>
>>> On Fri, Jun 29, 2012 at 3:34 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>>> Commit f3d54fc4 factored it out of hw/ide.c for reuse.  Sensible,
>>>> except it was put into block.c.  Device-specific functionality should
>>>> be kept in device code, not the block layer.  Move it to
>>>> hw/hd-geometry.c, and make stylistic changes required to keep
>>>> checkpatch.pl happy.
>>>>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> ---
>>>>  block.c          |  121 ------------------------------------------
>>>>  block.h          |    1 -
>>>>  blockdev.h       |    5 ++
>>>>  hw/Makefile.objs |    2 +-
>>>>  hw/hd-geometry.c |  153 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  hw/ide/core.c    |    2 +-
>>>>  hw/scsi-disk.c   |    4 +-
>>>>  hw/virtio-blk.c  |    2 +-
>>>>  8 files changed, 163 insertions(+), 127 deletions(-)
>>>>  create mode 100644 hw/hd-geometry.c
>> [...]
>>>> diff --git a/blockdev.h b/blockdev.h
>>>> index 260e16b..7b05945 100644
>>>> --- a/blockdev.h
>>>> +++ b/blockdev.h
>>>> @@ -62,4 +62,9 @@ void qmp_change_blockdev(const char *device, const char *filename,
>>>>                          bool has_format, const char *format, Error **errp);
>>>>  void do_commit(Monitor *mon, const QDict *qdict);
>>>>  int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
>>>> +
>>>> +/* Hard disk geometry */
>>>> +void hd_geometry_guess(BlockDriverState *bs,
>>>> +                       int *pcyls, int *pheads, int *psecs);
>>>
>>> I'd move this to a separate header under hw/.
>>
>> hw/hd-geometry.h?  Or a header collecting shared block device model
>> code, say hw/block-common.h?
> 
> I'd be happy with either.

I think block-common.h is better. We don't have that much block layer
specific guest-side code that it would make sense to split it up.

>>>> diff --git a/hw/hd-geometry.c b/hw/hd-geometry.c
>>>> new file mode 100644
>>>> index 0000000..9b22e3f
>>>> --- /dev/null
>>>> +++ b/hw/hd-geometry.c
>>>> @@ -0,0 +1,153 @@
>>>> +/*
>>>> + * Hard disk geometry utilities
>>>> + *
>>>> + * Copyright (c) 2003 Fabrice Bellard
>>>> + *
>>>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>>>> + * of this software and associated documentation files (the "Software"), to deal
>>>> + * in the Software without restriction, including without limitation the rights
>>>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>>>> + * copies of the Software, and to permit persons to whom the Software is
>>>> + * furnished to do so, subject to the following conditions:
>>>> + *
>>>> + * The above copyright notice and this permission notice shall be included in
>>>> + * all copies or substantial portions of the Software.
>>>> + *
>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>>>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>>>> + * THE SOFTWARE.
>>>> + */
>>
>> By the way, X11 license.  I'd very much prefer to put my contributions
>> under GPLv2+.  Is there an acceptable way to do that?
> 
> IANAL, but isn't MIT license compatible with GPL?

IANAL either, but as I understand it:

It is, otherwise distributing qemu would be illegal. But compatibility
means more or less that you can mix them within one project, it doesn't
mean you can convert from one to the other.

> Then maybe you could
> license your contributions under GPLv2+, for example with the same
> statement as GPLv2 to GPLv2+:
> "Contributions after 2012-mm-dd are licensed under the terms of the
> GNU GPL, version 2 or (at your option) any later version."
> 
> Though after that, the file would be dual licensed. Those who want to
> use the MIT version can use the file before the change.

This might be possible. (Though true dual-licensing would mean that the
whole code, including new parts, can be used under either license. This
isn't what we would be doing.) Or have a GPL header and after that add a
notice like "Portions of the code are licensed under the following
conditions: <old license header>".

In any case it's pretty clear to me that you're not allowed to drop the
MIT license header (and therefore blockdev.c still needs a fix)

Kevin

Re: [Qemu-devel] [PATCH 03/32] vvfat: Fix partition table

[Kevin Wolf]

Am 29.06.2012 22:33, schrieb Blue Swirl:
> On Fri, Jun 29, 2012 at 3:34 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> Unless parameter ":floppy:" is given, vvfat creates a virtual image
>> with DOS MBR defining a single partition which holds the FAT file
>> system.  The size of the virtual image depends on the width of the
>> FAT: 32 MiB (CHS 64, 16, 63) for 12 bit FAT, 504 MiB (CHS 1024, 16,
>> 63) for 16 and 32 bit FAT, leaving (64*16-1)*63 = 64449 and
>> (1024*16-1)*64 = 1032129 sectors for the partition.
>>
>> However, it screws up the end of the partition in the MBR:
>>
>>    FAT width param.  start CHS  end CHS     start LBA  size
>>        :32:          0,1,1      1023,14,63       63    1032065
>>        :16:          0,1,1      1023,14,55       63    1032057
>>        :12:          0,1,1        63,14,55       63      64377
>>
>> The actual FAT file system nevertheless assumes the partition has
>> 1032129 or 64449 sectors.  Oops.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/vvfat.c |    7 ++++---
>>  1 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/vvfat.c b/block/vvfat.c
>> index 0fd3367..62745b5 100644
>> --- a/block/vvfat.c
>> +++ b/block/vvfat.c
>> @@ -394,11 +394,12 @@ static void init_mbr(BDRVVVFATState* s)
>>
>>     /* LBA is used when partition is outside the CHS geometry */
>>     lba = sector2CHS(s->bs, &partition->start_CHS, s->first_sectors_number-1);
>> -    lba|= sector2CHS(s->bs, &partition->end_CHS,   s->sector_count);
>> +    lba |= sector2CHS(s->bs, &partition->end_CHS, s->bs->total_sectors - 1);
>>
>>     /*LBA partitions are identified only by start/length_sector_long not by CHS*/
>> -    partition->start_sector_long =cpu_to_le32(s->first_sectors_number-1);
>> -    partition->length_sector_long=cpu_to_le32(s->sector_count - s->first_sectors_number+1);
>> +    partition->start_sector_long  = cpu_to_le32(s->first_sectors_number-1);
> 
> Spaces around '-'. Thanks for fixing the other cases, BTW.

For compensation there's an extra space before the '='.

>> +    partition->length_sector_long = cpu_to_le32(s->bs->total_sectors
>> +                                                - s->first_sectors_number + 1);

Just wondering... This should be the same as s->sector_count, right?
This whole geometry calculation code in vvfat is way too convoluted to
understand when you haven't looked at it for some months...

Kevin

Re: [Qemu-devel] [PATCH 04/32] vvfat: Do not clobber the user's geometry

[Kevin Wolf]

Am 29.06.2012 17:34, schrieb Markus Armbruster:
> vvfat creates a virtual VFAT filesystem with a certain logical
> geometry that depends on its options.  It sets the "geometry hint" to
> this geometry.  It is the only block driver to do this.
> 
> The geometry hint is about about *physical* geometry, and used only by
> certain hard disk device models.
> 
> vvfat's hint is normally invisible for device models, because
> bdrv_open() puts a raw format on top of vvfat's fat protocol.  That
> raw format is where drive_init() puts the user's geometry (if any),
> and where the device model gets it from.
> 
> Nobody complained, because the default physical geometry is the same
> as vvfat's logical geometry:
> 
>     opts        LCHS        def. PCHS
>                 1024,16,63  same
>     :32:        1024,16,63  same
>     :16:        1024,16,63  same
>     :12:          64,16,63  same
> 
> Except when you specify :floppy:
> 
>     opts        LCHS        def. PCHS
>        :floppy:   80, 2,36  5,16,63
>     :32:floppy:   80, 2,36  5,16,63
>     :16:floppy:   80, 2,36  5,16,63
>     :12:floppy:   80, 2,18  2,16,63
> 
> Silly thing to do for use with a hard disk.
> 
> However, the "raw" format can be suppressed by adding an
> redundant-looking "format=vvfat" to "file=fat:FOO".  Then, vvfat's
> hint clobbers the user's geometry, i.e. -drive options cyls, heads,
> secs get silently ignored.  Don't do that.
> 
> No change without format=vvfat.  With it, the user's hard disk
> geometry (-drive options cyls, heads, secs) is now obeyed, and the
> default hard disk geometry with :floppy: now matches the one without
> format=vvfat.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

For some values of "obeyed". If I understand correctly, the user defined
geometry will indeed by visible for the device emulation now, but this
still doesn't mean that vvfat also provides an image that matches this
geometry. Not sure if it is a good idea to allow such mismatches.

> @@ -1067,19 +1074,16 @@ DLOG(if (stderr == NULL) {
>      else
>  	dirname += i+1;
>  
> -    bs->total_sectors=bs->cyls*bs->heads*bs->secs;
> +    bs->total_sectors = cyls * heads * secs;
>  
> -    if(init_directories(s, dirname))
> +    if (init_directories(s, dirname, heads, secs)) {
>  	return -1;
> +    }
>  
>      s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count;
>  
>      if(s->first_sectors_number==0x40)
> -	init_mbr(s);
> -    else {
> -        /* MS-DOS does not like to know about CHS (?). */
> -	bs->heads = bs->cyls = bs->secs = 0;
> -    }
> +        init_mbr(s, cyls, heads, secs);

You can add braces here while touching the code.

Kevin

Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf

[Paolo Bonzini]

Il 03/07/2012 22:15, Andreas Färber ha scritto:
>> > 
>> > I'd also vote for %u. PRI*32 do not seem very useful compared to plain
>> > int versions.
> If it's not useful we should use unsigned int.

It just means that we're assuming 32-bit ints, which is a fact.  The day
an IL32 platform appears that has "typedef long int32_t", we'll worry
about PRIu32.

Paolo

> Mixing both is simply wrong. I'm okay with fixing it consistently in a
> follow-up though. The Broken Window theory scores again (lack of const,
> non-CamelCase type names, placement of {, wrong semicolons, etc.).

Re: [Qemu-devel] [PATCH 04/32] vvfat: Do not clobber the user's geometry

[Paolo Bonzini]

Il 04/07/2012 17:23, Kevin Wolf ha scritto:
>> >     opts        LCHS        def. PCHS
>> >        :floppy:   80, 2,36  5,16,63
>> >     :32:floppy:   80, 2,36  5,16,63
>> >     :16:floppy:   80, 2,36  5,16,63
>> >     :12:floppy:   80, 2,18  2,16,63
>> > 
>> > Silly thing to do for use with a hard disk.
>> > 
>> > However, the "raw" format can be suppressed by adding an
>> > redundant-looking "format=vvfat" to "file=fat:FOO".  Then, vvfat's
>> > hint clobbers the user's geometry, i.e. -drive options cyls, heads,
>> > secs get silently ignored.  Don't do that.
>> > 
>> > No change without format=vvfat.  With it, the user's hard disk
>> > geometry (-drive options cyls, heads, secs) is now obeyed, and the
>> > default hard disk geometry with :floppy: now matches the one without
>> > format=vvfat.
>> > 
>> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> For some values of "obeyed". If I understand correctly, the user defined
> geometry will indeed by visible for the device emulation now, but this
> still doesn't mean that vvfat also provides an image that matches this
> geometry. Not sure if it is a good idea to allow such mismatches.
> 

Does this only matter if you declare an image with :floppy: and pass it
as a hard disk?  Then we can honestly say we don't care...

Paolo

Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 847 bytes --]

On 07/04/2012 10:19 AM, Paolo Bonzini wrote:
> Il 03/07/2012 22:15, Andreas Färber ha scritto:
>>>>
>>>> I'd also vote for %u. PRI*32 do not seem very useful compared to plain
>>>> int versions.
>> If it's not useful we should use unsigned int.
> 
> It just means that we're assuming 32-bit ints, which is a fact.  The day
> an IL32 platform appears that has "typedef long int32_t", we'll worry
> about PRIu32.

Newlib's <stdint.h> does just that, on platforms that __have_long32.
Which makes Cygwin a great platform for flushing out these sorts of type
discrepancies (since cygwin inherits from newlib).

http://sourceware.org/cgi-bin/cvsweb.cgi/src/newlib/libc/include/stdint.h.diff?r1=1.8&r2=1.9&cvsroot=src&f=h

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH 04/32] vvfat: Do not clobber the user's geometry

[Kevin Wolf]

Am 04.07.2012 18:25, schrieb Paolo Bonzini:
> Il 04/07/2012 17:23, Kevin Wolf ha scritto:
>>>>     opts        LCHS        def. PCHS
>>>>        :floppy:   80, 2,36  5,16,63
>>>>     :32:floppy:   80, 2,36  5,16,63
>>>>     :16:floppy:   80, 2,36  5,16,63
>>>>     :12:floppy:   80, 2,18  2,16,63
>>>>
>>>> Silly thing to do for use with a hard disk.
>>>>
>>>> However, the "raw" format can be suppressed by adding an
>>>> redundant-looking "format=vvfat" to "file=fat:FOO".  Then, vvfat's
>>>> hint clobbers the user's geometry, i.e. -drive options cyls, heads,
>>>> secs get silently ignored.  Don't do that.
>>>>
>>>> No change without format=vvfat.  With it, the user's hard disk
>>>> geometry (-drive options cyls, heads, secs) is now obeyed, and the
>>>> default hard disk geometry with :floppy: now matches the one without
>>>> format=vvfat.
>>>>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> For some values of "obeyed". If I understand correctly, the user defined
>> geometry will indeed by visible for the device emulation now, but this
>> still doesn't mean that vvfat also provides an image that matches this
>> geometry. Not sure if it is a good idea to allow such mismatches.
>>
> 
> Does this only matter if you declare an image with :floppy: and pass it
> as a hard disk?  Then we can honestly say we don't care...

As I understand it, the patch has two effect:

1. the user's hard disk geometry (-drive options cyls, heads, secs) is
now obeyed
2. the default hard disk geometry with :floppy: now matches the one
without format=vvfat

I don't care about 2. indeed, but doing 1. right would be nice.

Kevin

Re: [Qemu-devel] [PATCH 05/32] qtest: Tidy up temporary files properly

[Kevin Wolf]

Am 29.06.2012 17:34, schrieb Markus Armbruster:
> Each test litters /tmp with several files: a pid file and two
> sockets.  Tidy up.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Thanks! Still leaves the files around if a test case fails, but much
better than before.

Kevin

Re: [Qemu-devel] [PATCH 04/32] vvfat: Do not clobber the user's geometry

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 04.07.2012 18:25, schrieb Paolo Bonzini:
>> Il 04/07/2012 17:23, Kevin Wolf ha scritto:
>>>>>     opts        LCHS        def. PCHS
>>>>>        :floppy:   80, 2,36  5,16,63
>>>>>     :32:floppy:   80, 2,36  5,16,63
>>>>>     :16:floppy:   80, 2,36  5,16,63
>>>>>     :12:floppy:   80, 2,18  2,16,63
>>>>>
>>>>> Silly thing to do for use with a hard disk.
>>>>>
>>>>> However, the "raw" format can be suppressed by adding an
>>>>> redundant-looking "format=vvfat" to "file=fat:FOO".  Then, vvfat's
>>>>> hint clobbers the user's geometry, i.e. -drive options cyls, heads,
>>>>> secs get silently ignored.  Don't do that.
>>>>>
>>>>> No change without format=vvfat.  With it, the user's hard disk
>>>>> geometry (-drive options cyls, heads, secs) is now obeyed, and the
>>>>> default hard disk geometry with :floppy: now matches the one without
>>>>> format=vvfat.
>>>>>
>>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> For some values of "obeyed". If I understand correctly, the user defined
>>> geometry will indeed by visible for the device emulation now, but this
>>> still doesn't mean that vvfat also provides an image that matches this
>>> geometry. Not sure if it is a good idea to allow such mismatches.

This patch doesn't let the user do anything he couldn't do before.  It
only makes the special case "vvfat without raw on top" consistent with
all other block backend configurations.  With "raw over vvfat" in
particular.

So, if anything needs additional user parameter validation, it's the
common case.  Note that any other image type could contain the very same
FAT filesystem as a vvfat one, with the very same impact on a guest
vulnerable to mismatches between geometry and block device contents.

To avoid confusion, let me recap geometry.

Physical geometry is a property of the disk.  Or rather, a property of
the disk's legacy CHS command interface; it ceased to have any useful
relation to platters and heads long ago.  Non-legacy commands use LBA to
address blocks.

Logical geometry is a property of the PC BIOS legacy CHS interface (int
13h).  It was invented when disks outgrew the BIOS's CHS limits.  It
need not match physical geometry (*not* matching is its purpose).  The
BIOS translates logical geometry to something the disk understands.
Long ago, this meant translating to physical CHS.  Nowadays, it means
translating to LBA.  As far as I can tell, SeaBIOS always translates to
LBA.  SeaBIOS picks a logical geometry based on what it finds in CMOS.

Logical geometry matters only for software that uses legacy int 13h.
DOS, basically.

Physical geometry matters only for software that talks to the disk
directly, using legacy CHS commands.  Sufficently old versions of Unix,
perhaps?

The choice of geometry isn't really important, as long as:

1. The mapping to LBA is sane: lba = ((c * #h) + h * #s) + (s-1)

   where c, h, s is the address, and #c, #h, #s is the geometry.
   Increasing first s, then h, then c runs through the lbas in order,
   regardless of what you picked for geometry.

   For physical geometry, the device model maps.  As far as I can tell,
   they all map sanely.

   For logical geometry, SeaBIOS maps.  As far as I can tell, it maps
   sanely.

   Actually, even an insane mapping would work for block drivers that
   only schlep blocks, as long as it doesn't change.  Same as with
   physical disks.

2. The OS is happy with the partition table.

   Some OSs want each partition to start and end on a cylinder boundary.
   For them, whatever geometry they use should be consistent with the
   partition table.

   Note that before PATCH 03/32, vvfat's partition end was screwed up,
   and nobody noticed.

>> Does this only matter if you declare an image with :floppy: and pass it
>> as a hard disk?  Then we can honestly say we don't care...
>
> As I understand it, the patch has two effect:
>
> 1. the user's hard disk geometry (-drive options cyls, heads, secs) is
> now obeyed

... even when the user suppresses raw by specifying format=vvfat with
file=fat:...  Consistent with how all other block drivers work.

> 2. the default hard disk geometry with :floppy: now matches the one
> without format=vvfat

Yes.

> I don't care about 2. indeed, but doing 1. right would be nice.

Re: [Qemu-devel] [PATCH 13/32] hd-geometry: Clean up confusing use of prior translation hint

[Kevin Wolf]

Am 29.06.2012 17:34, schrieb Markus Armbruster:
> When hd_geometry_guess() picks a geometry, it also picks the
> appropriate translation, but only when the prior translation hint is
> BIOS_ATA_TRANSLATION_AUTO.  Looks wrong, because such a prior
> translation would be passed to the BIOS whether it's suitable for the
> geometry or not.
> 
> Fortunately, that can't happen.  There are just two ways to for the

s/to//

> translation hint to get set to something other than
> BIOS_ATA_TRANSLATION_AUTO: drive_init() on behalf of -drive trans=...,
> and hd_geometry_guess().  Both set it only when they also set a valid
> geometry hint, i.e. one with a non-zero number of cylinders.
> 
> Since hd_geometry_guess() returns right away when it finds a valid
> geometry hint, translation can only be BIOS_ATA_TRANSLATION_AUTO in
> the remainder of the function.
> 
> Assert this, and simplify accordingly.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/hd-geometry.c |   17 +++++++----------
>  1 files changed, 7 insertions(+), 10 deletions(-)

Kevin

Re: [Qemu-devel] [PATCH 03/32] vvfat: Fix partition table

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 29.06.2012 22:33, schrieb Blue Swirl:
>> On Fri, Jun 29, 2012 at 3:34 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>> Unless parameter ":floppy:" is given, vvfat creates a virtual image
>>> with DOS MBR defining a single partition which holds the FAT file
>>> system.  The size of the virtual image depends on the width of the
>>> FAT: 32 MiB (CHS 64, 16, 63) for 12 bit FAT, 504 MiB (CHS 1024, 16,
>>> 63) for 16 and 32 bit FAT, leaving (64*16-1)*63 = 64449 and
>>> (1024*16-1)*64 = 1032129 sectors for the partition.
>>>
>>> However, it screws up the end of the partition in the MBR:
>>>
>>>    FAT width param.  start CHS  end CHS     start LBA  size
>>>        :32:          0,1,1      1023,14,63       63    1032065
>>>        :16:          0,1,1      1023,14,55       63    1032057
>>>        :12:          0,1,1        63,14,55       63      64377
>>>
>>> The actual FAT file system nevertheless assumes the partition has
>>> 1032129 or 64449 sectors.  Oops.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  block/vvfat.c |    7 ++++---
>>>  1 files changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/block/vvfat.c b/block/vvfat.c
>>> index 0fd3367..62745b5 100644
>>> --- a/block/vvfat.c
>>> +++ b/block/vvfat.c
>>> @@ -394,11 +394,12 @@ static void init_mbr(BDRVVVFATState* s)
>>>
>>>     /* LBA is used when partition is outside the CHS geometry */
>>>     lba = sector2CHS(s->bs, &partition->start_CHS, s->first_sectors_number-1);
>>> -    lba|= sector2CHS(s->bs, &partition->end_CHS,   s->sector_count);
>>> +    lba |= sector2CHS(s->bs, &partition->end_CHS, s->bs->total_sectors - 1);
>>>
>>>     /*LBA partitions are identified only by start/length_sector_long not by CHS*/
>>> -    partition->start_sector_long =cpu_to_le32(s->first_sectors_number-1);
>>> -    partition->length_sector_long=cpu_to_le32(s->sector_count - s->first_sectors_number+1);
>>> +    partition->start_sector_long  = cpu_to_le32(s->first_sectors_number-1);
>> 
>> Spaces around '-'. Thanks for fixing the other cases, BTW.
>
> For compensation there's an extra space before the '='.

The original lined up the two '='.  I preserved that.  Not that I care
for it.  Want me to drop the extra space?

>>> +    partition->length_sector_long = cpu_to_le32(s->bs->total_sectors
>>> +                                                - s->first_sectors_number + 1);
>
> Just wondering... This should be the same as s->sector_count, right?

Hmm.  vvfat_open() assigns:

    s->sector_count = cyls * heads * secs - (s->first_sectors_number - 1);
    bs->total_sectors = cyls * heads * secs;

But it then changes it minds and does:

    s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count;

> This whole geometry calculation code in vvfat is way too convoluted to
> understand when you haven't looked at it for some months...

Yes, it is.  I was briefly tempted to replace it, but then decided to
limit my footprint in this buck-crazy block driver to minimal bug fixes.

Re: [Qemu-devel] [PATCH 05/32] qtest: Tidy up temporary files properly

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 29.06.2012 17:34, schrieb Markus Armbruster:
>> Each test litters /tmp with several files: a pid file and two
>> sockets.  Tidy up.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Thanks! Still leaves the files around if a test case fails, but much
> better than before.

I didn't have the stomach to fix that part in the middle of this series
:)

Re: [Qemu-devel] [PATCH 13/32] hd-geometry: Clean up confusing use of prior translation hint

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 29.06.2012 17:34, schrieb Markus Armbruster:
>> When hd_geometry_guess() picks a geometry, it also picks the
>> appropriate translation, but only when the prior translation hint is
>> BIOS_ATA_TRANSLATION_AUTO.  Looks wrong, because such a prior
>> translation would be passed to the BIOS whether it's suitable for the
>> geometry or not.
>> 
>> Fortunately, that can't happen.  There are just two ways to for the
>
> s/to//

Got it, thanks!

[...]

Re: [Qemu-devel] [PATCH 03/32] vvfat: Fix partition table

[Kevin Wolf]

Am 05.07.2012 11:23, schrieb Markus Armbruster:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> Am 29.06.2012 22:33, schrieb Blue Swirl:
>>> On Fri, Jun 29, 2012 at 3:34 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>>> Unless parameter ":floppy:" is given, vvfat creates a virtual image
>>>> with DOS MBR defining a single partition which holds the FAT file
>>>> system.  The size of the virtual image depends on the width of the
>>>> FAT: 32 MiB (CHS 64, 16, 63) for 12 bit FAT, 504 MiB (CHS 1024, 16,
>>>> 63) for 16 and 32 bit FAT, leaving (64*16-1)*63 = 64449 and
>>>> (1024*16-1)*64 = 1032129 sectors for the partition.
>>>>
>>>> However, it screws up the end of the partition in the MBR:
>>>>
>>>>    FAT width param.  start CHS  end CHS     start LBA  size
>>>>        :32:          0,1,1      1023,14,63       63    1032065
>>>>        :16:          0,1,1      1023,14,55       63    1032057
>>>>        :12:          0,1,1        63,14,55       63      64377
>>>>
>>>> The actual FAT file system nevertheless assumes the partition has
>>>> 1032129 or 64449 sectors.  Oops.
>>>>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> ---
>>>>  block/vvfat.c |    7 ++++---
>>>>  1 files changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/block/vvfat.c b/block/vvfat.c
>>>> index 0fd3367..62745b5 100644
>>>> --- a/block/vvfat.c
>>>> +++ b/block/vvfat.c
>>>> @@ -394,11 +394,12 @@ static void init_mbr(BDRVVVFATState* s)
>>>>
>>>>     /* LBA is used when partition is outside the CHS geometry */
>>>>     lba = sector2CHS(s->bs, &partition->start_CHS, s->first_sectors_number-1);
>>>> -    lba|= sector2CHS(s->bs, &partition->end_CHS,   s->sector_count);
>>>> +    lba |= sector2CHS(s->bs, &partition->end_CHS, s->bs->total_sectors - 1);
>>>>
>>>>     /*LBA partitions are identified only by start/length_sector_long not by CHS*/
>>>> -    partition->start_sector_long =cpu_to_le32(s->first_sectors_number-1);
>>>> -    partition->length_sector_long=cpu_to_le32(s->sector_count - s->first_sectors_number+1);
>>>> +    partition->start_sector_long  = cpu_to_le32(s->first_sectors_number-1);
>>>
>>> Spaces around '-'. Thanks for fixing the other cases, BTW.
>>
>> For compensation there's an extra space before the '='.
> 
> The original lined up the two '='.  I preserved that.  Not that I care
> for it.  Want me to drop the extra space?

Ah, didn't notice that. I don't mind then.

>>>> +    partition->length_sector_long = cpu_to_le32(s->bs->total_sectors
>>>> +                                                - s->first_sectors_number + 1);
>>
>> Just wondering... This should be the same as s->sector_count, right?
> 
> Hmm.  vvfat_open() assigns:
> 
>     s->sector_count = cyls * heads * secs - (s->first_sectors_number - 1);
>     bs->total_sectors = cyls * heads * secs;
> 
> But it then changes it minds and does:
> 
>     s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count;

Which probably means that they differ if some sub-cluster sized space is
left unused at the end of the disk. It's not useful to have this space
included in the partition, but it doesn't hurt either. So I suppose
either way is fine.

>> This whole geometry calculation code in vvfat is way too convoluted to
>> understand when you haven't looked at it for some months...
> 
> Yes, it is.  I was briefly tempted to replace it, but then decided to
> limit my footprint in this buck-crazy block driver to minimal bug fixes.

Heh, quite understandable.

Kevin

Re: [Qemu-devel] [PATCH 03/32] vvfat: Fix partition table

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 05.07.2012 11:23, schrieb Markus Armbruster:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>>> Am 29.06.2012 22:33, schrieb Blue Swirl:
>>>> On Fri, Jun 29, 2012 at 3:34 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>>>> Unless parameter ":floppy:" is given, vvfat creates a virtual image
>>>>> with DOS MBR defining a single partition which holds the FAT file
>>>>> system.  The size of the virtual image depends on the width of the
>>>>> FAT: 32 MiB (CHS 64, 16, 63) for 12 bit FAT, 504 MiB (CHS 1024, 16,
>>>>> 63) for 16 and 32 bit FAT, leaving (64*16-1)*63 = 64449 and
>>>>> (1024*16-1)*64 = 1032129 sectors for the partition.
>>>>>
>>>>> However, it screws up the end of the partition in the MBR:
>>>>>
>>>>>    FAT width param.  start CHS  end CHS     start LBA  size
>>>>>        :32:          0,1,1      1023,14,63       63    1032065
>>>>>        :16:          0,1,1      1023,14,55       63    1032057
>>>>>        :12:          0,1,1        63,14,55       63      64377
>>>>>
>>>>> The actual FAT file system nevertheless assumes the partition has
>>>>> 1032129 or 64449 sectors.  Oops.
>>>>>
>>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>>> ---
>>>>>  block/vvfat.c |    7 ++++---
>>>>>  1 files changed, 4 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/block/vvfat.c b/block/vvfat.c
>>>>> index 0fd3367..62745b5 100644
>>>>> --- a/block/vvfat.c
>>>>> +++ b/block/vvfat.c
>>>>> @@ -394,11 +394,12 @@ static void init_mbr(BDRVVVFATState* s)
>>>>>
>>>>>     /* LBA is used when partition is outside the CHS geometry */
>>>>>     lba = sector2CHS(s->bs, &partition->start_CHS, s->first_sectors_number-1);
>>>>> -    lba|= sector2CHS(s->bs, &partition->end_CHS,   s->sector_count);
>>>>> +    lba |= sector2CHS(s->bs, &partition->end_CHS, s->bs->total_sectors - 1);
>>>>>
>>>>>     /*LBA partitions are identified only by start/length_sector_long not by CHS*/
>>>>> -    partition->start_sector_long =cpu_to_le32(s->first_sectors_number-1);
>>>>> -    partition->length_sector_long=cpu_to_le32(s->sector_count - s->first_sectors_number+1);
>>>>> +    partition->start_sector_long  = cpu_to_le32(s->first_sectors_number-1);
>>>>
>>>> Spaces around '-'. Thanks for fixing the other cases, BTW.
>>>
>>> For compensation there's an extra space before the '='.
>> 
>> The original lined up the two '='.  I preserved that.  Not that I care
>> for it.  Want me to drop the extra space?
>
> Ah, didn't notice that. I don't mind then.
>
>>>>> +    partition->length_sector_long = cpu_to_le32(s->bs->total_sectors
>>>>> +                                                - s->first_sectors_number + 1);
>>>
>>> Just wondering... This should be the same as s->sector_count, right?
>> 
>> Hmm.  vvfat_open() assigns:
>> 
>>     s->sector_count = cyls * heads * secs - (s->first_sectors_number - 1);
>>     bs->total_sectors = cyls * heads * secs;
>> 
>> But it then changes it minds and does:
>> 
>>     s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count;
>
> Which probably means that they differ if some sub-cluster sized space is
> left unused at the end of the disk. It's not useful to have this space
> included in the partition, but it doesn't hurt either. So I suppose
> either way is fine.

Complication: the partition should end on a cylinder boundary.  Shaving
off an unused tail may well interfere with that.  Let's stick to v1
here.

[...]

Re: [Qemu-devel] [PATCH 04/32] vvfat: Do not clobber the user's geometry

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 29.06.2012 17:34, schrieb Markus Armbruster:
[...]
>> @@ -1067,19 +1074,16 @@ DLOG(if (stderr == NULL) {
>>      else
>>  	dirname += i+1;
>>  
>> -    bs->total_sectors=bs->cyls*bs->heads*bs->secs;
>> +    bs->total_sectors = cyls * heads * secs;
>>  
>> -    if(init_directories(s, dirname))
>> +    if (init_directories(s, dirname, heads, secs)) {
>>  	return -1;
>> +    }
>>  
>>      s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count;
>>  
>>      if(s->first_sectors_number==0x40)
>> -	init_mbr(s);
>> -    else {
>> -        /* MS-DOS does not like to know about CHS (?). */
>> -	bs->heads = bs->cyls = bs->secs = 0;
>> -    }
>> +        init_mbr(s, cyls, heads, secs);
>
> You can add braces here while touching the code.

I'm not touching the if...  I'll do it if you insist.

Re: [Qemu-devel] [PATCH 03/32] vvfat: Fix partition table

[Kevin Wolf]

Am 05.07.2012 13:10, schrieb Markus Armbruster:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> Am 05.07.2012 11:23, schrieb Markus Armbruster:
>>> Kevin Wolf <kwolf@redhat.com> writes:
>>>
>>>> Am 29.06.2012 22:33, schrieb Blue Swirl:
>>>>> On Fri, Jun 29, 2012 at 3:34 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>>>>> +    partition->length_sector_long = cpu_to_le32(s->bs->total_sectors
>>>>>> +                                                - s->first_sectors_number + 1);
>>>>
>>>> Just wondering... This should be the same as s->sector_count, right?
>>>
>>> Hmm.  vvfat_open() assigns:
>>>
>>>     s->sector_count = cyls * heads * secs - (s->first_sectors_number - 1);
>>>     bs->total_sectors = cyls * heads * secs;
>>>
>>> But it then changes it minds and does:
>>>
>>>     s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count;
>>
>> Which probably means that they differ if some sub-cluster sized space is
>> left unused at the end of the disk. It's not useful to have this space
>> included in the partition, but it doesn't hurt either. So I suppose
>> either way is fine.
> 
> Complication: the partition should end on a cylinder boundary.  Shaving
> off an unused tail may well interfere with that.  Let's stick to v1
> here.

Good point. If anything, maybe add a comment.

Kevin

Re: [Qemu-devel] [PATCH 22/32] qtest: Cover qdev properties for disk geometry

[Kevin Wolf]

Am 29.06.2012 17:34, schrieb Markus Armbruster:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/hd-geo-test.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/hd-geo-test.c b/tests/hd-geo-test.c
> index 0ab573c..02eb5c2 100644
> --- a/tests/hd-geo-test.c
> +++ b/tests/hd-geo-test.c
> @@ -321,13 +321,15 @@ static void test_ide_drive_user(const char *dev, bool trans)
>      const chst expected_chst = { secs / (4 * 32) , 4, 32, trans };
>  
>      argc = setup_common(argv, ARRAY_SIZE(argv));
> -    opts = g_strdup_printf(",cyls=%d,heads=%d,secs=%d%s",
> +    opts = g_strdup_printf("%s,cyls=%d,heads=%d,secs=%d%s",
> +                           dev && !trans ? dev : "",
>                             expected_chst.cyls, expected_chst.heads,
>                             expected_chst.secs,
>                             trans ? ",trans=lba" : "");
>      cur_ide[0] = &expected_chst;
>      argc = setup_ide(argc, argv, ARRAY_SIZE(argv),
> -                     0, dev, backend_small, mbr_chs, opts);
> +                     0, dev && !trans ? opts : NULL, backend_small, mbr_chs,
> +                     dev && !trans ? "" : opts);
>      qtest_start(g_strjoinv(" ", argv));
>      test_cmos();
>      qtest_quit(global_qtest);

I've spent more time parsing this test code than I needed for the review
for most patches that touch actual code... Maybe an explicit if (dev &&
!trans) would help somewhat. don't know.

Kevin

Re: [Qemu-devel] [PATCH 22/32] qtest: Cover qdev properties for disk geometry

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 29.06.2012 17:34, schrieb Markus Armbruster:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/hd-geo-test.c |    6 ++++--
>>  1 files changed, 4 insertions(+), 2 deletions(-)
>> 
>> diff --git a/tests/hd-geo-test.c b/tests/hd-geo-test.c
>> index 0ab573c..02eb5c2 100644
>> --- a/tests/hd-geo-test.c
>> +++ b/tests/hd-geo-test.c
>> @@ -321,13 +321,15 @@ static void test_ide_drive_user(const char *dev, bool trans)
>>      const chst expected_chst = { secs / (4 * 32) , 4, 32, trans };
>>  
>>      argc = setup_common(argv, ARRAY_SIZE(argv));
>> -    opts = g_strdup_printf(",cyls=%d,heads=%d,secs=%d%s",
>> +    opts = g_strdup_printf("%s,cyls=%d,heads=%d,secs=%d%s",
>> +                           dev && !trans ? dev : "",
>>                             expected_chst.cyls, expected_chst.heads,
>>                             expected_chst.secs,
>>                             trans ? ",trans=lba" : "");
>>      cur_ide[0] = &expected_chst;
>>      argc = setup_ide(argc, argv, ARRAY_SIZE(argv),
>> -                     0, dev, backend_small, mbr_chs, opts);
>> +                     0, dev && !trans ? opts : NULL, backend_small, mbr_chs,
>> +                     dev && !trans ? "" : opts);
>>      qtest_start(g_strjoinv(" ", argv));
>>      test_cmos();
>>      qtest_quit(global_qtest);
>
> I've spent more time parsing this test code than I needed for the review
> for most patches that touch actual code... Maybe an explicit if (dev &&
> !trans) would help somewhat. don't know.

Sorry about that.  It gets better again in PATCH 26/32.

Re: [Qemu-devel] [PATCH 32/32] Relax IDE CHS limits from 16383, 16, 63 to 65535, 16, 255

[Kevin Wolf]

Am 29.06.2012 17:34, schrieb Markus Armbruster:
> New limits straight from ATA4 6.2 Register delivered data transfer
> command sector addressing.
> 
> I figure the old sector limit 63 was blindly copied from the BIOS
> int 13 limit.  Doesn't apply to the hardware.  No idea where the old
> cylinder limit comes from.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Now I think we have the very same thing in IDE, SCSI and virtio-blk.
Would it make sense to have a helper function in hd-geometry.c that
takes and validates the geometry from a BlockConf, applies defaults and
puts the result into device state fields passed by reference?

Kevin

Re: [Qemu-devel] [PATCH 32/32] Relax IDE CHS limits from 16383, 16, 63 to 65535, 16, 255

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 29.06.2012 17:34, schrieb Markus Armbruster:
>> New limits straight from ATA4 6.2 Register delivered data transfer
>> command sector addressing.
>> 
>> I figure the old sector limit 63 was blindly copied from the BIOS
>> int 13 limit.  Doesn't apply to the hardware.  No idea where the old
>> cylinder limit comes from.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Now I think we have the very same thing in IDE, SCSI and virtio-blk.
> Would it make sense to have a helper function in hd-geometry.c that
> takes and validates the geometry from a BlockConf, applies defaults and
> puts the result into device state fields passed by reference?

I can look into this, but I'm afraid we'd need two helpers, because of
IDE complications.


When it comes to block device models, IDE is *always* the troublemaker.
And a big reason for that is the messy data structures that are
impractical to clean up while we still support non-qdevified IDE.  Which
we do almost three years after IDE qdevification.

The laggards are:

* mac99, g3beige
  Alexander Graf <agraf@suse.de>

* spitz, borzoi, terrier
  Andrzej Zaborowski <balrogg@gmail.com>

* tosa
  unmaintained

* r2d
  Magnus Damm <magnus.damm@gmail.com>

Any plans to drag these boards into the current decade already?

Re: [Qemu-devel] [PATCH 32/32] Relax IDE CHS limits from 16383, 16, 63 to 65535, 16, 255

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Kevin Wolf <kwolf@redhat.com> writes:
>
>> Am 29.06.2012 17:34, schrieb Markus Armbruster:
>>> New limits straight from ATA4 6.2 Register delivered data transfer
>>> command sector addressing.
>>> 
>>> I figure the old sector limit 63 was blindly copied from the BIOS
>>> int 13 limit.  Doesn't apply to the hardware.  No idea where the old
>>> cylinder limit comes from.
>>> 
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>
>> Now I think we have the very same thing in IDE, SCSI and virtio-blk.
>> Would it make sense to have a helper function in hd-geometry.c that
>> takes and validates the geometry from a BlockConf, applies defaults and
>> puts the result into device state fields passed by reference?
>
> I can look into this, but I'm afraid we'd need two helpers, because of
> IDE complications.

I'd like to try this in a follow-up series, together with a few more
cleanups made possible by this series.  Okay?

[...]

Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf

[Blue Swirl]

On Tue, Jul 3, 2012 at 8:15 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 03.07.2012 21:11, schrieb Blue Swirl:
>> On Mon, Jul 2, 2012 at 2:34 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>> On Mon, Jul 2, 2012 at 3:15 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>>> Andreas Färber <afaerber@suse.de> writes:
>>>>
>>>>> Am 02.07.2012 14:55, schrieb Stefan Hajnoczi:
>>>>>> On Fri, Jun 29, 2012 at 05:34:40PM +0200, Markus Armbruster wrote:
>>>>>>> Best to use the same type, to avoid unwanted truncation or sign
>>>>>>> extension.
>>>>>>>
>>>>>>> BlockConf can't use plain int for cyls, heads and secs, because
>>>>>>> integer properties require an exact width.
>>>>>>>
>>>>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>>>>> ---
>>>>>>>  blockdev.h       |    2 +-
>>>>>>>  hw/hd-geometry.c |    4 ++--
>>>>>>>  hw/ide/core.c    |    2 +-
>>>>>>>  hw/scsi-disk.c   |    2 +-
>>>>>>>  hw/virtio-blk.c  |    2 +-
>>>>>>>  5 files changed, 6 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> It would be nice to update the hd_geometry_lchs_guess() trace event that
>>>>>> you added to use uint32_t + %u instead of int + %d.
>>>>>
>>>>> PRIu32?
>>>>
>>>> uint32_t: Good point, will do.
>>>>
>>>> Conversion specifier: trace-events routinely prints uint32_t with %d and
>>>> %x, never with PRI*32.  I'm happy to change from %d to %u, but reluctant
>>>> to add the first use of PRIu32.
>>>
>>> Either is fine by me although we might as well continue to do %u.
>>
>> I'd also vote for %u. PRI*32 do not seem very useful compared to plain
>> int versions.
>
> If it's not useful we should use unsigned int.
>
> Mixing both is simply wrong. I'm okay with fixing it consistently in a
> follow-up though. The Broken Window theory scores again (lack of const,
> non-CamelCase type names, placement of {, wrong semicolons, etc.).

Since the 'const' issues are not related to CODING_STYLE (for those
Anthony vetoes global reformatting), I think global patches
(preferably broken down for bisection though) should be OK.

What's wrong with the semicolons?

>
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf

[Blue Swirl]

On Wed, Jul 4, 2012 at 4:36 PM, Eric Blake <eblake@redhat.com> wrote:
> On 07/04/2012 10:19 AM, Paolo Bonzini wrote:
>> Il 03/07/2012 22:15, Andreas Färber ha scritto:
>>>>>
>>>>> I'd also vote for %u. PRI*32 do not seem very useful compared to plain
>>>>> int versions.
>>> If it's not useful we should use unsigned int.
>>
>> It just means that we're assuming 32-bit ints, which is a fact.  The day
>> an IL32 platform appears that has "typedef long int32_t", we'll worry
>> about PRIu32.
>
> Newlib's <stdint.h> does just that, on platforms that __have_long32.
> Which makes Cygwin a great platform for flushing out these sorts of type
> discrepancies (since cygwin inherits from newlib).
>
> http://sourceware.org/cgi-bin/cvsweb.cgi/src/newlib/libc/include/stdint.h.diff?r1=1.8&r2=1.9&cvsroot=src&f=h

Interesting, my thoughts went pretty much along Paolo's line. Now I
think PRI*32 indeed makes some sense.

But we don't support Cygwin builds (only mingw), would that be useful?

>
> --
> Eric Blake   eblake@redhat.com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
>
>

Re: [Qemu-devel] [PATCH 32/32] Relax IDE CHS limits from 16383, 16, 63 to 65535, 16, 255

[Kevin Wolf]

Am 05.07.2012 18:39, schrieb Markus Armbruster:
> Markus Armbruster <armbru@redhat.com> writes:
> 
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>>> Am 29.06.2012 17:34, schrieb Markus Armbruster:
>>>> New limits straight from ATA4 6.2 Register delivered data transfer
>>>> command sector addressing.
>>>>
>>>> I figure the old sector limit 63 was blindly copied from the BIOS
>>>> int 13 limit.  Doesn't apply to the hardware.  No idea where the old
>>>> cylinder limit comes from.
>>>>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>
>>> Now I think we have the very same thing in IDE, SCSI and virtio-blk.
>>> Would it make sense to have a helper function in hd-geometry.c that
>>> takes and validates the geometry from a BlockConf, applies defaults and
>>> puts the result into device state fields passed by reference?
>>
>> I can look into this, but I'm afraid we'd need two helpers, because of
>> IDE complications.
> 
> I'd like to try this in a follow-up series, together with a few more
> cleanups made possible by this series.  Okay?

Fine with me.

Kevin

Re: [Qemu-devel] [PATCH 32/32] Relax IDE CHS limits from 16383, 16, 63 to 65535, 16, 255

[Alexander Graf]

On 05.07.2012, at 18:20, Markus Armbruster wrote:

> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> Am 29.06.2012 17:34, schrieb Markus Armbruster:
>>> New limits straight from ATA4 6.2 Register delivered data transfer
>>> command sector addressing.
>>> 
>>> I figure the old sector limit 63 was blindly copied from the BIOS
>>> int 13 limit.  Doesn't apply to the hardware.  No idea where the old
>>> cylinder limit comes from.
>>> 
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> 
>> Now I think we have the very same thing in IDE, SCSI and virtio-blk.
>> Would it make sense to have a helper function in hd-geometry.c that
>> takes and validates the geometry from a BlockConf, applies defaults and
>> puts the result into device state fields passed by reference?
> 
> I can look into this, but I'm afraid we'd need two helpers, because of
> IDE complications.
> 
> 
> When it comes to block device models, IDE is *always* the troublemaker.
> And a big reason for that is the messy data structures that are
> impractical to clean up while we still support non-qdevified IDE.  Which
> we do almost three years after IDE qdevification.
> 
> The laggards are:
> 
> * mac99, g3beige
>  Alexander Graf <agraf@suse.de>
> 
> * spitz, borzoi, terrier
>  Andrzej Zaborowski <balrogg@gmail.com>
> 
> * tosa
>  unmaintained
> 
> * r2d
>  Magnus Damm <magnus.damm@gmail.com>
> 
> Any plans to drag these boards into the current decade already?

Got a pointer to a board that does it right?


Alex

Re: [Qemu-devel] [PATCH 32/32] Relax IDE CHS limits from 16383, 16, 63 to 65535, 16, 255

[Markus Armbruster]

Alexander Graf <agraf@suse.de> writes:

> On 05.07.2012, at 18:20, Markus Armbruster wrote:
[...]
>> When it comes to block device models, IDE is *always* the troublemaker.
>> And a big reason for that is the messy data structures that are
>> impractical to clean up while we still support non-qdevified IDE.  Which
>> we do almost three years after IDE qdevification.
>> 
>> The laggards are:
>> 
>> * mac99, g3beige
>>  Alexander Graf <agraf@suse.de>
>> 
>> * spitz, borzoi, terrier
>>  Andrzej Zaborowski <balrogg@gmail.com>
>> 
>> * tosa
>>  unmaintained
>> 
>> * r2d
>>  Magnus Damm <magnus.damm@gmail.com>
>> 
>> Any plans to drag these boards into the current decade already?
>
> Got a pointer to a board that does it right?

Any board that uses pci_cmd646_ide_init(), pci_piix[34]_ide_init(), or
vt82c686b_ide_init().  These create a qdev providing an IDE bus.  The
non-qdevified IDE controllers use ide_init2_with_non_qdev_drives()
instead.

Funny: g3beige has two IDE controllers, first one isn't qdevified
(created with pmac_ide_init()), second one is (created with
pci_cmd646_ide_init()).

Feel free to ask for more help!

Re: [Qemu-devel] [PATCH 32/32] Relax IDE CHS limits from 16383, 16, 63 to 65535, 16, 255

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 05.07.2012 18:39, schrieb Markus Armbruster:
>> Markus Armbruster <armbru@redhat.com> writes:
>> 
>>> Kevin Wolf <kwolf@redhat.com> writes:
>>>
>>>> Am 29.06.2012 17:34, schrieb Markus Armbruster:
>>>>> New limits straight from ATA4 6.2 Register delivered data transfer
>>>>> command sector addressing.
>>>>>
>>>>> I figure the old sector limit 63 was blindly copied from the BIOS
>>>>> int 13 limit.  Doesn't apply to the hardware.  No idea where the old
>>>>> cylinder limit comes from.
>>>>>
>>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>>
>>>> Now I think we have the very same thing in IDE, SCSI and virtio-blk.
>>>> Would it make sense to have a helper function in hd-geometry.c that
>>>> takes and validates the geometry from a BlockConf, applies defaults and
>>>> puts the result into device state fields passed by reference?
>>>
>>> I can look into this, but I'm afraid we'd need two helpers, because of
>>> IDE complications.
>> 
>> I'd like to try this in a follow-up series, together with a few more
>> cleanups made possible by this series.  Okay?
>
> Fine with me.

Done: [PATCH 0/4] Cleanups around hw/block-common.h

[Qemu-devel] [PATCH 32/32] Relax IDE CHS limits from 16383, 16, 63 to 65535, 16, 255

[Markus Armbruster]

New limits straight from ATA4 6.2 Register delivered data transfer
command sector addressing.

I figure the old sector limit 63 was blindly copied from the BIOS
int 13 limit.  Doesn't apply to the hardware.  No idea where the old
cylinder limit comes from.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/core.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 1ca7cdf..58a454f 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1935,16 +1935,16 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
     s->drive_kind = kind;
 
     bdrv_get_geometry(bs, &nb_sectors);
-    if (cylinders < 1 || cylinders > 16383) {
-        error_report("cyls must be between 1 and 16383");
+    if (cylinders < 1 || cylinders > 65535) {
+        error_report("cyls must be between 1 and 65535");
         return -1;
     }
     if (heads < 1 || heads > 16) {
         error_report("heads must be between 1 and 16");
         return -1;
     }
-    if (secs < 1 || secs > 63) {
-        error_report("secs must be between 1 and 63");
+    if (secs < 1 || secs > 255) {
+        error_report("secs must be between 1 and 255");
         return -1;
     }
     s->cylinders = cylinders;
-- 
1.7.6.5