Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OSPF improvements #9909

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@dsgagi
Copy link
Contributor

dsgagi commented Mar 5, 2019

  • Fixed ospf port listing, as current query only returned ospf ports for last AreaID in area foreach loop.
  • Added Area ID column to Ports table.
  • Removed whitespaces.

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

Update ospf.inc.php
- Fixed ospf port listing, as current query only returned ospf ports for last AreaID in `area` foreach loop.
- Added `Area ID` column to Ports table.
- Removed whitespaces.
@murrant

This comment has been minimized.

Copy link
Member

murrant commented Mar 6, 2019

Doesn't this make the outer foreach useless?

You grab all the ospf entries every loop...

I could be wrong as this code isn't the easiest to follow.

@dsgagi

This comment has been minimized.

Copy link
Contributor Author

dsgagi commented Mar 6, 2019

Do you mean area or instance foreach loop?

I agree, the code is not so easy to follow :)

Each ospf instance has it's own hidden row (with areas, ports and neighbor columns). I guess it's coded this way for rare cases when there are more ospf instances/processes running on the device.

As for area loop, it doesn't include ospfport loop. It includes only the code below.

foreach (dbFetchRows('SELECT * FROM `ospf_areas` WHERE `device_id` = ?', array($device['device_id'])) as $area) {
        $area_port_count         = dbFetchCell('SELECT COUNT(*) FROM `ospf_ports` WHERE `device_id` = ? AND `ospfIfAreaId` = ?', array($device['device_id'], $area['ospfAreaId']));
        $area_port_count_enabled = dbFetchCell("SELECT COUNT(*) FROM `ospf_ports` WHERE `ospfIfAdminStat` = 'enabled' AND `device_id` = ? AND `ospfIfAreaId` = ?", array($device['device_id'], $area['ospfAreaId']));

        echo '
                      <tbody>
                        <tr>
                          <td>' . $area['ospfAreaId'] . '</td>
                          <td>' . $area_port_count . '(' . $area_port_count_enabled . ')</td>
                          <td><span class="label label-' . $status_color . '">' . $instance['ospfAdminStat'] . '</span></td>
                        </tr>
                      </tbody>';
}

@murrant murrant changed the title Update ospf.inc.php OSPF improvements Mar 12, 2019

</tr>
</thead>
</div>';
foreach (dbFetchRows("SELECT * FROM `ospf_ports` AS O, `ports` AS P WHERE O.`ospfIfAdminStat` = 'enabled' AND O.`device_id` = ? AND O.`ospfIfAreaId` = ? AND P.port_id = O.port_id", array($device['device_id'], $area['ospfAreaId'])) as $ospfport) {

This comment has been minimized.

Copy link
@laf

laf Apr 9, 2019

Member

Don't you still want O.ospfIfAreaId here still? Otherwise all the ospf data is selected again.

This comment has been minimized.

Copy link
@dsgagi

dsgagi Apr 9, 2019

Author Contributor

Actually, this foreach loop is not included inside the foreach loop for the $area variable.

The end result is that port info is generated only for one ospf area (last one from the loop before).

@murrant

This comment has been minimized.

Copy link
Member

murrant commented Apr 12, 2019

Web files have been moved to includes/html, you will need to update your PR, sorry for the trouble. Let us know if you need any help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.