[libvirt] [PATCH 0/2] Couple of small fixes

These came to existence as I'm walking through code working on PR stuff (as in Persistent Reservations, not Public Relations). The fist patch is a bug fix, the second is an improvement. Michal Privoznik (2): virDomainDiskSourceFormatInternal: Avoid leaking @childBuf storage_conf: Make virStorageAuthDefFormat return void src/conf/domain_conf.c | 22 ++++++++-------------- src/conf/storage_conf.c | 6 ++---- src/util/virstoragefile.c | 4 +--- src/util/virstoragefile.h | 2 +- 4 files changed, 12 insertions(+), 22 deletions(-) -- 2.16.1

If formatting of storage encryption or private data fails we must jump to the error label instead of returning immediately otherwise @attrBuf and @childBuf might be leaked. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fdf5ad19e..b6ebe918d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22910,10 +22910,10 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, * as we found it. */ if (src->encryption && src->encryptionInherited && virStorageEncryptionFormat(&childBuf, src->encryption) < 0) - return -1; + goto error; if (virDomainDiskSourceFormatPrivateData(&childBuf, src, flags, xmlopt) < 0) - return -1; + goto error; if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0) goto error; -- 2.16.1

On Tue, Feb 20, 2018 at 12:30:31PM +0100, Michal Privoznik wrote:
If formatting of storage encryption or private data fails we must jump to the error label instead of returning immediately otherwise @attrBuf and @childBuf might be leaked.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fdf5ad19e..b6ebe918d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22910,10 +22910,10 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, * as we found it. */ if (src->encryption && src->encryptionInherited && virStorageEncryptionFormat(&childBuf, src->encryption) < 0) - return -1; + goto error;
if (virDomainDiskSourceFormatPrivateData(&childBuf, src, flags, xmlopt) < 0) - return -1; + goto error;
if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0) goto error;
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

This function returns nothing but zero. Therefore it makes no sense to have it returning an integer. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 18 ++++++------------ src/conf/storage_conf.c | 6 ++---- src/util/virstoragefile.c | 4 +--- src/util/virstoragefile.h | 2 +- 4 files changed, 10 insertions(+), 20 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b6ebe918d..92797f7c0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22901,10 +22901,8 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, * kept in the storage pool and would be overwritten anyway. * So avoid formatting it for volumes. */ if (src->auth && src->authInherited && - src->type != VIR_STORAGE_TYPE_VOLUME) { - if (virStorageAuthDefFormat(&childBuf, src->auth) < 0) - goto error; - } + src->type != VIR_STORAGE_TYPE_VOLUME) + virStorageAuthDefFormat(&childBuf, src->auth); /* If we found encryption as a child of <source>, then format it * as we found it. */ @@ -23100,10 +23098,8 @@ virDomainDiskDefFormat(virBufferPtr buf, /* Format as child of <disk> if defined there; otherwise, * if defined as child of <source>, then format later */ - if (def->src->auth && !def->src->authInherited) { - if (virStorageAuthDefFormat(buf, def->src->auth) < 0) - return -1; - } + if (def->src->auth && !def->src->authInherited) + virStorageAuthDefFormat(buf, def->src->auth); if (virDomainDiskSourceFormat(buf, def->src, def->startupPolicy, flags, xmlopt) < 0) @@ -23736,10 +23732,8 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf, if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI && - iscsisrc->src->auth) { - if (virStorageAuthDefFormat(buf, iscsisrc->src->auth) < 0) - return -1; - } + iscsisrc->src->auth) + virStorageAuthDefFormat(buf, iscsisrc->src->auth); virBufferAdjustIndent(buf, -2); if (!closedSource) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index f808cd291..b9135722c 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -953,10 +953,8 @@ virStoragePoolSourceFormat(virBufferPtr buf, virBufferAsprintf(buf, "<format type='%s'/>\n", format); } - if (src->auth) { - if (virStorageAuthDefFormat(buf, src->auth) < 0) - return -1; - } + if (src->auth) + virStorageAuthDefFormat(buf, src->auth); virBufferEscapeString(buf, "<vendor name='%s'/>\n", src->vendor); virBufferEscapeString(buf, "<product name='%s'/>\n", src->product); diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index f0b1d329c..d1972d6d1 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1891,7 +1891,7 @@ virStorageAuthDefParse(xmlDocPtr xml, xmlNodePtr root) } -int +void virStorageAuthDefFormat(virBufferPtr buf, virStorageAuthDefPtr authdef) { @@ -1908,8 +1908,6 @@ virStorageAuthDefFormat(virBufferPtr buf, &authdef->seclookupdef); virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</auth>\n"); - - return 0; } diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 9e8afa493..0095cd138 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -367,7 +367,7 @@ int virStorageFileGetSCSIKey(const char *path, void virStorageAuthDefFree(virStorageAuthDefPtr def); virStorageAuthDefPtr virStorageAuthDefCopy(const virStorageAuthDef *src); virStorageAuthDefPtr virStorageAuthDefParse(xmlDocPtr xml, xmlNodePtr root); -int virStorageAuthDefFormat(virBufferPtr buf, virStorageAuthDefPtr authdef); +void virStorageAuthDefFormat(virBufferPtr buf, virStorageAuthDefPtr authdef); virSecurityDeviceLabelDefPtr virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, -- 2.16.1

On Tue, Feb 20, 2018 at 12:30:32PM +0100, Michal Privoznik wrote:
This function returns nothing but zero. Therefore it makes no sense to have it returning an integer.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 18 ++++++------------ src/conf/storage_conf.c | 6 ++---- src/util/virstoragefile.c | 4 +--- src/util/virstoragefile.h | 2 +- 4 files changed, 10 insertions(+), 20 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (2)
-
Daniel P. Berrangé
-
Michal Privoznik