[PATCHv4 0/9] ch: Add Console support

[PATCH] qemu_driver:report guest...

[PATCH 00/24] virstoragetest:...

William Douglas

Thursday, 9 September 2021 Thu, 9 Sep '21

2:01 a.m.

This series enables console support in the cloud-hypervisor driver. Cloud-hypervisor only supports a single console or serial device at a time, hence the checks to ensure the domain configuration is only passing one or the other. Changes since v3: * Reset ch monitor's curl handle after a get info request * Improved PTY validation logic * Updated deviceValidateCallback to handle PTY validation * Added error reporting for unexpected PTY json data * Dropped the parallels scanning for PTY * Added a fix for handling deviceValidateCallback failures Changes since v2: * Squashed an additional patch. Changes since v1: * Added missing patch to add the virChrdev device * Added handling for multiple curl WRITEFUNCTION call backs * Added missing free for data.content * Removed redundant console configuration check * Improved handling of pty JSON data to make use of g_autoptr * Squashed two patches William Douglas (9): ch_domain: Add virChrdevs for console support ch_monitor: Make unused function static ch_monitor: Update virCHMonitorGet to handle accept a response ch_monitor: Use virCHMonitorGet to access cloud-hypervisor API ch_monitor: Add pty json builder function ch_process: Handle enabled console devices ch_domain: Allow controller and chr devices ch_driver: Add handler for console API ch_driver: Handle validation failure correctly src/ch/ch_domain.c | 40 +++++++++++++++++++- src/ch/ch_domain.h | 3 ++ src/ch/ch_driver.c | 76 ++++++++++++++++++++++++++++++++++++- src/ch/ch_monitor.c | 91 +++++++++++++++++++++++++++++++++++++++++++-- src/ch/ch_monitor.h | 3 ++ src/ch/ch_process.c | 84 +++++++++++++++++++++++++++++++++++++++++ 6 files changed, 290 insertions(+), 7 deletions(-) -- 2.31.1

Show replies by date

William Douglas

Thursday, 9 September Thu, 9 Sep

2:01 a.m.

New subject: [PATCH 1/9] ch_domain: Add virChrdevs for console support

Add and initialize a virChrdevs to the _virCHDomainObjPrivate structure in order to eventually track the consoles in use by a domain. Signed-off-by: William Douglas <william.douglas(a)intel.com> --- src/ch/ch_domain.c | 7 +++++++ src/ch/ch_domain.h | 3 +++ 2 files changed, 10 insertions(+) diff --git a/src/ch/ch_domain.c b/src/ch/ch_domain.c index 780a46ba00..a6b87e28e5 100644 --- a/src/ch/ch_domain.c +++ b/src/ch/ch_domain.c @@ -22,6 +22,7 @@ #include "ch_domain.h" #include "viralloc.h" +#include "virchrdev.h" #include "virlog.h" #include "virtime.h" @@ -146,6 +147,11 @@ virCHDomainObjPrivateAlloc(void *opaque G_GNUC_UNUSED) return NULL; } + if (!(priv->chrdevs = virChrdevAlloc())) { + g_free(priv); + return NULL; + } + return priv; } @@ -154,6 +160,7 @@ virCHDomainObjPrivateFree(void *data) { virCHDomainObjPrivate *priv = data; + virChrdevFree(priv->chrdevs); virCHDomainObjFreeJob(priv); g_free(priv); } diff --git a/src/ch/ch_domain.h b/src/ch/ch_domain.h index b4e0d4c212..61b34b0467 100644 --- a/src/ch/ch_domain.h +++ b/src/ch/ch_domain.h @@ -22,6 +22,7 @@ #include "ch_conf.h" #include "ch_monitor.h" +#include "virchrdev.h" /* Give up waiting for mutex after 30 seconds */ #define CH_JOB_WAIT_TIME (1000ull * 30) @@ -52,6 +53,8 @@ struct _virCHDomainObjPrivate { struct virCHDomainJobObj job; virCHMonitor *monitor; + + virChrdevs *chrdevs; }; extern virDomainXMLPrivateDataCallbacks virCHDriverPrivateDataCallbacks; -- 2.31.1

Daniel P. Berrangé

5:48 p.m.

