diff mbox

Question about total_sectors in block/vpc.c

Message ID BANLkTikLnE0g=9rkkLw3Pk+UPo5HSYxCUg@mail.gmail.com
State New
Headers show

Commit Message

Lyu Mitnick April 10, 2011, 9:02 a.m. UTC 
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(-)

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

Comments

Stefan Hajnoczi April 11, 2011, 8:33 a.m. UTC | #1 
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
Lyu Mitnick April 13, 2011, 8:59 p.m. UTC | #2 
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
Kevin Wolf April 14, 2011, 8:37 a.m. UTC | #3 
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
Lyu Mitnick April 15, 2011, 8:40 p.m. UTC | #4 
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
Kevin Wolf April 18, 2011, 1:13 p.m. UTC | #5 
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
diff mbox

Patch

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);
 }