Re: [dpdk-dev] [PATCH v7 10/11] eal: replace rte_panic instances in init sequence

[Arnon Warshavsky <arnon@qwilt.com>]

<...>

>
>   +     if (rte_config_init() != 0) {
>> +               rte_eal_init_alert("Failed to init configuration");
>> +               rte_errno = EFAULT;
>> +               return -1;
>> +       }
>> +
>> +       if (rte_mp_channel_init() < 0) {
>> +               rte_eal_init_alert("failed to init mp channel\n");
>> +               if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>> +                       rte_errno = EFAULT;
>> +                       return -1;
>> +               }
>> +       }
>>
>
> ^^^ this change looks unintended. Rebase artifact?
>
> +
>>         /* in secondary processes, memory init may allocate additional
>> fbarrays
>>          * not present in primary processes, so to avoid any potential
>> issues,
>>          * initialize memzones first.
>> @@ -671,6 +712,7 @@ static void rte_eal_init_alert(const char *msg)
>>                  */
>>                 if (pipe(lcore_config[i].pipe_master2slave) < 0)
>>                         rte_panic("Cannot create pipe\n");
>> +
>>                 if (pipe(lcore_config[i].pipe_slave2master) < 0)
>>                         rte_panic("Cannot create pipe\n");
>>
>
> ^^^ this looks unintended as well.
>
>   diff --git a/lib/librte_eal/linuxapp/eal/eal.c
>> b/lib/librte_eal/linuxapp/eal/eal.c
>> index 5b23bf0..54adaec 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal.c
>> @@ -160,7 +160,7 @@ enum rte_iova_mode
>>    * We also don't lock the whole file, so that in future we can use
>> read-locks
>>    * on other parts, e.g. memzones, to detect if there are running
>> secondary
>>    * processes. */
>> -static void
>> +static int
>>   rte_eal_config_create(void)
>>   {
>>         void *rte_mem_cfg_addr;
>> @@ -169,7 +169,7 @@ enum rte_iova_mode
>>         const char *pathname = eal_runtime_config_path();
>>         if (internal_config.no_shconf)
>> -               return;
>>
>
> <...>
>
>         }
>>         rte_mem_cfg_addr = mmap(rte_mem_cfg_addr,
>> sizeof(*rte_config.mem_config),
>>                                 PROT_READ | PROT_WRITE, MAP_SHARED,
>> mem_cfg_fd, 0);
>>   -     if (rte_mem_cfg_addr == MAP_FAILED){
>> -               rte_panic("Cannot mmap memory for rte_config\n");
>> +       if (rte_mem_cfg_addr == MAP_FAILED) {
>> +               RTE_LOG(CRIT, EAL, "%s(): Cannot mmap memory for
>> rte_config\n",
>> +                       __func__);
>> +               return -1;
>>         }
>>
>
> I think you forgot to close mem_cfg_fd and set it to -1 in case of error
> here.
>
>         memcpy(rte_mem_cfg_addr, &early_mem_config,
>> sizeof(early_mem_config));
>>         rte_config.mem_config = rte_mem_cfg_addr;
>> @@ -211,10 +221,11 @@ enum rte_iova_mode
>>          * processes could later map the config into this exact location
>> */
>>         rte_config.mem_config->mem_cfg_addr = (uintptr_t)
>> rte_mem_cfg_addr;
>>   +     return 0;
>>   }
>>
>>
>
> <...>
>
>         /* map it as read-only first */
>>         mem_config = (struct rte_mem_config *) mmap(NULL,
>> sizeof(*mem_config),
>>                         PROT_READ, MAP_SHARED, mem_cfg_fd, 0);
>> -       if (mem_config == MAP_FAILED)
>> -               rte_panic("Cannot mmap memory for rte_config! error %i
>> (%s)\n",
>> -                         errno, strerror(errno));
>> +       if (mem_config == MAP_FAILED) {
>> +               mem_cfg_fd = -1;
>>
>
> Forgot close() here, i think.
>
> +               RTE_LOG(CRIT, EAL, "%s(): Cannot mmap memory for
>> rte_config! error %i (%s)\n",
>> +                               __func__, errno, strerror(errno));
>> +               return -1;
>> +       }
>>         rte_config.mem_config = mem_config;
>> +
>> +       return 0;
>>   }
>>     /* reattach the shared config at exact memory location primary
>> process has it */
>>
>
> <...>
>
> +       if (rte_config_init() != 0)
>> +               return -1;
>> +
>>         if (rte_eal_log_init(logid, internal_config.syslog_facility) <
>> 0) {
>>                 rte_eal_init_alert("Cannot init logging.");
>>                 rte_errno = ENOMEM;
>> @@ -914,6 +946,7 @@ static void rte_eal_init_alert(const char *msg)
>>                  */
>>                 if (pipe(lcore_config[i].pipe_master2slave) < 0)
>>                         rte_panic("Cannot create pipe\n");
>> +
>>                 if (pipe(lcore_config[i].pipe_slave2master) < 0)
>>
>
> Again, looks like unintended whitespace change.
>
>                         rte_panic("Cannot create pipe\n");
>>
>>
>
> Thanks Anatoly