Re: [PATCH v2 1/3] test_driver: Implement virDomainGetMessages

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(a)gmail.com&gt; > > > --- > > > 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.