[Drbd-dev] user interface test of drbd.conf

[Drbd-dev] user interface test of drbd.conf

[Helmut Wollmersdorfer]

Hi,

I did some systematic test cases with the parameters and options against 
the man page.

As we do not have documents like Requirement Definition, Detailed 
Functional Specification, Interface Specification (same is true for most 
OSS/GPL projects), the documentation is the only source of "promised 
functionality".

Differences between docu and program behaviour mean wrong docu, wrong 
program, or both wrong.

Enough theory.

Here are some items:

Version 0.7.5
Method: drbdadm -d up all

1) Problem report
-----------------
docu quote:
| minor-count count
|   count may be a number from 1 to 255.

count 	expected-result	test-result
1 	valid   	passed
0 	invalid   	failed
-1 	invalid   	passed
255 	valid   	passed
256 	invalid   	failed
260 	invalid   	failed

Problem description: Parser accepts invalid values.
(assumed) generic requirement: Invalid values should be reported as 
error as near as possible to the interface. Invalid values should not 
enter a system and cause errors in later steps, or other components, or 
in worst case crash the system.

2) Problem report
-----------------
docu quote:
wfc-timeout
The sign is important.
Always use a negative value, positive will (try to) force
primary status, which is not what you want, if it  has  outdated
data.  Default is 0, which means unlimited. Unit is seconds.


100000 valid passed

Problem description: Very high, maybe senseless values are accepted.
 From my experience as tester I will not expect, that _any_ values will 
work correctly without "dry-run". Same is true for other values in 
drbd.conf.

BTW: An appropriate problem tracking system like bugzilla would be nice.

Helmut Wollmersdorfer

Re: [Drbd-dev] user interface test of drbd.conf

[Lars Ellenberg]

/ 2004-10-20 18:17:27 +0200
\ Helmut Wollmersdorfer:
> Version 0.7.5
> Method: drbdadm -d up all
> 
> 1) Problem report
> -----------------
> docu quote:
> | minor-count count
> |   count may be a number from 1 to 255.
> 
> count 	expected-result	test-result
> 1 	valid   	passed
> 0 	invalid   	failed
> -1 	invalid   	passed
> 255 	valid   	passed
> 256 	invalid   	failed
> 260 	invalid   	failed
> 
> Problem description: Parser accepts invalid values.
> (assumed) generic requirement: Invalid values should be reported as 
> error as near as possible to the interface. Invalid values should not 
> enter a system and cause errors in later steps, or other components, or 
> in worst case crash the system.

will fix that in drbdadm soonish.
though, the module does not load with invalid minor_count:

,- drbd/drbd_main.c
|	if (1 > minor_count||minor_count > 255) {
|		printk(KERN_ERR DEVICE_NAME
|			": invalid minor_count (%d)\n",minor_count);
|#ifdef MODULE
|		return -EINVAL;
|#else
|		minor_count = 8;
|#endif
|	}
`-

> 2) Problem report
> -----------------
> docu quote:
> wfc-timeout
> The sign is important.
> Always use a negative value, positive will (try to) force
> primary status, which is not what you want, if it  has  outdated
> data.  Default is 0, which means unlimited. Unit is seconds.

the current svn docu looks like this:

  wfc-timeout time
  Wait for connection timeout.
  The init script drbd(8) blocks the boot process
  until the DRBD resources are connected.
  This is so when the cluster manager starts later,
  it does not see a resource with internal split-brain.
  In case you want to limit the wait time, do it here.
  Default is 0, which means unlimited. Unit is seconds.
  
> 100000 valid passed
> 
> Problem description: Very high, maybe senseless values are accepted.

should we limit it to three days?

I don't get the next sentence:

> From my experience as tester I will not expect, that _any_ values will 
> work correctly without "dry-run".

hm?

> Same is true for other values in drbd.conf.


> BTW: An appropriate problem tracking system like bugzilla would be nice.

well, we have had a bugzilla, but we did not use it.
and it would be very fast very heavily out of date.
do you want to keep it up-to-date,
and kick Phillip or me every now and then?

	Lars Ellenberg

Re: [Drbd-dev] user interface test of drbd.conf

[Lars Ellenberg]

/ 2004-10-20 18:17:27 +0200
\ Helmut Wollmersdorfer:

> Problem description: senseless values are accepted.

[ complains about missing range checks in the drbd.conf parser ]


/ 2004-10-21 01:50:59 +0200
\ svn@svn.drbd.org:
> Author: lars
> Date: 2004-10-21 01:50:56 +0200 (Thu, 21 Oct 2004)
> New Revision: 1609
> 
> Modified:
>    branches/drbd-0.7/documentation/drbd.conf.sgml
>    branches/drbd-0.7/scripts/drbd.conf
>    branches/drbd-0.7/user/drbdadm_parser.y
> Log:
> added range_check to drbdadm_parser.y,
>  now helmut can add rangechecks where neccessary

now, if you come up with more range checks you think should be in there,
just go to drbdadm_parser.y, and insert a
  range_check(value, name, min, max)
line at the appropriate location, like this:

> Modified: branches/drbd-0.7/user/drbdadm_parser.y
> ===================================================================
> --- branches/drbd-0.7/user/drbdadm_parser.y	2004-10-20 13:28:36 UTC (rev 1608)
> +++ branches/drbd-0.7/user/drbdadm_parser.y	2004-10-20 23:50:56 UTC (rev 1609)
> @@ -149,9 +159,15 @@
>  glob_stmt:	  TK_DISABLE_IO_HINTS
>  			{ global_options.disable_io_hints=1;   }
>  		| TK_MINOR_COUNT TK_INTEGER
> -			{ global_options.minor_count=atoi($2); }
> -		| TK_DIALOG_REFRESH TK_INTEGER   
> -                        { global_options.dialog_refresh=atoi($2); }
> +		{
> +			global_options.minor_count=atoi($2);
> +			range_check(global_options.minor_count,"minor_count",1,255);
> +		}
> +		| TK_DIALOG_REFRESH TK_INTEGER
> +		{
> +			global_options.dialog_refresh=atoi($2);
> +			range_check(global_options.dialog_refresh,"dialog_refresh",0,600);
> +		}
>  		;

Re: [Drbd-dev] user interface test of drbd.conf

[Helmut Wollmersdorfer]

Lars Ellenberg wrote:
> / 2004-10-20 18:17:27 +0200
> \ Helmut Wollmersdorfer:

>>wfc-timeout
[...]
>>100000 valid passed

>>Problem description: Very high, maybe senseless values are accepted.

> should we limit it to three days?

Seems very high for a HA-System. On the other hand maybe someone wants 
the system to wait for a whole weekend.

3 days = 259200 seconds. I would like an easier to remember plain value 
like 100000, 200000 or 300000.

> I don't get the next sentence:

>>From my experience as tester I will not expect, that _any_ values will 
>>work correctly without "dry-run".

> hm?

Very easy explanation:
As we all know, usally numbers represented in text are converted to an 
easier to handle representation inside a program. This can be hex, 
decimal packed, floating etc. and mostly will be of fixed length. Even 
bignums will have an upper limit (e.g. 64Kdigits). If there is no check 
for a maximum value at input, funny things can happen in program logik 
afterwards: overruns, skip to negative values, truncated values, which 
possible could cause crazy program behaviour.

I do not aspect many of these errors, as the bug rate of drbd is very 
low (congratulations!) and at minor level.

> well, we have had a bugzilla, but we did not use it.
> and it would be very fast very heavily out of date.
> do you want to keep it up-to-date,
> and kick Phillip or me every now and then?

Yeah. But it needs a minimum of cooperation from developers. E.g. such 
messages like "solved, ready for retest", "won't solve", priorities etc.

Helmut Wollmersdorfer

Re: [Drbd-dev] user interface test of drbd.conf

[Lars Ellenberg]

/ 2004-10-21 14:36:16 +0200
\ Helmut Wollmersdorfer:
> Lars Ellenberg wrote:
> >/ 2004-10-20 18:17:27 +0200
> >\ Helmut Wollmersdorfer:
> 
> >>wfc-timeout
> [...]
> >>100000 valid passed
> 
> >>Problem description: Very high, maybe senseless values are accepted.
> 
> >should we limit it to three days?
> 
> Seems very high for a HA-System. On the other hand maybe someone wants 
> the system to wait for a whole weekend.
> 
> 3 days = 259200 seconds. I would like an easier to remember plain value 
> like 100000, 200000 or 300000.
> 
> >I don't get the next sentence:
> 
> >>From my experience as tester I will not expect, that _any_ values will 
> >>work correctly without "dry-run".
> 
> >hm?
> 
> Very easy explanation:
> As we all know, usally numbers represented in text are converted to an 
> easier to handle representation inside a program. This can be hex, 
> decimal packed, floating etc. and mostly will be of fixed length. Even 
> bignums will have an upper limit (e.g. 64Kdigits). If there is no check 
> for a maximum value at input, funny things can happen in program logik 
> afterwards: overruns, skip to negative values, truncated values, which 
> possible could cause crazy program behaviour.

but what has that to do with dry-run?

anyways, I think all "Numbers" in the drbd.conf file end up as int,
or unsigned int. at some places we actually convert them with atoi,
which does not detect errors.
so that should be fixed to use strtol.
and where we use strtol in drbdsetup.c, actually it is m_strtol(),
we should be more paranoid about not having an ERANGE, not exceeding 
the range of the target integer type when multiplying our "unit" into
it, and probably that the unit letter, if present, actually _is_ the
last character in that integer string.

and to improve the scanner, maybe limiting to 9 digits will do?

Index: user/drbdadm_scanner.fl
===================================================================
--- user/drbdadm_scanner.fl	(revision 1609)
+++ user/drbdadm_scanner.fl	(working copy)
@@ -57,8 +57,9 @@
 COMMENT			  \#[^\n]*
 WSC			  ({WS}|{COMMENT}\n)+
 ASSIGN			  {LS}
-NUM			  [0-9]+
-NUM_U			  [0-9]+[kmgKMG]? 
+OUT_OF_RANGE_NUM	  [0-9]{10,}[kmgKMG]? 
+NUM			  [0-9]{1,9}
+NUM_U			  [0-9]{1,9}[kmgKMG]? 
 NAME			  [/_.A-Za-z0-9-]+
 STRING			  ({NAME}|\"([^\"\\\n]*|\\.)*\")+
 USTRING			  \"([^\"\\\n]*|\\.)*
@@ -115,11 +116,19 @@
 
 <NUM>{
   {NUM}			yy_pop_state(); CP; return TK_INTEGER;
+  {OUT_OF_RANGE_NUM}	{
+			  PRINTF("%s: number too big\n", yytext);
+			  exit(E_syntax);
+			}
   {NDELIM}		expect_error("integer"); yy_pop_state();
 }
 
 <NUM_U>{
   {NUM_U}		yy_pop_state(); CP; return TK_INTEGER;
+  {OUT_OF_RANGE_NUM}	{
+			  PRINTF("%s: number too big\n", yytext);
+			  exit(E_syntax);
+			}
   {NDELIM}		expect_error("[0-9]+[KMG]?"); yy_pop_state();
 }
 

> >well, we have had a bugzilla, but we did not use it.
> >and it would be very fast very heavily out of date.
> >do you want to keep it up-to-date,
> >and kick Phillip or me every now and then?
> 
> Yeah. But it needs a minimum of cooperation from developers. E.g. such 
> messages like "solved, ready for retest", "won't solve", priorities etc.

actually, we still have it, it only no longer is linked from www.drbd.org
have a look at http://bugs.drbd.org

any comments by Philipp on this?

Re: [Drbd-dev] user interface test of drbd.conf

[Philipp Reisner]

> > >well, we have had a bugzilla, but we did not use it.
> > >and it would be very fast very heavily out of date.
> > >do you want to keep it up-to-date,
> > >and kick Phillip or me every now and then?
> >
> > Yeah. But it needs a minimum of cooperation from developers. E.g. such
> > messages like "solved, ready for retest", "won't solve", priorities etc.
>
> actually, we still have it, it only no longer is linked from www.drbd.org
> have a look at http://bugs.drbd.org
>
> any comments by Philipp on this?

I do not have any strong feelings about these things. If Helmut likes
to use bugs.drbd.org please do so. [If I remeber correctly everybody
can create open tasks there, but only the developers: Lars and me
can do things like assigning and closing etc...]

On the other hand I am also fine with simply mailing it here... on
drbd-dev.

As Lars pointed out, after filling bugs.drbd.org with interesting
issues it might be a good idea to trigger us (with a short post
to drbd-dev to actually process the open tasks in bugs.drbd.org)

So far DRBD was a too small project, that we actually use the 
bugtracking system.

-phil
-- 
: Dipl-Ing Philipp Reisner                      Tel +43-1-8178292-50 :
: LINBIT Information Technologies GmbH          Fax +43-1-8178292-82 :
: Schönbrunnerstr 244, 1120 Vienna, Austria    http://www.linbit.com :

Re: [Drbd-dev] user interface test of drbd.conf

[Helmut Wollmersdorfer]

Philipp Reisner wrote:

> I do not have any strong feelings about these things. If Helmut likes
> to use bugs.drbd.org please do so. [If I remeber correctly everybody
> can create open tasks there, but only the developers: Lars and me
> can do things like assigning and closing etc...]

AFAIR bugfly misses some minimum features like attaching larger files.

> On the other hand I am also fine with simply mailing it here... on
> drbd-dev.

> As Lars pointed out, after filling bugs.drbd.org with interesting
> issues it might be a good idea to trigger us (with a short post
> to drbd-dev to actually process the open tasks in bugs.drbd.org)

> So far DRBD was a too small project, that we actually use the 
> bugtracking system.

I would prefer mailing over bugfly. Mailing would be enough, as long as 
the item-rate per day/week does not exceed "dozens", and as long as the 
reaction time is within days (actually Lars reacts within hours in most 
cases :-))).

At least for simple patches which need no discussion, there is the 
direct way, and the comment in SVN is enough.

Helmut Wollmersdorfer

Re: [Drbd-dev] user interface test of drbd.conf

[Helmut Wollmersdorfer]

Lars Ellenberg wrote:
> / 2004-10-21 14:36:16 +0200
> \ Helmut Wollmersdorfer:
> 
>>Lars Ellenberg wrote:
>>
>>>/ 2004-10-20 18:17:27 +0200
>>>\ Helmut Wollmersdorfer:

>>>>From my experience as tester I will not expect, that _any_ values will 
>>>>work correctly without "dry-run".

>>>hm?

>>Very easy explanation:
>>As we all know, usally numbers represented in text are converted to an 
>>easier to handle representation inside a program. This can be hex, 
>>decimal packed, floating etc. and mostly will be of fixed length. Even 
>>bignums will have an upper limit (e.g. 64Kdigits). If there is no check 
>>for a maximum value at input, funny things can happen in program logik 
>>afterwards: overruns, skip to negative values, truncated values, which 
>>possible could cause crazy program behaviour.

> but what has that to do with dry-run?

If I only test with dry-run, I only test the parser. This cannot uncover 
errors in afterwards logic.

> and to improve the scanner, maybe limiting to 9 digits will do?
[...]
> +OUT_OF_RANGE_NUM	  [0-9]{10,}[kmgKMG]? 
> +NUM			  [0-9]{1,9}
> +NUM_U			  [0-9]{1,9}[kmgKMG]?
Let's try
999999999k ~ 1 T
999999999M ~ 1000 T
999999999G ~ 1000000 T

[...]
>  <NUM>{
>    {NUM}			yy_pop_state(); CP; return TK_INTEGER;
> +  {OUT_OF_RANGE_NUM}	{
> +			  PRINTF("%s: number too big\n", yytext);
Better:
+	PRINTF("%s: number exceeds limit of 9 digits\n", yytext);


Helmut Wollmersdorfer

Re: [Drbd-dev] user interface test of drbd.conf

[Lars Ellenberg]

/ 2004-10-21 17:56:14 +0200
\ Helmut Wollmersdorfer:
> >>>>From my experience as tester I will not expect, that _any_ values will 
> >>>>work correctly without "dry-run".
> 
> >>>hm?

> If I only test with dry-run, I only test the parser. This cannot uncover 
> errors in afterwards logic.
> 
> >and to improve the scanner, maybe limiting to 9 digits will do?
> [...]
> >+OUT_OF_RANGE_NUM	  [0-9]{10,}[kmgKMG]? 
> >+NUM			  [0-9]{1,9}
> >+NUM_U			  [0-9]{1,9}[kmgKMG]?
> Let's try
> 999999999k ~ 1 T
> 999999999M ~ 1000 T
> 999999999G ~ 1000000 T

well, of course there should be additional range_checks
in drbdadm_parser.y

but 9 digits do fit very well into a 32bit integer.
(the default return type of strtol on a 32bit arch).
then the overflow conditions need to be checked before
using the unit multiplier, as mentioned previously.

> [...]
> > <NUM>{
> >   {NUM}			yy_pop_state(); CP; return TK_INTEGER;
> >+  {OUT_OF_RANGE_NUM}	{
> >+			  PRINTF("%s: number too big\n", yytext);
> Better:
> +	PRINTF("%s: number exceeds limit of 9 digits\n", yytext);

yeah, whatever :)

I'm going to make up a decent patch somewhen this weekend, I guess.
though I'll be distracted by some major family meeting ...

	lge