[v2] Initial network support in ch driver.

[PATCH v2 0/5] Initial network support in ch driver.

Posted by Praveen K Paladugu 3 months, 1 week ago

v2:
* Refactor virSocketRecvHttpResponse to return responses without parsing http
  responses.
* Use errno to report errors in virsocket.c
* Address WIN32 build failure in virsocket.c
* Fix code indentations

Praveen K Paladugu (5):
  conf: Drop unused parameter
  hypervisor: Move domain interface mgmt methods
  util: Add util methods required by ch networking
  ch: Introduce version based cap for network support
  ch: Enable ETHERNET Network mode support

 po/POTFILES                       |   3 +
 src/ch/ch_capabilities.c          |   9 +
 src/ch/ch_capabilities.h          |   1 +
 src/ch/ch_conf.h                  |   4 +
 src/ch/ch_domain.c                |  41 +++
 src/ch/ch_domain.h                |   3 +
 src/ch/ch_interface.c             | 100 +++++++
 src/ch/ch_interface.h             |  35 +++
 src/ch/ch_monitor.c               | 213 +++++---------
 src/ch/ch_monitor.h               |   7 +-
 src/ch/ch_process.c               | 166 ++++++++++-
 src/ch/meson.build                |   2 +
 src/conf/domain_conf.c            |   1 -
 src/conf/domain_conf.h            |   3 +-
 src/hypervisor/domain_interface.c | 457 ++++++++++++++++++++++++++++++
 src/hypervisor/domain_interface.h |  45 +++
 src/hypervisor/meson.build        |   1 +
 src/libvirt_private.syms          |  11 +
 src/libxl/libxl_domain.c          |   2 +-
 src/libxl/libxl_driver.c          |   4 +-
 src/lxc/lxc_driver.c              |   2 +-
 src/lxc/lxc_process.c             |   4 +-
 src/qemu/qemu_command.c           |   8 +-
 src/qemu/qemu_hotplug.c           |  15 +-
 src/qemu/qemu_interface.c         | 339 +---------------------
 src/qemu/qemu_interface.h         |  11 -
 src/qemu/qemu_process.c           |  72 +----
 src/util/virsocket.c              | 116 ++++++++
 src/util/virsocket.h              |   3 +
 29 files changed, 1107 insertions(+), 571 deletions(-)
 create mode 100644 src/ch/ch_interface.c
 create mode 100644 src/ch/ch_interface.h
 create mode 100644 src/hypervisor/domain_interface.c
 create mode 100644 src/hypervisor/domain_interface.h

-- 
2.43.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Re: [PATCH v2 0/5] Initial network support in ch driver.

Posted by Praveen K Paladugu 2 months, 4 weeks ago

Ping.. for some reviews on this patchset.

-- 
Regards,
Praveen
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Re: [PATCH v2 0/5] Initial network support in ch driver.

Posted by Michal Prívozník 2 months, 4 weeks ago

On 1/16/24 22:25, Praveen K Paladugu wrote:
> v2:
> * Refactor virSocketRecvHttpResponse to return responses without parsing http
>   responses.
> * Use errno to report errors in virsocket.c
> * Address WIN32 build failure in virsocket.c
> * Fix code indentations
> 
> Praveen K Paladugu (5):
>   conf: Drop unused parameter
>   hypervisor: Move domain interface mgmt methods
>   util: Add util methods required by ch networking
>   ch: Introduce version based cap for network support
>   ch: Enable ETHERNET Network mode support
> 
>  po/POTFILES                       |   3 +
>  src/ch/ch_capabilities.c          |   9 +
>  src/ch/ch_capabilities.h          |   1 +
>  src/ch/ch_conf.h                  |   4 +
>  src/ch/ch_domain.c                |  41 +++
>  src/ch/ch_domain.h                |   3 +
>  src/ch/ch_interface.c             | 100 +++++++
>  src/ch/ch_interface.h             |  35 +++
>  src/ch/ch_monitor.c               | 213 +++++---------
>  src/ch/ch_monitor.h               |   7 +-
>  src/ch/ch_process.c               | 166 ++++++++++-
>  src/ch/meson.build                |   2 +
>  src/conf/domain_conf.c            |   1 -
>  src/conf/domain_conf.h            |   3 +-
>  src/hypervisor/domain_interface.c | 457 ++++++++++++++++++++++++++++++
>  src/hypervisor/domain_interface.h |  45 +++
>  src/hypervisor/meson.build        |   1 +
>  src/libvirt_private.syms          |  11 +
>  src/libxl/libxl_domain.c          |   2 +-
>  src/libxl/libxl_driver.c          |   4 +-
>  src/lxc/lxc_driver.c              |   2 +-
>  src/lxc/lxc_process.c             |   4 +-
>  src/qemu/qemu_command.c           |   8 +-
>  src/qemu/qemu_hotplug.c           |  15 +-
>  src/qemu/qemu_interface.c         | 339 +---------------------
>  src/qemu/qemu_interface.h         |  11 -
>  src/qemu/qemu_process.c           |  72 +----
>  src/util/virsocket.c              | 116 ++++++++
>  src/util/virsocket.h              |   3 +
>  29 files changed, 1107 insertions(+), 571 deletions(-)
>  create mode 100644 src/ch/ch_interface.c
>  create mode 100644 src/ch/ch_interface.h
>  create mode 100644 src/hypervisor/domain_interface.c
>  create mode 100644 src/hypervisor/domain_interface.h
> 

Now, this is almost correct. I only have couple of suggestions. No need
to resend another version. I've uploaded these patches among with my
suggestions to my gitlab:

  https://gitlab.com/MichalPrivoznik/libvirt/-/tree/ch_networking?ref_type=heads

You'll find 'fixup' commits there - these contains fixes to those small
issues I've raised in my review. I'll squash them in before merging.

Then, there's one suggestion - "Possible move of virSocketRecv() into
ch_process.c" - honestly, I don't feel like virSocketRecv() belongs into
src/util/ but I can't explain why really. For the time being we can have
it in src/ch/. I'd split this commit and squash its parts into
respective commits.

Please do check if code still works even with my fixups and whether you
agree with suggested changes. If so, I can give my reviewed by and merge
these.

Michal
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Re: [PATCH v2 0/5] Initial network support in ch driver.

Posted by Praveen Paladugu 2 months, 3 weeks ago