New subject: [PATCH 1/9] ch_domain: Add virChrdevs for console support

On Wed, Sep 08, 2021 at 11:01:15AM -0700, William Douglas wrote:

...

Needs virCHDomainObjFreeJob(priv);

...

+ g_free(priv); + return NULL; + } + return priv; }

Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

William Douglas

2:01 a.m.

New subject: [PATCH 2/9] ch_monitor: Make unused function static

The virCHMonitorGet function wasn't in use and was declared as non-static (though not in a header file). This function isn't going to be used outside of the monitor though so remove the initial declaration and define the function to be static. Future work should adjust this function to allow reading VM state needed for PTY enablement. Signed-off-by: William Douglas <william.douglas(a)intel.com> --- src/ch/ch_monitor.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 0f8752d1ed..b4bc10bfcf 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -54,7 +54,6 @@ VIR_ONCE_GLOBAL_INIT(virCHMonitor); int virCHMonitorShutdownVMM(virCHMonitor *mon); int virCHMonitorPutNoContent(virCHMonitor *mon, const char *endpoint); -int virCHMonitorGet(virCHMonitor *mon, const char *endpoint); static int virCHMonitorBuildCPUJson(virJSONValue *content, virDomainDef *vmdef) @@ -612,7 +611,7 @@ virCHMonitorPutNoContent(virCHMonitor *mon, const char *endpoint) return ret; } -int +static int virCHMonitorGet(virCHMonitor *mon, const char *endpoint) { g_autofree char *url = NULL; -- 2.31.1

Daniel P. Berrangé

5:46 p.m.

New subject: [PATCH 2/9] ch_monitor: Make unused function static

On Wed, Sep 08, 2021 at 11:01:16AM -0700, William Douglas wrote:

...

This patch breaks the compile because we end up with an unused static function. It just needs to be moved a little later in the patch series. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

William Douglas

2:01 a.m.

New subject: [PATCH 3/9] ch_monitor: Update virCHMonitorGet to handle accept a response

The virCHMonitorGet function needed to be able to return data from the hypervisor. This functionality is needed in order for the driver to support PTY enablement and getting details about the VM state. Signed-off-by: William Douglas <william.douglas(a)intel.com> --- src/ch/ch_monitor.c | 46 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index b4bc10bfcf..44b99ef07a 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -611,12 +611,36 @@ virCHMonitorPutNoContent(virCHMonitor *mon, const char *endpoint) return ret; } +struct curl_data { + char *content; + size_t size; +}; + +static size_t +curl_callback(void *contents, size_t size, size_t nmemb, void *userp) +{ + size_t content_size = size * nmemb; + struct curl_data *data = (struct curl_data *)userp; + + if (content_size == 0) + return content_size; + + data->content = g_realloc(data->content, data->size + content_size); + + memcpy(&(data->content[data->size]), contents, content_size); + data->size += content_size; + + return content_size; +} + static int -virCHMonitorGet(virCHMonitor *mon, const char *endpoint) +virCHMonitorGet(virCHMonitor *mon, const char *endpoint, virJSONValue **response) { g_autofree char *url = NULL; int responseCode = 0; int ret = -1; + struct curl_slist *headers = NULL; + struct curl_data data = {0}; url = g_strdup_printf("%s/%s", URL_ROOT, endpoint); @@ -628,12 +652,30 @@ virCHMonitorGet(virCHMonitor *mon, const char *endpoint) curl_easy_setopt(mon->handle, CURLOPT_UNIX_SOCKET_PATH, mon->socketpath); curl_easy_setopt(mon->handle, CURLOPT_URL, url); + if (response) { + headers = curl_slist_append(headers, "Accept: application/json"); + headers = curl_slist_append(headers, "Content-Type: application/json"); + curl_easy_setopt(mon->handle, CURLOPT_HTTPHEADER, headers); + curl_easy_setopt(mon->handle, CURLOPT_WRITEFUNCTION, curl_callback); + curl_easy_setopt(mon->handle, CURLOPT_WRITEDATA, (void *)&data); + } + responseCode = virCHMonitorCurlPerform(mon->handle); virObjectUnlock(mon); - if (responseCode == 200 || responseCode == 204) + if (responseCode == 200 || responseCode == 204) { ret = 0; + if (response) { + data.content = g_realloc(data.content, data.size + 1); + data.content[data.size] = 0; + *response = virJSONValueFromString(data.content); + } + } + + g_free(data.content); + /* reset the libcurl handle to avoid leaking a stack pointer to data */ + curl_easy_reset(mon->handle); return ret; } -- 2.31.1

