1214515 - [Find My Device][Kill Switch] Implement Persistent Data Block component in Gecko so it can manage partitions for Factory Reset Protection

Status: RESOLVED FIXED

(Firefox OS Graveyard :: FindMyDevice, defect)

Description

[Juan Gomez [:_AtilA_] (CET/CEST)]

KillSwitch API needs to access the raw partition where all the information related to Factory Reset Protection (FRP) is written. We need to implement it as a Gecko component and import it in the API.
Gonk HAL will be extended as well becuase we need a way to get the size of a data block by querying the kernel driver (ioctl).

The component will try to keep the same API as the AOSP PersistentDataBlockService.java [1] one.

[1] https://android.googlesource.com/platform/frameworks/base/+/master/services/core/java/com/android/server/PersistentDataBlockService.java

Comment 1

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: [WIP] Persistent Data Block component implementation

Comment 2

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: [WIP] Persistent Data Block implementation with some tests

Comment 3

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: Persistent Data Block implementation and unit tests

Final version, needs a review :)
Testing became kind of complicated because of partition layout structure on emulator doesn't support our persistent partition, so some tests relay on /cache partition and files.
Looking forward to integrate with Killswitch backend component... :)

Comment 4

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: Persistent Data Block implementation and unit tests

Trailing whitespaces removed :(

Comment 5

[:gerard-majax]

Comment on attachment 8684287 [details] [diff] [review]
Persistent Data Block implementation and unit tests

Review of attachment 8684287 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good but I would like a few things to be fixed before going forward.

::: b2g/components/PersistentDataBlock.jsm
@@ +37,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "OS", "resource://gre/modules/osfile.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Promise", "resource://gre/modules/Promise.jsm");
> +
> +
> +#ifdef MOZ_WIDGET_GONK

I am not sure we need that since we run that test only on Gonk

@@ +165,5 @@
> +  },
> +
> +  /*
> +  * Change the class behavior.
> +  * *** USE ONLY FOR TESTING ***

Please make it clear here what we need to bypass and why.

@@ +238,5 @@
> +      return Promise.reject();
> +    });
> +  },
> +
> +  uninit: function() {

nit: maybe moving the uninit() just below init(), for easier reading of the code

@@ +282,5 @@
> +  _computeDigest: function (storedDigest) {
> +    debug("_computeDigest()");
> +    var digest;
> +    var partition;
> +    return OS.File.open(this._dataBlockFile, {existing:true, append:false, read:true}).then( _partition => {

no space before the argument:
 .then(_partition => {

@@ +286,5 @@
> +    return OS.File.open(this._dataBlockFile, {existing:true, append:false, read:true}).then( _partition => {
> +      partition = _partition;
> +      return partition.read(DIGEST_SIZE_BYTES);
> +    }).then( digestDataRead => {
> +      //If storedDigest is passed as a parameter, the caller will likely compare the

Nit: '//' should be followed by space

@@ +296,5 @@
> +      return partition.read();
> +    }).then( data => {
> +      //Calculate Digest with the data retrieved after the digest
> +      let hasher = Cc["@mozilla.org/security/hash;1"].createInstance(Ci.nsICryptoHash);
> +      hasher.init(hasher.SHA256);

I guess SHA256 is because we only have 32 bytes to store the value ?

@@ +300,5 @@
> +      hasher.init(hasher.SHA256);
> +      hasher.update(data, data.length);
> +      let _digest = hasher.finish(true);
> +      digest = atob(_digest);
> +      debug("_computeDigest(): Digest = "  + _digest + "(" + digest.length + ")");

No way to reuse/do like dom/apps/AppsUtils.jsm:computeHash() ?

@@ +319,5 @@
> +  _getBlockDeviceSize: function() {
> +    debug("_getBlockDeviceSize()");
> +
> +    /* See _mode property */
> +    if( this._mode.testing === true ){

Remove the spaces around the condition:
 if (this._mode.testing === true) {

@@ +327,5 @@
> +
> +#ifdef MOZ_WIDGET_GONK
> +    const O_READONLY = 0;
> +    const O_NONBLOCK = 1 << 11;
> +    const BLKGETSIZE64 = 2147750514; //TODO: That's not the way. Gonk should provide a realiable method

Until then, please document how to get that value :), and make it clear where it comes from/why it might change

@@ +349,5 @@
> +    this._libc.close(fd);
> +    debug("_getBlockDeviceSize: size =" + size.value);
> +    return size.value;
> +#else
> +  log("_getBlockDeviceSize: ERROR: This funcionality is only supported in Gonk!");

Misalignment in indentation

@@ +365,5 @@
> +   *                                     Otherwise, sets it to 0.
> +   */
> +  _doSetOemUnlockEnabled: function(isSetOemUnlockEnabled){
> +    debug("_doSetOemUnlockEnabled()");
> +    var self = this;

You don't need this since you are using arrow functions

@@ +369,5 @@
> +    var self = this;
> +    var partition;
> +    return OS.File.open(this._dataBlockFile, {existing:true, append:false, write:true}).then( _partition => {
> +      partition = _partition;
> +      return partition.setPosition(self._getBlockDeviceSize() - 1, OS.File.POS_START);

Use this. instead of self.

@@ +372,5 @@
> +      partition = _partition;
> +      return partition.setPosition(self._getBlockDeviceSize() - 1, OS.File.POS_START);
> +    }).then( () => {
> +      return partition.write(new Uint8Array([ isSetOemUnlockEnabled === true ? 1 : 0 ]));
> +    }).then( bytesWrittenLength => {

We make no checking on that value ?

@@ +394,5 @@
> +   */
> +  _computeAndWriteDigest: function(){
> +    debug("_computeAndWriteDigest()");
> +    var digest;
> +    var self = this;

Same, remove.

@@ +402,5 @@
> +      return OS.File.open(self._dataBlockFile, {write:true, existing:true, append:false});
> +    }).then( _partition => {
> +      partition = _partition;
> +      return partition.write(digest);
> +    }).then( bytesWrittenLength => {

No check performed either?

@@ +414,5 @@
> +    });
> +  },
> +
> +  /**
> +   * Formats the persistent partition if the OEM Unlock Enabled field is set to true, and

We will format that at each boot? The OEM unlock is the one we toggle from Fastboot?

If so, we should be very clear that it means the feature won't work except on production-alike devices.

@@ +423,5 @@
> +   */
> +  _formatIfOemUnlockEnabled: function () {
> +    debug("_formatIfOemUnlockEnabled()");
> +    var self = this;
> +    return this.getOemUnlockEnabled().then( enabled => {

Maybe the name of that variable is confusing.

@@ +426,5 @@
> +    var self = this;
> +    return this.getOemUnlockEnabled().then( enabled => {
> +      self._libcutils.property_set(OEM_UNLOCK_PROPERTY,(enabled == true ? "1" : "0"));
> +      if(enabled === true){
> +        return self._formatPartition(enabled);

Why do we pass again |enabled| ?

@@ +442,5 @@
> +  },
> +
> +
> +  /**
> +   * Formats the persistent data partition with the propper strucutre.

typos :)

@@ +455,5 @@
> +    var self = this;
> +    var partition;
> +    return OS.File.open(this._dataBlockFile, {write:true, existing:true, append:false}).then( _partition => {
> +      partition = _partition;
> +      return partition.write(new Uint8Array(DIGEST_SIZE_BYTES));

We are writing an uninitialized array? Maybe we should rather explicitely set default values?

@@ +457,5 @@
> +    return OS.File.open(this._dataBlockFile, {write:true, existing:true, append:false}).then( _partition => {
> +      partition = _partition;
> +      return partition.write(new Uint8Array(DIGEST_SIZE_BYTES));
> +    }).then( bytesWrittenLength => {
> +      return partition.write(new Uint32Array([PARTITION_MAGIC]));

Same ?

@@ +459,5 @@
> +      return partition.write(new Uint8Array(DIGEST_SIZE_BYTES));
> +    }).then( bytesWrittenLength => {
> +      return partition.write(new Uint32Array([PARTITION_MAGIC]));
> +    }).then( bytesWrittenLength => {
> +      return partition.write(new Uint8Array(4));

Same ?

@@ +477,5 @@
> +
> +  /**
> +   * Check digest validity. If it's not, format the persistent partition
> +   * 
> +   * @return true Promise<bool> The checksum is valid so the prmomise is resolved to true

typo "prmomsise"

@@ +485,5 @@
> +  _enforceChecksumValidity: function() {
> +    debug("_enforceChecksumValidity");
> +    return this._computeDigest().then( digest => {
> +      debug("_enforceChecksumValidity: Digest computation succeed.");
> +      return Promise.resolve(true);

So the method "computeDigest" computes AND validates?

This is not what I read in this method :(

@@ +507,5 @@
> +  read: function() {
> +    debug("read()");
> +    var partition;
> +    var bytes;
> +    var totalDataSize = this.getDataFieldSize();

getDataFieldSize() ? But the variable is about total data size, and the way everything is written makes me thinking we are manipulating the whole block device, in parity with write() below.

@@ +514,5 @@
> +    }
> +    var self = this;
> +    return OS.File.open(this._dataBlockFile, {read:true, existing:true, append:false}).then( _partition => {
> +      partition = _partition;
> +      return partition.read(totalDataSize);

We do no seek ? So we are reading the whole device, including checksums etc. ?

@@ +549,5 @@
> +      return Promise.reject(-data.byteLength);
> +    }
> +
> +    var partition;
> +    var self = this;

Again, no self needed when you are using arrow functions.

@@ +594,5 @@
> +  wipe: function() {
> +    debug("wipe()");
> +
> +    if( this._mode.testing === true ){
> +      log("wipe: No wipe() funcionality in testing mode");

Is there really no way ? The limitations of that testing mode should be documented.

@@ +598,5 @@
> +      log("wipe: No wipe() funcionality in testing mode");
> +      return Promise.resolve();
> +    }
> +
> +#ifdef MOZ_WIDGET_GONK

read/write would be supported outside Gonk and not wipe?

@@ +602,5 @@
> +#ifdef MOZ_WIDGET_GONK
> +    const O_READONLY = 0;
> +    const O_RDWR = 2;
> +    const O_NONBLOCK = 1 << 11;
> +    const BLKSECDISCARD = 4733;

Please document where those two values are coming from and how to update them.

@@ +605,5 @@
> +    const O_NONBLOCK = 1 << 11;
> +    const BLKSECDISCARD = 4733;
> +    const BLKDISCARD = 4727;
> +
> +    var self = this;

NEIN§§§

@@ +624,5 @@
> +        return reject(-1);
> +      }
> +      let ret = self._libc.ioctl(fd, BLKSECDISCARD, rangeAddress);
> +      if(ret < 0){
> +        log("wipe: Something went wrong secure discarding block: errno: " + self.ctypes.errno);

That might be a bit confusing in a log file, make it clear that you are fallbacking on BLKDISCARD instead of BLKSECDISCARD. And if that one also fails, make it clear this is a very bad condition.

@@ +629,5 @@
> +        range[0] = 0;
> +        range[1] = blockDeviceLength;
> +        ret = self._libc.ioctl(fd, BLKDISCARD, rangeAddress);
> +        if(ret < 0){
> +          log("wipe: ERROR: discard failed: errno: " + self.ctypes.errno);

I would emphasize with a "CRITICAL:"

@@ +632,5 @@
> +        if(ret < 0){
> +          log("wipe: ERROR: discard failed: errno: " + self.ctypes.errno);
> +          return reject(-1);
> +        }else{
> +          log("wipe: non-secure discard used and succeed, because secure discard failed");

Looks good

@@ +662,5 @@
> +      return self._computeAndWriteDigest();
> +    }).then(() => {
> +      return Promise.resolve();
> +    });
> +  },

looks good

@@ +674,5 @@
> +  getOemUnlockEnabled: function() {
> +    log("getOemUnlockEnabled()");
> +    var ret = false;
> +    var partition;
> +    var self = this;

Still no need for self

@@ +707,5 @@
> +      //Jump the digest field
> +      return partition.setPosition(DIGEST_SIZE_BYTES, OS.File.POS_START);
> +    }).then( () => {
> +      //Read the Magic field
> +      return partition.read(4);

Shouldn't we avoid using hard coded value like 4 there? And it's the same for others.

@@ +710,5 @@
> +      //Read the Magic field
> +      return partition.read(4);
> +    }).then( _magic => {
> +      let magic = new Uint32Array(_magic.buffer);
> +      if( magic[0] === PARTITION_MAGIC )

Please add the brackets

@@ +718,5 @@
> +    }).then( _dataLength => {
> +      dataLength = new Uint32Array(_dataLength.buffer)[0];
> +      return partition.close();
> +    }).then( () => {
> +        if(dataLength)

Brackets and the indentation seems inconsistent with above

@@ +734,5 @@
> +   *
> +   * @return Promise<Number> A Promise resolved to the maximun number of bytes allowed for the data field
> +   *
> +   */
> +  getMaximunDataBlockSize: function() {

typo: getMaximun instead of getMaximum

::: b2g/components/test/unit/test_persistentdatablock_gonk.js
@@ +12,5 @@
> +
> +function run_test() {
> +  do_get_profile();
> +  Cu.import("resource://gre/modules/PersistentDataBlock.jsm");
> +  //We need to point to a valid partition for some of the tests. This is the /cache

Nit: Space after '//'

::: b2g/components/test/unit/xpcshell.ini
@@ +51,5 @@
>  # Bug 1193677: disable on B2G ICS Emulator for intermittent failures with IndexedDB
>  skip-if = ((toolkit != "gonk") || (toolkit == "gonk" && debug))
> +
> +[test_persistentdatablock_gonk.js]
> +head = head_persistentdatablock.js

Should rename head_persistentdatablock.js to file_persistentdatablock.js like all others

Comment 6

[Juan Gomez [:_AtilA_] (CET/CEST)]

(In reply to Alexandre LISSY :gerard-majax from comment #5)
> Comment on attachment 8684287 [details] [diff] [review]
> Persistent Data Block implementation and unit tests
>
> Review of attachment 8684287 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Looks good but I would like a few things to be fixed before going forward.
>
> ::: b2g/components/PersistentDataBlock.jsm
> @@ +37,5 @@
> > +XPCOMUtils.defineLazyModuleGetter(this, "OS", "resource://gre/modules/osfile.jsm");
> > +XPCOMUtils.defineLazyModuleGetter(this, "Promise", "resource://gre/modules/Promise.jsm");
> > +
> > +
> > +#ifdef MOZ_WIDGET_GONK
>
> I am not sure we need that since we run that test only on Gonk
Make sense, my plan was to fallback into something useless in case for some reason this component is being executed in non-Gonk environment. Applied the same logic in other parts of the code.

>
> @@ +165,5 @@
> > + },
> > +
> > + /*
> > + * Change the class behavior.
> > + * *** USE ONLY FOR TESTING ***
>
> Please make it clear here what we need to bypass and why.
>
> @@ +238,5 @@
> > + return Promise.reject();
> > + });
> > + },
> > +
> > + uninit: function() {
>
> nit: maybe moving the uninit() just below init(), for easier reading of the
> code
>
> @@ +282,5 @@
> > + _computeDigest: function (storedDigest) {
> > + debug("_computeDigest()");
> > + var digest;
> > + var partition;
> > + return OS.File.open(this._dataBlockFile, {existing:true, append:false, read:true}).then( _partition => {
>
> no space before the argument:
> .then(_partition => {
>
> @@ +286,5 @@
> > + return OS.File.open(this._dataBlockFile, {existing:true, append:false, read:true}).then( _partition => {
> > + partition = _partition;
> > + return partition.read(DIGEST_SIZE_BYTES);
> > + }).then( digestDataRead => {
> > + //If storedDigest is passed as a parameter, the caller will likely compare the
>
> Nit: '//' should be followed by space
>
> @@ +296,5 @@
> > + return partition.read();
> > + }).then( data => {
> > + //Calculate Digest with the data retrieved after the digest
> > + let hasher = Cc["@mozilla.org/security/hash;1"].createInstance(Ci.nsICryptoHash);
> > + hasher.init(hasher.SHA256);
>
> I guess SHA256 is because we only have 32 bytes to store the value ?
I just implemented the same logic as Android's. Vendors (like LGE) will probably already have tools which relay on this hash to work. I think that we shouldn't change it, should we?

>
> @@ +300,5 @@
> > + hasher.init(hasher.SHA256);
> > + hasher.update(data, data.length);
> > + let _digest = hasher.finish(true);
> > + digest = atob(_digest);
> > + debug("_computeDigest(): Digest = " + _digest + "(" + digest.length + ")");
>
> No way to reuse/do like dom/apps/AppsUtils.jsm:computeHash() ?
AppsUtils::computHash() method needs a string as an input parameter to work, but we are working with byte-arrays at this point, so I don't think it fits with our needs here. Anyway, atob() function will transform from Base64 to Binary, but hasher.finish(false) already gives the hash in binary format. I did this transformation because wanted to print the digest in a human-readable format in the log for future debugging purposes. We can just get
the binary format using .finish(false) and making it readable in logs by transforming to hexadecimal with a much simpler function (toHexString()), and get rid of b64ToUint6()/atob() methods. What do you think?

>
> @@ +319,5 @@
> > + _getBlockDeviceSize: function() {
> > + debug("_getBlockDeviceSize()");
> > +
> > + /* See _mode property */
> > + if( this._mode.testing === true ){
>
> Remove the spaces around the condition:
> if (this._mode.testing === true) {
>
> @@ +327,5 @@
> > +
> > +#ifdef MOZ_WIDGET_GONK
> > + const O_READONLY = 0;
> > + const O_NONBLOCK = 1 << 11;
> > + const BLKGETSIZE64 = 2147750514; //TODO: That's not the way. Gonk should provide a realiable method
>
> Until then, please document how to get that value :), and make it clear
> where it comes from/why it might change
I'm going to properly implement ioctl() related operations in Gonk. This method is higher unreliable because the values of ioctl() operations like BLKGETSIZE64 are masks which depends on specific kernel/drivers configuration, so there's no warranty to keep the values between kernel versions or even same kernel compilations for different devices.

>
> @@ +349,5 @@
> > + this._libc.close(fd);
> > + debug("_getBlockDeviceSize: size =" + size.value);
> > + return size.value;
> > +#else
> > + log("_getBlockDeviceSize: ERROR: This funcionality is only supported in Gonk!");
>
> Misalignment in indentation
>
> @@ +365,5 @@
> > + * Otherwise, sets it to 0.
> > + */
> > + _doSetOemUnlockEnabled: function(isSetOemUnlockEnabled){
> > + debug("_doSetOemUnlockEnabled()");
> > + var self = this;
>
> You don't need this since you are using arrow functions
>
> @@ +369,5 @@
> > + var self = this;
> > + var partition;
> > + return OS.File.open(this._dataBlockFile, {existing:true, append:false, write:true}).then( _partition => {
> > + partition = _partition;
> > + return partition.setPosition(self._getBlockDeviceSize() - 1, OS.File.POS_START);
>
> Use this. instead of self.
>
> @@ +372,5 @@
> > + partition = _partition;
> > + return partition.setPosition(self._getBlockDeviceSize() - 1, OS.File.POS_START);
> > + }).then( () => {
> > + return partition.write(new Uint8Array([ isSetOemUnlockEnabled === true ? 1 : 0 ]));
> > + }).then( bytesWrittenLength => {
>
> We make no checking on that value ?
OS.File.write() states that: "... may perform several I/O operations to ensure that the buffer is fully written.", anyway, this will be a cheap checking, so let's check :)

>
> @@ +394,5 @@
> > + */
> > + _computeAndWriteDigest: function(){
> > + debug("_computeAndWriteDigest()");
> > + var digest;
> > + var self = this;
>
> Same, remove.
>
> @@ +402,5 @@
> > + return OS.File.open(self._dataBlockFile, {write:true, existing:true, append:false});
> > + }).then( _partition => {
> > + partition = _partition;
> > + return partition.write(digest);
> > + }).then( bytesWrittenLength => {
>
> No check performed either?
>
> @@ +414,5 @@
> > + });
> > + },
> > +
> > + /**
> > + * Formats the persistent partition if the OEM Unlock Enabled field is set to true, and
>
> We will format that at each boot? The OEM unlock is the one we toggle from
> Fastboot?
The persistent partition will be formatted on every boot if:
* The stored digest is different from the computed one (so, there have been some kind of manipulation).
* The OEM Unlock Enabled byte is set to 1.
Don't know if this byte is toggled from Fastboot or from some other place, but this is the logic from Android implementation.
Formatting is about removing the content of the data field by setting to 0 the data length field, and setting the OEM Unlock Enabled field to 1 or 0 depending on the input parameter. A new digest is calculated and written as well.

>
> If so, we should be very clear that it means the feature won't work except
> on production-alike devices.
Not really sure if I understand what that means. Is that because of Fastboot is not enabled in production-alike devices, so we cannot set the OEM Unlock Enabled byte?
I actually provide a "public" method to set this field, so we can virtually change it whenever we want.

>
> @@ +423,5 @@
> > + */
> > + _formatIfOemUnlockEnabled: function () {
> > + debug("_formatIfOemUnlockEnabled()");
> > + var self = this;
> > + return this.getOemUnlockEnabled().then( enabled => {
>
> Maybe the name of that variable is confusing.
>
> @@ +426,5 @@
> > + var self = this;
> > + return this.getOemUnlockEnabled().then( enabled => {
> > + self._libcutils.property_set(OEM_UNLOCK_PROPERTY,(enabled == true ? "1" : "0"));
> > + if(enabled === true){
> > + return self._formatPartition(enabled);
>
> Why do we pass again |enabled| ?
Confusing? :). Yes, I can pass true so it'll be more readable.

>
> @@ +442,5 @@
> > + },
> > +
> > +
> > + /**
> > + * Formats the persistent data partition with the propper strucutre.
>
> typos :)
>
> @@ +455,5 @@
> > + var self = this;
> > + var partition;
> > + return OS.File.open(this._dataBlockFile, {write:true, existing:true, append:false}).then( _partition => {
> > + partition = _partition;
> > + return partition.write(new Uint8Array(DIGEST_SIZE_BYTES));
>
> We are writing an uninitialized array? Maybe we should rather explicitely
> set default values?
Ok, documentation explicitly states that they are initialized to 0 by default, and I saw code like that all over Gecko so I wrote it this way. Anyway, I don't like much either, so if we want to initialize to 0 ourserlfs, we just need to add a .fill(0).

>
> @@ +457,5 @@
> > + return OS.File.open(this._dataBlockFile, {write:true, existing:true, append:false}).then( _partition => {
> > + partition = _partition;
> > + return partition.write(new Uint8Array(DIGEST_SIZE_BYTES));
> > + }).then( bytesWrittenLength => {
> > + return partition.write(new Uint32Array([PARTITION_MAGIC]));
>
> Same ?
hmm nop, this is a slightly different initialization, notice the brackets []. This will initialize a one-element array which value will be PARTITION_MAGIC (a 32 bit value).

>
> @@ +459,5 @@
> > + return partition.write(new Uint8Array(DIGEST_SIZE_BYTES));
> > + }).then( bytesWrittenLength => {
> > + return partition.write(new Uint32Array([PARTITION_MAGIC]));
> > + }).then( bytesWrittenLength => {
> > + return partition.write(new Uint8Array(4));
>
> Same ?
>
> @@ +477,5 @@
> > +
> > + /**
> > + * Check digest validity. If it's not, format the persistent partition
> > + *
> > + * @return true Promise<bool> The checksum is valid so the prmomise is resolved to true
>
> typo "prmomsise"
>
> @@ +485,5 @@
> > + _enforceChecksumValidity: function() {
> > + debug("_enforceChecksumValidity");
> > + return this._computeDigest().then( digest => {
> > + debug("_enforceChecksumValidity: Digest computation succeed.");
> > + return Promise.resolve(true);
>
> So the method "computeDigest" computes AND validates?
>
> This is not what I read in this method :(

Nop, you've found a *bug* :(. computDigest will only compute the digest by reading all the bytes in the partition. Validation should be done outstide this method, by comparing the computed digest (returned value) with the stored digest in the partition (storedDigest *out* parameter).
I'll fix it.

>
> @@ +507,5 @@
> > + read: function() {
> > + debug("read()");
> > + var partition;
> > + var bytes;
> > + var totalDataSize = this.getDataFieldSize();
>
> getDataFieldSize() ? But the variable is about total data size, and the way
> everything is written makes me thinking we are manipulating the whole block
> device, in parity with write() below.
Ok, variable name is confusing. It's actually the size of the data field, not all the block. When we use write(data) to write into the data field, we need to rewrite digest, magic, data length and finally the data, but not the last byte (OEMUnlockEnabled), it's not even possible rewrite it with write().
I'll rename this variable.

>
> @@ +514,5 @@
> > + }
> > + var self = this;
> > + return OS.File.open(this._dataBlockFile, {read:true, existing:true, append:false}).then( _partition => {
> > + partition = _partition;
> > + return partition.read(totalDataSize);
>
> We do no seek ? So we are reading the whole device, including checksums etc.
> ?
Good point. Yes we DO need to seek.

>
> @@ +549,5 @@
> > + return Promise.reject(-data.byteLength);
> > + }
> > +
> > + var partition;
> > + var self = this;
>
> Again, no self needed when you are using arrow functions.
>
> @@ +594,5 @@
> > + wipe: function() {
> > + debug("wipe()");
> > +
> > + if( this._mode.testing === true ){
> > + log("wipe: No wipe() funcionality in testing mode");
>
> Is there really no way ? The limitations of that testing mode should be
> documented.
Wipe will fully remove the partition, that's it: data + format. So we need a partition that we can wipe without breaking the whole system. Emulators don't have this partition so far, so I'd prefer to skip this test.
Anyway, I'll document it in the code.

>
> @@ +598,5 @@
> > + log("wipe: No wipe() funcionality in testing mode");
> > + return Promise.resolve();
> > + }
> > +
> > +#ifdef MOZ_WIDGET_GONK
>
> read/write would be supported outside Gonk and not wipe?
Yep, reading and writing are supported via js APIs so they are multiplatform per se. Wipe implementation is going to be Gonk specific, as well as getting block device size.
The point here is not making this operations multiplatform so far, it can be done in the future if we need them, but for now, we will just fallback to something safe.

>
> @@ +602,5 @@
> > +#ifdef MOZ_WIDGET_GONK
> > + const O_READONLY = 0;
> > + const O_RDWR = 2;
> > + const O_NONBLOCK = 1 << 11;
> > + const BLKSECDISCARD = 4733;
>
> Please document where those two values are coming from and how to update
> them.
As said before, I'm going to properly implement this feature via Hal/Gonk.

>
> @@ +605,5 @@
> > + const O_NONBLOCK = 1 << 11;
> > + const BLKSECDISCARD = 4733;
> > + const BLKDISCARD = 4727;
> > +
> > + var self = this;
>
> NEIN§§§
hehehehe don't know why I used self.. I thought that inner arrow functions were using different context for this. 
I'll change all of them!!

>
> @@ +624,5 @@
> > + return reject(-1);
> > + }
> > + let ret = self._libc.ioctl(fd, BLKSECDISCARD, rangeAddress);
> > + if(ret < 0){
> > + log("wipe: Something went wrong secure discarding block: errno: " + self.ctypes.errno);
>
> That might be a bit confusing in a log file, make it clear that you are
> fallbacking on BLKDISCARD instead of BLKSECDISCARD. And if that one also
> fails, make it clear this is a very bad condition.
;)

>
> @@ +629,5 @@
> > + range[0] = 0;
> > + range[1] = blockDeviceLength;
> > + ret = self._libc.ioctl(fd, BLKDISCARD, rangeAddress);
> > + if(ret < 0){
> > + log("wipe: ERROR: discard failed: errno: " + self.ctypes.errno);
>
> I would emphasize with a "CRITICAL:"
>
> @@ +632,5 @@
> > + if(ret < 0){
> > + log("wipe: ERROR: discard failed: errno: " + self.ctypes.errno);
> > + return reject(-1);
> > + }else{
> > + log("wipe: non-secure discard used and succeed, because secure discard failed");
>
> Looks good
>
> @@ +662,5 @@
> > + return self._computeAndWriteDigest();
> > + }).then(() => {
> > + return Promise.resolve();
> > + });
> > + },
>
> looks good
>
> @@ +674,5 @@
> > + getOemUnlockEnabled: function() {
> > + log("getOemUnlockEnabled()");
> > + var ret = false;
> > + var partition;
> > + var self = this;
>
> Still no need for self
>
> @@ +707,5 @@
> > + //Jump the digest field
> > + return partition.setPosition(DIGEST_SIZE_BYTES, OS.File.POS_START);
> > + }).then( () => {
> > + //Read the Magic field
> > + return partition.read(4);
>
> Shouldn't we avoid using hard coded value like 4 there? And it's the same
> for others.
hmmmm... the documentation states that these fields should be 4 bytes (probably because they are unsigned integers). Anyway you are right, I'll use a const.

>
> @@ +710,5 @@
> > + //Read the Magic field
> > + return partition.read(4);
> > + }).then( _magic => {
> > + let magic = new Uint32Array(_magic.buffer);
> > + if( magic[0] === PARTITION_MAGIC )
>
> Please add the brackets
>
> @@ +718,5 @@
> > + }).then( _dataLength => {
> > + dataLength = new Uint32Array(_dataLength.buffer)[0];
> > + return partition.close();
> > + }).then( () => {
> > + if(dataLength)
>
> Brackets and the indentation seems inconsistent with above
>
> @@ +734,5 @@
> > + *
> > + * @return Promise<Number> A Promise resolved to the maximun number of bytes allowed for the data field
> > + *
> > + */
> > + getMaximunDataBlockSize: function() {
>
> typo: getMaximun instead of getMaximum
>
> ::: b2g/components/test/unit/test_persistentdatablock_gonk.js
> @@ +12,5 @@
> > +
> > +function run_test() {
> > + do_get_profile();
> > + Cu.import("resource://gre/modules/PersistentDataBlock.jsm");
> > + //We need to point to a valid partition for some of the tests. This is the /cache
>
> Nit: Space after '//'
>
> ::: b2g/components/test/unit/xpcshell.ini
> @@ +51,5 @@
> > # Bug 1193677: disable on B2G ICS Emulator for intermittent failures with IndexedDB
> > skip-if = ((toolkit != "gonk") || (toolkit == "gonk" && debug))
> > +
> > +[test_persistentdatablock_gonk.js]
> > +head = head_persistentdatablock.js
>
> Should rename head_persistentdatablock.js to file_persistentdatablock.js
> like all others

Ok!

Thanks for the review!! :)

Comment 7

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: Persistent Data Block implementation and unit tests (v2)

Changelog (relevant):
* After having a chat with Dave Hylands, I've finally implemented the ioctl() related methods using ctypes. We found that we can relay in the constant values of ioctl() operations. Anyway, I tested myself by making a simple C program and executing it on different environments (emulator-x86, emulator-arm, aries, flame and peak) and I found that the value changes depending on the underlying architecture (32 bits and 64 bits), so a fallback has been implemented to test with both architectures in case something wrong happens.

* Removed the btoa/atob and related functions, so now _computedDigest returns a binary string.

* enforceChecksumValidity() method fixed

* Some others tests has been fixed as well

* write() method fixed

Comment 8

[:gerard-majax]

Comment on attachment 8687153 [details] [diff] [review]
Persistent Data Block implementation and unit tests (v2)

Review of attachment 8687153 [details] [diff] [review]:
-----------------------------------------------------------------

There are still a lot of coding style nits to address :).

There are some tests that needs to be split I think, and some that looks to be lacking assertions. I would also like to see the testing going from bottom to up: making sure the first tests being executed are for the basics features/methods of the service, and then building on top of those. The patch is quite big so it might already be the case and I missed that :)

So, overall it's good, keep improving that but i'd like to have another look after you address the changes. Please, document what you fix for the next review pass :)

::: b2g/components/PersistentDataBlock.jsm
@@ +131,5 @@
> +  */
> +  init: function(mode) {
> +    debug("init()");
> +
> +    if(libcutils) {

nit: "if (", everywhere

@@ +165,5 @@
> +      } catch(ex) {
> +        log("Unable to open libc.so: ex = " + ex);
> +        throw Cr.NS_ERROR_FAILURE;
> +      }
> +#else

Maybe we should throw?

@@ +166,5 @@
> +        log("Unable to open libc.so: ex = " + ex);
> +        throw Cr.NS_ERROR_FAILURE;
> +      }
> +#else
> +      this._libc.handler.close  = () => { log("Only supported on Gonk!"); };

nit: extra space between "close" and "="

@@ +223,5 @@
> +  /**
> +   * Computes the digest of the entire data block.
> +   * The digest is saved in the first 32 bytes of the block.
> +   *
> +   * @param isStoredDigestReturned {Bool} An out parameter wich will tell the function to return

That is unclear: you are stating it is an "out parameter" but it is being used inside at line 246 and being read from. Is that the design from the Java implementation?

@@ +233,5 @@
> +   *                                  digest into the "stored" property.
> +   */
> +  _computeDigest: function (isStoredDigestReturned) {
> +    debug("_computeDigest()");
> +    var digest = {calculated: "", stored: ""};

nit: use let instead of var in methods

@@ +283,5 @@
> +    const O_READONLY = 0;
> +    const O_NONBLOCK = 1 << 11;
> +    /* Getting the correct values for ioctl() operations by reading the headers is not a trivial task, so
> +     * the better way to get the values below is by writting a simple test aplication in C that will
> +     * print the values to the output.

Maybe we should land that small app in gonk-misc/ ? To make it easy for anyone.

@@ +284,5 @@
> +    const O_NONBLOCK = 1 << 11;
> +    /* Getting the correct values for ioctl() operations by reading the headers is not a trivial task, so
> +     * the better way to get the values below is by writting a simple test aplication in C that will
> +     * print the values to the output.
> +     * 32bits and 64bits value for ioctl() BLKGETSIZE64 operation is different. So we will fallback in

What is the difference between both?

@@ +285,5 @@
> +    /* Getting the correct values for ioctl() operations by reading the headers is not a trivial task, so
> +     * the better way to get the values below is by writting a simple test aplication in C that will
> +     * print the values to the output.
> +     * 32bits and 64bits value for ioctl() BLKGETSIZE64 operation is different. So we will fallback in
> +     * case ioctl() returns EOTTY (22). */

typo: EOTTY => ENOTTY

@@ +317,5 @@
> +    this._libc.close(fd);
> +    debug("_getBlockDeviceSize: size =" + size.value);
> +    return size.value;
> +#else
> +  log("_getBlockDeviceSize: ERROR: This funcionality is only supported in Gonk!");

typo: functionnality (I would use "feature")

@@ +353,5 @@
> +      return Promise.resolve();
> +    }).catch(ex => {
> +      return Promise.reject();
> +    });
> +  },

That looks good :)

@@ +368,5 @@
> +    var digest;
> +    var partition;
> +    return this._computeDigest().then(_digest => {
> +      digest = _digest;
> +      return OS.File.open(this._dataBlockFile, {write:true, existing:true, append:false});

While the layout of the parition puts the digest first, I guess it would be nicer if we did explicitely seek to that position here: makes it more obvious, and just in case OS.File.open() tries to trick us :)

@@ +382,5 @@
> +    }).then(() => {
> +      debug("_computeAndWriteDigest: digest written to partition");
> +      return Promise.resolve(true);
> +    }).catch(ex => {
> +      log("_computeAndWritePARTITION_MADigest: Couldn't write digest in the persistent partion. ex = " + ex );

typo?

@@ +403,5 @@
> +        return this._formatPartition(true);
> +      }
> +      return Promise.resolve(false);
> +    }).then(result => {
> +      if(result && result === false)

nit: add "{" and "}"

@@ +415,5 @@
> +  },
> +
> +
> +  /**
> +   * Formats the persistent data partition with the propper structure.

typo: "proper"

@@ +455,5 @@
> +      return Promise.resolve();
> +    }).catch(ex => {
> +      log("_formatPartition: Failed to format block device!: ex = " + ex);
> +      return Promise.reject();
> +    });

Looks good.

@@ +459,5 @@
> +    });
> +  },
> +
> +  /**
> +   * Check digest validity. If it's not, format the persistent partition

"if it's not" seems like a part of the sentence is missing :)

@@ +518,5 @@
> +    }).catch(ex => {
> +      log("read: Failed to read entire data block. Exception: " + ex);
> +      return Promise.reject();
> +    });
> +  },

ok

@@ +573,5 @@
> +    }).catch(ex => {
> +      log("write: Failed to write to the persistent partition: ex = " + ex);
> +      return Promise.reject();
> +    });
> +  },

ok

@@ +593,5 @@
> +    const O_READONLY = 0;
> +    const O_RDWR = 2;
> +    const O_NONBLOCK = 1 << 11;
> +    // This constant value is the same under 32 and 64 bits arch.
> +    const BLKSECDISCARD = 4733;

Maybe we should note then as 0xABCD ?

@@ +598,5 @@
> +    // This constant value is the same under 32 and 64 bits arch.
> +    const BLKDISCARD = 4727;
> +
> +
> +

Nit: remove empty lines

@@ +617,5 @@
> +      }
> +      let ret = this._libc.ioctl(fd, BLKSECDISCARD, rangeAddress);
> +      if(ret < 0) {
> +        log("wipe: Something went wrong secure discarding block: errno: " + this.ctypes.errno + ": Falling back to non-secure discarding...");
> +        range[0] = 0;

Why do we re-set those values?

@@ +653,5 @@
> +      return this._computeAndWriteDigest();
> +    }).then(() => {
> +      return Promise.resolve();
> +    });
> +  },

ok

@@ +669,5 @@
> +    return OS.File.open(this._dataBlockFile, {existing:true, append:false, read:true}).then(_partition => {
> +      partition = _partition;
> +      return partition.setPosition(this._getBlockDeviceSize() - 1 , OS.File.POS_START);
> +    }).then(() => {
> +      return partition.read(1);

nit: avoid using hard coded constant, replace 1 with smething from a |const|.

@@ +680,5 @@
> +    }).catch(ex => {
> +      log("getOemUnlockEnabled: Error reading OEM unlock enabled byte from partition: ex = " + ex);
> +      return Promise.reject();
> +    });
> +  },

ok

@@ +721,5 @@
> +    }).catch(ex => {
> +      log("getDataFieldSize: Couldn't get data field size: ex = " + ex);
> +      return Promise.reject();
> +    });
> +  },

ok

@@ +735,5 @@
> +    return new Promise((resolve, reject) => {
> +      let actualSize = this._getBlockDeviceSize() - HEADER_SIZE_BYTES - 1;
> +      resolve(actualSize <= MAX_DATA_BLOCK_SIZE ? actualSize : MAX_DATA_BLOCK_SIZE);
> +    });
> +  }

ok

::: b2g/components/test/unit/file_persistentdatablock.js
@@ +10,5 @@
> +Cu.import("resource://gre/modules/NetUtil.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "OS", "resource://gre/modules/osfile.jsm");
> +const PARTITION_MAGIC =  0x19901873;

nit: extra space

@@ +12,5 @@
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "OS", "resource://gre/modules/osfile.jsm");
> +const PARTITION_MAGIC =  0x19901873;
> +const DIGEST_SIZE_BYTES = 32;
> +

Can't we reuse those values instead of redefining?

@@ +40,5 @@
> +  log("_prepareConfig: args.oem = " + args.oem );
> +  log("_prepareConfig: args.oemUnlockAllowed = " + args.oemUnlockAllowed );
> +
> +  /* This function will be called after passing all native stuff tests, so we will write into a file instead of a real
> +   * partition. Obviuosly, there are some native operations like getting the device block size or wipping, that will not

Can't we at least test them on another emulator partition? That will at least make sure this is working.

@@ +72,5 @@
> +  }).catch(ex => {
> +    log("_prepareConfig: ERROR: ex = " + ex);
> +    return Promise.reject();
> +  });
> +}

So at that moment, we have a fake FRP partition emulated within "/data/local/tmp/frp.test"

@@ +91,5 @@
> +    return Promise.resolve(byte[0]);
> +  }).catch(ex => {
> +    return Promise.reject();
> +  });
> +}

ok

@@ +98,5 @@
> +  var file;
> +  var header = {};
> +  return OS.File.open(PersistentDataBlock._dataBlockFile, {read:true, existing:true, append:false}).then(_file => {
> +    file = _file;
> +    return file.read(32);

Isnt' that kind of harcoded constant risky?

@@ +113,5 @@
> +    return Promise.resolve(header);
> +  }).catch(ex => {
> +    return Promise.reject();
> +  });
> +}

ok

@@ +135,5 @@
> +    return Promise.resolve(data);
> +  }).catch(ex => {
> +    return Promise.reject();
> +  });
> +}

ok

@@ +144,5 @@
> +  // Native operation tests go first
> +  // <NATIVE_TESTS>
> +  add_test(function test_getBlockDeviceSize() {
> +    // We will use emulator /cache partition to get it's size.
> +    PersistentDataBlock._dataBlockFile = "/dev/block/mtdblock2";

Can't we rely on platform/by-name path? Like for example "/dev/block/platform/msm_sdcc.1/by-name/cache"

@@ +150,5 @@
> +    // but we need to flip to testing mode after this test because we use files instead of partitions
> +    // and we cannot run this operation on files.
> +    PersistentDataBlock.setMode({testing:false});
> +    let blockSize = PersistentDataBlock._getBlockDeviceSize();
> +    ok(blockSize > 0, "test_getBlockDeviceSize: Block device size should be greater than 0");

Can't we better test than "> 0" ? Given it is emulator we should be able to make sure it's better than 0 and the actual value :)

Shouldn't we flip back the testing mode there? Now that we asserted

@@ +158,5 @@
> +  add_test(function test_wipe() {
> +    // Turning into testing mode again.
> +    PersistentDataBlock.setMode({testing:true});
> +    PersistentDataBlock.wipe().then(() => {
> +      // We don't evaluate anything because in testing mode we always return ok!

:( testing without testing is useless testing? Can't we do a bit better?

@@ +171,5 @@
> +  add_test(function test_computeDigest() {
> +    _prepareConfig().then(() => {
> +      PersistentDataBlock._computeDigest().then(digest => {
> +        const _EXPECTED_VALUE = "0004107e05f0e20dd0aa0ed0110900e01d0260100300bd04409e0cc04b0650be02e09909f0860f00fc05b033000d0";
> +        strictEqual([toHexString(digest.calculated.charCodeAt(i)) for (i in digest.calculated)].join(""), _EXPECTED_VALUE);

That condition really feels to complex to read to me. I guess we should separate the part:
> [toHexString(digest.calculated.charCodeAt(i)) for (i in digest.calculated)].join("")

into a separate variable:
> var calculated = [toHexString(digest.calculated.charCodeAt(i)) for (i in digest.calculated)].join("")
> strictEqual(calculated, _EXPECTED_VALUE);

@@ +187,5 @@
> +      run_next_test();
> +    }).catch(ex => {
> +      ok(false, "test_getOemUnlockedEnabled failed: ex:" + ex);
> +    });
> +  });

ok

@@ +193,5 @@
> +  add_test(function test_getOemUnlockedEnabled() {
> +    PersistentDataBlock.getOemUnlockEnabled().then(enabled => {
> +      log("test_getOemUnlockedEnabled: enabled is " + enabled);
> +      ok(enabled, "test_getOemUnlockedEnabled: should get enabled");
> +      ok(enabled === false || enabled === true, "test_getOemUnlockedEnabled failed, value of enabled is not true or false");

I feel like we will not know if we are testing what we want.

I would rather see that test splitted in several cases, to make sure that when we should have |enabled| being false (i.e., bit is 0) the value is really and only false. And the same for truth case.

@@ +209,5 @@
> +      strictEqual(byte, 1);
> +      return PersistentDataBlock.setOemUnlockEnabled(false);
> +    }).then(() => {
> +      return utils_getByteAt(PersistentDataBlock._getBlockDeviceSize() - 1);
> +    }).then(byte => {

Ok, but I think we should split that in two cases too.

@@ +232,5 @@
> +      run_next_test();
> +    }).catch(ex => {
> +      ok(false, "test_computeAndWriteDigest failed!: ex: " + ex);
> +    });
> +  });

ok

@@ +237,5 @@
> +
> +  add_test(function test_formatIfOemUnlockEnabled() {
> +    _prepareConfig({oem:true}).then(() => {
> +      return PersistentDataBlock._formatIfOemUnlockEnabled();
> +    }).then(result => {

Shouldn't we do something with result? Like testing if format was done really?

@@ +240,5 @@
> +      return PersistentDataBlock._formatIfOemUnlockEnabled();
> +    }).then(result => {
> +      return utils_getByteAt(PersistentDataBlock._getBlockDeviceSize() - 1);
> +    }).then(byte => {
> +      // Check if the last byte is 1

useless comment is useless :), better to say that we are testing if the "oemUnlockEnabled" field is 1

@@ +255,5 @@
> +      return PersistentDataBlock._formatPartition(true);
> +    }).then(() => {
> +      return utils_getByteAt(PersistentDataBlock._getBlockDeviceSize() - 1);
> +    }).then(byte => {
> +      // Check if the last byte is 1

Same remark about the comment

@@ +265,5 @@
> +      let magicSupposed = new Uint32Array([PARTITION_MAGIC]);
> +      strictEqual(magicRead[0], magicSupposed[0]);
> +      // In a formatted partition, the digest field is always 32 bytes of zeros.
> +      let digestSupposed = new Uint8Array(DIGEST_SIZE_BYTES);
> +      strictEqual(header.digest.join(""), "94227253995810864198417798821014713171138121254110134189198178208133167236184116199");

Please make sure we have proper documentation on how to compute/update those hardcoded digest values, for future

@@ +270,5 @@
> +      return PersistentDataBlock._formatPartition(false);
> +    }).then(() => {
> +      return utils_getByteAt(PersistentDataBlock._getBlockDeviceSize() - 1);
> +    }).then(byte => {
> +      // In this case OEM Unlock enabled byte should be set to 0

Make it clear it should be "0" because we passed "false" earlier, that helps understanding the code

@@ +279,5 @@
> +    });
> +  });
> +
> +  add_test(function test_enforceChecksumValidity() {
> +    //We need a valid partition layout to pass this test

Nit: missing space after "//"

@@ +282,5 @@
> +  add_test(function test_enforceChecksumValidity() {
> +    //We need a valid partition layout to pass this test
> +    _prepareConfig().then(() => {
> +      PersistentDataBlock._enforceChecksumValidity().then(() => {
> +        run_next_test();

We don't assert anything?

@@ +291,5 @@
> +  });
> +
> +  add_test(function test_read() {
> +    // Before reading, let's write some bytes of data first.
> +    PersistentDataBlock.write(new Uint8Array([1,2,3,4])).then(() => {

Should we really make use of PersistentDataBlock.write() before testing it? And should we make use of that at all when we are testing read() ? Shouldn't we write those values directly to our partition/file used for testing?

@@ +321,5 @@
> +      run_next_test();
> +    }).catch(ex => {
> +      ok(false, "test_write failed!: ex: " + ex);
> +    });
> +  });

ok

::: b2g/components/test/unit/test_persistentdatablock_gonk.js
@@ +12,5 @@
> +
> +function run_test() {
> +  do_get_profile();
> +  Cu.import("resource://gre/modules/PersistentDataBlock.jsm");
> +  //We need to point to a valid partition for some of the tests. This is the /cache

nit: spacing between "//" and the comment

Comment 9

[Michael:mtreese]

supports KS 2.6

Comment 10

[Juan Gomez [:_AtilA_] (CET/CEST)]

(In reply to Alexandre LISSY :gerard-majax from comment #8)

WOW! Pretty detailed review, nice! :)
My answers in the comments.

> Comment on attachment 8687153 [details] [diff] [review]
> Persistent Data Block implementation and unit tests (v2)
> 
> Review of attachment 8687153 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There are still a lot of coding style nits to address :).
> 
> There are some tests that needs to be split I think, and some that looks to
> be lacking assertions. I would also like to see the testing going from
> bottom to up: making sure the first tests being executed are for the basics
> features/methods of the service, and then building on top of those. The
> patch is quite big so it might already be the case and I missed that :)

My plan was to test partition required things together, like wipe/get-block-device-size, and then
all the methods one by one, following some order in some cases that makes sense: for example, I wanted
to test _computeDigest before enforceChecksumValidity, or write before read, because read needs a previous
write.

> 
> So, overall it's good, keep improving that but i'd like to have another look
> after you address the changes. Please, document what you fix for the next
> review pass :)
> 
> ::: b2g/components/PersistentDataBlock.jsm
> @@ +131,5 @@
> > +  */
> > +  init: function(mode) {
> > +    debug("init()");
> > +
> > +    if(libcutils) {
> 
> nit: "if (", everywhere
> 
> @@ +165,5 @@
> > +      } catch(ex) {
> > +        log("Unable to open libc.so: ex = " + ex);
> > +        throw Cr.NS_ERROR_FAILURE;
> > +      }
> > +#else
> 
> Maybe we should throw?

I just fallback to something safer, that dumps some info in the logs.

> 
> @@ +166,5 @@
> > +        log("Unable to open libc.so: ex = " + ex);
> > +        throw Cr.NS_ERROR_FAILURE;
> > +      }
> > +#else
> > +      this._libc.handler.close  = () => { log("Only supported on Gonk!"); };
> 
> nit: extra space between "close" and "="
> 
> @@ +223,5 @@
> > +  /**
> > +   * Computes the digest of the entire data block.
> > +   * The digest is saved in the first 32 bytes of the block.
> > +   *
> > +   * @param isStoredDigestReturned {Bool} An out parameter wich will tell the function to return
> 
> That is unclear: you are stating it is an "out parameter" but it is being
> used inside at line 246 and being read from. Is that the design from the
> Java implementation?

That was a remain. I updated it. There's no such *out* parameters in JavaScript, so I changed it but forgot
to update the comment :)

> 
> @@ +233,5 @@
> > +   *                                  digest into the "stored" property.
> > +   */
> > +  _computeDigest: function (isStoredDigestReturned) {
> > +    debug("_computeDigest()");
> > +    var digest = {calculated: "", stored: ""};
> 
> nit: use let instead of var in methods
> 
> @@ +283,5 @@
> > +    const O_READONLY = 0;
> > +    const O_NONBLOCK = 1 << 11;
> > +    /* Getting the correct values for ioctl() operations by reading the headers is not a trivial task, so
> > +     * the better way to get the values below is by writting a simple test aplication in C that will
> > +     * print the values to the output.
> 
> Maybe we should land that small app in gonk-misc/ ? To make it easy for
> anyone.

Hmmm, sounds good. I will.

> 
> @@ +284,5 @@
> > +    const O_NONBLOCK = 1 << 11;
> > +    /* Getting the correct values for ioctl() operations by reading the headers is not a trivial task, so
> > +     * the better way to get the values below is by writting a simple test aplication in C that will
> > +     * print the values to the output.
> > +     * 32bits and 64bits value for ioctl() BLKGETSIZE64 operation is different. So we will fallback in
> 
> What is the difference between both?

The ioctl() backend for this operation depends on types which size changes between 32 and 64 bits arch: http://lxr.free-electrons.com/source/block/ioctl.c#L444

> 
> @@ +285,5 @@
> > +    /* Getting the correct values for ioctl() operations by reading the headers is not a trivial task, so
> > +     * the better way to get the values below is by writting a simple test aplication in C that will
> > +     * print the values to the output.
> > +     * 32bits and 64bits value for ioctl() BLKGETSIZE64 operation is different. So we will fallback in
> > +     * case ioctl() returns EOTTY (22). */
> 
> typo: EOTTY => ENOTTY
> 
> @@ +317,5 @@
> > +    this._libc.close(fd);
> > +    debug("_getBlockDeviceSize: size =" + size.value);
> > +    return size.value;
> > +#else
> > +  log("_getBlockDeviceSize: ERROR: This funcionality is only supported in Gonk!");
> 
> typo: functionnality (I would use "feature")
> 
> @@ +353,5 @@
> > +      return Promise.resolve();
> > +    }).catch(ex => {
> > +      return Promise.reject();
> > +    });
> > +  },
> 
> That looks good :)
> 
> @@ +368,5 @@
> > +    var digest;
> > +    var partition;
> > +    return this._computeDigest().then(_digest => {
> > +      digest = _digest;
> > +      return OS.File.open(this._dataBlockFile, {write:true, existing:true, append:false});
> 
> While the layout of the parition puts the digest first, I guess it would be
> nicer if we did explicitely seek to that position here: makes it more
> obvious, and just in case OS.File.open() tries to trick us :)

Well, append:false will set the file pointer to the beginning. Anyway, you are suggesting to make a setPosition(0, POS_START) before writing, right?

> 
> @@ +382,5 @@
> > +    }).then(() => {
> > +      debug("_computeAndWriteDigest: digest written to partition");
> > +      return Promise.resolve(true);
> > +    }).catch(ex => {
> > +      log("_computeAndWritePARTITION_MADigest: Couldn't write digest in the persistent partion. ex = " + ex );
> 
> typo?

Mysterious ctrl + v I guess :) 

> 
> @@ +403,5 @@
> > +        return this._formatPartition(true);
> > +      }
> > +      return Promise.resolve(false);
> > +    }).then(result => {
> > +      if(result && result === false)
> 
> nit: add "{" and "}"
> 
> @@ +415,5 @@
> > +  },
> > +
> > +
> > +  /**
> > +   * Formats the persistent data partition with the propper structure.
> 
> typo: "proper"
> 
> @@ +455,5 @@
> > +      return Promise.resolve();
> > +    }).catch(ex => {
> > +      log("_formatPartition: Failed to format block device!: ex = " + ex);
> > +      return Promise.reject();
> > +    });
> 
> Looks good.
> 
> @@ +459,5 @@
> > +    });
> > +  },
> > +
> > +  /**
> > +   * Check digest validity. If it's not, format the persistent partition
> 
> "if it's not" seems like a part of the sentence is missing :)
> 
> @@ +518,5 @@
> > +    }).catch(ex => {
> > +      log("read: Failed to read entire data block. Exception: " + ex);
> > +      return Promise.reject();
> > +    });
> > +  },
> 
> ok
> 
> @@ +573,5 @@
> > +    }).catch(ex => {
> > +      log("write: Failed to write to the persistent partition: ex = " + ex);
> > +      return Promise.reject();
> > +    });
> > +  },
> 
> ok
> 
> @@ +593,5 @@
> > +    const O_READONLY = 0;
> > +    const O_RDWR = 2;
> > +    const O_NONBLOCK = 1 << 11;
> > +    // This constant value is the same under 32 and 64 bits arch.
> > +    const BLKSECDISCARD = 4733;
> 
> Maybe we should note then as 0xABCD ?

ok, changed to hexadecimal notation.

> 
> @@ +598,5 @@
> > +    // This constant value is the same under 32 and 64 bits arch.
> > +    const BLKDISCARD = 4727;
> > +
> > +
> > +
> 
> Nit: remove empty lines
> 
> @@ +617,5 @@
> > +      }
> > +      let ret = this._libc.ioctl(fd, BLKSECDISCARD, rangeAddress);
> > +      if(ret < 0) {
> > +        log("wipe: Something went wrong secure discarding block: errno: " + this.ctypes.errno + ": Falling back to non-secure discarding...");
> > +        range[0] = 0;
> 
> Why do we re-set those values?

Sure, we don't need to. JavaScript shouldn't change their values after calling ioctl.

> 
> @@ +653,5 @@
> > +      return this._computeAndWriteDigest();
> > +    }).then(() => {
> > +      return Promise.resolve();
> > +    });
> > +  },
> 
> ok
> 
> @@ +669,5 @@
> > +    return OS.File.open(this._dataBlockFile, {existing:true, append:false, read:true}).then(_partition => {
> > +      partition = _partition;
> > +      return partition.setPosition(this._getBlockDeviceSize() - 1 , OS.File.POS_START);
> > +    }).then(() => {
> > +      return partition.read(1);
> 
> nit: avoid using hard coded constant, replace 1 with smething from a |const|.
> 
> @@ +680,5 @@
> > +    }).catch(ex => {
> > +      log("getOemUnlockEnabled: Error reading OEM unlock enabled byte from partition: ex = " + ex);
> > +      return Promise.reject();
> > +    });
> > +  },
> 
> ok
> 
> @@ +721,5 @@
> > +    }).catch(ex => {
> > +      log("getDataFieldSize: Couldn't get data field size: ex = " + ex);
> > +      return Promise.reject();
> > +    });
> > +  },
> 
> ok
> 
> @@ +735,5 @@
> > +    return new Promise((resolve, reject) => {
> > +      let actualSize = this._getBlockDeviceSize() - HEADER_SIZE_BYTES - 1;
> > +      resolve(actualSize <= MAX_DATA_BLOCK_SIZE ? actualSize : MAX_DATA_BLOCK_SIZE);
> > +    });
> > +  }
> 
> ok
> 
> ::: b2g/components/test/unit/file_persistentdatablock.js
> @@ +10,5 @@
> > +Cu.import("resource://gre/modules/NetUtil.jsm");
> > +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> > +
> > +XPCOMUtils.defineLazyModuleGetter(this, "OS", "resource://gre/modules/osfile.jsm");
> > +const PARTITION_MAGIC =  0x19901873;
> 
> nit: extra space
> 
> @@ +12,5 @@
> > +
> > +XPCOMUtils.defineLazyModuleGetter(this, "OS", "resource://gre/modules/osfile.jsm");
> > +const PARTITION_MAGIC =  0x19901873;
> > +const DIGEST_SIZE_BYTES = 32;
> > +
> 
> Can't we reuse those values instead of redefining?
> 
> @@ +40,5 @@
> > +  log("_prepareConfig: args.oem = " + args.oem );
> > +  log("_prepareConfig: args.oemUnlockAllowed = " + args.oemUnlockAllowed );
> > +
> > +  /* This function will be called after passing all native stuff tests, so we will write into a file instead of a real
> > +   * partition. Obviuosly, there are some native operations like getting the device block size or wipping, that will not
> 
> Can't we at least test them on another emulator partition? That will at
> least make sure this is working.

The problem is that we'll mess the partition up if we test on it. The first time I tried testing on cache partition, but I needed to rebuild cache.img every time I run the tests, so basically, there's no way to 
test all the stuff with the actual emulators partition layout.
 
> 
> @@ +72,5 @@
> > +  }).catch(ex => {
> > +    log("_prepareConfig: ERROR: ex = " + ex);
> > +    return Promise.reject();
> > +  });
> > +}
> 
> So at that moment, we have a fake FRP partition emulated within
> "/data/local/tmp/frp.test"

Right

> 
> @@ +91,5 @@
> > +    return Promise.resolve(byte[0]);
> > +  }).catch(ex => {
> > +    return Promise.reject();
> > +  });
> > +}
> 
> ok
> 
> @@ +98,5 @@
> > +  var file;
> > +  var header = {};
> > +  return OS.File.open(PersistentDataBlock._dataBlockFile, {read:true, existing:true, append:false}).then(_file => {
> > +    file = _file;
> > +    return file.read(32);
> 
> Isnt' that kind of harcoded constant risky?

The right way to do it is by asking the implementation the size of the header, I didn't want to expose these internal details just to make the class simpler and cleaner.

> 
> @@ +113,5 @@
> > +    return Promise.resolve(header);
> > +  }).catch(ex => {
> > +    return Promise.reject();
> > +  });
> > +}
> 
> ok
> 
> @@ +135,5 @@
> > +    return Promise.resolve(data);
> > +  }).catch(ex => {
> > +    return Promise.reject();
> > +  });
> > +}
> 
> ok
> 
> @@ +144,5 @@
> > +  // Native operation tests go first
> > +  // <NATIVE_TESTS>
> > +  add_test(function test_getBlockDeviceSize() {
> > +    // We will use emulator /cache partition to get it's size.
> > +    PersistentDataBlock._dataBlockFile = "/dev/block/mtdblock2";
> 
> Can't we rely on platform/by-name path? Like for example
> "/dev/block/platform/msm_sdcc.1/by-name/cache"

I looked for various ways, but the emulator don't have any sysfs or devfs link to something more readable :( 

> 
> @@ +150,5 @@
> > +    // but we need to flip to testing mode after this test because we use files instead of partitions
> > +    // and we cannot run this operation on files.
> > +    PersistentDataBlock.setMode({testing:false});
> > +    let blockSize = PersistentDataBlock._getBlockDeviceSize();
> > +    ok(blockSize > 0, "test_getBlockDeviceSize: Block device size should be greater than 0");
> 
> Can't we better test than "> 0" ? Given it is emulator we should be able to
> make sure it's better than 0 and the actual value :)

Oh, not really sure if the size of the partitions will remain untouched between versions. if it does, yes, of course, we can put the size of the partition there :)

>
> Shouldn't we flip back the testing mode there? Now that we asserted
> 
> @@ +158,5 @@
> > +  add_test(function test_wipe() {
> > +    // Turning into testing mode again.
> > +    PersistentDataBlock.setMode({testing:true});
> > +    PersistentDataBlock.wipe().then(() => {
> > +      // We don't evaluate anything because in testing mode we always return ok!
> 
> :( testing without testing is useless testing? Can't we do a bit better?

hmmmm well, we can just remove this test then, because wiping a partition will disable it.
Don't know how to test it without actually creating the partition in the emulator.

> 
> @@ +171,5 @@
> > +  add_test(function test_computeDigest() {
> > +    _prepareConfig().then(() => {
> > +      PersistentDataBlock._computeDigest().then(digest => {
> > +        const _EXPECTED_VALUE = "0004107e05f0e20dd0aa0ed0110900e01d0260100300bd04409e0cc04b0650be02e09909f0860f00fc05b033000d0";
> > +        strictEqual([toHexString(digest.calculated.charCodeAt(i)) for (i in digest.calculated)].join(""), _EXPECTED_VALUE);
> 
> That condition really feels to complex to read to me. I guess we should
> separate the part:
> > [toHexString(digest.calculated.charCodeAt(i)) for (i in digest.calculated)].join("")
> 
> into a separate variable:
> > var calculated = [toHexString(digest.calculated.charCodeAt(i)) for (i in digest.calculated)].join("")
> > strictEqual(calculated, _EXPECTED_VALUE);

Done! :)

> 
> @@ +187,5 @@
> > +      run_next_test();
> > +    }).catch(ex => {
> > +      ok(false, "test_getOemUnlockedEnabled failed: ex:" + ex);
> > +    });
> > +  });
> 
> ok
> 
> @@ +193,5 @@
> > +  add_test(function test_getOemUnlockedEnabled() {
> > +    PersistentDataBlock.getOemUnlockEnabled().then(enabled => {
> > +      log("test_getOemUnlockedEnabled: enabled is " + enabled);
> > +      ok(enabled, "test_getOemUnlockedEnabled: should get enabled");
> > +      ok(enabled === false || enabled === true, "test_getOemUnlockedEnabled failed, value of enabled is not true or false");
> 
> I feel like we will not know if we are testing what we want.
> 
> I would rather see that test splitted in several cases, to make sure that
> when we should have |enabled| being false (i.e., bit is 0) the value is
> really and only false. And the same for truth case.

Yep, splitted in two tests.

> 
> @@ +209,5 @@
> > +      strictEqual(byte, 1);
> > +      return PersistentDataBlock.setOemUnlockEnabled(false);
> > +    }).then(() => {
> > +      return utils_getByteAt(PersistentDataBlock._getBlockDeviceSize() - 1);
> > +    }).then(byte => {
> 
> Ok, but I think we should split that in two cases too.

Splitted too.

> 
> @@ +232,5 @@
> > +      run_next_test();
> > +    }).catch(ex => {
> > +      ok(false, "test_computeAndWriteDigest failed!: ex: " + ex);
> > +    });
> > +  });
> 
> ok
> 
> @@ +237,5 @@
> > +
> > +  add_test(function test_formatIfOemUnlockEnabled() {
> > +    _prepareConfig({oem:true}).then(() => {
> > +      return PersistentDataBlock._formatIfOemUnlockEnabled();
> > +    }).then(result => {
> 
> Shouldn't we do something with result? Like testing if format was done
> really?

I splitted this test too. One with ome:true, and other with oem:false, so I can assert if result value is not the expected one for each case.

> 
> @@ +240,5 @@
> > +      return PersistentDataBlock._formatIfOemUnlockEnabled();
> > +    }).then(result => {
> > +      return utils_getByteAt(PersistentDataBlock._getBlockDeviceSize() - 1);
> > +    }).then(byte => {
> > +      // Check if the last byte is 1
> 
> useless comment is useless :), better to say that we are testing if the
> "oemUnlockEnabled" field is 1
> 
> @@ +255,5 @@
> > +      return PersistentDataBlock._formatPartition(true);
> > +    }).then(() => {
> > +      return utils_getByteAt(PersistentDataBlock._getBlockDeviceSize() - 1);
> > +    }).then(byte => {
> > +      // Check if the last byte is 1
> 
> Same remark about the comment
> 
> @@ +265,5 @@
> > +      let magicSupposed = new Uint32Array([PARTITION_MAGIC]);
> > +      strictEqual(magicRead[0], magicSupposed[0]);
> > +      // In a formatted partition, the digest field is always 32 bytes of zeros.
> > +      let digestSupposed = new Uint8Array(DIGEST_SIZE_BYTES);
> > +      strictEqual(header.digest.join(""), "94227253995810864198417798821014713171138121254110134189198178208133167236184116199");
> 
> Please make sure we have proper documentation on how to compute/update those
> hardcoded digest values, for future

Uh!... it's just: run the test once, get the values, update the constants and run again... I did
comment it at the _prepareConfig method. I'll comment here anyway. 

> 
> @@ +270,5 @@
> > +      return PersistentDataBlock._formatPartition(false);
> > +    }).then(() => {
> > +      return utils_getByteAt(PersistentDataBlock._getBlockDeviceSize() - 1);
> > +    }).then(byte => {
> > +      // In this case OEM Unlock enabled byte should be set to 0
> 
> Make it clear it should be "0" because we passed "false" earlier, that helps
> understanding the code
> 
> @@ +279,5 @@
> > +    });
> > +  });
> > +
> > +  add_test(function test_enforceChecksumValidity() {
> > +    //We need a valid partition layout to pass this test
> 
> Nit: missing space after "//"
> 
> @@ +282,5 @@
> > +  add_test(function test_enforceChecksumValidity() {
> > +    //We need a valid partition layout to pass this test
> > +    _prepareConfig().then(() => {
> > +      PersistentDataBlock._enforceChecksumValidity().then(() => {
> > +        run_next_test();
> 
> We don't assert anything?

Well, in this case, if the method resolves the promise means that all went right. It there's a problem the method will reject.

> 
> @@ +291,5 @@
> > +  });
> > +
> > +  add_test(function test_read() {
> > +    // Before reading, let's write some bytes of data first.
> > +    PersistentDataBlock.write(new Uint8Array([1,2,3,4])).then(() => {
> 
> Should we really make use of PersistentDataBlock.write() before testing it?
> And should we make use of that at all when we are testing read() ? Shouldn't
> we write those values directly to our partition/file used for testing?

Well I though that the safest way to write something that could be read later is by using the class write()
method, because it doesn't only write to the data field, it will update de digest and data field size as well.
I could make a helper like utils_getHeader() or utils_getByteAt(), but the implementation will be the same as
the class write() method.

> 
> @@ +321,5 @@
> > +      run_next_test();
> > +    }).catch(ex => {
> > +      ok(false, "test_write failed!: ex: " + ex);
> > +    });
> > +  });
> 
> ok
> 
> ::: b2g/components/test/unit/test_persistentdatablock_gonk.js
> @@ +12,5 @@
> > +
> > +function run_test() {
> > +  do_get_profile();
> > +  Cu.import("resource://gre/modules/PersistentDataBlock.jsm");
> > +  //We need to point to a valid partition for some of the tests. This is the /cache
> 
> nit: spacing between "//" and the comment

Great review Alex! thanks!
Expect a patch soon :)

Comment 11

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: 0001-Bug-1214515-PersistentDataBlock-b2g-component-implem.patch (v3)

Changelog:
* Fixed all the nits
* Fixed all coding styles
* Fixed some wrong comments and added information about how to calculate the digests on tests
* Split test_getOemUnlockedEnabled,test_setOemUnlockedEnabled,  test_formatIfOemUnlockEnabled in two.
* Explicitly positioning at the beginning of the partition in _computeAndWriteDigest() method.
* Changed most of the hardcoded numerics by constants

Comment 12

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: 0001-Bug-1214515-PersistentDataBlock-b2g-component-implem.patch (v4)

Changelog:
* Some coding styles nits fixed.

Comment 13

[:gerard-majax]

Comment on attachment 8691470 [details] [diff] [review]
0001-Bug-1214515-PersistentDataBlock-b2g-component-implem.patch (v4)

Review of attachment 8691470 [details] [diff] [review]:
-----------------------------------------------------------------

Overall good, but there are still coding style nits to fix. Also, I think we want to make sure any promise rejection contains the underlying exception that we caught.

::: b2g/components/PersistentDataBlock.jsm
@@ +12,5 @@
> + */
> +
> +"use strict";
> +
> +const DEBUG = true;

make sure to remove that before landing

@@ +40,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "OS", "resource://gre/modules/osfile.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Promise", "resource://gre/modules/Promise.jsm");
> +

nit: extra new line

@@ +47,5 @@
> +  Cu.import("resource://gre/modules/systemlibs.js");
> +  return libcutils;
> +});
> +
> +

nit: extra new line

@@ +97,5 @@
> +
> +  /**
> +   * Data block file
> +   */
> +  _dataBlockFile: "",

undefined ?

@@ +104,5 @@
> +  * Change the behavior of the class for some methods. Right now the only possible mode is
> +  * "testing" (String), which will fake the return value of some methods realted
> +  * to native operations with block devices, just for testing purposes.
> +  */
> +  _mode: {

we probably don't need to have _mode: { testing: bool }, just _testing: bool should be enough for now?

@@ +131,5 @@
> +  /**
> +  * Initialize the class.
> +  *
> +  */
> +  init: function(mode) {

shouldn't we guard against multiple .init() calls ?

@@ +169,5 @@
> +        log("Unable to open libc.so: ex = " + ex);
> +        throw Cr.NS_ERROR_FAILURE;
> +      }
> +#else
> +      throw Cr.NS_ERROR_ABORT;

maybe add a log?

@@ +173,5 @@
> +      throw Cr.NS_ERROR_ABORT;
> +#endif
> +    }
> +
> +

nit: extra empty new line

@@ +189,5 @@
> +    this._libc.handler.close();
> +    Services.obs.removeObserver(this, XPCOM_SHUTDOWN_OBSERVER_TOPIC);
> +  },
> +
> +  start: function() {

Who is responsible for calling this? There is no comment, and that component only calls .init().

@@ +210,5 @@
> +
> +    return true;
> +  },
> +
> +  // addObserver

