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 <[email protected]>
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 <[email protected]>
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 <[email protected]...>
Signed-off-by: Submitter Name <[email protected]...>
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.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
