Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
135853021:
NaCl: Expose NaClApp to embedding layer. (Closed)
|
Created:
6 years, 11 months ago by teravest Modified:
6 years, 11 months ago CC:
native-client-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/native_client/src/native_client.git@master Visibility:
Public. |
DescriptionNaCl: Expose NaClApp to embedding layer.
This is a small change in the API to expose NaClApp to the embedding layer.
There will be more changes in the future to clean this up, but I'm changing
this now so I can mail out patches to refactor the trusted plugin in Chrome.
BUG=http://crbug.com/333950
Patch Set 1 #Patch Set 2 : Update test also #
Total comments: 7
Patch Set 3 : Bigger change #Patch Set 4 : Build fixes applied #
Messages
Total messages: 15 (0 generated)
https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... File src/trusted/service_runtime/sel_main_chrome.c (right): https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... src/trusted/service_runtime/sel_main_chrome.c:417: struct NaClApp *nap = NaClAppCreate(); Is there a need to do it on the heap here? Could you just do it on the stack in this case, and get rid of NaClAppCreate? https://codereview.chromium.org/135853021/diff/40001/tests/sel_main_chrome/se... File tests/sel_main_chrome/sel_main_chrome_test.cc (right): https://codereview.chromium.org/135853021/diff/40001/tests/sel_main_chrome/se... tests/sel_main_chrome/sel_main_chrome_test.cc:138: struct NaClApp *nap = NaClAppCreate(); Here too... can you just do it on the stack?
On Thu, Jan 16, 2014 at 2:30 PM, <dmichael@chromium.org> wrote: > > https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... > File src/trusted/service_runtime/sel_main_chrome.c (right): > > https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... > src/trusted/service_runtime/sel_main_chrome.c:417: struct NaClApp *nap = > NaClAppCreate(); > Is there a need to do it on the heap here? Could you just do it on the > stack in this case, and get rid of NaClAppCreate? I have to have it on the heap here because I don't want to expose the definition of NaClApp. It's defined in sel_ldr.h, which has includes that collide with Chrome's code. So I allocate it on the heap here so I only have to pass around an opaque pointer. > > https://codereview.chromium.org/135853021/diff/40001/tests/sel_main_chrome/se... > File tests/sel_main_chrome/sel_main_chrome_test.cc (right): > > https://codereview.chromium.org/135853021/diff/40001/tests/sel_main_chrome/se... > tests/sel_main_chrome/sel_main_chrome_test.cc:138: struct NaClApp *nap = > NaClAppCreate(); > Here too... can you just do it on the stack? > > https://codereview.chromium.org/135853021/ -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at http://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/groups/opt_out.
On 2014/01/16 21:37:30, teravest wrote: > On Thu, Jan 16, 2014 at 2:30 PM, <mailto:dmichael@chromium.org> wrote: > > > > > https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... > > File src/trusted/service_runtime/sel_main_chrome.c (right): > > > > > https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... > > src/trusted/service_runtime/sel_main_chrome.c:417: struct NaClApp *nap = > > NaClAppCreate(); > > Is there a need to do it on the heap here? Could you just do it on the > > stack in this case, and get rid of NaClAppCreate? > > I have to have it on the heap here because I don't want to expose the > definition of NaClApp. It's defined in sel_ldr.h, which has includes > that collide with Chrome's code. So I allocate it on the heap here so > I only have to pass around an opaque pointer. > But the place in the code I was referencing is inside of sel_main_chrome.c... it should be fine to do it on the stack, shouldn't it? I can believe that maybe you need NaClAppCreate for some other client, but it looks odd here. You also never free it... it's not an important leak or anything, but if you do need NaClAppCreate, you might also want to provide NaClAppDestroy? I apologize if I'm missing something obvious. We can chat tomorrow and you can clear it up for me.
lgtm https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... File src/trusted/service_runtime/sel_main_chrome.c (right): https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... src/trusted/service_runtime/sel_main_chrome.c:417: struct NaClApp *nap = NaClAppCreate(); On 2014/01/16 21:30:26, dmichael wrote: > Is there a need to do it on the heap here? Could you just do it on the stack in > this case, and get rid of NaClAppCreate? Okay, we just chatted. Since: 1) This code is going to be deleted soon anyway and 2) Using NaClAppCreate here exercises that function (whereas a stack variable wouldn't) ... the code's good as you have it. I think a TODO to delete this function wouldn't hurt.
CQ is trying da patch. Follow status at https://nativeclient-status.appspot.com/cq/teravest@chromium.org/135853021/40001
Presubmit check for 135853021-40001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
src/trusted/service_runtime/sel_main_chrome.c
src/trusted/service_runtime/sel_main_chrome.h
https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... File src/trusted/service_runtime/sel_main_chrome.c (right): https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... src/trusted/service_runtime/sel_main_chrome.c:127: struct NaClApp* NaClAppCreate(void) { Spacing style should be " *" in NaCl-side code (same elsewhere in this change). https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... src/trusted/service_runtime/sel_main_chrome.c:129: if (!nap) NaCl style is "nap == NULL" https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... src/trusted/service_runtime/sel_main_chrome.c:133: if (NaClAppCtor(nap) != 0) { There's a correctness problem here. If NaClAppCreate() is called before NaClChromeMainStartApp(), the former will happen without NaClAllModulesInit() having been called. Can you run trybots for NaCl changes before sending them for review, please? I don't know if this works with sel_main_chrome_test on all platforms. BTW, "git cl try" only works for changes to the Chromium repo, not any other repo. You have to use "git try" instead. https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... File src/trusted/service_runtime/sel_main_chrome.h (right): https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... src/trusted/service_runtime/sel_main_chrome.h:157: struct NaClApp* NaClAppCreate(void); Why do you want to do the interface like this rather than the version from https://code.google.com/p/chromium/issues/detail?id=333950, which was: struct NaClApp *NaClChromeMainCreateApp(struct NaClChromeMainArgs *args); That seemed a little cleaner as an embedding interface because it returns a NaClApp that is more fully initialised. For consistency, NaClAppCreate() should be named "*Make" rather than "*Create" (matching other functions in the NaCl codebase), and should really have a "NaClChromeMain" prefix to match the rest of this header file.
On Fri, Jan 17, 2014 at 9:42 AM, <mseaborn@chromium.org> wrote: > > https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... > File src/trusted/service_runtime/sel_main_chrome.c (right): > > https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... > src/trusted/service_runtime/sel_main_chrome.c:127: struct NaClApp* > NaClAppCreate(void) { > Spacing style should be " *" in NaCl-side code (same elsewhere in this > change). > > https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... > src/trusted/service_runtime/sel_main_chrome.c:129: if (!nap) > NaCl style is "nap == NULL" > > https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... > src/trusted/service_runtime/sel_main_chrome.c:133: if (NaClAppCtor(nap) > != 0) { > There's a correctness problem here. If NaClAppCreate() is called before > NaClChromeMainStartApp(), the former will happen without > NaClAllModulesInit() having been called. > > Can you run trybots for NaCl changes before sending them for review, > please? I don't know if this works with sel_main_chrome_test on all > platforms. Do I need permission for using the trybots? I've run "git cl try" for my NaCl changes, but all the trybots get stuck at "try pending" and my jobs never get scheduled. > > BTW, "git cl try" only works for changes to the Chromium repo, not any > other repo. You have to use "git try" instead. > > https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... > File src/trusted/service_runtime/sel_main_chrome.h (right): > > https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... > src/trusted/service_runtime/sel_main_chrome.h:157: struct NaClApp* > NaClAppCreate(void); > Why do you want to do the interface like this rather than the version > from https://code.google.com/p/chromium/issues/detail?id=333950, which > was: > > struct NaClApp *NaClChromeMainCreateApp(struct NaClChromeMainArgs > *args); > > That seemed a little cleaner as an embedding interface because it > returns a NaClApp that is more fully initialised. > > For consistency, NaClAppCreate() should be named "*Make" rather than > "*Create" (matching other functions in the NaCl codebase), and should > really have a "NaClChromeMain" prefix to match the rest of this header > file. > > https://codereview.chromium.org/135853021/ -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at http://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/groups/opt_out.
On Fri, Jan 17, 2014 at 9:51 AM, Justin TerAvest <teravest@chromium.org> wrote: > On Fri, Jan 17, 2014 at 9:42 AM, <mseaborn@chromium.org> wrote: >> >> https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... >> File src/trusted/service_runtime/sel_main_chrome.c (right): >> >> https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... >> src/trusted/service_runtime/sel_main_chrome.c:127: struct NaClApp* >> NaClAppCreate(void) { >> Spacing style should be " *" in NaCl-side code (same elsewhere in this >> change). >> >> https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... >> src/trusted/service_runtime/sel_main_chrome.c:129: if (!nap) >> NaCl style is "nap == NULL" >> >> https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... >> src/trusted/service_runtime/sel_main_chrome.c:133: if (NaClAppCtor(nap) >> != 0) { >> There's a correctness problem here. If NaClAppCreate() is called before >> NaClChromeMainStartApp(), the former will happen without >> NaClAllModulesInit() having been called. >> >> Can you run trybots for NaCl changes before sending them for review, >> please? I don't know if this works with sel_main_chrome_test on all >> platforms. > > Do I need permission for using the trybots? I've run "git cl try" for > my NaCl changes, but all the trybots get stuck at "try pending" and my > jobs never get scheduled. > >> >> BTW, "git cl try" only works for changes to the Chromium repo, not any >> other repo. You have to use "git try" instead. Oh, sorry, I see now. >> >> https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... >> File src/trusted/service_runtime/sel_main_chrome.h (right): >> >> https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... >> src/trusted/service_runtime/sel_main_chrome.h:157: struct NaClApp* >> NaClAppCreate(void); >> Why do you want to do the interface like this rather than the version >> from https://code.google.com/p/chromium/issues/detail?id=333950, which >> was: >> >> struct NaClApp *NaClChromeMainCreateApp(struct NaClChromeMainArgs >> *args); >> >> That seemed a little cleaner as an embedding interface because it >> returns a NaClApp that is more fully initialised. >> >> For consistency, NaClAppCreate() should be named "*Make" rather than >> "*Create" (matching other functions in the NaCl codebase), and should >> really have a "NaClChromeMain" prefix to match the rest of this header >> file. >> >> https://codereview.chromium.org/135853021/ -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at http://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/groups/opt_out.
On Fri, Jan 17, 2014 at 9:42 AM, <mseaborn@chromium.org> wrote: > > https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... > File src/trusted/service_runtime/sel_main_chrome.c (right): > > https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... > src/trusted/service_runtime/sel_main_chrome.c:127: struct NaClApp* > NaClAppCreate(void) { > Spacing style should be " *" in NaCl-side code (same elsewhere in this > change). > > https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... > src/trusted/service_runtime/sel_main_chrome.c:129: if (!nap) > NaCl style is "nap == NULL" > > https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... > src/trusted/service_runtime/sel_main_chrome.c:133: if (NaClAppCtor(nap) > != 0) { > There's a correctness problem here. If NaClAppCreate() is called before > NaClChromeMainStartApp(), the former will happen without > NaClAllModulesInit() having been called. > > Can you run trybots for NaCl changes before sending them for review, > please? I don't know if this works with sel_main_chrome_test on all > platforms. > > BTW, "git cl try" only works for changes to the Chromium repo, not any > other repo. You have to use "git try" instead. Is there a way to make "git cl try" work for native_client? This is really confusing as a Chromium developer. > > https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... > File src/trusted/service_runtime/sel_main_chrome.h (right): > > https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... > src/trusted/service_runtime/sel_main_chrome.h:157: struct NaClApp* > NaClAppCreate(void); > Why do you want to do the interface like this rather than the version > from https://code.google.com/p/chromium/issues/detail?id=333950, which > was: > > struct NaClApp *NaClChromeMainCreateApp(struct NaClChromeMainArgs > *args); > > That seemed a little cleaner as an embedding interface because it > returns a NaClApp that is more fully initialised. It shouldn't matter to the embedding interface that NaClApp is more fully initialized since it can't see inside the NaClApp type anyway. My hope was to make this allocate the memory so the embedder could manage the lifetime however it likes. It's not clear how much should be initialized. In the method you proposed, should NaClChromeMainCreateApp(...) load the IRT? It's not clear. > > For consistency, NaClAppCreate() should be named "*Make" rather than > "*Create" (matching other functions in the NaCl codebase), and should > really have a "NaClChromeMain" prefix to match the rest of this header > file. > > https://codereview.chromium.org/135853021/ -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at http://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/groups/opt_out.
On 17 January 2014 09:08, Justin TerAvest <teravest@chromium.org> wrote: > On Fri, Jan 17, 2014 at 9:42 AM, <mseaborn@chromium.org> wrote:> > https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... > > src/trusted/service_runtime/sel_main_chrome.c:133: if (NaClAppCtor(nap) > > != 0) { > > There's a correctness problem here. If NaClAppCreate() is called before > > NaClChromeMainStartApp(), the former will happen without > > NaClAllModulesInit() having been called. > > > > Can you run trybots for NaCl changes before sending them for review, > > please? I don't know if this works with sel_main_chrome_test on all > > platforms. > > > > BTW, "git cl try" only works for changes to the Chromium repo, not any > > other repo. You have to use "git try" instead. > > Is there a way to make "git cl try" work for native_client? This is > really confusing as a Chromium developer. > You'll have to ask the Chrome Infrastructure team about that. "git cl try" works by having the buildbot master poll Rietveld for triggered try jobs. We tried to enable that polling a while ago in this change: http://src.chromium.org/viewvc/chrome?view=revision&revision=162253 However, it severely confused the Chromium buildbot master (see https://code.google.com/p/chromium/issues/detail?id=156266), so the change got reverted. I think this infrastructure currently only works for a single repo at a time. > > https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... > > File src/trusted/service_runtime/sel_main_chrome.h (right): > > > > > https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... > > src/trusted/service_runtime/sel_main_chrome.h:157: struct NaClApp* > > NaClAppCreate(void); > > Why do you want to do the interface like this rather than the version > > from https://code.google.com/p/chromium/issues/detail?id=333950, which > > was: > > > > struct NaClApp *NaClChromeMainCreateApp(struct NaClChromeMainArgs > > *args); > > > > That seemed a little cleaner as an embedding interface because it > > returns a NaClApp that is more fully initialised. > > It shouldn't matter to the embedding interface that NaClApp is more > fully initialized since it can't see inside the NaClApp type anyway. > The basic interface you proposed is OK with me. I just wondered if you had a specific use case for getting a NaClApp earlier, before creating a NaClChromeMainArgs. > My hope was to make this allocate the memory so the embedder could > manage the lifetime however it likes. It's not clear how much should > be initialized. > In any case I agree that the NaClApp shouldn't be allocated on the stack any more. In the method you proposed, should NaClChromeMainCreateApp(...) load > the IRT? It's not clear. In the interface I proposed, in the initial implementation NaClChromeMainLoadAndRunNexe() would do the work of loading the IRT -- though ideally that would just be an implementation detail. (Currently we load the IRT after the main nexe, but this is the result of a quirk in how the two ELF loaders are implemented.) I can see that would be more work to implement, though, because sel_main_chrome.c would have to stash the IRT descriptor somewhere between the calls. To make this simpler to implement, we could change that to: void NaClChromeMainLoadAndRunNexe(struct NaClApp *nap, struct NaClDesc *main_nexe_desc, struct NaClDesc *irt_desc); or, less self-consistently, but more consistent with the current interface: void NaClChromeMainLoadAndRunNexe(struct NaClApp *nap, struct NaClDesc *main_nexe_desc, int irt_fd); Mark -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at http://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/groups/opt_out.
Thanks, Mark. I've changed the embedding interface to look more like you describe, but not exactly. It looks good on the trybots, as far as I can tell. I now have: void NaClChromeMainLoadAndRunNexe(struct NaClApp *nap, int irt_fd); As I understand it, we'll drop args->imc_bootstrap_channel later, and add a "struct NaClDesc *main_nexe_desc" argument above, but I'm not ready for that just yet. Let me know what you think. On Tue, Jan 21, 2014 at 12:08 PM, Mark Seaborn <mseaborn@chromium.org> wrote: > On 17 January 2014 09:08, Justin TerAvest <teravest@chromium.org> wrote: >> >> On Fri, Jan 17, 2014 at 9:42 AM, <mseaborn@chromium.org> wrote:> >> https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... >> > src/trusted/service_runtime/sel_main_chrome.c:133: if (NaClAppCtor(nap) >> > != 0) { >> > There's a correctness problem here. If NaClAppCreate() is called before >> > NaClChromeMainStartApp(), the former will happen without >> > NaClAllModulesInit() having been called. >> > >> > Can you run trybots for NaCl changes before sending them for review, >> > please? I don't know if this works with sel_main_chrome_test on all >> > platforms. >> > >> > BTW, "git cl try" only works for changes to the Chromium repo, not any >> > other repo. You have to use "git try" instead. >> >> Is there a way to make "git cl try" work for native_client? This is >> really confusing as a Chromium developer. > > > You'll have to ask the Chrome Infrastructure team about that. "git cl try" > works by having the buildbot master poll Rietveld for triggered try jobs. > We tried to enable that polling a while ago in this change: > http://src.chromium.org/viewvc/chrome?view=revision&revision=162253 > However, it severely confused the Chromium buildbot master (see > https://code.google.com/p/chromium/issues/detail?id=156266), so the change > got reverted. I think this infrastructure currently only works for a single > repo at a time. > > >> > >> > https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... >> > File src/trusted/service_runtime/sel_main_chrome.h (right): >> > >> > >> > https://codereview.chromium.org/135853021/diff/40001/src/trusted/service_runt... >> > src/trusted/service_runtime/sel_main_chrome.h:157: struct NaClApp* >> > NaClAppCreate(void); >> > Why do you want to do the interface like this rather than the version >> > from https://code.google.com/p/chromium/issues/detail?id=333950, which >> > was: >> > >> > struct NaClApp *NaClChromeMainCreateApp(struct NaClChromeMainArgs >> > *args); >> > >> > That seemed a little cleaner as an embedding interface because it >> > returns a NaClApp that is more fully initialised. >> >> It shouldn't matter to the embedding interface that NaClApp is more >> fully initialized since it can't see inside the NaClApp type anyway. > > > The basic interface you proposed is OK with me. > > I just wondered if you had a specific use case for getting a NaClApp > earlier, before creating a NaClChromeMainArgs. > > >> >> My hope was to make this allocate the memory so the embedder could >> manage the lifetime however it likes. It's not clear how much should >> be initialized. > > > In any case I agree that the NaClApp shouldn't be allocated on the stack any > more. > > >> In the method you proposed, should NaClChromeMainCreateApp(...) load >> the IRT? It's not clear. > > > In the interface I proposed, in the initial implementation > NaClChromeMainLoadAndRunNexe() would do the work of loading the IRT -- > though ideally that would just be an implementation detail. (Currently we > load the IRT after the main nexe, but this is the result of a quirk in how > the two ELF loaders are implemented.) > > I can see that would be more work to implement, though, because > sel_main_chrome.c would have to stash the IRT descriptor somewhere between > the calls. > > To make this simpler to implement, we could change that to: > > void NaClChromeMainLoadAndRunNexe(struct NaClApp *nap, struct NaClDesc > *main_nexe_desc, struct NaClDesc *irt_desc); > > or, less self-consistently, but more consistent with the current interface: > > void NaClChromeMainLoadAndRunNexe(struct NaClApp *nap, struct NaClDesc > *main_nexe_desc, int irt_fd); > > Mark > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at http://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/groups/opt_out.
ping
Closing this for now. I chatted with Mark offline. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
