[RFC,1/2] support/scripts/genimage.sh: support creating a bmap image

Message ID 20240421095353.208034-2-dario.binacchi@amarulasolutions.com
State New
Headers show
Series
  • Add support to genimage.sh for creating a bmap image
Related show

Commit Message

Dario Binacchi April 21, 2024, 9:53 a.m. UTC
The patch adds an option to create, in addition to the usual image, an
image of type bmap that drastically reduces the amount of data that
needs to be written to an SD card, resulting in time savings.

This makes it possible to activate this option easily and maintain
backward compatibility for all configurations already using the genimage
tool for creating the image to be written to the SD card:

BR2_ROOTFS_POST_IMAGE_SCRIPT="support/scripts/genimage.sh"
+BR2_ROOTFS_POST_IMAGE_SCRIPT_ARGS="-c board/<manufacturer>/<boardname>/genimage.cfg -b"
+BR2_PACKAGE_HOST_BMAP_TOOLS=y

It follows that the script now assumes a broader functionality that
extends beyond just being an interface for the genimage tool.

An alternative implementation could have been to create another script,
such as support/scripts/bmap.sh, capable of creating the bmap image
using the same parameters passed to support/scripts/genimage.sh (i. e.
-c board/<manufacturer>/<boardname>/genimage.cfg). However, in this case,
the script would also need to know that the image is located in the
${BINARIES} directory. This could be achieved by adding an additional
parameter, but it might cause genimage.sh to fail due to an unrecognized
parameter.

Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
---
 support/scripts/genimage.sh | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

Comments

'Krzysztof Kozlowski' via Amarula Linux May 10, 2024, 7:54 p.m. UTC | #1
Hello Dario,

On Sun, 21 Apr 2024 11:53:52 +0200
Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote:

> The patch adds an option to create, in addition to the usual image, an
> image of type bmap that drastically reduces the amount of data that
> needs to be written to an SD card, resulting in time savings.
> 
> This makes it possible to activate this option easily and maintain
> backward compatibility for all configurations already using the genimage
> tool for creating the image to be written to the SD card:
> 
> BR2_ROOTFS_POST_IMAGE_SCRIPT="support/scripts/genimage.sh"
> +BR2_ROOTFS_POST_IMAGE_SCRIPT_ARGS="-c board/<manufacturer>/<boardname>/genimage.cfg -b"
> +BR2_PACKAGE_HOST_BMAP_TOOLS=y
> 
> It follows that the script now assumes a broader functionality that
> extends beyond just being an interface for the genimage tool.
> 
> An alternative implementation could have been to create another script,
> such as support/scripts/bmap.sh, capable of creating the bmap image
> using the same parameters passed to support/scripts/genimage.sh (i. e.
> -c board/<manufacturer>/<boardname>/genimage.cfg). However, in this case,
> the script would also need to know that the image is located in the
> ${BINARIES} directory. This could be achieved by adding an additional
> parameter, but it might cause genimage.sh to fail due to an unrecognized
> parameter.
> 
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>

Thanks for this proposal! Actually, I very much like the idea of
extending the usage of bmap-tools in Buildroot. I think it's a great
tool, and it makes a lot of sense to encourage more of its use.

Now the question is how to do this. To me your proposal looks
reasonable, though some people had the feedback that bmap-tools will
not work in all cases (should we then gracefully handle this failure
case?).

Arnout, Yann, Peter, Romain, what do you think? Do you see other
approaches to easily allow generating bmap-compatible images?

Another question, perhaps for Dario, or other people familiar with
genimage: if the rootfs.ext4 generated by Buildroot is sparse, is
genimage able to preserve the "holes" when generating the sdcard.img
that includes the rootfs.ext4 ?

Thanks!

Thomas
Michael Nazzareno Trimarchi May 10, 2024, 8:17 p.m. UTC | #2
Hi Thomas and all

On Fri, May 10, 2024 at 9:54 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello Dario,
>
> On Sun, 21 Apr 2024 11:53:52 +0200
> Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote:
>
> > The patch adds an option to create, in addition to the usual image, an
> > image of type bmap that drastically reduces the amount of data that
> > needs to be written to an SD card, resulting in time savings.
> >
> > This makes it possible to activate this option easily and maintain
> > backward compatibility for all configurations already using the genimage
> > tool for creating the image to be written to the SD card:
> >
> > BR2_ROOTFS_POST_IMAGE_SCRIPT="support/scripts/genimage.sh"
> > +BR2_ROOTFS_POST_IMAGE_SCRIPT_ARGS="-c board/<manufacturer>/<boardname>/genimage.cfg -b"
> > +BR2_PACKAGE_HOST_BMAP_TOOLS=y
> >
> > It follows that the script now assumes a broader functionality that
> > extends beyond just being an interface for the genimage tool.
> >
> > An alternative implementation could have been to create another script,
> > such as support/scripts/bmap.sh, capable of creating the bmap image
> > using the same parameters passed to support/scripts/genimage.sh (i. e.
> > -c board/<manufacturer>/<boardname>/genimage.cfg). However, in this case,
> > the script would also need to know that the image is located in the
> > ${BINARIES} directory. This could be achieved by adding an additional
> > parameter, but it might cause genimage.sh to fail due to an unrecognized
> > parameter.
> >
> > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
>
> Thanks for this proposal! Actually, I very much like the idea of
> extending the usage of bmap-tools in Buildroot. I think it's a great
> tool, and it makes a lot of sense to encourage more of its use.
>
> Now the question is how to do this. To me your proposal looks
> reasonable, though some people had the feedback that bmap-tools will
> not work in all cases (should we then gracefully handle this failure
> case?).
>
> Arnout, Yann, Peter, Romain, what do you think? Do you see other
> approaches to easily allow generating bmap-compatible images?
>
> Another question, perhaps for Dario, or other people familiar with
> genimage: if the rootfs.ext4 generated by Buildroot is sparse, is
> genimage able to preserve the "holes" when generating the sdcard.img
> that includes the rootfs.ext4 ?

