Closed Bug 919982 Opened 8 years ago Closed 8 years ago

Instantiate additional ril-daemon/rilproxy services for jellybean QEmu

Categories

(Firefox OS Graveyard :: Emulator, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: vicamo, Assigned: vicamo)

References

Details

Attachments

(3 files, 2 obsolete files)

To enable multisim/dsds development, we're going to instantiate additional rild/rilproxy services.  We're still not sure whether we should have multsim/dsds support on B2G ICS.
Some files in jellybean code base refer to "ril-daemon":

  1) external/sepolicy/property_contexts
    - Defines property context for "ctl.ril-daemon".  Will probably skip this for emulator.

  2) system/extras/tests/ext4/android_emmc_perf_tests.sh
    - Stop ril-daemon before testing emmc performance.  We probably won't be interested in emmc performance in emulator.

  3) system/core/rootdir/init.rc
    - Defines service "ril-daemon".  Additional ril-daemon services are now emulator-only, so we're not going to touch this file.

  4) system/core/init/property_service.c
    - Defines white list of uid/gid for access to property "ctl.ril-daemon".  Same as 1).
Attachment #809886 - Flags: review?(mwu)
Attachment #809888 - Flags: review?(yurenju.mozilla)
Attachment #809888 - Flags: review?(mwu)
Attachment #809888 - Flags: review?(htsai)
Attached file Github pull request for gaia (obsolete) —
Attachment #809889 - Flags: review?(yurenju.mozilla)
Attachment #809889 - Flags: review?(yurenju.mozilla) → review+
Comment on attachment 809888 [details]
Github pull request for device_generic_goldfish

looks good to me, r=yurenju
Attachment #809888 - Flags: review?(yurenju.mozilla) → review+
Attachment #809886 - Flags: review?(mwu) → review+
Can you use an android property instead of gecko pref to specify the number of ril daemons? We should avoid gecko preferences for device specific settings.
Attachment #809888 - Flags: review?(mwu)
Attachment #809888 - Flags: review?(htsai) → review+
(In reply to Michael Wu [:mwu] from comment #6)
> Can you use an android property instead of gecko pref to specify the number
> of ril daemons? We should avoid gecko preferences for device specific
> settings.

Sure.  I'll revise PR for device_generic_goldfish and replace PR for gaia with a Gecko patch instead.

Yuren, then it won't touches Gaia.  Sorry for bothering you.
Attachment #809889 - Attachment is obsolete: true
Comment on attachment 809888 [details]
Github pull request for device_generic_goldfish

Use Android property ro.ril.numclients instead.
Attachment #809888 - Flags: review+ → review?(mwu)
Attached patch Gecko patch (obsolete) — Splinter Review
Attachment #810939 - Flags: review?(htsai)
Is ro.ril.numclients something in use already, or are we inventing it? For properties that we introduce, we usually prefix it with ro.moz. to distinguish from properties introduced by vendors or Android. We have many ro.moz.ril.* properties already.
Attached patch Gecko patch : v2Splinter Review
Address comment 10, use "ro.moz.ril.numclients" instead.
Attachment #810939 - Attachment is obsolete: true
Attachment #810939 - Flags: review?(htsai)
Attachment #811051 - Flags: review?(htsai)
(In reply to Michael Wu [:mwu] from comment #10)
> Is ro.ril.numclients something in use already, or are we inventing it? For
> properties that we introduce, we usually prefix it with ro.moz. to
> distinguish from properties introduced by vendors or Android. We have many
> ro.moz.ril.* properties already.

That's a new property.  I've updated the pull request and Gecko patch.  Thank you.
Try: emulator only, with all tests.
https://tbpl.mozilla.org/?tree=Try&rev=2b9867acc532
Hmm, we shouldn't use ADDITIONAL_DEFAULT_PROPERTIES like that. It's unconditionally added to all builds. This isn't a problem with our builds since this goldfish device dir only exists on emulator builds, but device directories are suppose to coexist with other directories. It also adds itself to the property list in boot.img rather than system.img, which is where it really belongs. (looks like ADDITIONAL_BUILD_PROPERTIES can be used for system.img)

I don't think we need another review cycle though. r=me with this property added to https://github.com/mozilla-b2g/platform_build/blob/b2g-4.3_r2.1/target/board/generic/device.mk#L20 instead of the device directory.
Comment on attachment 809888 [details]
Github pull request for device_generic_goldfish

r=me with the property moved to the place mentioned before.
Attachment #809888 - Flags: review?(mwu) → review+
Attachment #811051 - Flags: review?(htsai) → review+
(In reply to Michael Wu [:mwu] from comment #14)
> I don't think we need another review cycle though. r=me with this property
> added to
> https://github.com/mozilla-b2g/platform_build/blob/b2g-4.3_r2.1/target/board/
> generic/device.mk#L20 instead of the device directory.

I don't think that's a good idea.  |board/generic/device.mk| is inherited by all products that inherit |product/full.mk|.[1]  And most all of our physical devices inherit |product/full.mk|:

  * https://github.com/mozilla-b2g/device-gp-keon/blob/master/full_keon.mk#L17
  * https://github.com/mozilla-b2g/android-device-crespo4g/blob/master/full_crespo4g.mk#L34
  * https://github.com/mozilla-b2g/device-wasabi/blob/master/full_wasabi.mk#L10
  * https://github.com/mozilla-b2g/device-leo/blob/master/full_leo.mk#L11
  * https://github.com/mozilla-b2g/device-helix/blob/master/full_helix.mk#L9
  ...

Correct me if I'm wrong.  Thank you.

[1]: https://github.com/mozilla-b2g/platform_build/blob/b2g-4.3_r2.1/target/product/full.mk#L23
Flags: needinfo?(mwu)
Hmm. You're right. However, the problem isn't that board/generic/device.mk is the wrong place, but that we should be inheriting from full_base_telephony.mk rather than full.mk. (ro.adb.qemud should *not* be set on devices..)

Go ahead with ADDITIONAL_BUILD_PROPERTIES and we'll sort out this problem with devices inheriting full.mk later.
Flags: needinfo?(mwu)
ADDITIONAL_{DEFAULT,BUILD}_PROPERTIES, in contrast to PRODUCT_{DEFAULT_,}PROPERTY_OVERRIDES, are both for properties applied to all builds unconditionally.  The only difference between them is, DEFAULT properties are in |/default.prop| and BUILD properties in |/system/build.prop|.  To avoid the problem you mentioned in comment 14, we have to use PRODUCT_*PROPERTY_OVERRIDES, which leads us to abuse of |full.mk| mentioned in comment 16.

I personally feel ADDITIONAL_DEFAULT_PROPERTIES is more like it because we have all service descriptors in |/init.goldfish.rc|, which is installed in bootimg as well.  I'd like to align these settings in the same place.  However, I also agree that's not a big deal, so we can always address this later whenever it becomes necessary.
Whiteboard: [leave open]
Blocks: 959019
You need to log in before you can comment on or make changes to this bug.