Hi Vipin,
Thanks for your great job on the multi-instance patch! The patch is
globally fine to me.
I just post some minor opinions on the patch:
(1) u_short instance
The instance ID is defined as an unsigned short value. It seems enough
for now, while I don't know whether it would be proper in the future
or for some special reason.
Is it more better to use u_int32_t?
Or at least, I suggest to add a type definition in zebra.h like:
typedef u_int32_t inst_t;
to avoid changing the code everywhere if we would need to change the
type.
(2) instance in ZAPI message
Since the ZAPI message format is changed, ZSERV_VERSION then must be
increased.
(3) changes of the libzebra APIs
Following is the comments from Paul to 6WIND VRF patches:
"Well, ideally, avoid incompatible changes to existing functions (e.g.
add new API). Otherwise, bump the version."
I think you also need it. :)
(4) the instance ID 0
I see in many places 0 is propagated into functions as the instance
ID value. Is it better to use a meaningful name like INSTANCE_XXX
instead of a number 0?
(5)
+void
+redist_add_instance (struct redist_proto *red, u_short instance)
+{
+ u_short *in;
+
+ red->enabled = 1;
+
+ if (!red->instances)
+ red->instances = list_new();
+
+ in = (u_short *)calloc(1, sizeof(u_short));
+ *in = instance;
+ listnode_add (red->instances, in);
+}
The redist flag is changed to a list for multiple instances. It
allocates 2 bytes for each instance.
Is it better to make use of the pointer itself? i.e.
+void
+redist_add_instance (struct redist_proto *red, u_short instance)
+{
+ red->enabled = 1;
+
+ if (!red->instances)
+ red->instances = list_new();
+
+ listnode_add (red->instances, (void *)instance);
+}
(6)
In redist_del_instance(), the memory allocated in redist_add_instance()
is not freed.
(7)
/* Configuration filename and directory. */
-char config_default[] = SYSCONFDIR OSPF_DEFAULT_CONFIG;
+char config_default[100];
/* Process ID saved for use by init system */
-const char *pid_file = PATH_OSPFD_PID;
+char pid_file[100];
+ char vty_path[100];
There should be a better way to define the string length than a hard-
coding value 100. For example:
char pid_file[2 * sizeof (PATH_OSPFD_PID)] = PATH_OSPFD_PID;
(8)
switch (opt)
{
+ case 'n':
+ instance = atoi(optarg);
+ if (instance < 1 || instance > 65535)
+ exit(0);
+ break;
I think the test "if (instance < 1 || instance > 65535)" is not proper
enough. Thinking of the fact that "instance" is an u_short value, that
test is equivalent to "if (instance == 0)". It can not filter out the
value higher than 65535 properly. For example:
ospfd -d -n 131071 // 0x1ffff
the incorrect instance ID 131071 will be handled as 65535. I suggest
to use an integer, like:
+int inst = 0;
switch (opt)
{
+ case 'n':
+ inst = atoi(optarg);
+ if (inst < 1 || inst > 65535)
+ exit(0);
+ instance = (u_short)inst;
+ break;
(9)
+ /* Create VTY socket */
+ if (instance)
+ {
+ sprintf(pid_file, "/var/run/quagga/ospfd-%d.pid", instance);
+ sprintf(vty_path, "/var/run/quagga/ospfd-%d.vty", instance);
+ }
Please make the proper use of PATH_OSPFD_PID and OSPF_VTYSH_PATH
instead of hard-coding the pathname.
(10)
- vty_serv_sock (vty_addr, vty_port, OSPF_VTYSH_PATH);
+ vty_serv_sock (vty_addr, vty_port, vty_path);
The "vty_port" is not adjusted according to the instance. Please give
an offset to the port, otherwise telnet will not work.
(11)
@@ -45,6 +45,7 @@ ospf_route_map_update (const char *name)
{
struct ospf *ospf;
int type;
+ u_short instance; // PENDING
What does this mean? It seems the variable is not used in the function.
(12)
+ospf_lookup_instance (u_short instance)
+{
+ ...
+
+ for (ALL_LIST_ELEMENTS (om->ospf, node, nnode, ospf))
+ if ((ospf->instance == 0 && instance == 0)
+ || (ospf->instance && instance && ospf->instance == instance))
+ return ospf;
+
+ return NULL;
+}
Why not just "if (ospf->instance == instance)" ?
(13)
In vtysh.c:
+#define MAXIMUM_INSTANCES 10
A dynamic value during runtime seems more acceptable instead of a
hard-coded limitation, according to David and Paul's comments to
the VRF patch (for VRF_MAX).
I think it should be the same for MAXIMUM_INSTANCES.
(14)
In vtysh.c:
+ /* ls /var/run/quagga/ and look for all files ending in .vty */
+ dir = opendir("/var/run/quagga/");
...
+ fprintf(stderr,
+ "Parsing /var/run/quagga/, client limit(%d) reached!\n", n);
+ break;
...
+ sprintf(client->path, "/var/run/quagga/%s", file->d_name);
Please make the proper use of PATH_OSPFD_PID and OSPF_VTYSH_PATH
instead of hard-coding the pathname.
(15)
Each protocol has a default distance for its routes. The distance decides
which route is preferred among the protocols.
For multiple OSPF instances, the default distance is the same value.
Then how to decide the preferred route in case that the distance is
not configured for the instances?
Best regards,
Feng Lu
_______________________________________________
Quagga-dev mailing list
[email protected]
https://lists.quagga.net/mailman/listinfo/quagga-dev
