[ovs-dev] [PATCH] Remove restriction on socket name

Ben Pfaff blp at nicira.com
Tue Jan 15 05:35:12 UTC 2013


Thanks for the revised patch.  I have a number of comments, mostly
process related.

"git am" noticed trailing whitespace:
    /home/blp/nicira/ovs/.git/rebase-apply/patch:21: trailing whitespace.

    warning: 1 line adds whitespace errors.

> Subject: [PATCH] Remove restriction on socket name

We usually start the subject of the commit message with a tag that
indicates the general piece of code being modified, and end it with a
period.  For example: "bridge: Remove restriction on socket name."

> From: Pavithra Ramesh <paramesh at vmware.com>

The "From:" line here indicates that "git send-email" think the author
is different from the person sending the email.  I guess that you should
set sendemail.from in your .gitconfig, like this:

[sendemail]
        from = Pavithra Ramesh <paramesh at vmware.com>

and then git send-email will not insert the From: line.

> Following patch removes restriction on the listening socket name that gets configured as bridge controller.
> Currently, we only connect to sockets in a specific directory with the name of the bridge.
> This patch removes the restriction on the bridge name (but keep the directory restriction).
> Bug #14029

The lines in the commit description are still very long.  There should
be a blank line between the description and the Bug # line.  The
Signed-off-by line is still missing.  Here is the information from
SubmittingPatches on Signed-off-by:

Please sign off on the patch as a submitter, and be sure to have the
author(s) sign off for patches that you did not author.

Simply include your name and email address as the last line of the commit
message before any comments (and author too, if that is not you):

Signed-off-by: Author Name <author.name at email.address...>
Signed-off-by: Submitter Name <submitter.name at email.address...>

By doing this, you are agreeing to the Developer's Certificate of Origin
(see below for more details).

Developer's Certificate of Origin
---------------------------------

To help track the author of a patch as well as the submission chain,
and be clear that the developer has authority to submit a patch for
inclusion in openvswitch please sign off your work.  The sign off
certifies the following:

    Developer's Certificate of Origin 1.1

    By making a contribution to this project, I certify that:

    (a) The contribution was created in whole or in part by me and I
        have the right to submit it under the open source license
        indicated in the file; or

    (b) The contribution is based upon previous work that, to the best
        of my knowledge, is covered under an appropriate open source
        license and I have the right under that license to submit that
        work with modifications, whether created in whole or in part
        by me, under the same open source license (unless I am
        permitted to submit under a different license), as indicated
        in the file; or

    (c) The contribution was provided directly to me by some other
        person who certified (a), (b) or (c) and I have not modified
        it.

    (d) I understand and agree that this project and the contribution
        are public and that a record of the contribution (including all
        personal information I submit with it, including my sign-off) is
        maintained indefinitely and may be redistributed consistent with
        this project or the open source license(s) involved.

> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 348faef..7c610cb 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -2792,21 +2792,32 @@ bridge_configure_remotes(struct bridge *br,
>              static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>              char *whitelist;
>  
> -            whitelist = xasprintf("unix:%s/%s.controller",
> +            /* Target is a listening socket */

I don't understand what this comment is saying.  This code is reached
whether the target is a listening (passive) socket or an active
(outgoing) socket.

> +            if (!strncmp(c->target, "unix:", 5)) {
> +               whitelist = xasprintf("unix:%s/",
> +                                     ovs_rundir());

I believe that the above two lines may be combined into one without
exceeding the 79-column limit.

> +               if(strncmp(c->target, whitelist, strlen(whitelist))) {

Missing space after "if".

> +                  goto error;
> +               }
> +   

Unusual blank line above.

> +            } else {
> +               whitelist = xasprintf("punix:%s/%s.controller",
> +                                     ovs_rundir(), br->name);
> +               if (!equal_pathnames(c->target, whitelist)) {
> +                   /* Prevent remote ovsdb-server users from accessing arbitrary
> +                    * Unix domain sockets and overwriting arbitrary local
> +                    * files. */
> +                   error:
> +                      VLOG_ERR_RL(&rl, "bridge %s: Not adding Unix domain socket "
> +                                  "controller \"%s\" due to possibility for remote "
> +                                  "exploit.  Instead, specify whitelisted \"%s\" or "
> +                                  "connect to \"unix:%s/%s.mgmt\" (which is always "
> +                                  "available without special configuration).",
> +                                  br->name, c->target, whitelist,
>                                    ovs_rundir(), br->name);
> -            if (!equal_pathnames(c->target, whitelist)) {
> -                /* Prevent remote ovsdb-server users from accessing arbitrary
> -                 * Unix domain sockets and overwriting arbitrary local
> -                 * files. */
> -                VLOG_ERR_RL(&rl, "bridge %s: Not adding Unix domain socket "
> -                            "controller \"%s\" due to possibility for remote "
> -                            "exploit.  Instead, specify whitelisted \"%s\" or "
> -                            "connect to \"unix:%s/%s.mgmt\" (which is always "
> -                            "available without special configuration).",
> -                            br->name, c->target, whitelist,
> -                            ovs_rundir(), br->name);
> -                free(whitelist);
> -                continue;
> +                   free(whitelist);
> +                   continue;
> +               }
>              }
>  
>              free(whitelist);

I believe that after this commit the log message is no longer completely
accurate.  It might be best to now use a different log message for
active and passive targets.

Thanks,

Ben.



More information about the dev mailing list