Re: [Libvirt-cim] [PATCH 4/8] libxkutil: Console Support
On 09/10/2013 10:38 PM, John Ferlan wrote:
[...]
[...]
>> + }
>> + 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
[...]
> +
> + 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)
> + 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