Message ID | BANLkTikLnE0g=9rkkLw3Pk+UPo5HSYxCUg@mail.gmail.com |
---|---|
State | New |
Headers | show |
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
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
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
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
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 --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); }