Daniel P. Berrangé

4:48 p.m.

New subject: [PATCH 3/9] ch_monitor: Update virCHMonitorGet to handle accept a response

On Wed, Sep 08, 2021 at 11:01:17AM -0700, William Douglas wrote:

...

FWIW, the type cast here is redundant as 'void *' can be directly assigned to/from any other type.

...

+ + if (content_size == 0) + return content_size; + + data->content = g_realloc(data->content, data->size + content_size); + + memcpy(&(data->content[data->size]), contents, content_size); + data->size += content_size; + + return content_size; +} + static int -virCHMonitorGet(virCHMonitor *mon, const char *endpoint) +virCHMonitorGet(virCHMonitor *mon, const char *endpoint, virJSONValue **response) { g_autofree char *url = NULL; int responseCode = 0; int ret = -1; + struct curl_slist *headers = NULL; + struct curl_data data = {0}; url = g_strdup_printf("%s/%s", URL_ROOT, endpoint); @@ -628,12 +652,30 @@ virCHMonitorGet(virCHMonitor *mon, const char *endpoint) curl_easy_setopt(mon->handle, CURLOPT_UNIX_SOCKET_PATH, mon->socketpath); curl_easy_setopt(mon->handle, CURLOPT_URL, url); + if (response) { + headers = curl_slist_append(headers, "Accept: application/json"); + headers = curl_slist_append(headers, "Content-Type: application/json"); + curl_easy_setopt(mon->handle, CURLOPT_HTTPHEADER, headers); + curl_easy_setopt(mon->handle, CURLOPT_WRITEFUNCTION, curl_callback); + curl_easy_setopt(mon->handle, CURLOPT_WRITEDATA, (void *)&data); + } + responseCode = virCHMonitorCurlPerform(mon->handle); virObjectUnlock(mon); - if (responseCode == 200 || responseCode == 204) + if (responseCode == 200 || responseCode == 204) { ret = 0; + if (response) { + data.content = g_realloc(data.content, data.size + 1); + data.content[data.size] = 0; + *response = virJSONValueFromString(data.content);

Oh, well need to add: if (!*response) return -1; in case JSON parsing fails

...

+ } + } + + g_free(data.content); + /* reset the libcurl handle to avoid leaking a stack pointer to data */ + curl_easy_reset(mon->handle); return ret; }

Daniel P. Berrangé

6:50 p.m.

New subject: [PATCH 3/9] ch_monitor: Update virCHMonitorGet to handle accept a response

On Thu, Sep 09, 2021 at 09:48:48AM +0100, Daniel P. Berrangé wrote:

...

