While implementing virDomainSaveImageGetXMLDesc and
virDomainSaveImageDefineXML for the test driver, I realized that there
exists already code for saving and loading test images which can be
reused. However, it needed to be extracted from testDomainSaveFlags and
testDomainRestoreFlags into separate functions. The new functions are
inspired by the corresponding QEMU driver code where e.g.
qemuDomainSaveImageOpen serves as a helper used by other functions.

This series of patches initially extracts the code mentioned above into
separate functions and then provides the test driver with
implementations for virDomainSaveImageGetXMLDesc and
virDomainSaveImageDefineXML which make use of the newly introduced
functions.

Ilias Stamatis (4):
  test_driver: extract image saving code into a separate function
  test_driver: extract image loading code into a separate function
  test_driver: implement virDomainSaveImageDefineXML
  test_driver: implement virDomainSaveImageGetXMLDesc

 src/test/test_driver.c | 281 ++++++++++++++++++++++++++++-------------
 1 file changed, 193 insertions(+), 88 deletions(-)

--
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, May 29, 2019 at 02:22:55PM +0200, Ilias Stamatis wrote:
> While implementing virDomainSaveImageGetXMLDesc and
> virDomainSaveImageDefineXML for the test driver, I realized that there
> exists already code for saving and loading test images which can be
> reused. However, it needed to be extracted from testDomainSaveFlags and
> testDomainRestoreFlags into separate functions. The new functions are
> inspired by the corresponding QEMU driver code where e.g.
> qemuDomainSaveImageOpen serves as a helper used by other functions.
>
> This series of patches initially extracts the code mentioned above into
> separate functions and then provides the test driver with
> implementations for virDomainSaveImageGetXMLDesc and
> virDomainSaveImageDefineXML which make use of the newly introduced
> functions.
>
> Ilias Stamatis (4):
>   test_driver: extract image saving code into a separate function
>   test_driver: extract image loading code into a separate function
>   test_driver: implement virDomainSaveImageDefineXML
>   test_driver: implement virDomainSaveImageGetXMLDesc

IMHO each public API should be introduced in a separate series.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Jun 03, 2019 at 10:36:58AM +0200, Erik Skultety wrote:
> On Wed, May 29, 2019 at 02:22:55PM +0200, Ilias Stamatis wrote:
> > While implementing virDomainSaveImageGetXMLDesc and
> > virDomainSaveImageDefineXML for the test driver, I realized that there
> > exists already code for saving and loading test images which can be
> > reused. However, it needed to be extracted from testDomainSaveFlags and
> > testDomainRestoreFlags into separate functions. The new functions are
> > inspired by the corresponding QEMU driver code where e.g.
> > qemuDomainSaveImageOpen serves as a helper used by other functions.
> >
> > This series of patches initially extracts the code mentioned above into
> > separate functions and then provides the test driver with
> > implementations for virDomainSaveImageGetXMLDesc and
> > virDomainSaveImageDefineXML which make use of the newly introduced
> > functions.
> >
> > Ilias Stamatis (4):
> >   test_driver: extract image saving code into a separate function
> >   test_driver: extract image loading code into a separate function
> >   test_driver: implement virDomainSaveImageDefineXML
> >   test_driver: implement virDomainSaveImageGetXMLDesc
> 
> IMHO each public API should be introduced in a separate series.

Any reason for that? These two are related and simple enough so to me it
actually make sense to introduce them together in one series.

Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Jun 3, 2019 at 1:50 PM Pavel Hrdina <phrdina@redhat.com> wrote:
>
> On Mon, Jun 03, 2019 at 10:36:58AM +0200, Erik Skultety wrote:
> > On Wed, May 29, 2019 at 02:22:55PM +0200, Ilias Stamatis wrote:
> > > While implementing virDomainSaveImageGetXMLDesc and
> > > virDomainSaveImageDefineXML for the test driver, I realized that there
> > > exists already code for saving and loading test images which can be
> > > reused. However, it needed to be extracted from testDomainSaveFlags and
> > > testDomainRestoreFlags into separate functions. The new functions are
> > > inspired by the corresponding QEMU driver code where e.g.
> > > qemuDomainSaveImageOpen serves as a helper used by other functions.
> > >
> > > This series of patches initially extracts the code mentioned above into
> > > separate functions and then provides the test driver with
> > > implementations for virDomainSaveImageGetXMLDesc and
> > > virDomainSaveImageDefineXML which make use of the newly introduced
> > > functions.
> > >
> > > Ilias Stamatis (4):
> > >   test_driver: extract image saving code into a separate function
> > >   test_driver: extract image loading code into a separate function
> > >   test_driver: implement virDomainSaveImageDefineXML
> > >   test_driver: implement virDomainSaveImageGetXMLDesc
> >
> > IMHO each public API should be introduced in a separate series.
>
> Any reason for that? These two are related and simple enough so to me it
> actually make sense to introduce them together in one series.
>
> Pavel

I also thought the same, since the patches touch certain parts of the
test driver code and logic so it would make sense to be reviewed
together by the same person.

Ilias

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Jun 03, 2019 at 08:27:29PM +0200, Ilias Stamatis wrote:
> On Mon, Jun 3, 2019 at 1:50 PM Pavel Hrdina <phrdina@redhat.com> wrote:
> >
> > On Mon, Jun 03, 2019 at 10:36:58AM +0200, Erik Skultety wrote:
> > > On Wed, May 29, 2019 at 02:22:55PM +0200, Ilias Stamatis wrote:
> > > > While implementing virDomainSaveImageGetXMLDesc and
> > > > virDomainSaveImageDefineXML for the test driver, I realized that there
> > > > exists already code for saving and loading test images which can be
> > > > reused. However, it needed to be extracted from testDomainSaveFlags and
> > > > testDomainRestoreFlags into separate functions. The new functions are
> > > > inspired by the corresponding QEMU driver code where e.g.
> > > > qemuDomainSaveImageOpen serves as a helper used by other functions.
> > > >
> > > > This series of patches initially extracts the code mentioned above into
> > > > separate functions and then provides the test driver with
> > > > implementations for virDomainSaveImageGetXMLDesc and
> > > > virDomainSaveImageDefineXML which make use of the newly introduced
> > > > functions.
> > > >
> > > > Ilias Stamatis (4):
> > > >   test_driver: extract image saving code into a separate function
> > > >   test_driver: extract image loading code into a separate function
> > > >   test_driver: implement virDomainSaveImageDefineXML
> > > >   test_driver: implement virDomainSaveImageGetXMLDesc
> > >
> > > IMHO each public API should be introduced in a separate series.
> >
> > Any reason for that? These two are related and simple enough so to me it
> > actually make sense to introduce them together in one series.
> >
> > Pavel
>
> I also thought the same, since the patches touch certain parts of the
> test driver code and logic so it would make sense to be reviewed
> together by the same person.
>
> Ilias

That was just my personal taste, perhaps mostly internally driven by my comment
in 2/4 which turned out to be a custom setting in my config. I don't have a
strong preference though, it doesn't change anything on the review process, I
would have continued (and I will) the review of this series anyway.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list