Sowmini.Varadhan at Sun.COM wrote:
> 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.
>
I've removed those two line. Thanks!
>>> 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.
>
>>> 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.
>
I'll add some comments. Thank you!
- Cathy