If you consider sparse the way that fastboot handle it, so I have
quick tested and seems
genimage can create sdcard.img and sdcard-sparse.img using a specific genconfig

I have tested with this

image sdcard.img {
    hdimage {
    }

    partition u-boot-tpl-spl-dtb {
        in-partition-table = "no"
        image = "idbloader.img"
        offset = 32K
    }

    partition u-boot-dtb {
        in-partition-table = "no"
        image = "u-boot.itb"
        offset = 8M
        size = 30M
    }

    partition rootfs {
        partition-type = 0x83
        image = "rootfs.ext4"
    }
}

image sdcard-sparse.img {
    android-sparse {
        image = sdcard.img
    }
}

output/images/sdcard.img:        DOS/MBR boot sector; partition 1 :
ID=0x83, start-CHS (0x4,215,20), end-CHS (0x51,85,4), startsector
77824, 1228800 sectors, extended partition table (last)
output/images/sdcard-sparse.img: Android sparse image, version: 1.0,
Total of 163328 4096-byte output blocks in 29 input chunks.

Michael

>
> Thanks!
>
> Thomas
> --
> Thomas Petazzoni, co-owner and CEO, Bootlin
> Embedded Linux and Kernel engineering and training
> https://bootlin.com
Dario Binacchi May 20, 2024, 8:05 a.m. UTC | #3
Hi Thomas, Gero and All

On Fri, May 10, 2024 at 9:54 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello Dario,
>
> On Sun, 21 Apr 2024 11:53:52 +0200
> Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote:
>
> > The patch adds an option to create, in addition to the usual image, an
> > image of type bmap that drastically reduces the amount of data that
> > needs to be written to an SD card, resulting in time savings.
> >
> > This makes it possible to activate this option easily and maintain
> > backward compatibility for all configurations already using the genimage
> > tool for creating the image to be written to the SD card:
> >
> > BR2_ROOTFS_POST_IMAGE_SCRIPT="support/scripts/genimage.sh"
> > +BR2_ROOTFS_POST_IMAGE_SCRIPT_ARGS="-c board/<manufacturer>/<boardname>/genimage.cfg -b"
> > +BR2_PACKAGE_HOST_BMAP_TOOLS=y
> >
> > It follows that the script now assumes a broader functionality that
> > extends beyond just being an interface for the genimage tool.
> >
> > An alternative implementation could have been to create another script,
> > such as support/scripts/bmap.sh, capable of creating the bmap image
> > using the same parameters passed to support/scripts/genimage.sh (i. e.
> > -c board/<manufacturer>/<boardname>/genimage.cfg). However, in this case,
> > the script would also need to know that the image is located in the
> > ${BINARIES} directory. This could be achieved by adding an additional
> > parameter, but it might cause genimage.sh to fail due to an unrecognized
> > parameter.
> >
> > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
>
> Thanks for this proposal! Actually, I very much like the idea of
> extending the usage of bmap-tools in Buildroot. I think it's a great
> tool, and it makes a lot of sense to encourage more of its use.
>
> Now the question is how to do this. To me your proposal looks
> reasonable, though some people had the feedback that bmap-tools will
> not work in all cases (should we then gracefully handle this failure
> case?).

If I understand correctly, the first patch in the series seems
reasonable to Gero as well.
Therefore, both of you are in favor of merging this patch. The issue
arises for Gero
regarding the application of the second patch, which I added only as
an example use case.
I think merging the first patch would give the opportunity to use
bmaptool and realize its
usefulness. After all, the patch is backward compatible. Its use could
then lead to subsequent
improvements.

Regarding the second patch, I then ask you: Is it correct not to
proceed with merging this
patch for the reason mentioned by Gero (i.e., encrypted directory)? It
seems to me that
bmaptool is used in Yocto, so is it correct to say that you would have
the same problem
with Yocto? I hope, Gero, you do not misunderstand me.

Thanks and regards,
Dario

>
> Arnout, Yann, Peter, Romain, what do you think? Do you see other
> approaches to easily allow generating bmap-compatible images?
>
> Another question, perhaps for Dario, or other people familiar with
> genimage: if the rootfs.ext4 generated by Buildroot is sparse, is
> genimage able to preserve the "holes" when generating the sdcard.img
> that includes the rootfs.ext4 ?
>
> Thanks!
>
> Thomas
> --
> Thomas Petazzoni, co-owner and CEO, Bootlin
> Embedded Linux and Kernel engineering and training
> https://bootlin.com
'Krzysztof Kozlowski' via Amarula Linux May 20, 2024, 9:23 a.m. UTC | #4
Hello Dario,

[ Your e-mail client configuration seems somewhat broken, your lines
  are badly wrapped ]

On Mon, 20 May 2024 10:05:34 +0200
Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote:

> > Now the question is how to do this. To me your proposal looks
> > reasonable, though some people had the feedback that bmap-tools will
> > not work in all cases (should we then gracefully handle this failure
> > case?).  
> 
> If I understand correctly, the first patch in the series seems
> reasonable to Gero as well.
> Therefore, both of you are in favor of merging this patch. The issue
> arises for Gero
> regarding the application of the second patch, which I added only as
> an example use case.
> I think merging the first patch would give the opportunity to use
> bmaptool and realize its
> usefulness. After all, the patch is backward compatible. Its use could
> then lead to subsequent
> improvements.
> 
> Regarding the second patch, I then ask you: Is it correct not to
> proceed with merging this
> patch for the reason mentioned by Gero (i.e., encrypted directory)? It
> seems to me that
> bmaptool is used in Yocto, so is it correct to say that you would have
> the same problem
> with Yocto? I hope, Gero, you do not misunderstand me.

Yes, the first patch is reasonable, but if we can't apply something
like the second patch, then it might mean that the approach taken in
the first patch isn't correct. Indeed, if we can't enable by default
the generation of bmap-capable images in defconfigs, it kind of limits
the usefulness of this bmap-tool integration.

I was hoping for the other BR maintainers to chime in with some opinion.

Do we have a way to detect that the bmap-tool generation will fail, and
in this case gracefully skip the bmap-tool logic?

Best regards,

