[PATCHv3 0/5] netdev: Extract GenerateName/ReserveName as common functions

Shi Lei posted 5 patches 3 years, 4 months ago
Test syntax-check failed
Failed in applying to current master (apply log)
src/bhyve/bhyve_command.c              |   3 +-
src/conf/domain_conf.c                 |   4 +-
src/interface/interface_backend_udev.c |   2 +-
src/libvirt_private.syms               |   4 +-
src/libxl/libxl_driver.c               |   2 +-
src/lxc/lxc_process.c                  |   5 +-
src/qemu/qemu_interface.c              |  16 +--
src/qemu/qemu_process.c                |   4 +-
src/util/virnetdev.c                   | 116 ++++++++++++++++
src/util/virnetdev.h                   |  27 +++-
src/util/virnetdevmacvlan.c            | 177 +++----------------------
src/util/virnetdevmacvlan.h            |  14 +-
src/util/virnetdevtap.c                | 100 +-------------
src/util/virnetdevtap.h                |   4 -
src/util/virnetdevveth.c               | 140 +++++--------------
15 files changed, 218 insertions(+), 400 deletions(-)
[PATCHv3 0/5] netdev: Extract GenerateName/ReserveName as common functions
Posted by Shi Lei 3 years, 4 months ago
V2 here: https://www.redhat.com/archives/libvir-list/2020-December/msg00563.html

Since V2:
     *  Fix libxl driver for missing changing virNetDevMacVLanReserveName

V1 here: https://www.redhat.com/archives/libvir-list/2020-December/msg00308.html

Since V1:
    (1) Remove virNetDev[Lock|Unlock]GenName.
        Only *lastID* needs to be protected. Now we lock *lastID*
        inside virNetDevReserveName and virNetDevGenerateName, then lock and
        unlock functions are no longer needed.

    (2) Shorten the locking range for generating names for tap and macvlan.
        Since virNetDevReserveName and virNetDevGenerateName are now with lock,
        we can call them directly rather than adding lock outside.

    (3) Rename *_GEN_NAME_TAP to *_GEN_NAME_VNET.

    (4) Now veth and tap share the same prefix "vnet".

    (5) Use name rather than type as the argument of virNetDevReserveName.
        Just follow the style of virNetDev[Tap|MacVlan]ReserveName.

    (6) Remove those in-between functions for tap and macvlan.

    (7) Remove useless ENUM_[DECL|IMPL] for enum virNetDevGenNameType.

    (8) Remove the *_NONE item for enum virNetDevGenNameType.

    (9) Remove useless <math.h> in virnetdevtap.

    (10) When @ifname of virNetDevGenerateName is NOT a template or NULL,
        just leave it unchanged.

    (11) Take advantage of the "g_strdup_printf(*ifname, id)" in
        virNetDevGenerateName, prevent the functions that call virNetDevTapCreate()
        from adding in "vnet%d" when ifname is empty.

    (12) Use VIR_NETDEV_MACVLAN_CREATE_WITH_TAP to distinguish macvtap and
        macvlan and remove those useless macros.


Shi Lei (5):
  netdev: Introduce several helper functions for generating unique netdev name
  netdevtap: Use common helper function to create unique tap name
  netdevmacvlan: Use helper function to create unique macvlan/macvtap name
  netdevveth: Simplify virNetDevVethCreate by using virNetDevGenerateName
  netdev: Prevent functions that call virNetDevTapCreate from adding 'vnet%d' into ifname

 src/bhyve/bhyve_command.c              |   3 +-
 src/conf/domain_conf.c                 |   4 +-
 src/interface/interface_backend_udev.c |   2 +-
 src/libvirt_private.syms               |   4 +-
 src/libxl/libxl_driver.c               |   2 +-
 src/lxc/lxc_process.c                  |   5 +-
 src/qemu/qemu_interface.c              |  16 +--
 src/qemu/qemu_process.c                |   4 +-
 src/util/virnetdev.c                   | 116 ++++++++++++++++
 src/util/virnetdev.h                   |  27 +++-
 src/util/virnetdevmacvlan.c            | 177 +++----------------------
 src/util/virnetdevmacvlan.h            |  14 +-
 src/util/virnetdevtap.c                | 100 +-------------
 src/util/virnetdevtap.h                |   4 -
 src/util/virnetdevveth.c               | 140 +++++--------------
 15 files changed, 218 insertions(+), 400 deletions(-)

