[Xorp-hackers] Patches not commited!?

[Kristian Larsson]

On Thu, Oct 13, 2005 at 12:16:42AM -0700, Atanu Ghosh 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.
So should I try to clean up my code or is it
pointless 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.
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;
> ----------------------------------------
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.
I think I've seen this in other utilities, but I'm
not 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.


Right now it "show bgp peers" doesn't seem to work
at all; "
 Xorp> show bgp peers 
 Usage: xorpsh_print_peers show bgp peers [detail]
 where detail enables verbose output.
 ERROR:
 Xorp> "

  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> _______________________________________________
>     Kristian> Xorp-hackers mailing list Xorp-hackers@icir.org
>     Kristian> http://mailman.ICSI.Berkeley.EDU/mailman/listinfo/xorp-hackers