On Tue, Jan 30, 2024 at 06:28:05PM +0100, Michal Pr??vozn??k wrote:
> On 1/16/24 22:25, Praveen K Paladugu wrote:
> > v2:
> > * Refactor virSocketRecvHttpResponse to return responses without parsing http
> >   responses.
> > * Use errno to report errors in virsocket.c
> > * Address WIN32 build failure in virsocket.c
> > * Fix code indentations
> > 
> > Praveen K Paladugu (5):
> >   conf: Drop unused parameter
> >   hypervisor: Move domain interface mgmt methods
> >   util: Add util methods required by ch networking
> >   ch: Introduce version based cap for network support
> >   ch: Enable ETHERNET Network mode support
> > 
> >  po/POTFILES                       |   3 +
> >  src/ch/ch_capabilities.c          |   9 +
> >  src/ch/ch_capabilities.h          |   1 +
> >  src/ch/ch_conf.h                  |   4 +
> >  src/ch/ch_domain.c                |  41 +++
> >  src/ch/ch_domain.h                |   3 +
> >  src/ch/ch_interface.c             | 100 +++++++
> >  src/ch/ch_interface.h             |  35 +++
> >  src/ch/ch_monitor.c               | 213 +++++---------
> >  src/ch/ch_monitor.h               |   7 +-
> >  src/ch/ch_process.c               | 166 ++++++++++-
> >  src/ch/meson.build                |   2 +
> >  src/conf/domain_conf.c            |   1 -
> >  src/conf/domain_conf.h            |   3 +-
> >  src/hypervisor/domain_interface.c | 457 ++++++++++++++++++++++++++++++
> >  src/hypervisor/domain_interface.h |  45 +++
> >  src/hypervisor/meson.build        |   1 +
> >  src/libvirt_private.syms          |  11 +
> >  src/libxl/libxl_domain.c          |   2 +-
> >  src/libxl/libxl_driver.c          |   4 +-
> >  src/lxc/lxc_driver.c              |   2 +-
> >  src/lxc/lxc_process.c             |   4 +-
> >  src/qemu/qemu_command.c           |   8 +-
> >  src/qemu/qemu_hotplug.c           |  15 +-
> >  src/qemu/qemu_interface.c         | 339 +---------------------
> >  src/qemu/qemu_interface.h         |  11 -
> >  src/qemu/qemu_process.c           |  72 +----
> >  src/util/virsocket.c              | 116 ++++++++
> >  src/util/virsocket.h              |   3 +
> >  29 files changed, 1107 insertions(+), 571 deletions(-)
> >  create mode 100644 src/ch/ch_interface.c
> >  create mode 100644 src/ch/ch_interface.h
> >  create mode 100644 src/hypervisor/domain_interface.c
> >  create mode 100644 src/hypervisor/domain_interface.h
> > 
> 
> Now, this is almost correct. I only have couple of suggestions. No need
> to resend another version. I've uploaded these patches among with my
> suggestions to my gitlab:
> 
>   https://gitlab.com/MichalPrivoznik/libvirt/-/tree/ch_networking?ref_type=heads
> 
> You'll find 'fixup' commits there - these contains fixes to those small
> issues I've raised in my review. I'll squash them in before merging.
> 
> Then, there's one suggestion - "Possible move of virSocketRecv() into
> ch_process.c" - honestly, I don't feel like virSocketRecv() belongs into
> src/util/ but I can't explain why really. For the time being we can have
> it in src/ch/. I'd split this commit and squash its parts into
> respective commits.
>
I don't feel strongly about keep virSocketRecv() in src/util. The method
seemed generic enough to keep it in src/util. If you are not comfortable
keeping it there, I am fine with moving it to src/ch.

> Please do check if code still works even with my fixups and whether you
> agree with suggested changes. If so, I can give my reviewed by and merge



> these.
>

Thanks for the detailed review on this patchset. I tested the patchset
in above branch and I noticed 2 bugs. One introduced by me in v2, and one
introduced by a fixup commit.

First bug is fixed by https://github.com/praveen-pk/libvirt/commit/6c3b068957d854c0b4f51925d3e87a88b41803a3.
Seems like I missed to test a domain XML without an IP address to guest
interface. This patch fixes the bug.

Second bug is fixed by https://github.com/praveen-pk/libvirt/commit/a7f7bcefbcc97f755b5075e52233288d12cdcfc4.
As `control` is now allocated from heap, the size is incorrectly set in
`msg.msg_controllen`. This patch fixes it.


I marked both these commits as `fixup2`. With these commits, the patchset looks
good. Please go ahead and merge.

If you'd like for me to send you a v3 with all the commit properly squashed, I
am happy to do that as well.
 
Regards,
Praveen

> Michal
> _______________________________________________
> Devel mailing list -- devel@lists.libvirt.org
> To unsubscribe send an email to devel-leave@lists.libvirt.org
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Re: [PATCH v2 0/5] Initial network support in ch driver.

Posted by Michal Prívozník 2 months, 3 weeks ago

On 2/1/24 16:40, Praveen Paladugu wrote:
> On Tue, Jan 30, 2024 at 06:28:05PM +0100, Michal Pr??vozn??k wrote:
>> On 1/16/24 22:25, Praveen K Paladugu wrote:
>>> v2:
>>> * Refactor virSocketRecvHttpResponse to return responses without parsing http
>>>   responses.
>>> * Use errno to report errors in virsocket.c
>>> * Address WIN32 build failure in virsocket.c
>>> * Fix code indentations
>>>
>>> Praveen K Paladugu (5):
>>>   conf: Drop unused parameter
>>>   hypervisor: Move domain interface mgmt methods
>>>   util: Add util methods required by ch networking
>>>   ch: Introduce version based cap for network support
>>>   ch: Enable ETHERNET Network mode support
>>>
>>>  po/POTFILES                       |   3 +
>>>  src/ch/ch_capabilities.c          |   9 +
>>>  src/ch/ch_capabilities.h          |   1 +
>>>  src/ch/ch_conf.h                  |   4 +
>>>  src/ch/ch_domain.c                |  41 +++
>>>  src/ch/ch_domain.h                |   3 +
>>>  src/ch/ch_interface.c             | 100 +++++++
>>>  src/ch/ch_interface.h             |  35 +++
>>>  src/ch/ch_monitor.c               | 213 +++++---------
>>>  src/ch/ch_monitor.h               |   7 +-
>>>  src/ch/ch_process.c               | 166 ++++++++++-
>>>  src/ch/meson.build                |   2 +
>>>  src/conf/domain_conf.c            |   1 -
>>>  src/conf/domain_conf.h            |   3 +-
>>>  src/hypervisor/domain_interface.c | 457 ++++++++++++++++++++++++++++++
>>>  src/hypervisor/domain_interface.h |  45 +++
>>>  src/hypervisor/meson.build        |   1 +
>>>  src/libvirt_private.syms          |  11 +
>>>  src/libxl/libxl_domain.c          |   2 +-
>>>  src/libxl/libxl_driver.c          |   4 +-
>>>  src/lxc/lxc_driver.c              |   2 +-
>>>  src/lxc/lxc_process.c             |   4 +-
>>>  src/qemu/qemu_command.c           |   8 +-
>>>  src/qemu/qemu_hotplug.c           |  15 +-
>>>  src/qemu/qemu_interface.c         | 339 +---------------------
>>>  src/qemu/qemu_interface.h         |  11 -
>>>  src/qemu/qemu_process.c           |  72 +----
>>>  src/util/virsocket.c              | 116 ++++++++
>>>  src/util/virsocket.h              |   3 +
>>>  29 files changed, 1107 insertions(+), 571 deletions(-)
>>>  create mode 100644 src/ch/ch_interface.c
>>>  create mode 100644 src/ch/ch_interface.h
>>>  create mode 100644 src/hypervisor/domain_interface.c
>>>  create mode 100644 src/hypervisor/domain_interface.h
>>>
>>
>> Now, this is almost correct. I only have couple of suggestions. No need
>> to resend another version. I've uploaded these patches among with my
>> suggestions to my gitlab:
>>
>>   https://gitlab.com/MichalPrivoznik/libvirt/-/tree/ch_networking?ref_type=heads
>>
>> You'll find 'fixup' commits there - these contains fixes to those small
>> issues I've raised in my review. I'll squash them in before merging.
>>
>> Then, there's one suggestion - "Possible move of virSocketRecv() into
>> ch_process.c" - honestly, I don't feel like virSocketRecv() belongs into
>> src/util/ but I can't explain why really. For the time being we can have
>> it in src/ch/. I'd split this commit and squash its parts into
>> respective commits.
>>
> I don't feel strongly about keep virSocketRecv() in src/util. The method
> seemed generic enough to keep it in src/util. If you are not comfortable
> keeping it there, I am fine with moving it to src/ch.
> 
>> Please do check if code still works even with my fixups and whether you
>> agree with suggested changes. If so, I can give my reviewed by and merge
> 
> 
> 
>> these.
>>
> 
> Thanks for the detailed review on this patchset. I tested the patchset
> in above branch and I noticed 2 bugs. One introduced by me in v2, and one
> introduced by a fixup commit.
> 
> First bug is fixed by https://github.com/praveen-pk/libvirt/commit/6c3b068957d854c0b4f51925d3e87a88b41803a3.
> Seems like I missed to test a domain XML without an IP address to guest
> interface. This patch fixes the bug.
> 
> Second bug is fixed by https://github.com/praveen-pk/libvirt/commit/a7f7bcefbcc97f755b5075e52233288d12cdcfc4.
> As `control` is now allocated from heap, the size is incorrectly set in
> `msg.msg_controllen`. This patch fixes it.

Oops, yes.

I've changed commits and pushed.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org