[Bug 956147] Review Request: wide-dhcpv6 - DHCPv6 Prefix Delegation client that works on PPP

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

 



Product: Fedora
https://bugzilla.redhat.com/show_bug.cgi?id=956147

--- Comment #10 from David Beveridge <dave@xxxxxxxxxxx> ---
(In reply to comment #5)
> - The upstream version is kinda awkward, but it's probably best to stick with
>   it as you did.
> - I would include your patches as source, instead of adding a github
> repository.

done

> 
> If you do the above, then the first section of macros can all go and it
> would be much cleaner
> and standard.

it sure is

>   Just add:  %global _hardened_build 1 at the top of the spec file, and make

done

>   sure the cflags/linker flags make it properly. You can check using this
>   script that is not yet integrated into rpmlint:
> https://nohats.ca/checksec.sh

not done yet

> - Add the sysconfig file(s) as SOURCEx: files, and remove the use of rh_dir
> - Don't use systemctl directly, use the proper macros
> - The exit 0 should not be needed in %post
> - Remove the %clean section, unless you're building for EL-5
> - Remove the %defattr line in the %files section, unless building for EL-5

done

> - You're using /etc/ppp which belongs to the ppp package, so add Requires:
>   for it

removed, it was crap anyway. need to cover this better in docs when we know how
initscripts will call it.

> - Why do you require the static version of flex ? flex-static seems to just 
>   install flex-devel? I do see it fails to compile without flex-devel, but
> this

changed

> - The resolv.conf hackery is problematic, but that can be tackled after the
>    package has made it in. (one should use a NM plugin to rewrite that file,
>    and if local DNSSEC is enabled you cannot touch  that file at all. And if
>    you chown, you probably also need chcon for SElinux)

I'm not a big fan if having DHCPv6 set DNS. 
Pretty much everyone will be running dual stack for now, 
and the IPv4 stack can handle DNS,
(or the administrator can just set it to 2001:4860:4860::8888)

> - Looks like there are some "sample" configs. That's not allowed outside of
>   the %doc area. The config files should work from package install if placed
>   in /etc. Apart from the sample files I see mention in /etc/wide-dhcpv6
>   / dhcp6c.conf of ethernet devices by names that are not valid for everyone.
>    That's also a problem.

all moved to %doc, and these could be expanded on when I know whats happening
with the initscripts.

> - Your /etc/sysconfig files have empty VAR= entries, which systemd really
> does not like. 
> (at least it did not in the past, not sure how it handles it these days)
> 
removed,

I also moved the systemd service files to %doc for now, 
as they contain config.

> I haven't run the full fedora-review yet, but I will do so when you've
> addressed most of the things listed here :)
> 

I've made lots of changes and done not much testing at this point,
and it's mothers day this weekend so I don't know how much time I'll
have for that till next week.

But here goes anyway...

http://repo.bevhost.com/fedora/wide-dhcpv6.spec
http://repo.bevhost.com/fedora/wide-dhcpv6-20080615-11.1.3.fc18.src.rpm


PS: still not sure about SELINUX policies.  I need to test this too.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=nKmKQ25Vyu&a=cc_unsubscribe
_______________________________________________
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]