diff mbox series

[1/5] diskpart handler: refactor: extract format_parts function

Message ID 20240826064006.41623-2-michael.adler@siemens.com
State Accepted
Delegated to: Stefano Babic
Headers show
Series Introduce FAT Filesystem Labeling | expand

Commit Message

Michael Adler Aug. 26, 2024, 6:40 a.m. UTC 
The diskpart() function has grown quite large, making it difficult to
manage and understand. To improve readability and maintainability, this
commit extracts the `format_parts` functionality into a separate
function. This also serves as a preliminary step to add further
functionality to it.

Signed-off-by: Michael Adler <michael.adler@siemens.com>
Signed-off-by: Christian Storm <christian.storm@siemens.com>
---
 handlers/diskpart_handler.c | 100 +++++++++++++++++++-----------------
 1 file changed, 53 insertions(+), 47 deletions(-)

Comments

Stefano Babic Sept. 6, 2024, 1:28 p.m. UTC | #1 
On 26.08.24 08:40, 'Michael Adler' via swupdate wrote:
> The diskpart() function has grown quite large, making it difficult to
> manage and understand. To improve readability and maintainability, this
> commit extracts the `format_parts` functionality into a separate
> function. This also serves as a preliminary step to add further
> functionality to it.
>
> Signed-off-by: Michael Adler <michael.adler@siemens.com>
> Signed-off-by: Christian Storm <christian.storm@siemens.com>
> ---
>   handlers/diskpart_handler.c | 100 +++++++++++++++++++-----------------
>   1 file changed, 53 insertions(+), 47 deletions(-)
>
> diff --git a/handlers/diskpart_handler.c b/handlers/diskpart_handler.c
> index 982c494e..23dba4f5 100644
> --- a/handlers/diskpart_handler.c
> +++ b/handlers/diskpart_handler.c
> @@ -12,7 +12,6 @@
>   #include <unistd.h>
>   #include <string.h>
>   #include <errno.h>
> -#include <ctype.h>
>   #include <sys/file.h>
>   #include <sys/ioctl.h>
>   #include <sys/types.h>
> @@ -1153,6 +1152,56 @@ handler_release:
>   	return ret;
>   }
>
> +static int format_parts(struct hnd_priv priv, struct img_type *img, struct create_table *createtable)
> +{
> +	int ret = 0;
> +	struct partition_data *part;
> +	LIST_FOREACH(part, &priv.listparts, next)
> +	{
> +		/*
> +		 * priv.listparts counts partitions starting with 0,
> +		 * but fdisk_partname expects the first partition having
> +		 * the number 1.
> +		 */
> +		size_t partno = part->partno + 1;
> +
> +		if (!strlen(part->fstype))
> +			continue; /* Don't touch partitions without fstype */
> +
> +		char *path = NULL;
> +		char *device = NULL;
> +
> +		path = realpath(img->device, NULL);
> +		if (!path)
> +			path = strdup(img->device);
> +		device = fdisk_partname(path, partno);
> +		free(path);
> +
> +		if (!createtable->parent && !part->force) {
> +			/* Check if file system exists */
> +			ret = diskformat_fs_exists(device, part->fstype);
> +
> +			if (ret < 0) {
> +				free(device);
> +				break;
> +			}
> +
> +			if (ret) {
> +				TRACE("Found %s file system on %s, skip mkfs", part->fstype, device);
> +				ret = 0;
> +				free(device);
> +				continue;
> +			}
> +		}
> +
> +		ret = diskformat_mkfs(device, part->fstype);
> +		free(device);
> +		if (ret)
> +			break;
> +	}
> +	return ret;
> +}
> +
>   static int diskpart(struct img_type *img,
>   	void __attribute__ ((__unused__)) *data)
>   {
> @@ -1409,59 +1458,16 @@ handler_release:
>   #ifdef CONFIG_DISKPART_FORMAT
>   	/* Create filesystems */
>   	if (!ret) {
> -		LIST_FOREACH(part, &priv.listparts, next) {
> -			/*
> -			 * priv.listparts counts partitions starting with 0,
> -			 * but fdisk_partname expects the first partition having
> -			 * the number 1.
> -			 */
> -			size_t partno = part->partno + 1;
> -
> -			if (!strlen(part->fstype))
> -				continue;  /* Don't touch partitions without fstype */
> -
> -			char *path = NULL;
> -			char *device = NULL;
> -
> -			path = realpath(img->device, NULL);
> -			if (!path)
> -				path = strdup(img->device);
> -			device = fdisk_partname(path, partno);
> -			free(path);
> -
> -			if (!createtable->parent && !part->force) {
> -				/* Check if file system exists */
> -				ret = diskformat_fs_exists(device, part->fstype);
> -
> -				if (ret < 0) {
> -					free(device);
> -					break;
> -				}
> -
> -				if (ret) {
> -					TRACE("Found %s file system on %s, skip mkfs",
> -						  part->fstype, device);
> -					ret = 0;
> -					free(device);
> -					continue;
> -				}
> -			}
> -
> -			ret = diskformat_mkfs(device, part->fstype);
> -			free(device);
> -			if (ret)
> -				break;
> -		}
> +		ret = format_parts(priv, img, createtable);
>   	}
>   #endif
>
> +	/* cleanup */
>   	LIST_FOREACH_SAFE(part, &priv.listparts, next, tmp) {
>   		LIST_REMOVE(part, next);
>   		free(part);
>   	}
> -
> -	if (createtable)
> -		free(createtable);
> +	free(createtable);
>
>   	/*
>   	 * Declare that handler has finished

Reviewed-by: Stefano Babic <stefano.babic@swupdate.org>
diff mbox series

Patch

diff --git a/handlers/diskpart_handler.c b/handlers/diskpart_handler.c
index 982c494e..23dba4f5 100644
--- a/handlers/diskpart_handler.c
+++ b/handlers/diskpart_handler.c
@@ -12,7 +12,6 @@ 
 #include <unistd.h>
 #include <string.h>
 #include <errno.h>
-#include <ctype.h>
 #include <sys/file.h>
 #include <sys/ioctl.h>
 #include <sys/types.h>
@@ -1153,6 +1152,56 @@  handler_release:
 	return ret;
 }
 
+static int format_parts(struct hnd_priv priv, struct img_type *img, struct create_table *createtable)
+{
+	int ret = 0;
+	struct partition_data *part;
+	LIST_FOREACH(part, &priv.listparts, next)
+	{
+		/*
+		 * priv.listparts counts partitions starting with 0,
+		 * but fdisk_partname expects the first partition having
+		 * the number 1.
+		 */
+		size_t partno = part->partno + 1;
+
+		if (!strlen(part->fstype))
+			continue; /* Don't touch partitions without fstype */
+
+		char *path = NULL;
+		char *device = NULL;
+
+		path = realpath(img->device, NULL);
+		if (!path)
+			path = strdup(img->device);
+		device = fdisk_partname(path, partno);
+		free(path);
+
+		if (!createtable->parent && !part->force) {
+			/* Check if file system exists */
+			ret = diskformat_fs_exists(device, part->fstype);
+
+			if (ret < 0) {
+				free(device);
+				break;
+			}
+
+			if (ret) {
+				TRACE("Found %s file system on %s, skip mkfs", part->fstype, device);
+				ret = 0;
+				free(device);
+				continue;
+			}
+		}
+
+		ret = diskformat_mkfs(device, part->fstype);
+		free(device);
+		if (ret)
+			break;
+	}
+	return ret;
+}
+
 static int diskpart(struct img_type *img,
 	void __attribute__ ((__unused__)) *data)
 {
@@ -1409,59 +1458,16 @@  handler_release:
 #ifdef CONFIG_DISKPART_FORMAT
 	/* Create filesystems */
 	if (!ret) {
-		LIST_FOREACH(part, &priv.listparts, next) {
-			/*
-			 * priv.listparts counts partitions starting with 0,
-			 * but fdisk_partname expects the first partition having
-			 * the number 1.
-			 */
-			size_t partno = part->partno + 1;
-
-			if (!strlen(part->fstype))
-				continue;  /* Don't touch partitions without fstype */
-
-			char *path = NULL;
-			char *device = NULL;
-
-			path = realpath(img->device, NULL);
-			if (!path)
-				path = strdup(img->device);
-			device = fdisk_partname(path, partno);
-			free(path);
-
-			if (!createtable->parent && !part->force) {
-				/* Check if file system exists */
-				ret = diskformat_fs_exists(device, part->fstype);
-
-				if (ret < 0) {
-					free(device);
-					break;
-				}
-
-				if (ret) {
-					TRACE("Found %s file system on %s, skip mkfs",
-						  part->fstype, device);
-					ret = 0;
-					free(device);
-					continue;
-				}
-			}
-
-			ret = diskformat_mkfs(device, part->fstype);
-			free(device);
-			if (ret)
-				break;
-		}
+		ret = format_parts(priv, img, createtable);
 	}
 #endif
 
+	/* cleanup */
 	LIST_FOREACH_SAFE(part, &priv.listparts, next, tmp) {
 		LIST_REMOVE(part, next);
 		free(part);
 	}
-
-	if (createtable)
-		free(createtable);
+	free(createtable);
 
 	/*
 	 * Declare that handler has finished