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:
> 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->passwd);
> +
> + 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->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