tomato.js bugs/issues -- let's clean this up

Discussion in 'Tomato Firmware' started by koitsu, Apr 8, 2013.

  1. koitsu

    koitsu Network Guru Member

    The more time I've been spending with the Tomato/TomatoUSB code, the more bugs/issues I keep finding. The most recent one I found is shown here:


    The fixIP() function, called by v_ip(), which splits a dotted-string (e.g. IP or FQDN) up into its parts and stores the results in an array, then iterates over each array member, turns the value into a string by performing an arithmetic operation on it (...), and then checks to see if the result is a number (value has to be between 0-255, and has to pass isNaN()). The reason this is silly/stupid is that other functions clearly use regex strings (via RegExp), so why not just validate using^\d+\.\d+\.\d+\.\d+$/); and be done with it.

    Oh, and don't forget about IPv6... *shakes head*

    All these functions that try to do input validation for a hostname vs. IP, in my opinion, need to be removed. If some underlying program/feature requires an IP address in dotted-quad format rather than accepting an FQDN, that's perfectly fine (use the above check) -- but for FQDNs there's really no way to validate them aside from a DNS lookup (and even that might not be wise, particularly for short hostnames (non-FQ)). I say all of this knowing quite well the importance of input validation (especially when involving SQL), but I imagine a lot of this is just totally superfluous.


    tomato.js is a whopping/immense 60KBytes, and with virtually no comments. A lot of variables/etc. are also single-letter, which makes things very difficult to follow. I'm still trying to wrap my head about functions like E() and W(). Ugh.

    Also, I found that the version in EasyTomato is significantly better formatted -- it's been tabbed/indented for readability, while the version in Toastman hasn't. If someone tries to argue that removal of indentation means less bytes thus less data to transfer, I'll punch them in the neck. :p

    I'm not a fan of Javascript in general, mainly because it gets abused heavily these days (meaning used for all the wrong things), so I have to sift through all this junk, and it's a huge time sink.
  2. Mangix

    Mangix Networkin' Nut Member

    The tomato codebase is full of hacks built upon hacks. When you have a project where multiple people contribute in parts, this is the inevitable result. John the Ripper is probably the best example of this. The code is downright painful to read in the community enhanced version.

    Deindenting could probably be done as part of a makefile. As I've seen shibby's <4MB builds grow to above flashable levels, saving space is probably necessary.
  3. Monk E. Boy

    Monk E. Boy Network Guru Member

    Indenting code doesn't result in larger executables. The output file is the same size, because the actual code in the file is the same size.

    Cleaning up code to make it easier to read/interpret, that can make for larger executables... but it sounds like some of what he's proposed would make the firmware smaller by removing unnecessary conversions.
  4. jerrm

    jerrm Network Guru Member

    Validation routines that try to be too smart have always annoyed me. It's even worse when they are too dumb to do what they are attempting.
  5. RMerlin

    RMerlin Network Guru Member

    He's talking Javascript here tho, not source code.

    I know that Asus does "compress" the webui code on some of their lower-end devices to save up space. I never looked however to see if it was integrated in the build environment or something they did manually for those specific devices.

    A valid question now would be: should the existing webui code be cleaned up, or should this be seen as an opportunity to rewrite it, and get rid of that oddly licensed webui code? I know there has been some replacements being developped lately, but they too had their share of problems. An "official" replacement would probably need to be as lightweight as it currently is, with as little dependencies on external libraries as possible. jQuery should ideally be the only external library getting used.
  6. jerrm

    jerrm Network Guru Member

    There is no compiled executable here - it's javascript, so formatting will impact distribution size.


    I agree this is a bad place to cut corners. After compression, I doubt there would be an significant increase in the image size.

    If consensus is we absolutely must minimize the size of the distributed file, I'd say keep a well formatted version in the source tree and use one of the javascript "compressors" to have a stripped down version in the image for smaller build targets.
  7. Monk E. Boy

    Monk E. Boy Network Guru Member

    D'oh, yeah, I should've caught onto the .js

    That's what I get for not drinking my coffee on Monday morning.
  8. jerrm

    jerrm Network Guru Member

    For Tomato, a proper UI rewrite would really need to start with tomato's httpd if you want to start addressing licensing issues, and go on from there. Agree it would be nice, but probably only really warranted if someone gets serious about "porting" tomato without encumbrances to a newer platform.
  9. Toastman

    Toastman Super Moderator Staff Member Member

    If you change the GUI, it isn't tomato any more. Jonathan built Tomato with very specific ideals, the GUI was the most important part of this. Please guys, respect his wishes. If you want to change everything, you can't call it Tomato.

    While on this subject, If you add something to a persons build and push it to git or release it, don't use the original modder's release name or build number/series - then he isn't going to be held responsible for your builds or issues when things don't work. Use your own name, or wait until a modder wishes to include your contribution in his own releases. I do hope you will all understand the need for this. It's common courtesy...

    Much of the tomato code is scrappily indented. It was like that several years ago from the Linksys source. I used to tidy up files that were used regularly in my branch but every time code was merged from Teddy or anyone else, it trashed the formatting again. We all just got tired of trying to keep it tidy. The problem was compounded by different text editors displaying visually different formatting.

    I can tell you, if someone really wants to do a code tidy-up, it will not be as simple as you may think.
    mstombs and pharma like this.
  10. Victek

    Victek Network Guru Member

    Agree .. it's not a webui question or indent ... buildroot is the begining and 'porting' opportunity for a better tomato.
    pharma and Elfew like this.
  11. koitsu

    koitsu Network Guru Member

  12. Victek

    Victek Network Guru Member

    Agree, ternary added ;), Thanks.
  13. Kevin Darbyshire-Bryant

    Kevin Darbyshire-Bryant Networkin' Nut Member

    Commit 735da6fd5601363bedd214c371f91944188c79bc for koitsu
  14. blackwind

    blackwind Networkin' Nut Member

  15. koitsu

    koitsu Network Guru Member

    I'm not so sure setting the cookie expiry to year 2038 is ideal, like I said in the thread. That is purely a "personal" opinion/solution -- I honestly feel they should expire after 365 days from their initial creation. I fully agree the existing method/model used is utterly nonsensical, but I do not agree that a static expiry of the epoch is a wise choice.
  16. blackwind

    blackwind Networkin' Nut Member

    Feel free to rewrite the patch as you see fit, then -- let's just get it done and committed. We know the solution, so let's not sideline it for the next several years like so many open source projects routinely do because of negligible implementation quibbles.
  17. Toastman

    Toastman Super Moderator Staff Member Member

    can we agree on a final change? I will add this in the next few days.
  18. koitsu

    koitsu Network Guru Member

    I've done some reading on this -- there are actually more caveats to having the cookies expire at N days/weeks/months/years in advance (which is what I propose) than just a hard-coded value. I'll discuss this in the other thread. If blackwind can answer the questions I have, then I think we can reach an agreement (with a slight modification to the timestamp he proposed, but I'm about 99% certain he'll be okay with that). Edit: Figured out the answer myself. I updated the thread anyway. :)
  19. mstombs

    mstombs Network Guru Member

    Are you considering Shibby's language changeable version - all user visible string strings put into a swappavle dictionary?
  20. Victek

    Victek Network Guru Member

    mstombs, I started with shibby in 2010 the language dictionary by some users request (LATAM, Poland). Today I don't receive any request... ;)
  21. Elfew

    Elfew Network Guru Member

    I can translate it to Czech, np if you want
  1. This site uses cookies to help personalise content, tailor your experience and to keep you logged in if you register.
    By continuing to use this site, you are consenting to our use of cookies.
    Dismiss Notice