diff mbox

[RFC] discover/pxe-parser: Retrieve configs asynchronously

Message ID 1463554362-17320-1-git-send-email-sam@mendozajonas.com
State Superseded
Headers show

Commit Message

Sam Mendoza-Jonas May 18, 2016, 6:52 a.m. UTC 
Depending on the configuration of the DHCP server and the network, tftp
requests made by the pxe parser can timeout. The pxe parser makes these
requests synchronously so several timeouts can block the server
completely for several minutes, leaving the server unresponsive to UI
requests.

Rework the pxe parser such that it handles the result of each tftp
request in a callback, which can complete after iterate_parsers() has
returned. A discover_context and conf_context are cloned for each
request so that the callbacks can handle new boot options even after the
iterate loop has completed.

Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
---
 discover/pxe-parser.c | 143 ++++++++++++++++++++++++++++++++++++--------------
 lib/url/url.c         |   2 +-
 lib/url/url.h         |   1 +
 3 files changed, 105 insertions(+), 41 deletions(-)

Comments

Jeremy Kerr June 13, 2016, 8:47 a.m. UTC | #1 
Hi Sam,

> +	/*
> +	 * Retrieving PXE configs over the network can take some time depending
> +	 * on factors such as slow network, malformed paths, bad DNS, and
> +	 * overzealous firewalls. Instead of blocking the discover server while
> +	 * we wait for these, spawn an asynchronous job for each URL we can
> +	 * parse and process the resulting files in a callback.
> +	 * Since the caller will free the discover_context once
> +	 * iterate_parsers() completes, we need to copy the discover_context
> +	 * under a different talloc context so that it survives until the
> +	 * callback it finished with it.
> +	 */

This sounds a little dangerous - there may be other allocations made
under the original discover_context that get freed after
iterate_parsers() returns there.

Can we either:

 - have this code create a second reference to the talloc-ed
   discover_context, so that it doesn't get freed until both references
   have finished using the context (which may require checking that
   talloc_ref and friends work the way we assume), or

 - rethink the lifetime of the contexts, so that we only free once we
   know that the context is unused.

Cheers,


Jeremy
Sam Mendoza-Jonas June 14, 2016, 1 a.m. UTC | #2 
On Mon, Jun 13, 2016 at 06:47:01PM +1000, Jeremy Kerr wrote:
> Hi Sam,
> 
> > +	/*
> > +	 * Retrieving PXE configs over the network can take some time depending
> > +	 * on factors such as slow network, malformed paths, bad DNS, and
> > +	 * overzealous firewalls. Instead of blocking the discover server while
> > +	 * we wait for these, spawn an asynchronous job for each URL we can
> > +	 * parse and process the resulting files in a callback.
> > +	 * Since the caller will free the discover_context once
> > +	 * iterate_parsers() completes, we need to copy the discover_context
> > +	 * under a different talloc context so that it survives until the
> > +	 * callback it finished with it.
> > +	 */
> 
> This sounds a little dangerous - there may be other allocations made
> under the original discover_context that get freed after
> iterate_parsers() returns there.

Yeah it certainly blew up a few times before it was ready :) I've sanity
checked the lifetimes of what happens with each discover context to the
point that I'm fairly happy with it - but it still leaves it a bit
vulnerable to future changes.

> 
> Can we either:
> 
>  - have this code create a second reference to the talloc-ed
>    discover_context, so that it doesn't get freed until both references
>    have finished using the context (which may require checking that
>    talloc_ref and friends work the way we assume), or

I like this, sounds it should be fairly straightforward with
talloc_reference and talloc_free/talloc_unlink, I'll give it a shot.

> 
>  - rethink the lifetime of the contexts, so that we only free once we
>    know that the context is unused.
> 
> Cheers,
> 
> 
> Jeremy
diff mbox

Patch

