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

Chip, Can you open new bugs to fix "sdl" support and "password" issues ? Regards, Sharad Mishra Open Virtualization Linux Technology Center IBM libvirt-cim-bounces(a)redhat.com wrote on 04/06/2011 05:46:23 PM: > Chip Vincent <cvincent(a)linux.vnet.ibm.com&gt; > Sent by: libvirt-cim-bounces(a)redhat.com > > 04/06/2011 05:46 PM > > Please respond to > cvincent(a)linux.vnet.ibm.com; Please respond to > List for discussion and development of libvirt CIM <libvirt-cim(a)redhat.com&gt; > > To > > libvirt-cim(a)redhat.com > > cc > > Subject > > Re: [Libvirt-cim] [PATCH] Add support for console/serial grahpics devices > > 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(a)redhat.com wrote on 04/05/2011 07:35:51 AM: > > > > > Chip Vincent <cvincent(a)linux.vnet.ibm.com&gt; > > > Sent by: libvirt-cim-bounces(a)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(a)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(a)linux.vnet.ibm.com > > _______________________________________________ > Libvirt-cim mailing list > Libvirt-cim(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvirt-cim _______________________________________________ Libvirt-cim mailing list Libvirt-cim(a)redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim