Re: [Libvirt-cim] [PATCH 7/8] VSMS: Support for domains with console devices

> if (strstr(id, "[") != NULL) { > /* its an ipv6 address */ > ret = sscanf(id, "%a[^]]]:%as", &tmp_ip, &tmp_port); > + tmp_ip = realloc(tmp_ip, strlen(tmp_ip) + 2); What if tmp_ip == NULL? This one needs to be checked...

> strcat(tmp_ip, "]"); > } else { > ret = sscanf(id, "%a[^:]:%as", &tmp_ip, &tmp_port); > } > - if (ret != 2) { > + /* ret == 2: address and port, ret == 1: address only */ > + if (ret < 1) { > ret = 0; > goto out; > } > - if (ip) > + if (ip) { > *ip = strdup(tmp_ip); > + CU_DEBUG("IP = '%s'",*ip); strdup() could return NULL, although I suppose it's only important for debugging...

> + } > - if (port) > + if (port && tmp_port) { > *port = strdup(tmp_port); > - > - ret = 1; > + CU_DEBUG("Port = '%s'",*port); Again - strdup() could return NULL > + } > out: > - if (ip && port) > - CU_DEBUG("Exiting parse_vnc_address, ip is %s, port is %s", > - *ip, *port); > - > free(tmp_ip); > free(tmp_port); > return ret; > } > +static bool parse_console_url(const char *url, > + char **protocol, > + char **host, > + char **port) > +{ > + bool success = false; > + char *tmp_protocol = NULL; > + char *tmp_address = NULL; > + > + CU_DEBUG("Entering parse_console_url:'%s'", url); > + > + if (sscanf(url,"%a[^:]://%as", &tmp_protocol, &tmp_address) != 2) > + goto out; > + > + if (parse_ip_address(tmp_address, host, port) < 1) > + goto out; > + > + if (protocol) { > + *protocol = strdup(tmp_protocol); > + CU_DEBUG("Protocol = '%s'", *protocol); strdup() failure again > + } > + > + success = true; > + > + out: > + free(tmp_protocol); > + free(tmp_address); > + > + return success; > +} > + > +static const char *_unixsock_console_rasd_to_vdev(CMPIInstance *inst, > + struct console_device *cdev) > +{ > + const char *val = NULL; > + const char *val2 = NULL; > + char* protocol = NULL; > + > + cdev->source_dev.unixsock.mode = NULL; > + if (cu_get_str_prop(inst,"ConnectURL", &val) == CMPI_RC_OK) { > + CU_DEBUG("ConnectURL = '%s'", val); > + cdev->source_dev.unixsock.mode = strdup("connect"); > + } > + > + if (cu_get_str_prop(inst, "BindURL", &val2) == CMPI_RC_OK) { > + if (cdev->source_dev.unixsock.mode != NULL) > + return "ConsoleRASD: Only ConnectURL or BindURL are allowed for " > + "UNIX domain socket client/server."; Not quite sure I understand the above check - are you checking that both Connect and Bind aren't being used? Then perhaps the message should state - cannot supply both ConnectURL and BindURL. Of course the above

strdup() could fail, but that's a different issue... > + CU_DEBUG("BindURL = '%s'", val2); > + cdev->source_dev.unixsock.mode = strdup("bind"); > + val = val2; > + } > + > + if (val) { > + if (!parse_console_url(val, &protocol, &cdev->source_dev.unixsock.path, NULL)) > + return "ConsoleRASD: Invalid ConnectURL or BindURL for " > + "UNIX domain socket client/server."; > + > + if (!STREQC("file", protocol)) { > + CU_DEBUG("Wrong ConsoleRASD protocol specified: '%s'",protocol); > + free(protocol); > + return "ConsoleRASD: Protocol 'file' was not specified for " > + "ConnectURL or BindURL for UNIX domain socket client/server."; > + } > + free(protocol); > + } else Prefer to see "} else {" as it's cleaner and no one mistakenly adds a line eventually. BTW: We could get here if strdup() fails...

> + > + if (cu_get_str_prop(inst, "BindURL", &val2) == CMPI_RC_OK) { > + if (cdev->source_dev.tcp.mode != NULL) > + return "ConsoleRASD: Only ConnectURL or BindURL are allowed for " > + "TCP client/server console."; Similar comment from above - cannot supply both.

> + CU_DEBUG("BindURL = '%s'",val2); > + cdev->source_dev.tcp.mode = strdup("bind"); > + val = val2; > + } > + > + if (val) { > + if (!parse_console_url(val, > + &cdev->source_dev.tcp.protocol, > + &cdev->source_dev.tcp.host, > + &cdev->source_dev.tcp.service)) > + return "ConsoleRASD: Invalid ConnectURL or BindURL for " > + "TCP client/server console."; > + if (cdev->source_dev.tcp.service == NULL) > + return "ConsoleRASD: Missing TCP port for TCP client/server console."; > + } else Again the "} else {"

> + return "ConsoleRASD: ConnectURL or BindURL not specified for " > + "TCP client/server console."; > + > + if (STREQC("udp",cdev->source_dev.tcp.protocol)) { > + CU_DEBUG("Wrong ConsoleRASD protocol specified: '%s'", cdev->source_dev.tcp.protocol); > + return "ConsoleRASD: Invalid protocol 'udp' was specified at " > + "TCP client/server console."; > + } > + > + if (STREQC("file", cdev->source_dev.tcp.protocol)) { > + CU_DEBUG("Wrong ConsoleRASD protocol specified: '%s'",cdev->source_dev.tcp.protocol); > + return "ConsoleRASD: Invalid protocol 'file' was specified at " > + "TCP client/server console."; > + } > + > + return NULL; > +} > + > +static const char *console_rasd_to_vdev(CMPIInstance *inst, > + struct virt_device *dev) > +{ > + int rc = 0; > + const char *msg = NULL; > + const char *val = NULL; > + struct console_device *cdev = &dev->dev.console; > + uint16_t tmp; > + > + rc = cu_get_u16_prop(inst, "SourceType", &tmp); > + if (rc != CMPI_RC_OK) > + return "ConsoleRASD: SourceType field not specified."; > + > + if (tmp < 0 || tmp >= CIM_CHARDEV_SOURCE_TYPE_INVALIDTYPE) tmp is a uint16_t so a < 0 comparison is not possible.

> + return "ConsoleRASD: Invalid SourceType value"; > + > + cdev->source_type = tmp; > + CU_DEBUG("Processeing SourceType: %d", cdev->source_type); > + > + /* property not required */ > + if (cu_get_str_prop(inst, "TargetType", &val) == CMPI_RC_OK) > + cdev->target_type = strdup(val); > + CU_DEBUG("TargetType is '%s'", cdev->target_type); More strdup/debug Nothing overly serious - let me know how you want to proceed especially with respect to realloc() failure and unsigned compare less than zero