useless comment :)

@@ +237,5 @@
> +   *                                  digest into the "stored" property.
> +   */
> +  _computeDigest: function (isStoredDigestReturned) {
> +    debug("_computeDigest()");
> +    let digest = {calculated: "", stored: ""};

using |undefined| instead of |""| ?

also, s/calculated/computed

@@ +310,5 @@
> +        log("_getBlockDeviceSize: errno is ENOTTY, falling back to 64 bit version of BLKGETSIZE64...");
> +        ret = this._libc.ioctl(fd, BLKGETSIZE64_64_BITS, sizeAddress);
> +        if (ret < 0){
> +          log("_getBlockDeviceSize: BLKGETSIZE64 failed again!. errno = " + this.ctypes.errno);
> +          throw Cr.NS_ERROR_FAILURE;

aren't we leaking fd ?

@@ +314,5 @@
> +          throw Cr.NS_ERROR_FAILURE;
> +        }
> +      }else{
> +        log("_getBlockDeviceSize: couldn't get block device size!: errno = " + this.ctypes.errno);
> +        throw Cr.NS_ERROR_FAILURE;

same, I fear we are leaking fd

@@ +322,5 @@
> +    debug("_getBlockDeviceSize: size =" + size.value);
> +    return size.value;
> +#else
> +  log("_getBlockDeviceSize: ERROR: This feature is only supported in Gonk!");
> +  return -1;

nit: bogus indentation

@@ +355,5 @@
> +      debug("_doSetOemUnlockEnabled: OEM unlock enabled written to " + oemUnlockByte);
> +      this._libcutils.property_set(OEM_UNLOCK_PROPERTY, oemUnlockByte);
> +      return Promise.resolve();
> +    }).catch(ex => {
> +      return Promise.reject();

rejection should include the exception we caught, for easier error handling above?

@@ +403,5 @@
> +   */
> +  _formatIfOemUnlockEnabled: function () {
> +    debug("_formatIfOemUnlockEnabled()");
> +    return this.getOemUnlockEnabled().then(enabled => {
> +      this._libcutils.property_set(OEM_UNLOCK_PROPERTY,(enabled == true ? "1" : "0"));

nit: === instead of ==

@@ +411,5 @@
> +      return Promise.resolve(false);
> +    }).then(result => {
> +      if (result === false) {
> +        return Promise.resolve(false);
> +      }else{

nit: coding style

@@ +433,5 @@
> +   */
> +  _formatPartition: function(isSetOemUnlockEnabled) {
> +    debug("_formatPartition()");
> +    let partition;
> +    return OS.File.open(this._dataBlockFile, {write:true, existing:true, append:false}).then(_partition => {

shouldn't we verify file/partition size?

@@ +476,5 @@
> +   */
> +  _enforceChecksumValidity: function() {
> +    debug("_enforceChecksumValidity");
> +    return this._computeDigest(true).then(digest => {
> +      if (digest.stored != digest.calculated) {

maybe we should also ensure the computed or the stored value is a valid non null length string?

@@ +477,5 @@
> +  _enforceChecksumValidity: function() {
> +    debug("_enforceChecksumValidity");
> +    return this._computeDigest(true).then(digest => {
> +      if (digest.stored != digest.calculated) {
> +        log("_enforceChecksumValidity: Validation failed! Stored digest: " + [toHexString(digest.stored.charCodeAt(i)) for(i in digest.stored)].join("") +

nit: we are doing that often, maybe we should have a little helper for that conversion?

@@ +541,5 @@
> +    // Ensure that we don't overwrite digest/magic/data-length and the last byte
> +    let maxBlockSize = this._getBlockDeviceSize() - (DIGEST_SIZE_BYTES + HEADER_SIZE_BYTES + 1);
> +    if (data.byteLength > maxBlockSize) {
> +      log("write: Couldn't write more than " + maxBlockSize + " bytes to the partition. " +
> +            maxBlockSize + " bytes given.");

nit: misalignment

@@ +573,5 @@
> +      return this._computeAndWriteDigest();
> +    }).then(couldComputeAndWriteDigest => {
> +      if (couldComputeAndWriteDigest === true) {
> +        return Promise.resolve(data.byteLength);
> +      }else{

nit: coding style, missing spaces

@@ +615,5 @@
> +      if (range[1] == 0) {
> +        log("wipe: Block device size is 0!");
> +        return reject();
> +      }
> +      let fd = this._libc.open(this._dataBlockFile, O_RDWR);

that fd is never closed

@@ +627,5 @@
> +        ret = this._libc.ioctl(fd, BLKDISCARD, rangeAddress);
> +        if (ret < 0) {
> +          log("wipe: CRITICAL: non-secure discarding failed too!!: errno: " + this.ctypes.errno);
> +          return reject();
> +        }else{

nit: missing spaces

@@ +656,5 @@
> +    return this._doSetOemUnlockEnabled(enabled).then(() => {
> +      return this._computeAndWriteDigest();
> +    }).then(() => {
> +      return Promise.resolve();
> +    });

no catch, no reject ?

@@ +671,5 @@
> +    let ret = false;
> +    let partition;
> +    return OS.File.open(this._dataBlockFile, {existing:true, append:false, read:true}).then(_partition => {
> +      partition = _partition;
> +      return partition.setPosition(this._getBlockDeviceSize() - OEM_UNLOCK_ENABLED_BYTES , OS.File.POS_START);

nit: extra space before the comma

@@ +673,5 @@
> +    return OS.File.open(this._dataBlockFile, {existing:true, append:false, read:true}).then(_partition => {
> +      partition = _partition;
> +      return partition.setPosition(this._getBlockDeviceSize() - OEM_UNLOCK_ENABLED_BYTES , OS.File.POS_START);
> +    }).then(() => {
> +      return partition.read(1);

hardcoded value ? Shouldn't we .read(OEM_UNLOCK_ENABLED_BYTES) ?

@@ +697,5 @@
> +    let partition
> +    let dataLength = 0;
> +    return OS.File.open(this._dataBlockFile, {read:true, existing:true, append:false}).then(_partition => {
> +      partition = _partition;
> +      // Jump the digest field

"Jump to"

@@ +704,5 @@
> +      // Read the Magic field
> +      return partition.read(PARTITION_MAGIC_SIZE_BYTES);
> +    }).then(_magic => {
> +      let magic = new Uint32Array(_magic.buffer)[0];
> +      if (magic === PARTITION_MAGIC){

coding style, missing spaces

@@ +706,5 @@
> +    }).then(_magic => {
> +      let magic = new Uint32Array(_magic.buffer)[0];
> +      if (magic === PARTITION_MAGIC){
> +        return partition.read(PARTITION_MAGIC_SIZE_BYTES);
> +      }else{

coding style, missing spaces

@@ +712,5 @@
> +        return Promise.reject();
> +      }
> +    }).then(_dataLength => {
> +      debug("getDataFieldSize: _dataLength = " + _dataLength);
> +      if (_dataLength)

missing brackets

@@ +716,5 @@
> +      if (_dataLength)
> +        dataLength = new Uint32Array(_dataLength.buffer)[0];
> +      return partition.close();
> +    }).then(() => {
> +      if (dataLength && dataLength != 0){

missing space

@@ +718,5 @@
> +      return partition.close();
> +    }).then(() => {
> +      if (dataLength && dataLength != 0){
> +        return Promise.resolve(dataLength);
> +      }else{

idem

::: b2g/components/test/unit/file_persistentdatablock.js
@@ +29,5 @@
> +function _prepareConfig(_args) {
> +  let args = _args || {};
> +  // This digest has been previously calculated given the data to be written later, and setting the OEM Unlocked Enabled byte
> +  // to 1. If we need different values, some tests will fail because this precalculated digest won't be valid then.
> +  args.digest = args.digest || new Uint8Array([0x0,0x41,0x7e,0x5f,0xe2,0xdd,0xaa,0xed,0x11,0x90,0x0e,0x1d,0x26,0x10,0x30,0xbd,

nit: spaces after commas, and maybe group to limit to 80 cols ?

@@ +32,5 @@
> +  // to 1. If we need different values, some tests will fail because this precalculated digest won't be valid then.
> +  args.digest = args.digest || new Uint8Array([0x0,0x41,0x7e,0x5f,0xe2,0xdd,0xaa,0xed,0x11,0x90,0x0e,0x1d,0x26,0x10,0x30,0xbd,
> +                                               0x44,0x9e,0xcc,0x4b,0x65,0xbe,0x2e,0x99,0x9f,0x86,0xf0,0xfc,0x5b,0x33,0x00,0xd0]);
> +  args.dataLength = args.dataLength || 6;
> +  args.data = args.data || new Uint8Array(["P","A","S","S","W","D"]);

nit: spaces after commas

@@ +43,5 @@
> +  log("_prepareConfig: args.oem = " + args.oem);
> +  log("_prepareConfig: args.oemUnlockAllowed = " + args.oemUnlockAllowed);
> +
> +  /* This function will be called after passing all native stuff tests, so we will write into a file instead of a real
> +   * partition. Obviuosly, there are some native operations like getting the device block size or wipping, that will not

nit: typo "obviously"

@@ +144,5 @@
> +function _installTests() {
> +  // <NATIVE_TESTS> Native operation tests go first
> +  add_test(function test_getBlockDeviceSize() {
> +    // We will use emulator /cache partition to get it's size.
> +    PersistentDataBlock._dataBlockFile = "/dev/block/mtdblock2";

nit: move the /dev/block to a const at the start of the file

@@ +150,5 @@
> +    // but we need to flip to testing mode after this test because we use files instead of partitions
> +    // and we cannot run this operation on files.
> +    PersistentDataBlock.setMode({testing:false});
> +    let blockSize = PersistentDataBlock._getBlockDeviceSize();
> +    ok(blockSize > 0, "test_getBlockDeviceSize: Block device size should be greater than 0");

nit: I'm still sure we can do better than 0 there, block device size do not change that much I guess ?

@@ +204,5 @@
> +      ok(false, "test_setOemUnlockedEnabledToTrue failed!: ex: " + ex);
> +    });
> +  });
> +
> + add_test(function test_setOemUnlockedEnabledToFalse() {

nit: misindentation

@@ +255,5 @@
> +    }).then(header => {
> +      log("test_computeAndWriteDigest: header = " + header);
> +      let magicRead = new Uint32Array(header.magic.buffer);
> +      let magicSupposed = new Uint32Array([PARTITION_MAGIC]);
> +      strictEqual(magicRead[0], magicSupposed[0]);

why only the first byte?

@@ +297,5 @@
> +  });
> +
> +  add_test(function test_formatPartition() {
> +    // Restore a fullfilled partition so we can check if formatting works...
> +    _prepareConfig({oem:true}).then(() => {

maybe we should assert the values before formatting?

@@ +328,5 @@
> +
> +  add_test(function test_enforceChecksumValidity() {
> +    // We need a valid partition layout to pass this test
> +    _prepareConfig().then(() => {
> +      PersistentDataBlock._enforceChecksumValidity().then(() => {

no assertion?

Comment 14

[Juan Gomez [:_AtilA_] (CET/CEST)]

(In reply to Alexandre LISSY :gerard-majax from comment #13)

Looks like we are approaching the final version :)

> Comment on attachment 8691470 [details] [diff] [review]
> 0001-Bug-1214515-PersistentDataBlock-b2g-component-implem.patch (v4)
> 
> Review of attachment 8691470 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall good, but there are still coding style nits to fix. Also, I think we
> want to make sure any promise rejection contains the underlying exception
> that we caught.
> 
> ::: b2g/components/PersistentDataBlock.jsm
> @@ +12,5 @@
> > + */
> > +
> > +"use strict";
> > +
> > +const DEBUG = true;
> 
> make sure to remove that before landing

nice advice... (god!) ;)

> 
> @@ +40,5 @@
> > +Cu.import("resource://gre/modules/Services.jsm");
> > +
> > +XPCOMUtils.defineLazyModuleGetter(this, "OS", "resource://gre/modules/osfile.jsm");
> > +XPCOMUtils.defineLazyModuleGetter(this, "Promise", "resource://gre/modules/Promise.jsm");
> > +
> 
> nit: extra new line
> 
> @@ +47,5 @@
> > +  Cu.import("resource://gre/modules/systemlibs.js");
> > +  return libcutils;
> > +});
> > +
> > +
> 
> nit: extra new line
> 
> @@ +97,5 @@
> > +
> > +  /**
> > +   * Data block file
> > +   */
> > +  _dataBlockFile: "",
> 
> undefined ?

I just want to make clear that this variable will contain a string, I think is a good practice for readability, isn't it? :)

> 
> @@ +104,5 @@
> > +  * Change the behavior of the class for some methods. Right now the only possible mode is
> > +  * "testing" (String), which will fake the return value of some methods realted
> > +  * to native operations with block devices, just for testing purposes.
> > +  */
> > +  _mode: {
> 
> we probably don't need to have _mode: { testing: bool }, just _testing: bool
> should be enough for now?
> 
> @@ +131,5 @@
> > +  /**
> > +  * Initialize the class.
> > +  *
> > +  */
> > +  init: function(mode) {
> 
> shouldn't we guard against multiple .init() calls ?

Actually, the implementation of the method is safe for multiple initializations

> 
> @@ +169,5 @@
> > +        log("Unable to open libc.so: ex = " + ex);
> > +        throw Cr.NS_ERROR_FAILURE;
> > +      }
> > +#else
> > +      throw Cr.NS_ERROR_ABORT;
> 
> maybe add a log?
> 
> @@ +173,5 @@
> > +      throw Cr.NS_ERROR_ABORT;
> > +#endif
> > +    }
> > +
> > +
> 
> nit: extra empty new line
> 
> @@ +189,5 @@
> > +    this._libc.handler.close();
> > +    Services.obs.removeObserver(this, XPCOM_SHUTDOWN_OBSERVER_TOPIC);
> > +  },
> > +
> > +  start: function() {
> 
> Who is responsible for calling this? There is no comment, and that component
> only calls .init().

You are right, forgot to comment. It'll be called on every boot so we can format the persistent partition depending
on some states.

> 
> @@ +210,5 @@
> > +
> > +    return true;
> > +  },
> > +
> > +  // addObserver
> 
> useless comment :)
> 
> @@ +237,5 @@
> > +   *                                  digest into the "stored" property.
> > +   */
> > +  _computeDigest: function (isStoredDigestReturned) {
> > +    debug("_computeDigest()");
> > +    let digest = {calculated: "", stored: ""};
> 
> using |undefined| instead of |""| ?

Same as above, good for readeabilty, right? They are goning to be strings soon :).

> 
> also, s/calculated/computed
> 
> @@ +310,5 @@
> > +        log("_getBlockDeviceSize: errno is ENOTTY, falling back to 64 bit version of BLKGETSIZE64...");
> > +        ret = this._libc.ioctl(fd, BLKGETSIZE64_64_BITS, sizeAddress);
> > +        if (ret < 0){
> > +          log("_getBlockDeviceSize: BLKGETSIZE64 failed again!. errno = " + this.ctypes.errno);
> > +          throw Cr.NS_ERROR_FAILURE;
> 
> aren't we leaking fd ?

.. OMG! :(

> 
> @@ +314,5 @@
> > +          throw Cr.NS_ERROR_FAILURE;
> > +        }
> > +      }else{
> > +        log("_getBlockDeviceSize: couldn't get block device size!: errno = " + this.ctypes.errno);
> > +        throw Cr.NS_ERROR_FAILURE;
> 
> same, I fear we are leaking fd

... OMFG! :(((

> 
> @@ +322,5 @@
> > +    debug("_getBlockDeviceSize: size =" + size.value);
> > +    return size.value;
> > +#else
> > +  log("_getBlockDeviceSize: ERROR: This feature is only supported in Gonk!");
> > +  return -1;
> 
> nit: bogus indentation
> 
> @@ +355,5 @@
> > +      debug("_doSetOemUnlockEnabled: OEM unlock enabled written to " + oemUnlockByte);
> > +      this._libcutils.property_set(OEM_UNLOCK_PROPERTY, oemUnlockByte);
> > +      return Promise.resolve();
> > +    }).catch(ex => {
> > +      return Promise.reject();
> 
> rejection should include the exception we caught, for easier error handling
> above?
> 
> @@ +403,5 @@
> > +   */
> > +  _formatIfOemUnlockEnabled: function () {
> > +    debug("_formatIfOemUnlockEnabled()");
> > +    return this.getOemUnlockEnabled().then(enabled => {
> > +      this._libcutils.property_set(OEM_UNLOCK_PROPERTY,(enabled == true ? "1" : "0"));
> 
> nit: === instead of ==
> 
> @@ +411,5 @@
> > +      return Promise.resolve(false);
> > +    }).then(result => {
> > +      if (result === false) {
> > +        return Promise.resolve(false);
> > +      }else{
> 
> nit: coding style
> 
> @@ +433,5 @@
> > +   */
> > +  _formatPartition: function(isSetOemUnlockEnabled) {
> > +    debug("_formatPartition()");
> > +    let partition;
> > +    return OS.File.open(this._dataBlockFile, {write:true, existing:true, append:false}).then(_partition => {
> 
> shouldn't we verify file/partition size?

Write() will reject if we get out of bounds.

> 
> @@ +476,5 @@
> > +   */
> > +  _enforceChecksumValidity: function() {
> > +    debug("_enforceChecksumValidity");
> > +    return this._computeDigest(true).then(digest => {
> > +      if (digest.stored != digest.calculated) {
> 
> maybe we should also ensure the computed or the stored value is a valid non
> null length string?

_computeDigest() will ensure that those conditions will never happen, they will be expressed as rejects.

> 
> @@ +477,5 @@
> > +  _enforceChecksumValidity: function() {
> > +    debug("_enforceChecksumValidity");
> > +    return this._computeDigest(true).then(digest => {
> > +      if (digest.stored != digest.calculated) {
> > +        log("_enforceChecksumValidity: Validation failed! Stored digest: " + [toHexString(digest.stored.charCodeAt(i)) for(i in digest.stored)].join("") +
> 
> nit: we are doing that often, maybe we should have a little helper for that
> conversion?

Makes sense.

> 
> @@ +541,5 @@
> > +    // Ensure that we don't overwrite digest/magic/data-length and the last byte
> > +    let maxBlockSize = this._getBlockDeviceSize() - (DIGEST_SIZE_BYTES + HEADER_SIZE_BYTES + 1);
> > +    if (data.byteLength > maxBlockSize) {
> > +      log("write: Couldn't write more than " + maxBlockSize + " bytes to the partition. " +
> > +            maxBlockSize + " bytes given.");
> 
> nit: misalignment
> 
> @@ +573,5 @@
> > +      return this._computeAndWriteDigest();
> > +    }).then(couldComputeAndWriteDigest => {
> > +      if (couldComputeAndWriteDigest === true) {
> > +        return Promise.resolve(data.byteLength);
> > +      }else{
> 
> nit: coding style, missing spaces
> 
> @@ +615,5 @@
> > +      if (range[1] == 0) {
> > +        log("wipe: Block device size is 0!");
> > +        return reject();
> > +      }
> > +      let fd = this._libc.open(this._dataBlockFile, O_RDWR);
> 
> that fd is never closed

... ¬¬

> 
> @@ +627,5 @@
> > +        ret = this._libc.ioctl(fd, BLKDISCARD, rangeAddress);
> > +        if (ret < 0) {
> > +          log("wipe: CRITICAL: non-secure discarding failed too!!: errno: " + this.ctypes.errno);
> > +          return reject();
> > +        }else{
> 
> nit: missing spaces
> 
> @@ +656,5 @@
> > +    return this._doSetOemUnlockEnabled(enabled).then(() => {
> > +      return this._computeAndWriteDigest();
> > +    }).then(() => {
> > +      return Promise.resolve();
> > +    });
> 
> no catch, no reject ?
> 
> @@ +671,5 @@
> > +    let ret = false;
> > +    let partition;
> > +    return OS.File.open(this._dataBlockFile, {existing:true, append:false, read:true}).then(_partition => {
> > +      partition = _partition;
> > +      return partition.setPosition(this._getBlockDeviceSize() - OEM_UNLOCK_ENABLED_BYTES , OS.File.POS_START);
> 
> nit: extra space before the comma
> 
> @@ +673,5 @@
> > +    return OS.File.open(this._dataBlockFile, {existing:true, append:false, read:true}).then(_partition => {
> > +      partition = _partition;
> > +      return partition.setPosition(this._getBlockDeviceSize() - OEM_UNLOCK_ENABLED_BYTES , OS.File.POS_START);
> > +    }).then(() => {
> > +      return partition.read(1);
> 
> hardcoded value ? Shouldn't we .read(OEM_UNLOCK_ENABLED_BYTES) ?

We should :)

> 
> @@ +697,5 @@
> > +    let partition
> > +    let dataLength = 0;
> > +    return OS.File.open(this._dataBlockFile, {read:true, existing:true, append:false}).then(_partition => {
> > +      partition = _partition;
> > +      // Jump the digest field
> 
> "Jump to"

hahahaha, I wanted to say "skip" indeed :)

> 
> @@ +704,5 @@
> > +      // Read the Magic field
> > +      return partition.read(PARTITION_MAGIC_SIZE_BYTES);
> > +    }).then(_magic => {
> > +      let magic = new Uint32Array(_magic.buffer)[0];
> > +      if (magic === PARTITION_MAGIC){
> 
> coding style, missing spaces
> 
> @@ +706,5 @@
> > +    }).then(_magic => {
> > +      let magic = new Uint32Array(_magic.buffer)[0];
> > +      if (magic === PARTITION_MAGIC){
> > +        return partition.read(PARTITION_MAGIC_SIZE_BYTES);
> > +      }else{
> 
> coding style, missing spaces
> 
> @@ +712,5 @@
> > +        return Promise.reject();
> > +      }
> > +    }).then(_dataLength => {
> > +      debug("getDataFieldSize: _dataLength = " + _dataLength);
> > +      if (_dataLength)
> 
> missing brackets
> 
> @@ +716,5 @@
> > +      if (_dataLength)
> > +        dataLength = new Uint32Array(_dataLength.buffer)[0];
> > +      return partition.close();
> > +    }).then(() => {
> > +      if (dataLength && dataLength != 0){
> 
> missing space
> 
> @@ +718,5 @@
> > +      return partition.close();
> > +    }).then(() => {
> > +      if (dataLength && dataLength != 0){
> > +        return Promise.resolve(dataLength);
> > +      }else{
> 
> idem
> 
> ::: b2g/components/test/unit/file_persistentdatablock.js
> @@ +29,5 @@
> > +function _prepareConfig(_args) {
> > +  let args = _args || {};
> > +  // This digest has been previously calculated given the data to be written later, and setting the OEM Unlocked Enabled byte
> > +  // to 1. If we need different values, some tests will fail because this precalculated digest won't be valid then.
> > +  args.digest = args.digest || new Uint8Array([0x0,0x41,0x7e,0x5f,0xe2,0xdd,0xaa,0xed,0x11,0x90,0x0e,0x1d,0x26,0x10,0x30,0xbd,
> 
> nit: spaces after commas, and maybe group to limit to 80 cols ?
> 
> @@ +32,5 @@
> > +  // to 1. If we need different values, some tests will fail because this precalculated digest won't be valid then.
> > +  args.digest = args.digest || new Uint8Array([0x0,0x41,0x7e,0x5f,0xe2,0xdd,0xaa,0xed,0x11,0x90,0x0e,0x1d,0x26,0x10,0x30,0xbd,
> > +                                               0x44,0x9e,0xcc,0x4b,0x65,0xbe,0x2e,0x99,0x9f,0x86,0xf0,0xfc,0x5b,0x33,0x00,0xd0]);
> > +  args.dataLength = args.dataLength || 6;
> > +  args.data = args.data || new Uint8Array(["P","A","S","S","W","D"]);
> 
> nit: spaces after commas
> 
> @@ +43,5 @@
> > +  log("_prepareConfig: args.oem = " + args.oem);
> > +  log("_prepareConfig: args.oemUnlockAllowed = " + args.oemUnlockAllowed);
> > +
> > +  /* This function will be called after passing all native stuff tests, so we will write into a file instead of a real
> > +   * partition. Obviuosly, there are some native operations like getting the device block size or wipping, that will not
> 
> nit: typo "obviously"
> 
> @@ +144,5 @@
> > +function _installTests() {
> > +  // <NATIVE_TESTS> Native operation tests go first
> > +  add_test(function test_getBlockDeviceSize() {
> > +    // We will use emulator /cache partition to get it's size.
> > +    PersistentDataBlock._dataBlockFile = "/dev/block/mtdblock2";
> 
> nit: move the /dev/block to a const at the start of the file
> 
> @@ +150,5 @@
> > +    // but we need to flip to testing mode after this test because we use files instead of partitions
> > +    // and we cannot run this operation on files.
> > +    PersistentDataBlock.setMode({testing:false});
> > +    let blockSize = PersistentDataBlock._getBlockDeviceSize();
> > +    ok(blockSize > 0, "test_getBlockDeviceSize: Block device size should be greater than 0");
> 
> nit: I'm still sure we can do better than 0 there, block device size do not
> change that much I guess ?

Ok, I did a quick search and looks like all emulators use the same /cache partition size. I juts don't want to fix future orange
tests because of that ;). I cannot think in any use case where _getBlockDevice() fails returning some possitive integer, that's why
being > 0 was enough for me. But it's ok, let's make the test more usefull and check for a real size.

> 
> @@ +204,5 @@
> > +      ok(false, "test_setOemUnlockedEnabledToTrue failed!: ex: " + ex);
> > +    });
> > +  });
> > +
> > + add_test(function test_setOemUnlockedEnabledToFalse() {
> 
> nit: misindentation
> 
> @@ +255,5 @@
> > +    }).then(header => {
> > +      log("test_computeAndWriteDigest: header = " + header);
> > +      let magicRead = new Uint32Array(header.magic.buffer);
> > +      let magicSupposed = new Uint32Array([PARTITION_MAGIC]);
> > +      strictEqual(magicRead[0], magicSupposed[0]);
> 
> why only the first byte?

Because each element of the array is 32 bits length for Uint32Array type, and the magic nunmber is a 32bit integer as well
(... maybe you thought it was an Uint8Array? :))

> 
> @@ +297,5 @@
> > +  });
> > +
> > +  add_test(function test_formatPartition() {
> > +    // Restore a fullfilled partition so we can check if formatting works...
> > +    _prepareConfig({oem:true}).then(() => {
> 
> maybe we should assert the values before formatting?

Yeah, the Magic field and the data length field at least.

> 
> @@ +328,5 @@
> > +
> > +  add_test(function test_enforceChecksumValidity() {
> > +    // We need a valid partition layout to pass this test
> > +    _prepareConfig().then(() => {
> > +      PersistentDataBlock._enforceChecksumValidity().then(() => {
> 
> no assertion?

Another pretty detailed review Alex, thanks for that!
Hope that my next patches don't have that many style code nits! ;)

Comment 15

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: 0001-Bug-1214515-PersistentDataBlock-b2g-component-implem.patch (v5)

Chagelog:
* Coding style nits fixed
* More comments
* File descriptor leaking fixed
* Move hexadecimal conversion from string/array to a helper function
* test_getBlockDeviceSize() now check for real partition size

Comment 16

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: 0001-Bug-1214515-PersistentDataBlock-b2g-component-implem.patch

Changelog:
* Changing "let" for "var" in the test

Comment 17

[:gerard-majax]

Comment on attachment 8693652 [details] [diff] [review]
0001-Bug-1214515-PersistentDataBlock-b2g-component-implem.patch

Review of attachment 8693652 [details] [diff] [review]:
-----------------------------------------------------------------

Good overall, but please fix those nits and provide green try link :)

::: b2g/components/PersistentDataBlock.jsm
@@ +63,5 @@
> +  function toHexChar(charCode) {
> +    return ("0" + charCode.toString(16).slice(-2));
> +  }
> +  let hexString = "";
> +  if(typeof data === "string") {

nit: missing space between |if| and |(|

@@ +73,5 @@
> +}
> +
> +function arr2bstr(arr) {
> +  let bstr = "";
> +  for(let i = 0; i < arr.length; i++) {

nit: missing space after |for|

@@ +74,5 @@
> +
> +function arr2bstr(arr) {
> +  let bstr = "";
> +  for(let i = 0; i < arr.length; i++) {
> +    bstr +=  String.fromCharCode(arr[i]);

nit: extra space after |=|

@@ +178,5 @@
> +#endif
> +    }
> +
> +    this._dataBlockFile = this._libcutils.property_get(PERSISTENT_DATA_BLOCK_PROPERTY);
> +    if (this._dataBlockFile === null) {

nit: if (!this._dataBlockFile) ?

::: b2g/components/test/unit/file_persistentdatablock.js
@@ +29,5 @@
> +  function toHexChar(charCode) {
> +    return ("0" + charCode.toString(16).slice(-2));
> +  }
> +  let hexString = "";
> +  if(typeof data === "string") {

nit: missing space between |if| and |(|

@@ +41,5 @@
> +function _prepareConfig(_args) {
> +  let args = _args || {};
> +  // This digest has been previously calculated given the data to be written later, and setting the OEM Unlocked Enabled byte
> +  // to 1. If we need different values, some tests will fail because this precalculated digest won't be valid then.
> +  args.digest = args.digest || new Uint8Array([0x0, 0x41, 0x7e, 0x5f, 0xe2, 0xdd, 0xaa, 0xed, 0x11, 0x90, 0x0e, 0x1d, 0x26,

nit: 0x0 => 0x00

@@ +136,5 @@
> +  let file;
> +  let data;
> +  return OS.File.open(PersistentDataBlock._dataBlockFile, {read:true, existing:true, append:false}).then(_file => {
> +    file = _file;
> +    return file.setPosition(36, OS.File.POS_START);

nit: shouldn't we avoir hardcoding that position 36 ?

@@ +147,5 @@
> +  }).then(_data => {
> +    data = _data;
> +    return file.close();
> +  }).then(() => {
> +    return Promise.resolve(data);69206016

what is this "69206016" ?

@@ +163,5 @@
> +    // but we need to flip to testing mode after this test because we use files instead of partitions
> +    // and we cannot run this operation on files.
> +    PersistentDataBlock.setTestingMode(false);
> +    let blockSize = PersistentDataBlock._getBlockDeviceSize();
> +    ok(blockSize !== 69206016, "test_getBlockDeviceSize: Block device size should be greater than 0");

nit: put the blockSize value in a const defined at the start of the file?

Comment 18

[Juan Gomez [:_AtilA_] (CET/CEST)]

(In reply to Alexandre LISSY :gerard-majax from comment #17)
> Comment on attachment 8693652 [details] [diff] [review]
> 0001-Bug-1214515-PersistentDataBlock-b2g-component-implem.patch
> 
> Review of attachment 8693652 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Good overall, but please fix those nits and provide green try link :)
> 
> ::: b2g/components/PersistentDataBlock.jsm
> @@ +63,5 @@
> > +  function toHexChar(charCode) {
> > +    return ("0" + charCode.toString(16).slice(-2));
> > +  }
> > +  let hexString = "";
> > +  if(typeof data === "string") {
> 
> nit: missing space between |if| and |(|
> 
> @@ +73,5 @@
> > +}
> > +
> > +function arr2bstr(arr) {
> > +  let bstr = "";
> > +  for(let i = 0; i < arr.length; i++) {
> 
> nit: missing space after |for|
> 
> @@ +74,5 @@
> > +
> > +function arr2bstr(arr) {
> > +  let bstr = "";
> > +  for(let i = 0; i < arr.length; i++) {
> > +    bstr +=  String.fromCharCode(arr[i]);
> 
> nit: extra space after |=|
> 
> @@ +178,5 @@
> > +#endif
> > +    }
> > +
> > +    this._dataBlockFile = this._libcutils.property_get(PERSISTENT_DATA_BLOCK_PROPERTY);
> > +    if (this._dataBlockFile === null) {
> 
> nit: if (!this._dataBlockFile) ?

The method property_get() returns null if fails, so it seems legit to compare with it, right? :) 
 
> 
> ::: b2g/components/test/unit/file_persistentdatablock.js
> @@ +29,5 @@
> > +  function toHexChar(charCode) {
> > +    return ("0" + charCode.toString(16).slice(-2));
> > +  }
> > +  let hexString = "";
> > +  if(typeof data === "string") {
> 
> nit: missing space between |if| and |(|
> 
> @@ +41,5 @@
> > +function _prepareConfig(_args) {
> > +  let args = _args || {};
> > +  // This digest has been previously calculated given the data to be written later, and setting the OEM Unlocked Enabled byte
> > +  // to 1. If we need different values, some tests will fail because this precalculated digest won't be valid then.
> > +  args.digest = args.digest || new Uint8Array([0x0, 0x41, 0x7e, 0x5f, 0xe2, 0xdd, 0xaa, 0xed, 0x11, 0x90, 0x0e, 0x1d, 0x26,
> 
> nit: 0x0 => 0x00
> 
> @@ +136,5 @@
> > +  let file;
> > +  let data;
> > +  return OS.File.open(PersistentDataBlock._dataBlockFile, {read:true, existing:true, append:false}).then(_file => {
> > +    file = _file;
> > +    return file.setPosition(36, OS.File.POS_START);
> 
> nit: shouldn't we avoir hardcoding that position 36 ?
> 
> @@ +147,5 @@
> > +  }).then(_data => {
> > +    data = _data;
> > +    return file.close();
> > +  }).then(() => {
> > +    return Promise.resolve(data);69206016
> 
> what is this "69206016" ?

A fugitive paste... :(

> 
> @@ +163,5 @@
> > +    // but we need to flip to testing mode after this test because we use files instead of partitions
> > +    // and we cannot run this operation on files.
> > +    PersistentDataBlock.setTestingMode(false);
> > +    let blockSize = PersistentDataBlock._getBlockDeviceSize();
> > +    ok(blockSize !== 69206016, "test_getBlockDeviceSize: Block device size should be greater than 0");
> 
> nit: put the blockSize value in a const defined at the start of the file?

Comment 19

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: 0001-Bug-1214515-PersistentDataBlock-b2g-component-implem.patch

Changelog:
* Coding style nits fixed
* Added some consts

Comment 20

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: 0001-Bug-1214515-PersistentDataBlock-b2g-component-implem.patch

Changelog:
* Changes some confusing text messages in the tests

Comment 21

[:gerard-majax]

Comment on attachment 8695203 [details] [diff] [review]
0001-Bug-1214515-PersistentDataBlock-b2g-component-implem.patch

Review of attachment 8695203 [details] [diff] [review]:
-----------------------------------------------------------------

Seems all good to me, checkin-needed once you have stable green try :)

::: b2g/components/PersistentDataBlock.jsm
@@ +275,5 @@
> +    }).catch(ex => {
> +      log("_computeDigest(): Failed to read partition: ex = " + ex);
> +      return Promise.reject(ex);
> +    });
> +  },

ok

@@ +335,5 @@
> +#else
> +    log("_getBlockDeviceSize: ERROR: This feature is only supported in Gonk!");
> +    return -1;
> +#endif
> +  },

ok

@@ +367,5 @@
> +      return Promise.resolve();
> +    }).catch(ex => {
> +      return Promise.reject(ex);
> +    });
> +  },

ok

@@ +400,5 @@
> +    }).catch(ex => {
> +      log("_computeAndWriteDigest: Couldn't write digest in the persistent partion. ex = " + ex );
> +      return Promise.reject(ex);
> +    });
> +  },

ok

@@ +428,5 @@
> +    }).catch(ex => {
> +      log("_formatIfOemUnlockEnabled: An error ocurred!. ex = " + ex);
> +      return Promise.reject(ex);
> +    });
> +  },

ok

@@ +472,5 @@
> +    }).catch(ex => {
> +      log("_formatPartition: Failed to format block device!: ex = " + ex);
> +      return Promise.reject(ex);
> +    });
> +  },

ok

@@ +501,5 @@
> +        log("_enforceChecksumValidity: Error ocurred while formating the partition!: ex = " + ex);
> +        return Promise.reject(ex);
> +      });
> +    });
> +  },

ok

@@ +534,5 @@
> +    }).catch(ex => {
> +      log("read: Failed to read entire data block. Exception: " + ex);
> +      return Promise.reject(ex);
> +    });
> +  },

ok

@@ +589,5 @@
> +    }).catch(ex => {
> +      log("write: Failed to write to the persistent partition: ex = " + ex);
> +      return Promise.reject(ex);
> +    });
> +  },

ok

@@ +650,5 @@
> +#else
> +    log("wipe: ERROR: This feature is only supported in Gonk!");
> +    return Promise.reject();
> +#endif
> +  },

ok

@@ +670,5 @@
> +      return Promise.resolve();
> +    }).catch(ex => {
> +      return Promise.reject(ex);
> +    });
> +  },

ok

@@ +697,5 @@
> +    }).catch(ex => {
> +      log("getOemUnlockEnabled: Error reading OEM unlock enabled byte from partition: ex = " + ex);
> +      return Promise.reject(ex);
> +    });
> +  },

ok

@@ +738,5 @@
> +    }).catch(ex => {
> +      log("getDataFieldSize: Couldn't get data field size: ex = " + ex);
> +      return Promise.reject(ex);
> +    });
> +  },

ok

@@ +752,5 @@
> +    return new Promise((resolve, reject) => {
> +      let actualSize = this._getBlockDeviceSize() - HEADER_SIZE_BYTES - OEM_UNLOCK_ENABLED_BYTES;
> +      resolve(actualSize <= MAX_DATA_BLOCK_SIZE ? actualSize : MAX_DATA_BLOCK_SIZE);
> +    });
> +  }

ok

::: b2g/components/test/unit/file_persistentdatablock.js
@@ +90,5 @@
> +  }).catch(ex => {
> +    log("_prepareConfig: ERROR: ex = " + ex);
> +    return Promise.reject(ex);
> +  });
> +}

ok

@@ +108,5 @@
> +    return Promise.resolve(byte[0]);
> +  }).catch(ex => {
> +    return Promise.reject(ex);
> +  });
> +}

ok

@@ +131,5 @@
> +  }).catch(ex => {
> +    return Promise.reject(ex);
> +  });
> +}
> +

ok

@@ +152,5 @@
> +    return Promise.resolve(data);
> +  }).catch(ex => {
> +    return Promise.reject(ex);
> +  });
> +}

ok

@@ +166,5 @@
> +    PersistentDataBlock.setTestingMode(false);
> +    let blockSize = PersistentDataBlock._getBlockDeviceSize();
> +    ok(blockSize !== CACHE_PARTITION_SIZE, "test_getBlockDeviceSize: Block device size should be greater than 0");
> +    run_next_test();
> +  });

ok

@@ +178,5 @@
> +    }).catch(ex => {
> +      // ... something went really really bad if this happens.
> +      ok(false, "test_wipe failed!: ex: " + ex);
> +    });
> +  });

ok

@@ +194,5 @@
> +      }).catch(ex => {
> +        ok(false, "test_computeDigest failed!: ex: " + ex);
> +      });
> +    });
> +  });

ok

@@ +204,5 @@
> +      run_next_test();
> +    }).catch(ex => {
> +      ok(false, "test_getOemUnlockedEnabled failed: ex:" + ex);
> +    });
> +  });

ok

@@ +216,5 @@
> +      run_next_test();
> +    }).catch(ex => {
> +      ok(false, "test_setOemUnlockedEnabledToTrue failed!: ex: " + ex);
> +    });
> +  });

ok

@@ +228,5 @@
> +      run_next_test();
> +    }).catch(ex => {
> +      ok(false, "test_setOemUnlockedEnabledToFalse failed!: ex: " + ex);
> +    });
> +  });

ok

@@ +244,5 @@
> +      });
> +    }).catch(ex => {
> +      ok(false, "test_getOemUnlockedEnabledWithTrue failed: An error ocurred while setting the OEM Unlock Enabled byte to true: ex:" + ex);
> +    });
> +  });

ok

@@ +260,5 @@
> +      });
> +    }).catch(ex => {
> +      ok(false, "test_getOemUnlockedEnabledWithFalse failed: An error ocurred while setting the OEM Unlock Enabled byte to false: ex:" + ex);
> +    });
> +  });

ok

@@ +276,5 @@
> +      run_next_test();
> +    }).catch(ex => {
> +      ok(false, "test_computeAndWriteDigest failed!: ex: " + ex);
> +    });
> +  });

ok

@@ +306,5 @@
> +      strictEqual(byte, 0);
> +      run_next_test();
> +    }).catch(ex => {
> +      ok(false, "test_formatIfOemUnlockEnabledWithFalse failed!: ex: " + ex);
> +    });

ok

@@ +337,5 @@
> +      run_next_test();
> +    }).catch(ex => {
> +      ok(false, "test_formatPartition failed!: ex: " + ex);
> +    });
> +  });

ok

@@ +349,5 @@
> +      }).catch(ex => {
> +        ok(false, "test_enforceChecksumValidityWithValidChecksum failed!: ex: " + ex);
> +      });
> +    });
> +  });

ok

@@ +373,5 @@
> +      }).catch(ex => {
> +        ok(false, "test_enforceChecksumValidityWithValidChecksum failed!: ex: " + ex);
> +      });
> +    });
> +  });

ok

Comment 22

[Juan Gomez [:_AtilA_] (CET/CEST)]

This is the most stable try-link I can provide so far. Looks like there are some problems with the taskcluster so there are a lot of intermittent orange test. Anyway, none of the X1 tests done in that job, failed at my test.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=da738f0347f9&selectedJob=14394995

Comment 23

[Carsten Book [:Tomcat]]

this failed to apply:

Tomcats-MacBook-Pro-2:b2g-inbound Tomcat$ hg qimport bz://1214515  && hg qpush && hg qrefresh -e
Fetching... done
Parsing... done
adding 1214515 to series file
renamed 1214515 -> 0001-Bug-1214515-PersistentDataBlock-b2g-component-implem.patch
applying 0001-Bug-1214515-PersistentDataBlock-b2g-component-implem.patch
patching file b2g/components/moz.build
Hunk #1 FAILED at 72
1 out of 1 hunks FAILED -- saving rejects to file b2g/components/moz.build.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 0001-Bug-1214515-PersistentDataBlock-b2g-component-implem.patch

Comment 24

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: 0001-Bug-1214515-PersistentDataBlock-b2g-component-implem.patch

Comment 25

[Carsten Book [:Tomcat]]

Hi Atila, could you take a look:
renamed 1214515 -> 0001-Bug-1214515-PersistentDataBlock-b2g-component-implem.patch
applying 0001-Bug-1214515-PersistentDataBlock-b2g-component-implem.patch
patching file b2g/components/moz.build
Hunk #1 FAILED at 72
1 out of 1 hunks FAILED -- saving rejects to file b2g/components/moz.build.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory

Comment 26

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: 0001-Bug-1214515-PersistentDataBlock-b2g-component-implem.patch

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/c472eeb03435

Comment 28

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=3557695&repo=b2g-inbound

Comment 29

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/b2g-inbound/rev/47dd516d5540

Comment 30

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: 0001-Bug-1214515-PersistentDataBlock-b2g-component-implem.patch

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/ad9a8dfbf99e

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ad9a8dfbf99e