On 2016-08-08 22:39, Ralf Ramsauer wrote:
> And keep the sorted output format of the former python script.
>
> Signed-off-by: Ralf Ramsauer <r...@ramses-pyramidenbau.de>
>
> diff --git a/tools/jailhouse-cell-list b/tools/jailhouse-cell-list
> deleted file mode 100755
> index d5a535b..0000000
> --- a/tools/jailhouse-cell-list
> +++ /dev/null
> @@ -1,40 +0,0 @@
> -#!/usr/bin/env python
> -
> -# Jailhouse, a Linux-based partitioning hypervisor
> -#
> -# Copyright (c) Siemens AG, 2014
> -#
> -# Authors:
> -# Jan Kiszka <jan.kis...@siemens.com>
> -#
> -# This work is licensed under the terms of the GNU GPL, version 2. See
> -# the COPYING file in the top-level directory.
> -
> -import glob
> -import os
> -import sys
> -
> -
> -if len(sys.argv) > 1:
> - print("usage: %s" % os.path.basename(sys.argv[0]).replace("-", " "))
> - exit(0 if sys.argv[1] in ("--help", "-h") else 1)
> -
> -cells = []
> -for cell_path in glob.glob('/sys/devices/jailhouse/cells/*'):
> - cells.append({
> - 'id': os.path.basename(cell_path),
> - 'name': open(cell_path + "/name").readline().strip(),
> - 'state': open(cell_path + "/state").readline().strip(),
> - 'cpus_assigned_list':
> - open(cell_path + "/cpus_assigned_list").readline().strip(),
> - 'cpus_failed_list':
> - open(cell_path + "/cpus_failed_list").readline().strip()
> - })
> -
> -line_format = "%-8s%-24s%-16s%-24s%-24s"
> -if not cells == []:
> - print(line_format % ("ID", "Name", "State",
> - "Assigned CPUs", "Failed CPUs"))
> -for cell in sorted(cells, key=lambda cell: cell['id']):
> - print(line_format % (cell['id'], cell['name'], cell['state'],
> - cell['cpus_assigned_list'],
> cell['cpus_failed_list']))
> diff --git a/tools/jailhouse.c b/tools/jailhouse.c
> index ae2ed3f..fe4aa6e 100644
> --- a/tools/jailhouse.c
> +++ b/tools/jailhouse.c
> @@ -14,6 +14,7 @@
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> +#include <dirent.h>
> #include <unistd.h>
> #include <fcntl.h>
> #include <errno.h>
> @@ -27,6 +28,7 @@
>
> #define JAILHOUSE_EXEC_DIR LIBEXECDIR "/jailhouse"
> #define JAILHOUSE_DEVICE "/dev/jailhouse"
> +#define JAILHOUSE_CELLS "/sys/devices/jailhouse/cells/"
>
> enum shutdown_load_mode {LOAD, SHUTDOWN};
>
> @@ -34,11 +36,17 @@ struct extension {
> char *cmd, *subcmd, *help;
> };
>
> +struct jailhouse_cell_info {
> + struct jailhouse_cell_id id;
> + char *state;
> + char *cpus_assigned_list;
> + char *cpus_failed_list;
> +};
> +
> static const struct extension extensions[] = {
> { "cell", "linux", "CELLCONFIG KERNEL [-i | --initrd FILE]\n"
> " [-c | --cmdline \"STRING\"] "
> "[-w | --write-params FILE]" },
> - { "cell", "list", "" },
> { "cell", "stats", "{ ID | [--name] NAME }" },
> { "config", "create", "[-h] [-g] [-r ROOT] "
> "[--mem-inmates MEM_INMATES]\n"
> @@ -57,6 +65,7 @@ static void __attribute__((noreturn)) help(char *prog, int
> exit_status)
> " enable SYSCONFIG\n"
> " disable\n"
> " cell create CELLCONFIG\n"
> + " cell list\n"
> " cell load { ID | [--name] NAME } "
> "{ IMAGE | { -s | --string } \"STRING\" }\n"
> " [-a | --address ADDRESS] ...\n"
> @@ -163,6 +172,20 @@ static void *read_file(const char *name, size_t *size)
> return buffer;
> }
>
> +static char *read_sysfs_entry(const char *name)
> +{
> + char *ret;
> + size_t size;
> +
> + ret = read_file(name, &size);
> +
> + /* enforce the string to be null-terminated */
> + if (size && ret[size-1] != '\0')
Second check is not needed: in the worst case, we overwrite 0 with 0...
> + ret[size-1] = '\0';
> +
> + return ret;
> +}
> +
> static int enable(int argc, char *argv[])
> {
> void *config;
> @@ -251,6 +274,82 @@ static bool match_opt(const char *argv, const char
> *short_opt,
> strcmp(argv, long_opt) == 0;
> }
>
> +static struct jailhouse_cell_info *get_cell_info(const int id)
> +{
> + char buf[sizeof(JAILHOUSE_CELLS) + 3 + sizeof("/cpus_assigned_list") +
> 1];
This buffer size calculation wants to be careful, but if some other
string length changes, it will also break. Let's just use a safe larger
size, like 64 or 128. You use snprintf below anyway.
> + struct jailhouse_cell_info *cinfo;
> + char *tmp;
> +
> + cinfo = calloc(1, sizeof(struct jailhouse_cell_info));
calloc could fail and return NULL. OK, unlikely.
Why calloc (for one element) and not malloc?
> +
> + /* set cell id */
> + cinfo->id.id = id;
> +
> + /* get cell name */
> + snprintf(buf, sizeof(buf), JAILHOUSE_CELLS "%d/name", id);
> + tmp = read_sysfs_entry(buf);
> + strncpy(cinfo->id.name, tmp, JAILHOUSE_CELL_ID_NAMELEN);
> + free(tmp);
> +
> + /* get cell state */
> + snprintf(buf, sizeof(buf), JAILHOUSE_CELLS "%d/state", id);
> + cinfo->state = read_sysfs_entry(buf);
> +
> + /* get assigned cpu list */
> + snprintf(buf, sizeof(buf), JAILHOUSE_CELLS "%d/cpus_assigned_list", id);
> + cinfo->cpus_assigned_list = read_sysfs_entry(buf);
> +
> + /* get failed cpu list */
> + snprintf(buf, sizeof(buf), JAILHOUSE_CELLS "%d/cpus_failed_list", id);
> + cinfo->cpus_failed_list = read_sysfs_entry(buf);
> +
> + return cinfo;
> +}
> +
> +static void cell_info_free(struct jailhouse_cell_info *cinfo)
> +{
> + free(cinfo->state);
> + free(cinfo->cpus_assigned_list);
> + free(cinfo->cpus_failed_list);
> + free(cinfo);
> +}
Single-caller function...
> +
> +static int cell_list(int argc, char *argv[])
> +{
> + struct dirent **namelist;
> + struct jailhouse_cell_info *cinfo;
> + int id, i, num_entries;
> + (void)argv;
> +
> + if (argc != 3)
> + help(argv[0], 1);
> +
> + printf("%-8s%-24s%-16s%-24s%-24s\n",
> + "ID", "Name", "State", "Assigned CPUs", "Failed CPUs");
> +
> + num_entries = scandir(JAILHOUSE_CELLS, &namelist, 0, alphasort);
> + if (num_entries < 0) {
> + perror("scandir");
> + exit(1);
Will we panic here if jailhouse isn't loaded yet? The current version
just does nothing, not even print the banner.
> + }
> +
> + for (i = 0; i < num_entries; i++) {
> + /* Skip '.' and '..' */
> + if (namelist[i]->d_name[0] != '.') {
If you invert the test here and just do "continue;", you could save
indention below.
> + id = (int)strtol(namelist[i]->d_name, NULL, 10);
> +
> + cinfo = get_cell_info(id);
> + printf("%-8d%-24s%-16s%-24s%-24s\n", cinfo->id.id,
> cinfo->id.name,
> + cinfo->state, cinfo->cpus_assigned_list,
> cinfo->cpus_failed_list);
Overlong lines.
> + cell_info_free(cinfo);
> + }
> + free(namelist[i]);
> + }
> + free(namelist);
> +
> + return 0;
> +}
> +
> static int cell_shutdown_load(int argc, char *argv[],
> enum shutdown_load_mode mode)
> {
> @@ -369,6 +468,8 @@ static int cell_management(int argc, char *argv[])
>
> if (strcmp(argv[2], "create") == 0) {
> err = cell_create(argc, argv);
> + } else if (strcmp(argv[2], "list") == 0) {
> + err = cell_list(argc, argv);
> } else if (strcmp(argv[2], "load") == 0) {
> err = cell_shutdown_load(argc, argv, LOAD);
> } else if (strcmp(argv[2], "start") == 0) {
>
Just minor comments now, looks good in general.
But take your time for v4, I'm not going to close a merge window these
days ;).
Jan
--
You received this message because you are subscribed to the Google Groups
"Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to jailhouse-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
