[Libvirt-cim] [PATCH] Add support for console/serial grahpics devices
Sharad Mishra
snmishra at us.ibm.com
Thu Apr 7 14:02:04 UTC 2011
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 at redhat.com wrote on 04/06/2011 05:46:23 PM:
> Chip Vincent <cvincent at linux.vnet.ibm.com>
> Sent by: libvirt-cim-bounces at redhat.com
>
> 04/06/2011 05:46 PM
>
> Please respond to
> cvincent at linux.vnet.ibm.com; Please respond to
> List for discussion and development of libvirt CIM
<libvirt-cim at redhat.com>
>
> To
>
> libvirt-cim at 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 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
>
> _______________________________________________
> Libvirt-cim mailing list
> Libvirt-cim at redhat.com
> https://www.redhat.com/mailman/listinfo/libvirt-cim
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvirt-cim/attachments/20110407/1a47643b/attachment.htm>
More information about the Libvirt-cim
mailing list