-- 
2.25.1


Re: [PATCHv3 0/5] netdev: Extract GenerateName/ReserveName as common functions
Posted by Daniel Henrique Barboza 3 years, 4 months ago

On 12/13/20 10:50 PM, Shi Lei wrote:
> V2 here: https://www.redhat.com/archives/libvir-list/2020-December/msg00563.html
> 
> Since V2:
>       *  Fix libxl driver for missing changing virNetDevMacVLanReserveName
> 
> V1 here: https://www.redhat.com/archives/libvir-list/2020-December/msg00308.html
> 
> Since V1:
>      (1) Remove virNetDev[Lock|Unlock]GenName.
>          Only *lastID* needs to be protected. Now we lock *lastID*
>          inside virNetDevReserveName and virNetDevGenerateName, then lock and
>          unlock functions are no longer needed.
> 
>      (2) Shorten the locking range for generating names for tap and macvlan.
>          Since virNetDevReserveName and virNetDevGenerateName are now with lock,
>          we can call them directly rather than adding lock outside.
> 
>      (3) Rename *_GEN_NAME_TAP to *_GEN_NAME_VNET.
> 
>      (4) Now veth and tap share the same prefix "vnet".
> 
>      (5) Use name rather than type as the argument of virNetDevReserveName.
>          Just follow the style of virNetDev[Tap|MacVlan]ReserveName.
> 
>      (6) Remove those in-between functions for tap and macvlan.
> 
>      (7) Remove useless ENUM_[DECL|IMPL] for enum virNetDevGenNameType.
> 
>      (8) Remove the *_NONE item for enum virNetDevGenNameType.
> 
>      (9) Remove useless <math.h> in virnetdevtap.
> 
>      (10) When @ifname of virNetDevGenerateName is NOT a template or NULL,
>          just leave it unchanged.
> 
>      (11) Take advantage of the "g_strdup_printf(*ifname, id)" in
>          virNetDevGenerateName, prevent the functions that call virNetDevTapCreate()
>          from adding in "vnet%d" when ifname is empty.
> 
>      (12) Use VIR_NETDEV_MACVLAN_CREATE_WITH_TAP to distinguish macvtap and
>          macvlan and remove those useless macros.
> 
> 
> Shi Lei (5):
>    netdev: Introduce several helper functions for generating unique netdev name
>    netdevtap: Use common helper function to create unique tap name
>    netdevmacvlan: Use helper function to create unique macvlan/macvtap name
>    netdevveth: Simplify virNetDevVethCreate by using virNetDevGenerateName
>    netdev: Prevent functions that call virNetDevTapCreate from adding 'vnet%d' into ifname


Series LGTM:


Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>


> 
>   src/bhyve/bhyve_command.c              |   3 +-
>   src/conf/domain_conf.c                 |   4 +-
>   src/interface/interface_backend_udev.c |   2 +-
>   src/libvirt_private.syms               |   4 +-
>   src/libxl/libxl_driver.c               |   2 +-
>   src/lxc/lxc_process.c                  |   5 +-
>   src/qemu/qemu_interface.c              |  16 +--
>   src/qemu/qemu_process.c                |   4 +-
>   src/util/virnetdev.c                   | 116 ++++++++++++++++
>   src/util/virnetdev.h                   |  27 +++-
>   src/util/virnetdevmacvlan.c            | 177 +++----------------------
>   src/util/virnetdevmacvlan.h            |  14 +-
>   src/util/virnetdevtap.c                | 100 +-------------
>   src/util/virnetdevtap.h                |   4 -
>   src/util/virnetdevveth.c               | 140 +++++--------------
>   15 files changed, 218 insertions(+), 400 deletions(-)
> 

