[patch iwhd 1/2] do not leak "pending"

Pete Zaitcev zaitcev at redhat.com
Thu Jul 7 23:06:24 UTC 2011


Currently we set "pending" in case anything goes wrong, we crash or
whatnot, so that users can know that ami-id may be invalid. This is
fine, but if the context of POST is lost, it is not possible to
figure out what is going on: either the registration is still in
progress, or it is failed. Of course if we actually crash after
setting "pending", then it's bad. But we also leave "pending"
dangling for any error exit of dc-rhev-image, and that is wrong.

So, plug the return paths where "pending" may remain set after
everything is over. Not that this patch is not complete: the waitpid
is going to be fixed over with another patch.

http://www.aeolusproject.org/redmine/issues/1888

---
 backend.c |  104 ++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 72 insertions(+), 32 deletions(-)

commit 47c46704ef3b469bdd52f6d5e515ec2bb374fd3c
Author: Pete Zaitcev <zaitcev at yahoo.com>
Date:   Thu Jul 7 16:55:20 2011 -0600

    do not leak "pending"
    
    Currently we set "pending" in case anything goes wrong, we crash or
    whatnot, so that users can know that ami-id may be invalid. This is
    fine, but if the context of POST is lost, it is not possible to
    figure out what is going on: either the registration is still in
    progress, or it is failed. Of course if we actually crash after
    setting "pending", then it's bad. But we also leave "pending"
    dangling for any error exit of dc-rhev-image, and that is wrong.
    
    So, plug the return paths where "pending" may remain set after
    everything is over. Not that this patch is not complete: the waitpid
    is going to be fixed over with another patch.
    
    http://www.aeolusproject.org/redmine/issues/1888

