Patchwork [4/6] fs/ext2: use the ext2 variant to name the generated rootfs image

login
register
mail settings
Submitter Yann E. MORIN
Date March 7, 2013, 10:04 p.m.
Message ID <1d86b1b76a2c56aaf0098ec389e570862e7aa3fb.1362693453.git.yann.morin.1998@free.fr>
Download mbox | patch
Permalink /patch/225981/
State Changes Requested
Headers show

Comments

Yann E. MORIN - March 7, 2013, 10:04 p.m.
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Arnout Vandecappelle <arnout@mind.be>
---
 fs/ext2/ext2.mk |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Thomas Petazzoni - March 10, 2013, 1:55 p.m.
Dear Yann E. MORIN,

On Thu,  7 Mar 2013 23:04:41 +0100, Yann E. MORIN wrote:
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Arnout Vandecappelle <arnout@mind.be>
> ---
>  fs/ext2/ext2.mk |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/ext2/ext2.mk b/fs/ext2/ext2.mk
> index 1660d9c..57acad7 100644
> --- a/fs/ext2/ext2.mk
> +++ b/fs/ext2/ext2.mk
> @@ -29,4 +29,4 @@ define ROOTFS_EXT2_CMD
>  	PATH=$(TARGET_PATH) $(EXT2_ENV) fs/ext2/genext2fs.sh -d $(TARGET_DIR) $(EXT2_OPTS) $@
>  endef
>  
> -$(eval $(call ROOTFS_TARGET,ext2))
> +$(eval $(call ROOTFS_TARGET,ext2,ext$(BR2_TARGET_ROOTFS_EXT2_GEN)))

It is not a very strong opinion here, but I'm not sure I like this idea
of the file extension being dependent on BR2_TARGET_ROOTFS_EXT2_GEN.

I think I would have preferred something that renames the filesystem
to:

$(eval $(call ROOTFS_TARGET,ext))

which generates a rootfs.ext image, with a compatibility symbolic link
ext2 -> ext. This can for example be done in a
ROOTFS_EXT_POST_GEN_HOOKS.

Best regards,

Thomas
Arnout Vandecappelle - March 12, 2013, 5:42 p.m.
On 03/10/13 14:55, Thomas Petazzoni wrote:
> Dear Yann E. MORIN,
>
> On Thu,  7 Mar 2013 23:04:41 +0100, Yann E. MORIN wrote:
>> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
>> Cc: Arnout Vandecappelle <arnout@mind.be>
>> ---
>>   fs/ext2/ext2.mk |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/ext2/ext2.mk b/fs/ext2/ext2.mk
>> index 1660d9c..57acad7 100644
>> --- a/fs/ext2/ext2.mk
>> +++ b/fs/ext2/ext2.mk
>> @@ -29,4 +29,4 @@ define ROOTFS_EXT2_CMD
>>   	PATH=$(TARGET_PATH) $(EXT2_ENV) fs/ext2/genext2fs.sh -d $(TARGET_DIR) $(EXT2_OPTS) $@
>>   endef
>>
>> -$(eval $(call ROOTFS_TARGET,ext2))
>> +$(eval $(call ROOTFS_TARGET,ext2,ext$(BR2_TARGET_ROOTFS_EXT2_GEN)))
>
> It is not a very strong opinion here, but I'm not sure I like this idea
> of the file extension being dependent on BR2_TARGET_ROOTFS_EXT2_GEN.
>
> I think I would have preferred something that renames the filesystem
> to:
>
> $(eval $(call ROOTFS_TARGET,ext))
>
> which generates a rootfs.ext image, with a compatibility symbolic link
> ext2 -> ext. This can for example be done in a
> ROOTFS_EXT_POST_GEN_HOOKS.

  That sounds like a good idea to me.

  Regards,
  Arnout
Yann E. MORIN - March 12, 2013, 10:51 p.m.
Thomas, All,