Re: [PATCHv3 0/5] netdev: Extract GenerateName/ReserveName as common functions
Posted by Laine Stump 3 years, 4 months ago
On 12/14/20 12:49 PM, Daniel Henrique Barboza wrote:
> 
> 
> On 12/13/20 10:50 PM, Shi Lei wrote:
>> V2 here: 
>> https://www.redhat.com/archives/libvir-list/2020-December/msg00563.html
>>
>> Since V2:
>>       *  Fix libxl driver for missing changing 
>> virNetDevMacVLanReserveName
>>
>> V1 here: 
>> https://www.redhat.com/archives/libvir-list/2020-December/msg00308.html
>>
>> Since V1:
>>      (1) Remove virNetDev[Lock|Unlock]GenName.
>>          Only *lastID* needs to be protected. Now we lock *lastID*
>>          inside virNetDevReserveName and virNetDevGenerateName, then 
>> lock and
>>          unlock functions are no longer needed.
>>
>>      (2) Shorten the locking range for generating names for tap and 
>> macvlan.
>>          Since virNetDevReserveName and virNetDevGenerateName are now 
>> with lock,
>>          we can call them directly rather than adding lock outside.
>>
>>      (3) Rename *_GEN_NAME_TAP to *_GEN_NAME_VNET.
>>
>>      (4) Now veth and tap share the same prefix "vnet".
>>
>>      (5) Use name rather than type as the argument of 
>> virNetDevReserveName.
>>          Just follow the style of virNetDev[Tap|MacVlan]ReserveName.
>>
>>      (6) Remove those in-between functions for tap and macvlan.
>>
>>      (7) Remove useless ENUM_[DECL|IMPL] for enum virNetDevGenNameType.
>>
>>      (8) Remove the *_NONE item for enum virNetDevGenNameType.
>>
>>      (9) Remove useless <math.h> in virnetdevtap.
>>
>>      (10) When @ifname of virNetDevGenerateName is NOT a template or 
>> NULL,
>>          just leave it unchanged.
>>
>>      (11) Take advantage of the "g_strdup_printf(*ifname, id)" in
>>          virNetDevGenerateName, prevent the functions that call 
>> virNetDevTapCreate()
>>          from adding in "vnet%d" when ifname is empty.
>>
>>      (12) Use VIR_NETDEV_MACVLAN_CREATE_WITH_TAP to distinguish 
>> macvtap and
>>          macvlan and remove those useless macros.
>>
>>
>> Shi Lei (5):
>>    netdev: Introduce several helper functions for generating unique 
>> netdev name
>>    netdevtap: Use common helper function to create unique tap name
>>    netdevmacvlan: Use helper function to create unique macvlan/macvtap 
>> name
>>    netdevveth: Simplify virNetDevVethCreate by using 
>> virNetDevGenerateName
>>    netdev: Prevent functions that call virNetDevTapCreate from adding 
>> 'vnet%d' into ifname
> 
> 
> Series LGTM:

There are still a couple problems with this that we're working on 
(including an infinitely repeating segfault on restart if there are any 
running lxc domains with a type='direct' interface), so please don't 
push it!

> 
> 
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> 
> 
>>
>>   src/bhyve/bhyve_command.c              |   3 +-
>>   src/conf/domain_conf.c                 |   4 +-
>>   src/interface/interface_backend_udev.c |   2 +-
>>   src/libvirt_private.syms               |   4 +-
>>   src/libxl/libxl_driver.c               |   2 +-
>>   src/lxc/lxc_process.c                  |   5 +-
>>   src/qemu/qemu_interface.c              |  16 +--
>>   src/qemu/qemu_process.c                |   4 +-
>>   src/util/virnetdev.c                   | 116 ++++++++++++++++
>>   src/util/virnetdev.h                   |  27 +++-
>>   src/util/virnetdevmacvlan.c            | 177 +++----------------------
>>   src/util/virnetdevmacvlan.h            |  14 +-
>>   src/util/virnetdevtap.c                | 100 +-------------
>>   src/util/virnetdevtap.h                |   4 -
>>   src/util/virnetdevveth.c               | 140 +++++--------------
>>   15 files changed, 218 insertions(+), 400 deletions(-)
>>
> 

Re: [PATCHv3 0/5] netdev: Extract GenerateName/ReserveName as common functions
Posted by Laine Stump 3 years, 4 months ago
On 12/13/20 8:50 PM, Shi Lei wrote:
> V2 here: https://www.redhat.com/archives/libvir-list/2020-December/msg00563.html
>
> Since V2:
>       *  Fix libxl driver for missing changing virNetDevMacVLanReserveName
>
> V1 here: https://www.redhat.com/archives/libvir-list/2020-December/msg00308.html
>
> Since V1:
>      (1) Remove virNetDev[Lock|Unlock]GenName.
>          Only *lastID* needs to be protected. Now we lock *lastID*
>          inside virNetDevReserveName and virNetDevGenerateName, then lock and
>          unlock functions are no longer needed.
>
>      (2) Shorten the locking range for generating names for tap and macvlan.
>          Since virNetDevReserveName and virNetDevGenerateName are now with lock,
>          we can call them directly rather than adding lock outside.
>
>      (3) Rename *_GEN_NAME_TAP to *_GEN_NAME_VNET.
>
>      (4) Now veth and tap share the same prefix "vnet".
>
>      (5) Use name rather than type as the argument of virNetDevReserveName.
>          Just follow the style of virNetDev[Tap|MacVlan]ReserveName.
>
>      (6) Remove those in-between functions for tap and macvlan.
>
>      (7) Remove useless ENUM_[DECL|IMPL] for enum virNetDevGenNameType.
>
>      (8) Remove the *_NONE item for enum virNetDevGenNameType.
>
>      (9) Remove useless <math.h> in virnetdevtap.
>
>      (10) When @ifname of virNetDevGenerateName is NOT a template or NULL,
>          just leave it unchanged.
>
>      (11) Take advantage of the "g_strdup_printf(*ifname, id)" in
>          virNetDevGenerateName, prevent the functions that call virNetDevTapCreate()
>          from adding in "vnet%d" when ifname is empty.
>
>      (12) Use VIR_NETDEV_MACVLAN_CREATE_WITH_TAP to distinguish macvtap and
>          macvlan and remove those useless macros.


Thanks for working through all of those.


For Patches 1-4:

Reviewed-by: Laine Stump <laine@redhat.com>


and pushed. Thanks for your contribution!


For Patch 5 - it turned out simpler to just write a short series of 
patches to replace that, which I posted as 
https://www.redhat.com/archives/libvir-list/2020-December/msg00744.html


>
>
> Shi Lei (5):
>    netdev: Introduce several helper functions for generating unique netdev name
>    netdevtap: Use common helper function to create unique tap name
>    netdevmacvlan: Use helper function to create unique macvlan/macvtap name
>    netdevveth: Simplify virNetDevVethCreate by using virNetDevGenerateName
>    netdev: Prevent functions that call virNetDevTapCreate from adding 'vnet%d' into ifname
>
>   src/bhyve/bhyve_command.c              |   3 +-
>   src/conf/domain_conf.c                 |   4 +-
>   src/interface/interface_backend_udev.c |   2 +-
>   src/libvirt_private.syms               |   4 +-
>   src/libxl/libxl_driver.c               |   2 +-
>   src/lxc/lxc_process.c                  |   5 +-
>   src/qemu/qemu_interface.c              |  16 +--
>   src/qemu/qemu_process.c                |   4 +-
>   src/util/virnetdev.c                   | 116 ++++++++++++++++
>   src/util/virnetdev.h                   |  27 +++-
>   src/util/virnetdevmacvlan.c            | 177 +++----------------------
>   src/util/virnetdevmacvlan.h            |  14 +-
>   src/util/virnetdevtap.c                | 100 +-------------
>   src/util/virnetdevtap.h                |   4 -
>   src/util/virnetdevveth.c               | 140 +++++--------------
>   15 files changed, 218 insertions(+), 400 deletions(-)
>