[libvirt] [PATCH 0/4] test_driver: implement virDomainSaveImageGetXMLDesc and virDomainSaveImageDefineXML

[Erik Skultety]

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 at 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