diff --git a/discover/pxe-parser.c b/discover/pxe-parser.c
index 95547c3..557e9ee 100644
--- a/discover/pxe-parser.c
+++ b/discover/pxe-parser.c
@@ -8,6 +8,7 @@ 
 #include <talloc/talloc.h>
 #include <url/url.h>
 #include <log/log.h>
+#include <file/file.h>
 
 #include "parser.h"
 #include "parser-conf.h"
@@ -193,87 +194,149 @@  static void pxe_process_pair(struct conf_context *ctx,
 
 }
 
-static int pxe_parse(struct discover_context *dc)
+static void pxe_conf_parse_cb(struct load_url_result *result, void *data)
 {
-	struct pb_url *pxe_base_url, *url;
-	struct pxe_parser_info *parser_info;
-	char **pxe_conf_files, **filename;
-	struct conf_context *conf;
-	bool complete_url;
+	struct conf_context *conf = data;
+	struct device_handler *handler;
+	char *buf = NULL;
 	int len, rc;
-	char *buf;
 
-	/* Expects dhcp event parameters to support network boot */
-	if (!dc->event)
-		return -1;
+	if (!data)
+		return;
+
+	if (!result || result->status != LOAD_OK) {
+		pb_log("Failed to load remote file '%s'\n", conf->dc->conf_url->full);
+		return;
+	}
+
+	rc = read_file(conf, result->local, &buf, &len);
+	if (rc) {
+		pb_log("Read failed during pxe callback for %s\n",
+		       result->local);
+		goto out_clean;
+	}
+
+	conf_parse_buf(conf, buf, len);
+
+	handler = talloc_parent(conf);
+	device_handler_discover_context_commit(handler, conf->dc);
+
+	talloc_free(buf);
+out_clean:
+	talloc_free(conf);
+	if (result->cleanup_local)
+		unlink(result->local);
+}
+
+static struct conf_context *copy_context(void *ctx,
+					   struct discover_context *orig)
+{
+	struct conf_context *conf;
+	struct discover_context *dc;
+	struct pxe_parser_info *info;
 
-	conf = talloc_zero(dc, struct conf_context);
+	conf = talloc_zero(ctx, struct conf_context);
 
 	if (!conf)
-		goto out;
+		return NULL;
+
+	dc = talloc(conf, struct discover_context);
+	dc->device = orig->device;
+	dc->network = orig->network;
+	dc->event = orig->event;
+	list_init(&dc->boot_options);
 
 	conf->dc = dc;
 	conf->get_pair = conf_get_pair_space;
 	conf->process_pair = pxe_process_pair;
 	conf->finish = pxe_finish;
 
-	parser_info = talloc_zero(conf, struct pxe_parser_info);
-	conf->parser_info = parser_info;
+	info = talloc_zero(conf, struct pxe_parser_info);
+	conf->parser_info = info;
+
+	return conf;
+}
+
+static int pxe_parse(struct discover_context *dc)
+{
+	struct pb_url *conf_url, *pxe_base_url, *url;
+	char **pxe_conf_files, **filename;
+	void *ctx = talloc_parent(dc);
+	struct load_url_result *result;
+	struct conf_context *conf;
+	bool complete_url;
+
+	/* Expects dhcp event parameters to support network boot */
+	if (!dc->event)
+		return -1;
 
-	dc->conf_url = user_event_parse_conf_url(dc, dc->event, &complete_url);
-	if (!dc->conf_url)
+	/*
+	 * Retrieving PXE configs over the network can take some time depending
+	 * on factors such as slow network, malformed paths, bad DNS, and
+	 * overzealous firewalls. Instead of blocking the discover server while
+	 * we wait for these, spawn an asynchronous job for each URL we can
+	 * parse and process the resulting files in a callback.
+	 * Since the caller will free the discover_context once
+	 * iterate_parsers() completes, we need to copy the discover_context
+	 * under a different talloc context so that it survives until the
+	 * callback it finished with it.
+	 */
+
+	conf = copy_context(ctx, dc);
+	conf_url = user_event_parse_conf_url(conf->dc,
+						conf->dc->event, &complete_url);
+	if (!conf_url)
 		goto out_conf;
+	conf->dc->conf_url = conf_url;
 
 	if (complete_url) {
 		/* we have a complete URL; use this and we're done. */
-		rc = parser_request_url(dc, dc->conf_url, &buf, &len);
-		if (rc)
+		result = load_url_async(conf->dc, conf->dc->conf_url,
+					pxe_conf_parse_cb, conf);
+		if (!result) {
+			pb_log("load_url_async fails for %s\n", conf_url->path);
 			goto out_conf;
+		}
 	} else {
-		pxe_conf_files = user_event_parse_conf_filenames(dc, dc->event);
+		pxe_conf_files = user_event_parse_conf_filenames(conf->dc,
+								conf->dc->event);
 		if (!pxe_conf_files)
 			goto out_conf;
 
-		rc = -1;
-
-		pxe_base_url = pb_url_join(dc, dc->conf_url, pxelinux_prefix);
+		pxe_base_url = pb_url_join(conf->dc, conf->dc->conf_url,
+					   pxelinux_prefix);
 		if (!pxe_base_url)
 			goto out_pxe_conf;
 
 		for (filename = pxe_conf_files; *filename; filename++) {
-			url = pb_url_join(dc, pxe_base_url, *filename);
+			if (!conf) {
+				conf = copy_context(ctx, dc);
+				conf->dc->conf_url = pb_url_copy(conf->dc, conf_url);
+			}
+			url = pb_url_join(conf->dc, pxe_base_url, *filename);
 			if (!url)
 				continue;
 
-			rc = parser_request_url(dc, url, &buf, &len);
-			if (!rc) /* found one, just break */
-				break;
-
-			talloc_free(url);
+			result = load_url_async(conf, url, pxe_conf_parse_cb, conf);
+			if (!result) {
+				pb_log("load_url_async fails for %s\n",
+				       conf->dc->conf_url->path);
+				goto out_pxe_conf;
+			}
+			/* conf now needed by callback, don't reuse */
+			conf = NULL;
 		}
 
 		talloc_free(pxe_base_url);
-
-		/* No configuration file found on the boot server */
-		if (rc)
-			goto out_pxe_conf;
-
 		talloc_free(pxe_conf_files);
 	}
 
-	/* Call the config file parser with the data read from the file */
-	conf_parse_buf(conf, buf, len);
-
-	talloc_free(buf);
-	talloc_free(conf);
-
 	return 0;
 
 out_pxe_conf:
 	talloc_free(pxe_conf_files);
 out_conf:
 	talloc_free(conf);
-out:
 	return -1;
 }
 
diff --git a/lib/url/url.c b/lib/url/url.c
index 7202f49..6eeced3 100644
--- a/lib/url/url.c
+++ b/lib/url/url.c
@@ -246,7 +246,7 @@  static void pb_url_update_full(struct pb_url *url)
 	url->full = pb_url_to_string(url);
 }
 
-static struct pb_url *pb_url_copy(void *ctx, const struct pb_url *url)
+struct pb_url *pb_url_copy(void *ctx, const struct pb_url *url)
 {
 	struct pb_url *new_url;
 
diff --git a/lib/url/url.h b/lib/url/url.h
index 25e1ad8..9043615 100644
--- a/lib/url/url.h
+++ b/lib/url/url.h
@@ -62,6 +62,7 @@  struct pb_url {
 
 bool is_url(const char *str);
 struct pb_url *pb_url_parse(void *ctx, const char *url_str);
+struct pb_url *pb_url_copy(void *ctx, const struct pb_url *url);
 struct pb_url *pb_url_join(void *ctx, const struct pb_url *url, const char *s);
 char *pb_url_to_string(struct pb_url *url);