Hello,
I did a code review of some of the server code for distcc. I added
comments where functions' actions weren't evident from the name, and
addressed some issues I found:
src/bulk.c: 210: duplication of code to close fd and return
src/io.c: 166: EAGAIN is checked twice. EINTR needs checking to be
signal-safe.
208: There's no EOF condition for writes. This and the last buglet
were previously mentioned on the distcc mailing list.
src/serve.c: 317: If an intermediate transmission fails, we shouldn't
send the DOTO 0
src/util.c: 210: I think it's safer to install a sigaction instead of a
one-shot call to signal() to handle SIGPIPEs
The attached patch was generated against the latest sources in arch. I'd
appreciate any comments.
Regards,
Thomas Kho
Signed-off-by: Thomas Kho <[EMAIL PROTECTED]>
I did a code review of some of the server code for distcc. I added comments
where a function's actions weren't evident from the name, and addressed some
issues I found:
src/bulk.c: 210: duplication of code to close fd and return
src/io.c: 166: EAGAIN is checked twice. EINTR needs checking to be signal-safe.
208: There's no EOF condition for writes. This and the last buglet
were previously mentioned on the distcc mailing list.
src/serve.c: 317: If an intermediate transmission fails, we shouldn't send the
DOTO 0
src/util.c: 210: I think it's safer to install a sigaction instead of a
one-shot call to signal() to handle SIGPIPEs
I'd appreciate any comments.
Thomas Kho
--- ../../arch2/devel/src/arg.c 2005-12-15 13:23:21.590021000 -0800
+++ src/arg.c 2005-12-15 16:17:04.387132000 -0800
@@ -97,6 +97,7 @@
return 0;
}
+/* TODO: dcc_note_* functions probably belong together */
static void dcc_note_compiled(const char *input_file, const char *output_file)
{
const char *input_base, *output_base;
--- ../../arch2/devel/src/bulk.c 2005-12-15 13:23:10.363260000 -0800
+++ src/bulk.c 2005-12-15 16:45:39.592661000 -0800
@@ -129,6 +129,9 @@
}
+/**
+ * Compresses a local file and trasmits to network.
+ **/
static int dcc_x_file_lzo1x(int out_fd,
int in_fd,
const char *token,
@@ -162,10 +165,9 @@
/**
- * Transmit from a local file to the network. Sends TOKEN, LENGTH, BODY,
- * where the length is the appropriate compressed length.
+ * Transmit from a local file to the network. Sends TOKEN, LENGTH, BODY.
*
- * Does compression if needed.
+ * Does compression if needed, and sets length appropriately.
*
* @param ofd File descriptor for the network connection.
* @param fname Name of the file to send.
@@ -207,10 +209,6 @@
return EXIT_PROTOCOL_ERROR;
}
- if (ifd != -1)
- dcc_close(ifd);
- return 0;
-
failed:
if (ifd != -1)
dcc_close(ifd);
@@ -321,6 +319,9 @@
}
+/**
+ * Receive a token and file
+ **/
int dcc_r_token_file(int in_fd,
const char *token,
const char *fname,
--- ../../arch2/devel/src/exec.c 2005-12-15 13:23:41.341251000 -0800
+++ src/exec.c 2005-12-15 15:55:23.951256000 -0800
@@ -238,7 +238,7 @@
* basename.
*
* Actually this code is called on both the client and server, which might
- * cause unintnded behaviour in contrived cases, like giving a full path
+ * cause unintended behaviour in contrived cases, like giving a full path
* to a file that doesn't exist. I don't think that's a problem. */
slash = strrchr(argv[0], '/');
--- ../../arch2/devel/src/io.c 2005-12-15 13:23:33.418183000 -0800
+++ src/io.c 2005-12-15 16:51:09.398481000 -0800
@@ -67,6 +67,8 @@
/**
+ * Block the process until the fd is readable.
+ *
* @todo Perhaps only apply the timeout for initial connections, not when
* doing regular IO.
**/
@@ -149,6 +151,8 @@
/**
* Read exactly @p len bytes from a file.
+ *
+ * @p fd can be blocking or non-blocking. Read is signal-safe.
**/
int dcc_readx(int fd, void *buf, size_t len)
{
@@ -163,7 +167,7 @@
return ret;
else
continue;
- } else if (r == -1 && errno == EAGAIN) {
+ } else if (r == -1 && errno == EINTR) {
continue;
} else if (r == -1) {
rs_log_error("failed to read: %s", strerror(errno));
@@ -185,6 +189,8 @@
* Write bytes to an fd. Keep writing until we're all done or something goes
* wrong.
*
+ * @p fd can be blocking or non-blocking. Write is signal-safe.
+ *
* @returns 0 or exit code.
**/
int dcc_writex(int fd, const void *buf, size_t len)
@@ -205,9 +211,6 @@
} else if (r == -1) {
rs_log_error("failed to write: %s", strerror(errno));
return EXIT_IO_ERROR;
- } else if (r == 0) {
- rs_log_error("unexpected eof on fd%d", fd);
- return EXIT_TRUNCATED;
} else {
buf = &((char *) buf)[r];
len -= r;
--- ../../arch2/devel/src/serve.c 2005-12-15 13:23:10.367257000 -0800
+++ src/serve.c 2005-12-15 16:52:06.771146000 -0800
@@ -266,6 +266,7 @@
if ((ret = dcc_make_tmpnam("distcc", ".stdout", &out_fname)))
goto out_cleanup;
+ /* Remove files created by dcc_make_tmpnam */
dcc_remove_if_exists(err_fname);
dcc_remove_if_exists(out_fname);
@@ -314,9 +315,10 @@
if ((ret = dcc_x_result_header(out_fd, protover))
|| (ret = dcc_x_cc_status(out_fd, status))
|| (ret = dcc_x_file(out_fd, err_fname, "SERR", compr, NULL))
- || (ret = dcc_x_file(out_fd, out_fname, "SOUT", compr, NULL))
- || WIFSIGNALED(status)
- || WEXITSTATUS(status)) {
+ || (ret = dcc_x_file(out_fd, out_fname, "SOUT", compr, NULL))) {
+ /* Something went wrong during transmit, so stop sending */
+ } else if (WIFSIGNALED(status)
+ || WEXITSTATUS(status)) {
/* Something went wrong, so send DOTO 0 */
dcc_x_token_int(out_fd, "DOTO", 0);
} else {
--- ../../arch2/devel/src/tempfile.c 2005-12-15 13:23:41.393200000 -0800
+++ src/tempfile.c 2005-12-15 12:50:06.755805000 -0800
@@ -239,7 +239,8 @@
/**
* Create a file inside the temporary directory and register it for
- * later cleanup, and return its name.
+ * later cleanup, and return its name. A unique identifier is appended to the
+ * filename.
*
* The file will be reopened later, possibly in a child. But we know
* that it exists with appropriately tight permissions.
--- ../../arch2/devel/src/util.c 2005-12-15 13:23:10.735903000 -0800
+++ src/util.c 2005-12-15 16:11:44.046791000 -0800
@@ -154,7 +154,7 @@
/**
* Look up a boolean environment option, which must be either "0" or
- * "1". The default, if it's not set or is empty, is @p default.
+ * "1". The default, if it's not set or is empty, is @p default_value.
**/
int dcc_getenv_bool(const char *name, int default_value)
{
@@ -207,8 +207,11 @@
**/
int dcc_ignore_sigpipe(int val)
{
- if (signal(SIGPIPE, val ? SIG_IGN : SIG_DFL) == SIG_ERR) {
- rs_log_warning("signal(SIGPIPE, %s) failed: %s",
+ struct sigaction sa;
+ memset(&sa, 0, sizeof(sa));
+ sa.sa_handler = val ? SIG_IGN : SIG_DFL;
+ if (sigaction(SIGPIPE, &sa, NULL) != 0) {
+ rs_log_warning("sigaction SIGPIPE %s failed: %s",
val ? "ignore" : "default",
strerror(errno));
return EXIT_DISTCC_FAILED;
__
distcc mailing list http://distcc.samba.org/
To unsubscribe or change options:
https://lists.samba.org/mailman/listinfo/distcc
