LVS
lvs-devel
Google
 
Web LinuxVirtualServer.org

Re: [PATCH 2/2] IPVS: Add genetlink interface implementation

To: Julius Volz <juliusv@xxxxxxxxxx>
Subject: Re: [PATCH 2/2] IPVS: Add genetlink interface implementation
Cc: netdev@xxxxxxxxxxxxxxx, lvs-devel@xxxxxxxxxxxxxxx, vbusam@xxxxxxxxxx, horms@xxxxxxxxxxxx, davem@xxxxxxxxxxxxx
From: Patrick McHardy <kaber@xxxxxxxxx>
Date: Wed, 09 Jul 2008 18:43:40 +0200
Julius Volz wrote:
Add the implementation of the new Generic Netlink interface to IPVS and keep
the old set/getsockopt interface for userspace backwards compatibility.

Signed-off-by: Julius Volz <juliusv@xxxxxxxxxx>

Just a few quick comments, will try to do a full review later:

+ * Policy used for commands that operate on service, destination
+ * or daemon entries
+ */
+static struct nla_policy ip_vs_entries_policy[IPVS_ENTRY_ATTR_MAX + 1]
+__read_mostly = {

These can all be const (and have the __read_mostly annotation removed).

+static int ip_vs_genl_fill_stats(struct sk_buff *skb, int container_type,
+                                struct ip_vs_stats *stats)
+{
+       struct nlattr *nl_stats = nla_nest_start(skb, container_type);
+       if (!nl_stats)
+               goto nla_put_failure;

Unbalanced locking.

+
+       spin_lock_bh(&stats->lock);
+
+       NLA_PUT_U32(skb, IPVS_STATS_ATTR_CONNS, stats->conns);
+       NLA_PUT_U32(skb, IPVS_STATS_ATTR_INPKTS, stats->inpkts);
+       NLA_PUT_U32(skb, IPVS_STATS_ATTR_OUTPKTS, stats->outpkts);
+       NLA_PUT_U64(skb, IPVS_STATS_ATTR_INBYTES, stats->inbytes);
+       NLA_PUT_U64(skb, IPVS_STATS_ATTR_OUTBYTES, stats->outbytes);
+       NLA_PUT_U32(skb, IPVS_STATS_ATTR_CPS, stats->cps);
+       NLA_PUT_U32(skb, IPVS_STATS_ATTR_INPPS, stats->inpps);
+       NLA_PUT_U32(skb, IPVS_STATS_ATTR_OUTPPS, stats->outpps);
+       NLA_PUT_U32(skb, IPVS_STATS_ATTR_INBPS, stats->inbps);
+       NLA_PUT_U32(skb, IPVS_STATS_ATTR_OUTBPS, stats->outbps);
+
+       spin_unlock_bh(&stats->lock);
+
+       nla_nest_end(skb, nl_stats);
+
+       return 0;
+
+nla_put_failure:
+       spin_unlock_bh(&stats->lock);
+
+       nla_nest_cancel(skb, nl_stats);
+       return -EMSGSIZE;
+}
+
+static int ip_vs_genl_set_cmd(struct sk_buff *skb, struct genl_info *info)
+{
+       struct ip_vs_service *svc;
+       struct ip_vs_service_user usvc;
+       struct ip_vs_dest_user udest;
+       int ret = 0, cmd, flags;
+       int need_full_svc = 0, need_full_dest = 0;
+
+       cmd = info->genlhdr->cmd;
+       flags = info->nlhdr->nlmsg_flags;
+
+       /* increase the module use count */
+       ip_vs_use_count_inc();

This looks fishy - the reference probably must be taken by
genetlink before calling the command handler.

+int ip_vs_genl_register(void)
+{
+       int ret, i;
+
+       ret = genl_register_family(&ip_vs_genl_family);
+       if (ret)
+               return ret;
+
+       for (i = 0; i < ARRAY_SIZE(ip_vs_genl_ops); i++) {
+               ret = genl_register_ops(&ip_vs_genl_family, &ip_vs_genl_ops[i]);
+               if (ret)
+                       goto err_out;
+       }
+       return 0;
+
+err_out:
+       genl_unregister_family(&ip_vs_genl_family);
+       return ret;
+}
+
+void ip_vs_genl_unregister(void)
+{
+       genl_unregister_family(&ip_vs_genl_family);

Doesn't it also has to unregister the ops?
--
To unsubscribe from this list: send the line "unsubscribe lvs-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

<Prev in Thread] Current Thread [Next in Thread>