[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