Re: Review Request 49348: Added implementation to Appc Runtime Isolator.

[Gilbert Song]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49348/#review142765
-----------------------------------------------------------




src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 247)
<https://reviews.apache.org/r/49348/#comment208386>

    This is a bug. You may get segfault if `!command.has_value() === true`.
    
    Let's dont check `command.value().empty()` here. If it is an empty string, 
errors will be returned by `execlp`.



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 255)
<https://reviews.apache.org/r/49348/#comment208387>

    Same here. If we check on `!command.value().empty()` here. We are leaking 
some cases in logic. Please remove it.



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (lines 263 - 289)
<https://reviews.apache.org/r/49348/#comment208399>

    We can optimize the logic here:
    
    ```
    if (app.exec_size() > 0) {
      command.set_value(app.exec(0));
      
      command.clear_arguments();
      command.add_arguments(app.exec(0));
      
      if (!containerConfig.has_task_info()) {
        command.mutable_ARGUMENTS()->MergeFrom(
            containerConfig.executor_info().command().arguments());
      } else {
        command.mutable_arguments()->MergeFrom(
            containerConfig.task_info().command().arguments());
      }
      
      if (command.arguments_size() == 1) {
        for (int i = 1; i < app.exec_size(); i++) {
          command.add_arguments(app.exec(i));
        }
      }
    } else {
      return Error("No executable is found");
    }


- Gilbert Song


On July 14, 2016, 10:27 a.m., Srinivas Brahmaroutu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49348/
> -----------------------------------------------------------
> 
> (Updated July 14, 2016, 10:27 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-4778
>     https://issues.apache.org/jira/browse/MESOS-4778
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added implementation to Appc Runtime Isolator.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/isolators/appc/runtime.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49348/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>