On (10/15/07 18:55), Cathy Zhou wrote:
>
>> Is an else clause missing here for the success case?
>
> No. in the success case, it should also calls dladm_destroy_conf(conf) and
> continue.
The lines are:
506
507 status = dladm_write_conf(conf);
508 if (status != DLADM_STATUS_OK)
509 goto destroyconf;
510
511 destroyconf:
512 dladm_destroy_conf(conf);
513 if (status != DLADM_STATUS_OK) {
514 if (orig_portstr != NULL)
515 free(orig_portstr);
516 return (status);
517 }
Lines 508-508 look redundant.
>> Not sure how we would get to line 520 if DLADM_OPT_PERSIST is set.
>> See next comment:
>
> Discuss. I am not sure what do you mean here.
The code is:
411 if (flags & DLADM_OPT_PERSIST) {
:
513 if (status != DLADM_STATUS_OK) {
514 if (orig_portstr != NULL)
515 free(orig_portstr);
516 return (status);
517 }
518 }
519
520 if (!(flags & DLADM_OPT_ACTIVE))
521 goto done;
Some comments at line 519 would help understand what each block
of code is addressing.
>> - i_dladm_aggr_add_rmv() :
>> There seem to be some constraints about what flags are possible- these
>> should be clarified via comments. For example, orig_portstr is only
>> calloc-ed if DLADM_OPT_PERSIST is set; Is it possible to have !PERSIST
>> but ACTIVE?
>
> Yes. It is.
>
>> If yes, then we would
>> get to line 552 with null orig_portstr, and seg fault at the strlen
>> in dladm_set_conf_field().
>
> No. Not that line would only be called if DLADM_OPT_PERSIST is set.
Ok, I missed that. Again, it would help to have comments to help
understand each case, since there are a lot of flag-checks and
goto statements interacting with each other, and it's hard to keep
track of which case is being addressed when reading the code.
Thanks for all the other responses..
--Sowmini
