On Wed, Nov 11, 2015 at 09:39:53AM +0100, Christian Loehle wrote:
> diff --git a/configure.ac b/configure.ac
> index f481c50..8b68828 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -563,6 +563,10 @@ AC_ARG_WITH([hyperv],
> [AS_HELP_STRING([--with-hyperv],
> [add Hyper-V support @<:@default=check@:>@])])
> m4_divert_text([DEFAULTS], [with_hyperv=check])
> +AC_ARG_WITH([jailhouse],
> + [AS_HELP_STRING([--with-jailhouse],
> + [add Jailhouse support @<:@default=yes@:>@])])
> +m4_divert_text([DEFAULTS], [with_jailhouse=yes])
> AC_ARG_WITH([test],
> [AS_HELP_STRING([--with-test],
> [add test driver support @<:@default=yes@:>@])])
> @@ -722,6 +726,16 @@ AM_CONDITIONAL([WITH_VMWARE], [test "$with_vmware" = "yes"])
>
>
> dnl
> +dnl Checks for the Jailhouse driver
> +dnl
> +
> +if test "$with_jailhouse" = "yes"; then
> + AC_DEFINE_UNQUOTED([WITH_JAILHOUSE], 1, [whether Jailhouse driver is enabled])
> +fi
> +AM_CONDITIONAL([WITH_JAILHOUSE], [test "$with_jailhouse" = "yes"])
> +
> +
> +dnl
> dnl check for XDR
> dnl
>
> @@ -1087,6 +1101,12 @@ dnl
> LIBVIRT_DRIVER_CHECK_BHYVE
>
> dnl
> +dnl Checks for Jailhouse driver
> +dnl
> +
> +AM_CONDITIONAL([WITH_JAILHOUSE], [test "$with_jailhouse" = "yes"])
> +
> +dnl
> dnl check for shell that understands <> redirection without truncation,
> dnl needed by src/qemu/qemu_monitor_{text,json}.c.
> dnl
> @@ -2830,6 +2850,7 @@ AC_MSG_NOTICE([ ESX: $with_esx])
> AC_MSG_NOTICE([ Hyper-V: $with_hyperv])
> LIBVIRT_DRIVER_RESULT_VZ
> LIBVIRT_DRIVER_RESULT_BHYVE
> +AC_MSG_NOTICE([Jailhouse: $with_jailhouse])
> AC_MSG_NOTICE([ Test: $with_test])
> AC_MSG_NOTICE([ Remote: $with_remote])
> AC_MSG_NOTICE([ Network: $with_network])
Rather than putting all the rules in configure.ac, can you create
a m4/virt-driver-jailhouse.m4 file and then call its fnuctions
from configure.ac Take a look at m4/virt-driver-vz.m4 for an
example of best practice.
> diff --git a/src/jailhouse/README b/src/jailhouse/README
> new file mode 100644
> index 0000000..96c32b1
> --- /dev/null
> +++ b/src/jailhouse/README
> @@ -0,0 +1,3 @@
> +The jailhouse hypervisor driver for the libvirt project aims to provide rudimentary support for managing jailhouse with the libvirt library. The main advantage of this is the possibility to use virt-manager as a GUI to manage Jailhouse cells. Thus the driver is mainly built around the API calls that virt-manager uses and needs.
> +Due to the concept of Jailhouse a lot of libvirt functions can't be realized, so this driver isn't as full-featured as upstream drivers of the libvirt project.
> +Currently the driver relies on the Jailhouse binary, which has to be passed when connecting a libvirt client to it(e.g. virt-manager -c jailhouse:///path/to/jailhouse/tools/jailhouse). Be aware though that the driver doesn't store any information about cells, so most API calls use "jailhouse cell list" every time they're called to get the current state.
Can you line wrap this file at 80 characters or less
> +
> +/*
> + * The driver requeries the cells on most calls, it stores the result of the last query, so it can copy the UUIDs in the new query if the cell is the same(otherwise it just generates a new one)
> + * not preserving the UUID results in a lot of bugs in libvirts clients.
And line wrap this comment
> + */
> +struct jailhouse_driver {
For naming can you make all structs have 'virJailHouse' as the
name prefix. so eg virJailHouseDriver
> + char *binary;
> + size_t lastQueryCellsCount;
> + struct jailhouse_cell* lastQueryCells;
> +};
> +
> +/*
> + * CPUs are currently unused but this might change
> + */
> +struct jailhouse_cell {
And virJailHouseCell, etc...
> + int id;
> + char name[NAMELENGTH+1];
> + int state;
> + int *assignedCPUs; //Don't use cpumask because remote system might have different # of cpus
> + int assignedCPUsLength;
> + int *failedCPUs;
> + int failedCPUsLength;
> + unsigned char uuid[VIR_UUID_BUFLEN];
> +};
Also, we usually provide 2 typedefs eg
typedef struct virJailHouseCell virJailHouseCell;
typedef virJailHouseCell *virJailHouseCellPtr;
so that we can omit 'struct' when declaring variables
and casting.
> +
> +/*
> + * helper function that returns the number as an integer and sets i to be the first char after the number
> + */
> +static int
> +charsToInt(char* chars, size_t *i)
> +{
> + int result = 0;
> + while (chars[*i] != ',' && chars[*i] != '-' && chars[*i] != ' ') {
> + result *= 10;
> + result += chars[*i] - '0';
> + (*i)++;
> + }
> + return result;
> +}
You can use virStrToLong_i() instead of creating this.
> +/*
> + * Takes a string in the format of "jailhouse cell list" as input,
It would be nice to include an example of the output here
for people who aren't clear about the format being parsed.
> + * allocates an int array in which every CPU is explicitly listed and saves a pointer in cpusptr
> + */
> +static size_t
> +parseCPUs(char* output, int **cpusptr)
Similar to struct naming, can you use 'virJailHouse' as the
prefix for all function names too.
The 'output' parameter looks like it should be 'const char*'
rather than just 'char *'.
> +{
> + size_t i;
> + size_t count = 1;
> + int number;
> + int* cpus;
> + if (output[0] == ' ') {
> + *cpusptr = NULL;
> + return 0;
Slightly more normal practice is to use -1 for error
conditions - if you change the return type to ssize_t
instead of size_t you can still use -1.
> + }
> + for (i = 0; i<CPULENGTH; i++) {
> + number = charsToInt(output, &i);
> + if (output[i] == ',') {
> + count++;
> + } else if (output[i] == '-') {
> + i++;
> + count += charsToInt(output, &i) - number;
> + }
> + }
> + if (VIR_ALLOC_N(cpus, count)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to allocate CPUs array of size %zu"), count);
No need to report an error - VIR_ALLOC_N does that for you these
days.
> + return 0;
> + }
> + size_t j = 0;
> + i = 0;
> + while (output[i] != ' ') {
> + number = charsToInt(output, &i);
> + if (output[i] == ',' || output[i] == ' ') {
> + cpus[j++] = number;
> + } else if (output[i] == '-') {
> + i++;
> + int nextNumber = charsToInt(output, &i);
> + for (; number <= nextNumber; number++) cpus[j++] = number;
> + }
> + i++;
> + }
> + *cpusptr = cpus;
> + return count;
> +}
> +
> +/*
> + * calls "jailhouse cell list" and parses the output in an array of jailhouse_cell
> + */
> +static size_t
> +parseListOutput(virConnectPtr conn, struct jailhouse_cell **parsedOutput)
> +{
> + virCommandPtr cmd = virCommandNew(((struct jailhouse_driver *)conn->privateData)->binary);
> + virCommandAddArg(cmd, "cell");
> + virCommandAddArg(cmd, "list");
> + virCommandAddEnvPassCommon(cmd);
> + char *output;
We tend to prefer that all variables be decalared at the start
of the function rather than inline - otherwise when you have
'goto' statements you can have non-obvious cases where a
variable is not initialized as expected.
> + virCommandSetOutputBuffer(cmd, &output);
> + size_t count = -1; // Don't count table header line
> + size_t i = 0;
> + if (virCommandRun(cmd, NULL) < 0)
> + goto error;
> + while (output[i] != '\0') {
> + if (output[i] == '\n') count++;
> + i++;
> + }
> + if (VIR_ALLOC_N(*parsedOutput, count)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to allocate jailhouse_cell array of size %zu"), count);
Again no need to report error. You need to check
VIR_ALLOC_N(...) < 0
too - this error check is inverted right now
> + goto error;
> + }
> + if (*parsedOutput == NULL)
> + goto error;
This is redundant if you check VIR_ALLOC_N retrn
status correctly.
But more generally, it feels like it would probably have
made sense to use virStringSplit(output, "\n") to break
it into separate lines
> + i = 0;
> + size_t j;
> + while (output[i++] != '\n'); // Skip table header line
> + for (j = 0; j < count; j++) {
> + size_t k;
> + for (k = 0; k <= IDLENGTH; k++) // char after number needs to be NUL for virStrToLong
> + if (output[i+k] == ' ') {
> + output[i+k] = '\0';
> + break;
> + }
> + char c = output[i+IDLENGTH];
> + output[i+IDLENGTH] = '\0'; // in case ID is 8 chars long, so beginning of name won't get parsed
> + if (virStrToLong_i(output+i, NULL, 0, &(*parsedOutput)[j].id))
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to parse id to long: %s"), output+i);
> + output[i+IDLENGTH] = c;
> + i += IDLENGTH;
> + if (virStrncpy((*parsedOutput)[j].name, output+i, NAMELENGTH, NAMELENGTH+1) == NULL)
> + // should never happen
> + goto error;
> + (*parsedOutput)[j].name[NAMELENGTH] = '\0';
> + for (k = 0; k < NAMELENGTH; k++)
> + if ((*parsedOutput)[j].name[k] == ' ')
> + break;
> + (*parsedOutput)[j].name[k] = '\0';
> + i += NAMELENGTH;
> + if (STREQLEN(output+i, STATERUNNINGSTRING, STATELENGTH)) (*parsedOutput)[j].state = STATERUNNING;
> + else if (STREQLEN(output+i, STATESHUTDOWNSTRING, STATELENGTH)) (*parsedOutput)[j].state = STATESHUTDOWN;
> + else if (STREQLEN(output+i, STATEFAILEDSTRING, STATELENGTH)) (*parsedOutput)[j].state = STATEFAILED;
> + else if (STREQLEN(output+i, STATERUNNINGLOCKEDSTRING, STATELENGTH)) (*parsedOutput)[j].state = STATERUNNINGLOCKED;
> + i += STATELENGTH;
> + (*parsedOutput)[j].assignedCPUsLength = parseCPUs(output+i, &((*parsedOutput)[j].assignedCPUs));
> + i += CPULENGTH;
> + (*parsedOutput)[j].failedCPUsLength = parseCPUs(output+i, &((*parsedOutput)[j].failedCPUs));
> + i += CPULENGTH;
> + i++; // skip \n
> + }
It would be nice to include an example of the 'cell list' output to understand
what this code is trying to parse. Then i migth be able to suggest a way
to do this more simply.
> + VIR_FREE(output);
> + virCommandFree(cmd);
> + return count;
> + error:
> + for (i = 0; i < count; i++) {
> + VIR_FREE((*parsedOutput)[i].assignedCPUs);
> + VIR_FREE((*parsedOutput)[i].failedCPUs);
> + }
> + VIR_FREE(*parsedOutput);
> + *parsedOutput = NULL;
> + VIR_FREE(output);
> + output = NULL;
> + virCommandFree(cmd);
> + return -1;
> +}
> +/*
> + * Check cells for cell and copies UUID if found, otherwise generates a new one, this is to preserve UUID in libvirt
> + */
> +static void setUUID(struct jailhouse_cell *cells, size_t count, struct jailhouse_cell* cell) {
> + size_t i;
> + for (i = 0; i < count; i++) {
> + if (strncmp(cells[i].name, cell->name, NAMELENGTH+1))
> + continue;
Use STREQLEN() instead of strncmp - btw if you rnu 'make syntax-check' it
ought to have warned you about this.
> + memcpy(cell->uuid, cells[i].uuid, VIR_UUID_BUFLEN);
> + return;
> + }
> + virUUIDGenerate(cell->uuid);
> +}
> +static void
> +getCurrentCellList(virConnectPtr conn)
> +{
> + size_t lastCount = ((struct jailhouse_driver *)conn->privateData)->lastQueryCellsCount;
> + struct jailhouse_cell *lastCells = ((struct jailhouse_driver *)conn->privateData)->lastQueryCells;
Rather than casting priateData many times it is nicer to just
declare a variable for it at the start of the function - likewise
for many other methods in this file.
eg
virJailHouseDriverPtr driver = conn->privateData;
> + struct jailhouse_cell *cells = NULL;
> + size_t i;
> + size_t count = parseListOutput(conn, &cells);
> + for (i = 0; i < count; i++)
> + setUUID(lastCells, lastCount, cells+i);
> + for (i = 0; i < lastCount; i++) {
> + VIR_FREE(lastCells[i].assignedCPUs);
> + VIR_FREE(lastCells[i].failedCPUs);
> + }
> + VIR_FREE(lastCells);
> + ((struct jailhouse_driver *)conn->privateData)->lastQueryCells = cells;
> + ((struct jailhouse_driver *)conn->privateData)->lastQueryCellsCount = count;
> +}
> +/*
> + * There currently is no straightforward way for the driver to retrieve those,
> + * so maxMem, memory and cpuTime have dummy values
> + */
> +static int
> +jailhouseDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info)
> +{
> + struct jailhouse_cell *cell = virDomainPtrToCell(domain);
> + if (cell == NULL)
> + return -1;
> + info->state = cellToVirDomainState(cell);
> + info->maxMem = 1;
> + info->memory = 1;
> + info->nrVirtCpu = cell->assignedCPUsLength;
> + info->cpuTime = 1;
Just set them all to 0 rather than 1, as that's a better sentinel for
apps to detect to realize this data is not provided right now.
> +static int
> +jailhouseDomainShutdown(virDomainPtr domain)
> +{
> + virCommandPtr cmd = virCommandNew(((struct jailhouse_driver *)domain->conn->privateData)->binary);
> + virCommandAddArg(cmd, "cell");
> + virCommandAddArg(cmd, "shutdown");
> + char buf[IDLENGTH+1];
> + snprintf(buf, IDLENGTH+1, "%d", domain->id);
> + virCommandAddArg(cmd, buf);
You can just do
virCommandAddArgFormat(cmd, "%d", domain->id);
> + virCommandAddEnvPassCommon(cmd);
> + int resultcode = virCommandRun(cmd, NULL);
> + virCommandFree(cmd);
> + if (resultcode < 0)
> + return -1;
> + return 0;
> +}
> +static int
> +jailhouseDomainDestroy(virDomainPtr domain)
> +{
> + virCommandPtr cmd = virCommandNew(((struct jailhouse_driver *)domain->conn->privateData)->binary);
> + virCommandAddArg(cmd, "cell");
> + virCommandAddArg(cmd, "destroy");
> + char buf[IDLENGTH+1];
> + snprintf(buf, IDLENGTH+1, "%d", domain->id);
> + virCommandAddArg(cmd, buf);
Same coment about virCommandAddArgFormat() here and
in later methods too.
> + virCommandAddEnvPassCommon(cmd);
> + int resultcode = virCommandRun(cmd, NULL);
> + virCommandFree(cmd);
> + if (resultcode < 0)
> + return -1;
> + return 0;
> +}
> +
> +/*
> + * Returns a dummy capabilities XML for virt-manager
> + */
> +static char *
> +jailhouseConnectGetCapabilities(virConnectPtr conn ATTRIBUTE_UNUSED)
> +{
> + char* caps;
> + if (VIR_STRDUP(caps, "<capabilities></capabilities>") != 1)
> + return NULL;
Please fill in a virCapabiltiesPtr object then use the API
to format XML.
> + return caps;
> +}
> +
> +/*
> + * Returns a dummy XML for virt-manager
> + */
> +static char *
> +jailhouseDomainGetXMLDesc(virDomainPtr domain, unsigned int flags)
> +{
> + virCheckFlags(0, NULL);
> + char buf[200];
> + char uuid[VIR_UUID_STRING_BUFLEN];
> + virDomainGetUUIDString(domain, uuid);
> + snprintf(buf, 200, "<domain type =\"jailhouse\">\n\
> + <name>%s</name>\n\
> + <uuid>%s</uuid>\n\
> + </domain>", domain->name, uuid);
Here please fill in a virDomainDefPtr object and again use the API
to format XML
> + char* result;
> + if (VIR_STRDUP(result, buf) != 1)
> + return NULL;
> + return result;
> +}
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list