Subject [ md PATCH 4/6] : md to support page size chunks in the case of raid 0

Subject [ md PATCH 4/6] : md to support page size chunks in the case of raid 0

[raz ben yehuda]

md to support page size chunks in the case of raid 0
Signed-off-by: raziebe@gmail.com
 md.c |   28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)
---
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 279007a..aab183e 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -443,9 +443,13 @@ static inline sector_t calc_dev_sboffset(struct block_device *bdev)
 static sector_t calc_num_sectors(mdk_rdev_t *rdev, unsigned chunk_size)
 {
 	sector_t num_sectors = rdev->sb_start;
+	if (chunk_size) {
+		sector_t chunk_sects = chunk_size>>9;
+		sector_t x = num_sectors;
+		sector_div(x, chunk_sects);
+		num_sectors = x*chunk_sects;
+	}
 
-	if (chunk_size)
-		num_sectors &= ~((sector_t)chunk_size/512 - 1);
 	return num_sectors;
 }
 
@@ -3518,11 +3522,11 @@ min_sync_store(mddev_t *mddev, const char *buf, size_t len)
 
 	/* Must be a multiple of chunk_size */
 	if (mddev->chunk_size) {
-		if (min & (sector_t)((mddev->chunk_size>>9)-1))
+		unsigned long long temp = min;
+		if (sector_div(temp, (mddev->chunk_size>>9)))
 			return -EINVAL;
 	}
 	mddev->resync_min = min;
-
 	return len;
 }
 
@@ -3555,7 +3559,8 @@ max_sync_store(mddev_t *mddev, const char *buf, size_t len)
 
 		/* Must be a multiple of chunk_size */
 		if (mddev->chunk_size) {
-			if (max & (sector_t)((mddev->chunk_size>>9)-1))
+			unsigned long long temp = max;
+			if (sector_div(temp, (mddev->chunk_size>>9)))
 				return -EINVAL;
 		}
 		mddev->resync_max = max;
@@ -3996,14 +4001,23 @@ static int do_md_run(mddev_t * mddev)
 				chunk_size, MAX_CHUNK_SIZE);
 			return -EINVAL;
 		}
+
 		/*
 		 * chunk-size has to be a power of 2
 		 */
-		if ( (1 << ffz(~chunk_size)) != chunk_size) {
+		if ((1 << ffz(~chunk_size)) != chunk_size &&
+			 mddev->level != 0) {
 			printk(KERN_ERR "chunk_size of %d not valid\n", chunk_size);
 			return -EINVAL;
 		}
-
+		/*
+		* raid0 chunk size has to divide by a page
+		*/
+		if (mddev->level == 0 && (chunk_size % PAGE_SIZE)) {
+			printk(KERN_ERR "chunk_size of %d not valid\n",
+				chunk_size);
+			return -EINVAL;
+		}
 		/* devices must have minimum size of one chunk */
 		list_for_each_entry(rdev, &mddev->disks, same_set) {
 			if (test_bit(Faulty, &rdev->flags))

Re: Subject [ md PATCH 4/6] : md to support page size chunks in the case of raid 0

[Neil Brown]

On Tuesday May 19, raziebe@gmail.com wrote:
> md to support page size chunks in the case of raid 0
> Signed-off-by: raziebe@gmail.com
>  md.c |   28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
> ---
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 279007a..aab183e 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -443,9 +443,13 @@ static inline sector_t calc_dev_sboffset(struct block_device *bdev)
>  static sector_t calc_num_sectors(mdk_rdev_t *rdev, unsigned chunk_size)
>  {
>  	sector_t num_sectors = rdev->sb_start;
> +	if (chunk_size) {
> +		sector_t chunk_sects = chunk_size>>9;
> +		sector_t x = num_sectors;
> +		sector_div(x, chunk_sects);
> +		num_sectors = x*chunk_sects;
> +	}
>  
> -	if (chunk_size)
> -		num_sectors &= ~((sector_t)chunk_size/512 - 1);
>  	return num_sectors;
>  }

That's OK.... though you have removed the blank line separating the
variable declarations from the code.  I like to keep that blank line
there.
And you have added a blank line before the "return", which I only
mention because.....

>  
> @@ -3518,11 +3522,11 @@ min_sync_store(mddev_t *mddev, const char *buf, size_t len)
>  
>  	/* Must be a multiple of chunk_size */
>  	if (mddev->chunk_size) {
> -		if (min & (sector_t)((mddev->chunk_size>>9)-1))
> +		unsigned long long temp = min;
> +		if (sector_div(temp, (mddev->chunk_size>>9)))
>  			return -EINVAL;
>  	}
>  	mddev->resync_min = min;
> -
>  	return len;
>  }

You have removed the blank line before the return here ??? consistency
is a good thing.

And 'temp' should be 'sector_t'.  'sector_div' requires a 'sector_t'
for the first argument.


>  
> @@ -3555,7 +3559,8 @@ max_sync_store(mddev_t *mddev, const char *buf, size_t len)
>  
>  		/* Must be a multiple of chunk_size */
>  		if (mddev->chunk_size) {
> -			if (max & (sector_t)((mddev->chunk_size>>9)-1))
> +			unsigned long long temp = max;
> +			if (sector_div(temp, (mddev->chunk_size>>9)))
>  				return -EINVAL;

Again, temp must be sector_t.

>  		}
>  		mddev->resync_max = max;
> @@ -3996,14 +4001,23 @@ static int do_md_run(mddev_t * mddev)
>  				chunk_size, MAX_CHUNK_SIZE);
>  			return -EINVAL;
>  		}
> +
>  		/*
>  		 * chunk-size has to be a power of 2
>  		 */
> -		if ( (1 << ffz(~chunk_size)) != chunk_size) {
> +		if ((1 << ffz(~chunk_size)) != chunk_size &&
> +			 mddev->level != 0) {
>  			printk(KERN_ERR "chunk_size of %d not valid\n", chunk_size);
>  			return -EINVAL;
>  		}

I wold really like to remove any knowledge about specific raid levels
from the common (md.c) code and keep it all in the personality modules
(is that a job for you Andre ??).
So I definitely don't want to add a test for ->level here.

So I would like to see the tests for chunk_size removed do_md_run and
included in each personalities ->run function.  This would be a series
of patches that adds the checks in ->run where needed, then removes it
from md.c.  Would you like to do that?

> -
> +		/*
> +		* raid0 chunk size has to divide by a page
> +		*/
> +		if (mddev->level == 0 && (chunk_size % PAGE_SIZE)) {
> +			printk(KERN_ERR "chunk_size of %d not valid\n",
> +				chunk_size);
> +			return -EINVAL;
> +		}

Why should the chunk_size be a multiple of PAGE_SIZE?
I suspect it should be a multiple of hardsect_size for each component
device (which, thanks to blk_queue_stack_limits, we can check by just
checking the hardsect_size of the mddev device after all the calls to
blk_queue_stack_limits in create_strip_zones, or in raid0_run.
And again, these checks need to move to raid0.c


Thanks,
NeilBrown

Re: Subject [ md PATCH 4/6] : md to support page size chunks in the case of raid 0

[raz ben yehuda]

On Tue, 2009-05-19 at 10:47 +1000, Neil Brown wrote:
> On Tuesday May 19, raziebe@gmail.com wrote:
> > md to support page size chunks in the case of raid 0
> > Signed-off-by: raziebe@gmail.com
> >  md.c |   28 +++++++++++++++++++++-------
> >  1 file changed, 21 insertions(+), 7 deletions(-)
> > ---
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index 279007a..aab183e 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -443,9 +443,13 @@ static inline sector_t calc_dev_sboffset(struct block_device *bdev)
> >  static sector_t calc_num_sectors(mdk_rdev_t *rdev, unsigned chunk_size)
> >  {
> >  	sector_t num_sectors = rdev->sb_start;
> > +	if (chunk_size) {
> > +		sector_t chunk_sects = chunk_size>>9;
> > +		sector_t x = num_sectors;
> > +		sector_div(x, chunk_sects);
> > +		num_sectors = x*chunk_sects;
> > +	}
> >  
> > -	if (chunk_size)
> > -		num_sectors &= ~((sector_t)chunk_size/512 - 1);
> >  	return num_sectors;
> >  }
> 
> That's OK.... though you have removed the blank line separating the
> variable declarations from the code.  I like to keep that blank line
> there.
> And you have added a blank line before the "return", which I only
> mention because.....
> 
> >  
> > @@ -3518,11 +3522,11 @@ min_sync_store(mddev_t *mddev, const char *buf, size_t len)
> >  
> >  	/* Must be a multiple of chunk_size */
> >  	if (mddev->chunk_size) {
> > -		if (min & (sector_t)((mddev->chunk_size>>9)-1))
> > +		unsigned long long temp = min;
> > +		if (sector_div(temp, (mddev->chunk_size>>9)))
> >  			return -EINVAL;
> >  	}
> >  	mddev->resync_min = min;
> > -
> >  	return len;
> >  }
> 
> You have removed the blank line before the return here ??? consistency
> is a good thing.
> 
> And 'temp' should be 'sector_t'.  'sector_div' requires a 'sector_t'
> for the first argument.
> 
> 
> >  
> > @@ -3555,7 +3559,8 @@ max_sync_store(mddev_t *mddev, const char *buf, size_t len)
> >  
> >  		/* Must be a multiple of chunk_size */
> >  		if (mddev->chunk_size) {
> > -			if (max & (sector_t)((mddev->chunk_size>>9)-1))
> > +			unsigned long long temp = max;
> > +			if (sector_div(temp, (mddev->chunk_size>>9)))
> >  				return -EINVAL;
> 
> Again, temp must be sector_t.
> 
> >  		}
> >  		mddev->resync_max = max;
> > @@ -3996,14 +4001,23 @@ static int do_md_run(mddev_t * mddev)
> >  				chunk_size, MAX_CHUNK_SIZE);
> >  			return -EINVAL;
> >  		}
> > +
> >  		/*
> >  		 * chunk-size has to be a power of 2
> >  		 */
> > -		if ( (1 << ffz(~chunk_size)) != chunk_size) {
> > +		if ((1 << ffz(~chunk_size)) != chunk_size &&
> > +			 mddev->level != 0) {
> >  			printk(KERN_ERR "chunk_size of %d not valid\n", chunk_size);
> >  			return -EINVAL;
> >  		}
> 
> I wold really like to remove any knowledge about specific raid levels
> from the common (md.c) code and keep it all in the personality modules
> (is that a job for you Andre ??).
> So I definitely don't want to add a test for ->level here.
> 
> So I would like to see the tests for chunk_size removed do_md_run and
> included in each personalities ->run function.  This would be a series
> of patches that adds the checks in ->run where needed, then removes it
> from md.c.  Would you like to do that?
yes. As i understand , patch will not be accepted without it. 
> 
> > -
> > +		/*
> > +		* raid0 chunk size has to divide by a page
> > +		*/
> > +		if (mddev->level == 0 && (chunk_size % PAGE_SIZE)) {
> > +			printk(KERN_ERR "chunk_size of %d not valid\n",
> > +				chunk_size);
> > +			return -EINVAL;
> > +		}
> 
> Why should the chunk_size be a multiple of PAGE_SIZE?
> I suspect it should be a multiple of hardsect_size for each component
> device (which, thanks to blk_queue_stack_limits, we can check by just
> checking the hardsect_size of the mddev device after all the calls to
> blk_queue_stack_limits in create_strip_zones, or in raid0_run.
> And again, these checks need to move to raid0.c
i can. but it means we have to fix mdadm to get sectors and not
kilobytes. if you want, i can do it **after the reshape**. 
> 
> 
> Thanks,
> NeilBrown

Re: Subject [ md PATCH 4/6] : md to support page size chunks in the case of raid 0

[Andre Noll]

[-- Attachment #1: Type: text/plain, Size: 1983 bytes --]

On Tue, May 19, 2009 at 10:47:56AM +1000, Neil Brown wrote:

> And 'temp' should be 'sector_t'.  'sector_div' requires a 'sector_t'
> for the first argument.

...

> Again, temp must be sector_t.

How about rolling our own md_sector_div() which at least checks for
such bugs via the

        (void)(((typeof((temp)) *)0) == ((sector_t *)0))

trick?

> > @@ -3996,14 +4001,23 @@ static int do_md_run(mddev_t * mddev)
> >  				chunk_size, MAX_CHUNK_SIZE);
> >  			return -EINVAL;
> >  		}
> > +
> >  		/*
> >  		 * chunk-size has to be a power of 2
> >  		 */
> > -		if ( (1 << ffz(~chunk_size)) != chunk_size) {
> > +		if ((1 << ffz(~chunk_size)) != chunk_size &&
> > +			 mddev->level != 0) {
> >  			printk(KERN_ERR "chunk_size of %d not valid\n", chunk_size);
> >  			return -EINVAL;
> >  		}
> 
> I wold really like to remove any knowledge about specific raid levels
> from the common (md.c) code and keep it all in the personality modules
> (is that a job for you Andre ??).
> So I definitely don't want to add a test for ->level here.
> 
> So I would like to see the tests for chunk_size removed do_md_run and
> included in each personalities ->run function.  This would be a series
> of patches that adds the checks in ->run where needed, then removes it
> from md.c.  Would you like to do that?

Sure, I can give it a try. Though I'm not sure I fully understand
what would be the difference between the checks in the individual
->run functions. Currently, in do_md_run() we check that

        * chunk_size <= MAX_CHUNK_SIZE
        * chunk_size is a power of two
        * the rdev is at least one chunk large

None of these checks depend on the raid level, so the above change
that allows chunk sizes which are not a power of two for raid0 would
be the only difference. Are you anticipating that the requirements
of the various raid levels with respect to chunk size will further
diverge in the future?

Thanks
Andre

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Subject [ md PATCH 4/6] : md to support page size chunks in the case of raid 0

[Neil Brown]

On Wednesday May 20, maan@systemlinux.org wrote:
> On Tue, May 19, 2009 at 10:47:56AM +1000, Neil Brown wrote:
> 
> > And 'temp' should be 'sector_t'.  'sector_div' requires a 'sector_t'
> > for the first argument.
> 
> ...
> 
> > Again, temp must be sector_t.
> 
> How about rolling our own md_sector_div() which at least checks for
> such bugs via the
> 
>         (void)(((typeof((temp)) *)0) == ((sector_t *)0))
> 
> trick?

Interesting idea..
You would still need to eyeball that code to make sure md_sector_div
was used instead of sector_div, though I guess that is more obvious..
Maybe sector_div should have that test in it globally...
Though I've actually hit more problems with the second arg not being
"unsigned long" like it should be.

> 
> > > @@ -3996,14 +4001,23 @@ static int do_md_run(mddev_t * mddev)
> > >  				chunk_size, MAX_CHUNK_SIZE);
> > >  			return -EINVAL;
> > >  		}
> > > +
> > >  		/*
> > >  		 * chunk-size has to be a power of 2
> > >  		 */
> > > -		if ( (1 << ffz(~chunk_size)) != chunk_size) {
> > > +		if ((1 << ffz(~chunk_size)) != chunk_size &&
> > > +			 mddev->level != 0) {
> > >  			printk(KERN_ERR "chunk_size of %d not valid\n", chunk_size);
> > >  			return -EINVAL;
> > >  		}
> > 
> > I wold really like to remove any knowledge about specific raid levels
> > from the common (md.c) code and keep it all in the personality modules
> > (is that a job for you Andre ??).
> > So I definitely don't want to add a test for ->level here.
> > 
> > So I would like to see the tests for chunk_size removed do_md_run and
> > included in each personalities ->run function.  This would be a series
> > of patches that adds the checks in ->run where needed, then removes it
> > from md.c.  Would you like to do that?
> 
> Sure, I can give it a try. Though I'm not sure I fully understand
> what would be the difference between the checks in the individual
> ->run functions. Currently, in do_md_run() we check that
> 
>         * chunk_size <= MAX_CHUNK_SIZE
>         * chunk_size is a power of two
>         * the rdev is at least one chunk large
> 
> None of these checks depend on the raid level, so the above change
> that allows chunk sizes which are not a power of two for raid0 would
> be the only difference. Are you anticipating that the requirements
> of the various raid levels with respect to chunk size will further
> diverge in the future?

The second paragraph is already done (in for-next).  md.c doesn't
check chunksize any more, that is only checked in personality modules
that care.

What I was suggesting for you is the previous paragraph.  Removing all
knowledge about specific raid levels from md.c.
So where ever md.c tests the value for mddev->level, ask the question:
how can this information be extracted from the module rather than
having this hard-coded test.

Probably the easiest would be moving


	if (mddev->recovery_cp != MaxSector &&
	    mddev->level >= 1)
		printk(KERN_ERR "md: %s: raid array is not clean"
		       " -- starting background reconstruction\n",
		       mdname(mddev));

into various ->run methods.
Probably the hardest would be all the messing with LEVEL_MULTIPATH.
Somewhere in between would be the tests for which levels support
bitmaps.

In general, we just want to delay the test until we are in personality
code and do it there.

Does that make sense?

Thanks,
NeilBrown

Re: Subject [ md PATCH 4/6] : md to support page size chunks in the case of raid 0

[raz ben yehuda]

On Wed, 2009-05-20 at 20:17 +1000, Neil Brown wrote:
> On Wednesday May 20, maan@systemlinux.org wrote:
> > On Tue, May 19, 2009 at 10:47:56AM +1000, Neil Brown wrote:
> > 
> > > And 'temp' should be 'sector_t'.  'sector_div' requires a 'sector_t'
> > > for the first argument.
> > 
> > ...
> > 
> > > Again, temp must be sector_t.
> > 
> > How about rolling our own md_sector_div() which at least checks for
> > such bugs via the
> > 
> >         (void)(((typeof((temp)) *)0) == ((sector_t *)0))
> > 
> > trick?
nice. did not know typeof is available in c. 
> Interesting idea..
> You would still need to eyeball that code to make sure md_sector_div
> was used instead of sector_div, though I guess that is more obvious..
> Maybe sector_div should have that test in it globally...
> Though I've actually hit more problems with the second arg not being
> "unsigned long" like it should be.
> 
> > 
> > > > @@ -3996,14 +4001,23 @@ static int do_md_run(mddev_t * mddev)
> > > >  				chunk_size, MAX_CHUNK_SIZE);
> > > >  			return -EINVAL;
> > > >  		}
> > > > +
> > > >  		/*
> > > >  		 * chunk-size has to be a power of 2
> > > >  		 */
> > > > -		if ( (1 << ffz(~chunk_size)) != chunk_size) {
> > > > +		if ((1 << ffz(~chunk_size)) != chunk_size &&
> > > > +			 mddev->level != 0) {
> > > >  			printk(KERN_ERR "chunk_size of %d not valid\n", chunk_size);
> > > >  			return -EINVAL;
> > > >  		}
> > > 
> > > I wold really like to remove any knowledge about specific raid levels
> > > from the common (md.c) code and keep it all in the personality modules
> > > (is that a job for you Andre ??).
> > > So I definitely don't want to add a test for ->level here.
> > > 
> > > So I would like to see the tests for chunk_size removed do_md_run and
> > > included in each personalities ->run function.  This would be a series
> > > of patches that adds the checks in ->run where needed, then removes it
> > > from md.c.  Would you like to do that?
> > 
> > Sure, I can give it a try. Though I'm not sure I fully understand
> > what would be the difference between the checks in the individual
> > ->run functions. Currently, in do_md_run() we check that
> > 
> >         * chunk_size <= MAX_CHUNK_SIZE
> >         * chunk_size is a power of two
> >         * the rdev is at least one chunk large
> > 
> > None of these checks depend on the raid level, so the above change
> > that allows chunk sizes which are not a power of two for raid0 would
> > be the only difference. Are you anticipating that the requirements
> > of the various raid levels with respect to chunk size will further
> > diverge in the future?
> 
> The second paragraph is already done (in for-next).  md.c doesn't
> check chunksize any more, that is only checked in personality modules
> that care.
> 
> What I was suggesting for you is the previous paragraph.  Removing all
> knowledge about specific raid levels from md.c.
> So where ever md.c tests the value for mddev->level, ask the question:
> how can this information be extracted from the module rather than
> having this hard-coded test.
> 
> Probably the easiest would be moving
> 
> 
> 	if (mddev->recovery_cp != MaxSector &&
> 	    mddev->level >= 1)
> 		printk(KERN_ERR "md: %s: raid array is not clean"
> 		       " -- starting background reconstruction\n",
> 		       mdname(mddev));
> 
> into various ->run methods.
> Probably the hardest would be all the messing with LEVEL_MULTIPATH.
> Somewhere in between would be the tests for which levels support
> bitmaps.
> 
> In general, we just want to delay the test until we are in personality
> code and do it there.
why not add a new method ? pers->validate_parms ?  
> Does that make sense?
> 
> Thanks,
> NeilBrown
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Subject [ md PATCH 4/6] : md to support page size chunks in the case of raid 0

[Andre Noll]

[-- Attachment #1: Type: text/plain, Size: 3318 bytes --]

On 20:17, Neil Brown wrote:

> > How about rolling our own md_sector_div() which at least checks for
> > such bugs via the
> > 
> >         (void)(((typeof((temp)) *)0) == ((sector_t *)0))
> > 
> > trick?
> 
> Interesting idea..
> You would still need to eyeball that code to make sure md_sector_div
> was used instead of sector_div, though I guess that is more obvious..

We could change all sector_div()s to md_sector_div() in one patch
and refuse to take patches that introduce new uses of sector_div().

> Maybe sector_div should have that test in it globally...

Actually such tests are already in the sector_div version in
asm-generic/div64.c, but that gets only used if CONFIG_LBD is set. If
it is unset, no checks are performed at all. So yes, I believe
it makes sense to add these checks there as well. As Jens (CC'ed)
is the person who touched this code most recently (back in 2007),
he can probably tell best if such a change would be appreciated.

> Though I've actually hit more problems with the second arg not being
> "unsigned long" like it should be.

At I understand it, the second arg should be u32, but it's also OK
to call sector_div() with a 64-bit second arg as long as the _value_
of this arg fits into a u32. As this condition can only be checked at
runtime and with a performance penalty, it might be worth to fix up
all md users of sector_div() which use something different than u32
as the second arg. An md_sector_div() with additional checks could
help to find all such places. And the usual "this must be sector_t"
review comment would then become "please use md_sector_div()" :-)

> What I was suggesting for you is the previous paragraph.  Removing all
> knowledge about specific raid levels from md.c.
> So where ever md.c tests the value for mddev->level, ask the question:
> how can this information be extracted from the module rather than
> having this hard-coded test.
> 
> Probably the easiest would be moving
> 
> 
> 	if (mddev->recovery_cp != MaxSector &&
> 	    mddev->level >= 1)
> 		printk(KERN_ERR "md: %s: raid array is not clean"
> 		       " -- starting background reconstruction\n",
> 		       mdname(mddev));
> 
> into various ->run methods.
> Probably the hardest would be all the messing with LEVEL_MULTIPATH.
> Somewhere in between would be the tests for which levels support
> bitmaps.
> 
> In general, we just want to delay the test until we are in personality
> code and do it there.
> 
> Does that make sense?

Yes, it does, and I will have a look at it. One immediate problem I
can see is that the straight-forward pushdown approach will add quite
some code duplication. For example the above check for MaxSector and
the prink would have to be duplicated to each ->run function except
raid0, linear and multipath.

Adding a common check_reconstruction() function to md.c which is called
from the personalities that care would avoid the duplication but looks
a bit like over-engineering in this particular case. However, it might
be the right approach for more complicated "semi-personality-dependent"
code.

I suppose it has to be decided on a per-case basis which approach fits
best.

Thanks
Andre
-- 
The only person who always got his work done by Friday was Robinson Crusoe

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Subject [ md PATCH 4/6] : md to support page size chunks in the case of raid 0

[Neil Brown]

On Wednesday May 20, raziebe@gmail.com wrote:
> > 
> > In general, we just want to delay the test until we are in personality
> > code and do it there.
> why not add a new method ? pers->validate_parms ?  

because a new method should not be necessary.  We should be able to
manage with what we have - or even fewer...

NeilBrown