Patchwork [05,of,14,RFC,c-hglib:level0] hg_close: close the connection with hg cmd server for the given handle

login
register
mail settings
Submitter Iulian Stana
Date Sept. 3, 2013, 8:30 p.m.
Message ID <1d20e8ae28ff6b4b39eb.1378240244@doppler>
Download mbox | patch
Permalink /patch/2310/
State Deferred, archived
Headers show

Comments

Iulian Stana - Sept. 3, 2013, 8:30 p.m.
# HG changeset patch
# User Iulian Stana <julian.stana@gmail.com>
# Date 1378234875 -10800
#      Tue Sep 03 22:01:15 2013 +0300
# Node ID 1d20e8ae28ff6b4b39eb527888c3869ab5e98b86
# Parent  3b4648c6815ba9e1c46a7e2d1184a72952c6e537
hg_close: close the connection with hg cmd server for the given handle

The Handle paramater it's pass by address because I want to set the handle
pointer to NULL. It's a defence action. In this way I can test if a handle is
set or not.
Giovanni Gherdovich - Sept. 12, 2013, 4:49 a.m.
:::: # HG changeset patch
:::: # User Iulian Stana <julian.stana at gmail.com>
:::: # Date 1378234875 -10800
:::: #      Tue Sep 03 22:01:15 2013 +0300
:::: # Node ID 1d20e8ae28ff6b4b39eb527888c3869ab5e98b86
:::: # Parent  3b4648c6815ba9e1c46a7e2d1184a72952c6e537
:::: hg_close: close the connection with hg cmd server for the given handle
::::
:::: The Handle paramater it's pass by address because I want to set the handle
:::: pointer to NULL. It's a defence action. In this way I can test if
a handle is
:::: set or not.
::::
:::: diff --git a/client.c b/client.c
:::: --- a/client.c
:::: +++ b/client.c
:::: @@ -149,5 +149,28 @@
::::   return handle;
::::  }
::::
:::: +/*
:::: + * Close the connection for the given handle.
:::: + * */
:::: +int hg_close(hg_handle **handle)


I rebounce a comment from Laurence Holst (Aug 6th)
http://thread.gmane.org/gmane.comp.version-control.mercurial.devel/62348/focus=62498
that I second, and didn't get enough attention.

It's about the prototype

int hg_close(hg_handle **handle);

Kevin:
:::: One '*' too many, unless you're planning on closing
:::: multiple handles at once...

Iulian:
:::: It's a defense action, I need to set the pointer to NULL,
:::: to be sure that there will be no other calls after hg_close.
:::: (We can assume that our users will not make this kind of action,
:::: then I will change to only one '*')

Laurence:
:::: That does not help if the handle is also in another variable
:::: (e.g. of a calling function).
::::
:::: I do think it’s smart to defend against this, but it’s better prevented
:::: by always handing out unique handles that will cause an error
:::: if reused after closing.


:::: +{
:::: + if(!(*handle)) {
:::: + errno = EINVAL;
:::: + return -1;
:::: + }
:::: +
:::: + if(kill((*handle)->childpid, SIGKILL) < 0){
:::: + return -1;
:::: + }
:::: + close((*handle)->p_read);
:::: + close((*handle)->p_write);
:::: +
:::: + free_data(*handle);
:::: + free(*handle);
:::: + *handle = NULL;
:::: +
:::: + return 0;
:::: +}
:::: +
::::  #include "client.h"
::::  #include "utils.h"
:::: diff --git a/client.h b/client.h
:::: --- a/client.h
:::: +++ b/client.h
:::: @@ -71,7 +71,19 @@
::::   * */
::::  hg_handle *hg_open(const char *path, char *encoding);
::::
:::: -
:::: +/**
:::: + * \brief Close the connection for the given handle.
:::: + *
:::: + * Erase the handle and free the memory
:::: + * \param handle The handle of the connection that I want to close
:::: + * \retval  0 if successful
:::: + * \retval -1 to indicate an error, with errno set appropriately.
:::: + *
:::: + * errno can be:
:::: + *      - EINVAL   - Invalid argument ( handle it's set to a null pointer)
:::: + *      - kill(2) command errors
:::: + * */
:::: +int hg_close(hg_handle **handle);
::::
::::
::::  #endif

Patch

diff --git a/client.c b/client.c
--- a/client.c
+++ b/client.c
@@ -149,5 +149,28 @@ 
 	return handle;
 }
 
+/*
+ * Close the connection for the given handle.
+ * */
+int hg_close(hg_handle **handle)
+{
+	if(!(*handle)) {
+		errno = EINVAL;
+		return -1;
+	}
+
+	if(kill((*handle)->childpid, SIGKILL) < 0){
+		return -1;
+	}
+	close((*handle)->p_read);
+	close((*handle)->p_write);
+
+	free_data(*handle);
+	free(*handle);
+	*handle = NULL;
+
+	return 0;
+}
+
 #include "client.h"
 #include "utils.h"
diff --git a/client.h b/client.h
--- a/client.h
+++ b/client.h
@@ -71,7 +71,19 @@ 
  * */
 hg_handle *hg_open(const char *path, char *encoding);
 
-
+/**
+ * \brief Close the connection for the given handle.
+ * 
+ * Erase the handle and free the memory
+ * \param handle The handle of the connection that I want to close
+ * \retval  0 if successful
+ * \retval -1 to indicate an error, with errno set appropriately.
+ * 
+ * errno can be:
+ *      - EINVAL   - Invalid argument ( handle it's set to a null pointer)
+ *      - kill(2) command errors
+ * */
+int hg_close(hg_handle **handle);
 
 
 #endif