On 05/05/2010 02:51 AM, David Cantrell wrote:
> On Wed, 5 May 2010, Steffen Maier wrote:
>> On 05/04/2010 11:40 PM, David Cantrell wrote:
>>> ---
>>> utils/geninitrdsz.c | 51
>>> +++++++++++++++++++++++++++++++++++----------------
>>> 1 files changed, 35 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/utils/geninitrdsz.c b/utils/geninitrdsz.c
>>> index 6dfd976..b8c824a 100644
>>> --- a/utils/geninitrdsz.c
>>> +++ b/utils/geninitrdsz.c
>>> @@ -1,11 +1,12 @@
>>> /*
>>> - * geninitrdsz.c
>>> - * Generate initrd.size file for zSeries platforms.
>>> + * gen-initrd-addr.c
>>
>> * gen-initrd-patch.c
>>
>>> + * Generate initrd.addr file for s390x platforms.
>>
>> * Generate initrd.patch file for s390x platforms.
>
> Does it really matter if it's called initrd.addr or initrd.patch? Or even
> initrd.size for that matter? I used initrd.addr thinking that it might
> be a
> good idea for people to not mistake this file for the output of diff(1).
Agreed that a .patch suffix could be mixed up with textual diffs.
I don't like .addr (and .size neither). It is misleading because the
generated file does not just contain the load address but also the
initrd size, the latter of which we've always had and still need. Since
the generated file is actually a binary patch, how about
"initrd.binpatch" or "initrd.addrsize"?
>>> @@ -33,27 +34,45 @@
>>> #include <string.h>
>>>
>>> int main(int argc,char **argv) {
>>> if (argc != 3) {
>>
>> if (argc != 4) {
>
> The create_initrd_patch.c file attached to comment #31 says:
>
> if (argc != 3) {
I know. The tool as provided by IBM is fully functional. It takes two
arguments, a load address integer and a file name for initrd to
determine its file size by means of the stat syscall. Our tool would
then write out a file "initrd.patch" to the current working directory.
You started to adapt the tool to the anaconda build scripts environment
which is perfectly fine. However, you introduced bugs. I was just trying
to help you fix those bugs and thus help you with your integration.
>>> + return EXIT_SUCCESS;
>>
>> I know our tool had this with zero, but how can calling the tool
>> incorrectly exit with a successful error level?
>
> Because it's just printing the usage information?
I would understand this, if the user called it with --help or the like.
However, here we print usage only on incorrect calls which are IMHO
errors. Granted, in comparison to the other issues that's nit picking.
>>> + fd = open(argv[2], O_CREAT | O_RDWR, S_IRUSR | S_IWUSR);
>>
>> fd = open(argv[3], O_CREAT | O_RDWR, S_IRUSR | S_IWUSR);
>>
>> In order to be able to specify all those funny absolute paths on
>> mk-images, we want to write to the outfile in the 3rd command line
>> argument and not overwrite our precious input file initrd.img.
>
> That's fine, but how about getting it correct in the version of
> create_initrd_patch.c that you attach to the BZ?
Our tool works perfectly fine. If you need to adapt it to your
distro-specific build environment you can do that.
>>> + if (write(fd, &zero, sizeof(int)) == -1) {
>>> + perror("writing initrd.addr (zero) ");
>>> + return errno;
>>
>> perror("writing output patch file (first zero) ");
>> return 2;
>>
>>> + }
>>> +
>>> + if (write(fd, &addr, sizeof(int)) == -1) {
>>> + perror("writing initrd.addr (zero) ");
>>> + return errno;
>>
>> perror("writing output patch file (address) ");
>> return 3;
>>
>>> + }
> The return code changes, fine, but again, those were not in
> create_initrd_patch.c. In fact, the version attached to comment #31 did
> not
> check the return value of write().
I'm just saying that if we care for errno, which we should, then we
should do it right(TM). Using errno for the exit error level just seemed
wrong to me considering that the exit status can be between 0 and 255
but errno could be anything. This is what the glibc info doc says:
"For the same reason, it does not work to use the value of `errno' as
the exit status--these can exceed 255."
> It appears the version of create_initrd_patch.c you know and love is not
> the same as what was attached to the BZ.
I only know the old geninitrdsz, our new tools proposed in the BZ, and
your adaptations. I tried to help you and get the latter two in sync.
> There were minimal changes between
> geninitrdsz.c and create_initrd_patch.c, which is why I merged in the
> changes
> and renamed the file in anaconda's source as opposed to just taking
> create_initrd_patch.c as-is.
Adapting the tool is fine. However, there were major changes between
geninitrdsz and our new tool.
> Before running at me with pitchforks in the future, could you make sure the
> story in the BZ matches what you're going by?
My code review comments match both the BZ comments as well as your
adaptations.
Steffen
Linux on System z Development
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martin Jetter
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
