Btrfs devel - Oct 2010 - IOCTL #21 part two: btrfs progs patch, including iso 8601 timeout support

the ISO 8601 duration support is very loose, but I believe it is
accurate for valid
input. Without any non-numeric designators, the timeout is interpreted
as seconds,
so
    btrfs fi reclaim 10.3321 /my_btrfs_mount ||  echo timed out
will wait 10332 ms before echoing, if the pending subvolume deletions
take longer than that.

Timeout defaults to 0, and path defaults to current directory.

On Wednesday, 06 October, 2010, David Nicol wrote:
> the ISO 8601 duration support is very loose, but I believe it is
> accurate for valid
> input. Without any non-numeric designators, the timeout is interpreted
> as seconds,
> so
>     btrfs fi reclaim 10.3321 /my_btrfs_mount ||  echo timed out
> will wait 10332 ms before echoing, if the pending subvolume deletions
> take longer than that.
> 
> Timeout defaults to 0, and path defaults to current directory.
> 
Please the next time put your patch inline or it is more difficult to 
highlight a suggestion.

Any way:

1)
[...]
+	{ do_wait4clean, 1002, /* require at most two args */
+	  "filesystem reclaim", "<path> [timeout]\n"
+		"Wait for cleanup of deleted subvolumes in the filesystem 
<path>.\n"
+		"Optional timeout in whole or partial seconds, or ISO8601 
string.\n"
+	},
[...]
+int do_wait4clean(int argc, char **argv)
+{
+	int fd, res;
+        struct btrfs_ioctl_cleaner_wait_args w4c_arg;
+
+	char	*path = ".";
+        w4c_arg.ms = 0UL;
+
+        if (argc > 1) 
+	     path = argv[1];
+        if (argc > 2) 
+             w4c_arg.ms = iso8601toms(argv[2]);
 
In the man page and in the help the syntax is reported as:

 btrfs filesystem reclaim <path> [<timeout>]

instead it should be

 btrfs filesystem reclaim [<path> [<timeout>]]

and it has to be reported that path is optional and if it is omitted the 
current CWD is taken.

2) I think that it is more reasonable avoid a "strict" iso 8601 syntax
for the
time. I suggest to use a simple syntax like

0.xxxx -> subsecond 
s or nothing -> seconds
m -> minutes
h -> hour
d -> day
M -> month (but make sense to wait up to a month ?)
[...]

3) As minor issue I have to point out that "reclaim" seems (to me)
that the
user has to start this command in order to obtain more space, like if this 
command starts a garbage collector. In any case the help/man page explain 
clearly the behavior of the command.

