diff mbox series

[05/14] discover/grub2: expose a struct for grub2 file references

Message ID 20191120024306.16526-6-jk@ozlabs.org
State Accepted
Headers show
Series discover/grub2: Add support for grub2 file specifiers | expand

Commit Message

Jeremy Kerr Nov. 20, 2019, 2:42 a.m. UTC
Currently, we have struct grub2_resource_info to keep references to boot
payloads that may be returned in boot options, and be (conditionally)
resolved by the parser.

We'd also like to use the same semantics for other file references in
the grub2 parser, for arbitrary usage in scripts - where files are
also referenced by a path and an optional device.

To do this, this change moves struct grub2_resource_info to grub2.h, and
renames to struct grub2_file. Future changes will use this for
script-internal file handling.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
---
 discover/grub2/grub2.c | 24 +++++++++---------------
 discover/grub2/grub2.h |  8 ++++++++
 2 files changed, 17 insertions(+), 15 deletions(-)

Comments

samjonas Dec. 2, 2019, 5:47 a.m. UTC | #1
On Wed, 2019-11-20 at 10:42 +0800, Jeremy Kerr wrote:
> Currently, we have struct grub2_resource_info to keep references to
> boot
> payloads that may be returned in boot options, and be (conditionally)
> resolved by the parser.
> 
> We'd also like to use the same semantics for other file references in
> the grub2 parser, for arbitrary usage in scripts - where files are
> also referenced by a path and an optional device.
> 
> To do this, this change moves struct grub2_resource_info to grub2.h,
> and
> renames to struct grub2_file. Future changes will use this for
> script-internal file handling.
> 
> Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
> ---
>  discover/grub2/grub2.c | 24 +++++++++---------------
>  discover/grub2/grub2.h |  8 ++++++++
>  2 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/discover/grub2/grub2.c b/discover/grub2/grub2.c
> index f62ccdd..412298b 100644
> --- a/discover/grub2/grub2.c
> +++ b/discover/grub2/grub2.c
> @@ -33,17 +33,12 @@ static const char *const grub2_conf_files[] = {
>  	NULL
>  };
>  
> -struct grub2_resource_info {
> -	char *root;
> -	char *path;
> -};
> -
>  /* we use slightly different resources for grub2 */
>  struct resource *create_grub2_resource(struct discover_boot_option
> *opt,
>  		struct discover_device *orig_device,
>  		const char *root, const char *path)
>  {
> -	struct grub2_resource_info *info;
> +	struct grub2_file *file;
>  	struct resource *res;
>  
>  	if (strstr(path, "://")) {
> @@ -55,13 +50,12 @@ struct resource *create_grub2_resource(struct
> discover_boot_option *opt,
>  	res = talloc(opt, struct resource);
>  
>  	if (root) {
> -		info = talloc(res, struct grub2_resource_info);
> -		talloc_reference(info, root);
> -		info->root = talloc_strdup(info, root);
> -		info->path = talloc_strdup(info, path);
> +		file = talloc(res, struct grub2_file);
> +		file->dev = talloc_strdup(file, root);
> +		file->path = talloc_strdup(file, path);

Should be fine I think but caught my eye; do we lose anything by
dropping that talloc_reference()?

>  
>  		res->resolved = false;
> -		res->info = info;
> +		res->info = file;
>  
>  	} else
>  		resolve_resource_against_device(res, orig_device,
> path);
> @@ -72,18 +66,18 @@ struct resource *create_grub2_resource(struct
> discover_boot_option *opt,
>  bool resolve_grub2_resource(struct device_handler *handler,
>  		struct resource *res)
>  {
> -	struct grub2_resource_info *info = res->info;
> +	struct grub2_file *file = res->info;
>  	struct discover_device *dev;
>  
>  	assert(!res->resolved);
>  
> -	dev = device_lookup_by_uuid(handler, info->root);
> +	dev = device_lookup_by_uuid(handler, file->dev);
>  
>  	if (!dev)
>  		return false;
>  
> -	resolve_resource_against_device(res, dev, info->path);
> -	talloc_free(info);
> +	resolve_resource_against_device(res, dev, file->path);
> +	talloc_free(file);
>  
>  	return true;
>  }
> diff --git a/discover/grub2/grub2.h b/discover/grub2/grub2.h
> index 68176fb..73d91b2 100644
> --- a/discover/grub2/grub2.h
> +++ b/discover/grub2/grub2.h
> @@ -107,6 +107,14 @@ struct grub2_parser {
>  	bool			inter_word;
>  };
>  
> +/* References to files in grub2 consist of an optional device and a
> path
> + * (specified here by UUID). If the dev is unspecified, we fall back
> to a
> + * default - usually the 'root' environment variable. */
> +struct grub2_file {
> +	char *dev;
> +	char *path;
> +};
> +
>  /* type for builtin functions */
>  typedef int (*grub2_function)(struct grub2_script *script, void
> *data,
>  				int argc, char *argv[]);
Jeremy Kerr Dec. 2, 2019, 6:15 a.m. UTC | #2
Hi Sam,

> > @@ -55,13 +50,12 @@ struct resource *create_grub2_resource(struct
> > discover_boot_option *opt,
> >  	res = talloc(opt, struct resource);
> >  
> >  	if (root) {
> > -		info = talloc(res, struct grub2_resource_info);
> > -		talloc_reference(info, root);
> > -		info->root = talloc_strdup(info, root);
> > -		info->path = talloc_strdup(info, path);
> > +		file = talloc(res, struct grub2_file);
> > +		file->dev = talloc_strdup(file, root);
> > +		file->path = talloc_strdup(file, path);
> 
> Should be fine I think but caught my eye; do we lose anything by
> dropping that talloc_reference()?

That wasn't needed in the first place - since we talloc_strdup() root,
we never needed an additional reference to it. It looks like I should
have removed that in 1513dd5d0, where we changed from using the root var
to the strdup.

Cheers,


Jeremy
diff mbox series

Patch

diff --git a/discover/grub2/grub2.c b/discover/grub2/grub2.c
index f62ccdd..412298b 100644
--- a/discover/grub2/grub2.c
+++ b/discover/grub2/grub2.c
@@ -33,17 +33,12 @@  static const char *const grub2_conf_files[] = {
 	NULL
 };
 
-struct grub2_resource_info {
-	char *root;
-	char *path;
-};
-
 /* we use slightly different resources for grub2 */
 struct resource *create_grub2_resource(struct discover_boot_option *opt,
 		struct discover_device *orig_device,
 		const char *root, const char *path)
 {
-	struct grub2_resource_info *info;
+	struct grub2_file *file;
 	struct resource *res;
 
 	if (strstr(path, "://")) {
@@ -55,13 +50,12 @@  struct resource *create_grub2_resource(struct discover_boot_option *opt,
 	res = talloc(opt, struct resource);
 
 	if (root) {
-		info = talloc(res, struct grub2_resource_info);
-		talloc_reference(info, root);
-		info->root = talloc_strdup(info, root);
-		info->path = talloc_strdup(info, path);
+		file = talloc(res, struct grub2_file);
+		file->dev = talloc_strdup(file, root);
+		file->path = talloc_strdup(file, path);
 
 		res->resolved = false;
-		res->info = info;
+		res->info = file;
 
 	} else
 		resolve_resource_against_device(res, orig_device, path);
@@ -72,18 +66,18 @@  struct resource *create_grub2_resource(struct discover_boot_option *opt,
 bool resolve_grub2_resource(struct device_handler *handler,
 		struct resource *res)
 {
-	struct grub2_resource_info *info = res->info;
+	struct grub2_file *file = res->info;
 	struct discover_device *dev;
 
 	assert(!res->resolved);
 
-	dev = device_lookup_by_uuid(handler, info->root);
+	dev = device_lookup_by_uuid(handler, file->dev);
 
 	if (!dev)
 		return false;
 
-	resolve_resource_against_device(res, dev, info->path);
-	talloc_free(info);
+	resolve_resource_against_device(res, dev, file->path);
+	talloc_free(file);
 
 	return true;
 }
diff --git a/discover/grub2/grub2.h b/discover/grub2/grub2.h
index 68176fb..73d91b2 100644
--- a/discover/grub2/grub2.h
+++ b/discover/grub2/grub2.h
@@ -107,6 +107,14 @@  struct grub2_parser {
 	bool			inter_word;
 };
 
+/* References to files in grub2 consist of an optional device and a path
+ * (specified here by UUID). If the dev is unspecified, we fall back to a
+ * default - usually the 'root' environment variable. */
+struct grub2_file {
+	char *dev;
+	char *path;
+};
+
 /* type for builtin functions */
 typedef int (*grub2_function)(struct grub2_script *script, void *data,
 				int argc, char *argv[]);