[Merge] lp:~sergiusens/goget-ubuntu-touch/bootassets intolp:goget-ubuntu-touch
Sergio Schvezov
sergio.schvezov at canonical.com
Fri Mar 27 15:15:31 UTC 2015
On viernes 27 de marzo de 2015 11h'57:17 ART, Michael Vogt wrote:
> This looks good, I have two inline comments, I think it would
> be good to consolidate copyFile() (see inline comment). I know
> that there are few tests currently, but it seems like
> setupBootAssetFiles() and setupBootAssetRawFiles() might be good
> candidates and from a quick look it seems like testing them is
> straightforward (especially the later one can be dangerous if
> its buggy).
Right, this is a candidate for testing and reason I fear landing.
> Diff comments:
>
>> === added file 'diskimage/bootloader.go'
>> --- diskimage/bootloader.go 1970-01-01 00:00:00 +0000
>> +++ diskimage/bootloader.go 2015-03-27 13:28:35 +0000
>> @@ -0,0 +1,86 @@
>> +// ...
>
> You may consider using os.OpenFiledst, O_CREATE|O_WRONLY,
> os.Stat(src).FileMode()) here. AFAICS the file mode is not
> preserved in the copy.
>
> There is also a existing copyFile implementation in
> ubuntu-device-flash/common.go so maybe create a common/common.go
> or helpers or whatnot to avoid re-implementing this?
Will do!
>> + if err != nil {
>> + return err
>> + }
>> + defer dstFile.Close()
>> +
>> + srcFile, err := os.Open(src)
>> + if err != nil {
>> + return err
>> + }
>> + defer srcFile.Close()
>> +
>> + reader := bufio.NewReader(srcFile)
>> + writer := bufio.NewWriter(dstFile)
>> + defer func() {
>> + if err != nil {
>> + writer.Flush()
>> + }
>> + }()
>> + if _, err = io.Copy(writer, reader); err != nil {
>> + return err
>> + }
>> + writer.Flush()
>> + return nil
>> +}
>>
>> === modified file 'diskimage/core_grub.go'
>> --- diskimage/core_grub.go 2015-03-27 13:28:35 +0000
>> +++ diskimage/core_grub.go 2015-03-27 13:28:35 +0000
>> @@ -281,7 +281,7 @@
>> return img.baseMount
>> }
>>
>> -func (img *CoreGrubImage) SetupBoot() error {
>> +func (img *CoreGrubImage) SetupBoot(oemRootPath string) error {
>> for _, dev := range []string{"dev", "proc", "sys"} {
>> src := filepath.Join("/", dev)
>> dst := filepath.Join(img.System(), dev)
>> @@ -368,7 +368,7 @@
>> return nil
>> }
>>
>> -func (img *CoreGrubImage) FlashExtra(devicePart string) error {
>> +func (img *CoreGrubImage) FlashExtra(oemRootPath, devicePart
>> string) error {
>> return nil
>> }
>>
>>
>> === modified file 'diskimage/core_uboot.go'
>> --- diskimage/core_uboot.go 2015-03-27 13:28:35 +0000
>> +++ diskimage/core_uboot.go 2015-03-27 13:28:35 +0000
>> @@ -11,7 +11,6 @@
>> "bufio"
>> "errors"
>> "fmt"
>> - "io"
>> "io/ioutil"
>> "os"
>> "os/exec"
>> @@ -290,7 +289,7 @@
>> return img.baseMount
>> }
>>
>> -func (img CoreUBootImage) SetupBoot() error {
>> +func (img CoreUBootImage) SetupBoot(oemRootPath string) error {
>> // destinations
>> bootPath := filepath.Join(img.baseMount, string(bootDir))
>> bootAPath := filepath.Join(bootPath, "a")
>> @@ -336,8 +335,16 @@
>> }
>> }
>>
>> - if err := img.provisionUenv(bootuEnvPath); err != nil {
>> - return err
>> + // if the oem package provides BootAssets use it directly, if not
>> + // provisionUenv for backwards compatibility.
>> + if bootAssets := img.oem.OEM.Hardware.BootAssets; bootAssets != nil {
>> + if err := setupBootAssetFiles(bootPath, oemRootPath,
>> bootAssets.Files); err != nil {
>> + return err
>> + }
>> + } else {
>> + if err := img.provisionUenv(bootuEnvPath); err != nil {
>> + return err
>> + }
>> }
>>
>> // create /boot/uboot
>> @@ -435,7 +442,14 @@
>> return nil
>> }
>>
>> -func (img *CoreUBootImage) FlashExtra(devicePart string) error {
>> +func (img *CoreUBootImage) FlashExtra(oemRootPath, devicePart
>> string) error {
>> + // if the oem package has bootloader assets use this and skip the
>> + // deprecated device part setup
>> + if bootAssets := img.oem.OEM.Hardware.BootAssets; bootAssets != nil {
>> + return setupBootAssetRawFiles(img.location, oemRootPath,
>> bootAssets.RawFiles)
>> + }
>> +
>> + // this is for backwards compatibility
>> if img.oem.Platform() == "" {
>> return nil
>> }
>> @@ -490,30 +504,3 @@
>>
>> return nil
>> }
>> -
>> -func copyFile(src, dst string) error {
>> - dstFile, err := os.Create(dst)
>> - if err != nil {
>> - return err
>> - }
>> - defer dstFile.Close()
>> -
>> - srcFile, err := os.Open(src)
>> - if err != nil {
>> - return err
>> - }
>> - defer srcFile.Close()
>> -
>> - reader := bufio.NewReader(srcFile)
>> - writer := bufio.NewWriter(dstFile)
>> - defer func() {
>> - if err != nil {
>> - writer.Flush()
>> - }
>> - }()
>> - if _, err = io.Copy(writer, reader); err != nil {
>> - return err
>> - }
>> - writer.Flush()
>> - return nil
>> -}
>>
>> === modified file 'ubuntu-device-flash/core.go'
>> --- ubuntu-device-flash/core.go 2015-03-27 13:28:35 +0000
>> +++ ubuntu-device-flash/core.go 2015-03-27 13:28:35 +0000
>> @@ -66,8 +66,9 @@
>> Keyboard string `long:"keyboard-layout"
>> description:"Specify the keyboard layout" default:"us"`
>> } `group:"Development"`
>>
>> - hardware diskimage.HardwareDescription
>> - oem diskimage.OemDescription
>> + hardware diskimage.HardwareDescription
>> + oem diskimage.OemDescription
>> + stagingRootPath string
>> }
>>
>> var coreCmd CoreCmd
>> @@ -98,9 +99,10 @@
>> }
>>
>> fmt.Println("Determining oem configuration")
>> - if err := coreCmd.extractOemDescription(coreCmd.Oem); err != nil {
>> + if err := coreCmd.extractOem(coreCmd.Oem); err != nil {
>> return err
>> }
>> + defer os.RemoveAll(coreCmd.stagingRootPath)
>>
>> if !globalArgs.DownloadOnly {
>> if syscall.Getuid() != 0 {
>> @@ -275,7 +277,8 @@
>> return err
>> }
>>
>> - if err := img.FlashExtra(devicePart); err != nil {
>> + oemRootPath := filepath.Join(coreCmd.stagingRootPath,
>> coreCmd.oem.InstallPath())
>> + if err := img.FlashExtra(oemRootPath, devicePart); err != nil {
>> return err
>> }
>>
>> @@ -344,7 +347,8 @@
>> return err
>> }
>>
>> - if err := img.SetupBoot(); err != nil {
>> + oemRootPath := filepath.Join(coreCmd.stagingRootPath,
>> coreCmd.oem.InstallPath())
>> + if err := img.SetupBoot(oemRootPath); err != nil {
>> return err
>> }
>>
>> @@ -522,7 +526,7 @@
>> return string(pubKey), err
>> }
>>
>> -func (coreCmd *CoreCmd) extractOemDescription(oemPackage string) error {
>> +func (coreCmd *CoreCmd) extractOem(oemPackage string) error {
>> if oemPackage == "" {
>> return nil
>> }
>> @@ -531,7 +535,13 @@
>> if err != nil {
>> return err
>> }
>> - defer os.RemoveAll(tempDir)
>> +
>> + // we need to fix the permissions for tempdir
>
> Why do we need to fix the permissions for the tempdir? That was
> the question that entered my head when I read this :)
Because of euid root and dirs being exec only for the owner, so when later
trying to reach it when dropping privs it all breaks.
>> + if err := os.Chmod(tempDir, 0755); err != nil {
>> + return err
>> + }
>> +
>> + coreCmd.stagingRootPath = tempDir
>>
>> snappy.SetRootDir(tempDir)
>> defer snappy.SetRootDir("/")
>>
>
>
--
https://code.launchpad.net/~sergiusens/goget-ubuntu-touch/bootassets/+merge/254393
Your team Ubuntu Phablet Team is subscribed to branch lp:goget-ubuntu-touch.
More information about the Ubuntu-reviews
mailing list