[Libstoragemgmt-devel] [PATCH] cleanup and bug fix

[Ma Shimiao]

Add ErrorCreate when memory is not enough and can not access plugin.
Make some cleanup.
Add optional_data free when free Disk Record.

Signed-off-by: Ma Shimiao <***@cn.fujitsu.com>
---
src/lsm_datatypes.cpp | 44 +++++++++++++++++++++++++++++++++++---------
1 file changed, 35 insertions(+), 9 deletions(-)

diff --git a/src/lsm_datatypes.cpp b/src/lsm_datatypes.cpp
index 7e6415d..54c1432 100644
--- a/src/lsm_datatypes.cpp
+++ b/src/lsm_datatypes.cpp
@@ -230,6 +230,7 @@ void freeConnection(lsmConnect *c)
}

free(c);
+ c = NULL;
}
}

@@ -294,6 +295,11 @@ int loadDriver(lsmConnect *c, const char *plugin_name, const char *password,
const char *plugin_dir = uds_path();

if (asprintf(&plugin_file, "%s/%s", plugin_dir, plugin_name) == -1) {
+ *e = lsmErrorCreate(LSM_ERR_NO_MEMORY,
+ LSM_ERR_DOMAIN_FRAME_WORK,
+ LSM_ERR_LEVEL_ERROR, "Not enough memory",
+ NULL, NULL, NULL, 0 );
+
return LSM_ERR_NO_MEMORY;
}

@@ -317,6 +323,11 @@ int loadDriver(lsmConnect *c, const char *plugin_name, const char *password,
rc = LSM_ERR_PLUGIN_DLOPEN;
}
} else {
+ *e = lsmErrorCreate(LSM_ERR_PLUGIN_PERMISSIONS,
+ LSM_ERR_DOMAIN_FRAME_WORK,
+ LSM_ERR_LEVEL_ERROR, "Unable to access plugin",
+ NULL, NULL, NULL, 0 );
+
rc = LSM_ERR_PLUGIN_PERMISSIONS;
}

@@ -395,6 +406,7 @@ int lsmErrorFree(lsmErrorPtr e)

e->magic = LSM_DEL_MAGIC(LSM_ERROR_MAGIC);
free(e);
+ e = NULL;

return LSM_ERR_OK;
}
@@ -629,6 +641,9 @@ void lsmInitiatorRecordFree(lsmInitiator *i)
if (i->id) {
free(i->id);
i->id = NULL;
+ }
+
+ if (i->name) {
free(i->name);
i->name = NULL;
}
@@ -794,9 +809,9 @@ lsmVolume *lsmVolumeRecordCopy(lsmVolume *vol)
{
lsmVolume *rc = NULL;
if( LSM_IS_VOL(vol) ) {
- rc = lsmVolumeRecordAlloc( vol->id, vol->name, vol->vpd83,
- vol->blockSize, vol->numberOfBlocks,
- vol->status, vol->system_id, vol->pool_id);
+ rc = lsmVolumeRecordAlloc(vol->id, vol->name, vol->vpd83,
+ vol->blockSize, vol->numberOfBlocks,
+ vol->status, vol->system_id, vol->pool_id);
}
return rc;
}
@@ -853,14 +868,25 @@ void lsmDiskRecordFree(lsmDisk *d)
if ( LSM_IS_DISK(d) ) {
d->magic = LSM_DEL_MAGIC(LSM_DISK_MAGIC);

- free(d->id);
- d->id = NULL;
+ if( d->id ) {
+ free(d->id);
+ d->id = NULL;
+ }

- free(d->name);
- d->name = NULL;
+ if( d->name ) {
+ free(d->name);
+ d->name = NULL;
+ }
+
+ if( d->system_id ) {
+ free(d->system_id);
+ d->system_id = NULL;
+ }

- free(d->system_id);
- d->system_id = NULL;
+ if (d->optional_data) {
+ lsmOptionalDataRecordFree(d->optional_data);
+ d->optional_data = NULL;
+ }

free(d);
}

[Tony Asleson]

Comments inline below.

