[Libvirt-cim] [PATCH] Add support for console/serial grahpics devices
Chip Vincent
cvincent at linux.vnet.ibm.com
Thu Apr 7 17:27:19 UTC 2011
Opened bug to add end-to-end support for sdl. The password logic is
working as designed (password is not saved by libvirt-cim, but does show
a password has been set).
On 04/07/2011 10:02 AM, Sharad Mishra wrote:
> 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
>
>
>
> _______________________________________________
> Libvirt-cim mailing list
> Libvirt-cim at redhat.com
> https://www.redhat.com/mailman/listinfo/libvirt-cim
--
Chip Vincent
Open Virtualization
IBM Linux Technology Center
cvincent at linux.vnet.ibm.com
More information about the Libvirt-cim
mailing list