----- "Daniel Veillard" <veillard@redhat.com> wrote:

...

+ default: + VIR_ERROR(_("unexpected secret usage type %d"), def->usage_type); + break;

Hum, since the virSecretDefPtr is allocated by our own code, it's probably better to remove the default so that the compiler can tell us we missed one enum case if new ones gets added.

...

+ type_str = virXPathString(conn, "string(./usage/@type)", ctxt); + if (type_str == NULL) { + virSecretReportError(conn, VIR_ERR_XML_ERROR, "%s", + _("unknown secret usage type"));

_("missing secret usage type") would be more appropriate I guess

...

+ case VIR_SECRET_USAGE_TYPE_VOLUME: + def->usage.volume = virXPathString(conn, "string(./usage/volume)", + ctxt); + break; + + default:

Again default: here means a mismatch between virSecretUsageTypeTypeFromString and this function, best handled statically IMHO.

Thanks for the review, attached is an updated patch. Mirek