On Wed, May 22, 2013 at 12:27 PM, Bernardo Dal Seno <[email protected]>wrote:
> On 22 May 2013 10:28, Thomas Thrainer <[email protected]> wrote: > > > > > > > > On Mon, May 20, 2013 at 5:11 PM, Bernardo Dal Seno <[email protected]> > > wrote: > >> > >> The option is parsed but ignored, for the moment. > >> > >> Signed-off-by: Bernardo Dal Seno <[email protected]> > >> --- > >> lib/cli.py | 3 +++ > >> lib/cmdlib/instance_storage.py | 1 + > >> lib/constants.py | 2 ++ > >> man/gnt-instance.rst | 13 +++++++++---- > >> 4 files changed, 15 insertions(+), 4 deletions(-) > >> > >> diff --git a/lib/cli.py b/lib/cli.py > >> index 32ef069..0bddf43 100644 > >> --- a/lib/cli.py > >> +++ b/lib/cli.py > >> @@ -2647,6 +2647,9 @@ def GenericInstanceCreate(mode, opts, args): > >> raise errors.OpPrereqError("Invalid disk size for disk %d: > %s" > >> % > >> (didx, err), errors.ECODE_INVAL) > >> elif constants.IDISK_ADOPT in ddict: > >> + if constants.IDISK_SPINDLES in ddict: > >> + raise errors.OpPrereqError("spindles is not a valid option > >> when" > >> + " adopting a disk", > >> errors.ECODE_INVAL) > > > > > > That a bit strange to read, as the if goes like > > if IDISK_SIZE > > if ADOPT > > error > > ... do stuff ... > > elif ADOPT > > if SPINDLES > > error > > ... do stuff ... > > > > wouldn't it be more readable to do > > > > if ADOPT > > if IDISK_SIZE > > error > > if SPINDLES > > error > > ... do stuff ... > > elif IDISK_SIZE > > ... do stuff ... > > > > It's just that the same condition (IDISK_ADOPT) is checked twice, in two > > different levels, which makes it a bit hard to follow. > > Actually the two look very similar to me: There are four IFs, two > nesting levels, and one condition checked (twice) at different levels. > So I think it's more a matter of a personal taste. Btw, this is not > really related to my patch, so we need a separate patch if we want to > rewrite this. > >> If only a subset should be recreated, any number of ``disk`` options > can > >> be specified. It expects a disk index and an optional list of disk > >> -parameters to change. Only ``size`` and ``mode`` can be changed while > >> +parameters to change. Only ``size``, ``spindles``, and ``mode`` can be > >> +changed while > > > > > > Why not fill the line? > > I did it to simplify the work of the reviewer, but you're right, the > man page will last much longer than the review. Interdiff: > > diff --git a/man/gnt-instance.rst b/man/gnt-instance.rst > index 5e7cb25..0942c0b 100644 > --- a/man/gnt-instance.rst > +++ b/man/gnt-instance.rst > @@ -1107,15 +1107,14 @@ The ``--disk add:size=*SIZE*,[options..]`` option > adds a > instance, and ``--disk *N*:add:size=*SIZE*,[options..]`` will add a disk > to the the instance at a specific index. The available options are the > same as in the **add** command(``spindles``, ``mode``, ``name``, ``vg``, > -``metavg``). > -When adding an ExtStorage disk the ``provider=*PROVIDER*`` option is > -also mandatory and specifies the ExtStorage provider. Also, for > -ExtStorage disks arbitrary parameters can be passed as additional comma > -separated options, same as in the **add** command. -The ``--disk remove`` > -option will remove the last disk of the instance. Use > -``--disk `` *ID*``:remove`` to remove a disk by its identifier. *ID* > -can be the index of the disk, the disks's name or the disks's UUID. The > -``--disk *ID*:modify[,options...]`` will change the options of the disk. > +``metavg``). When adding an ExtStorage disk the ``provider=*PROVIDER*`` > +option is also mandatory and specifies the ExtStorage provider. Also, > +for ExtStorage disks arbitrary parameters can be passed as additional > +comma separated options, same as in the **add** command. -The ``--disk > +remove`` option will remove the last disk of the instance. Use ``--disk > +`` *ID*``:remove`` to remove a disk by its identifier. *ID* can be the > +index of the disk, the disks's name or the disks's UUID. The ``--disk > +*ID*:modify[,options...]`` will change the options of the disk. > Available options are: > > mode > @@ -1632,10 +1631,9 @@ normal operation and as such the impact of this is > low. > If only a subset should be recreated, any number of ``disk`` options can > be specified. It expects a disk index and an optional list of disk > parameters to change. Only ``size``, ``spindles``, and ``mode`` can be > -changed while > -recreating disks. To recreate all disks while changing parameters on > -a subset only, a ``--disk`` option must be given for every disk of the > -instance. > +changed while recreating disks. To recreate all disks while changing > +parameters on a subset only, a ``--disk`` option must be given for every > +disk of the instance. > > Optionally the instance's disks can be recreated on different > nodes. This can be useful if, for example, the original nodes of the > > > Thanks, > Bernardo > LGTM, Thanks. -- Thomas Thrainer | Software Engineer | [email protected] | Google Germany GmbH Dienerstr. 12 80331 München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Katherine Stephens