Thomas
Yann E. MORIN May 21, 2024, 5:25 a.m. UTC | #5
Michael, All,

On 2024-05-10 22:17 +0200, Michael Nazzareno Trimarchi spake thusly:
> On Fri, May 10, 2024 at 9:54 PM Thomas Petazzoni
> <thomas.petazzoni@bootlin.com> wrote:
> > On Sun, 21 Apr 2024 11:53:52 +0200
> > Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote:
> > > The patch adds an option to create, in addition to the usual image, an
> > > image of type bmap that drastically reduces the amount of data that
> > > needs to be written to an SD card, resulting in time savings.
[--SNIP--]
> > Another question, perhaps for Dario, or other people familiar with
> > genimage: if the rootfs.ext4 generated by Buildroot is sparse, is
> > genimage able to preserve the "holes" when generating the sdcard.img
> > that includes the rootfs.ext4 ?

> Arnout, Yann, Peter, Romain, what do you think? Do you see other
> approaches to easily allow generating bmap-compatible images?

I wonder if we should really do that, in fact.

I believe it is in-scope that we do generate disk images, which we do
with genimage. However, transferring, distributing, writing, etc.. the
generated images I think is out of scope. We may provide tools to help
with that, though, and we do by providing {host-,}bmap-tools.

> If you consider sparse the way that fastboot handle it, so I have
> quick tested and seems
> genimage can create sdcard.img and sdcard-sparse.img using a specific genconfig

I think the question by Thomas was whether, given an input sparse file,
genimage will keep the generated image sparse in the same places the
input file was (only shifted by the offset that input file was copied
at in the image).

For example, mkfs.ext can create sparse files, and such a file can be
fed to genimage, to create a disk image. We want the generated disk
image to be sparsed in the same positions the ext2 image was, e.g. with
'X' as no-hole blocks, '.' as holes, and 'B' as the bootloader:

  rootfs.ext2        XXXXX........XXX...XXXX....XXXXX........X....
  disk.img       BBBBXXXXX........XXX...XXXX....XXXXX........X....

Note that some X block can be all-zeroes, because they are the bitmap of
the filesystem; those must not be skipped when writing to the final
storage, otherwise the filesystem would be corrupted. If genimage
decides that all-zeroes block are sparsed, then this is broken.

I think that was the question raised by Thomas, and I don't understand
how your reply answers that, so maybe I missed something...

Regards,
Yann E. MORIN.
Michael Nazzareno Trimarchi May 21, 2024, 9:28 a.m. UTC | #6
Yann, all

On Tue, May 21, 2024 at 7:25 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>
> Michael, All,
>
> On 2024-05-10 22:17 +0200, Michael Nazzareno Trimarchi spake thusly:
> > On Fri, May 10, 2024 at 9:54 PM Thomas Petazzoni
> > <thomas.petazzoni@bootlin.com> wrote:
> > > On Sun, 21 Apr 2024 11:53:52 +0200
> > > Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote:
> > > > The patch adds an option to create, in addition to the usual image, an
> > > > image of type bmap that drastically reduces the amount of data that
> > > > needs to be written to an SD card, resulting in time savings.
> [--SNIP--]
> > > Another question, perhaps for Dario, or other people familiar with
> > > genimage: if the rootfs.ext4 generated by Buildroot is sparse, is
> > > genimage able to preserve the "holes" when generating the sdcard.img
> > > that includes the rootfs.ext4 ?
>
> > Arnout, Yann, Peter, Romain, what do you think? Do you see other
> > approaches to easily allow generating bmap-compatible images?
>
> I wonder if we should really do that, in fact.
>
> I believe it is in-scope that we do generate disk images, which we do
> with genimage. However, transferring, distributing, writing, etc.. the
> generated images I think is out of scope. We may provide tools to help
> with that, though, and we do by providing {host-,}bmap-tools.
>
> > If you consider sparse the way that fastboot handle it, so I have
> > quick tested and seems
> > genimage can create sdcard.img and sdcard-sparse.img using a specific genconfig
>
> I think the question by Thomas was whether, given an input sparse file,
> genimage will keep the generated image sparse in the same places the
> input file was (only shifted by the offset that input file was copied
> at in the image).
>
> For example, mkfs.ext can create sparse files, and such a file can be
> fed to genimage, to create a disk image. We want the generated disk
> image to be sparsed in the same positions the ext2 image was, e.g. with
> 'X' as no-hole blocks, '.' as holes, and 'B' as the bootloader:
>
>   rootfs.ext2        XXXXX........XXX...XXXX....XXXXX........X....
>   disk.img       BBBBXXXXX........XXX...XXXX....XXXXX........X....
>
> Note that some X block can be all-zeroes, because they are the bitmap of
> the filesystem; those must not be skipped when writing to the final
> storage, otherwise the filesystem would be corrupted. If genimage
> decides that all-zeroes block are sparsed, then this is broken.
>
> I think that was the question raised by Thomas, and I don't understand
> how your reply answers that, so maybe I missed something...
>

Seems I did not understand the question. My idea on the topic is how
we can generate
images that we can easily flash. Now I understand the question of
Thomas and thank you.

Yann can I ask how we can use an image generated as you describe before?

Michael

> Regards,
> Yann E. MORIN.
>
> --
> .-----------------.--------------------.------------------.--------------------.
> |  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
> | +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
> | +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
> | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
> '------------------------------^-------^------------------^--------------------'
Dario Binacchi May 21, 2024, 3:06 p.m. UTC | #7
Hello Thomas, Gero and All,

