[Qemu-devel] Question about total_sectors in block/vpc.c

[Qemu-devel] Question about total_sectors in block/vpc.c

[Lyu Mitnick]

[-- 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 --]

Re: [Qemu-devel] Question about total_sectors in block/vpc.c

[Stefan Hajnoczi]

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

Re: [Qemu-devel] Question about total_sectors in block/vpc.c

[Lyu Mitnick]

[-- 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 --]

Re: [Qemu-devel] Question about total_sectors in block/vpc.c

[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

Re: [Qemu-devel] Question about total_sectors in block/vpc.c

[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 <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.

Re: [Qemu-devel] Question about total_sectors in block/vpc.c

[Lyu Mitnick]

[-- 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 --]

Re: [Qemu-devel] Question about total_sectors in block/vpc.c

[Lyu Mitnick]

[-- 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 --]

Re: [Qemu-devel] Question about total_sectors in block/vpc.c

[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

Re: [Qemu-devel] Question about total_sectors in block/vpc.c

[Lyu Mitnick]

[-- 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 --]

Re: [Qemu-devel] Question about total_sectors in block/vpc.c

[Kevin Wolf]

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