On Sunday 10 March 2013 Thomas Petazzoni wrote:
> On Thu,  7 Mar 2013 23:04:41 +0100, Yann E. MORIN wrote:
> > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > Cc: Arnout Vandecappelle <arnout@mind.be>
> > ---
> >  fs/ext2/ext2.mk |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/fs/ext2/ext2.mk b/fs/ext2/ext2.mk
> > index 1660d9c..57acad7 100644
> > --- a/fs/ext2/ext2.mk
> > +++ b/fs/ext2/ext2.mk
> > @@ -29,4 +29,4 @@ define ROOTFS_EXT2_CMD
> >  	PATH=$(TARGET_PATH) $(EXT2_ENV) fs/ext2/genext2fs.sh -d $(TARGET_DIR) $(EXT2_OPTS) $@
> >  endef
> >  
> > -$(eval $(call ROOTFS_TARGET,ext2))
> > +$(eval $(call ROOTFS_TARGET,ext2,ext$(BR2_TARGET_ROOTFS_EXT2_GEN)))
> 
> It is not a very strong opinion here, but I'm not sure I like this idea
> of the file extension being dependent on BR2_TARGET_ROOTFS_EXT2_GEN.
> 
> I think I would have preferred something that renames the filesystem
> to:
> 
> $(eval $(call ROOTFS_TARGET,ext))
> 
> which generates a rootfs.ext image, with a compatibility symbolic link
> ext2 -> ext. This can for example be done in a
> ROOTFS_EXT_POST_GEN_HOOKS.

The problem I see with renaming the file system is that the FS infra uses
the name of the filesystem to see if it is enabled by way of:

    fs/common.mk:
    79  ifeq ($$(BR2_TARGET_ROOTFS_$(2)),y)
    80  TARGETS += rootfs-$(1)
    81  endif

where:
  - $(1) is the name of the FS as supplied to ROOTFS_TARGET
  - $(2) is the uppercase of $(1)

So, if we rename ext2 -> ext, we'd have to also rename BR2_TARGET_ROOTFS_EXT2
to BR2_TARGET_ROOTFS_EXT which was duly refused by Peter and Arnout:
    http://lists.busybox.net/pipermail/buildroot/2013-February/067587.html

    <Peter>
    Arnout's first comment was about the defconfigs /
    BR2_TARGET_ROOTFS_EXT{2,} change, which we both dislike
    </Peter>

However, Peter and Arnout expressed there interest in renaming the FS:
    <Peter>
    the 2nd was about the fs/ renames which should be transparent to
    the user.
    </Peter>

Unfortunately, it is not transparent, as it implies a rename of the kconfig
symbol, which in turn would imply fixing the defconfigs, and the boards
documentation.

True, I've reverted the whole rename without explanations. I forgot to
mention that in the series' introductory email.

So, what next? I see four options:

 0- don't rename anything, continue calling it .ext2 (although it may be
    ext2, ext3 or ext4): status-quo;
 1- only rename the fs directory, which *is* transparent
      - fs/ext2                 ->  fs/ext
 2- completely rename the filesystem:
      - fs/ext2                 ->  fs/ext
      - BR2_TARGET_ROOTFS_EXT2  ->  BR2_TARGET_ROOTFS_EXT
    but do not fix the defconfigs and boards doc. Anyway, add a deprecated
    symbol for BR2_TARGET_ROOTFS_EXT2.
 3- same as 3, but also fix defconfigs and boards doc.

For all those four options, we can add a post-fs-hook that symlinks the
image file with the correct extension.

Finally, the curent proposal (allow to specify the FS extension) is not
incompatible with any of the above four options.

So, up to you guys. ;-)

Regards,
Yann E. MORIN.
Arnout Vandecappelle - March 14, 2013, 7:22 a.m.
On 03/12/13 23:51, Yann E. MORIN wrote:
> So, what next? I see four options:
>
>   0- don't rename anything, continue calling it .ext2 (although it may be
>      ext2, ext3 or ext4): status-quo;
>   1- only rename the fs directory, which*is*  transparent
>        - fs/ext2                 ->  fs/ext
>   2- completely rename the filesystem:
>        - fs/ext2                 ->  fs/ext
>        - BR2_TARGET_ROOTFS_EXT2  ->  BR2_TARGET_ROOTFS_EXT
>      but do not fix the defconfigs and boards doc. Anyway, add a deprecated
>      symbol for BR2_TARGET_ROOTFS_EXT2.
>   3- same as 3, but also fix defconfigs and boards doc.
>
> For all those four options, we can add a post-fs-hook that symlinks the
> image file with the correct extension.
>
> Finally, the curent proposal (allow to specify the FS extension) is not
> incompatible with any of the above four options.

  I don't have a problem with the directory and the config symbol using 