4)
[...]
+unsigned long iso8601toms(char *P){
+    unsigned long ms;
+    double component;
+    char *ptr;
+    char *endptr;
+    short M_min = 0;
+
+    ms = 0UL;
+    ptr = P;
+    for(;;){
+       component=strtod(ptr, &endptr);
+       switch (*endptr)
+       {
+          case ''P'': /* anchor */ case ''p'':
+             if (ptr > P)
+	         fprintf(stderr, "ignoring non-initial P "
+                         "in ISO8601 duration string %s\n", P);
+             break;
+          case ''Y'': /* years */ case ''y'':
+             component *= 12;
+          BIGM:
+             /* average days in a gregorian month */
+             component *= (365.2425 / 12.0);
+             /* ms in a day */
+             component *= ( 24 * 3600 * 1000 );
+             ms += component;
+             break;
+          case ''T'': /* Time (not date) anchor */ case
''t'':
+             M_min = 1;
+             break;
+          case ''W'': /* week */ case ''w'':
+             component *= 7;
+          case ''D'': /* day */ case ''d'':
+             component *=  24 ;
+          case ''H'': /* hour */ case ''h'':
+             component *= 60;
+             M_min = 1;
+          case ''M'': /* month, or minute */ case
''m'':
+             if (!M_min++)
+                 goto BIGM;
+             component *= 60;
[...]
In the function  I suggest to avoid if possible a goto in a switch case. I 
think that it is more clear to repeat few lines instead of doing a goto.

Reagrds
G.Baroncelli


-- 
gpg key@ keyserver.linux.it: Goffredo Baroncelli (ghigo)
<kreijack@inwind.it>
Key fingerprint = 4769 7E51 5293 D36C 814E  C054 BF04 F161 3DC5 0512
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Oct 7, 2010 at 1:10 AM, Goffredo Baroncelli <kreijack@libero.it>
wrote:
> On Wednesday, 06 October, 2010, David Nicol wrote:
>> the ISO 8601 duration support is very loose, but I believe it is
>> accurate for valid
> In the man page and in the help the syntax is reported as:
>
>  btrfs filesystem reclaim <path> [<timeout>]
>
> instead it should be
>
>  btrfs filesystem reclaim [<path> [<timeout>]]
>
> and it has to be reported that path is optional and if it is omitted the
> current CWD is taken.
D''oh! yes you are right.


> 2) I think that it is more reasonable avoid a "strict" iso 8601
syntax for the
> time. I suggest to use a simple syntax like
>
> 0.xxxx -> subsecond
> s or nothing -> seconds
> m -> minutes
> h -> hour
> d -> day
> M -> month (but make sense to wait up to a month ?)
> [...]
That''s what the function provides, aside from differentiating between
M and m instead of using the ISO8601 disambiguation rule (as explained
on wikipedia.) After sleeping on it I''m thinking that a better route
would be either

(1) having the timeout in seconds.subseconds and providing a separate
tool that will parse ISO8601 durations, which is suggested to invoke
in backticks. Greater reusability and fewer library functions linked
into the binaries for the win.

or

(2) only supporting the WDHMS designators, nothing larger, (not big-M
or Y), thus removing the ambiguity. Y or M before [TWDH] would be a
fatal error. No longer caring how many seconds in a year for the win.
> 3) As minor issue I have to point out that "reclaim" seems (to
me) that the
> user has to start this command in order to obtain more space, like if this
> command starts a garbage collector. In any case the help/man page explain
> clearly the behavior of the command.
My hope is that the current behavior is a temporary stand-in for
something to be developed later that will more aggressively collect
garbage.

> +       switch (*endptr)
> +       {
> +          case ''P'': /* anchor */ case
''p'':
> +             if (ptr > P)
> +                fprintf(stderr, "ignoring non-initial P "
> +                         "in ISO8601 duration string %s\n", P);
> +             break;
> +          case ''Y'': /* years */ case
''y'':
> +             component *= 12;
> +          BIGM:
> +             /* average days in a gregorian month */
> +             component *= (365.2425 / 12.0);
> +             /* ms in a day */
> +             component *= ( 24 * 3600 * 1000 );
> +             ms += component;
> +             break;
> +          case ''T'': /* Time (not date) anchor */ case
''t'':
> +             M_min = 1;
> +             break;
> +          case ''W'': /* week */ case
''w'':
> +             component *= 7;
> +          case ''D'': /* day */ case
''d'':
> +             component *=  24 ;
> +          case ''H'': /* hour */ case
''h'':
> +             component *= 60;
> +             M_min = 1;
> +          case ''M'': /* month, or minute */ case
''m'':
> +             if (!M_min++)
> +                 goto BIGM;
> +             component *= 60;
> [...]
> In the function  I suggest to avoid if possible a goto in a switch case. I
> think that it is more clear to repeat few lines instead of doing a goto.
Well it isn''t a whole Duff''s device, but by choosing to use
the goto
here the constant of year length in seconds only has to appear once. I
guess I could define a symbol. Stacking the cases like this should cut
down on the number of jumps in the compiled machine code, as each
"break" in a switch is short for a jump to the end. I''d be
okay with
testing nearer the top so the jump is forward instead of backwards.

I believe that having a goto in a switch statement with a lot of
fallthroughs in its logic (such as this one) highlights that a C
switch statement makes a jump table rather than being a macro for an
oddly crippled series of else-if statements referring to the same
topic, which is what some providers of switch syntax in certain other
languages like to do with the construct.

review: http://en.wikipedia.org/wiki/Duff''s_device

and I''m trying to optimize for size, not speed.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

anyway I think I got the logic wrong: the function as given yesterday
would misparse

   PT1M1M
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Oct 7, 2010 at 9:25 AM, David Nicol <davidnicol@gmail.com>
wrote:
> anyway I think I got the logic wrong: the function as given yesterday
> would misparse
>
>   PT1M1M
no it wouldn''t! because of the post-increment on the how-to-parse
''M''
state variable, the BIGM label can only get jumped to the first time,
and not following T, W, D or H. Rearranging it to have a forward jump
would lose this.

Better to have an initial sanity-check pass to verify that all
non-numerics are expected and in an acceptable order, and switch one
of the kinds of Ms to some other letter at that time, then zip through
the pieces with a case statement with no logic in it at all, just
stacked multiplications.

Goffredo -- this thinking-out-loud is not noise?
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Oct 7, 2010 at 1:10 AM, Goffredo Baroncelli <kreijack@libero.it>
wrote:
> Please the next time put your patch inline or it is more difficult to
> highlight a suggestion.
* drop support for years and months, except as identified usage errors

* lower-case ''m'' now minutes

* escalate syntax warnings to fatal exits


diff --git a/iso8601toms.c b/iso8601toms.c
index a1ee9bd..f982d34 100644
--- a/iso8601toms.c
+++ b/iso8601toms.c
@@ -25,6 +25,8 @@

     accept a non-integer as the last numeric component

+    always treats "m" as minutes
+
  it silently accepts:

     out of order duration type letters
@@ -35,10 +37,12 @@

     non-integers in any position

- it warns on:
+ it halts and catches fire on:

     P or p appearing somewhere besides the beginning of the string

+    Attempts to use Years or Months
+
     unrecognized characters


@@ -53,7 +57,6 @@ unsigned long iso8601toms(char *P){
     char *ptr;
     char *endptr;
     short M_min = 0;
-
     ms = 0UL;
     ptr = P;
     for(;;){
@@ -62,18 +65,13 @@ unsigned long iso8601toms(char *P){
        {
           case ''P'': /* anchor */ case ''p'':
              if (ptr > P)
-                fprintf(stderr, "ignoring non-initial P "
-                         "in ISO8601 duration string %s\n", P);
-             break;
-          case ''Y'': /* years */ case ''y'':
-             component *= 12;
-          BIGM:
-             /* average days in a gregorian month */
-             component *= (365.2425 / 12.0);
-             /* ms in a day */
-             component *= ( 24 * 3600 * 1000 );
-             ms += component;
-             break;
+                fprintf(stderr, "non-initial P "
+                         "in duration string %s\n", P);
+                 exit (-1);
+          case ''Y'': /* year */ case ''y'':
+                fprintf(stderr, "Years are not supported "
+                         "in duration string %s\n", P);
+                 exit (-1);
           case ''T'': /* Time (not date) anchor */ case
''t'':
              M_min = 1;
              break;
@@ -84,9 +82,15 @@ unsigned long iso8601toms(char *P){
           case ''H'': /* hour */ case ''h'':
              component *= 60;
              M_min = 1;
-          case ''M'': /* month, or minute */ case
''m'':
-             if (!M_min++)
-                 goto BIGM;
+          case ''M'': /* month, or minute */
+             if (M_min == 0 ){
+                fprintf(stderr, "Months are not supported "
+                         "in duration string %s\n"
+                         "use ''m'' instead or prefix a
''T''\n"
+                         , P);
+                 exit (-1);
+             };
+          case ''m'': /* minute */
              component *= 60;
           case ''S'': /* second */ case ''s'':
           case ''\0'': /* default to second */
@@ -96,10 +100,11 @@ unsigned long iso8601toms(char *P){

           default:
             fprintf(stderr,
-                "ignoring unexpected char [%c] "
-                "in iso8601 duration string %s\n",
+                "unexpected char [%c] in duration string %s\n"
+                "valid designators are [WwDdHhMmSs] with implied
trailing S.\n",
                 *endptr, P
              );
+             exit (-1);
        };
        if (!*endptr)
           return (ms);
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html