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

[Chip Vincent]

Thanks for the comments. My responses below.

On 04/06/2011 03:19 PM, Sharad Mishra wrote:
> 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 at redhat.com wrote on 04/05/2011 07:35:51 AM:
>
>  > Chip Vincent <cvincent at linux.vnet.ibm.com>
>  > Sent by: libvirt-cim-bounces at redhat.com
>  >
>  > + 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
>

The password is only clear text when the CIM instance is created and 
passed to libvirt. Once the password is set, it cannot be fetched 
(except by a secure libvirt connection, I think). The only was to change 
the password today is delete the instance (or XML) and recreate with a 
new password. That is why the existence of a password is determine via 
an attribute in the libvirt-cim data store.

NOTE: This is the original logic and was not changed in this patch, just 
re-factored to be contained within the "vnc" code path.


>  > @@ -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

The original code only handled vnc connections and would incorrectly 
create other ResourceSubTypes, such as 'sdl'. For example, the original 
code would create sdl objects with a internet address in the XML and 
pass to libvirt, which would then reject the device. Since sdl did not 
appear to be full supported, I added it to the explicitly not supported 
list. We have other code that "tolerates" sdl, but more work is needed 
to ensure complete support. I'll defer this work to another bug/patch.

>
>  > /* 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";
>  > + }
>  > +
>
> _______________________________________________
> Libvirt-cim mailing list
> Libvirt-cim at redhat.com
> https://www.redhat.com/mailman/listinfo/libvirt-cim

If there are no other issues, I'll resolve the line-length issues before 
pushing. Agreed?

-- 
Chip Vincent
Open Virtualization
IBM Linux Technology Center
cvincent at linux.vnet.ibm.com