On Mon, May 20, 2024 at 11:23 AM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello Dario,
>
> [ Your e-mail client configuration seems somewhat broken, your lines
>   are badly wrapped ]
>
> On Mon, 20 May 2024 10:05:34 +0200
> Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote:
>
> > > Now the question is how to do this. To me your proposal looks
> > > reasonable, though some people had the feedback that bmap-tools will
> > > not work in all cases (should we then gracefully handle this failure
> > > case?).
> >
> > If I understand correctly, the first patch in the series seems
> > reasonable to Gero as well.
> > Therefore, both of you are in favor of merging this patch. The issue
> > arises for Gero
> > regarding the application of the second patch, which I added only as
> > an example use case.
> > I think merging the first patch would give the opportunity to use
> > bmaptool and realize its
> > usefulness. After all, the patch is backward compatible. Its use could
> > then lead to subsequent
> > improvements.
> >
> > Regarding the second patch, I then ask you: Is it correct not to
> > proceed with merging this
> > patch for the reason mentioned by Gero (i.e., encrypted directory)? It
> > seems to me that
> > bmaptool is used in Yocto, so is it correct to say that you would have
> > the same problem
> > with Yocto? I hope, Gero, you do not misunderstand me.
>
> Yes, the first patch is reasonable, but if we can't apply something
> like the second patch, then it might mean that the approach taken in
> the first patch isn't correct. Indeed, if we can't enable by default
> the generation of bmap-capable images in defconfigs, it kind of limits
> the usefulness of this bmap-tool integration.
>
> I was hoping for the other BR maintainers to chime in with some opinion.
>
> Do we have a way to detect that the bmap-tool generation will fail, and
> in this case gracefully skip the bmap-tool logic?

I ran some tests and tried to reproduce Gero's issue.
With these changes, in case of an error, the make command would not fail.

diff --git a/support/scripts/genimage.sh b/support/scripts/genimage.sh
index 72592ebb710d..122aa0e0dc81 100755
--- a/support/scripts/genimage.sh
+++ b/support/scripts/genimage.sh
@@ -59,7 +59,12 @@ if [ "${GENIMAGE_CREATE_BMAP}" = "yes" ]; then
     cnt=$(grep -c "${image}" "${GENIMAGE_CFG}")
     [ "${cnt}" -gt 1 ] && continue;
     image_path="${BINARIES_DIR}/${image}"
-    bmaptool create "${image_path}" -o "${image_path}.bmap"
-    gzip -c "${image_path}" > "${image_path}.gz"
+    TMP_STDERR=$(mktemp -t .tmp.genimage.XXXXXXXXXX)
+    bmaptool create "${image_path}" -o "${image_path}.bmap" 2>${TMP_STDERR}
+    if test $? -eq 0 ; then
+      gzip -c "${image_path}" > "${image_path}.gz"
+    else
+      echo "Cannot generate bmap for file ${image_path}. Please read
log error ${TMP_STDERR} for details"
+    fi
   done < <(grep '^image ' "${GENIMAGE_CFG}" | cut -d ' ' -f 2)
 fi

The change generally handles any type of error, but in the case
highlighted by Gero, it seems to me
that not too much time is wasted encountering the error, since
bmaptool initially tries to understand if
the system supports the FIEMAP ioctl or the SEEK_HOLE/SEEK_DATA
functionality of the file seek
syscall.

As can be seen in the Filemap.py file:

def filemap(image):
    """
    Create and return an instance of a Filemap class - 'FilemapFiemap' or
    'FilemapSeek', depending on what the system we run on supports. If the
    FIEMAP ioctl is supported, an instance of the 'FilemapFiemap' class is
    returned. Otherwise, if 'SEEK_HOLE' is supported an instance of the
    'FilemapSeek' class is returned. If none of these are supported, the
    function generates an 'Error' type exception.
    """

    try:
        return FilemapFiemap(image)
    except ErrorNotSupp:
        return FilemapSeek(image)

Can these changes make the patches acceptable?

Thanks and regards,
Dario

>
> Best regards,
>
> Thomas
> --
> Thomas Petazzoni, co-owner and CEO, Bootlin
> Embedded Linux and Kernel engineering and training
> https://bootlin.com
Yann E. MORIN May 21, 2024, 5:44 p.m. UTC | #8
Michael, All,

On 2024-05-21 11:28 +0200, Michael Nazzareno Trimarchi spake thusly:
> On Tue, May 21, 2024 at 7:25 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> > I think the question by Thomas was whether, given an input sparse file,
> > genimage will keep the generated image sparse in the same places the
> > input file was (only shifted by the offset that input file was copied
> > at in the image).
[--SNIP--]
> Seems I did not understand the question. My idea on the topic is how
> we can generate
> images that we can easily flash. Now I understand the question of
> Thomas and thank you.
> 
> Yann can I ask how we can use an image generated as you describe before?

    $ make pc_x86_64_efi_defconfig
    $ make
    $ ls -sl output/images/{rootfs.ext2,disk.img}

I tried a bit earlier, and the genimage file is indeed sparse, but I
could not find a tool that would report the list of holes/non-holes in a
file, and their offset, e.g. something like:

                size    offset
    non-hole    2MiB    @0
    hole        1MiB    @2MiB
    non-hole    16MiB   @3MiB
 and so on...

So I could not validate whether the hole/non-hole structure is the same
in the generated disk.img and the rootfs.ext2. If you have any idea how
to do that, I'm all eyes.

Regards,
Yann E. MORIN.
Yann E. MORIN May 21, 2024, 8:36 p.m. UTC | #9
Thomas, Dario, All,

On 2024-05-20 11:23 +0200, Thomas Petazzoni via buildroot spake thusly:
[--SNIP--]
> Yes, the first patch is reasonable, but if we can't apply something
> like the second patch, then it might mean that the approach taken in
> the first patch isn't correct. Indeed, if we can't enable by default
> the generation of bmap-capable images in defconfigs, it kind of limits
> the usefulness of this bmap-tool integration.
> 
> I was hoping for the other BR maintainers to chime in with some opinion.

Given the issues on existing setups, I am of the opinion that we should
not enable the creation of bmap images by default, in any of our
defconfig.

Now, do we want to include that support in our somewhat-generic genimage
helper script? Technically, I don't have a strong opinion against,
except it would not be exercised very often. We could have our CI jobs
turn it on explicitly, though, so that's not too much of an issue.

However, users will not have an easy way to turn it on, except by adding
somewhat-arcane command line options in the post-image script args.

Also, it is relatively easy to add a site-local post-image script, which
can be added to the list after the bundled one, and which will call
bmap-tool, e.g. something like (kinda pseudo shell in places):

    #!/usr/bin/env bash

    getopts for -c configfile -z -j -J blablalba

    # Only the last image is interesting, as it presumably contains all
    # the others
    last_image="$(
        sed -r -e '/^image ([^[:space:]]+).*/!d; s//\1/' "${cfg_file}" \
        |tail -n 1
    )"
    image_path="${BINARIES_DIR}/${last_image}"

    bmaptool create "${image_path}" -o "${image_path}.bmap"

    if -z; then
        gzip "${image_path}"
    elif -j; then
        bzip2 "${image_path}"
    elif -J; then
        xz "${image_path}"
    fi

Ah one point: I think our generic geninage helper should ignore unknown
options, rather than error out. This would I believe help with users
adding site-local scripts like the above (notice the -z -j -J options
for example).

Still, my opinion is that our defconfigs, and the associated scripts,
are a starting point for booting known boards and somehow show-casing
Buildroot, and set the user on tracks for them to further customise the
configuration and the scripts to match their needs.

In this case, I don't think bmap-tools provides any special interest for
that goal, given the integration in Buildroot is really easy, and very
use-case specific (i.e. I know projects for which bmap-tools would be a
drawback, given the generated images have zero hole).

> Do we have a way to detect that the bmap-tool generation will fail, and
> in this case gracefully skip the bmap-tool logic?

I kind of disagree there. Given we won't enable the creation of bmap
images by default, then it means the user will have to explicitly
request the creation of a bmap image and thus, if we can't generate it,
then we must fail explicitly.

Regards,
Yann E. MORIN.
'Krzysztof Kozlowski' via Amarula Linux July 15, 2024, 2:05 p.m. UTC | #10
Hello Dario,

On Sun, 21 Apr 2024 11:53:52 +0200
Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote:

> The patch adds an option to create, in addition to the usual image, an
> image of type bmap that drastically reduces the amount of data that
> needs to be written to an SD card, resulting in time savings.
> 
> This makes it possible to activate this option easily and maintain
> backward compatibility for all configurations already using the genimage
> tool for creating the image to be written to the SD card:
> 
> BR2_ROOTFS_POST_IMAGE_SCRIPT="support/scripts/genimage.sh"
> +BR2_ROOTFS_POST_IMAGE_SCRIPT_ARGS="-c board/<manufacturer>/<boardname>/genimage.cfg -b"
> +BR2_PACKAGE_HOST_BMAP_TOOLS=y
> 
> It follows that the script now assumes a broader functionality that
> extends beyond just being an interface for the genimage tool.
> 
> An alternative implementation could have been to create another script,
> such as support/scripts/bmap.sh, capable of creating the bmap image
> using the same parameters passed to support/scripts/genimage.sh (i. e.
> -c board/<manufacturer>/<boardname>/genimage.cfg). However, in this case,
> the script would also need to know that the image is located in the
> ${BINARIES} directory. This could be achieved by adding an additional
> parameter, but it might cause genimage.sh to fail due to an unrecognized
> parameter.
> 
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>

We took advantage of the on-going Buildroot hackathon to discuss live,
and we pushed your patch, but with some changes:

(1) We don't add an option to script, but rather we look in the
Buildroot .config file is host-bmap-tools has been enabled. If it is
enabled, we assume the user wants bmap images

(2) We generate bmap images for all images, as long as they exist in
$(BINARIES_DIR). Indeed your trick of counting how many times the name
of the image was referenced in the genimage.cfg to try to guess what is
the "final" image (like sdcard.img) would fail badly if the final image
was:

image a {

}

because the image name "a" is probably found more than once in the
genimage.cfg.

However, with that, we are not going to apply your PATCH 2/2, because
we don't want to force everyone to build host-bmap-tools. Now everyone
only has to enable BR2_PACKAGE_HOST_BMAP_TOOLS=y in their configuration
to automagically get bmap-capable images generated. Perhaps what should
be done now is to document this somewhere in the manual. The question
is of course where to document it, and on that I'm not sure.

Note: it might be relevant to also adjust the filesystem generation
code in fs/common.mk to also generate a bmap-capable image of the
rootfs image itself, also based on the availability of host-bmap-tools.

Thanks for your work!

