[ovs-dev] [PATCH repost] dpdk: Refactor and simplify calculating vhost socket dir.

[Darrell Ball]

On 7/7/17, 5:58 AM, "ovs-dev-bounces at openvswitch.org on behalf of Aaron Conole" <ovs-dev-bounces at openvswitch.org on behalf of aconole at redhat.com> wrote:

    Ben Pfaff <blp at ovn.org> writes:
    
    > It took me a few minutes to figure out what the code for deciding on the
    > vhost socket directory was doing, because it was needlessly complicated.
    > This simplifies it.
    >
    > Build tested only.
    >
    > Signed-off-by: Ben Pfaff <blp at ovn.org>
    > ---
    > This is a repost of a patch first posted in April.
    
    Somehow this dropped off my radar.  Sorry.

Maybe worth adding a CC and Fixes

CC: Aaron Conole <aconole at redhat.com>
Fixes: d8a8f353c23ee (“netdev-dpdk: Restrict vhost_sock_dir”)

    
    >  lib/dpdk.c | 64 ++++++++++++++++++++++----------------------------------------
    >  1 file changed, 23 insertions(+), 41 deletions(-)
    >
    > diff --git a/lib/dpdk.c b/lib/dpdk.c
    > index 8da6c324407e..1bd31ba345f5 100644
    > --- a/lib/dpdk.c
    > +++ b/lib/dpdk.c
    > @@ -42,29 +42,29 @@ static FILE *log_stream = NULL;       /* Stream for DPDK log redirection */
    >  
    >  static char *vhost_sock_dir = NULL;   /* Location of vhost-user sockets */
    >  
    > -static int
    > -process_vhost_flags(char *flag, const char *default_val, int size,
    > -                    const struct smap *ovs_other_config,
    > -                    char **new_val)
    > +static char *
    > +get_custom_vhost_sock_dir(const char *stem, const char *suffix)
    >  {
    > -    const char *val;
    > -    int changed = 0;
    > +    if (!suffix) {
    > +        return NULL;
    > +    }
    >  
    > -    val = smap_get(ovs_other_config, flag);
    > +    if (strstr(suffix, "..")) {
    > +        VLOG_ERR("ignoring vhost-sock-dir '%s' with invalid characters '..'",
    > +                 suffix);
    > +        return NULL;
    > +    }
    >  
    > -    /* Process the vhost-sock-dir flag if it is provided, otherwise resort to
    > -     * default value.
    > -     */
    > -    if (val && (strlen(val) <= size)) {
    > -        changed = 1;
    > -        *new_val = xstrdup(val);
    > -        VLOG_INFO("User-provided %s in use: %s", flag, *new_val);
    > -    } else {
    > -        VLOG_INFO("No %s provided - defaulting to %s", flag, default_val);
    > -        *new_val = xstrdup(default_val);
    > +    struct stat s;
    > +    char *dir = xasprintf("%s/%s", stem, suffix);
    > +    if (stat(dir, &s)) {
    > +        VLOG_ERR("%s: ignoring requested vhost socket directory (%s)",
    > +                 dir, ovs_strerror(errno));
    > +        free(dir);
    > +        return NULL;
    >      }
    >  
    > -    return changed;
    > +    return dir;
    >  }
    >  
    >  static char **
    > @@ -311,7 +311,6 @@ dpdk_init__(const struct smap *ovs_other_config)
    >      bool auto_determine = true;
    >      int err = 0;
    >      cpu_set_t cpuset;
    > -    char *sock_dir_subcomponent;
    >  
    >      log_stream = fopencookie(NULL, "w+", dpdk_log_func);
    >      if (log_stream == NULL) {
    > @@ -321,28 +320,11 @@ dpdk_init__(const struct smap *ovs_other_config)
    >          rte_openlog_stream(log_stream);
    >      }
    >  
    > -    if (process_vhost_flags("vhost-sock-dir", ovs_rundir(),
    > -                            NAME_MAX, ovs_other_config,
    > -                            &sock_dir_subcomponent)) {
    > -        struct stat s;
    > -        if (!strstr(sock_dir_subcomponent, "..")) {
    > -            vhost_sock_dir = xasprintf("%s/%s", ovs_rundir(),
    > -                                       sock_dir_subcomponent);
    > -
    > -            err = stat(vhost_sock_dir, &s);
    > -            if (err) {
    > -                VLOG_ERR("vhost-user sock directory '%s' does not exist.",
    > -                         vhost_sock_dir);
    > -            }
    > -        } else {
    > -            vhost_sock_dir = xstrdup(ovs_rundir());
    > -            VLOG_ERR("vhost-user sock directory request '%s/%s' has invalid"
    > -                     "characters '..' - using %s instead.",
    > -                     ovs_rundir(), sock_dir_subcomponent, ovs_rundir());
    > -        }
    > -        free(sock_dir_subcomponent);
    > -    } else {
    > -        vhost_sock_dir = sock_dir_subcomponent;
    > +    vhost_sock_dir = get_custom_vhost_sock_dir(ovs_rundir(),
    > +                                               smap_get(ovs_other_config,
    > +                                                        "vhost-sock-dir"));
    > +    if (!vhost_sock_dir) {
    > +        vhost_sock_dir = xstrdup(ovs_rundir());
    >      }
    
    Because of the way DPDK works, I think we need a VLOG_INFO here
    indicating which path we chose for the vhost socket directory.
    Otherwise, if the user changes the db value and forgets to restart, we
    have no indicator to tell us where to look.  Unless there's some other
    way of knowing.
    
    Otherwise, LGTM.

If it was tested, then I won’t bother.


    
    >  
    >      argv = grow_argv(&argv, 0, 1);
    _______________________________________________
    dev mailing list
    dev at openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=JJMih84adYRvWhtsWpsg8AJNXFDOFJMGJWX-nU3WQNk&s=ZLwfX00i0GIxl4Oe4hlMUZuWcAuFELTxTpZyIbhMF98&e=