[LTP] [PATCH v3] fzsync: revoke thread_b if parent hits an accidental break

[Richard Palethorpe]

Li Wang <liwang@redhat.com> writes:

> On Wed, Sep 25, 2019 at 8:13 PM Richard Palethorpe <rpalethorpe@suse.de>
> wrote:
>
>> ...
>> I'm not sure what you are saying. However you could do something like
>> this (I haven't tested it):
>>
>
> I misunderstood your words in the last mail, sorry about that.

No problem. This is also my fault.

>
>
>>
>> struct tst_fzsync_run_thread
>> {
>>         void *(*run)(void *);
>>         void *arg;
>> };
>>
>> static void tst_fzsync_thread_wrapper(void *arg)
>> {
>>         struct tst_fzsync_run_thread t = *(struct tst_fzsync_run_thread
>> *)arg;
>>
>>         pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS);
>>         pthread_setcancelstate(PTHREAD_CANCEL_ENABLE);
>>
>>         t.run(t.arg);
>> }
>>
>> static void tst_fzsync_pair_reset(..., struct tst_fzsync_run_thread *run_b)
>>
>
> I'd like to keep the tst_fzync_pair_reset() API no change to user.
>
> The patch v4.1 like below, is there any improper place in usage?

LGTM! 

>
> @@ -218,12 +219,36 @@ static void tst_fzsync_pair_init(struct
> tst_fzsync_pair *pair)
>  static void tst_fzsync_pair_cleanup(struct tst_fzsync_pair *pair)
>  {
>         if (pair->thread_b) {
> -               tst_atomic_store(1, &pair->exit);
> +               /* Revoke thread B if parent hits accidental break */
> +               if (!pair->exit) {
> +                       tst_atomic_store(1, &pair->exit);
> +                       usleep(100000);
> +                       pthread_cancel(pair->thread_b);
> +               }
>                 SAFE_PTHREAD_JOIN(pair->thread_b, NULL);
>                 pair->thread_b = 0;
>         }
>  }
>
> +/** To store the run_b pointer and pass to tst_fzsync_thread_wrapper */
> +struct tst_fzsync_run_thread {
> +       void *(*func)(void *);
> +       void *arg;
> +};
> +
> +/**
> + * Wrap run_b for tst_fzsync_pair_reset to enable pthread cancel
> + * at the start of the thread B.
> + */
> +static void *tst_fzsync_thread_wrapper(void *run_thread)
> +{
> +       struct tst_fzsync_run_thread t = *(struct tst_fzsync_run_thread
> *)run_thread;
> +
> +       pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, NULL);
> +       pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, NULL);
> +       return t.func(t.arg);
> +}
> +
>  /**
>   * Zero some stat fields
>   *
> @@ -271,8 +296,10 @@ static void tst_fzsync_pair_reset(struct
> tst_fzsync_pair *pair,
>         pair->a_cntr = 0;
>         pair->b_cntr = 0;
>         pair->exit = 0;
> -       if (run_b)
> -               SAFE_PTHREAD_CREATE(&pair->thread_b, 0, run_b, 0);
> +       if (run_b) {
> +               struct tst_fzsync_run_thread wrap_run_b = {.func = run_b,
> .arg = NULL};
> +               SAFE_PTHREAD_CREATE(&pair->thread_b, 0,
> tst_fzsync_thread_wrapper, &wrap_run_b);
> +       }
>
>         pair->exec_time_start = (float)tst_timeout_remaining();
>  }
>
>
>> Note that in any case you can't reliably cast a function pointer to a
>> void pointer without some magic. I am guessing wrapping it in a struct
>> is the clearest way to do it.
>>
>
> Good to know this, I searched on google and confirmed that the C standard
> does not allow to cast function pointer to void*, thanks!
>
>
>>
>> You can remove the arg altogether, but I kept it because we have a
>> struct anyway to wrap the function pointer.
>>
>
> Yes, to keep it make the wrapper struct is clear to understand.


-- 
Thank you,
Richard.