Thomas
'Krzysztof Kozlowski' via Amarula Linux July 15, 2024, 2:23 p.m. UTC | #11
On 15/07/2024 16:05, Thomas Petazzoni via buildroot wrote:
> Hello Dario,
> 
> On Sun, 21 Apr 2024 11:53:52 +0200
> Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote:
> 
>> The patch adds an option to create, in addition to the usual image, an
>> image of type bmap that drastically reduces the amount of data that
>> needs to be written to an SD card, resulting in time savings.
>>
>> This makes it possible to activate this option easily and maintain
>> backward compatibility for all configurations already using the genimage
>> tool for creating the image to be written to the SD card:
>>
>> BR2_ROOTFS_POST_IMAGE_SCRIPT="support/scripts/genimage.sh"
>> +BR2_ROOTFS_POST_IMAGE_SCRIPT_ARGS="-c board/<manufacturer>/<boardname>/genimage.cfg -b"
>> +BR2_PACKAGE_HOST_BMAP_TOOLS=y
>>
>> It follows that the script now assumes a broader functionality that
>> extends beyond just being an interface for the genimage tool.
>>
>> An alternative implementation could have been to create another script,
>> such as support/scripts/bmap.sh, capable of creating the bmap image
>> using the same parameters passed to support/scripts/genimage.sh (i. e.
>> -c board/<manufacturer>/<boardname>/genimage.cfg). However, in this case,
>> the script would also need to know that the image is located in the
>> ${BINARIES} directory. This could be achieved by adding an additional
>> parameter, but it might cause genimage.sh to fail due to an unrecognized
>> parameter.
>>
>> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> 
> We took advantage of the on-going Buildroot hackathon to discuss live,
> and we pushed your patch, but with some changes:
> 
> (1) We don't add an option to script, but rather we look in the
> Buildroot .config file is host-bmap-tools has been enabled. If it is
> enabled, we assume the user wants bmap images

  Instead of looking in the config, we could also look for it in PATH (with "if 
which bmaptool >/dev/null 2>&1").

  Regards,
  Arnout

> 
> (2) We generate bmap images for all images, as long as they exist in
> $(BINARIES_DIR). Indeed your trick of counting how many times the name
> of the image was referenced in the genimage.cfg to try to guess what is
> the "final" image (like sdcard.img) would fail badly if the final image
> was:
> 
> image a {
> 
> }
> 
> because the image name "a" is probably found more than once in the
> genimage.cfg.
> 
> However, with that, we are not going to apply your PATCH 2/2, because
> we don't want to force everyone to build host-bmap-tools. Now everyone
> only has to enable BR2_PACKAGE_HOST_BMAP_TOOLS=y in their configuration
> to automagically get bmap-capable images generated. Perhaps what should
> be done now is to document this somewhere in the manual. The question
> is of course where to document it, and on that I'm not sure.
> 
> Note: it might be relevant to also adjust the filesystem generation
> code in fs/common.mk to also generate a bmap-capable image of the
> rootfs image itself, also based on the availability of host-bmap-tools.
> 
> Thanks for your work!
> 
> Thomas

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
'Krzysztof Kozlowski' via Amarula Linux July 15, 2024, 2:27 p.m. UTC | #12
On Mon, 15 Jul 2024 16:23:53 +0200
Arnout Vandecappelle <arnout@mind.be> wrote:

> > (1) We don't add an option to script, but rather we look in the
> > Buildroot .config file is host-bmap-tools has been enabled. If it is
> > enabled, we assume the user wants bmap images  
> 
>   Instead of looking in the config, we could also look for it in PATH (with "if 
> which bmaptool >/dev/null 2>&1").

Yes, that was another option, but then for the same Buildroot
configuration and version, the build results would be different from
machine A (which has bmap-tools installed system-wide) and machine B
(which doesn't have bmap-tools installed system-wide). From my
perspective, this breaks the principle of least surprise.

Thomas
Dario Binacchi July 16, 2024, 6:16 a.m. UTC | #13
Hello Thomas and Arnount,

Thank you for applying the patch. More than once, during the patch
tests, I thought to myself:
If only I could speed up the flashing of the board using bmaptool.
And finally, it is possible.

And thanks also for the suggestions on further developments that could
be carried out.

Thanks and regards,
Dario

On Mon, Jul 15, 2024 at 4:27 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> On Mon, 15 Jul 2024 16:23:53 +0200
> Arnout Vandecappelle <arnout@mind.be> wrote:
>
> > > (1) We don't add an option to script, but rather we look in the
> > > Buildroot .config file is host-bmap-tools has been enabled. If it is
> > > enabled, we assume the user wants bmap images
> >
> >   Instead of looking in the config, we could also look for it in PATH (with "if
> > which bmaptool >/dev/null 2>&1").
>
> Yes, that was another option, but then for the same Buildroot
> configuration and version, the build results would be different from
> machine A (which has bmap-tools installed system-wide) and machine B
> (which doesn't have bmap-tools installed system-wide). From my
> perspective, this breaks the principle of least surprise.
>
> Thomas
> --
> Thomas Petazzoni, co-owner and CEO, Bootlin
> Embedded Linux and Kernel engineering and training
> https://bootlin.com
Peter Korsgaard Aug. 27, 2024, 6:55 p.m. UTC | #14
>>>>> "Thomas" == Thomas Petazzoni via buildroot <buildroot@buildroot.org> writes:

Hi,

 >> An alternative implementation could have been to create another script,
 >> such as support/scripts/bmap.sh, capable of creating the bmap image
 >> using the same parameters passed to support/scripts/genimage.sh (i. e.
 >> -c board/<manufacturer>/<boardname>/genimage.cfg). However, in this case,
 >> the script would also need to know that the image is located in the
 >> ${BINARIES} directory. This could be achieved by adding an additional
 >> parameter, but it might cause genimage.sh to fail due to an unrecognized
 >> parameter.
 >> 
 >> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>

 > We took advantage of the on-going Buildroot hackathon to discuss live,
 > and we pushed your patch, but with some changes:

 > (1) We don't add an option to script, but rather we look in the
 > Buildroot .config file is host-bmap-tools has been enabled. If it is
 > enabled, we assume the user wants bmap images

Sorry for the slow response, I only got to this now while backporting
for LTS, but I don't like the fact that it hardcodes gzip compression
(with a specific compression level), especially as it is
hidden/optionless now, E.G. it seems quite random that if you have a
genimage.cfg creating a sdcard.img (or whatever) and you then enable
host-bmaptools you all of a sudden don't have sdcard.img anymore, but
sdcard.img.gz (and sdcard.img.bmap) and "waste" time doing the
compression you may or may not want.

I would suggest to drop the gzip step and only create the .bmap files
(which presumably is quite fast?)

What do you say?
'Krzysztof Kozlowski' via Amarula Linux Aug. 28, 2024, 1:42 p.m. UTC | #15
On Tue, 27 Aug 2024 20:55:00 +0200
Peter Korsgaard <peter@korsgaard.com> wrote:

> Sorry for the slow response, I only got to this now while backporting
> for LTS, but I don't like the fact that it hardcodes gzip compression
> (with a specific compression level), especially as it is
> hidden/optionless now, E.G. it seems quite random that if you have a
> genimage.cfg creating a sdcard.img (or whatever) and you then enable
> host-bmaptools you all of a sudden don't have sdcard.img anymore, but
> sdcard.img.gz (and sdcard.img.bmap) and "waste" time doing the
> compression you may or may not want.
> 
> I would suggest to drop the gzip step and only create the .bmap files
> (which presumably is quite fast?)
> 
> What do you say?

I understand your concern, but compression is also what helps in making
those sparse images small. Of course, as long as you keep them where
Buildroot generated them, they are sparse, so they don't take much
space. But as soon as you "distribute" them, they usually loose their
sparse-ness, and this is where compressing the image + the bmap
metadata file helps in lot in reducing the amount of data.

But fair enough, I assume this compression can be done as a custom
post-processing step by whoever needs it.

Thomas
Peter Korsgaard Aug. 28, 2024, 1:58 p.m. UTC | #16
>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@bootlin.com> writes:

Hi,

 >> I would suggest to drop the gzip step and only create the .bmap files
 >> (which presumably is quite fast?)
 >> 
 >> What do you say?

 > I understand your concern, but compression is also what helps in making
 > those sparse images small.

NIT: No, it is the sparseness (E.G. the holes). Compression in fact
breaks the holes (replaces them by zeros):

truncate -s 1G disk.img
du -hs disk.img
0       disk.img

gzip disk.img
du -hs disk.img.gz
1020K   disk.img.gz

gunzip disk.img.gz
du -hs disk.img
1.0G    disk.img

To transfer a file with holes without losing them you need an archival
format that understands sparse files, E.G. tar -S:

truncate -s 1G disk.img
du -hs disk.img
0       disk.img

tar cfSz disk.tar.gz disk.img
du -hs disk.tar.gz
4.0K    disk.tar.gz

rm disk.img
tar zxf disk.tar.gz
du -hs disk.img
0       disk.img

Notice also that the .tar.gz is a lot smaller than the img.gz


 > Of course, as long as you keep them where Buildroot generated them,
 > they are sparse, so they don't take much space. But as soon as you
 > "distribute" them, they usually loose their sparse-ness, and this is
 > where compressing the image + the bmap metadata file helps in lot in
 > reducing the amount of data.

Correct, unless an archival format supporting sparse files is used, so
if anything we should make a tarball of the images - But that is also
not so simple, E.G.:

- Busybox tar cannot extract sparse tar files
- One tarball for everything or a tarball for each image?
- What compression algorithm and parameters to use?

So it very fast ends up in a very use case specific setup, hence my
suggestion to NOT do it and leave such customizations to a post-image
script.

 > But fair enough, I assume this compression can be done as a custom
 > post-processing step by whoever needs it.

Indeed.
'Krzysztof Kozlowski' via Amarula Linux Aug. 29, 2024, 9:44 p.m. UTC | #17
Hello Peter,

On Wed, 28 Aug 2024 15:58:21 +0200
Peter Korsgaard <peter@korsgaard.com> wrote:

>  > I understand your concern, but compression is also what helps in making
>  > those sparse images small.  
> 
> NIT: No, it is the sparseness (E.G. the holes). Compression in fact
> breaks the holes (replaces them by zeros):

You didn't understand my point, and Arnout explained it better on the
chat.

My point is that it is *very* easy to lose the sparseness. You copy the
file around, you share it in Google Drive, or download it over HTTP or
what not, and bang the sparseness is lost.

Instead, with a compressed file, you get a file that has pretty much
the same size as the sparse file, but you can move it around, transfer
it, it won't consume its real size. And the bmap metadata file allows
to use/flash that compressed file with the same efficiency as the
original sparse file.

In fact, bmap is quite useless is the file sparseness is retained:
instead of using bmap metadata, you could just ask what are the holes
of the file and skip them when flashing. So really the whole point of
bmap metadata IMO is that they allow to do fast flashing (skipping
holes) even if the sparseness of the file has been lost.

Thomas
Peter Korsgaard Aug. 30, 2024, 7:14 a.m. UTC | #18
>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@bootlin.com> writes:

Hi,

 >> NIT: No, it is the sparseness (E.G. the holes). Compression in fact
 >> breaks the holes (replaces them by zeros):

 > You didn't understand my point, and Arnout explained it better on the
 > chat.

 > My point is that it is *very* easy to lose the sparseness. You copy the
 > file around, you share it in Google Drive, or download it over HTTP or
 > what not, and bang the sparseness is lost.

Sure, but that is all about distribution, where as mentioned before you
may want to compress it with gz/xz/zstd/.. and/or stick it in a sensible
archival format like tar, ..

The specifics of that is very use case specific, so IMHO not something
where we should enforce silly old .gz - Especially as bmap supports a
number of different formats (by shelling out to the extractors). From
bmaptool/TransRead.py:

This module allows opening and reading local and remote files and decompress
them on-the-fly if needed. Remote files are read using urllib (except of
"ssh://" URLs, which are handled differently). Supported file extentions are:
'bz2', 'gz', 'xz', 'lzo', 'zst' and a "tar" version of them: 'tar.bz2', 'tbz2',
'tbz', 'tb2', 'tar.gz', 'tgz', 'tar.xz', 'txz', 'tar.lzo', 'tzo', 'tar.lz4',
'tlz4', '.tar.zst', 'tzst'.
This module uses the following system programs for decompressing: pbzip2, bzip2,
gzip, pigz, xz, lzop, lz4, zstd, tar and unzip.


 > Instead, with a compressed file, you get a file that has pretty much
 > the same size as the sparse file, but you can move it around, transfer
 > it, it won't consume its real size. And the bmap metadata file allows
 > to use/flash that compressed file with the same efficiency as the
 > original sparse file.

 > In fact, bmap is quite useless is the file sparseness is retained:
 > instead of using bmap metadata, you could just ask what are the holes
 > of the file and skip them when flashing. So really the whole point of
 > bmap metadata IMO is that they allow to do fast flashing (skipping
 > holes) even if the sparseness of the file has been lost.

Does such a tool exist? I am not aware of one. If so, maybe we should
recommend that instead of dd for writing sdcard.img from the host?

Anyway, I find bmap a bit half-baked, E.G. rather than wasting time (and
space) compressing and decompressing all the holes that are then just
ignored it would make more sense to have a file format encoding the
holes (E.G. tar --sparse or something custom), then optionally compress
that for transmission and have something small and simple (E.G. not in
python) on the host/target to write it to a device.

But oh well, it is what it is. I don't need such a tool often enough to
care to write it ;)

I'll send a patch to drop the gzip step from genimage.sh.
'Krzysztof Kozlowski' via Amarula Linux Aug. 30, 2024, 3:29 p.m. UTC | #19
Hello Peter,

On Fri, 30 Aug 2024 09:14:37 +0200
Peter Korsgaard <peter@korsgaard.com> wrote:

> Does such a tool exist? I am not aware of one. If so, maybe we should
> recommend that instead of dd for writing sdcard.img from the host?

I'm not aware of any tool like this.

> 
> Anyway, I find bmap a bit half-baked, E.G. rather than wasting time (and
> space) compressing and decompressing all the holes that are then just
> ignored it would make more sense to have a file format encoding the
> holes (E.G. tar --sparse or something custom), then optionally compress
> that for transmission and have something small and simple (E.G. not in
> python) on the host/target to write it to a device.

The Android sparse image format looks like to be what you want.
See https://2net.co.uk/tutorial/android-sparse-image-format. There are some
C/C++ tools at https://android.googlesource.com/platform/system/core/+/master/libsparse/
to generate/manipulate Android sparse images.

Thomas
Michael Nazzareno Trimarchi Aug. 30, 2024, 3:32 p.m. UTC | #20
Hi Thomas

On Fri, Aug 30, 2024 at 5:29 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello Peter,
>
> On Fri, 30 Aug 2024 09:14:37 +0200
> Peter Korsgaard <peter@korsgaard.com> wrote:
>
> > Does such a tool exist? I am not aware of one. If so, maybe we should
> > recommend that instead of dd for writing sdcard.img from the host?
>
> I'm not aware of any tool like this.
>
> >
> > Anyway, I find bmap a bit half-baked, E.G. rather than wasting time (and
> > space) compressing and decompressing all the holes that are then just
> > ignored it would make more sense to have a file format encoding the
> > holes (E.G. tar --sparse or something custom), then optionally compress
> > that for transmission and have something small and simple (E.G. not in
> > python) on the host/target to write it to a device.
>
> The Android sparse image format looks like to be what you want.
> See https://2net.co.uk/tutorial/android-sparse-image-format. There are some
> C/C++ tools at https://android.googlesource.com/platform/system/core/+/master/libsparse/
> to generate/manipulate Android sparse images.
>

As I mention some time ago android sparse format can be generate with
genimage.cfg

image sdcard-sparse.img {
    android-sparse {
        image = sdcard.img
    }
}

Nothing needs to be changed. Fastboot should able to handle it

Michael

> Thomas
> --
> Thomas Petazzoni, co-owner and CEO, Bootlin
> Embedded Linux and Kernel engineering and training
> https://bootlin.com
Peter Korsgaard Aug. 31, 2024, 1:20 p.m. UTC | #21
>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@bootlin.com> writes:

Hi,

>> Anyway, I find bmap a bit half-baked, E.G. rather than wasting time (and
 >> space) compressing and decompressing all the holes that are then just
 >> ignored it would make more sense to have a file format encoding the
 >> holes (E.G. tar --sparse or something custom), then optionally compress
 >> that for transmission and have something small and simple (E.G. not in
 >> python) on the host/target to write it to a device.

 > The Android sparse image format looks like to be what you want.
 > See https://2net.co.uk/tutorial/android-sparse-image-format. There are some
 > C/C++ tools at https://android.googlesource.com/platform/system/core/+/master/libsparse/
 > to generate/manipulate Android sparse images.

Ahh yes, that looks more sensible than the bmap stuff - Thanks.
Peter Korsgaard Aug. 31, 2024, 1:28 p.m. UTC | #22
>>>>> "Michael" == Michael Nazzareno Trimarchi <michael@amarulasolutions.com> writes:

Hi,

 >> The Android sparse image format looks like to be what you want.
 >> See https://2net.co.uk/tutorial/android-sparse-image-format. There are some
 >> C/C++ tools at https://android.googlesource.com/platform/system/core/+/master/libsparse/
 >> to generate/manipulate Android sparse images.
 >> 

 > As I mention some time ago android sparse format can be generate with
 > genimage.cfg

 > image sdcard-sparse.img {
 >     android-sparse {
 >         image = sdcard.img
 >     }
 > }

 > Nothing needs to be changed. Fastboot should able to handle it

Ahh nice, I wasn't aware of that!

Patch

diff --git a/support/scripts/genimage.sh b/support/scripts/genimage.sh
index 2796e19eb778..72592ebb710d 100755
--- a/support/scripts/genimage.sh
+++ b/support/scripts/genimage.sh
@@ -4,19 +4,26 @@  die() {
   cat <<EOF >&2
 Error: $@
 
-Usage: ${0} -c GENIMAGE_CONFIG_FILE
+Usage: ${0} -c GENIMAGE_CONFIG_FILE [-b]
+  -b    create bmap image
+  -c    configuration file
+
 EOF
   exit 1
 }
 
 # Parse arguments and put into argument list of the script
-opts="$(getopt -n "${0##*/}" -o c: -- "$@")" || exit $?
+opts="$(getopt -n "${0##*/}" -o bc: -- "$@")" || exit $?
 eval set -- "$opts"
 
 GENIMAGE_TMP="${BUILD_DIR}/genimage.tmp"
+GENIMAGE_CREATE_BMAP="no"
 
 while true ; do
 	case "$1" in
+	-b)
+	  GENIMAGE_CREATE_BMAP="yes"
+	  shift 1 ;;
 	-c)
 	  GENIMAGE_CFG="${2}";
 	  shift 2 ;;
@@ -46,3 +53,13 @@  genimage \
 	--inputpath "${BINARIES_DIR}"  \
 	--outputpath "${BINARIES_DIR}" \
 	--config "${GENIMAGE_CFG}"
+
+if [ "${GENIMAGE_CREATE_BMAP}" = "yes" ]; then
+  while IFS= read -r image; do
+    cnt=$(grep -c "${image}" "${GENIMAGE_CFG}")
+    [ "${cnt}" -gt 1 ] && continue;
+    image_path="${BINARIES_DIR}/${image}"
+    bmaptool create "${image_path}" -o "${image_path}.bmap"
+    gzip -c "${image_path}" > "${image_path}.gz"
+  done < <(grep '^image ' "${GENIMAGE_CFG}" | cut -d ' ' -f 2)
+fi