Zhou Zheng Sheng has posted comments on this change. Change subject: Add simple text-based console in vdsClient ......................................................................
Patch Set 2: Verified; I would prefer that you didn't submit this (1 inline comment) Successfully connect to the guest console, but I have some different ideas on the command design. .................................................... File vdsm_cli/vdsClient.py Line 1614: protocol = "tls" if self.useSSL else "tcp" Line 1615: conn = "qemu+" + protocol + "://" + self.server + "/system" Line 1616: p = subprocess.Popen(["virsh", "-c", conn, "console", args[0]]) Line 1617: p.wait() Line 1618: return 0, '' I do not agree with you on the virsh connection string here. There are many ways to connect to libvirt. For example, when connecting to local libvirtd, sometimes neither tls nor tcp is enabled, and only unix socket is enabled. This code can not adapt to the above situation. Secondly, self.useSSL is a flag for connecting vdsm, not a flag for connection to libvirt. We can connectI think it's not good to re-use a member variable for this different purpose. However re-using self.server as destination for virsh here is OK. The protocols for connecting vdsm and libvirt can be different, but the destination always the same. Thirdly, we get the console by referring its name, I think UUID is more suitable here. The user refer to the VM by UUID, then vdsClient can firstly get the VM name from vdsm by UUID, then open a console to that VM by name. If we open a VM console by name, there is no difference for the user to use virsh directly. So I think we can re-design the command as follow. vdsClient [-s] host openVmConsole [tsl|tcp|socket] vm-uuid If the [tls|tcp|socket] part is omitted, then the default transport is Unix socket. The disadvantage here is the transport detail is leaked to the user here, but we can not do better unless either the remote vdsm can tunnel a libvirt connection for us or vdsm can tell us the libvirtd transport related configurations. If it did, then more information is leaked out of vdsm, not good... If vdsm can provide a tunnel to console through xmlrpc or ssh as you implemented in the other patch, most of the problems I mentioned can be solved. Line 1619: Line 1620: Line 1621: if __name__ == '__main__': Line 1622: if _glusterEnabled: -- To view, visit http://gerrit.ovirt.org/8041 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I368dbcbc2e180161f256f3fb450c344acb6a6c8a Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xu He Jie <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Mark Wu <[email protected]> Gerrit-Reviewer: Ryan Harper <[email protected]> Gerrit-Reviewer: ShaoHe Feng <[email protected]> Gerrit-Reviewer: Xu He Jie <[email protected]> Gerrit-Reviewer: Zhou Zheng Sheng <[email protected]> _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
