On Tue, Mar 10, 2015 at 19:20:28 -0400, John Ferlan wrote: > Coverity complains that "net_set" is compared to NULL before calling > xen_network_set_free, but used rather liberally before that. While > I was looking at the code I also noted that if the virAsprintfQuiet > fails, then we leak our structures - so I added those too. > > Signed-off-by: John Ferlan <[email protected]> > --- > src/xenapi/xenapi_utils.c | 25 +++++++++++++++---------- > 1 file changed, 15 insertions(+), 10 deletions(-) > > diff --git a/src/xenapi/xenapi_utils.c b/src/xenapi/xenapi_utils.c > index ce09dfe..0c13745 100644 > --- a/src/xenapi/xenapi_utils.c > +++ b/src/xenapi/xenapi_utils.c > @@ -399,14 +399,14 @@ createVifNetwork(virConnectPtr conn, xen_vm vm, int > device, > xen_network_set *net_set = NULL; > xen_network_record *net_rec = NULL; > int cnt = 0; > - if (xen_network_get_all(session, &net_set)) { > - for (cnt = 0; cnt < net_set->size; cnt++) { > - if (xen_network_get_record(session, &net_rec, > net_set->contents[cnt])) { > - if (STREQ(net_rec->bridge, bridge)) { > - break; > - } else { > - xen_network_record_free(net_rec); > - } > + if (!xen_network_get_all(session, &net_set)) > + return -1
missing semicolon?
Also @vm_opt is allocated in the declarations section and would be
leaked here. Pre-existing though, so I won't insist on fixing that too.
> + for (cnt = 0; cnt < net_set->size; cnt++) {
> + if (xen_network_get_record(session, &net_rec,
> net_set->contents[cnt])) {
> + if (STREQ(net_rec->bridge, bridge)) {
> + break;
> + } else {
> + xen_network_record_free(net_rec);
> }
> }
> }
> @@ -425,8 +425,13 @@ createVifNetwork(virConnectPtr conn, xen_vm vm, int
> device,
> vif_record->other_config = xen_string_string_map_alloc(0);
> vif_record->runtime_properties = xen_string_string_map_alloc(0);
> vif_record->qos_algorithm_params = xen_string_string_map_alloc(0);
> - if (virAsprintfQuiet(&vif_record->device, "%d", device) < 0)
> + if (virAsprintfQuiet(&vif_record->device, "%d", device) < 0) {
> + xen_vif_free(vif);
@vif is allocated few lines below so no need to free it yet.
> + xen_vif_record_free(vif_record);
> + xen_network_record_free(net_rec);
> + xen_network_set_free(net_set);
> return -1;
> + }
> xen_vif_create(session, &vif, vif_record);
> if (!vif) {
> xen_vif_free(vif);
> @@ -438,7 +443,7 @@ createVifNetwork(virConnectPtr conn, xen_vm vm, int
> device,
> xen_vif_record_free(vif_record);
> xen_network_record_free(net_rec);
> }
> - if (net_set != NULL) xen_network_set_free(net_set);
> + xen_network_set_free(net_set);
> return -1;
> }
Looks good, ACK if you compile test this patch before pushing, with or
without fixing of the other existing leak.
Peter
signature.asc
Description: Digital signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
