[ovs-dev] [PATCH 2/2] vlog: Add vlog/close command.

Ben Pfaff blp at ovn.org
Wed Feb 3 21:46:50 UTC 2016


On Wed, Jan 27, 2016 at 04:57:41PM -0500, Russell Bryant wrote:
> On 12/08/2015 01:59 PM, Ben Pfaff wrote:
> > Requested-by: P R Dinesh
> > Requested-at: https://github.com/openvswitch/ovs/pull/94
> > Signed-off-by: Ben Pfaff <blp at ovn.org>
> 
> Acked-by: Russell Bryant <russell at ovn.org>

Thanks!  I'm going to send a revision of this as part of a longer
series.

> >  static void
> > +vlog_unixctl_close(struct unixctl_conn *conn, int argc OVS_UNUSED,
> > +                   const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> > +{
> > +    ovs_mutex_lock(&log_file_mutex);
> > +    if (log_fd >= 0) {
> > +        close(log_fd);
> > +        log_fd = -1;
> > +
> > +        async_append_destroy(log_writer);
> > +        log_writer = NULL;
> > +
> > +        struct vlog_module *mp;
> > +        LIST_FOR_EACH (mp, list, &vlog_modules) {
> > +            update_min_level(mp);
> > +        }
> > +    }
> > +    ovs_mutex_unlock(&log_file_mutex);
> 
> This got me looking around at where log_file_mutex is used.  One thing I
> noticed was that it seems like it should be locked in
> vlog_insert_module().  In pratice it doesn't matter since it's only
> called in global constructors at startup.  Maybe it makes sense in case
> someone decides to call vlog_insert_module() dynamically for some reason.
> 
> If we do that, maybe the lock should be held in vlog_module_from_name(),
> as well.
> 
> Again, not a real issue today, but I'm curious what you think ...

Fair enough.  I'll send a separate patch.

> The two tests are *very* similar.  It'd be nice to share most of this as
> is done in other parts of the test suit, but it's not that big of a
> deal.  The minor differences might make it not worth the trouble.

At the moment I don't see much benefit, so I'm inclined to leave it.



More information about the dev mailing list