[Bug 1101521] Review Request: geomorph - A height field editor for Linux

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

 



https://bugzilla.redhat.com/show_bug.cgi?id=1101521



--- Comment #19 from Lubomir Rintel <lkundrak@xxxxx> ---
(In reply to Ralf Corsepius from comment #17)
> (In reply to Lubomir Rintel from comment #13)
> > (In reply to Ralf Corsepius from comment #12)
> > > (In reply to Lubomir Rintel from comment #10)
> 
> > That said, this is solemnly packager's responsibility.
> No. One of the basic prerequisites of a package to be packaged for Fedora is
> "function". It's one of the core purposes of a review to asure this.
> 
> > If he's able to cope
> > with the issues (and it seems to me he is)
> I can't judge - The packager is a new-comer. He may-be, he may-be not.

That's fine, noone expects you to judge anyone, especially in a packaging
review ticket. Apart from that, the packager was quick to address the format
string issue once the need to do so arose.

> > (would it be submitted for the review if it didn't?)
> We have seen a lot of low qualtity junk going into Fedora, just because
> "somebody wanted it", because a reviewer and a submitter were playing review
> ping-pong, or because reviewers were working carelessly.

Well, in this case the packager was already maintaining a public repository of
the packages he's importing. I'm assuming it had a purpose. Luckily I, as well
as you, don't have to do arbitrary decisions based upon wild guesses.

> > > Openly said, I would not have approved this package because of the code
> > > quality.
> > 
> > Thanks for sharing your attitude, but this would be just you making up
> > guidelines. I'm positive that if someone challenged your decision given the
> > relevance of your reasoning it would be overturned.
> OK, you leave me no joice but to file bugs against this package.

As I was in a position to give you choices. You're always more than welcome to
file tickets about issues you encounter. I strongly suggest that you care about
quality of the bug reports and only report and demonstrate actual malfunctions
-- not merely reporting potential issues from compiler warnings.

(In reply to Ralf Corsepius from comment #18)
> FYI: All these warnings are serious and likely to be causing malfunctions:

As far as my C knowledge goes, they might be not be serious and might not cause
malfunctions. I haven't inspected the code. Note that I'm not advocating
writing shitty code or encouraging anyone to ignore the warnings. I'm merely
implying you're bringing too much drama into the play.

That said, you're very welcome to inspect the code, decide upon actual impact
and report bugs. Please don't jump into premature judgements from the presence
of warnings themselves!

> utils.c:409:191: warning: cast to pointer from integer of different size
> [-Wint-to-pointer-cast]
> menus_n_tools.c:44:55: warning: cast to pointer from integer of different
> size [-Wint-to-pointer-cast]
> menus_n_tools.c:110:85: warning: cast to pointer from integer of different
> size [-Wint-to-pointer-cast]

If nothing uses the full integer range, an actual issue might not be hit.

> x_alloc.c:256:5: warning: suggest explicit braces to avoid ambiguous 'else'
> [-Wparentheses]

Are you serious? This is a style issue, not necessarily an actual bug.

> gl_preview.c:1040:4: warning: too many arguments for format
> [-Wformat-extra-args]

Well, an extra value pushed to stack. If it's the last one, noone is going to
notice.

> render.c:122:4: warning: format '%d' expects argument of type 'int', but
> argument 3 has type 'long int' [-Wformat=]
> render.c:122:4: warning: format '%d' expects argument of type 'int', but
> argument 4 has type 'long int' [-Wformat=]
> render.c:201:3: warning: pointer targets in passing argument 3 of
> 'write_png' differ in signedness [-Wpointer-sign]

Again, if the full range is not used, noone will encounter issues with these.

> hf_dialog_options.c:47:4: warning: format '%d' expects argument of type
> 'int', but argument 2 has type 'struct GtkWidget *' [-Wformat=]
> hf_dialog_options.c:49:3: warning: format '%d' expects argument of type
> 'int', but argument 2 has type 'struct GtkWidget *' [-Wformat=]

Well, a bit weird to represent a pointer in decimal; but wouldn't it be weird
to communicate an address to the user in the first place?

> main.c:113:6: warning: too many arguments for format [-Wformat-extra-args]

Again, extra stack value. Weird, but potentially no biggie.

> stack.c:33:34: warning: assignment from incompatible pointer type
> thisappinit.c:78:2: warning: initialization from incompatible pointer type
> thisappinit.c:78:2: warning: (near initialization for
> 'doc_type_list[0].defaults')
> thisappinit.c:83:2: warning: initialization from incompatible pointer type
> thisappinit.c:83:2: warning: (near initialization for
> 'doc_type_list[0].set_creation_container')

You can cast pointer types back and forth via incompatible one and noone will
notice any issue.

> thisappinit.c:663:15: warning: array subscript is above array bounds
> [-Warray-bounds]

This is the only one that it is really hard to excuse. It might be that it's
not exploitable and leads to no crash due to dumb luck with alignment, but it
would be really nice (and presumably easy) to fix it.

Didier, if you are listening, I'm wondering if you could consider addressing at
least the last one and communicate these upstream?

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review





[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]