Patch to allow gmirror to set priority of a disk

[Mel Flynn]

Hi Pawel,

I'll post-process in the future. I'm so used to if( cond ), cause it allows me 
to more easily spot the outer bounds of the condition, it's hard to conform to 
style(9) for me. :)

On Saturday 05 September 2009 11:10:30 Pawel Jakub Dawidek wrote:
> On Fri, Sep 04, 2009 at 10:16:34AM +0200, Mel Flynn wrote:
> > On Thursday 03 September 2009 15:57:41 Pawel Jakub Dawidek wrote:
> > > On Thu, Sep 03, 2009 at 03:48:37PM +0200, Mel Flynn wrote:
> > > > On Thursday 03 September 2009 14:44:07 Pawel Jakub Dawidek wrote:
> > > > > I'd suggest doing this not as separate gmirror(8) subcommand, but
> > > > > as an extension to 'configure' subcommand, where one can provide
> > > > > priority by giving '-p' argument.
> > > >
> > > > Except I didn't see how configure was implemented. Am I correct that
> > > > this is g_mirror_ctl_configure in sys/geom/mirror/g_mirror_ctl.c?
> > >
> > > Yes, you are correct.
> >
> > Ok, new patch. I gathered from configure_slice, that my assumption was
> > about flag handling is correct. Patch is only compile tested.
>
> In general the patch looks good, although I haven't tested it yet. I do
> have some comments though.
>
> > Index: sbin/geom/class/mirror/geom_mirror.c
> > ===================================================================
> > --- sbin/geom/class/mirror/geom_mirror.c	(revision 196803)
> > +++ sbin/geom/class/mirror/geom_mirror.c	(working copy)
> > @@ -47,7 +47,7 @@
> >
> >  static char label_balance[] = "split", configure_balance[] = "none";
> >  static intmax_t label_slice = 4096, configure_slice = -1;
> > -static intmax_t insert_priority = 0;
> > +static intmax_t insert_priority = 0, configure_priority = -1;
> >
> >  static void mirror_main(struct gctl_req *req, unsigned flags);
> >  static void mirror_activate(struct gctl_req *req);
> > @@ -71,10 +71,11 @@
> >  		{ 'F', "nofailsync", NULL, G_TYPE_BOOL },
> >  		{ 'h', "hardcode", NULL, G_TYPE_BOOL },
> >  		{ 'n', "noautosync", NULL, G_TYPE_BOOL },
> > +		{ 'p', "priority", &configure_priority, G_TYPE_NUMBER },
> >  		{ 's', "slice", &configure_slice, G_TYPE_NUMBER },
> >  		G_OPT_SENTINEL
> >  	    },
> > -	    NULL, "[-adfFhnv] [-b balance] [-s slice] name"
> > +	    NULL, "[-adfFhnv] [-b balance] [-s slice]  [-p prio] name [prov]"
>
> Style, extra space before '[-p prio]'.
>
> I also wonder if something like this should be possible:
>
> 	# gmirror configure -b round-robin -p 1 gm0 da0
>
> In you current implementation it will change balance algorithm for all
> the providers and in addition priority of da0 provider. This doesn't
> look clear for my taste (will balance algorithm be changed on da0 only
> or on all providers?).

Right, I found it convenient, but now that I think it about it, it's confusing 
from user perspective.

> I'd prefer:
>
> 	gmirror configure [-adfFhnv] [-b balance] [-s slice] name
> 	gmirror configure -p prio name prov
>
> Although I'm not sure if the infrastructure can actually print two usage
> patterns.

I tried two g_command structs for the same command, like so:
@@ -76,6 +76,13 @@
            },
            NULL, "[-adfFhnv] [-b balance] [-s slice] name"
        },
+       { "configure", G_FLAG_VERBOSE, NULL,
+           {
+               { 'p', "priority", &configure_priority, G_TYPE_NUMBER },
+               G_OPT_SENTINEL
+           },
+           NULL, "-p prio name prov"
+       },
        { "deactivate", G_FLAG_VERBOSE, NULL, G_NULL_OPTS, NULL,
            "[-v] name prov ..."
        },

But it errors if -p is given with illegal option, so either g_command needs to 
grow a second usage string, we need to do something like below, or we care 
less about usage and more about the man page.

@@ -71,10 +71,12 @@
                { 'F', "nofailsync", NULL, G_TYPE_BOOL },
                { 'h', "hardcode", NULL, G_TYPE_BOOL },
                { 'n', "noautosync", NULL, G_TYPE_BOOL },
+               { 'p', "priority", &configure_priority, G_TYPE_NUMBER },
                { 's', "slice", &configure_slice, G_TYPE_NUMBER },
                G_OPT_SENTINEL
            },
-           NULL, "[-adfFhnv] [-b balance] [-s slice] name"
+           NULL, "[-adfFhnv] [-b balance] [-s slice] name\n"
+                 "       gmirror configure -p priority name prov"
        },
        { "deactivate", G_FLAG_VERBOSE, NULL, G_NULL_OPTS, NULL,
            "[-v] name prov ..."

Fixing style issues now and reimplementing.
-- 
Mel