[libvirt PATCH v2 06/16] qemu: implement persistent file cache for nbdkit caps

[Jonathon Jongsma]

On 9/27/22 6:49 AM, Peter Krempa wrote:
>> +
>> +    if (virXPathLongLong("string(./selfctime)", ctxt, &l) < 0) {
>> +        virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                       _("missing selfctime in nbdkit capabilities XML"));
>> +        return -1;
>> +    }
>> +    nbdkitCaps->libvirtCtime = (time_t)l;
>> +
>> +    nbdkitCaps->libvirtVersion = 0;
>> +    if (virXPathULong("string(./selfvers)", ctxt, &lu) == 0)
>> +        nbdkitCaps->libvirtVersion = lu;
>> +
>> +    if (nbdkitCaps->libvirtCtime != virGetSelfLastChanged() ||
>> +        nbdkitCaps->libvirtVersion != LIBVIR_VERSION_NUMBER) {
>> +        VIR_DEBUG("Outdated capabilities in %s: libvirt changed "
>> +                  "(%lld vs %lld, %lu vs %lu), stopping load",
>> +                  nbdkitCaps->path,
>> +                  (long long)nbdkitCaps->libvirtCtime,
>> +                  (long long)virGetSelfLastChanged(),
>> +                  (unsigned long)nbdkitCaps->libvirtVersion,
>> +                  (unsigned long)LIBVIR_VERSION_NUMBER);
>> +        return 1;
>> +    }
>> +
>> +    if (qemuNbdkitCapsValidateBinary(nbdkitCaps, ctxt) < 0)
>> +        return -1;
>> +
>> +    if (virXPathLongLong("string(./nbdkitctime)", ctxt, &l) < 0) {
>> +        virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                       _("missing nbdkitctime in nbdkit capabilities XML"));
>> +        return -1;
>> +    }
>> +    nbdkitCaps->ctime = (time_t)l;
>> +
>> +    if (virXPathLongLong("string(./plugindirmtime)", ctxt, &l) == 0)
>> +        nbdkitCaps->pluginDirMtime = (time_t)l;
>> +
>> +    if (virXPathLongLong("string(./filterdirmtime)", ctxt, &l) == 0)
>> +        nbdkitCaps->filterDirMtime = (time_t)l;
> 
> Any reason why these are optional but the ctime of nbdkit is not?

Because the format function below does not serialize these values to the 
xml in the case where the directories don't exist. But I will change it 
so that we format these fields to the xml even in the case where the 
directories are not found. The time value will simply be 0 in this case.

> 
>> +
>> +    if (qemuNbdkitCapsParseFlags(nbdkitCaps, ctxt) < 0)
>> +        return -1;
>> +
>> +    if ((nbdkitCaps->version = virXPathString("string(./version)", ctxt)) == NULL) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                       _("missing version in nbdkit capabilities cache"));
>> +        return -1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +
>> +static void*
>> +virNbdkitCapsLoadFile(const char *filename,
>> +                      const char *binary,
>> +                      void *privData G_GNUC_UNUSED,
>> +                      bool *outdated)
>> +{
>> +    g_autoptr(qemuNbdkitCaps) nbdkitCaps = qemuNbdkitCapsNew(binary);
>> +    int ret;
>> +
>> +    if (!nbdkitCaps)
>> +        return NULL;
>> +
>> +    ret = qemuNbdkitCapsLoadCache(nbdkitCaps, filename);
>> +    if (ret < 0)
>> +        return NULL;
>> +    if (ret == 1) {
>> +        *outdated = true;
>> +        return NULL;
>> +    }
>> +
>> +    return g_steal_pointer(&nbdkitCaps);
>> +}
>> +
>> +
>> +static char*
>> +qemuNbdkitCapsFormatCache(qemuNbdkitCaps *nbdkitCaps)
>> +{
>> +    g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
>> +    size_t i;
>> +
>> +    virBufferAddLit(&buf, "<nbdkitCaps>\n");
>> +    virBufferAdjustIndent(&buf, 2);
>> +
>> +    virBufferEscapeString(&buf, "<path>%s</path>\n",
>> +                          nbdkitCaps->path);
>> +    virBufferAsprintf(&buf, "<nbdkitctime>%llu</nbdkitctime>\n",
>> +                      (long long)nbdkitCaps->ctime);
>> +    if (nbdkitCaps->pluginDirMtime > 0) {
>> +        virBufferAsprintf(&buf, "<plugindirmtime>%llu</plugindirmtime>\n",
> 
> The format substitution is specifying an unsigned long long ...
> 
>> +                          (long long)nbdkitCaps->pluginDirMtime);
> 
> while you typecast to long long. Similarly above in the parser you parse
> as long long instead of unsigned long long.

I have to admit, this code was blatantly copied and adapted from 
qemu_capabilities.c. So that code also has the same issue. But I can 
change this to format as a signed integer since the underlying time_t 
type is signed.

Jonathon