On Wed, Sep 08, 2021 at 11:01:17AM -0700, William Douglas wrote: > The virCHMonitorGet function needed to be able to return data from the > hypervisor. This functionality is needed in order for the driver to > support PTY enablement and getting details about the VM state. > > Signed-off-by: William Douglas <william.douglas(a)intel.com> > --- > src/ch/ch_monitor.c | 46 +++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 44 insertions(+), 2 deletions(-) > > diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c > index b4bc10bfcf..44b99ef07a 100644 > --- a/src/ch/ch_monitor.c > +++ b/src/ch/ch_monitor.c > @@ -611,12 +611,36 @@ virCHMonitorPutNoContent(virCHMonitor *mon, const char *endpoint) > return ret; > } > > +struct curl_data { > + char *content; > + size_t size; > +}; > + > +static size_t > +curl_callback(void *contents, size_t size, size_t nmemb, void *userp) > +{ > + size_t content_size = size * nmemb; > + struct curl_data *data = (struct curl_data *)userp; FWIW, the type cast here is redundant as 'void *' can be directly assigned to/from any other type. > + > + if (content_size == 0) > + return content_size; > + > + data->content = g_realloc(data->content, data->size + content_size); > + > + memcpy(&(data->content[data->size]), contents, content_size); > + data->size += content_size; > + > + return content_size; > +} > + > static int > -virCHMonitorGet(virCHMonitor *mon, const char *endpoint) > +virCHMonitorGet(virCHMonitor *mon, const char *endpoint, virJSONValue **response) > { > g_autofree char *url = NULL; > int responseCode = 0; > int ret = -1; > + struct curl_slist *headers = NULL; > + struct curl_data data = {0}; > > url = g_strdup_printf("%s/%s", URL_ROOT, endpoint); > > @@ -628,12 +652,30 @@ virCHMonitorGet(virCHMonitor *mon, const char *endpoint) > curl_easy_setopt(mon->handle, CURLOPT_UNIX_SOCKET_PATH, mon->socketpath); > curl_easy_setopt(mon->handle, CURLOPT_URL, url); > > + if (response) { > + headers = curl_slist_append(headers, "Accept: application/json"); > + headers = curl_slist_append(headers, "Content-Type: application/json"); > + curl_easy_setopt(mon->handle, CURLOPT_HTTPHEADER, headers); > + curl_easy_setopt(mon->handle, CURLOPT_WRITEFUNCTION, curl_callback); > + curl_easy_setopt(mon->handle, CURLOPT_WRITEDATA, (void *)&data); > + } > + > responseCode = virCHMonitorCurlPerform(mon->handle); > > virObjectUnlock(mon); > > - if (responseCode == 200 || responseCode == 204) > + if (responseCode == 200 || responseCode == 204) { > ret = 0; > + if (response) { > + data.content = g_realloc(data.content, data.size + 1); > + data.content[data.size] = 0; > + *response = virJSONValueFromString(data.content); Oh, well need to add: if (!*response) return -1; in case JSON parsing fails

Actually slight more to avoid a leak. @@ -665,14 +665,17 @@ virCHMonitorGet(virCHMonitor *mon, const char *endpoint, virJSONValue **response virObjectUnlock(mon); if (responseCode == 200 || responseCode == 204) { - ret = 0; if (response) { data.content = g_realloc(data.content, data.size + 1); data.content[data.size] = 0; *response = virJSONValueFromString(data.content); + if (!*response) + goto cleanup; } + ret = 0; } + cleanup: g_free(data.content); /* reset the libcurl handle to avoid leaking a stack pointer to data */ curl_easy_reset(mon->handle); Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

William Douglas

2:01 a.m.

New subject: [PATCH 4/9] ch_monitor: Use virCHMonitorGet to access cloud-hypervisor API

Now that virCHMonitorGet is capable of handling data returned by the cloud-hypervisor API, make use of this via virCHMonitorGetInfo to call into the vm.info endpoint. Signed-off-by: William Douglas <william.douglas(a)intel.com> --- src/ch/ch_monitor.c | 15 +++++++++++++++ src/ch/ch_monitor.h | 3 +++ 2 files changed, 18 insertions(+) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 44b99ef07a..28c70c7281 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -753,3 +753,18 @@ virCHMonitorResumeVM(virCHMonitor *mon) { return virCHMonitorPutNoContent(mon, URL_VM_RESUME); } + +/** + * virCHMonitorGetInfo: + * @mon: Pointer to the monitor + * @info: Get VM info + * + * Retrive the VM info and store in @info + * + * Returns 0 on success. + */ +int +virCHMonitorGetInfo(virCHMonitor *mon, virJSONValue **info) +{ + return virCHMonitorGet(mon, URL_VM_INFO, info); +} diff --git a/src/ch/ch_monitor.h b/src/ch/ch_monitor.h index e717e11cbc..e39b4eb8b2 100644 --- a/src/ch/ch_monitor.h +++ b/src/ch/ch_monitor.h @@ -23,6 +23,7 @@ #include <curl/curl.h> #include "virobject.h" +#include "virjson.h" #include "domain_conf.h" #define URL_ROOT "http://localhost/api/v1" @@ -34,6 +35,7 @@ #define URL_VM_REBOOT "vm.reboot" #define URL_VM_Suspend "vm.pause" #define URL_VM_RESUME "vm.resume" +#define URL_VM_INFO "vm.info" typedef struct _virCHMonitor virCHMonitor; @@ -58,3 +60,4 @@ int virCHMonitorShutdownVM(virCHMonitor *mon); int virCHMonitorRebootVM(virCHMonitor *mon); int virCHMonitorSuspendVM(virCHMonitor *mon); int virCHMonitorResumeVM(virCHMonitor *mon); +int virCHMonitorGetInfo(virCHMonitor *mon, virJSONValue **info); -- 2.31.1

William Douglas

2:01 a.m.

New subject: [PATCH 5/9] ch_monitor: Add pty json builder function

Add function to build the the json structure to configure a PTY in cloud-hypervisor. The devices themselves still aren't allowed in configurations yet though. Signed-off-by: William Douglas <william.douglas(a)intel.com> --- src/ch/ch_monitor.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 28c70c7281..a01c031064 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -89,6 +89,30 @@ virCHMonitorBuildCPUJson(virJSONValue *content, virDomainDef *vmdef) return -1; } +static int +virCHMonitorBuildPTYJson(virJSONValue *content, virDomainDef *vmdef) +{ + virJSONValue *ptys = virJSONValueNewObject(); + + if (vmdef->nconsoles) { + g_autoptr(virJSONValue) pty = virJSONValueNewObject(); + if (virJSONValueObjectAppendString(pty, "mode", "Pty") < 0) + return -1; + if (virJSONValueObjectAppend(content, "console", &pty) < 0) + return -1; + } + + if (vmdef->nserials) { + g_autoptr(virJSONValue) pty = virJSONValueNewObject(); + if (virJSONValueObjectAppendString(ptys, "mode", "Pty") < 0) + return -1; + if (virJSONValueObjectAppend(content, "serial", &pty) < 0) + return -1; + } + + return 0; +} + static int virCHMonitorBuildKernelRelatedJson(virJSONValue *content, virDomainDef *vmdef) { @@ -370,6 +394,9 @@ virCHMonitorBuildVMJson(virDomainDef *vmdef, char **jsonstr) goto cleanup; } + if (virCHMonitorBuildPTYJson(content, vmdef) < 0) + goto cleanup; + if (virCHMonitorBuildCPUJson(content, vmdef) < 0) goto cleanup; -- 2.31.1

William Douglas

2:01 a.m.

New subject: [PATCH 6/9] ch_process: Handle enabled console devices

Add functionality to allow libvirt console to connect to the cloud-hypervisor created PTY associated with a VM by updating the domain with console path information. This has to be run after the VM is created by cloud-hypervisor. Signed-off-by: William Douglas <william.douglas(a)intel.com> --- src/ch/ch_process.c | 84 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c index 93b1f7f97e..1a01ca9384 100644 --- a/src/ch/ch_process.c +++ b/src/ch/ch_process.c @@ -28,6 +28,7 @@ #include "ch_process.h" #include "viralloc.h" #include "virerror.h" +#include "virjson.h" #include "virlog.h" #define VIR_FROM_THIS VIR_FROM_CH @@ -52,6 +53,86 @@ virCHProcessConnectMonitor(virCHDriver *driver, return monitor; } +static void +virCHProcessUpdateConsoleDevice(virDomainObj *vm, + virJSONValue *config, + const char *device) +{ + const char *path; + virDomainChrDef *chr = NULL; + virJSONValue *dev, *file; + + if (!config) + return; + + dev = virJSONValueObjectGet(config, device); + if (!dev) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing '%s' in 'config' from cloud-hypervisor"), + device); + return; + } + + file = virJSONValueObjectGet(dev, "file"); + if (!file) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing 'file' in '%s' from cloud-hypervisor"), + device); + return; + } + + path = virJSONValueGetString(file); + if (!path) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unable to parse contents of 'file' field in '%s' from cloud-hypervisor"), + device); + return; + } + + if (STREQ(device, "console")) { + chr = vm->def->consoles[0]; + } else if (STREQ(device, "serial")) { + chr = vm->def->serials[0]; + } + + if (chr && chr->source) + chr->source->data.file.path = g_strdup(path); +} + +static void +virCHProcessUpdateConsole(virDomainObj *vm, + virJSONValue *info) +{ + virJSONValue *config; + + config = virJSONValueObjectGet(info, "config"); + if (!config) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing 'config' in info query result from cloud-hypervisor")); + return; + } + + if (vm->def->nconsoles > 0) + virCHProcessUpdateConsoleDevice(vm, config, "console"); + if (vm->def->nserials > 0) + virCHProcessUpdateConsoleDevice(vm, config, "serial"); +} + +static int +virCHProcessUpdateInfo(virDomainObj *vm) +{ + virJSONValue *info; + virCHDomainObjPrivate *priv = vm->privateData; + if (virCHMonitorGetInfo(priv->monitor, &info) < 0) + return -1; + + virCHProcessUpdateConsole(vm, info); + + virJSONValueFree(info); + + return 0; +} + /** * virCHProcessStart: * @driver: pointer to driver structure @@ -92,6 +173,9 @@ int virCHProcessStart(virCHDriver *driver, vm->pid = priv->monitor->pid; vm->def->id = vm->pid; + + virCHProcessUpdateInfo(vm); + virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, reason); return 0; -- 2.31.1

William Douglas

2:01 a.m.

New subject: [PATCH 7/9] ch_domain: Allow controller and chr devices

With the console and serial device handling fully functional, allow the required device types to be specified in the domain configuration. The configuration only supports a single serial or console device. Signed-off-by: William Douglas <william.douglas(a)intel.com> --- src/ch/ch_domain.c | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/src/ch/ch_domain.c b/src/ch/ch_domain.c index a6b87e28e5..ed01f637b1 100644 --- a/src/ch/ch_domain.c +++ b/src/ch/ch_domain.c @@ -215,6 +215,8 @@ chValidateDomainDeviceDef(const virDomainDeviceDef *dev, case VIR_DOMAIN_DEVICE_NET: case VIR_DOMAIN_DEVICE_MEMORY: case VIR_DOMAIN_DEVICE_VSOCK: + case VIR_DOMAIN_DEVICE_CONTROLLER: + case VIR_DOMAIN_DEVICE_CHR: break; case VIR_DOMAIN_DEVICE_LEASE: @@ -224,12 +226,10 @@ chValidateDomainDeviceDef(const virDomainDeviceDef *dev, case VIR_DOMAIN_DEVICE_VIDEO: case VIR_DOMAIN_DEVICE_HOSTDEV: case VIR_DOMAIN_DEVICE_WATCHDOG: - case VIR_DOMAIN_DEVICE_CONTROLLER: case VIR_DOMAIN_DEVICE_GRAPHICS: case VIR_DOMAIN_DEVICE_HUB: case VIR_DOMAIN_DEVICE_REDIRDEV: case VIR_DOMAIN_DEVICE_SMARTCARD: - case VIR_DOMAIN_DEVICE_CHR: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: case VIR_DOMAIN_DEVICE_RNG: @@ -254,6 +254,35 @@ chValidateDomainDeviceDef(const virDomainDeviceDef *dev, return -1; } + if ((def->nconsoles && + def->consoles[0]->source->type == VIR_DOMAIN_CHR_TYPE_PTY) + && (def->nserials && + def->serials[0]->source->type == VIR_DOMAIN_CHR_TYPE_PTY)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Only a single console or serial can be configured for this domain")); + return -1; + } else if (def->nconsoles > 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Only a single console can be configured for this domain")); + return -1; + } else if (def->nserials > 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Only a single serial can be configured for this domain")); + return -1; + } + + if (def->nconsoles && def->consoles[0]->source->type != VIR_DOMAIN_CHR_TYPE_PTY) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Console can only be enabled for a PTY")); + return -1; + } + + if (def->nserials && def->serials[0]->source->type != VIR_DOMAIN_CHR_TYPE_PTY) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Serial can only be enabled for a PTY")); + return -1; + } + return 0; } -- 2.31.1

William Douglas

2:01 a.m.

New subject: [PATCH 8/9] ch_driver: Add handler for console API

Enable the handler function to find and open the console character device that will be used by the console API. Signed-off-by: William Douglas <william.douglas(a)intel.com> --- src/ch/ch_driver.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index ac958d73a8..c821459fc5 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -30,6 +30,7 @@ #include "viraccessapicheck.h" #include "viralloc.h" #include "virbuffer.h" +#include "virchrdev.h" #include "vircommand.h" #include "virerror.h" #include "virfile.h" @@ -816,6 +817,78 @@ static int chDomainGetInfo(virDomainPtr dom, return ret; } +static int +chDomainOpenConsole(virDomainPtr dom, + const char *dev_name, + virStreamPtr st, + unsigned int flags) +{ + virDomainObj *vm = NULL; + int ret = -1; + size_t i; + virDomainChrDef *chr = NULL; + virCHDomainObjPrivate *priv; + + virCheckFlags(VIR_DOMAIN_CONSOLE_SAFE | VIR_DOMAIN_CONSOLE_FORCE, -1); + + if (!(vm = chDomObjFromDomain(dom))) + goto cleanup; + + if (virDomainOpenConsoleEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (virDomainObjCheckActive(vm) < 0) + goto cleanup; + + priv = vm->privateData; + + if (dev_name) { + for (i = 0; !chr && i < vm->def->nconsoles; i++) { + if (vm->def->consoles[i]->info.alias && + STREQ(dev_name, vm->def->consoles[i]->info.alias)) + chr = vm->def->consoles[i]; + } + for (i = 0; !chr && i < vm->def->nserials; i++) { + if (STREQ(dev_name, vm->def->serials[i]->info.alias)) + chr = vm->def->serials[i]; + } + } else { + if (vm->def->nconsoles && + vm->def->consoles[0]->source->type == VIR_DOMAIN_CHR_TYPE_PTY) + chr = vm->def->consoles[0]; + else if (vm->def->nserials && + vm->def->serials[0]->source->type == VIR_DOMAIN_CHR_TYPE_PTY) + chr = vm->def->serials[0]; + } + + if (!chr) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot find character device %s"), + NULLSTR(dev_name)); + goto cleanup; + } + + if (chr->source->type != VIR_DOMAIN_CHR_TYPE_PTY) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("character device %s is not using a PTY"), + dev_name ? dev_name : NULLSTR(chr->info.alias)); + goto cleanup; + } + + /* handle mutually exclusive access to console devices */ + ret = virChrdevOpen(priv->chrdevs, chr->source, st, + (flags & VIR_DOMAIN_CONSOLE_FORCE) != 0); + + if (ret == 1) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("Active console session exists for this domain")); + ret = -1; + } + + cleanup: + virDomainObjEndAPI(&vm); + return ret; +} + static int chStateCleanup(void) { if (ch_driver == NULL) @@ -913,6 +986,7 @@ static virHypervisorDriver chHypervisorDriver = { .domainGetXMLDesc = chDomainGetXMLDesc, /* 7.5.0 */ .domainGetInfo = chDomainGetInfo, /* 7.5.0 */ .domainIsActive = chDomainIsActive, /* 7.5.0 */ + .domainOpenConsole = chDomainOpenConsole, /* 7.8.0 */ .nodeGetInfo = chNodeGetInfo, /* 7.5.0 */ }; -- 2.31.1

William Douglas

2:01 a.m.

New subject: [PATCH 9/9] ch_driver: Handle validation failure correctly

When validation like deviceValidateCallback fails, the vm will not be set and so the call to virDomainObjListRemove will be passed a NULL pointer causing a segfault. To prevent this add a check that the vm is defined before calling out to virDomainObjListRemove. Signed-off-by: William Douglas <william.douglas(a)intel.com> --- src/ch/ch_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index c821459fc5..1824d2fd16 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -262,7 +262,7 @@ chDomainCreateXML(virConnectPtr conn, virCHDomainObjEndJob(vm); cleanup: - if (!dom) { + if (vm && !dom) { virDomainObjListRemove(driver->domains, vm); } virDomainDefFree(vmdef); -- 2.31.1

Daniel P. Berrangé

6:51 p.m.

On Wed, Sep 08, 2021 at 11:01:14AM -0700, William Douglas wrote:

...

The comments I've had are fairly simple, and I made them locally when testing and reviewing the code. So I'll just push your series directly with the suggested changes, to avoid need to repost a v5 Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

1151

days inactive

1152

days old

devel@lists.libvirt.org

Manage subscription

14 comments

2 participants

tags (0)

participants (2)

Daniel P. Berrangé
William Douglas

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

[PATCHv4 0/9] ch: Add Console support