[Xorp-hackers] Patches not commited!?
Atanu Ghosh
atanu@ICSI.Berkeley.EDU
Fri, 14 Oct 2005 09:35:05 -0700
Hi,
I would wait to submit a patch until we have refactored the code.
The documentation on TimeVal:
<http://www.xorp.org/releases/current/docs/kdoc/html/libxorp/TimeVal.html>.
We have a draft coding standards that could do with being more prominent:
<http://xorpc.icir.org/cgi-bin/cvsweb.cgi/xorp/devnotes/coding-style.txt>
Some recent change seems to have stopped the bgp show commands from
working. We are looking into this now.
Atanu.
>>>>> "Kristian" == Kristian Larsson <kristian@juniks.net> writes:
Kristian> On Thu, Oct 13, 2005 at 12:16:42AM -0700, Atanu Ghosh
Kristian> wrote:
>> Hi,
>>
>> I believe the code as we wrote it has a design flaw, the fetching
>> of the data about the peer and the printing should not be mixed.
>>
>> I was going to rewrite the code to fetch all the data about the
>> peer and then when it had all been accumulated pass it to
>> different output routines. I believe that would make it easier to
>> manage. I wasn't ignoring your patch I just didn't think that it
>> could be applied as it is.
Kristian> So should I try to clean up my code or is it pointless
Kristian> since you will change the code anyway?
>> I actually applied your patch as I liked the form of the output
>> but a number of things concerned me when I looked at the code.
Kristian> I'm glad you liked it :)
>> This construct writes a zero to an arbitary location.
>> ----------------------------------------
>> char *host; strcpy(host, "");
>> ----------------------------------------
>>
>> Try this:
>> ----------------------------------------
>> main() { char *host = 0; strcpy(host, ""); }
>> ----------------------------------------
>>
>> We have a TimeVal structure that can be used for manipulating
>> time:
>> ----------------------------------------
>> + w = (time - time%604800)/604800; + d = ((time%604800) -
>> (time%604800)%86400)/86400; + h = ((time%604800)%86400 -
>> ((time%604800)%86400)%3600)/3600; + m =
>> (((time%604800)%86400)%3600 -
>> (((time%604800)%86400)%3600)%60)/60; + s =
>> (((time%604800)%86400)%3600)%60;
>> ----------------------------------------
Kristian> Hmm, were do I find docuemntation on the above?
>> There were a bunch of unused variable:
>> ----------------------------------------
>> stringstream ss,tin,tout; string t,in,out;
>> ----------------------------------------
>>
>> There is also mixed usage of cout and stdio I wasn't sure that
>> this would work portably.
Kristian> I think I've seen this in other utilities, but I'm not
Kristian> sure.
>> I should have sent you a commentary earlier but I was hoping to
>> have the time to restructure the code and then adapt your output
>> routine which I really like.
Kristian> Right now it "show bgp peers" doesn't seem to work at all;
Kristian> "
Xorp> show bgp peers
Kristian> Usage: xorpsh_print_peers show bgp peers [detail] where
Kristian> detail enables verbose output. ERROR:
Xorp> "
Kristian> Kristian.
>> Atanu.
>>
>> >>>>> "Kristian" == Kristian Larsson <kristian@juniks.net>
>> writes:
>>
Kristian> On Tue, Oct 11, 2005 at 09:47:59AM -0700, Pavlin
Kristian> Radoslavov wrote:
>> >> > I understand you have quite a lot to do ;) > It seems that
>> not >> many of the developers listed on > the xorp webpage are
>> actually >> active. Perhaps > another person with CVS access
>> could help >> out. And > with that person having bugs/BugZilla as
>> main > focus >> all these small bugs could be resolved very >
>> quickly.
>> >> >
>> >> > Anyway here is a (partial?) list. > 53 > 54 > 64 > 161 >
>> 199 > >> 212 > 217
>> >>
>> >> Thanks for the list. Accidentally, last night I found by
>> accident >> and already fixed the problem described in Bugzilla
>> entry #212, >> so I just marked that FIXED.
>> >>
>> >> Atanu is the one to take care of #217, but recently he is >>
>> overloaded with the OSPF implementation so probably after that he
>> >> will take care of it.
>> >>
>> >> Note that in general even if there is a patch for a problem it
>> is >> not just a question of applying that patch. We need to
>> verify it >> and modify it if necessary (e.g., to match the XORP
>> >> codingguidelines, etc). Occasionally, we need to think about
>> the >> high-level impact of a solution. A not so good example
>> could be >> whether we want to have a special template that
>> defines the >> header of the XORP configuration files or whether
>> the format of >> that header would be hardcoded inside the *.cc
>> files.
>> >>
>> >> Anyway, I created a note for myself with the above bugzilla >>
>> entries so I will try to address them sooner than later (though
>> >> note that some of them may have to be postponed until after
>> the >> release because of the significant impact they have on the
>> >> system).
>> >>
>> >> Unfortunately, it is quite hard to have a person that
>> addresses >> all the bugzilla entries, because typically fixing a
>> bug requires >> deep understanding of how the particular module
>> works. E.g., I >> deal with multicast bugs, Atanu deals with BGP
>> bugs, etc.
>>
Kristian> Valid point ;) I was thinking of all the small trivial
Kristian> fixes. Somehow I feel they should be given some form of
Kristian> priority. If someone sends a patch to BZ I think it's
Kristian> important that the patch is at least looked over.
>>
Kristian> Perhaps an addition to BugZilla which makes it possible to
Kristian> search for bugs with patches. This way you could search
Kristian> for patched bugs ones a week, verify and commit :)
>>
Kristian> Kristian. _______________________________________________
Kristian> Xorp-hackers mailing list Xorp-hackers@icir.org
Kristian> http://mailman.ICSI.Berkeley.EDU/mailman/listinfo/xorp-hackers