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

[Jeff Garzik <jeff@garzik.org>]

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