[Libvirt-cim] [PATCH 4/8] libxkutil: Console Support

[Viktor Mihajlovski]

On 09/10/2013 10:38 PM, John Ferlan wrote:
[...]
>> +
>> +        CU_DEBUG("console device type ID = %d", cdev->source_type);
>> +        free(source_type_str);
>
> This is free()'d again in err:, so we get a Coverity complaint about a
> double free. You need a "source_type_str = NULL;" here.
oops ... will fix by moving to the end
>
>> +
>> +        for (child = node->children; child != NULL; child = child->next) {
>> +                if (XSTREQ(child->name, "target")) {
>> +                        cdev->target_type = get_attr_value(child, "type");
>> +                        CU_DEBUG("Console device target type = '%s'",
>> +                                 cdev->target_type);
>
> If get_attr_value() returns NULL in cdev->target_type, then your
> CU_DEBUG() isn't going to be happy.
actually, I expect CU_DEBUG to handle that gracefully... as it does in
other places (see parse_graphics_device, vnc port determination)
>
[...]
>> +                                }
>> +                                free(udp_source_mode);
>
>
> This is free()'d again in err:, so we get a Coverity complaint about a
> double free.  You'll need a udp_source_mode = NULL; here.
yep
>
>> +                                break;
>> +                        case CIM_CHARDEV_SOURCE_TYPE_TCP:
>> +                                cdev->source_dev.tcp.mode =
>> +                                        get_attr_value(child, "mode");
>> +                                cdev->source_dev.tcp.host =
>> +                                        get_attr_value(child, "host");
>> +                                cdev->source_dev.tcp.service =
>> +                                        get_attr_value(child, "service");
>> +                                break;
>> +
>> +                        default:
>> +                                /* Nothing to do for :
>> +                                   CIM_CHARDEV_SOURCE_TYPE_STDIO
>> +                                   CIM_CHARDEV_SOURCE_TYPE_NULL
>> +                                   CIM_CHARDEV_SOURCE_TYPE_VC
>> +                                   CIM_CHARDEV_SOURCE_TYPE_SPICEVMC
>> +                                */
>> +                                break;
>> +                        }
>> +                }
>
> Not a whole lot of NULL error checking in any of the get_attr_value()
> calls, but that seems to be par for the course in the libvirt-cim code.
at some point in time we should consider a 'spring clean'
>
> The rest seemed fine to me.  I can squash in the above listed changes
> before pushing unless you really want to make a v2.
I'll make a v2, as I saw some other nits, like
[...]
>> +        if (-1 == asprintf(&vdev->id, "charconsole:%s", target_port_ID)) {
(left hand side constant in comparison) which escaped my attention
before ...
>> +                CU_DEBUG("Failed to create charconsole id string");
>> +                goto err;
>> +        }

-- 

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