[Patch 3/4] chunkd: make error paths more regular

[Patch 3/4] chunkd: make error paths more regular

[Pete Zaitcev]

1. Don't try to save a call in a function where normal and error unfolding
   sequences are different.
2. Use exception labels linked to what caused them, not to what cleanup
   has to be done.
3. Balance fs_open <-> fs_close.

Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>

---
 server/be-fs.c  |   18 ++++++++----------
 server/chunkd.h |    1 -
 server/server.c |   14 ++++++--------
 3 files changed, 14 insertions(+), 19 deletions(-)

Jeff, I generally do not interfere into the style, such as the offset
for continuation arguments etc. In this case I had to puzzle out if
error paths were correct and figured it's cheaper to clean them.
For fs_free case, I had to think again what you meant by such an
unusual arrangement. Apparently it's just a random artefact.

commit d10b113308ac5bda13d524ebb3f42f610a9ff8c8
Author: Master <zaitcev@lembas.zaitcev.lan>
Date:   Fri Dec 25 22:25:41 2009 -0700

    Cleanup.

diff --git a/server/be-fs.c b/server/be-fs.c
index 89635f8..6551c83 100644
--- a/server/be-fs.c
+++ b/server/be-fs.c
@@ -75,7 +75,7 @@ int fs_open(void)
 	}
 
 	if (!tchdbsetmutex(hdb))
-		goto out_hdb;
+		goto out_mut;
 
 	omode = HDBOREADER | HDBONOLCK | HDBOWRITER | HDBOCREAT | HDBOTSYNC;
 	if (!tchdbopen(hdb, db_fn, omode)) {
@@ -86,24 +86,22 @@ int fs_open(void)
 
 	chunkd_srv.tbl_master = hdb;
 
-out:
 	free(db_fn);
-	return rc;
+	return 0;
 
+out_mut:
 out_hdb:
 	tchdbdel(hdb);
-	goto out;
+out:
+	free(db_fn);
+	return rc;
 }
 
 void fs_close(void)
 {
 	tchdbclose(chunkd_srv.tbl_master);
-}
-
-void fs_free(void)
-{
-	if (chunkd_srv.tbl_master)
-		tchdbdel(chunkd_srv.tbl_master);
+	tchdbdel(chunkd_srv.tbl_master);
+	chunkd_srv.tbl_master = NULL;
 }
 
 bool fs_table_open(const char *user, const void *kbuf, size_t klen,
diff --git a/server/chunkd.h b/server/chunkd.h
index a18a6d4..d0b0ea6 100644
--- a/server/chunkd.h
+++ b/server/chunkd.h
@@ -220,7 +220,6 @@ extern struct hail_log cldu_hail_log;
 /* be-fs.c */
 extern int fs_open(void);
 extern void fs_close(void);
-extern void fs_free(void);
 extern struct backend_obj *fs_obj_new(uint32_t table_id, const void *kbuf, size_t klen,
 				      enum chunk_errcode *err_code);
 extern struct backend_obj *fs_obj_open(uint32_t table_id, const char *user,
diff --git a/server/server.c b/server/server.c
index 381772b..1b48905 100644
--- a/server/server.c
+++ b/server/server.c
@@ -1664,13 +1664,13 @@ int main (int argc, char *argv[])
 
 	if (fs_open()) {
 		rc = 1;
-		goto err_out_session;
+		goto err_out_fs;
 	}
 
 	if (cld_begin(chunkd_srv.ourhost, chunkd_srv.cell, chunkd_srv.nid,
 		      &chunkd_srv.loc, NULL)) {
 		rc = 1;
-		goto err_out_fs;
+		goto err_out_cld;
 	}
 
 	/* set up server networking */
@@ -1686,19 +1686,17 @@ int main (int argc, char *argv[])
 
 	applog(LOG_INFO, "shutting down");
 
+	/* net_close(); */
 err_out_listen:
 	cld_end();
-err_out_fs:
+err_out_cld:
 	fs_close();
+err_out_fs:
+	g_hash_table_destroy(chunkd_srv.fd_info);
 err_out_session:
-	/* net_close(); */
 	unlink(chunkd_srv.pid_file);
 	close(chunkd_srv.pid_fd);
 err_out:
-	if (strict_free) {
-		fs_free();
-		g_hash_table_destroy(chunkd_srv.fd_info);
-	}
 	closelog();
 	return rc;
 }

Re: [Patch 3/4] chunkd: make error paths more regular

[Jeff Garzik]

On 12/26/2009 01:34 AM, Pete Zaitcev wrote:
> diff --git a/server/be-fs.c b/server/be-fs.c
> index 89635f8..6551c83 100644
> --- a/server/be-fs.c
> +++ b/server/be-fs.c
> @@ -75,7 +75,7 @@ int fs_open(void)
>   	}
>
>   	if (!tchdbsetmutex(hdb))
> -		goto out_hdb;
> +		goto out_mut;
>
>   	omode = HDBOREADER | HDBONOLCK | HDBOWRITER | HDBOCREAT | HDBOTSYNC;
>   	if (!tchdbopen(hdb, db_fn, omode)) {
> @@ -86,24 +86,22 @@ int fs_open(void)
>
>   	chunkd_srv.tbl_master = hdb;
>
> -out:
>   	free(db_fn);
> -	return rc;
> +	return 0;
>
> +out_mut:
>   out_hdb:
>   	tchdbdel(hdb);
> -	goto out;
> +out:
> +	free(db_fn);
> +	return rc;
>   }
>

