"Daniel P. Berrange" <berrange@xxxxxxxxxx> wrote:
> On Fri, Apr 18, 2008 at 09:27:05PM +0100, Daniel P. Berrange wrote:
>> This patch updates Xen HVM to allow use of serial ¶llel ports, though
>> XenD limits you to single one of each even though QEMU supports many.
>> It also updates the <console> tag to support new syntax extensions.
>
> This version fixes a few memory leaks in the previous code...
Hi Dan,
> Index: src/xend_internal.c
> ===================================================================
> RCS file: /data/cvs/libvirt/src/xend_internal.c,v
> retrieving revision 1.180
> diff -u -p -r1.180 xend_internal.c
> --- src/xend_internal.c 10 Apr 2008 16:54:54 -0000 1.180
> +++ src/xend_internal.c 24 Apr 2008 15:10:00 -0000
> @@ -1376,6 +1376,243 @@ xend_parse_sexp_desc_os(virConnectPtr xe
> return(0);
> }
>
> +
> +int
> +xend_parse_sexp_desc_char(virConnectPtr conn,
> + virBufferPtr buf,
> + const char *devtype,
> + int portNum,
> + const char *value,
> + const char *tty)
> +{
> + const char *type;
> + int telnet = 0;
> + char *bindPort = NULL;
> + char *bindHost = NULL;
> + char *connectPort = NULL;
> + char *connectHost = NULL;
> + char *path = NULL;
> + int ret = -1;
> +
> + if (value[0] == '/') {
> + type = "dev";
> + } else if (STREQLEN(value, "null", 4)) {
> + type = "null";
> + value = NULL;
> + } else if (STREQLEN(value, "vc", 2)) {
> + type = "vc";
> + value = NULL;
> + } else if (STREQLEN(value, "pty", 3)) {
> + type = "pty";
> + value = NULL;
> + } else if (STREQLEN(value, "stdio", 5)) {
> + type = "stdio";
> + value = NULL;
> + } else if (STREQLEN(value, "file:", 5)) {
> + type = "file";
> + value += 5;
> + } else if (STREQLEN(value, "pipe:", 5)) {
> + type = "pipe";
> + value += 5;
> + } else if (STREQLEN(value, "tcp:", 4)) {
> + type = "tcp";
> + value += 4;
> + } else if (STREQLEN(value, "telnet:", 4)) {
That "4" should be 7.
It'd be nice to avoid the riskily redundant length,
by using a STR* macro that requires a literal string S
as argument #2, and uses sizeof(S)-1 as the length argument.
This code could be factored even more with a loop.
The only special cases would be value[0]=='/' and
"telnet:" having type of "tcp" and setting telnet=1.
> + type = "tcp";
> + value += 7;
> + telnet = 1;
> + } else if (STREQLEN(value, "udp:", 4)) {
> + type = "udp";
> + value += 4;
> + } else if (STREQLEN(value, "unix:", 5)) {
> + type = "unix";
> + value += 5;
> + } else {
> + virXendError(conn, VIR_ERR_INTERNAL_ERROR,
> + _("Unknown char device type"));
> + return -1;
> + }
> +
...
> + } else if (STREQ(type, "unix")) {
> + const char *offset = strchr(value, ',');
> + int dolisten = 0;
> + if (offset)
> + path = strndup(value, (offset - value));
> + else
> + path = strdup(value);
> + if (path == NULL)
> + goto no_memory;
> +
> + if (strstr(offset, ",listen") != NULL)
If the strchr call above finds no comma, then
"offset" will be NULL and strstr call will segfault.
...
> + tmp = sexpr_node(root, "domain/image/hvm/parallel");
> + if (tmp && STRNEQ(tmp, "none")) {
> + /* XXX does XenD stuff parallel port tty info into xenstore somewhere ? */
> + if (xend_parse_sexp_desc_char(conn, &buf, "parallel", 0, tmp, NULL) < 0)
> + goto error;
> + }
> + } else {
> + /* Paravirt always has a console */
> + if (tty) {
> + virBufferVSprintf(&buf, " <console type='pty' tty='%s'>\n", tty);
> + virBufferVSprintf(&buf, " <source path='%s'/>\n", tty);
> + } else {
> + virBufferAddLit(&buf, " <console type='pty'>\n");
> + }
> + virBufferAddLit(&buf, " <target port='0'/>\n");
> + virBufferAddLit(&buf, " </console>\n");
> }
> + free(tty);
>
> virBufferAddLit(&buf, " </devices>\n");
> virBufferAddLit(&buf, "</domain>\n");
All of the virBufferAddLit and virBufferVSprintf calls
above can fail with ENOMEM. I see that there are *many* more
virBufferAddLit and virBufferVSprintf calls that ignore their
return values (over 300), so maybe I'm missing something.
> Index: src/xend_internal.h
> ===================================================================
> RCS file: /data/cvs/libvirt/src/xend_internal.h,v
> retrieving revision 1.40
> diff -u -p -r1.40 xend_internal.h
> --- src/xend_internal.h 10 Apr 2008 16:54:54 -0000 1.40
> +++ src/xend_internal.h 24 Apr 2008 15:10:00 -0000
> @@ -20,12 +20,12 @@
>
> #include "libvirt/libvirt.h"
> #include "capabilities.h"
> +#include "buf.h"
>
> #ifdef __cplusplus
> extern "C" {
> #endif
>
> -
> /**
> * \brief Setup the connection to a xend instance via TCP
> * \param host The host name to connect to
> @@ -180,6 +180,13 @@ char *xenDaemonDomainDumpXMLByName(virCo
> */
> int xend_log(virConnectPtr xend, char *buffer, size_t n_buffer);
>
> + int xend_parse_sexp_desc_char(virConnectPtr conn,
> + virBufferPtr buf,
> + const char *devtype,
> + int portNum,
> + const char *value,
> + const char *tty);
> +
> char *xend_parse_domain_sexp(virConnectPtr conn, char *root, int xendConfigVersion);
>
> /* refactored ones */
> Index: src/xm_internal.c
> ===================================================================
> RCS file: /data/cvs/libvirt/src/xm_internal.c,v
> retrieving revision 1.70
> diff -u -p -r1.70 xm_internal.c
> --- src/xm_internal.c 10 Apr 2008 16:54:54 -0000 1.70
> +++ src/xm_internal.c 24 Apr 2008 15:10:01 -0000
> @@ -1025,11 +1025,22 @@ char *xenXMDomainFormatXML(virConnectPtr
> }
>
> if (hvm) {
> - if (xenXMConfigGetString(conf, "serial", &str) == 0 && !strcmp(str, "pty")) {
> - virBufferAddLit(buf, " <console/>\n");
> + if (xenXMConfigGetString(conf, "parallel", &str) == 0) {
> + if (STRNEQ(str, "none"))
> + xend_parse_sexp_desc_char(conn, buf, "parallel", 0, str, NULL);
> + }
> + if (xenXMConfigGetString(conf, "serial", &str) == 0) {
> + if (STRNEQ(str, "none")) {
> + xend_parse_sexp_desc_char(conn, buf, "serial", 0, str, NULL);
> + /* Add back-compat console tag for primary console */
> + xend_parse_sexp_desc_char(conn, buf, "console", 0, str, NULL);
> + }
> }
> - } else { /* Paravirt implicitly always has a console */
> - virBufferAddLit(buf, " <console/>\n");
> + } else {
> + /* Paravirt implicitly always has a single console */
> + virBufferAddLit(buf, " <console type='pty'>\n");
> + virBufferAddLit(buf, " <target port='0'/>\n");
> + virBufferAddLit(buf, " </console>\n");
> }
>
> virBufferAddLit(buf, " </devices>\n");
> @@ -2267,14 +2278,38 @@ virConfPtr xenXMParseXMLToConfig(virConn
> obj = NULL;
>
> if (hvm) {
> - obj = xmlXPathEval(BAD_CAST "count(/domain/devices/console) > 0", ctxt);
> - if ((obj != NULL) && (obj->type == XPATH_BOOLEAN) &&
> - (obj->boolval)) {
> - if (xenXMConfigSetString(conf, "serial", "pty") < 0)
> + xmlNodePtr cur;
> + cur = virXPathNode("/domain/devices/parallel[1]", ctxt);
> + if (cur != NULL) {
> + char scratch[PATH_MAX];
> +
> + if (virDomainParseXMLOSDescHVMChar(conn, scratch, sizeof(scratch), cur) < 0) {
> + goto error;
> + }
> +
> + if (xenXMConfigSetString(conf, "parallel", scratch) < 0)
> + goto error;
> + } else {
> + if (xenXMConfigSetString(conf, "parallel", "none") < 0)
> goto error;
> }
> - xmlXPathFreeObject(obj);
> - obj = NULL;
> +
> + cur = virXPathNode("/domain/devices/serial[1]", ctxt);
> + if (cur != NULL) {
> + char scratch[PATH_MAX];
> + if (virDomainParseXMLOSDescHVMChar(conn, scratch, sizeof(scratch), cur) < 0)
> + goto error;
> + if (xenXMConfigSetString(conf, "serial", scratch) < 0)
> + goto error;
> + } else {
> + if (virXPathBoolean("count(/domain/devices/console) > 0", ctxt)) {
> + if (xenXMConfigSetString(conf, "serial", "pty") < 0)
> + goto error;
> + } else {
> + if (xenXMConfigSetString(conf, "serial", "none") < 0)
> + goto error;
> + }
> + }
> }
>
> xmlFreeDoc(doc);
> Index: src/xml.c
> ===================================================================
> RCS file: /data/cvs/libvirt/src/xml.c,v
> retrieving revision 1.117
> diff -u -p -r1.117 xml.c
> --- src/xml.c 10 Apr 2008 16:54:54 -0000 1.117
> +++ src/xml.c 24 Apr 2008 15:10:01 -0000
> @@ -673,6 +673,178 @@ virDomainParseXMLGraphicsDescVFB(virConn
> }
>
>
> +int
> +virDomainParseXMLOSDescHVMChar(virConnectPtr conn,
> + char *buf,
> + size_t buflen,
> + xmlNodePtr node)
> +{
> + xmlChar *type = NULL;
Can you use "const char *" above, instead of "xmlChar *"?
If so, that'd let you remove most of those risky and
ugly (const char*) casts below.
Ditto for all of these others.
> + xmlChar *path = NULL;
> + xmlChar *bindHost = NULL;
> + xmlChar *bindService = NULL;
> + xmlChar *connectHost = NULL;
> + xmlChar *connectService = NULL;
> + xmlChar *mode = NULL;
> + xmlChar *protocol = NULL;
> + xmlNodePtr cur;
> +
> + type = xmlGetProp(node, BAD_CAST "type");
> +
> + if (type != NULL) {
> + cur = node->children;
> + while (cur != NULL) {
> + if (cur->type == XML_ELEMENT_NODE) {
> + if (xmlStrEqual(cur->name, BAD_CAST "source")) {
> + if (mode == NULL)
> + mode = xmlGetProp(cur, BAD_CAST "mode");
> +
> + if (STREQ((const char *)type, "dev") ||
> + STREQ((const char *)type, "file") ||
> + STREQ((const char *)type, "pipe") ||
> + STREQ((const char *)type, "unix")) {
> + if (path == NULL)
> + path = xmlGetProp(cur, BAD_CAST "path");
> +
> + } else if (STREQ((const char *)type, "udp") ||
> + STREQ((const char *)type, "tcp")) {
> + if (mode == NULL ||
> + STREQ((const char *)mode, "connect")) {
> +
> + if (connectHost == NULL)
> + connectHost = xmlGetProp(cur, BAD_CAST "host");
> + if (connectService == NULL)
> + connectService = xmlGetProp(cur, BAD_CAST "service");
> + } else {
> + if (bindHost == NULL)
> + bindHost = xmlGetProp(cur, BAD_CAST "host");
> + if (bindService == NULL)
> + bindService = xmlGetProp(cur, BAD_CAST "service");
> + }
> +
> + if (STREQ((const char*)type, "udp")) {
> + xmlFree(mode);
> + mode = NULL;
> + }
> + }
> + } else if (xmlStrEqual(cur->name, BAD_CAST "protocol")) {
> + if (protocol == NULL)
> + protocol = xmlGetProp(cur, BAD_CAST "type");
> + }
> + }
> + cur = cur->next;
> + }
> + }
> +
> + if (type == NULL ||
> + STREQ((const char *)type, "pty")) {
> + strncpy(buf, "pty", buflen);
> + } else if (STREQ((const char *)type, "null") ||
> + STREQ((const char *)type, "stdio") ||
> + STREQ((const char *)type, "vc")) {
> + snprintf(buf, buflen, "%s", type);
> + } else if (STREQ((const char *)type, "file") ||
> + STREQ((const char *)type, "dev") ||
> + STREQ((const char *)type, "pipe")) {
...
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list