[libvirt] [PATCH v2 04/12] tests: Add virfilemock -- the new super mock

[Martin Kletzander]

On Fri, Apr 07, 2017 at 10:08:35AM +0200, Erik Skultety wrote:
>> +#include <config.h>
>> +
>> +#include "virmock.h"
>> +#include "virfilemock.h"
>
>^^These are local, could you group them with the rest below the system ones?
>(This was pointed out to me during review once or twice).
>

Yeah, I forgot to get to the header cleaning part.  Most of this was
copy-paste from cgroupmock and then I just added my new header.

>> +
>> +#include <stdio.h>
>> +#include <stdlib.h>
>^^These get pulled in by virmock.h, but it's always nice to be explicit about
>being able to use "printf" :P.
>
>> +#include <unistd.h>
>
>I've successfully built the patch without ^^this header. You can drop it.
>
>> +#include <fcntl.h>
>
>...
>
>> +#include <sys/stat.h>
>> +#include <sys/types.h>
>
>It built without ^^these 2 as well.
>
>> +#include <dirent.h>
>
>^This one gets pulled in by virfile.h
>
>> +
>> +#include "viralloc.h"
>> +#include "virstring.h"
>> +#include "virfile.h"
>> +
>> +
>> +/* Mapping for prefix overrides */
>> +static size_t noverrides;
>> +static const char **overrides;
>> +
>> +/* nprefixes == noverrides, but two variables make it easier to use
>> + * VIR_*_ELEMENT macros */
>> +static size_t nprefixes;
>> +static const char **prefixes;
>> +
>> +/* TODO: callbacks */
>
>Just out of curiosity, what callbacks?
>

See commit message.  I don't like repeating myself.  Or should I copy
the commit message to the comment?

>[...]
>
>> +
>> +
>> +void
>> +virFileMockRemovePrefix(const char *prefix)
>> +{
>> +    size_t i = 0;
>> +
>> +    for (i = 0; i < noverrides; i++) {
>> +        if (STREQ(prefixes[i], prefix))
>
>Since you're removing a single element, you can just delete the prefix here and
>then break from the loop.
>

Yeah, thta was the first variant I went with, but then I didn't like the
extra indentation level, so I changed it to this.  Can we get a hacking
file section talking about this?  I don't like repainting the shed every
new series.

>> +            break;
>> +    }
>> +
>> +    if (i == noverrides)
>> +        return;
>
>You won't be needing ^^this then.
>
>> +
>> +    VIR_DELETE_ELEMENT(overrides, i, noverrides);
>> +    VIR_DELETE_ELEMENT(prefixes, i, nprefixes);
>> +}
>> +
>
>ACK with the adjustments (with the exception if it would somehow break BSD
>again :)).
>

I'll try to check this one (and the following ones as well) on BSD
before pushing.  Hopefully I'll manage to install the machine before
going home.

>Erik
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170407/66befaf6/attachment-0001.sig>