Re: [PATCH v2] Make sure that filedescriptors are only closed once

[Stefan Berger <stefanb@xxxxxxxxxxxxxxxxxx>]

 On 10/15/2010 07:16 AM, Daniel P. Berrange wrote:
> On Thu, Oct 14, 2010 at 09:31:59PM -0400, Stefan Berger wrote:
> >   V2:
> >    also the path where close() failed needs to be corrected;
> >
> > ->  time for a VIR_CLOSE() ?
> >
> > Setting fd=-1 in these places makes sure that the fd is not closed twice
> > later on in 'clean:'. This is of course a recipe for disaster in a
> > multi-threaded system where the filedescriptor can belong to something
> > else right away.
> >
> > At least we have a pattern...
> >
> > (looks like I can close the augeas bug as well...)
> >
> > Signed-off-by: Stefan Berger<stefanb@xxxxxxxxxx>
> >
> > ---
> >   src/conf/domain_conf.c   |    5 ++++-
> >   src/conf/network_conf.c  |    5 ++++-
> >   src/conf/nwfilter_conf.c |   10 ++++++++--
> >   src/conf/storage_conf.c  |    5 ++++-
> >   4 files changed, 20 insertions(+), 5 deletions(-)
> >
> > Index: libvirt-acl/src/conf/domain_conf.c
> > ===================================================================
> > --- libvirt-acl.orig/src/conf/domain_conf.c
> > +++ libvirt-acl/src/conf/domain_conf.c
> > @@ -6802,13 +6802,16 @@ int virDomainSaveXML(const char *configD
> >           virReportSystemError(errno,
> >                                _("cannot save config file '%s'"),
> >                                configFile);
> > -        goto cleanup;
> > +        goto cleanup_free;
> >       }
> >
> > +    fd = -1;
> > +
> >       ret = 0;
> >    cleanup:
> >       if (fd != -1)
> >           close(fd);
> > + cleanup_free:
> >       VIR_FREE(configFile);
> >       return ret;
> >   }
> > Index: libvirt-acl/src/conf/network_conf.c
> > ===================================================================
> > --- libvirt-acl.orig/src/conf/network_conf.c
> > +++ libvirt-acl/src/conf/network_conf.c
> > @@ -691,15 +691,18 @@ int virNetworkSaveXML(const char *config
> >           virReportSystemError(errno,
> >                                _("cannot save config file '%s'"),
> >                                configFile);
> > -        goto cleanup;
> > +        goto cleanup_free;
> >       }
> >
> > +    fd = -1;
> > +
> >       ret = 0;
> >
> >    cleanup:
> >       if (fd != -1)
> >           close(fd);
> >
> > + cleanup_free:
> >       VIR_FREE(configFile);
> >
> >       return ret;
> > Index: libvirt-acl/src/conf/nwfilter_conf.c
> > ===================================================================
> > --- libvirt-acl.orig/src/conf/nwfilter_conf.c
> > +++ libvirt-acl/src/conf/nwfilter_conf.c
> > @@ -2197,15 +2197,18 @@ int virNWFilterSaveXML(const char *confi
> >           virReportSystemError(errno,
> >                                _("cannot save config file '%s'"),
> >                                configFile);
> > -        goto cleanup;
> > +        goto cleanup_free;
> >       }
> >
> > +    fd = -1;
> > +
> >       ret = 0;
> >
> >    cleanup:
> >       if (fd != -1)
> >           close(fd);
> >
> > + cleanup_free:
> >       VIR_FREE(configFile);
> >
> >       return ret;
> > @@ -2608,15 +2611,18 @@ virNWFilterPoolObjSaveDef(virNWFilterDri
> >           virReportSystemError(errno,
> >                                _("cannot save config file %s"),
> >                                pool->configFile);
> > -        goto cleanup;
> > +        goto cleanup_free;
> >       }
> >
> > +    fd = -1;
> > +
> >       ret = 0;
> >
> >    cleanup:
> >       if (fd != -1)
> >           close(fd);
> >
> > + cleanup_free:
> >       VIR_FREE(xml);
> >
> >       return ret;
> > Index: libvirt-acl/src/conf/storage_conf.c
> > ===================================================================
> > --- libvirt-acl.orig/src/conf/storage_conf.c
> > +++ libvirt-acl/src/conf/storage_conf.c
> > @@ -1564,15 +1564,18 @@ virStoragePoolObjSaveDef(virStorageDrive
> >           virReportSystemError(errno,
> >                                _("cannot save config file %s"),
> >                                pool->configFile);
> > -        goto cleanup;
> > +        goto cleanup_free;
> >       }
> >
> > +    fd = -1;
> > +
> >       ret = 0;
> >
> >    cleanup:
> >       if (fd != -1)
> >           close(fd);
> >
> > + cleanup_free:
> >       VIR_FREE(xml);
> >
> >       return ret;
> ACK

Well, what about a VIR_CLOSE() to prevent such mistakes in the future.

   Stefan
> Daniel

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list