search

Telecominfraproject / ols-ucentral-schema

OpenLAN/Shasta contribution repo
BSD 3-Clause "New" or "Revised" License
1 stars 0 forks source link

The ols-ucentral-client src/ucentral-client/proto.c depends on the et… #3

Closed mikehansen1970 closed 8 months ago

mikehansen1970 commented 9 months ago

…hernet port speed and duplex values being present, even if the port itself is not enabled:

            enabled =
                    cJSON_IsTrue(cJSON_GetObjectItemCaseSensitive(eth, "enabled"));
            duplex =
                    cJSON_GetStringValue(cJSON_GetObjectItemCaseSensitive(eth, "duplex"));
            speed =
                    cJSON_GetNumberValue(cJSON_GetObjectItemCaseSensitive(eth, "speed"));

            if (!duplex || !speed || !select_ports) {
                    UC_LOG_ERR("Ethernet obj doesn't hold duplex, speed or select-ports fields, parse failed\n");
                    return -1;
            }

While personally I believe it might be better not to care about speed and duplex setting for a port that is disabled, if they are required, it makes sense to define defaults for them in the schema, as it is highly likely that any operator configuring a switch would not think it necessary to define these values on disabled physical ports.

Speed is defaulted to 1000 Duplex is defaulted to full Signed-off-by: Mike Hansen mike.hansen@netexperience.com

Cahb commented 9 months ago

Hello @mikehansen1970! Thanks for the changes and detailed explanation;

I personally think the PR is fine, however a few notes:

  1. You are right that disabled port's speed/duplex should not be taken into account, it's a change to consider;
  2. I think it makes sense to default speed and duplex, however I'm unaware whether there's any openwifi compatible hardware that is capped (due to hw limitations) to fast ethernet for example (cannot check right now, as i cannot login into telecominfraproject / atlassian to check compatible HW list); if there's at least a single AP or for example there's a plan to support such hw (max speed 100 mbit/s), i think it could intoduce some issues when handling this cfg on such devices;
  3. The reason for original behavior in OLS client, is that cfg is a complete configuration that has to be applied: if GW omitted speed / duplex, it means (at least from the perspective of the original design) that config is invalid; e.g. GW is trying to configure port, but does this incompletely; There's also an option to omit the port from config at all, which would render port be disabled by design;

Also, are you aware by any chance what's the APs behavior in such case? Do they simply skip the duplex/speed in case if port's configured to go admin down?