diff --git a/backend.c b/backend.c
index cd1a685..4102a00 100644
--- a/backend.c
+++ b/backend.c
@@ -508,6 +508,7 @@ s3_register (my_state *ms, const provider_t *prov, const char *next,
 
 	if (pipe(organ) < 0) {
 		error (0, errno, _("pipe creation failed"));
+		strcpy(ami_id_buf,"failed (pipe)");
 		goto cleanup;
 	}
 
@@ -516,6 +517,7 @@ s3_register (my_state *ms, const provider_t *prov, const char *next,
 		error (0, errno, _("fork failed"));
 		close(organ[0]);
 		close(organ[1]);
+		strcpy(ami_id_buf,"failed (fork)");
 		goto cleanup;
 	}
 
@@ -538,18 +540,20 @@ s3_register (my_state *ms, const provider_t *prov, const char *next,
 			*s = '\0';
 		if (regexec(&s3_success_pat,buf,2,match,0) == 0) {
 			buf[match[1].rm_eo] = '\0';
-			DPRINTF("found AMI ID: %s\n",buf+match[1].rm_so);
-			sprintf(ami_id_buf,"OK %.60s",buf+match[1].rm_so);
+			s = buf+match[1].rm_so;
+			DPRINTF("found AMI ID: %s\n",s);
+			snprintf(ami_id_buf,sizeof(ami_id_buf),"OK %s",s);
 			rc = MHD_HTTP_OK;
 		}
 		else if (regexec(&s3_failure_pat,buf,2,match,0) == 0) {
 			buf[match[1].rm_eo] = '\0';
-			DPRINTF("found error marker: %s\n",buf+match[1].rm_so);
-			sprintf(ami_id_buf,"failed %.56s",buf+match[1].rm_so);
+			s = buf+match[1].rm_so;
+			DPRINTF("found error marker: %s\n",s);
+			snprintf(ami_id_buf,sizeof(ami_id_buf),"failed %s",s);
 			rc = MHD_HTTP_INTERNAL_SERVER_ERROR;
 		}
 		else {
-			DPRINTF("ignoring line: <%s>\n",buf);
+			DPRINTF("<%s>\n",buf);
 		}
 	}
 	fclose(fp);
@@ -557,18 +561,23 @@ s3_register (my_state *ms, const provider_t *prov, const char *next,
 	DPRINTF("waiting for child...\n");
 	if (waitpid(pid,&wstat,0) < 0) {
 		error (0, errno, _("waitpid failed"));
+		strcpy(ami_id_buf,"failed (waitpid)");
 		goto cleanup;
 	}
 	if (!WIFEXITED(wstat)) {
 		error (0, 0, _("%s is killed (status 0x%x)"), cmd, wstat);
-		goto cleanup;
 	}
-	if (WEXITSTATUS(wstat)) {
+	else if (WEXITSTATUS(wstat)) {
 		error (0, 0, _("%s exited with code %d"),
 		       cmd, WEXITSTATUS(wstat));
-		goto cleanup;
 	}
-	DPRINTF("...child exited\n");
+	else {
+		DPRINTF("...child exited\n");
+	}
+
+	if (strncmp(ami_id_buf,"pending",sizeof("pending")-1)==0) {
+		strcpy(ami_id_buf,"failed (back-end)");
+	}
 
 cleanup:
 	/*
@@ -1546,6 +1555,7 @@ fs_rhevm_register (my_state *ms, const provider_t *prov, const char *next,
 
 	if (pipe(organ) < 0) {
 		error (0, errno, _("pipe creation failed"));
+		strcpy(ami_id_buf,"failed (pipe)");
 		goto cleanup;
 	}
 
@@ -1554,6 +1564,7 @@ fs_rhevm_register (my_state *ms, const provider_t *prov, const char *next,
 		error (0, errno, _("fork failed"));
 		close(organ[0]);
 		close(organ[1]);
+		strcpy(ami_id_buf,"failed (fork)");
 		goto cleanup;
 	}
 
@@ -1569,6 +1580,7 @@ fs_rhevm_register (my_state *ms, const provider_t *prov, const char *next,
 	if (!fp) {
 		error (0, 0, _("could not open parent pipe stream"));
 		close(organ[0]);
+		strcpy(ami_id_buf,"failed (fdopen)");
 		goto cleanup;
 	}
 	while (fgets(buf,sizeof(buf)-1,fp)) {
@@ -1576,16 +1588,20 @@ fs_rhevm_register (my_state *ms, const provider_t *prov, const char *next,
 			*s = '\0';
 		if (regexec(&s3_success_pat,buf,2,match,0) == 0) {
 			buf[match[1].rm_eo] = '\0';
-			DPRINTF("found image UUID: %s\n",buf+match[1].rm_so);
-			sprintf(ami_id_buf,"OK %.60s",buf+match[1].rm_so);
+			s = buf+match[1].rm_so;
+			DPRINTF("found image UUID: %s\n",s);
+			snprintf(ami_id_buf,sizeof(ami_id_buf),"OK %s",s);
 			ret = MHD_HTTP_OK;
 		}
 		else if (strncmp(buf,"ERROR",sizeof("ERROR")-1) == 0) {
-			DPRINTF("found err marker: %s\n",buf+sizeof("ERROR"));
-			sprintf(ami_id_buf,"failed %.56s",buf+sizeof("ERROR"));
+			s = buf+sizeof("ERROR")-1;
+			if (*s == ' ')
+				s++;
+			DPRINTF("found err marker: %s\n",s);
+			snprintf(ami_id_buf,sizeof(ami_id_buf),"failed %s",s);
 		}
 		else {
-			DPRINTF("ignoring line: <%s>\n",buf);
+			DPRINTF("<%s>\n",buf);
 		}
 	}
 	fclose(fp);
@@ -1593,18 +1609,27 @@ fs_rhevm_register (my_state *ms, const provider_t *prov, const char *next,
 	DPRINTF("waiting for child...\n");
 	if (waitpid(pid,&wstat,0) < 0) {
 		error (0, errno, _("waitpid failed"));
+		strcpy(ami_id_buf,"failed (waitpid)");
 		goto cleanup;
 	}
 	if (!WIFEXITED(wstat)) {
 		error (0, 0, _("%s is killed (status 0x%x)"), cmd, wstat);
-		goto cleanup;
 	}
-	if (WEXITSTATUS(wstat)) {
+	else if (WEXITSTATUS(wstat)) {
 		error (0, 0, _("%s exited with code %d"),
 		       cmd, WEXITSTATUS(wstat));
-		goto cleanup;
 	}
-	DPRINTF("...child exited\n");
+	else {
+		DPRINTF("...child exited\n");
+	}
+
+	if (strncmp(ami_id_buf,"pending",sizeof("pending")-1)==0) {
+		/*
+		 * Unfortunately "pending" may get stuck if dc-rhev-image
+		 * bums on us by crashing or otherwise fails to post result.
+		 */
+		strcpy(ami_id_buf,"failed (back-end)");
+	}
 
 cleanup:
 	/*
@@ -1697,6 +1722,7 @@ fs_condor_register (my_state *ms, const provider_t *prov, const char *next,
 
 	if (pipe(organ) < 0) {
 		error (0, errno, _("pipe creation failed"));
+		strcpy(ami_id_buf,"failed (pipe)");
 		goto cleanup;
 	}
 
@@ -1705,6 +1731,7 @@ fs_condor_register (my_state *ms, const provider_t *prov, const char *next,
 		error (0, errno, _("fork failed"));
 		close(organ[0]);
 		close(organ[1]);
+		strcpy(ami_id_buf,"failed (fork)");
 		goto cleanup;
 	}
 
@@ -1720,6 +1747,7 @@ fs_condor_register (my_state *ms, const provider_t *prov, const char *next,
 	if (!fp) {
 		error (0, 0, _("could not open parent pipe stream"));
 		close(organ[0]);
+		strcpy(ami_id_buf,"failed (fdopen)");
 		goto cleanup;
 	}
 	while (fgets(buf,sizeof(buf)-1,fp)) {
@@ -1727,12 +1755,13 @@ fs_condor_register (my_state *ms, const provider_t *prov, const char *next,
 			*s = '\0';
 		if (regexec(&s3_success_pat,buf,2,match,0) == 0) {
 			buf[match[1].rm_eo] = '\0';
-			DPRINTF("found AMI ID: %s\n",buf+match[1].rm_so);
-			sprintf(ami_id_buf,"OK %.60s",buf+match[1].rm_so);
+			s = buf+match[1].rm_so;
+			DPRINTF("found image UUID: %s\n",s);
+			snprintf(ami_id_buf,sizeof(ami_id_buf),"OK %s",s);
 			rc = MHD_HTTP_OK;
 		}
 		else {
-			DPRINTF("ignoring line: <%s>\n",buf);
+			DPRINTF("<%s>\n",buf);
 		}
 	}
 	fclose(fp);
@@ -1740,18 +1769,23 @@ fs_condor_register (my_state *ms, const provider_t *prov, const char *next,
 	DPRINTF("waiting for child...\n");
 	if (waitpid(pid,&wstat,0) < 0) {
 		error (0, errno, _("waitpid failed"));
+		strcpy(ami_id_buf,"failed (waitpid)");
 		goto cleanup;
 	}
 	if (!WIFEXITED(wstat)) {
 		error (0, 0, _("%s is killed (status 0x%x)"), cmd, wstat);
-		goto cleanup;
 	}
-	if (WEXITSTATUS(wstat)) {
+	else if (WEXITSTATUS(wstat)) {
 		error (0, 0, _("%s exited with code %d"),
 		       cmd, WEXITSTATUS(wstat));
-		goto cleanup;
 	}
-	DPRINTF("...child exited\n");
+	else {
+		DPRINTF("...child exited\n");
+	}
+
+	if (strncmp(ami_id_buf,"pending",sizeof("pending")-1)==0) {
+		strcpy(ami_id_buf,"failed (back-end)");
+	}
 
 cleanup:
 	(void)meta_set_value(ms->bucket,ms->key,"ami-id",ami_id_buf);
@@ -1883,12 +1917,13 @@ fs_vmw_register (my_state *ms, const provider_t *prov, const char *next,
 			*s = '\0';
 		if (regexec(&s3_success_pat,buf,2,match,0) == 0) {
 			buf[match[1].rm_eo] = '\0';
-			DPRINTF("found image tag: %s\n",buf+match[1].rm_so);
-			sprintf(ami_id_buf,"OK %.60s",buf+match[1].rm_so);
+			s = buf+match[1].rm_so;
+			DPRINTF("found image tag: %s\n",s);
+			snprintf(ami_id_buf,sizeof(ami_id_buf),"OK %s",s);
 			ret = MHD_HTTP_OK;
 		}
 		else {
-			DPRINTF("ignoring line: <%s>\n",buf);
+			DPRINTF("<%s>\n",buf);
 		}
 	}
 	fclose(fp);
@@ -1896,18 +1931,23 @@ fs_vmw_register (my_state *ms, const provider_t *prov, const char *next,
 	DPRINTF("waiting for child...\n");
 	if (waitpid(pid,&wstat,0) < 0) {
 		error (0, errno, _("waitpid failed"));
+		strcpy(ami_id_buf,"failed (waitpid)");
 		goto cleanup;
 	}
 	if (!WIFEXITED(wstat)) {
 		error (0, 0, _("%s is killed (status 0x%x)"), cmd, wstat);
-		goto cleanup;
 	}
-	if (WEXITSTATUS(wstat)) {
+	else if (WEXITSTATUS(wstat)) {
 		error (0, 0, _("%s exited with code %d"),
 		       cmd, WEXITSTATUS(wstat));
-		goto cleanup;
 	}
-	DPRINTF("...child exited\n");
+	else {
+		DPRINTF("...child exited\n");
+	}
+
+	if (strncmp(ami_id_buf,"pending",sizeof("pending")-1)==0) {
+		strcpy(ami_id_buf,"failed (back-end)");
+	}
 
 cleanup:
 	free(vm_image);



More information about the iwhd-devel mailing list