ext2 - these are anyway internal kitchen. I wouldn't want to change only 
the directory name and not the config symbol, because that does make 
reading the code more difficult.

  What is important, is that the name in the images directory has the 
correct extension. This can be done either by the addition ROOTFS_TARGET 
argument, or by symlinking in a post-hook. I'm slightly in favour of the 
latter, because it is much simpler. On the other hand, Yann has done the 
change already and it may be useful for other filesystems at some point too.

  So for me, it is either Yann's implementation, or symlinking in a 
post-hook.


  BTW, note that the image still is an ext2 image (it can be read by ext2 
code). So having the rootfs.ext4 -> rootfs.ext2 symlink is not a problem 
at all.

  Regards,
  Arnout
Yann E. MORIN - March 14, 2013, 6:16 p.m.
Arnout, Thomas, All,

On Thursday 14 March 2013 Arnout Vandecappelle wrote:
> On 03/12/13 23:51, Yann E. MORIN wrote:
> > So, what next? I see four options:
> >
> >   0- don't rename anything, continue calling it .ext2 (although it may be
> >      ext2, ext3 or ext4): status-quo;
> >   1- only rename the fs directory, which*is*  transparent
> >        - fs/ext2                 ->  fs/ext
> >   2- completely rename the filesystem:
> >        - fs/ext2                 ->  fs/ext
> >        - BR2_TARGET_ROOTFS_EXT2  ->  BR2_TARGET_ROOTFS_EXT
> >      but do not fix the defconfigs and boards doc. Anyway, add a deprecated
> >      symbol for BR2_TARGET_ROOTFS_EXT2.
> >   3- same as 3, but also fix defconfigs and boards doc.
> >
> > For all those four options, we can add a post-fs-hook that symlinks the
> > image file with the correct extension.
> >
> > Finally, the curent proposal (allow to specify the FS extension) is not
> > incompatible with any of the above four options.
> 
>   I don't have a problem with the directory and the config symbol using 
> ext2 - these are anyway internal kitchen. I wouldn't want to change only 
> the directory name and not the config symbol, because that does make 
> reading the code more difficult.

OK, fair enough.

>   What is important, is that the name in the images directory has the 
> correct extension. This can be done either by the addition ROOTFS_TARGET 
> argument, or by symlinking in a post-hook. I'm slightly in favour of the 
> latter, because it is much simpler. On the other hand, Yann has done the 
> change already and it may be useful for other filesystems at some point too.
> 
>   So for me, it is either Yann's implementation, or symlinking in a 
> post-hook.

In that case, I think the post-image-hook is more appropriate.
I will rework this to use a hook.

I will also drop patch 3/6:
    fs: allow image generators to specify file-extension

>   BTW, note that the image still is an ext2 image (it can be read by ext2 
> code). So having the rootfs.ext4 -> rootfs.ext2 symlink is not a problem 
> at all.

OK, will do.

Thank you!

Regards,
Yann E. MORIN.

Patch

diff --git a/fs/ext2/ext2.mk b/fs/ext2/ext2.mk
index 1660d9c..57acad7 100644
--- a/fs/ext2/ext2.mk
+++ b/fs/ext2/ext2.mk
@@ -29,4 +29,4 @@  define ROOTFS_EXT2_CMD
 	PATH=$(TARGET_PATH) $(EXT2_ENV) fs/ext2/genext2fs.sh -d $(TARGET_DIR) $(EXT2_OPTS) $@
 endef
 
-$(eval $(call ROOTFS_TARGET,ext2))
+$(eval $(call ROOTFS_TARGET,ext2,ext$(BR2_TARGET_ROOTFS_EXT2_GEN)))