public inbox for ltp@lists.linux.it
 help / color / mirror / Atom feed
From: Cyril Hrubis <chrubis@suse.cz>
To: ltp@lists.linux.it
Subject: [LTP] [PATCH v2 4/4] lib: mkfs: use 'mke2fs' for formatting extN filesystems
Date: Mon, 4 Dec 2017 17:24:28 +0100	[thread overview]
Message-ID: <20171204162428.GB21055@rei> (raw)
In-Reply-To: <20171114021118.103786-5-sspatil@google.com>

Hi!
> @@ -43,7 +45,18 @@ void tst_mkfs_(const char *file, const int lineno, void (cleanup_fn)(void),
>  		return;
>  	}
>  
> -	snprintf(mkfs, sizeof(mkfs), "mkfs.%s", fs_type);
> +	if (tst_get_mkfs(fs_type, mkfs, NAME_MAX))
> +		tst_brkm(TCONF, cleanup_fn,
> +			 "mkfs.%s not found for %s", fs_type, fs_type);
> +
> +	if (!strcmp(mkfs, "mke2fs")) {
> +		/* insert '-t <fs_type> in arguments here */
> +		argv[pos++] = "-t";
> +		strcat(fs_opts_str, "-t ");
> +		argv[pos++] = fs_type;
> +		strcat(fs_opts_str, fs_type);
> +		strcat(fs_opts_str, " ");
> +	}

Hmm, this is kind of ugly, why do we have tst_get_mkfs() function and
then this if that fills it's arguments.

Well what about we rather exported the tst_fs_is_supported() function
and used it here before we even try to assemble the command to format
the filesystem.

I would have done it as following:

Change the is_supported() to tst_fs_is_supported() what would check for
mke2fs as a fallback command for ext*. Then the whole logic related to
mkfs would go into the tst_mkfs() function, including again the fallback
to mke2fs command for ext*.

Or we can move the ttst_supported_fs_types.c code to tst_mkfs.c (in a
separate patch) in order to get rid of the artificial boundary between
the two modules. That should free us to draft better code without a need
to draft publicly exposed interface between these two modules.

>  	if (fs_opts) {
>  		for (i = 0; fs_opts[i]; i++) {
> diff --git a/lib/tst_supported_fs_types.c b/lib/tst_supported_fs_types.c
> index a23b1ed52..a62dda0da 100644
> --- a/lib/tst_supported_fs_types.c
> +++ b/lib/tst_supported_fs_types.c
> @@ -29,31 +29,43 @@ static const char *const fs_type_whitelist[] = {
>  	"ext2",
>  	"ext3",
>  	"ext4",
> +#ifndef __ANDROID__
>  	"xfs",
>  	"btrfs",
>  	"vfat",
>  	"exfat",
>  	"ntfs",
> +#endif
>  	NULL
>  };

What is the exact reason for this ifdef? Shouldn't the code handle the
missing support gracefully even without it?

-- 
Cyril Hrubis
chrubis@suse.cz

  reply	other threads:[~2017-12-04 16:24 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-14  2:11 [LTP] [PATCH v2 0/4] Miscellaneous fixes for Android systems Sandeep Patil
2017-11-14  2:11 ` [LTP] [PATCH v2 1/4] mm: mallocstress: use new library and safe macros Sandeep Patil
2017-11-14  2:11 ` [LTP] [PATCH 2/4] checkpoint: add TST_CHECKPOINT_INIT() for new library headers Sandeep Patil
2017-11-14  2:11 ` [LTP] [PATCH v2 3/4] mm: mallocstress: use futexes instead of SysV semaphores Sandeep Patil
2017-11-20 12:36   ` Jan Stancek
2017-11-28 14:25     ` Sandeep Patil
2017-11-14  2:11 ` [LTP] [PATCH v2 4/4] lib: mkfs: use 'mke2fs' for formatting extN filesystems Sandeep Patil
2017-12-04 16:24   ` Cyril Hrubis [this message]
2018-02-22 23:11     ` Sandeep Patil
2017-12-04 16:52   ` Jan Stancek
2017-12-06 12:16     ` Cyril Hrubis

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20171204162428.GB21055@rei \
    --to=chrubis@suse.cz \
    --cc=ltp@lists.linux.it \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox