Mauricio Vásquez <mauricio.vasquezber...@studenti.polito.it> writes:
> Dear Aaron,
>
> The general idea looks good to me.
>
> Just some comments inline
>
> On 3 December 2015 at 05:23, Aaron Conole <acon...@redhat.com> wrote:
>
<< snipped >>
>> + if(lookup) {
>> + char **newargv = grow_argv(argv, ret, 2);
>> +
>> + if (!newargv)
>> + return ret;
>> +
>> + *argv = newargv;
>> + (*argv)[ret++] = strdup(opts[i].dpdk_option);
>> + (*argv)[ret++] = strdup(lookup);
>> + }
>> + }
>>
>> - /* Remove the --dpdk argument from arg list.*/
>> - argc--;
>> - argv++;
>> + return ret;
>> +}
>>
>>
> argv shall be NULL terminated, i.e. argv[argc] shall be NULL.
Bah, not sure how I forgot the argv/argc conventions.
I'll fix when I submit the formal patch, thanks for catching this.
>
> - /* Reject --user option */
>> - int i;
>> - for (i = 0; i < argc; i++) {
>> - if (!strcmp(argv[i], "--user")) {
>> - VLOG_ERR("Can not mix --dpdk and --user options, aborting.");
>> - }
>> +void
>> +dpdk_init(const struct ovsrec_open_vswitch *ovs_cfg)
>> +{
>> + static int initialized = 0;
>> +
>> + char **argv;
>> + int result;
>> + int argc;
>> +
<< snipped >>
>>
>> /* Make sure things are initialized ... */
>> result = rte_eal_init(argc, argv);
>> if (result < 0) {
>> ovs_abort(result, "Cannot init EAL");
>> }
>> +
>> + for(result = 0; result < argc-1; ++result)
>> + free(argv[result]);
>> +
>> + free(argv);
>>
> I am not sure if freeing argv is valid in this case, C99 standard states
> that it shall remain valid until the program terminates.
I hope dpdk doesn't rely on that behavior, but you're correct. I'll
switch to using an atexit() function that delays releasing all the memory.
Thanks so much for the review!
-Aaron
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
