[PATCH 0/3] Console Fixes and Enhancements
Enabling cimtest for the Console RASDs required a few improvements. The XML generation code was simplified, a logical device class for consoles had to be added and a bug in the KVM redirection service was fixed. Viktor Mihajlovski (3): libxkutil: Simplify XML handling of consoles Virt_Device: Add a device class for consoles KVMRedirectionSAP: Only return redirection SAPs for VNC graphics libxkutil/device_parsing.c | 12 +----- libxkutil/xmlgen.c | 76 --------------------------------- schema/DisplayController.mof | 15 +++++++ schema/DisplayController.registration | 3 ++ src/Virt_Device.c | 18 ++++---- src/Virt_ElementSettingData.c | 3 ++ src/Virt_KVMRedirectionSAP.c | 23 ++++++---- src/Virt_SettingsDefineState.c | 6 +++ src/Virt_SystemDevice.c | 3 ++ src/Virt_VSSDComponent.c | 3 ++ 10 files changed, 59 insertions(+), 103 deletions(-) -- 1.7.9.5
The attempt to avoid duplication of console definitions was a bit too complicated and error prone. We move the generation of console XML entirely to the new console code. Further, it's incorrect to restrict PTY consoles being represented as GraphicsRASDs to virtio only. At least serial must be allowed, but it doesn't really hurt to map all PTYs to GraphicsRASD consoles. Further, the parsing code returned two instances for serial consoles, one from the /domain/devices/console and /domain/devices/serial both representing the same device. This confuses cimtest and, more problematic, will prevent DefineSystem to work, if a reference VSSD with a serial console is passed in. Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> Reviewed-by: Daniel Hansel <daniel.hansel@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 12 +------ libxkutil/xmlgen.c | 76 -------------------------------------------- 2 files changed, 1 insertion(+), 87 deletions(-) diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index 55c8d57..aecca4c 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -47,7 +47,7 @@ * still part of the graphics. */ #define GRAPHICS_XPATH (xmlChar *)"/domain/devices/graphics | "\ - "/domain/devices/console | /domain/devices/serial" + "/domain/devices/console" #define INPUT_XPATH (xmlChar *)"/domain/devices/input" #define DEFAULT_BRIDGE "xenbr0" @@ -948,16 +948,6 @@ static int parse_graphics_device(xmlNode *node, struct virt_device **vdevs) else if (XSTREQ(child->name, "target")) { gdev->dev.vnc.port = get_attr_value(child, "port"); - /* The graphics pty console can only be a - virtio console. If 'type' is not set in the - xml, the default of libvirt is virtio.*/ - char *t_type = get_attr_value(child, "type"); - if (t_type != NULL && !STREQC(t_type, "virtio")) { - CU_DEBUG("Not a pty-virtio graphics console"); - free(t_type); - goto err; - } - free(t_type); } } } diff --git a/libxkutil/xmlgen.c b/libxkutil/xmlgen.c index 45bfb04..7e8801d 100644 --- a/libxkutil/xmlgen.c +++ b/libxkutil/xmlgen.c @@ -42,36 +42,11 @@ typedef const char *(*devfn_t)(xmlNodePtr node, struct domain *dominfo); typedef const char *(*poolfn_t)(xmlNodePtr node, struct virt_pool *pool); typedef const char *(*resfn_t)(xmlNodePtr node, struct virt_pool_res *res); -static int _count_graphics_console_definitions(struct domain *dominfo) -{ - int i; - int num = 0; - - for (i = 0; i < dominfo->dev_graphics_ct; i++) { - struct virt_device *_dev = &dominfo->dev_graphics[i]; - if (_dev->type == CIM_RES_TYPE_UNKNOWN) - continue; - - struct graphics_device *dev = &_dev->dev.graphics; - - if (STREQC(dev->type, "console")) { - num++; - } - } - CU_DEBUG("Found %d console defintions in graphics devices.",num); - return num; - -} - static const char *console_xml(xmlNodePtr root, struct domain *dominfo) { int i; xmlNodePtr console; xmlNodePtr tmp; - int num_graphics_consol_def = 0; - int num_suppressed_console_def = 0; - - num_graphics_consol_def = _count_graphics_console_definitions(dominfo); for (i = 0; i < dominfo->dev_console_ct; i++) { struct virt_device *_dev = &dominfo->dev_console[i]; @@ -80,25 +55,6 @@ static const char *console_xml(xmlNodePtr root, struct domain *dominfo) struct console_device *cdev = &_dev->dev.console; - /* Due to backward compatibility, the graphics device handling - is still parsing consoles: - source = pty, target = virtio (which is the default target) - But the console device handling processes these kind of - consoles too. This would lead to a duplication of these - default consoles in the domain xml definition. - This code prevents the console handling of writing xml for - duplicate pty/virtio consoles which are written by the - graphics device handling. */ - if (cdev->source_type == CIM_CHARDEV_SOURCE_TYPE_PTY && - (cdev->target_type == NULL || - STREQC(cdev->target_type, "virtio"))) { - if (num_suppressed_console_def < - num_graphics_consol_def) { - num_suppressed_console_def++; - continue; - } - } - console = xmlNewChild(root, NULL, BAD_CAST "console", NULL); if (console == NULL) return XML_ERROR; @@ -760,35 +716,6 @@ static const char *graphics_vnc_xml(xmlNodePtr root, return NULL; } -static const char *graphics_pty_xml(xmlNodePtr root, - struct graphics_device *dev) -{ - xmlNodePtr pty = NULL; - xmlNodePtr tmp = NULL; - - pty = xmlNewChild(root, NULL, BAD_CAST dev->type, NULL); - if (pty == NULL) - return XML_ERROR; - - xmlNewProp(pty, BAD_CAST "type", BAD_CAST "pty"); - - tmp = xmlNewChild(pty, NULL, BAD_CAST "source", NULL); - if (tmp == NULL) - return XML_ERROR; - - if(dev->dev.vnc.host) - xmlNewProp(tmp, BAD_CAST "path", BAD_CAST dev->dev.vnc.host); - - tmp = xmlNewChild(pty, NULL, BAD_CAST "target", NULL); - if (tmp == NULL) - return XML_ERROR; - - if(dev->dev.vnc.port) - xmlNewProp(tmp, BAD_CAST "port", BAD_CAST dev->dev.vnc.port); - - return NULL; -} - static const char *graphics_xml(xmlNodePtr root, struct domain *dominfo) { const char *msg = NULL; @@ -803,9 +730,6 @@ static const char *graphics_xml(xmlNodePtr root, struct domain *dominfo) if (STREQC(dev->type, "vnc") || STREQC(dev->type, "sdl")) msg = graphics_vnc_xml(root, dev); - else if (STREQC(dev->type, "console") || - STREQC(dev->type, "serial")) - msg = graphics_pty_xml(root, dev); else continue; -- 1.7.9.5
libvirt-cim has a very strict assumption about the presence of a CIM_LogicalDevice class being associated to a RASD. It is practically impossible to extend the cimtest framework for the ConsoleRASD class without having a matching device class. Adding a new ConsoleDisplayController class for this purpose. Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> Reviewed-by: Daniel Hansel <daniel.hansel@linux.vnet.ibm.com> --- schema/DisplayController.mof | 15 +++++++++++++++ schema/DisplayController.registration | 3 +++ src/Virt_Device.c | 18 ++++++++++-------- src/Virt_ElementSettingData.c | 3 +++ src/Virt_SettingsDefineState.c | 6 ++++++ src/Virt_SystemDevice.c | 3 +++ src/Virt_VSSDComponent.c | 3 +++ 7 files changed, 43 insertions(+), 8 deletions(-) diff --git a/schema/DisplayController.mof b/schema/DisplayController.mof index b57c9cc..8ccdc07 100644 --- a/schema/DisplayController.mof +++ b/schema/DisplayController.mof @@ -15,3 +15,18 @@ class LXC_DisplayController : CIM_DisplayController { }; +[ Provider("cmpi::Virt_Device") ] +class Xen_ConsoleDisplayController : CIM_DisplayController +{ +}; + +[ Provider("cmpi::Virt_Device") ] +class KVM_ConsoleDisplayController : CIM_DisplayController +{ +}; + +[ Provider("cmpi::Virt_Device") ] +class LXC_ConsoleDisplayController : CIM_DisplayController +{ +}; + diff --git a/schema/DisplayController.registration b/schema/DisplayController.registration index de8adc9..e7e9770 100644 --- a/schema/DisplayController.registration +++ b/schema/DisplayController.registration @@ -3,3 +3,6 @@ Xen_DisplayController root/virt Virt_Device Virt_Device instance KVM_DisplayController root/virt Virt_Device Virt_Device instance LXC_DisplayController root/virt Virt_Device Virt_Device instance +Xen_ConsoleDisplayController root/virt Virt_Device Virt_Device instance +KVM_ConsoleDisplayController root/virt Virt_Device Virt_Device instance +LXC_ConsoleDisplayController root/virt Virt_Device Virt_Device instance diff --git a/src/Virt_Device.c b/src/Virt_Device.c index aa47276..498ce2c 100644 --- a/src/Virt_Device.c +++ b/src/Virt_Device.c @@ -263,12 +263,12 @@ static CMPIInstance *console_instance(const CMPIBroker *broker, conn = virDomainGetConnect(dom); inst = get_typed_instance(broker, pfx_from_conn(conn), - "DisplayController", + "ConsoleDisplayController", ns, true); if (inst == NULL) { - CU_DEBUG("Failed to get instance for DisplayController"); + CU_DEBUG("Failed to get instance for ConsoleDisplayController"); return NULL; } @@ -541,17 +541,19 @@ static bool device_instances(const CMPIBroker *broker, uint16_t res_type_from_device_classname(const char *classname) { - if (strstr(classname, "NetworkPort")) + if (strstr(classname, "_NetworkPort")) return CIM_RES_TYPE_NET; - else if (strstr(classname, "LogicalDisk")) + else if (strstr(classname, "_LogicalDisk")) return CIM_RES_TYPE_DISK; - else if (strstr(classname, "Memory")) + else if (strstr(classname, "_Memory")) return CIM_RES_TYPE_MEM; - else if (strstr(classname, "Processor")) + else if (strstr(classname, "_Processor")) return CIM_RES_TYPE_PROC; - else if (strstr(classname, "DisplayController")) + else if (strstr(classname, "_DisplayController")) return CIM_RES_TYPE_GRAPHICS; - else if (strstr(classname, "PointingDevice")) + else if (strstr(classname, "_ConsoleDisplayController")) + return CIM_RES_TYPE_CONSOLE; + else if (strstr(classname, "_PointingDevice")) return CIM_RES_TYPE_INPUT; else return CIM_RES_TYPE_UNKNOWN; diff --git a/src/Virt_ElementSettingData.c b/src/Virt_ElementSettingData.c index c257710..c088e49 100644 --- a/src/Virt_ElementSettingData.c +++ b/src/Virt_ElementSettingData.c @@ -128,18 +128,21 @@ static char* resource_allocation_setting_data[] = { "Xen_NetResourceAllocationSettingData", "Xen_ProcResourceAllocationSettingData", "Xen_GraphicsResourceAllocationSettingData", + "Xen_ConsoleResourceAllocationSettingData", "Xen_InputResourceAllocationSettingData", "KVM_DiskResourceAllocationSettingData", "KVM_MemResourceAllocationSettingData", "KVM_NetResourceAllocationSettingData", "KVM_ProcResourceAllocationSettingData", "KVM_GraphicsResourceAllocationSettingData", + "KVM_ConsoleResourceAllocationSettingData", "KVM_InputResourceAllocationSettingData", "LXC_DiskResourceAllocationSettingData", "LXC_MemResourceAllocationSettingData", "LXC_NetResourceAllocationSettingData", "LXC_ProcResourceAllocationSettingData", "LXC_GraphicsResourceAllocationSettingData", + "LXC_ConsoleResourceAllocationSettingData", "LXC_InputResourceAllocationSettingData", NULL }; diff --git a/src/Virt_SettingsDefineState.c b/src/Virt_SettingsDefineState.c index f30f45f..be2ded5 100644 --- a/src/Virt_SettingsDefineState.c +++ b/src/Virt_SettingsDefineState.c @@ -327,18 +327,21 @@ static char* logical_device[] = { "Xen_NetworkPort", "Xen_LogicalDisk", "Xen_DisplayController", + "Xen_ConsoleDisplayController", "Xen_PointingDevice", "KVM_Processor", "KVM_Memory", "KVM_NetworkPort", "KVM_LogicalDisk", "KVM_DisplayController", + "KVM_ConsoleDisplayController", "KVM_PointingDevice", "LXC_Processor", "LXC_Memory", "LXC_NetworkPort", "LXC_LogicalDisk", "LXC_DisplayController", + "LXC_ConsoleDisplayController", "LXC_PointingDevice", NULL }; @@ -350,18 +353,21 @@ static char* resource_allocation_setting_data[] = { "Xen_ProcResourceAllocationSettingData", "Xen_GraphicsResourceAllocationSettingData", "Xen_InputResourceAllocationSettingData", + "Xen_ConsoleResourceAllocationSettingData", "KVM_DiskResourceAllocationSettingData", "KVM_MemResourceAllocationSettingData", "KVM_NetResourceAllocationSettingData", "KVM_ProcResourceAllocationSettingData", "KVM_GraphicsResourceAllocationSettingData", "KVM_InputResourceAllocationSettingData", + "KVM_ConsoleResourceAllocationSettingData", "LXC_DiskResourceAllocationSettingData", "LXC_MemResourceAllocationSettingData", "LXC_NetResourceAllocationSettingData", "LXC_ProcResourceAllocationSettingData", "LXC_GraphicsResourceAllocationSettingData", "LXC_InputResourceAllocationSettingData", + "LXC_ConsoleResourceAllocationSettingData", NULL }; diff --git a/src/Virt_SystemDevice.c b/src/Virt_SystemDevice.c index 3a2f7ce..d2e526d 100644 --- a/src/Virt_SystemDevice.c +++ b/src/Virt_SystemDevice.c @@ -135,18 +135,21 @@ static char* part_component[] = { "Xen_NetworkPort", "Xen_LogicalDisk", "Xen_DisplayController", + "Xen_ConsoleDisplayController", "Xen_PointingDevice", "KVM_Processor", "KVM_Memory", "KVM_NetworkPort", "KVM_LogicalDisk", "KVM_DisplayController", + "KVM_ConsoleDisplayController", "KVM_PointingDevice", "LXC_Processor", "LXC_Memory", "LXC_NetworkPort", "LXC_LogicalDisk", "LXC_DisplayController", + "LXC_ConsoleDisplayController", "LXC_PointingDevice", NULL }; diff --git a/src/Virt_VSSDComponent.c b/src/Virt_VSSDComponent.c index 378de96..35bffde 100644 --- a/src/Virt_VSSDComponent.c +++ b/src/Virt_VSSDComponent.c @@ -132,6 +132,7 @@ static char* part_component[] = { "Xen_NetResourceAllocationSettingData", "Xen_ProcResourceAllocationSettingData", "Xen_GraphicsResourceAllocationSettingData", + "Xen_ConsoleResourceAllocationSettingData", "Xen_InputResourceAllocationSettingData", "KVM_DiskResourceAllocationSettingData", "KVM_MemResourceAllocationSettingData", @@ -139,12 +140,14 @@ static char* part_component[] = { "KVM_ProcResourceAllocationSettingData", "KVM_GraphicsResourceAllocationSettingData", "KVM_InputResourceAllocationSettingData", + "KVM_ConsoleResourceAllocationSettingData", "LXC_DiskResourceAllocationSettingData", "LXC_MemResourceAllocationSettingData", "LXC_NetResourceAllocationSettingData", "LXC_ProcResourceAllocationSettingData", "LXC_GraphicsResourceAllocationSettingData", "LXC_InputResourceAllocationSettingData", + "LXC_ConsoleResourceAllocationSettingData", NULL }; -- 1.7.9.5
Since pty consoles still show up as GraphicsRASDs the check for VNC redirection proved to be too rigid. Instead of failing the associator call, PTY consoles will just be ignored now. This is an old bug, but will be exposed when cimtest is extended to handle consoles. Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> Reviewed-by: Daniel Hansel <daniel.hansel@linux.vnet.ibm.com> --- src/Virt_KVMRedirectionSAP.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/src/Virt_KVMRedirectionSAP.c b/src/Virt_KVMRedirectionSAP.c index 708b0d1..1a2aa69 100644 --- a/src/Virt_KVMRedirectionSAP.c +++ b/src/Virt_KVMRedirectionSAP.c @@ -265,9 +265,11 @@ static CMPIStatus get_vnc_sessions(const CMPIBroker *broker, } static bool check_graphics(virDomainPtr dom, - struct domain **dominfo) + struct domain **dominfo, + int *pos) { int ret = 0; + int i; ret = get_dominfo(dom, dominfo); if (!ret) { @@ -280,12 +282,16 @@ static bool check_graphics(virDomainPtr dom, return false; } - if (!STREQC((*dominfo)->dev_graphics->dev.graphics.type, "vnc")) { - CU_DEBUG("Only vnc devices have console redirection sessions"); - return false; + for (i = 0; i < (*dominfo)->dev_graphics_ct; i++) { + if (STREQC((*dominfo)->dev_graphics[i].dev.graphics.type, "vnc")) { + if (pos) + *pos = i; + return true; + } } - return true; + CU_DEBUG("Only vnc devices have console redirection sessions"); + return false; } static CMPIStatus return_console_sap(const CMPIObjectPath *ref, @@ -362,12 +368,13 @@ CMPIStatus enum_console_sap(const CMPIBroker *broker, } for (i = 0; i < count; i++) { - if (!check_graphics(domain_list[i], &dominfo)) { + int pos; + if (!check_graphics(domain_list[i], &dominfo, &pos)) { cleanup_dominfo(&dominfo); continue; } - ret = sscanf(dominfo->dev_graphics->dev.graphics.dev.vnc.port, + ret = sscanf(dominfo->dev_graphics[pos].dev.graphics.dev.vnc.port, "%d", &lport); if (ret != 1) { @@ -449,7 +456,7 @@ CMPIStatus get_console_sap_by_name(const CMPIBroker *broker, goto out; } - if (!check_graphics(dom, &dominfo)) { + if (!check_graphics(dom, &dominfo, NULL)) { virt_set_status(broker, &s, CMPI_RC_ERR_FAILED, conn, -- 1.7.9.5
On 09/20/2013 05:25 PM, Viktor Mihajlovski wrote:
Enabling cimtest for the Console RASDs required a few improvements. The XML generation code was simplified, a logical device class for consoles had to be added and a bug in the KVM redirection service was fixed.
Viktor Mihajlovski (3): libxkutil: Simplify XML handling of consoles Virt_Device: Add a device class for consoles KVMRedirectionSAP: Only return redirection SAPs for VNC graphics
libxkutil/device_parsing.c | 12 +----- libxkutil/xmlgen.c | 76 --------------------------------- schema/DisplayController.mof | 15 +++++++ schema/DisplayController.registration | 3 ++ src/Virt_Device.c | 18 ++++---- src/Virt_ElementSettingData.c | 3 ++ src/Virt_KVMRedirectionSAP.c | 23 ++++++---- src/Virt_SettingsDefineState.c | 6 +++ src/Virt_SystemDevice.c | 3 ++ src/Virt_VSSDComponent.c | 3 ++ 10 files changed, 59 insertions(+), 103 deletions(-)
Ping? -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 10/02/2013 05:16 AM, Viktor Mihajlovski wrote:
On 09/20/2013 05:25 PM, Viktor Mihajlovski wrote:
Enabling cimtest for the Console RASDs required a few improvements. The XML generation code was simplified, a logical device class for consoles had to be added and a bug in the KVM redirection service was fixed.
Viktor Mihajlovski (3): libxkutil: Simplify XML handling of consoles Virt_Device: Add a device class for consoles KVMRedirectionSAP: Only return redirection SAPs for VNC graphics
libxkutil/device_parsing.c | 12 +----- libxkutil/xmlgen.c | 76 --------------------------------- schema/DisplayController.mof | 15 +++++++ schema/DisplayController.registration | 3 ++ src/Virt_Device.c | 18 ++++---- src/Virt_ElementSettingData.c | 3 ++ src/Virt_KVMRedirectionSAP.c | 23 ++++++---- src/Virt_SettingsDefineState.c | 6 +++ src/Virt_SystemDevice.c | 3 ++ src/Virt_VSSDComponent.c | 3 ++ 10 files changed, 59 insertions(+), 103 deletions(-)
Ping?
Sorry - been heads down in another area... Always meant to get to this.. Anyway, about the only concern I have is the applicability of the changes in patch 2/3 to 'Virt_Device.c' in 'res_type_from_device_classname' at least with respect it be related to the console changes. It seems to be a separate issue unrelated to ConsoleDisplayController and should be its own patch. I also suppose in patch 3/3 the 'check_graphics' could change from a bool to an int returning "pos" as -1 or >= 0 for it's location in list. But that's just a minor thing. John
On 10/02/2013 01:41 PM, John Ferlan wrote: ...
Anyway, about the only concern I have is the applicability of the changes in patch 2/3 to 'Virt_Device.c' in 'res_type_from_device_classname' at least with respect it be related to the console changes. It seems to be a separate issue unrelated to ConsoleDisplayController and should be its own patch.
I actually made the issue by choosing the name of ConsoleDisplayController which is a superstring of DisplayController. I could move the strstr check of the longer string before the shorter one with the same result in a more compact patch.
I also suppose in patch 3/3 the 'check_graphics' could change from a bool to an int returning "pos" as -1 or >= 0 for it's location in list. But that's just a minor thing.
Now that you say it, this sounds more elegant...with the change discussed above this warrants a V2. I will be out returning Monday and will send out the revised version then. Thanks! -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 10/02/2013 11:04 AM, Viktor Mihajlovski wrote:
On 10/02/2013 01:41 PM, John Ferlan wrote: ...
Anyway, about the only concern I have is the applicability of the changes in patch 2/3 to 'Virt_Device.c' in 'res_type_from_device_classname' at least with respect it be related to the console changes. It seems to be a separate issue unrelated to ConsoleDisplayController and should be its own patch.
I actually made the issue by choosing the name of ConsoleDisplayController which is a superstring of DisplayController. I could move the strstr check of the longer string before the shorter one with the same result in a more compact patch.
It wasn't clear to me why the checks got an extra "_", eg: - if (strstr(classname, "NetworkPort")) + if (strstr(classname, "_NetworkPort")) I get the following: - else if (strstr(classname, "DisplayController")) + else if (strstr(classname, "_DisplayController")) return CIM_RES_TYPE_GRAPHICS; - else if (strstr(classname, "PointingDevice")) + else if (strstr(classname, "_ConsoleDisplayController")) + return CIM_RES_TYPE_CONSOLE; + else if (strstr(classname, "_PointingDevice")) But I'm OK witht the explanation. John
I also suppose in patch 3/3 the 'check_graphics' could change from a bool to an int returning "pos" as -1 or >= 0 for it's location in list. But that's just a minor thing.
Now that you say it, this sounds more elegant...with the change discussed above this warrants a V2. I will be out returning Monday and will send out the revised version then. Thanks!
participants (2)
-
John Ferlan -
Viktor Mihajlovski