Let's make use of the auto __cleanup capabilities cleaning up any
now unnecessary goto paths. For virStorageAuthDefCopy use authdef
and ret as consistently as similar other code.
Signed-off-by: John Ferlan <jferlan@redhat.com>
---
src/conf/domain_conf.c | 29 +++++++++++------------------
src/conf/storage_conf.c | 6 ++----
src/qemu/qemu_parse_command.c | 6 ++----
src/util/virstoragefile.c | 33 ++++++++++++++-------------------
src/util/virstoragefile.h | 1 +
5 files changed, 30 insertions(+), 45 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1fc4c8a35a..5699a60549 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7578,10 +7578,9 @@ virDomainHostdevSubsysSCSIiSCSIDefParseXML(xmlNodePtr sourcenode,
virDomainHostdevSubsysSCSIPtr def,
xmlXPathContextPtr ctxt)
{
- int ret = -1;
int auth_secret_usage = -1;
xmlNodePtr cur;
- virStorageAuthDefPtr authdef = NULL;
+ VIR_AUTOPTR(virStorageAuthDef) authdef = NULL;
virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &def->u.iscsi;
/* For the purposes of command line creation, this needs to look
@@ -7594,23 +7593,23 @@ virDomainHostdevSubsysSCSIiSCSIDefParseXML(xmlNodePtr sourcenode,
if (!(iscsisrc->src->path = virXMLPropString(sourcenode, "name"))) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("missing iSCSI hostdev source path name"));
- goto cleanup;
+ return -1;
}
if (virDomainStorageNetworkParseHosts(sourcenode, &iscsisrc->src->hosts,
&iscsisrc->src->nhosts) < 0)
- goto cleanup;
+ return -1;
if (iscsisrc->src->nhosts < 1) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("missing the host address for the iSCSI hostdev"));
- goto cleanup;
+ return -1;
}
if (iscsisrc->src->nhosts > 1) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("only one source host address may be specified "
"for the iSCSI hostdev"));
- goto cleanup;
+ return -1;
}
cur = sourcenode->children;
@@ -7618,30 +7617,25 @@ virDomainHostdevSubsysSCSIiSCSIDefParseXML(xmlNodePtr sourcenode,
if (cur->type == XML_ELEMENT_NODE &&
virXMLNodeNameEqual(cur, "auth")) {
if (!(authdef = virStorageAuthDefParse(cur, ctxt)))
- goto cleanup;
+ return -1;
if ((auth_secret_usage =
virSecretUsageTypeFromString(authdef->secrettype)) < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("invalid secret type %s"),
authdef->secrettype);
- goto cleanup;
+ return -1;
}
if (auth_secret_usage != VIR_SECRET_USAGE_TYPE_ISCSI) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("hostdev invalid secret type '%s'"),
authdef->secrettype);
- goto cleanup;
+ return -1;
}
- iscsisrc->src->auth = authdef;
- authdef = NULL;
+ VIR_STEAL_PTR(iscsisrc->src->auth, authdef);
}
cur = cur->next;
}
- ret = 0;
-
- cleanup:
- virStorageAuthDefFree(authdef);
- return ret;
+ return 0;
}
static int
@@ -9684,7 +9678,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
virStorageEncryptionPtr encryption = NULL;
char *serial = NULL;
char *startupPolicy = NULL;
- virStorageAuthDefPtr authdef = NULL;
+ VIR_AUTOPTR(virStorageAuthDef) authdef = NULL;
char *tray = NULL;
char *removable = NULL;
char *logical_block_size = NULL;
@@ -10094,7 +10088,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
VIR_FREE(target);
VIR_FREE(tray);
VIR_FREE(removable);
- virStorageAuthDefFree(authdef);
VIR_FREE(devaddr);
VIR_FREE(serial);
virStorageEncryptionFree(encryption);
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 1ee31ca676..9cd3d836a3 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -458,7 +458,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
int nsource;
size_t i;
virStoragePoolOptionsPtr options;
- virStorageAuthDefPtr authdef = NULL;
+ VIR_AUTOPTR(virStorageAuthDef) authdef = NULL;
char *name = NULL;
char *port = NULL;
char *ver = NULL;
@@ -584,8 +584,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
goto cleanup;
}
- source->auth = authdef;
- authdef = NULL;
+ VIR_STEAL_PTR(source->auth, authdef);
}
/* Option protocol version string (NFSvN) */
@@ -615,7 +614,6 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
VIR_FREE(port);
VIR_FREE(nodeset);
- virStorageAuthDefFree(authdef);
return ret;
}
diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c
index c4650f01e0..81691cb85e 100644
--- a/src/qemu/qemu_parse_command.c
+++ b/src/qemu/qemu_parse_command.c
@@ -59,7 +59,7 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr uri,
char *sock = NULL;
char *volimg = NULL;
char *secret = NULL;
- virStorageAuthDefPtr authdef = NULL;
+ VIR_AUTOPTR(virStorageAuthDef) authdef = NULL;
if (VIR_ALLOC(def->src->hosts) < 0)
goto error;
@@ -133,8 +133,7 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr uri,
if (VIR_STRDUP(authdef->secrettype, secrettype) < 0)
goto error;
}
- def->src->auth = authdef;
- authdef = NULL;
+ VIR_STEAL_PTR(def->src->auth, authdef);
/* Cannot formulate a secretType (eg, usage or uuid) given
* what is provided.
@@ -152,7 +151,6 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr uri,
error:
virStorageNetHostDefClear(def->src->hosts);
VIR_FREE(def->src->hosts);
- virStorageAuthDefFree(authdef);
goto cleanup;
}
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 8319ba9c8c..b3f5e0204d 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1879,26 +1879,24 @@ virStorageAuthDefFree(virStorageAuthDefPtr authdef)
virStorageAuthDefPtr
virStorageAuthDefCopy(const virStorageAuthDef *src)
{
- virStorageAuthDefPtr ret;
+ VIR_AUTOPTR(virStorageAuthDef) authdef = NULL;
+ virStorageAuthDefPtr ret = NULL;
- if (VIR_ALLOC(ret) < 0)
+ if (VIR_ALLOC(authdef) < 0)
return NULL;
- if (VIR_STRDUP(ret->username, src->username) < 0)
- goto error;
+ if (VIR_STRDUP(authdef->username, src->username) < 0)
+ return NULL;
/* Not present for storage pool, but used for disk source */
- if (VIR_STRDUP(ret->secrettype, src->secrettype) < 0)
- goto error;
- ret->authType = src->authType;
+ if (VIR_STRDUP(authdef->secrettype, src->secrettype) < 0)
+ return NULL;
+ authdef->authType = src->authType;
- if (virSecretLookupDefCopy(&ret->seclookupdef, &src->seclookupdef) < 0)
- goto error;
+ if (virSecretLookupDefCopy(&authdef->seclookupdef, &src->seclookupdef) < 0)
+ return NULL;
+ VIR_STEAL_PTR(ret, authdef);
return ret;
-
- error:
- virStorageAuthDefFree(ret);
- return NULL;
}
@@ -1907,7 +1905,7 @@ virStorageAuthDefParse(xmlNodePtr node,
xmlXPathContextPtr ctxt)
{
xmlNodePtr saveNode = ctxt->node;
- virStorageAuthDefPtr authdef = NULL;
+ VIR_AUTOPTR(virStorageAuthDef) authdef = NULL;
virStorageAuthDefPtr ret = NULL;
xmlNodePtr secretnode = NULL;
char *authtype = NULL;
@@ -1958,7 +1956,6 @@ virStorageAuthDefParse(xmlNodePtr node,
cleanup:
VIR_FREE(authtype);
- virStorageAuthDefFree(authdef);
ctxt->node = saveNode;
return ret;
@@ -2832,7 +2829,7 @@ virStorageSourceParseRBDColonString(const char *rbdstr,
{
char *options = NULL;
char *p, *e, *next;
- virStorageAuthDefPtr authdef = NULL;
+ VIR_AUTOPTR(virStorageAuthDef) authdef = NULL;
/* optionally skip the "rbd:" prefix if provided */
if (STRPREFIX(rbdstr, "rbd:"))
@@ -2895,9 +2892,8 @@ virStorageSourceParseRBDColonString(const char *rbdstr,
if (VIR_STRDUP(authdef->secrettype,
virSecretUsageTypeToString(VIR_SECRET_USAGE_TYPE_CEPH)) < 0)
goto error;
- src->auth = authdef;
+ VIR_STEAL_PTR(src->auth, authdef);
src->authInherited = true;
- authdef = NULL;
/* Cannot formulate a secretType (eg, usage or uuid) given
* what is provided.
@@ -2936,7 +2932,6 @@ virStorageSourceParseRBDColonString(const char *rbdstr,
error:
VIR_FREE(options);
- virStorageAuthDefFree(authdef);
return -1;
}
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index a98d5103fa..f1164dde9b 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -543,4 +543,5 @@ void virStorageFileReportBrokenChain(int errcode,
virStorageSourcePtr src,
virStorageSourcePtr parent);
+VIR_DEFINE_AUTOPTR_FUNC(virStorageAuthDef, virStorageAuthDefFree);
#endif /* LIBVIRT_VIRSTORAGEFILE_H */
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Feb 06, 2019 at 08:41:33AM -0500, John Ferlan wrote:
> Let's make use of the auto __cleanup capabilities cleaning up any
> now unnecessary goto paths. For virStorageAuthDefCopy use authdef
> and ret as consistently as similar other code.
>
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
> src/conf/domain_conf.c | 29 +++++++++++------------------
> src/conf/storage_conf.c | 6 ++----
> src/qemu/qemu_parse_command.c | 6 ++----
> src/util/virstoragefile.c | 33 ++++++++++++++-------------------
> src/util/virstoragefile.h | 1 +
> 5 files changed, 30 insertions(+), 45 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 1fc4c8a35a..5699a60549 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -7578,10 +7578,9 @@ virDomainHostdevSubsysSCSIiSCSIDefParseXML(xmlNodePtr sourcenode,
> virDomainHostdevSubsysSCSIPtr def,
> xmlXPathContextPtr ctxt)
> {
> - int ret = -1;
> int auth_secret_usage = -1;
> xmlNodePtr cur;
> - virStorageAuthDefPtr authdef = NULL;
> + VIR_AUTOPTR(virStorageAuthDef) authdef = NULL;
For better readability I prefer declaring VIR_AUTO variables at the end of the
declare block (multiple occurrences throughout the patch)...
...
> + VIR_STEAL_PTR(iscsisrc->src->auth, authdef);
^Unrelated change, there should be a trivial patch preceding this one taking
care of the VIR_STEAL_PTR replacements.
...
>
> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> index a98d5103fa..f1164dde9b 100644
> --- a/src/util/virstoragefile.h
> +++ b/src/util/virstoragefile.h
> @@ -543,4 +543,5 @@ void virStorageFileReportBrokenChain(int errcode,
> virStorageSourcePtr src,
> virStorageSourcePtr parent);
>
> +VIR_DEFINE_AUTOPTR_FUNC(virStorageAuthDef, virStorageAuthDefFree);
^This defines a static function, so the ';' is unnecessary, it doesn't hurt,
but you know, consistency ;). Also, keep the original newline below.
Reviewed-by: Erik Skultety <eskultet@redhat.com>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 2/7/19 4:10 AM, Erik Skultety wrote:
> On Wed, Feb 06, 2019 at 08:41:33AM -0500, John Ferlan wrote:
>> Let's make use of the auto __cleanup capabilities cleaning up any
>> now unnecessary goto paths. For virStorageAuthDefCopy use authdef
>> and ret as consistently as similar other code.
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>> src/conf/domain_conf.c | 29 +++++++++++------------------
>> src/conf/storage_conf.c | 6 ++----
>> src/qemu/qemu_parse_command.c | 6 ++----
>> src/util/virstoragefile.c | 33 ++++++++++++++-------------------
>> src/util/virstoragefile.h | 1 +
>> 5 files changed, 30 insertions(+), 45 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 1fc4c8a35a..5699a60549 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -7578,10 +7578,9 @@ virDomainHostdevSubsysSCSIiSCSIDefParseXML(xmlNodePtr sourcenode,
>> virDomainHostdevSubsysSCSIPtr def,
>> xmlXPathContextPtr ctxt)
>> {
>> - int ret = -1;
>> int auth_secret_usage = -1;
>> xmlNodePtr cur;
>> - virStorageAuthDefPtr authdef = NULL;
>> + VIR_AUTOPTR(virStorageAuthDef) authdef = NULL;
>
> For better readability I prefer declaring VIR_AUTO variables at the end of the
> declare block (multiple occurrences throughout the patch)...
>
> ...
>
I don't mind moving them. I generally just try to keep the usages together.
>> + VIR_STEAL_PTR(iscsisrc->src->auth, authdef);
>
> ^Unrelated change, there should be a trivial patch preceding this one taking
> care of the VIR_STEAL_PTR replacements.
>
> ...
You'll find this sprinkled throughout - generating separate patches
could explode the series into perhaps 30+ patches which I doubt anyone
would jump at the chance to review. I can separate before pushing and I
assume by extracting it/them I can just add your R-by too...
>
>>
>> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
>> index a98d5103fa..f1164dde9b 100644
>> --- a/src/util/virstoragefile.h
>> +++ b/src/util/virstoragefile.h
>> @@ -543,4 +543,5 @@ void virStorageFileReportBrokenChain(int errcode,
>> virStorageSourcePtr src,
>> virStorageSourcePtr parent);
>>
>> +VIR_DEFINE_AUTOPTR_FUNC(virStorageAuthDef, virStorageAuthDefFree);
>
> ^This defines a static function, so the ';' is unnecessary, it doesn't hurt,
> but you know, consistency ;). Also, keep the original newline below.
>
Perhaps because I had recently reviewed Cole's series about removing the
semicolon from VIR_ENUM_DECL, VIR_ENUM_IMPL, VIR_LOG_INIT, and
VIR_ONCE_GLOBAL_INIT - it was just "fresh" in my mind that macros should
have semicolons (moreso than actually looking at that definition). I
can change them all.
Tks -
John
> Reviewed-by: Erik Skultety <eskultet@redhat.com>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Feb 07, 2019 at 07:12:08AM -0500, John Ferlan wrote:
>
>
> On 2/7/19 4:10 AM, Erik Skultety wrote:
> > On Wed, Feb 06, 2019 at 08:41:33AM -0500, John Ferlan wrote:
> >> Let's make use of the auto __cleanup capabilities cleaning up any
> >> now unnecessary goto paths. For virStorageAuthDefCopy use authdef
> >> and ret as consistently as similar other code.
> >>
> >> Signed-off-by: John Ferlan <jferlan@redhat.com>
> >> ---
> >> src/conf/domain_conf.c | 29 +++++++++++------------------
> >> src/conf/storage_conf.c | 6 ++----
> >> src/qemu/qemu_parse_command.c | 6 ++----
> >> src/util/virstoragefile.c | 33 ++++++++++++++-------------------
> >> src/util/virstoragefile.h | 1 +
> >> 5 files changed, 30 insertions(+), 45 deletions(-)
> >>
> >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> >> index 1fc4c8a35a..5699a60549 100644
> >> --- a/src/conf/domain_conf.c
> >> +++ b/src/conf/domain_conf.c
> >> @@ -7578,10 +7578,9 @@ virDomainHostdevSubsysSCSIiSCSIDefParseXML(xmlNodePtr sourcenode,
> >> virDomainHostdevSubsysSCSIPtr def,
> >> xmlXPathContextPtr ctxt)
> >> {
> >> - int ret = -1;
> >> int auth_secret_usage = -1;
> >> xmlNodePtr cur;
> >> - virStorageAuthDefPtr authdef = NULL;
> >> + VIR_AUTOPTR(virStorageAuthDef) authdef = NULL;
> >
> > For better readability I prefer declaring VIR_AUTO variables at the end of the
> > declare block (multiple occurrences throughout the patch)...
> >
> > ...
> >
>
> I don't mind moving them. I generally just try to keep the usages together.
>
>
> >> + VIR_STEAL_PTR(iscsisrc->src->auth, authdef);
> >
> > ^Unrelated change, there should be a trivial patch preceding this one taking
> > care of the VIR_STEAL_PTR replacements.
> >
> > ...
>
> You'll find this sprinkled throughout - generating separate patches
> could explode the series into perhaps 30+ patches which I doubt anyone
> would jump at the chance to review. I can separate before pushing and I
> assume by extracting it/them I can just add your R-by too...
That's why I mentioned trivial, I don't need to review those, as long as you
split them before pushing, I'm fine with that and the Rb applies, sorry for not
being clearer about that.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Feb 07, 2019 at 07:12:08AM -0500, John Ferlan wrote: > > >On 2/7/19 4:10 AM, Erik Skultety wrote: >> On Wed, Feb 06, 2019 at 08:41:33AM -0500, John Ferlan wrote: >>> Let's make use of the auto __cleanup capabilities cleaning up any >>> now unnecessary goto paths. For virStorageAuthDefCopy use authdef >>> and ret as consistently as similar other code. >>> >>> Signed-off-by: John Ferlan <jferlan@redhat.com> >>> --- >>> src/conf/domain_conf.c | 29 +++++++++++------------------ >>> src/conf/storage_conf.c | 6 ++---- >>> src/qemu/qemu_parse_command.c | 6 ++---- >>> src/util/virstoragefile.c | 33 ++++++++++++++------------------- >>> src/util/virstoragefile.h | 1 + >>> 5 files changed, 30 insertions(+), 45 deletions(-) >>> >>> + VIR_STEAL_PTR(iscsisrc->src->auth, authdef); >> >> ^Unrelated change, there should be a trivial patch preceding this one taking >> care of the VIR_STEAL_PTR replacements. >> >> ... > >You'll find this sprinkled throughout - generating separate patches >could explode the series into perhaps 30+ patches which I doubt anyone >would jump at the chance to review. I can separate before pushing and I >assume by extracting it/them I can just add your R-by too... > Splitting the changes into simple patches doing one thing at a time actually makes review easier, even though it makes the series longer. Jano -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, 2019-02-07 at 07:12 -0500, John Ferlan wrote: > On 2/7/19 4:10 AM, Erik Skultety wrote: > > On Wed, Feb 06, 2019 at 08:41:33AM -0500, John Ferlan wrote: > > > +VIR_DEFINE_AUTOPTR_FUNC(virStorageAuthDef, virStorageAuthDefFree); > > > > ^This defines a static function, so the ';' is unnecessary, it doesn't hurt, > > but you know, consistency ;). Also, keep the original newline below. > > Perhaps because I had recently reviewed Cole's series about removing the You meant *adding* here, right? > semicolon from VIR_ENUM_DECL, VIR_ENUM_IMPL, VIR_LOG_INIT, and > VIR_ONCE_GLOBAL_INIT - it was just "fresh" in my mind that macros should > have semicolons (moreso than actually looking at that definition). I > can change them all. Please keep the semicolon! If a macro is used like a function, then its call sites should also look like those of a function. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Feb 07, 2019 at 04:24:13PM +0100, Andrea Bolognani wrote: > On Thu, 2019-02-07 at 07:12 -0500, John Ferlan wrote: > > On 2/7/19 4:10 AM, Erik Skultety wrote: > > > On Wed, Feb 06, 2019 at 08:41:33AM -0500, John Ferlan wrote: > > > > +VIR_DEFINE_AUTOPTR_FUNC(virStorageAuthDef, virStorageAuthDefFree); > > > > > > ^This defines a static function, so the ';' is unnecessary, it doesn't hurt, > > > but you know, consistency ;). Also, keep the original newline below. > > > > Perhaps because I had recently reviewed Cole's series about removing the > > You meant *adding* here, right? > > > semicolon from VIR_ENUM_DECL, VIR_ENUM_IMPL, VIR_LOG_INIT, and > > VIR_ONCE_GLOBAL_INIT - it was just "fresh" in my mind that macros should > > have semicolons (moreso than actually looking at that definition). I > > can change them all. > > Please keep the semicolon! If a macro is used like a function, then > its call sites should also look like those of a function. Except VIR_DEFINE_AUTOPTR_FUNC macro is not used like a function, it defines a function. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, 2019-02-07 at 16:55 +0100, Erik Skultety wrote: > On Thu, Feb 07, 2019 at 04:24:13PM +0100, Andrea Bolognani wrote: > > Please keep the semicolon! If a macro is used like a function, then > > its call sites should also look like those of a function. > > Except VIR_DEFINE_AUTOPTR_FUNC macro is not used like a function, it defines a > function. Sure, and the way you make that happen is by writing MACRO_NAME(argument_one, argument_two); How is that not using it like a function? :) -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Feb 07, 2019 at 05:11:24PM +0100, Andrea Bolognani wrote: > On Thu, 2019-02-07 at 16:55 +0100, Erik Skultety wrote: > > On Thu, Feb 07, 2019 at 04:24:13PM +0100, Andrea Bolognani wrote: > > > Please keep the semicolon! If a macro is used like a function, then > > > its call sites should also look like those of a function. > > > > Except VIR_DEFINE_AUTOPTR_FUNC macro is not used like a function, it defines a > > function. > > Sure, and the way you make that happen is by writing > > MACRO_NAME(argument_one, argument_two); > > How is that not using it like a function? :) I may need to replace my dictionary, because the way I understand the expression "like a function" is that the macro is called like function and it behaves like a function, i.e. returns a value, IOW by using the macro its expansion will perform the usual set of operation on the stack that a function call involves (push parameters, return address, jump into function, pop the return value...) Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, 2019-02-07 at 17:36 +0100, Erik Skultety wrote: > On Thu, Feb 07, 2019 at 05:11:24PM +0100, Andrea Bolognani wrote: > > On Thu, 2019-02-07 at 16:55 +0100, Erik Skultety wrote: > > > On Thu, Feb 07, 2019 at 04:24:13PM +0100, Andrea Bolognani wrote: > > > > Please keep the semicolon! If a macro is used like a function, then > > > > its call sites should also look like those of a function. > > > > > > Except VIR_DEFINE_AUTOPTR_FUNC macro is not used like a function, it defines a > > > function. > > > > Sure, and the way you make that happen is by writing > > > > MACRO_NAME(argument_one, argument_two); > > > > How is that not using it like a function? :) > > I may need to replace my dictionary, because the way I understand the > expression "like a function" is that the macro is called like function and it > behaves like a function, i.e. returns a value, IOW by using the macro its > expansion will perform the usual set of operation on the stack that a function > call involves (push parameters, return address, jump into function, > pop the return value...) I guess abort() is not a function either then, since it doesn't have any parameters to push or values to return! :P Anyway, the point is that we have already started mandating the use of semicolon after other macros that expand to definitions, such as VIR_ENUM_DECL(), VIR_ENUM_IMPL(), and VIR_ONCE_GLOBAL_INIT(): we should do the same for VIR_DEFINE_AUTOPTR_FUNC() and increase consistency instead of pushing in the opposite direction. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 2/7/19 11:54 AM, Andrea Bolognani wrote: > On Thu, 2019-02-07 at 17:36 +0100, Erik Skultety wrote: >> On Thu, Feb 07, 2019 at 05:11:24PM +0100, Andrea Bolognani wrote: >>> On Thu, 2019-02-07 at 16:55 +0100, Erik Skultety wrote: >>>> On Thu, Feb 07, 2019 at 04:24:13PM +0100, Andrea Bolognani wrote: >>>>> Please keep the semicolon! If a macro is used like a function, then >>>>> its call sites should also look like those of a function. >>>> >>>> Except VIR_DEFINE_AUTOPTR_FUNC macro is not used like a function, it defines a >>>> function. >>> >>> Sure, and the way you make that happen is by writing >>> >>> MACRO_NAME(argument_one, argument_two); >>> >>> How is that not using it like a function? :) >> >> I may need to replace my dictionary, because the way I understand the >> expression "like a function" is that the macro is called like function and it >> behaves like a function, i.e. returns a value, IOW by using the macro its >> expansion will perform the usual set of operation on the stack that a function >> call involves (push parameters, return address, jump into function, >> pop the return value...) > > I guess abort() is not a function either then, since it doesn't have > any parameters to push or values to return! :P > > Anyway, the point is that we have already started mandating the use > of semicolon after other macros that expand to definitions, such as > VIR_ENUM_DECL(), VIR_ENUM_IMPL(), and VIR_ONCE_GLOBAL_INIT(): we > should do the same for VIR_DEFINE_AUTOPTR_FUNC() and increase > consistency instead of pushing in the opposite direction. > Since the issue is consistency, how about a patch that adds the ; to the existing VIR_DEFINE_AUTOPTR_FUNC's that don't have it? Ironically, I found one that has it: VIR_DEFINE_AUTOPTR_FUNC(virSEVCapability, virSEVCapabilitiesFree); John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Feb 07, 2019 at 12:12:14PM -0500, John Ferlan wrote: > > > On 2/7/19 11:54 AM, Andrea Bolognani wrote: > > On Thu, 2019-02-07 at 17:36 +0100, Erik Skultety wrote: > >> On Thu, Feb 07, 2019 at 05:11:24PM +0100, Andrea Bolognani wrote: > >>> On Thu, 2019-02-07 at 16:55 +0100, Erik Skultety wrote: > >>>> On Thu, Feb 07, 2019 at 04:24:13PM +0100, Andrea Bolognani wrote: > >>>>> Please keep the semicolon! If a macro is used like a function, then > >>>>> its call sites should also look like those of a function. > >>>> > >>>> Except VIR_DEFINE_AUTOPTR_FUNC macro is not used like a function, it defines a > >>>> function. > >>> > >>> Sure, and the way you make that happen is by writing > >>> > >>> MACRO_NAME(argument_one, argument_two); > >>> > >>> How is that not using it like a function? :) > >> > >> I may need to replace my dictionary, because the way I understand the > >> expression "like a function" is that the macro is called like function and it > >> behaves like a function, i.e. returns a value, IOW by using the macro its > >> expansion will perform the usual set of operation on the stack that a function > >> call involves (push parameters, return address, jump into function, > >> pop the return value...) > > > > I guess abort() is not a function either then, since it doesn't have > > any parameters to push or values to return! :P > > > > Anyway, the point is that we have already started mandating the use > > of semicolon after other macros that expand to definitions, such as > > VIR_ENUM_DECL(), VIR_ENUM_IMPL(), and VIR_ONCE_GLOBAL_INIT(): we > > should do the same for VIR_DEFINE_AUTOPTR_FUNC() and increase > > consistency instead of pushing in the opposite direction. > > > > Since the issue is consistency, how about a patch that adds the ; to the > existing VIR_DEFINE_AUTOPTR_FUNC's that don't have it? Ironically, I > found one that has it: > > VIR_DEFINE_AUTOPTR_FUNC(virSEVCapability, virSEVCapabilitiesFree); That sounds reasonable, I didn't want to end up with one set of macros following one direction and another following other one, so go ahead ;). Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.