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

[Viktor Mihajlovski]

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