[Merge] ~jchittum/livecd-rootfs:ensure-vmtools-in-vmdk-header into livecd-rootfs:ubuntu/master

[Robert C Jennings]

Review: Needs Fixing

Sorry I didn't catch this sooner, but I have 2 more comments.

Diff comments:

> diff --git a/live-build/functions b/live-build/functions
> index e4b9042..f53b59e 100644
> --- a/live-build/functions
> +++ b/live-build/functions
> @@ -237,27 +237,38 @@ modify_vmdk_header() {
>      # Extract the vmdk header for manipulation
>      dd if="${vmdk_name}" of="${descriptor}" bs=1 skip=512 count=1024
>  
> -    # The sed lines below is where the magic is. Specifically:
> -    #   ddb.toolsVersion: sets the open-vm-tools so that VMware shows
> -    #       the tooling as current
> -    #   ddb.virtualHWVersion: set the version to 7, which covers most
> -    #       current versions of VMware
> -    #   createType: make sure its set to stream Optimized
> -    #   remove the vmdk-stream-converter comment and replace with
> -    #       # Disk DescriptorFile. This is needed for Virtualbox
> -    #   remove the comments from vmdk-stream-converter which causes
> -    #       VirtualBox and others to fail VMDK validation
> -
> -    sed -e 's|# Description file.*|# Disk DescriptorFile|' \
> -        -e '/# Believe this is random*/d' \
> -        -e '/# Indicates no parent/d' \
> -        -e '/# The Disk Data Base/d' \
> -        -e 's|ddb.comment.*|ddb.toolsVersion = "2147483647"|' \
> -            "${descriptor}" > "${newdescriptor}"
> -
> -    # The header is cannot be bigger than 1024
> -    expr $(stat --format=%s ${newdescriptor}) \< 1024 > /dev/null 2>&1 || {
> -        echo "descriptor is too large, VMDK will be invalid!"; exit 1; }
> +    # cat header so we are aware of the original descriptor for debugging
> +    cat $descriptor

Can you pop an 'echo something something' to give context in the log about what we're looking at when you cat the file?  Also, how (bad) does the 'cat $descriptor' look in the debug log before trimming null bytes?

> +
> +    # trim null bytes to treat as standard text file
> +    tr -d '\000' < $descriptor > $newdescriptor
> +
> +    # add newline to newdescriptor
> +    echo "" >> $newdescriptor
> +
> +    # add required tools version
> +    echo -n 'ddb.toolsVersion = "2147483647"' >> $newdescriptor
> +
> +    # check ddb.toolsVersion in descriptor, otherwise image will fail
> +    if ! grep -q 'ddb.toolsVersion' $newdescriptor; then 

Now that we're echoing into the file rather than the prior sed implementation we don't need this check. The old sed implementation was looking to replace one directive with the toolsVersion and that pattern was missing in the file, so toolsVersion was never added.  But with this code now if the echo and redirect doesn't work we'll have a non-zero exit which will fail the build.

> +        echo 'failed to write version. Descriptor invalid'; \
> +        exit 1
> +    fi
> +
> +    # diff original descriptor and new descriptor for debugging
> +    # diff exits 1 if difference. pipefail not set so piping diff
> +    # to cat prints diff and swallows exit 1
> +    diff --text $descriptor $newdescriptor | cat
> +
> +
> +    # The header must be 1024 or less before padding
> +    if ! expr $(stat --format=%s ${newdescriptor}) \< 1025 > /dev/null 2>&1; then
> +        echo "descriptor is too large, VMDK will be invalid!"; 
> +        exit 1
> +    fi
> +
> +    # reset newdescriptor to be 1024
> +    truncate --no-create --size=1K $newdescriptor
>  
>      # Overwrite the vmdk header with our new, modified one
>      dd conv=notrunc,nocreat \


-- 
https://code.launchpad.net/~jchittum/livecd-rootfs/+git/livecd-rootfs/+merge/392401
Your team Ubuntu Core Development Team is requested to review the proposed merge of ~jchittum/livecd-rootfs:ensure-vmtools-in-vmdk-header into livecd-rootfs:ubuntu/master.