Code review comment for lp:~milo/lava-tool/lava-167

On Thu, Jun 13, 2013 at 2:46 AM, Antonio Terceiro
<email address hidden> wrote:
>> == added file 'lava/device/commands.py'
>> -- lava/device/commands.py 1970-01-01 00:00:00 +0000
>> ++ lava/device/commands.py 2013-06-12 13:21:33 +0000
>> @ -0,0 +1,237 @@
> [snip]
>> # Default lava-dispatcher paths.
>> DEFAULT_DISPATCHER_PATH = os.path.join("etc", "lava-dispatcher")
>> USER_DISPATCHER_PATH = os.path.join(os.path.expanduser("~"), ".config",
>> "lava-dispatcher")
>
> It would be better to
>
> from lava_dispatcher.import user_config_path, system_config_path
>
> and use those instead, so you don't need to duplicate the logic here. We don't
> want to make the dispatcher a hard dependency of lava-tool, but if we don't
> have the dispatcher available, just fail the entire thing.

Modifying the logic in the _get_dispatcher_paths() and the other
methods that deal with picking the correct path, I think it is not
necessary to import "user_config_path" and "system_config_path" from
the dispatcher.

Since we get the paths via lava_dispatcher.config.search_path(), it
will return those paths already, and if we cannot import it we will
fail.

> Right here. We should baild out if the dispatcher is not available for device
> management. After all without a local dispatcher there is no point in managing
> devices.

Sounds cool.
I was trying to be a little bit more "gentle" and fallback to let
users create devices anyway. :-)

> I think we should not give too many options here: if the user is capable of
> choosing the right directory to write to, he doesn't need lava-tool to do it
> for him; he would just create a file with $EDITOR there.
>
> Instead of this, I think you should assume that dispatcher branch of mine, and
> use the following strategy:
>
> 1) finding where to create a new configurarion file:
>
> the first of [system_dispatcher_path, user_config_path] that is writable
>
> 2) finding an existing configuration file to edit:
>
> from lava_dispatcher.config import get_config_file
> [...]
>
> conf_file = get_config_file("devices/%s.conf", device_name)
> if conf_file:
> # edit
> else:
> print("No device %s found" % device_name)
> exit(-1)
>
> I think that doing this will simplify a lot things in this file.

OK, I reworked this part, probably in a slighter different way.
This is how it will work now for the add command:

- We check if a device with that config file name already exists: this
is now done with lava_dispatcher.config.get_config_file. If we cannot
import the function, we fail.
- If the device already exists, we exit printing to use "lava device
config DEVICE" (as was before)
- Otherwise, get the paths from the dispatcher using
lava_dispatcher.config.search_path() (again, if it gives errors we
fail)
- Check if one of those paths is writable and pick the first one that
is, otherwise fail
- At this stage we are sure that the device doesn't exists and we can create it.

There might be another check we want to do though: if
lava_dispatcher.config.get_config_file() returns a path to an existing
file that is not writable by the user (in particular for the "config"
command).

> I think something like this would be more readable:
>
> TEMPLATES = {
> 'panda': {
> 'device_type': 'panda',
> 'connection_command' None,
> },
> vexpress: {
> 'device_type': 'vexpress',
> 'connection_command' None,
> },
> }

Fixed.
Thanks!

--
Milo Casagrande | Automation Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs