Re: [Libvirt-cim] [PATCH] Add support for console/serial grahpics devices

Chip Vincent <cvincent@linux.vnet.ibm.com> Sent by: libvirt-cim-bounces@redhat.com

04/05/2011 07:35 AM

Please respond to List for discussion and development of libvirt CIM <libvirt-cim@redhat.com>

To

libvirt-cim@redhat.com

cc

Subject

[Libvirt-cim] [PATCH] Add support for console/serial grahpics devices

# HG changeset patch # User Chip Vincent <cvincent@us.ibm.com> # Date 1301520765 14400 # Node ID 18793660dbc8f0755062a7f90902379288501f4c # Parent a521a11eeec4b41399ca954ab17b874a708eb4b3 Add support for console/serial grahpics devices

Add support for Graphics RASD ResourceSubType = console | serial. This includes support for allowing more than a single graphics RASD instance. Instances can be created externally or during DefineSystem. No changes to current level of hotswapping.

Signed-off-by: Chip Vincent <cvincent@us.ibm.com>

diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -40,7 +40,8 @@ #define NET_XPATH (xmlChar *)"/domain/devices/interface" #define EMU_XPATH (xmlChar *)"/domain/devices/emulator" #define MEM_XPATH (xmlChar *)"/domain/memory | /domain/currentMemory" -#define GRAPHICS_XPATH (xmlChar *)"/domain/devices/graphics" +#define GRAPHICS_XPATH (xmlChar *)"/domain/devices/graphics | "\ + "/domain/devices/console | /domain/devices/serial" #define INPUT_XPATH (xmlChar *)"/domain/devices/input"

#define DEFAULT_BRIDGE "xenbr0" @@ -501,6 +502,7 @@ { struct virt_device *vdev = NULL; struct graphics_device *gdev = NULL; + xmlNode *child = NULL;

vdev = calloc(1, sizeof(*vdev)); if (vdev == NULL) @@ -509,24 +511,51 @@ gdev = &(vdev->dev.graphics);

gdev->type = get_attr_value(node, "type"); - gdev->port = get_attr_value(node, "port"); - gdev->host = get_attr_value(node, "listen"); - gdev->keymap = get_attr_value(node, "keymap"); - if (gdev->type == NULL) goto err;

+ CU_DEBUG("graphics device type = %s", gdev->type); + if (STREQC(gdev->type, "vnc")) { - if (gdev->port == NULL) + gdev->port = get_attr_value(node, "port"); + gdev->host = get_attr_value(node, "listen"); + gdev->keymap = get_attr_value(node, "keymap"); + + if (gdev->port == NULL || gdev->host == NULL) + goto err; + } + else if (STREQC(gdev->type, "pty")) { + if (node->name == NULL) goto err;

- if (gdev->host == NULL) - goto err; + /* Change type to serial, console, etc. It will be converted back + in xmlgen.c */ + free(gdev->type); + gdev->type = strdup((char *)node->name); + + for (child = node->children; child != NULL; + child = child->next) { + if (XSTREQ(child->name, "source")) + gdev->host = get_attr_value(child, "path"); + else if (XSTREQ(child->name, "target")) + gdev->port = get_attr_value(child, "port"); + } + } + else { + CU_DEBUG("Unknown graphics type %s", gdev->type); + goto err; }

vdev->type = CIM_RES_TYPE_GRAPHICS; vdev->id = strdup("graphics");

+ /* FIXME: IDs should be unique, but that breaks existing tests. + ret = asprintf(&vdev->id, "graphics:%s", gdev->type); + if(ret == -1) { + CU_DEBUG("Failed to create graphics is string"); + goto err; + } */ + *vdevs = vdev;

return 1; diff --git a/libxkutil/xmlgen.c b/libxkutil/xmlgen.c --- a/libxkutil/xmlgen.c +++ b/libxkutil/xmlgen.c @@ -401,43 +401,92 @@ return NULL; }

+static const char *graphics_vnc_xml(xmlNodePtr root, + struct graphics_device *dev) +{ + xmlNodePtr tmp = NULL; + + tmp = xmlNewChild(root, NULL, BAD_CAST "graphics", NULL); + if (tmp == NULL) + return XML_ERROR; + + xmlNewProp(tmp, BAD_CAST "type", BAD_CAST dev->type); + + if (STREQC(dev->type, "sdl")) + return NULL; + + if (dev->port) { + xmlNewProp(tmp, BAD_CAST "port", BAD_CAST dev->port); + if (STREQC(dev->port, "-1")) + xmlNewProp(tmp, BAD_CAST "autoport", BAD_CAST "yes"); + else + xmlNewProp(tmp, BAD_CAST "autoport", BAD_CAST "no"); + } + + if (dev->host) + xmlNewProp(tmp, BAD_CAST "listen", BAD_CAST dev->host); + + if (dev->passwd) + xmlNewProp(tmp, BAD_CAST "passwd", BAD_CAST dev->

+ + if (dev->keymap) + xmlNewProp(tmp, BAD_CAST "keymap", BAD_CAST dev-> keymap); + + 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->host) + xmlNewProp(tmp, BAD_CAST "path", BAD_CAST dev->host); + + tmp = xmlNewChild(pty, NULL, BAD_CAST "target", NULL); + if (tmp == NULL) + return XML_ERROR; + + if(dev->port) + xmlNewProp(tmp, BAD_CAST "port", BAD_CAST dev->port); + + return NULL; +} + static const char *graphics_xml(xmlNodePtr root, struct domain *dominfo) { + const char *msg = NULL; int i;

for (i = 0; i < dominfo->dev_graphics_ct; i++) { - xmlNodePtr tmp; struct virt_device *_dev = &dominfo->dev_graphics[i]; if (_dev->type == CIM_RES_TYPE_UNKNOWN) continue;

struct graphics_device *dev = &_dev->dev.graphics;

- tmp = xmlNewChild(root, NULL, BAD_CAST "graphics", NULL); - if (tmp == NULL) - return XML_ERROR; - - xmlNewProp(tmp, BAD_CAST "type", BAD_CAST dev->type); - - if (STREQC(dev->type, "sdl")) - goto out; - - if (STREQC(dev->port, "-1")) - xmlNewProp(tmp, BAD_CAST "autoport", BAD_CAST "yes"); - else { - xmlNewProp(tmp, BAD_CAST "autoport", BAD_CAST "no"); - xmlNewProp(tmp, BAD_CAST "port", BAD_CAST dev->

Chip, My comments are inline below at 2 places. There are few lines in this patch that are longer than 80 characters, please correct them. Thanks, Sharad Mishra Open Virtualization Linux Technology Center IBM libvirt-cim-bounces@redhat.com wrote on 04/05/2011 07:35:51 AM: passwd); port);

- } - xmlNewProp(tmp, BAD_CAST "listen", BAD_CAST dev->host); - xmlNewProp(tmp, BAD_CAST "keymap", BAD_CAST dev-> keymap); - - if (dev->passwd != NULL) - xmlNewProp(tmp, - BAD_CAST "passwd", - BAD_CAST dev->passwd); + 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; + + if(msg != NULL) + return msg; }

- out: return NULL; }

