Re: [pve-devel] [PATCH 1/1] initial ifupdown2 package

2018-05-17 Thread Alexandre DERUMIER
Hi Wolfgang,

Thanks for review !


>>I'd like to have more info on patch 0007-add-dummy-..., probably best 
>>to just write a more descriptive commit message there. 
yes, indeed. It's to avoid to launch ifupdown proxmox 
scripts(bridgevlanport,mtu), without remove them, as they are still provide by 
pve-manager package.
I'll add description.



>>Note that we started using git submodules with mirrors of upstream git 
>>repos for several packages by now, rather than having a `make download` 
>>target producing a tarball. We could do that here as well. I think it's 
>>more convenient, too. 

didn't known. I'll try to do this way.


>>As for the patch workflow, I see there are a bunch which are going 
>>upstream and a bunch of pve-specific ones. Would be good to separate 
>>them somehow. (I haven't been very consistent in that regard, though. 
>>eg. in qemu we have a pve/ and an extra/ direcory, while in lxc we have 
>>the pve patches directly in the patches dir and a fixes/ subdir). 
>>One or the other might be nice to use. That way it's also more 
>>convenient to (automatically) update patch files if you have your custom 
>>changes as a branch in a git-clone. 
ok, I'll split both with pve/ and extra/


>>PS: You don't need to use a cover letter for single-patch mails, 
>>especially if it only copies the commit message, as it's already in the 
>>patch itself anyway ;-) 
Ok, I was not sure about this.

Maybe add a note to
https://pve.proxmox.com/wiki/Developer_Documentation
;)





----- Mail original -----
De: "Wolfgang Bumiller" 
À: "Alexandre Derumier" 
Cc: "pve-devel" 
Envoyé: Jeudi 17 Mai 2018 10:16:02
Objet: Re: [pve-devel] [PATCH 1/1] initial ifupdown2 package

On Wed, May 16, 2018 at 12:01:40PM +0200, Alexandre Derumier wrote: 
> --- 
> Makefile | 42 + 
> debian/changelog | 174 + 
> debian/compat | 1 + 
> debian/control | 31  
> debian/copyright | 28  
> debian/ifupdown2.postinst | 86 ++ 
> ...0001-start-networking-add-usr-bin-in-PATH.patch | 28  
> ...ns-scripts-fix-ENV-for-interfaces-options.patch | 29  
> debian/patches/0003-config-tuning.patch | 52 ++ 
> .../0004-manual-interfaces-set-link-up.patch | 58 +++ 
> ...e-tap-veth-fwpr-interfaces-from-bridge-on.patch | 27  
> ...6-netlink-IFLA_BRPORT_ARP_SUPPRESS-use-32.patch | 29  
> ...0007-add-dummy-mtu-bridgevlanport-modules.patch | 69  
> .../patches/0008-add-vxlan-physdev-support.patch | 159 +++ 
> debian/patches/series | 8 + 
> debian/rules | 21 +++ 
> 16 files changed, 842 insertions(+) 
> create mode 100644 Makefile 
> create mode 100644 debian/changelog 
> create mode 100644 debian/compat 
> create mode 100644 debian/control 
> create mode 100644 debian/copyright 
> create mode 100644 debian/ifupdown2.postinst 
> create mode 100644 
> debian/patches/0001-start-networking-add-usr-bin-in-PATH.patch 
> create mode 100644 
> debian/patches/0002-addons-scripts-fix-ENV-for-interfaces-options.patch 
> create mode 100644 debian/patches/0003-config-tuning.patch 
> create mode 100644 debian/patches/0004-manual-interfaces-set-link-up.patch 
> create mode 100644 
> debian/patches/0005-don-t-remove-tap-veth-fwpr-interfaces-from-bridge-on.patch
>  
> create mode 100644 
> debian/patches/0006-netlink-IFLA_BRPORT_ARP_SUPPRESS-use-32.patch 
> create mode 100644 
> debian/patches/0007-add-dummy-mtu-bridgevlanport-modules.patch 
> create mode 100644 debian/patches/0008-add-vxlan-physdev-support.patch 
> create mode 100644 debian/patches/series 
> create mode 100755 debian/rules 
> (...) 

Looks interesting, seems like you spent quite some time with this 
already. 

If we add this, a couple of things: 

I'd like to have more info on patch 0007-add-dummy-..., probably best 
to just write a more descriptive commit message there. 

Note that we started using git submodules with mirrors of upstream git 
repos for several packages by now, rather than having a `make download` 
target producing a tarball. We could do that here as well. I think it's 
more convenient, too. 

As for the patch workflow, I see there are a bunch which are going 
upstream and a bunch of pve-specific ones. Would be good to separate 
them somehow. (I haven't been very consistent in that regard, though. 
eg. in qemu we have a pve/ and an extra/ direcory, while in lxc we have 
the pve patches directly in the patches dir and a fixes/ subdir). 
One or the other might be nice to use. That way it's also more 
convenient to (automatically) update patch files if you have your custom 
changes as a branch in a git-clone. 

PS: You don't need to use a cover letter for single-patch mails, 
especially if it only copies the commit message, as it's already in the 
patch itself anyway ;-) 

___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


Re: [pve-devel] [PATCH 1/1] initial ifupdown2 package

2018-05-17 Thread Wolfgang Bumiller
On Wed, May 16, 2018 at 12:01:40PM +0200, Alexandre Derumier wrote:
> ---
>  Makefile   |  42 +
>  debian/changelog   | 174 
> +
>  debian/compat  |   1 +
>  debian/control |  31 
>  debian/copyright   |  28 
>  debian/ifupdown2.postinst  |  86 ++
>  ...0001-start-networking-add-usr-bin-in-PATH.patch |  28 
>  ...ns-scripts-fix-ENV-for-interfaces-options.patch |  29 
>  debian/patches/0003-config-tuning.patch|  52 ++
>  .../0004-manual-interfaces-set-link-up.patch   |  58 +++
>  ...e-tap-veth-fwpr-interfaces-from-bridge-on.patch |  27 
>  ...6-netlink-IFLA_BRPORT_ARP_SUPPRESS-use-32.patch |  29 
>  ...0007-add-dummy-mtu-bridgevlanport-modules.patch |  69 
>  .../patches/0008-add-vxlan-physdev-support.patch   | 159 +++
>  debian/patches/series  |   8 +
>  debian/rules   |  21 +++
>  16 files changed, 842 insertions(+)
>  create mode 100644 Makefile
>  create mode 100644 debian/changelog
>  create mode 100644 debian/compat
>  create mode 100644 debian/control
>  create mode 100644 debian/copyright
>  create mode 100644 debian/ifupdown2.postinst
>  create mode 100644 
> debian/patches/0001-start-networking-add-usr-bin-in-PATH.patch
>  create mode 100644 
> debian/patches/0002-addons-scripts-fix-ENV-for-interfaces-options.patch
>  create mode 100644 debian/patches/0003-config-tuning.patch
>  create mode 100644 debian/patches/0004-manual-interfaces-set-link-up.patch
>  create mode 100644 
> debian/patches/0005-don-t-remove-tap-veth-fwpr-interfaces-from-bridge-on.patch
>  create mode 100644 
> debian/patches/0006-netlink-IFLA_BRPORT_ARP_SUPPRESS-use-32.patch
>  create mode 100644 
> debian/patches/0007-add-dummy-mtu-bridgevlanport-modules.patch
>  create mode 100644 debian/patches/0008-add-vxlan-physdev-support.patch
>  create mode 100644 debian/patches/series
>  create mode 100755 debian/rules
> (...)

Looks interesting, seems like you spent quite some time with this
already.

If we add this, a couple of things:

I'd like to have more info on patch 0007-add-dummy-..., probably best
to just write a more descriptive commit message there.

Note that we started using git submodules with mirrors of upstream git
repos for several packages by now, rather than having a `make download`
target producing a tarball. We could do that here as well. I think it's
more convenient, too.

As for the patch workflow, I see there are a bunch which are going
upstream and a bunch of pve-specific ones. Would be good to separate
them somehow. (I haven't been very consistent in that regard, though.
eg. in qemu we have a pve/ and an extra/ direcory, while in lxc we have
the pve patches directly in the patches dir and a fixes/ subdir).
One or the other might be nice to use. That way it's also more
convenient to (automatically) update patch files if you have your custom
changes as a branch in a git-clone.

PS: You don't need to use a cover letter for single-patch mails,
especially if it only copies the commit message, as it's already in the
patch itself anyway ;-)

___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel