| Message ID | 1312376904-16115-42-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 03.08.2011 15:08, schrieb Markus Armbruster: > Device models should be able to set it without an unclean include of > block_int.h. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > block.c | 6 ++++-- > block.h | 1 + > hw/ide/core.c | 2 +- > hw/scsi-disk.c | 2 +- > hw/virtio-blk.c | 3 +-- > 5 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/block.c b/block.c > index fed0c16..67d9429 100644 > --- a/block.c > +++ b/block.c > @@ -453,7 +453,6 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename, > bs->encrypted = 0; > bs->valid_key = 0; > bs->open_flags = flags; > - /* buffer_alignment defaulted to 512, drivers can change this value */ > bs->buffer_alignment = 512; This comment is still right. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 03.08.2011 15:08, schrieb Markus Armbruster: >> Device models should be able to set it without an unclean include of >> block_int.h. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> block.c | 6 ++++-- >> block.h | 1 + >> hw/ide/core.c | 2 +- >> hw/scsi-disk.c | 2 +- >> hw/virtio-blk.c | 3 +-- >> 5 files changed, 8 insertions(+), 6 deletions(-) >> >> diff --git a/block.c b/block.c >> index fed0c16..67d9429 100644 >> --- a/block.c >> +++ b/block.c >> @@ -453,7 +453,6 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename, >> bs->encrypted = 0; >> bs->valid_key = 0; >> bs->open_flags = flags; >> - /* buffer_alignment defaulted to 512, drivers can change this value */ >> bs->buffer_alignment = 512; > > This comment is still right. It's also next to useless. I hate comments paraphrasing the code :) I'll keep it if you insist.
Am 02.09.2011 17:30, schrieb Markus Armbruster: > Kevin Wolf <kwolf@redhat.com> writes: > >> Am 03.08.2011 15:08, schrieb Markus Armbruster: >>> Device models should be able to set it without an unclean include of >>> block_int.h. >>> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>> --- >>> block.c | 6 ++++-- >>> block.h | 1 + >>> hw/ide/core.c | 2 +- >>> hw/scsi-disk.c | 2 +- >>> hw/virtio-blk.c | 3 +-- >>> 5 files changed, 8 insertions(+), 6 deletions(-) >>> >>> diff --git a/block.c b/block.c >>> index fed0c16..67d9429 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -453,7 +453,6 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename, >>> bs->encrypted = 0; >>> bs->valid_key = 0; >>> bs->open_flags = flags; >>> - /* buffer_alignment defaulted to 512, drivers can change this value */ >>> bs->buffer_alignment = 512; >> >> This comment is still right. > > It's also next to useless. I hate comments paraphrasing the code :) > > I'll keep it if you insist. The information that the comment gives isn't that we set it to 512, but that this is only a default and "drivers" (should be "devices", I think) are expected to change it. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 02.09.2011 17:30, schrieb Markus Armbruster: >> Kevin Wolf <kwolf@redhat.com> writes: >> >>> Am 03.08.2011 15:08, schrieb Markus Armbruster: >>>> Device models should be able to set it without an unclean include of >>>> block_int.h. >>>> >>>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>>> --- >>>> block.c | 6 ++++-- >>>> block.h | 1 + >>>> hw/ide/core.c | 2 +- >>>> hw/scsi-disk.c | 2 +- >>>> hw/virtio-blk.c | 3 +-- >>>> 5 files changed, 8 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/block.c b/block.c >>>> index fed0c16..67d9429 100644 >>>> --- a/block.c >>>> +++ b/block.c >>>> @@ -453,7 +453,6 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename, >>>> bs->encrypted = 0; >>>> bs->valid_key = 0; >>>> bs->open_flags = flags; >>>> - /* buffer_alignment defaulted to 512, drivers can change this value */ >>>> bs->buffer_alignment = 512; >>> >>> This comment is still right. >> >> It's also next to useless. I hate comments paraphrasing the code :) >> >> I'll keep it if you insist. > > The information that the comment gives isn't that we set it to 512, but > that this is only a default and "drivers" (should be "devices", I think) > are expected to change it. Devices can and do change any number of default values set up here, and documenting that just for buffer_alignment is useless at best, misleading at worst. Nevertheless, I'll keep it if you insist.
diff --git a/block.c b/block.c index fed0c16..67d9429 100644 --- a/block.c +++ b/block.c @@ -453,7 +453,6 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename, bs->encrypted = 0; bs->valid_key = 0; bs->open_flags = flags; - /* buffer_alignment defaulted to 512, drivers can change this value */ bs->buffer_alignment = 512; pstrcpy(bs->filename, sizeof(bs->filename), filename); @@ -3070,7 +3069,10 @@ BlockDriverAIOCB *bdrv_aio_ioctl(BlockDriverState *bs, return NULL; } - +void bdrv_set_buffer_alignment(BlockDriverState *bs, int align) +{ + bs->buffer_alignment = align; +} void *qemu_blockalign(BlockDriverState *bs, size_t size) { diff --git a/block.h b/block.h index 3dfd2a4..3885208 100644 --- a/block.h +++ b/block.h @@ -269,6 +269,7 @@ int bdrv_img_create(const char *filename, const char *fmt, const char *base_filename, const char *base_fmt, char *options, uint64_t img_size, int flags); +void bdrv_set_buffer_alignment(BlockDriverState *bs, int align); void *qemu_blockalign(BlockDriverState *bs, size_t size); #define BDRV_SECTORS_PER_DIRTY_CHUNK 2048 diff --git a/hw/ide/core.c b/hw/ide/core.c index 24e10b2..bd59016 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -1821,7 +1821,7 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind, s->smart_selftest_count = 0; if (kind == IDE_CD) { bdrv_set_dev_ops(bs, &ide_cd_block_ops, s); - bs->buffer_alignment = 2048; + bdrv_set_buffer_alignment(bs, 2048); } else { if (!bdrv_is_inserted(s->bs)) { error_report("Device needs media, but drive is empty"); diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c index 5159e5d..71d6408 100644 --- a/hw/scsi-disk.c +++ b/hw/scsi-disk.c @@ -1275,7 +1275,7 @@ static int scsi_initfn(SCSIDevice *dev, uint8_t scsi_type) return -1; } s->cluster_size = s->qdev.blocksize / 512; - s->bs->buffer_alignment = s->qdev.blocksize; + bdrv_set_buffer_alignment(s->bs, s->qdev.blocksize); s->qdev.type = scsi_type; qemu_add_vm_change_state_handler(scsi_dma_restart_cb, s); diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c index a517150..811d709 100644 --- a/hw/virtio-blk.c +++ b/hw/virtio-blk.c @@ -15,7 +15,6 @@ #include "qemu-error.h" #include "trace.h" #include "blockdev.h" -#include "block_int.h" #include "virtio-blk.h" #ifdef __linux__ # include <scsi/sg.h> @@ -585,7 +584,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf, register_savevm(dev, "virtio-blk", virtio_blk_id++, 2, virtio_blk_save, virtio_blk_load, s); bdrv_set_dev_ops(s->bs, &virtio_block_ops, s); - s->bs->buffer_alignment = conf->logical_block_size; + bdrv_set_buffer_alignment(s->bs, conf->logical_block_size); add_boot_device_path(conf->bootindex, dev, "/disk@0,0");
Device models should be able to set it without an unclean include of block_int.h. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- block.c | 6 ++++-- block.h | 1 + hw/ide/core.c | 2 +- hw/scsi-disk.c | 2 +- hw/virtio-blk.c | 3 +-- 5 files changed, 8 insertions(+), 6 deletions(-)