diff --git a/schema/ResourceAllocationSettingData.mof b/schema/ ResourceAllocationSettingData.mof --- a/schema/ResourceAllocationSettingData.mof +++ b/schema/ResourceAllocationSettingData.mof @@ -216,8 +216,10 @@ ] class Xen_GraphicsResourceAllocationSettingData : Xen_ResourceAllocationSettingData { - [Description ("VNC Address. IPv4 in a.b.c.d:port or" - "IPv6 in [ip]:port format")] + [Description ("If ResourceSubType is 'vnc', this is a VNC Address. " + "IPv4 in a.b.c.d:port or IPv6 in [ip]:port format. If ResourceSubType " + "is 'console', this is a character device path in " + "path:port format (e.g., '/dev/pts/3:0'\)")] string Address;

[Description ("Keyboard keymapping")] @@ -235,8 +237,10 @@ ] class KVM_GraphicsResourceAllocationSettingData : KVM_ResourceAllocationSettingData { - [Description ("VNC Address. IPv4 in a.b.c.d:port or" - "IPv6 in [ip]:port format")] + [Description ("If ResourceSubType is 'vnc', this is a VNC Address. " + "IPv4 in a.b.c.d:port or IPv6 in [ip]:port format. If ResourceSubType " + "is 'console', this is a character device path in " + "path:port format (e.g., '/dev/pts/3:0'\)")] string Address;

[Description ("Keyboard keymapping")] @@ -254,8 +258,10 @@ ] class LXC_GraphicsResourceAllocationSettingData : LXC_ResourceAllocationSettingData { - [Description ("VNC Address. IPv4 in a.b.c.d:port or" - "IPv6 in [ip]:port format")] + [Description ("If ResourceSubType is 'vnc', this is a VNC Address. " + "IPv4 in a.b.c.d:port or IPv6 in [ip]:port format. If ResourceSubType " + "is 'console', this is a character device path in " + "path:port format (e.g., '/dev/pts/3:0'\)")] string Address;

[Description ("Keyboard keymapping")] diff --git a/src/Virt_Device.c b/src/Virt_Device.c --- a/src/Virt_Device.c +++ b/src/Virt_Device.c @@ -189,14 +189,13 @@ int rc; char *vp_str = NULL;

- if (STREQC(dev->type, "vnc")) + if (STREQC(dev->type, "sdl")) + rc = asprintf(&vp_str, "%s", dev->type); + else rc = asprintf(&vp_str, "%s/%s:%s", dev->type, dev->host, dev->port); - else - rc = asprintf(&vp_str, "%s", dev->type); - if (rc == -1) return 0;

diff --git a/src/Virt_RASD.c b/src/Virt_RASD.c --- a/src/Virt_RASD.c +++ b/src/Virt_RASD.c @@ -421,44 +421,56 @@ CMSetProperty(inst, "ResourceSubType", (CMPIValue *)dev->dev.graphics.type, CMPI_chars);

- if (STREQC(dev->dev.graphics.type, "vnc")) { + if (STREQC(dev->dev.graphics.type, "sdl")) + rc = asprintf(&addr_str, "%s", dev->dev.graphics.type); + else { rc = asprintf(&addr_str, "%s:%s", dev->dev.graphics.host, dev->dev.graphics.port); - if (rc == -1) + } + + CU_DEBUG("graphics Address = %s", addr_str); + + if (rc == -1) + goto out; + + CMSetProperty(inst, "Address", + (CMPIValue *)addr_str, CMPI_chars); + + if (STREQC(dev->dev.graphics.type, "vnc")) { + CMSetProperty(inst, "KeyMap", + (CMPIValue *)dev->dev.graphics.keymap, CMPI_chars); + + conn = connect_by_classname(_BROKER, classname, &s); + if (conn == NULL) goto out;

- CMSetProperty(inst, "Address", - (CMPIValue *)addr_str, CMPI_chars); + dom = virDomainLookupByName(conn, name); + if (dom == NULL) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_NOT_FOUND, + "Domain %s not found", + name); + goto out; + }

- CMSetProperty(inst, "KeyMap", - (CMPIValue *)dev->dev.graphics.keymap, CMPI_chars); + infostore = infostore_open(dom); + if (infostore != NULL) + has_passwd = infostore_get_bool(infostore, + "has_vnc_passwd"); + + if (has_passwd) { + CU_DEBUG("has password");

Password is being set to "*****". According to libvirt.org, password should appear in clear text. I am not seeing password attribute in libvirt xml even when password is being passed to libvirt-cim. Here is the libvirt xml generated - <graphics type='vnc' port='5910' autoport='no' listen='127.0.0.1' keymap='en-us'/> I was expecting something like this - <graphics type="vnc" port="5910" autoport="no" listen="127.0.0.1" passwd="test0all" keymap="en-us"/> Libvirt-cim log indicates that code knew about password but lost it somewhere - device_parsing.c(517): graphics device type = vnc Virt_RASD.c(433): graphics Address = 127.0.0.1:5910 misc_util.c(75): Connecting to libvirt with uri `qemu:///system' infostore.c(89): Path is /etc/libvirt/cim/QEMU_test_kvmredsap_dom Virt_RASD.c(464): has password

+ CMSetProperty(inst, "Password", + (CMPIValue *)"********", CMPI_chars); + } + + infostore_close(infostore); + + /* FIXME: Populate the IsIPv6Only */ }

- conn = connect_by_classname(_BROKER, classname, &s); - if (conn == NULL) - goto out; - - dom = virDomainLookupByName(conn, name); - if (dom == NULL) { - cu_statusf(_BROKER, &s, - CMPI_RC_ERR_NOT_FOUND, - "Domain %s not found", - name); - goto out; - } - - infostore = infostore_open(dom); - if (infostore != NULL) - has_passwd = infostore_get_bool(infostore, "has_vnc_passwd"); - - if (has_passwd) - CMSetProperty(inst, "Password", - (CMPIValue *)"********", CMPI_chars); - - infostore_close(infostore); - out: free(addr_str); virDomainFree(dom); diff --git a/src/Virt_SettingsDefineCapabilities.c b/src/ Virt_SettingsDefineCapabilities.c --- a/src/Virt_SettingsDefineCapabilities.c +++ b/src/Virt_SettingsDefineCapabilities.c @@ -1694,10 +1694,9 @@ inst = sdc_rasd_inst(&s, ref, CIM_RES_TYPE_GRAPHICS, DEVICE_RASD);

CMSetProperty(inst, "InstanceID", (CMPIValue *)id, CMPI_chars); - + CMSetProperty(inst, "Address", (CMPIValue *)addr, CMPI_chars); + if (STREQC(type, "vnc")) { - CMSetProperty(inst, "Address", (CMPIValue *)addr, CMPI_chars); - CMSetProperty(inst, "KeyMap", (CMPIValue *)"en-us", CMPI_chars); }

diff --git a/src/Virt_VirtualSystemManagementService.c b/src/ Virt_VirtualSystemManagementService.c --- a/src/Virt_VirtualSystemManagementService.c +++ b/src/Virt_VirtualSystemManagementService.c @@ -405,7 +405,7 @@

static bool add_default_devs(struct domain *domain) { - if (domain->dev_graphics_ct != 1) { + if (domain->dev_graphics_ct < 1) { if (!default_graphics_device(domain)) return false; } @@ -1027,6 +1027,38 @@ return NULL; }

+static int parse_console_address(const char *id, + char **path, + char **port) +{ + int ret; + char *tmp_path = NULL; + char *tmp_port = NULL; + + CU_DEBUG("Entering parse_console_address, address is %s", id); + + ret = sscanf(id, "%a[^:]:%as", &tmp_path, &tmp_port); + + if (ret != 2) { + ret = 0; + goto out; + } + + if (path) + *path = strdup(tmp_path); + + if (port) + *port = strdup(tmp_port); + + ret = 1; + + out: + CU_DEBUG("Exiting parse_console_address, ip is %s, port is %s", + *path, *port); + + return ret; +} + static int parse_vnc_address(const char *id, char **ip, char **port) @@ -1059,8 +1091,6 @@

out: CU_DEBUG("Exiting parse_vnc_address, ip is %s, port is %s", *ip, *port); - free(tmp_ip); - free(tmp_port);

return ret; } @@ -1068,9 +1098,8 @@ static const char *graphics_rasd_to_vdev(CMPIInstance *inst, struct virt_device *dev) { - const char *val; + const char *val = NULL; const char *msg = NULL; - const char *keymap; bool ipv6 = false; int ret;

@@ -1080,36 +1109,67 @@ } dev->dev.graphics.type = strdup(val);

+ CU_DEBUG("graphics type = %s", dev->dev.graphics.type); +

If graphics type is "sdl", then the logic below leads to error message that "sdl" is unsupported

/* FIXME: Add logic to prevent address:port collisions */ - if (cu_get_str_prop(inst, "Address", &val) != CMPI_RC_OK) { - CU_DEBUG("no graphics port defined, giving default"); - if (cu_get_bool_prop(inst, "IsIPv6Only", &ipv6) != CMPI_RC_OK) - ipv6 = false; - if (ipv6) - dev->dev.graphics.host = strdup("[::1]"); - else - dev->dev.graphics.host = strdup("127.0.0.1"); - dev->dev.graphics.port = strdup("-1"); - } else { - ret = parse_vnc_address(val, - &dev->dev.graphics.host, - &dev->dev.graphics.port); + if (STREQC(dev->dev.graphics.type, "vnc")) { + if (cu_get_str_prop(inst, "Address", &val) !=

CMPI_RC_OK) {

+ CU_DEBUG("graphics Address empty, using default"); + + if (cu_get_bool_prop(inst, "IsIPV6Only", &ipv6) != CMPI_RC_OK) + ipv6 = false; + + if(ipv6) + val = "[::1]:-1"; + else + val = "127.0.0.1:-1"; + } + + ret = parse_vnc_address(val, + &dev->dev.graphics.host, + &dev->dev.graphics.port); if (ret != 1) { msg = "GraphicsRASD field Address not valid"; goto out; } + + if (cu_get_str_prop(inst, "KeyMap", &val) != CMPI_RC_OK) + dev->dev.graphics.keymap = strdup("en-us"); + else + dev->dev.graphics.keymap = strdup(val); + + if (cu_get_str_prop(inst, "Password", &val) != CMPI_RC_OK) { + CU_DEBUG("vnc password is not set"); + dev->dev.graphics.passwd = NULL; + } else { + CU_DEBUG("vnc password is set"); + dev->dev.graphics.passwd = strdup(val); + } } - - if (cu_get_str_prop(inst, "KeyMap", &keymap) != CMPI_RC_OK) - keymap = "en-us"; - - dev->dev.graphics.keymap = strdup(keymap); - - if (cu_get_str_prop(inst, "Password", &val) != CMPI_RC_OK) { - dev->dev.graphics.passwd = NULL; - } else { - dev->dev.graphics.passwd = strdup(val); - } + else if (STREQC(dev->dev.graphics.type, "console") || + STREQC(dev->dev.graphics.type, "serial")) { + if (cu_get_str_prop(inst, "Address", &val) != CMPI_RC_OK) { + CU_DEBUG("graphics Address empty, using default"); + val = "/dev/pts/0:0"; + } + + ret = parse_console_address(val, + &dev->dev.graphics.host, + &dev->dev.graphics.port); + if (ret != 1) { + msg = "GraphicsRASD field Address not valid"; + goto out; + } + } else { + CU_DEBUG("Unsupported graphics type %s", dev->dev.graphics.type); + msg = "Unsupported graphics type"; + goto out; + } + + CU_DEBUG("graphics = %s:%s:%s", + dev->dev.graphics.type, + dev->dev.graphics.host, + dev->dev.graphics.port);

out: return msg; @@ -1250,6 +1310,9 @@ "DiskResourceAllocationSettingData in a single " "guest";

+ if (dev->type == CIM_RES_TYPE_GRAPHICS) + continue; + if (STREQC(ptr->id, dev->id)) { CU_DEBUG("Overriding device %s from refconf", ptr->id); cleanup_virt_device(ptr); @@ -1358,11 +1421,19 @@ ncount, &domain-> dev_net_ct); } else if (type == CIM_RES_TYPE_GRAPHICS) { - domain->dev_graphics_ct = 1; + struct virt_device dev; + int ncount = count + domain->dev_graphics_ct; + + memset(&dev, 0, sizeof(dev)); msg = rasd_to_vdev(inst, domain, - &domain->dev_graphics[0], + &dev, ns); + if (msg == NULL) + msg = add_device_nodup(&dev, + domain-> dev_graphics, + ncount, + &domain->dev_graphics_ct); } else if (type == CIM_RES_TYPE_INPUT) { domain->dev_input_ct = 1; msg = rasd_to_vdev(inst, @@ -2318,13 +2389,6 @@ goto out; }

- if ((type == CIM_RES_TYPE_GRAPHICS) && (*count > 0)) { - cu_statusf(_BROKER, &s, - CMPI_RC_ERR_FAILED, - "A resource already exists for type %" PRIu16, type); - goto out; - } - list = realloc(*_list, ((*count)+1)*sizeof(struct virt_device)); if (list == NULL) { /* No memory */

_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim