2007/9/30, Denys Vlasenko <[EMAIL PROTECTED]>:
> On Saturday 29 September 2007 04:14, Loïc Grenié wrote:
> >
> >     Here it is. I've tried to take care of all the remarks.
> >
> >        Loïc
>
>  /* How to determine who we are? find 3rd char from the end:
>   * kill, killall, killall5
> - *  ^i       ^a        ^l  - it's unique
> - * (checking from the start is complicated by /bin/kill... case) */
> -       const char char3 = argv[0][strlen(argv[0]) - 3];
> -#define killall (ENABLE_KILLALL && char3 == 'a')
> -#define killall5 (ENABLE_KILLALL5 && char3 == 'l')
> + *   4      7        8     - the number of characters of the name is unique
> + */
> +       const char namelen = strlen(applet_name);
> +#define killall (ENABLE_KILLALL && namelen == 7)
> +#define killall5 (ENABLE_KILLALL5 && namelen == 8)
>  #endif
>
> You didn't read kill.c header comment carefully. kill.c is special.
> applet_name may not be set correctly for it.

    Sorry.

> +       int scan_mask = PSSCAN_STAT;
>
> You mean, PSSCAN_COMM - "I want to know COMM field". Which brings a question:
> what will happen to processes with 15+ char names (they get truncated comm 
> field)?

    They are not matched unless you give -f flag (consistent with GNU
  pgrep).

> +       const int NMATCH = 2;
> +       regmatch_t re_regs[NMATCH];
>
> Why do you need to find first TWO matches?

     Sorry I copied too fast.

> +       if (pkill && list) { // -l
> +               if (argc || opt & ~0x01)
> +                       bb_perror_nomsg_and_die();
> +               /* Print the whole signal list */
> +               print_signames_and_exit();
> +       }
>
> What perror is supposed to print here? What value is in errno here?
> I will simply do:
>        if (pkill) {
>                 if (OPT_LIST) /* -l: print the whole signal list */
>                         print_signames_and_exit();
>                 ....
>
>
> +               if (p->pid != pid &&
> +                       (regexec(&re_buffer, cmd, NMATCH, re_regs, 0) != 
> REG_NOMATCH &&
> +                               (OPT_ANCHOR ||
> +                                (re_regs[0].rm_so == 0 && re_regs[0].rm_eo 
> == strlen(cmd))))
> +                               ^ OPT_INVERT)
> +               {
>
> Readability took a dive here. You fell victim of it too, i think:
> bug, should be !OPT_ANCHOR instead of OPT_ANCHOR.

    You are correct.

> I applied a modified version of it to svn. Thanks.

    Your version is better.

    There was a typo at line 122 of pgrep.c and I made an error: kill
  cannot exit. I've modified print_signames_and_exit in print_signames
  and added a return EXIT_SUCCESS; to pgrep_main.

          Loïc
_______________________________________________
busybox mailing list
busybox@busybox.net
http://busybox.net/cgi-bin/mailman/listinfo/busybox

Reply via email to