On 09/11/2013 12:05 AM, John Ferlan wrote: [...]
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...
yes ... this will become ugly ...
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... I was prepared to contradict here ... but I am changing my mind while I type, stupid me ... I will rework the previous patches wrt to this as well
+ }
- 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 wording needs improvement ... agreed 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...
agree [...]
+ + 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.
will do
+ 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 {"
agree
+ 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.
yep
+ 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
-> V2
-- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294