Post by Ma Shimiao
Add ErrorCreate when memory is not enough and can not access plugin.
Make some cleanup.
Add optional_data free when free Disk Record.
---
src/lsm_datatypes.cpp | 44 +++++++++++++++++++++++++++++++++++---------
1 file changed, 35 insertions(+), 9 deletions(-)
diff --git a/src/lsm_datatypes.cpp b/src/lsm_datatypes.cpp
index 7e6415d..54c1432 100644
--- a/src/lsm_datatypes.cpp
+++ b/src/lsm_datatypes.cpp
@@ -230,6 +230,7 @@ void freeConnection(lsmConnect *c)
}
free(c);
+ c = NULL;
}
}
@@ -294,6 +295,11 @@ int loadDriver(lsmConnect *c, const char *plugin_name, const char *password,
const char *plugin_dir = uds_path();
if (asprintf(&plugin_file, "%s/%s", plugin_dir, plugin_name) == -1) {
+ *e = lsmErrorCreate(LSM_ERR_NO_MEMORY,
+ LSM_ERR_DOMAIN_FRAME_WORK,
+ LSM_ERR_LEVEL_ERROR, "Not enough memory",
+ NULL, NULL, NULL, 0 );
+

If we just got an allocation failure, I don't think we should try and
allocate an error message stating that too as it will most likely fail.

Post by Ma Shimiao
return LSM_ERR_NO_MEMORY;
}
@@ -317,6 +323,11 @@ int loadDriver(lsmConnect *c, const char *plugin_name, const char *password,
rc = LSM_ERR_PLUGIN_DLOPEN;
}
} else {
+ *e = lsmErrorCreate(LSM_ERR_PLUGIN_PERMISSIONS,
+ LSM_ERR_DOMAIN_FRAME_WORK,
+ LSM_ERR_LEVEL_ERROR, "Unable to access plugin",
+ NULL, NULL, NULL, 0 );
+
rc = LSM_ERR_PLUGIN_PERMISSIONS;
}
@@ -395,6 +406,7 @@ int lsmErrorFree(lsmErrorPtr e)
e->magic = LSM_DEL_MAGIC(LSM_ERROR_MAGIC);
free(e);
+ e = NULL;
return LSM_ERR_OK;
}
@@ -629,6 +641,9 @@ void lsmInitiatorRecordFree(lsmInitiator *i)
if (i->id) {
free(i->id);
i->id = NULL;
+ }
+
+ if (i->name) {
free(i->name);
i->name = NULL;
}
@@ -794,9 +809,9 @@ lsmVolume *lsmVolumeRecordCopy(lsmVolume *vol)
{
lsmVolume *rc = NULL;
if( LSM_IS_VOL(vol) ) {
- rc = lsmVolumeRecordAlloc( vol->id, vol->name, vol->vpd83,
- vol->blockSize, vol->numberOfBlocks,
- vol->status, vol->system_id, vol->pool_id);
+ rc = lsmVolumeRecordAlloc(vol->id, vol->name, vol->vpd83,
+ vol->blockSize, vol->numberOfBlocks,
+ vol->status, vol->system_id, vol->pool_id);
}
return rc;
}
@@ -853,14 +868,25 @@ void lsmDiskRecordFree(lsmDisk *d)
if ( LSM_IS_DISK(d) ) {
d->magic = LSM_DEL_MAGIC(LSM_DISK_MAGIC);
- free(d->id);
- d->id = NULL;
+ if( d->id ) {
+ free(d->id);
+ d->id = NULL;
+ }
- free(d->name);
- d->name = NULL;
+ if( d->name ) {
+ free(d->name);
+ d->name = NULL;
+ }
+
+ if( d->system_id ) {
+ free(d->system_id);
+ d->system_id = NULL;
+ }

free(NULL) is valid as free checks for null. Thus we really don't need
to do a check before we call free. I was actually going to go through
and strip out other `if` checks before we call free.

Post by Ma Shimiao
- free(d->system_id);
- d->system_id = NULL;
+ if (d->optional_data) {
+ lsmOptionalDataRecordFree(d->optional_data);
+ d->optional_data = NULL;
+ }
free(d);
}

Good catch on the optional_data! We do have a memory leak here!

Please amend patch and re-submit!

Thanks!

Regards,
Tony

[MaShimiao]

I will make V2 to just fix memory leak.
Below is my opinion about checks before calling free.

Post by Tony Asleson

Post by Ma Shimiao
- free(d->id);

Post by Ma Shimiao
- d->id = NULL;
+ if( d->id ) {
+ free(d->id);
+ d->id = NULL;
+ }
- free(d->name);
- d->name = NULL;
+ if( d->name ) {
+ free(d->name);
+ d->name = NULL;
+ }
+
+ if( d->system_id ) {
+ free(d->system_id);
+ d->system_id = NULL;
+ }

free(NULL) is valid as free checks for null. Thus we really don't need
to do a check before we call free. I was actually going to go through
and strip out other `if` checks before we call free.

free(NULL) is actually right.
As we know, After calling free, pointers set to NULL is necessary.
That can avoid some errors.
In my opinion, checks before we call free is not to make sure free(NULL) will not happen.
It is intend to avoid doing free and setting NULL when pointer is NULL.

Best Regards,