* [Qemu-devel] Question about total_sectors in block/vpc.c @ 2011-04-09 16:51 Lyu Mitnick 2011-04-09 20:05 ` Stefan Hajnoczi 0 siblings, 1 reply; 10+ messages in thread From: Lyu Mitnick @ 2011-04-09 16:51 UTC (permalink / raw) To: qemu-devel [-- Attachment #1: Type: text/plain, Size: 268 bytes --] 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 [-- Attachment #2: Type: text/html, Size: 390 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] Question about total_sectors in block/vpc.c 2011-04-09 16:51 [Qemu-devel] Question about total_sectors in block/vpc.c Lyu Mitnick @ 2011-04-09 20:05 ` Stefan Hajnoczi 2011-04-10 9:02 ` Lyu Mitnick 2011-04-11 18:14 ` Christoph Hellwig 0 siblings, 2 replies; 10+ messages in thread From: Stefan Hajnoczi @ 2011-04-09 20:05 UTC (permalink / raw) To: Lyu Mitnick; +Cc: Kevin Wolf, qemu-devel, Christoph Hellwig On Sat, Apr 9, 2011 at 5:51 PM, Lyu Mitnick <mitnick.lyu@gmail.com> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] Question about total_sectors in block/vpc.c 2011-04-09 20:05 ` Stefan Hajnoczi @ 2011-04-10 9:02 ` Lyu Mitnick 2011-04-11 8:33 ` Stefan Hajnoczi 2011-04-11 18:14 ` Christoph Hellwig 1 sibling, 1 reply; 10+ messages in thread From: Lyu Mitnick @ 2011-04-10 9:02 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: Kevin Wolf, qemu-devel, Christoph Hellwig [-- Attachment #1: Type: text/plain, Size: 1857 bytes --] 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 <mitnick.lyu@gmail.com> --- 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 <stefanha@gmail.com> > On Sat, Apr 9, 2011 at 5:51 PM, Lyu Mitnick <mitnick.lyu@gmail.com> 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 [-- Attachment #2: Type: text/html, Size: 2784 bytes --] ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] Question about total_sectors in block/vpc.c 2011-04-10 9:02 ` Lyu Mitnick @ 2011-04-11 8:33 ` Stefan Hajnoczi 2011-04-13 20:59 ` Lyu Mitnick 0 siblings, 1 reply; 10+ messages in thread From: Stefan Hajnoczi @ 2011-04-11 8:33 UTC (permalink / raw) To: Lyu Mitnick; +Cc: Kevin Wolf, qemu-devel, Christoph Hellwig 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] Question about total_sectors in block/vpc.c 2011-04-11 8:33 ` Stefan Hajnoczi @ 2011-04-13 20:59 ` Lyu Mitnick 2011-04-14 8:37 ` Kevin Wolf 0 siblings, 1 reply; 10+ messages in thread From: Lyu Mitnick @ 2011-04-13 20:59 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: Kevin Wolf, qemu-devel, Christoph Hellwig [-- Attachment #1: Type: text/plain, Size: 1556 bytes --] 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 <stefanha@gmail.com> > 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 [-- Attachment #2: Type: text/html, Size: 2231 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] Question about total_sectors in block/vpc.c 2011-04-13 20:59 ` Lyu Mitnick @ 2011-04-14 8:37 ` Kevin Wolf 2011-04-15 20:40 ` Lyu Mitnick 0 siblings, 1 reply; 10+ messages in thread From: Kevin Wolf @ 2011-04-14 8:37 UTC (permalink / raw) To: Lyu Mitnick; +Cc: Stefan Hajnoczi, qemu-devel, Christoph Hellwig 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] Question about total_sectors in block/vpc.c 2011-04-14 8:37 ` Kevin Wolf @ 2011-04-15 20:40 ` Lyu Mitnick 2011-04-18 13:13 ` Kevin Wolf 0 siblings, 1 reply; 10+ messages in thread From: Lyu Mitnick @ 2011-04-15 20:40 UTC (permalink / raw) To: Kevin Wolf; +Cc: Stefan Hajnoczi, qemu-devel, Christoph Hellwig [-- Attachment #1: Type: text/plain, Size: 1216 bytes --] Hello Kevin, 2011/4/14 Kevin Wolf <kwolf@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 } in QEMU?? Thanks Mitnick [-- Attachment #2: Type: text/html, Size: 1895 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] Question about total_sectors in block/vpc.c 2011-04-15 20:40 ` Lyu Mitnick @ 2011-04-18 13:13 ` Kevin Wolf 0 siblings, 0 replies; 10+ messages in thread From: Kevin Wolf @ 2011-04-18 13:13 UTC (permalink / raw) To: Lyu Mitnick; +Cc: Stefan Hajnoczi, qemu-devel, Christoph Hellwig Am 15.04.2011 22:40, schrieb Lyu Mitnick: > Hello Kevin, > > 2011/4/14 Kevin Wolf <kwolf@redhat.com <mailto:kwolf@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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] Question about total_sectors in block/vpc.c 2011-04-09 20:05 ` Stefan Hajnoczi 2011-04-10 9:02 ` Lyu Mitnick @ 2011-04-11 18:14 ` Christoph Hellwig 2011-04-11 22:40 ` Lyu Mitnick 1 sibling, 1 reply; 10+ messages in thread From: Christoph Hellwig @ 2011-04-11 18:14 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: Kevin Wolf, Christoph Hellwig, Lyu Mitnick, qemu-devel On Sat, Apr 09, 2011 at 09:05:41PM +0100, Stefan Hajnoczi wrote: > On Sat, Apr 9, 2011 at 5:51 PM, Lyu Mitnick <mitnick.lyu@gmail.com> 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] Question about total_sectors in block/vpc.c 2011-04-11 18:14 ` Christoph Hellwig @ 2011-04-11 22:40 ` Lyu Mitnick 0 siblings, 0 replies; 10+ messages in thread From: Lyu Mitnick @ 2011-04-11 22:40 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel [-- Attachment #1: Type: text/plain, Size: 1707 bytes --] 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 <hch@lst.de> > On Sat, Apr 09, 2011 at 09:05:41PM +0100, Stefan Hajnoczi wrote: > > On Sat, Apr 9, 2011 at 5:51 PM, Lyu Mitnick <mitnick.lyu@gmail.com> > 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 [-- Attachment #2: Type: text/html, Size: 3235 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2011-04-18 13:11 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-04-09 16:51 [Qemu-devel] Question about total_sectors in block/vpc.c Lyu Mitnick 2011-04-09 20:05 ` Stefan Hajnoczi 2011-04-10 9:02 ` Lyu Mitnick 2011-04-11 8:33 ` Stefan Hajnoczi 2011-04-13 20:59 ` Lyu Mitnick 2011-04-14 8:37 ` Kevin Wolf 2011-04-15 20:40 ` Lyu Mitnick 2011-04-18 13:13 ` Kevin Wolf 2011-04-11 18:14 ` Christoph Hellwig 2011-04-11 22:40 ` Lyu Mitnick
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.