intel / idxd-config

Accel-config / libaccel-config
Other
56 stars 35 forks source link

accel-config: Clean up memory leak, errouneous assignment, and unused path variable #20

Closed snits closed 1 year ago

snits commented 1 year ago

The following pattern occurs with wq_base, group_base, and engine_base in accfg/lib/libaccfg.c:

// Where X is one of: wq, group, engine
X_base_string = strdup(X_base);
sscanf(X_base_string,...);
...
X->X_path = strdup(X_base);

and X_base_string is never cleaned up with free().

Instead of calling strdup() twice, just assign X_base_string to X->X_path, clean up the error checking related to the 2nd strdup(), and make the error path clean up more consistent between the cases.

Also a really minor clean up of test in accfg/test.c, so covscan stops complaining about it.

Signed-off-by: Jerry Snitselaar jsnitsel@redhat.com

snits commented 1 year ago

@ramesh-thomas I added that 2nd commit later when I noticed that it doesn't appear that anything is making use of that path variable. So not tied to the memory leak complaints from coverity, but I figured I would just tack it on this merge request.

snits commented 1 year ago

Darn it. I need to fix something up that I missed in the conflict resolution.

ramesh-thomas commented 1 year ago

Darn it. I need to fix something up that I missed in the conflict resolution.

Sure. Let me know when you are done. I will merge them and make a new release. Thanks.

snits commented 1 year ago

Darn it. I need to fix something up that I missed in the conflict resolution.

Sure. Let me know when you are done. I will merge them and make a new release. Thanks.

I think it should be good now.

snits commented 1 year ago

I dropped the error path changes for now, and just pushed the fix up of the leak and erroneous assignment, plus the path variable clean up. I'll look at doing a re-organization of those error paths separately if you would like that.

snits commented 1 year ago

I dropped the error path changes for now, and just pushed the fix up of the leak and erroneous assignment, plus the path variable clean up. I'll look at doing a re-organization of those error paths separately if you would like that.

And then after dinner, and walking the dog I decided to stick it on top as a separate patch. :) @ramesh-thomas let me know if you want me to pull that from this pr.

ramesh-thomas commented 1 year ago

And then after dinner, and walking the dog I decided to stick it on top as a separate patch. :) @ramesh-thomas let me know if you want me to pull that from this pr.

I think your patch improves the readability. The previous code, though not broken, was relying too much on the fact that free(NULL) is a nop and was doing redundant freeing. I have merged it into pending. Will move to stable once the location of the config files are finalized.