Re: [libvirt PATCH v2 06/16] qemu: implement persistent file cache for nbdkit caps
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