| Message ID | 54DCD80E.5030608@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
Am 12.02.2015 um 17:42 schrieb Christian Borntraeger: > Am 12.02.2015 um 16:46 schrieb Stefan Hajnoczi: >> On Mon, Jan 19, 2015 at 03:34:56PM +0100, Ekaterina Tumanova wrote: >>> Updates v5 -> v6: >>> >>> Minor Updates according the last review from Stefan Hajnoczi: >>> 1. Do not change the flow of code, factored out of raw_probe_alignment. >>> 2. added #ifdef __linux__ in 2 places of raw-posix.c, mentioned by reviewer. >>> 3. adjusted the comment hdev_probe_geometry according suggestment. >>> 4. use bdrv_nb_sectors(bs) instead of bs->total_sectors. >>> 5. do not discard error blk_probe_blocksizes(). now has rc. >>> 6. put the 512-byte default blocksize value in blkconf_blocksizes. >>> 7. drop the default parameter from the DEFINE_PROP_BLOCKSIZE() macro. >> >> Unfortunately this series breaks "make check" so it cannot be merged: >> >> GTESTER check-qtest-x86_64 >> qemu-system-x86_64: logical_block_size must be 512 for IDE >> qemu-system-x86_64: Device initialization failed. >> qemu-system-x86_64: Initialization of device ide-cd failed >> Broken pipe >> GTester: last random seed: R02S942fac7e56eff09e8ab7a7f7fecf847e >> > > This particular message came in with > > commit d20051856cd2fa8f10fed2d2a0b2751de5f7b20d > Author: Kevin Wolf <kwolf@redhat.com> > Date: Wed Dec 3 13:21:32 2014 +0100 > > ide: Check validity of logical block size > > so something like > diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c > index 353854c..2680275 100644 > --- a/hw/ide/qdev.c > +++ b/hw/ide/qdev.c > @@ -163,7 +163,8 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind) > return -1; > } > > - if (dev->conf.logical_block_size != 512) { > + if (dev->conf.logical_block_size != 512 && > + dev->conf.logical_block_size != 0) { > error_report("logical_block_size must be 512 for IDE"); > return -1; > } > > > will fix this. > > Now we have > block.c:582: bdrv_probe_blocksizes: Assertion `drv != ((void *)0)' failed > > so > we need something like this on top - I guess. > > diff --git a/block.c b/block.c > index dbc2519..8bbcc6d 100644 > --- a/block.c > +++ b/block.c > @@ -579,8 +579,7 @@ int bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz) > { > BlockDriver *drv = bs->drv; > > - assert(drv != NULL); > - if (drv->bdrv_probe_blocksizes) { > + if (drv && drv->bdrv_probe_blocksizes) { > return drv->bdrv_probe_blocksizes(bs, bsz); > } > > @@ -597,8 +596,7 @@ int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo) > { > BlockDriver *drv = bs->drv; > > - assert(drv != NULL); > - if (drv->bdrv_probe_geometry) { > + if (drv && drv->bdrv_probe_geometry) { > return drv->bdrv_probe_geometry(bs, geo); > } > > > Kate, I think its time for a v7 :-( Of course, please double check my quick hacks. I still see some broken pipe messages, so I must have missed something.
Am 12.02.2015 um 17:42 hat Christian Borntraeger geschrieben: > Am 12.02.2015 um 16:46 schrieb Stefan Hajnoczi: > > On Mon, Jan 19, 2015 at 03:34:56PM +0100, Ekaterina Tumanova wrote: > >> Updates v5 -> v6: > >> > >> Minor Updates according the last review from Stefan Hajnoczi: > >> 1. Do not change the flow of code, factored out of raw_probe_alignment. > >> 2. added #ifdef __linux__ in 2 places of raw-posix.c, mentioned by reviewer. > >> 3. adjusted the comment hdev_probe_geometry according suggestment. > >> 4. use bdrv_nb_sectors(bs) instead of bs->total_sectors. > >> 5. do not discard error blk_probe_blocksizes(). now has rc. > >> 6. put the 512-byte default blocksize value in blkconf_blocksizes. > >> 7. drop the default parameter from the DEFINE_PROP_BLOCKSIZE() macro. > > > > Unfortunately this series breaks "make check" so it cannot be merged: > > > > GTESTER check-qtest-x86_64 > > qemu-system-x86_64: logical_block_size must be 512 for IDE > > qemu-system-x86_64: Device initialization failed. > > qemu-system-x86_64: Initialization of device ide-cd failed > > Broken pipe > > GTester: last random seed: R02S942fac7e56eff09e8ab7a7f7fecf847e > > > > This particular message came in with > > commit d20051856cd2fa8f10fed2d2a0b2751de5f7b20d > Author: Kevin Wolf <kwolf@redhat.com> > Date: Wed Dec 3 13:21:32 2014 +0100 > > ide: Check validity of logical block size > > so something like > diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c > index 353854c..2680275 100644 > --- a/hw/ide/qdev.c > +++ b/hw/ide/qdev.c > @@ -163,7 +163,8 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind) > return -1; > } > > - if (dev->conf.logical_block_size != 512) { > + if (dev->conf.logical_block_size != 512 && > + dev->conf.logical_block_size != 0) { > error_report("logical_block_size must be 512 for IDE"); > return -1; > } > > > will fix this. It would probably be better to set the default first and then make sure that the final value, no matter whether explicitly specified or default, is 512. Kevin
Am 13.02.2015 um 08:50 schrieb Kevin Wolf: > Am 12.02.2015 um 17:42 hat Christian Borntraeger geschrieben: >> Am 12.02.2015 um 16:46 schrieb Stefan Hajnoczi: >>> On Mon, Jan 19, 2015 at 03:34:56PM +0100, Ekaterina Tumanova wrote: >>>> Updates v5 -> v6: >>>> >>>> Minor Updates according the last review from Stefan Hajnoczi: >>>> 1. Do not change the flow of code, factored out of raw_probe_alignment. >>>> 2. added #ifdef __linux__ in 2 places of raw-posix.c, mentioned by reviewer. >>>> 3. adjusted the comment hdev_probe_geometry according suggestment. >>>> 4. use bdrv_nb_sectors(bs) instead of bs->total_sectors. >>>> 5. do not discard error blk_probe_blocksizes(). now has rc. >>>> 6. put the 512-byte default blocksize value in blkconf_blocksizes. >>>> 7. drop the default parameter from the DEFINE_PROP_BLOCKSIZE() macro. >>> >>> Unfortunately this series breaks "make check" so it cannot be merged: >>> >>> GTESTER check-qtest-x86_64 >>> qemu-system-x86_64: logical_block_size must be 512 for IDE >>> qemu-system-x86_64: Device initialization failed. >>> qemu-system-x86_64: Initialization of device ide-cd failed >>> Broken pipe >>> GTester: last random seed: R02S942fac7e56eff09e8ab7a7f7fecf847e >>> >> >> This particular message came in with >> >> commit d20051856cd2fa8f10fed2d2a0b2751de5f7b20d >> Author: Kevin Wolf <kwolf@redhat.com> >> Date: Wed Dec 3 13:21:32 2014 +0100 >> >> ide: Check validity of logical block size >> >> so something like >> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c >> index 353854c..2680275 100644 >> --- a/hw/ide/qdev.c >> +++ b/hw/ide/qdev.c >> @@ -163,7 +163,8 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind) >> return -1; >> } >> >> - if (dev->conf.logical_block_size != 512) { >> + if (dev->conf.logical_block_size != 512 && >> + dev->conf.logical_block_size != 0) { >> error_report("logical_block_size must be 512 for IDE"); >> return -1; >> } >> >> >> will fix this. > > It would probably be better to set the default first and then make sure > that the final value, no matter whether explicitly specified or default, > is 512. > > Kevin +1 Yes, this should minimize the impact.
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c index 353854c..2680275 100644 --- a/hw/ide/qdev.c +++ b/hw/ide/qdev.c @@ -163,7 +163,8 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind) return -1; } - if (dev->conf.logical_block_size != 512) { + if (dev->conf.logical_block_size != 512 && + dev->conf.logical_block_size != 0) { error_report("logical_block_size must be 512 for IDE"); return -1; } will fix this. Now we have block.c:582: bdrv_probe_blocksizes: Assertion `drv != ((void *)0)' failed so we need something like this on top - I guess. diff --git a/block.c b/block.c index dbc2519..8bbcc6d 100644 --- a/block.c +++ b/block.c @@ -579,8 +579,7 @@ int bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz) { BlockDriver *drv = bs->drv; - assert(drv != NULL); - if (drv->bdrv_probe_blocksizes) { + if (drv && drv->bdrv_probe_blocksizes) { return drv->bdrv_probe_blocksizes(bs, bsz); } @@ -597,8 +596,7 @@ int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo) { BlockDriver *drv = bs->drv; - assert(drv != NULL); - if (drv->bdrv_probe_geometry) { + if (drv && drv->bdrv_probe_geometry) { return drv->bdrv_probe_geometry(bs, geo); }