Re: [dpdk-dev] [PATCH v5] app/pdump: add pudmp exits with primary support.

[Suanming.Mou <mousuanming@huawei.com>]

On 2019/4/30 17:42, Burakov, Anatoly wrote:
> On 30-Apr-19 12:35 PM, Suanming.Mou wrote:
>> When primary app exits, the residual running pdump will stop the
>> primary app to restart. Add pdump exits with primary support.
>>
>> Suggested-by: Varghese, Vipin <vipin.varghese@intel.com>
>> Suggested-by: Burakov, Anatoly <anatoly.burakov@intel.com>
>> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
>> ---
>
> <snip>
>
>>   static void
>> +disable_primary_monitor(void)
>> +{
>> +    int ret;
>> +
>> +    /* Don't worry about it is primary exit case. The alarm cancel
>> +     * function will take care about that. */
>> +    ret = rte_eal_alarm_cancel(monitor_primary, NULL);
>> +    if (ret < 0)
>> +        printf("Fail to disable monitor fail:%d\n", ret);
>
> Double fail :)
Ah, yes, sorry for that the code gets worse.  :(
>
>> +}
>> +
>> +static void
>>   signal_handler(int sig_num)
>>   {
>>       if (sig_num == SIGINT) {
>> @@ -910,6 +936,19 @@ struct parse_val {
>>           ;
>>   }
>>   +static void
>> +enable_primary_monitor(void)
>> +{
>> +    int ret;
>> +
>> +    /* Once primary exits, so will pdump. */
>> +    ret = rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary, NULL);
>> +    if (ret < 0) {
>> +        cleanup_pdump_resources();
>> +        rte_exit(EXIT_FAILURE, "Fail to monitor primary:%d\n", ret);
>> +    }
>
> Why is this function void, when you could've called rte_exit() in the 
> caller on failure? And why is it such a fatal error to set up the 
> timer? IMO just a warning would've been enough.

Here comes with two issues:

Q1. The return value of the function:

A1: I'm so sorry that it does not seem to make sense to check the 
function's return value. Does it mean if we change the timer set up from 
error to warning, then we can use the return value to judge if need to 
disable the primary_monitor?

Q2. The choice when rte_eal_alarm_set fail:

A2: OK, agree with that.

>
>> +}
>> +
>>   int
>>   main(int argc, char **argv)
>>   {
>> @@ -950,11 +989,13 @@ struct parse_val {
>>               rte_exit(EXIT_FAILURE, "Invalid argument\n");
>>       }
>>   -    /* create mempool, ring and vdevs info */
>> +    /* create mempool, ring, vdevs info and primary monitor */
>>       create_mp_ring_vdev();
>>       enable_pdump();
>> +    enable_primary_monitor();
>>       dump_packets();
>>   +    disable_primary_monitor();
>>       cleanup_pdump_resources();
>>       /* dump debug stats */
>>       print_pdump_stats();
>>
>
>