This part is OK.


> -void fs_free(void)
> -{
> -	if (chunkd_srv.tbl_master)
> -		tchdbdel(chunkd_srv.tbl_master);
> +	tchdbdel(chunkd_srv.tbl_master);
> +	chunkd_srv.tbl_master = NULL;
>   }
>
>   bool fs_table_open(const char *user, const void *kbuf, size_t klen,
> diff --git a/server/chunkd.h b/server/chunkd.h
> index a18a6d4..d0b0ea6 100644
> --- a/server/chunkd.h
> +++ b/server/chunkd.h
> @@ -220,7 +220,6 @@ extern struct hail_log cldu_hail_log;
>   /* be-fs.c */
>   extern int fs_open(void);
>   extern void fs_close(void);
> -extern void fs_free(void);
>   extern struct backend_obj *fs_obj_new(uint32_t table_id, const void *kbuf, size_t klen,
>   				      enum chunk_errcode *err_code);
>   extern struct backend_obj *fs_obj_open(uint32_t table_id, const char *user,
> diff --git a/server/server.c b/server/server.c
> index 381772b..1b48905 100644
> --- a/server/server.c
> +++ b/server/server.c
> @@ -1664,13 +1664,13 @@ int main (int argc, char *argv[])
>
>   	if (fs_open()) {
>   		rc = 1;
> -		goto err_out_session;
> +		goto err_out_fs;
>   	}
>
>   	if (cld_begin(chunkd_srv.ourhost, chunkd_srv.cell, chunkd_srv.nid,
>   		&chunkd_srv.loc, NULL)) {
>   		rc = 1;
> -		goto err_out_fs;
> +		goto err_out_cld;
>   	}
>
>   	/* set up server networking */
> @@ -1686,19 +1686,17 @@ int main (int argc, char *argv[])
>
>   	applog(LOG_INFO, "shutting down");
>
> +	/* net_close(); */
>   err_out_listen:
>   	cld_end();
> -err_out_fs:
> +err_out_cld:
>   	fs_close();
> +err_out_fs:
> +	g_hash_table_destroy(chunkd_srv.fd_info);
>   err_out_session:
> -	/* net_close(); */
>   	unlink(chunkd_srv.pid_file);
>   	close(chunkd_srv.pid_fd);
>   err_out:
> -	if (strict_free) {
> -		fs_free();
> -		g_hash_table_destroy(chunkd_srv.fd_info);
> -	}


I think it was misunderstood what is going on here.  Read first what 
strict_free does, the docs are in "chunkd --help" output.

>         { "strict-free", 1001, NULL, 0,
>           "For memory-checker runs.  When shutting down server, free local "
>           "heap, rather than simply exit(2)ing and letting OS clean up." },

It can be significantly faster to exit() rather than needlessly walk 
data structures and free them, when the OS can do the same thing, only 
faster and with more efficiency.

Therefore the end code in main() handles both normal shutdown, after 
possibly considerable runtime has elapsed, and error shutdown.  Toss 
into the mix memory-checker code, and the result can be quite confusing, 
I admit.

The only time we need to free heap objects is the rare, non-default case 
of memory checker (valgrind, here) runs.

	Jeff