[PATCH v2 1/3] test_driver: Implement virDomainGetMessages

[Martin Kletzander]

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 at gmail.com>
>> > ---
>> > 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,
>Luke
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20210628/0d978da1/attachment-0001.sig>