Re: [Xen-devel] [PATCH v4 4/7] libxl: add infrastructure to track and query 'recent' domids

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Paul Durrant writes ("[PATCH v4 4/7] libxl: add infrastructure to track and 
query 'recent' domids"):
> A domid is considered recent if the domain it represents was destroyed
> less than a specified number of seconds ago. The number can be set using
> the environment variable LIBXL_DOMID_REUSE_TIMEOUT. If the variable does
> not exist then a default value of 60s is used.
> 
> Whenever a domain is destroyed, a time-stamped record will be written into
> a history file (/var/run/xen/domid-history). To avoid the history file
> growing too large, any records with time-stamps that indicate that the
> age of a domid has exceeded the re-use timeout will also be purged.
> 
> A new utility function, libxl__is_recent_domid(), has been added. This
> function reads the same history file checking whether a specified domid
> has a record that does not exceed the re-use timeout. Since this utility
> function does not write to the file, no records are actually purged by it.
> 
> NOTE: The history file is purged on boot to it is safe to use
>       CLOCK_MONOTONIC as a time source.

Thanks.

> diff --git a/tools/helpers/xen-init-dom0.c b/tools/helpers/xen-init-dom0.c
> index a1e5729458..56f69ab66f 100644
> --- a/tools/helpers/xen-init-dom0.c
> +++ b/tools/helpers/xen-init-dom0.c

Thanks.  This part is good.

> +static void libxl__mark_domid_recent(libxl__gc *gc, uint32_t domid)
> +{
...
> +    clock_gettime(CLOCK_MONOTONIC, &ts);
> +
> +    while (of && fgets(line, sizeof(line), of)) {
> +        unsigned long sec;
> +        unsigned int ignored;
> +
> +        if (sscanf(line, "%lu %u", &sec, &ignored) != 2) {
> +            LOGED(ERROR, domid, "ignoring malformed line: %s", line);
> +            continue;
> +        }
> +
> +        if (ts.tv_sec - sec > timeout)
> +            continue; /* Ignore expired entries */

I find this code quite similar to this code

> +bool libxl__is_domid_recent(libxl__gc *gc, uint32_t domid)
...
> +    while (!feof(f)) {
> +        unsigned long sec;
> +        unsigned int check;
> +
> +        if (fscanf(f, "%lu %u", &sec, &check) != 2)
> +            continue;
> +
> +        if (check == domid && ts.tv_sec - sec <= timeout) {
> +            recent = true;
> +            break;
> +        }

This makes me uncomfortable.  For one thing, it duplicates any bugs
(and there is at least one error handling anomaly here) and duplicates
my review effort looking for those bugs :-).

Do you think this can be combined somehow ?  Possibilities seem to
include: explicit read_recent_{init,entry,finish} functions; a single
function with a "per-entry" callback; same but with a macro.  If you
do that you can also have it have handle the "file does not exist"
special case.

Also, the stdio error handling doesn't seem quite right.  What if
fgets gets a read error ?

While I'm looking at this, I found

> +    while (of && fgets(line, sizeof(line), of)) {

that confusing.  of!=0 is an entry condition, not a termination
condition.  When I first read this I looked for modifications to of in
the loop but of course there aren't any.

If you really want to keep it like this I guess I will tolerate it to
avoid the argument...

> +    fflush(nf);

Missing error check.  Also you should fclose here, not just fflush.
When you do that, set nf to 0 so the out block doesn't re-close it.

> +bool libxl__is_domid_recent(libxl__gc *gc, uint32_t domid)
> +{
...
> +    name = GCSPRINTF("%s/domid-history", libxl__run_dir_path());
> +    f = fopen(name, "r");
> +    if (!f) {
> +        if (errno != ENOENT) LOGED(WARN, domid, "failed to open %s", name);

I think this (and other unexpected manipulation failures) should be
fatal, rather than merely a warning.  That means your function should
return rc.  Sorry.  Same goes for libxl__mark_domid_recent.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel