Re: [PATCH] Add a progress bar to file downloads.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Aug 11, 2008, at 7:52 AM, Ricky Zhou wrote:

Here's a patch to add progress bars to file downloads.  Like I
mentioned, I'm pretty new to C, so please feel free to tear this apart
and let me know if I'm doing anything in a non-proper way.


The patch looks nice. New to C and you are working with libnewt...I'm very sorry. :)

I have some comments having just read through the patch. Other people might want to add comments:

1) Please use the same indentation style that the rest of the code is using. We are not consistent in the anaconda code, but if you're adding something, make sure you follow the same indentation style.

2) Check the return value of malloc() and handle it appropriately. Again, this is not something that is consistent throughout the code, but it's another thing on my to do list. 'Appropriately' depends on whether or not error recovery is possible. Generally no and if malloc() fails, it's ok to log a message and abort(). Checking for things like this helps finding where things are failing later on. People will say malloc should not fail, which is true, it shouldn't if you want your program to continue. But, it can, and if it does we want to know about it and stop then.

3) alloca() is used a lot in our code, but it's not one of my favorite functions because it's not allowed to fail even though memory resources are finite. I would prefer we use malloc() or calloc() just to gain the capability of checking for the failure on allocation. In my opinion, it's worth the tradeoffs that alloca() would give us.

Other than those things, I like what I see.

--
David Cantrell <dcantrell@xxxxxxxxxx>
Red Hat / Honolulu, HI

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

[Index of Archives]     [Kickstart]     [Fedora Users]     [Fedora Legacy List]     [Fedora Maintainers]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [Yosemite Photos]     [KDE Users]     [Fedora Tools]
  Powered by Linux