[Qemu-devel] Question about total_sectors in block/vpc.c
Hell all, I have take a look of block/vpc.c and meet a question in vpc_create(). At the line 550, the code is: total_sectors = options->value.n / 512; I am wondering whether the size between total_sectors * 512 and options->value.n would be discard. Thanks Mitnick
Re: [Qemu-devel] Question about total_sectors in block/vpc.c
On Sat, Apr 9, 2011 at 5:51 PM, Lyu Mitnick wrote: > Hell all, > I have take a look of block/vpc.c and meet a question in vpc_create(). At > the line > 550, the code is: > total_sectors = options->value.n / 512; > I am wondering whether the size between total_sectors * 512 > and options->value.n > would be discard. Yes, it rounds down. This reflects the assumption that a block device cannot be addressed below 512 byte sectors. Because of this block devices size must be a multiple of 512 bytes. I think a reasonable protection would be to have block.c:bdrv_create() fail if size is not a multiple of BDRV_SECTOR_SIZE. This way other image formats are protected too. Stefan
Re: [Qemu-devel] Question about total_sectors in block/vpc.c
Hello Stefan,
Is it your means:
There is an assumption that a block device cannot be addressed below 512
byte sectors.
A reasonable protection in block.c:bdrv_create() to check whether size is a
multiple of BDRV_SECTOR_SIZE.
Signed-off-by: Mitnick Lyu
---
block.c | 10 ++
1 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/block.c b/block.c
index f731c7a..a80ec49 100644
--- a/block.c
+++ b/block.c
@@ -239,6 +239,16 @@ int bdrv_create(BlockDriver *drv, const char* filename,
if (!drv->bdrv_create)
return -ENOTSUP;
+ while (options && options->name) {
+ if (!strcmp(options->name, "size")) {
+ if (options->value.n % 512 == 0)
+ break;
+ else
+ return -EINVAL;
+ }
+ options++;
+ }
+
return drv->bdrv_create(filename, options);
}
--
1.7.0.4
2011/4/10 Stefan Hajnoczi
> On Sat, Apr 9, 2011 at 5:51 PM, Lyu Mitnick wrote:
> > Hell all,
> > I have take a look of block/vpc.c and meet a question in vpc_create(). At
> > the line
> > 550, the code is:
> > total_sectors = options->value.n / 512;
> > I am wondering whether the size between total_sectors * 512
> > and options->value.n
> > would be discard.
>
> Yes, it rounds down. This reflects the assumption that a block device
> cannot be addressed below 512 byte sectors. Because of this block
> devices size must be a multiple of 512 bytes.
>
> I think a reasonable protection would be to have block.c:bdrv_create()
> fail if size is not a multiple of BDRV_SECTOR_SIZE. This way other
> image formats are protected too.
>
> Stefan
>
By the way, how could I know a submitted patch is accepted or not??
Thanks a lot
Mitnick
Re: [Qemu-devel] Question about total_sectors in block/vpc.c
On Sun, Apr 10, 2011 at 05:02:20PM +0800, Lyu Mitnick wrote:
> diff --git a/block.c b/block.c
> index f731c7a..a80ec49 100644
> --- a/block.c
> +++ b/block.c
> @@ -239,6 +239,16 @@ int bdrv_create(BlockDriver *drv, const char* filename,
> if (!drv->bdrv_create)
> return -ENOTSUP;
>
> + while (options && options->name) {
> + if (!strcmp(options->name, "size")) {
> + if (options->value.n % 512 == 0)
> + break;
> + else
> + return -EINVAL;
> + }
> + options++;
> + }
Please use BDRV_SECTOR_SIZE instead of hardcoding 512.
get_option_parameter() does the search for you, please use it instead of
duplicating the loop.
Please see the CODING_STYLE and HACKING files, new code should follow it:
* Indentation is 4 spaces
* Always use {} even for if/else with single-statement bodies
Stefan
Re: [Qemu-devel] Question about total_sectors in block/vpc.c
Hello Christoph, Stefan I am wondering whether the problem occurred that value of BDRV_SECTOR_SIZE is a macro constant (defined at block.h). This problem could be fixed if we use variable instead of macro to implement BDRV_SECTOR_SIZE. Each block device may reassign the value if needed. Is it right?? Thanks a lot 2011/4/12 Christoph Hellwig > On Sat, Apr 09, 2011 at 09:05:41PM +0100, Stefan Hajnoczi wrote: > > On Sat, Apr 9, 2011 at 5:51 PM, Lyu Mitnick > wrote: > > > Hell all, > > > I have take a look of block/vpc.c and meet a question in > vpc_create().?At > > > the line > > > 550, the code is: > > > total_sectors = options->value.n / 512; > > > I am wondering whether the size between?total_sectors * 512 > > > and?options->value.n > > > would be discard. > > > > Yes, it rounds down. This reflects the assumption that a block device > > cannot be addressed below 512 byte sectors. Because of this block > > devices size must be a multiple of 512 bytes. > > > > I think a reasonable protection would be to have block.c:bdrv_create() > > fail if size is not a multiple of BDRV_SECTOR_SIZE. This way other > > image formats are protected too. > > There are block devices that aren't alignment to 512 bytes. Audio CDROMs > are > the most prominent example, or AS/400 disks. I don't think these matter > for > emulation, but if we'd ever implement DIF/DIX emulation inside qemu we'd > have to store the protection information somewhere. It still wouldn't > work with existing disk format, so adding the above check into the formats > bdrv_create routines sounds fine, but doing it in the core block code > might not be an overly smart idea. > > Mitnick
Re: [Qemu-devel] Question about total_sectors in block/vpc.c
On Sat, Apr 09, 2011 at 09:05:41PM +0100, Stefan Hajnoczi wrote: > On Sat, Apr 9, 2011 at 5:51 PM, Lyu Mitnick wrote: > > Hell all, > > I have take a look of block/vpc.c and meet a question in vpc_create().?At > > the line > > 550, the code is: > > total_sectors = options->value.n / 512; > > I am wondering whether the size between?total_sectors * 512 > > and?options->value.n > > would be discard. > > Yes, it rounds down. This reflects the assumption that a block device > cannot be addressed below 512 byte sectors. Because of this block > devices size must be a multiple of 512 bytes. > > I think a reasonable protection would be to have block.c:bdrv_create() > fail if size is not a multiple of BDRV_SECTOR_SIZE. This way other > image formats are protected too. There are block devices that aren't alignment to 512 bytes. Audio CDROMs are the most prominent example, or AS/400 disks. I don't think these matter for emulation, but if we'd ever implement DIF/DIX emulation inside qemu we'd have to store the protection information somewhere. It still wouldn't work with existing disk format, so adding the above check into the formats bdrv_create routines sounds fine, but doing it in the core block code might not be an overly smart idea.
Re: [Qemu-devel] Question about total_sectors in block/vpc.c
Hello Stefan,
I have a question about get_option_parameter(). I am wondering whether
get_option_parameter is suitable to use instead of doing the search by
myself
in the case like following:
/* Read out options */
while (options && options->name) {
if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
// do something
} else if (!strcmp(options->name, BLOCK_OPT_CLUSTER_SIZE)) {
// do something
}
options++;
}
Thanks
2011/4/11 Stefan Hajnoczi
> On Sun, Apr 10, 2011 at 05:02:20PM +0800, Lyu Mitnick wrote:
> > diff --git a/block.c b/block.c
> > index f731c7a..a80ec49 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -239,6 +239,16 @@ int bdrv_create(BlockDriver *drv, const char*
> filename,
> > if (!drv->bdrv_create)
> > return -ENOTSUP;
> >
> > + while (options && options->name) {
> > + if (!strcmp(options->name, "size")) {
> > + if (options->value.n % 512 == 0)
> > + break;
> > + else
> > + return -EINVAL;
> > + }
> > + options++;
> > + }
>
> Please use BDRV_SECTOR_SIZE instead of hardcoding 512.
>
> get_option_parameter() does the search for you, please use it instead of
> duplicating the loop.
>
> Please see the CODING_STYLE and HACKING files, new code should follow it:
> * Indentation is 4 spaces
> * Always use {} even for if/else with single-statement bodies
>
> Stefan
>
Mitnick
Re: [Qemu-devel] Question about total_sectors in block/vpc.c
Am 13.04.2011 22:59, schrieb Lyu Mitnick:
> Hello Stefan,
>
> I have a question about get_option_parameter(). I am wondering whether
> get_option_parameter is suitable to use instead of doing the search by
> myself
> in the case like following:
>
> /* Read out options */
> while (options && options->name) {
> if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> // do something
> } else if (!strcmp(options->name, BLOCK_OPT_CLUSTER_SIZE)) {
>// do something
> }
> options++;
> }
Yes, I think it is, though you need to check whether the option has been
set in order to allow use default values.
Kevin
Re: [Qemu-devel] Question about total_sectors in block/vpc.c
Hello Kevin,
2011/4/14 Kevin Wolf
> Am 13.04.2011 22:59, schrieb Lyu Mitnick:
> > Hello Stefan,
> >
> > I have a question about get_option_parameter(). I am wondering whether
> > get_option_parameter is suitable to use instead of doing the search by
> > myself
> > in the case like following:
> >
> > /* Read out options */
> > while (options && options->name) {
> > if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> > // do something
> > } else if (!strcmp(options->name, BLOCK_OPT_CLUSTER_SIZE)) {
> >// do something
> > }
> > options++;
> > }
>
> Yes, I think it is, though you need to check whether the option has been
> set in order to allow use default values.
>
> Kevin
>
I have no idea about the mean of "check whether the option has been set in
order to allow use default values" , would you mind to give me an example
about
it??
So as the example above. I am wondering whether the code should be
rewritten
as:
/* Read out options */
if(get_option_parameter(options, BLOCK_OPT_SIZE)) {
// do something
}
if (get_option_parameter(options, BLOCK_OPT_CLUSTER_SIZE)) {
// do something
}
in QEMU??
Thanks
Mitnick
Re: [Qemu-devel] Question about total_sectors in block/vpc.c
Am 15.04.2011 22:40, schrieb Lyu Mitnick:
> Hello Kevin,
>
> 2011/4/14 Kevin Wolf mailto:kw...@redhat.com>>
>
> Am 13.04.2011 22:59, schrieb Lyu Mitnick:
> > Hello Stefan,
> >
> > I have a question about get_option_parameter(). I am wondering whether
> > get_option_parameter is suitable to use instead of doing the
> search by
> > myself
> > in the case like following:
> >
> > /* Read out options */
> > while (options && options->name) {
> > if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> > // do something
> > } else if (!strcmp(options->name, BLOCK_OPT_CLUSTER_SIZE)) {
> >// do something
> > }
> > options++;
> > }
>
> Yes, I think it is, though you need to check whether the option has been
> set in order to allow use default values.
>
> Kevin
>
>
> I have no idea about the mean of "check whether the option has been set in
> order to allow use default values" , would you mind to give me an
> example about
> it??
>
> So as the example above. I am wondering whether the code should be
> rewritten
> as:
>
> /* Read out options */
> if(get_option_parameter(options, BLOCK_OPT_SIZE)) {
> // do something
> }
>
> if (get_option_parameter(options, BLOCK_OPT_CLUSTER_SIZE)) {
> // do something
> }
get_option_parameter would never return NULL in your example.
Kevin
