On Mon, 2021-06-28 at 16:12 +0200, Martin Kletzander wrote:
> On Mon, Jun 28, 2021 at 10:45:38AM +0800, Luke Yue wrote:
> > On Thu, 2021-06-24 at 17:19 +0200, Martin Kletzander wrote:
> > > On Thu, Jun 24, 2021 at 06:59:59PM +0800, Luke Yue wrote:
> > > > Signed-off-by: Luke Yue <lukedyue@xxxxxxxxx>
> > > > ---
> > > > src/test/test_driver.c | 53
> > > > ++++++++++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 53 insertions(+)
> > > >
> > > > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > > > index 65710b78ef..dff96bceb6 100644
> > > > --- a/src/test/test_driver.c
> > > > +++ b/src/test/test_driver.c
> > > > @@ -9331,6 +9331,58 @@
> > > > testDomainCheckpointDelete(virDomainCheckpointPtr checkpoint,
> > > > return ret;
> > > > }
> > > >
> > > > +static int
> > > > +testDomainGetMessages(virDomainPtr dom,
> > > > + char ***msgs,
> > > > + unsigned int flags)
> > > > +{
> > > > + virDomainObj *vm = NULL;
> > > > + int rv = -1;
> > > > + size_t i, n;
> > > > + int nmsgs;
> > > > +
> > > > + virCheckFlags(VIR_DOMAIN_MESSAGE_DEPRECATION |
> > > > + VIR_DOMAIN_MESSAGE_TAINTING, -1);
> > > > +
> > > > + if (!(vm = testDomObjFromDomain(dom)))
> > > > + return -1;
> > > > +
> > > > + *msgs = NULL;
> > > > + nmsgs = 0;
> > > > + n = 0;
> > > > +
> > > > + if (!flags || (flags & VIR_DOMAIN_MESSAGE_TAINTING)) {
> > > > + nmsgs += __builtin_popcount(vm->taint);
> > > > + *msgs = g_renew(char *, *msgs, nmsgs+1);
> > > > +
> > > > + for (i = 0; i < VIR_DOMAIN_TAINT_LAST; i++) {
> > > > + if (vm->taint & (1 << i)) {
> > > > + (*msgs)[n++] = g_strdup_printf(
> > > > + _("tainted: %s"),
> > > > + _(virDomainTaintMessageTypeToString(i)));
> > > > + }
> > > > + }
> > > > + }
> > > > +
> > > > + if (!flags || (flags & VIR_DOMAIN_MESSAGE_DEPRECATION)) {
> > > > + nmsgs += vm->ndeprecations;
> > > > + *msgs = g_renew(char *, *msgs, nmsgs+1);
> > > > +
> > > > + for (i = 0; i < vm->ndeprecations; i++) {
> > > > + (*msgs)[n++] = g_strdup_printf(
> > > > + _("deprecated configuration: %s"),
> > > > + vm->deprecations[i]);
> > > > + }
> > > > + }
> > > > +
> > > > + (*msgs)[nmsgs] = NULL;
> > > > +
> > > > + rv = nmsgs;
> > > > +
> > > > + virDomainObjEndAPI(&vm);
> > > > + return rv;
> > > > +}
> > > > +
> > >
> > > As I mentioned in the review for v1, it is nice that someone can
> > > check how this
> > > API behaves without spinning up a VM, just using the test
> > > driver.
> > > However most
> > > of this code is copy-paste from the qemu driver and can hence be
> > > extracted to a
> > > separate function which would be called from both drivers (and
> > > maybe
> > > more than
> > > the ones mentioned here are using the same logic). That would
> > > make
> > > the codebase
> > > smaller, the library smaller, it would keep the test driver
> > > updated
> > > and it would
> > > actually test the codepath used in the qemu driver.
> > > >
> >
> > I'm sorry, I forgot that. So if I want to extract a function like
> > below
> >
> > ```
> > int
> > virDomainGetMessagesFromVM(virDomainObj *vm,
> > char **msgs,
> > unsigned int flags)
> > {
> > size_t i, n;
> > int nmsgs;
> > int rv = -1;
> >
> > *msgs = NULL;
> > nmsgs = 0;
> > n = 0;
> >
> > ...
> >
> > rv = nmsgs;
> >
> > return rv;
> > }
> > ```
> >
> > Where should I place this function? In `domain_conf.c` or somewhere
> > else?
> >
>
> Good question. We do not have that many cases of such functions, so
> it does not
> have one file in the repo, however you can start by looking at what
> are the main
> parameters of the function. This one operates on virDomainObj, so it
> can either
> be in the driver itself (which we are trying to avoid) and somewhere
> in src/conf
> based on further info. Searching for other such functions, I just
> used a simple
> grep for this:
>
> rg '\(virDomainObj \*' src/conf
>
> or:
>
> git grep '(virDomainObj \*' src/conf
>
> and you can see that it does not fit in domain_event or domain_audit
> and it will
> most probably just be in domain.conf just like you suggested. I must
> admit that
> it is not a very clean approach as src/conf is mostly handling
> virDomainDef, but
> I guess we do not have enough of similar functions that would
> facilitate another
> creation of a new file in the directory. I would not stress myself
> out too much
> about that as the worst thing that can happen is that someone will
> have a better
> idea.
>
>
Thanks so much! I learned a lot from you.
So I will implement the function and put it in domain_conf.c until
someone gives a better solution.
And I have another question, as it's freeze for 7.5.0, should I change
the version number in comments to 7.6.0 in the new patches? Thanks!
