[libvirt] [PATCH 0/2] syntax check: hanging braces
Another case where it is worth enforcing a common existing style, and documenting it in HACKING. Here, there were fewer offenders, so the series is not split in as many pieces. Obviously depends on my earlier series for if-else {} checks, as otherwise this will detect several 'else' as lacking a hanging brace. Eric Blake (2): maint: use hanging curly braces maint: tighten curly brace syntax checking HACKING | 27 +++++++++++++++++++++++++++ cfg.mk | 18 ++++++++++++------ docs/hacking.html.in | 31 +++++++++++++++++++++++++++++++ src/conf/domain_conf.c | 6 ++---- src/interface/interface_backend_netcf.c | 9 +++------ src/network/bridge_driver.c | 3 +-- src/util/virfile.c | 3 +-- src/util/virnetdev.c | 3 +-- src/util/virnetdevmacvlan.c | 5 ++--- src/util/virtypedparam.c | 3 +-- src/util/virutil.c | 6 ++---- src/vbox/vbox_common.c | 4 ++-- tests/seclabeltest.c | 6 ++---- 13 files changed, 87 insertions(+), 37 deletions(-) -- 1.9.3
Our style overwhelmingly uses hanging braces (the open brace hangs at the end of the compound condition, rather than on its own line), with the primary exception of the top level function body. Fix the few remaining outliers, before adding a syntax check in a later patch. * src/interface/interface_backend_netcf.c (netcfStateReload) (netcfInterfaceClose, netcf_to_vir_err): Correct use of { in compound statement. * src/conf/domain_conf.c (virDomainHostdevDefFormatSubsys) (virDomainHostdevDefFormatCaps): Likewise. * src/network/bridge_driver.c (networkAllocateActualDevice): Likewise. * src/util/virfile.c (virBuildPathInternal): Likewise. * src/util/virnetdev.c (virNetDevGetVirtualFunctions): Likewise. * src/util/virnetdevmacvlan.c (virNetDevMacVLanVPortProfileCallback): Likewise. * src/util/virtypedparam.c (virTypedParameterAssign): Likewise. * src/util/virutil.c (virGetWin32DirectoryRoot) (virFileWaitForDevices): Likewise. * src/vbox/vbox_common.c (vboxDumpNetwork): Likewise. * tests/seclabeltest.c (main): Likewise. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.c | 6 ++---- src/interface/interface_backend_netcf.c | 9 +++------ src/network/bridge_driver.c | 3 +-- src/util/virfile.c | 3 +-- src/util/virnetdev.c | 3 +-- src/util/virnetdevmacvlan.c | 5 ++--- src/util/virtypedparam.c | 3 +-- src/util/virutil.c | 6 ++---- src/vbox/vbox_common.c | 4 ++-- tests/seclabeltest.c | 6 ++---- 10 files changed, 17 insertions(+), 31 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 508de21..efc2eb0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16068,8 +16068,7 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf, virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); - switch (def->source.subsys.type) - { + switch (def->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: if (usbsrc->vendor) { virBufferAsprintf(buf, "<vendor id='0x%.4x'/>\n", usbsrc->vendor); @@ -16146,8 +16145,7 @@ virDomainHostdevDefFormatCaps(virBufferPtr buf, virBufferAddLit(buf, "<source>\n"); virBufferAdjustIndent(buf, 2); - switch (def->source.caps.type) - { + switch (def->source.caps.type) { case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_STORAGE: virBufferEscapeString(buf, "<block>%s</block>\n", def->source.caps.u.storage.block); diff --git a/src/interface/interface_backend_netcf.c b/src/interface/interface_backend_netcf.c index 1b9ace5..c55a080 100644 --- a/src/interface/interface_backend_netcf.c +++ b/src/interface/interface_backend_netcf.c @@ -126,8 +126,7 @@ netcfStateReload(void) virObjectLock(driverState); ncf_close(driverState->netcf); - if (ncf_init(&driverState->netcf, NULL) != 0) - { + if (ncf_init(&driverState->netcf, NULL) != 0) { /* this isn't a good situation, because we can't shut down the * driver as there may still be connections to it. If we set * the netcf handle to NULL, any subsequent calls to netcf @@ -178,8 +177,7 @@ netcfGetMinimalDefForDevice(struct netcf_if *iface) static int netcf_to_vir_err(int netcf_errcode) { - switch (netcf_errcode) - { + switch (netcf_errcode) { case NETCF_NOERROR: /* no error, everything ok */ return VIR_ERR_OK; @@ -285,8 +283,7 @@ static int netcfInterfaceClose(virConnectPtr conn) { - if (conn->interfacePrivateData != NULL) - { + if (conn->interfacePrivateData != NULL) { virObjectUnref(conn->interfacePrivateData); conn->interfacePrivateData = NULL; } diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 2886866..0bc4a4d 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -3871,8 +3871,7 @@ networkAllocateActualDevice(virDomainDefPtr dom, iface->data.network.actual->data.hostdev.def.source.subsys.type = dev->type; iface->data.network.actual->data.hostdev.def.source.subsys.u.pci.addr = dev->device.pci; - switch (netdef->forward.driverName) - { + switch (netdef->forward.driverName) { case VIR_NETWORK_FORWARD_DRIVER_NAME_DEFAULT: backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT; break; diff --git a/src/util/virfile.c b/src/util/virfile.c index b6f5e3f..cfb6cc1 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1172,8 +1172,7 @@ virBuildPathInternal(char **path, ...) path_component = va_arg(ap, char *); virBufferAdd(&buf, path_component, -1); - while ((path_component = va_arg(ap, char *)) != NULL) - { + while ((path_component = va_arg(ap, char *)) != NULL) { virBufferAddChar(&buf, '/'); virBufferAdd(&buf, path_component, -1); } diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index cf526ec..58bf27c 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1132,8 +1132,7 @@ virNetDevGetVirtualFunctions(const char *pfname, if (VIR_ALLOC_N(*vfname, *n_vfname) < 0) goto cleanup; - for (i = 0; i < *n_vfname; i++) - { + for (i = 0; i < *n_vfname; i++) { if (virPCIGetAddrString((*virt_fns)[i]->domain, (*virt_fns)[i]->bus, (*virt_fns)[i]->slot, diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 50aabc5..c83341c 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2010-2013 Red Hat, Inc. + * Copyright (C) 2010-2014 Red Hat, Inc. * Copyright (C) 2010-2012 IBM Corporation * * This library is free software; you can redistribute it and/or @@ -583,8 +583,7 @@ virNetDevMacVLanVPortProfileCallback(struct nlmsghdr *hdr, VIR_DEBUG("IFLA_VF_MAC = %2x:%2x:%2x:%2x:%2x:%2x", m[0], m[1], m[2], m[3], m[4], m[5]); - if (virMacAddrCmpRaw(&calld->macaddress, mac->mac)) - { + if (virMacAddrCmpRaw(&calld->macaddress, mac->mac)) { /* Repeat the same check for a broadcast mac */ size_t i; diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index 668a7df..de2d447 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -195,8 +195,7 @@ virTypedParameterAssign(virTypedParameterPtr param, const char *name, goto cleanup; } param->type = type; - switch (type) - { + switch (type) { case VIR_TYPED_PARAM_INT: param->value.i = va_arg(ap, int); break; diff --git a/src/util/virutil.c b/src/util/virutil.c index 2edbec5..04113bb 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1153,8 +1153,7 @@ virGetWin32DirectoryRoot(char **path) *path = NULL; - if (GetWindowsDirectory(windowsdir, ARRAY_CARDINALITY(windowsdir))) - { + if (GetWindowsDirectory(windowsdir, ARRAY_CARDINALITY(windowsdir))) { const char *tmp; /* Usually X:\Windows, but in terminal server environments * might be an UNC path, AFAIK. @@ -1499,8 +1498,7 @@ void virFileWaitForDevices(void) * If this fails for any reason, we still have the backup of polling for * 5 seconds for device nodes. */ - if (virRun(settleprog, &exitstatus) < 0) - {} + ignore_value(virRun(settleprog, &exitstatus)); } #else void virFileWaitForDevices(void) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index eecfff6..b9858ee 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3576,8 +3576,8 @@ vboxDumpNetwork(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine, PR MACAddress[8], MACAddress[9], MACAddress[10], MACAddress[11]); /* XXX some real error handling here some day ... */ - if (virMacAddrParse(macaddr, &def->nets[netAdpIncCnt]->mac) < 0) - {} + ignore_value(virMacAddrParse(macaddr, + &def->nets[netAdpIncCnt]->mac)); netAdpIncCnt++; diff --git a/tests/seclabeltest.c b/tests/seclabeltest.c index cd34b6b..51765c9 100644 --- a/tests/seclabeltest.c +++ b/tests/seclabeltest.c @@ -24,16 +24,14 @@ main(int argc ATTRIBUTE_UNUSED, char **argv ATTRIBUTE_UNUSED) } model = virSecurityManagerGetModel(mgr); - if (!model) - { + if (!model) { fprintf(stderr, "Failed to copy secModel model: %s", strerror(errno)); return EXIT_FAILURE; } doi = virSecurityManagerGetDOI(mgr); - if (!doi) - { + if (!doi) { fprintf(stderr, "Failed to copy secModel DOI: %s", strerror(errno)); return EXIT_FAILURE; -- 1.9.3
On 09/04/14 01:17, Eric Blake wrote:
Our style overwhelmingly uses hanging braces (the open brace hangs at the end of the compound condition, rather than on its own line), with the primary exception of the top level function body. Fix the few remaining outliers, before adding a syntax check in a later patch.
* src/interface/interface_backend_netcf.c (netcfStateReload) (netcfInterfaceClose, netcf_to_vir_err): Correct use of { in compound statement. * src/conf/domain_conf.c (virDomainHostdevDefFormatSubsys) (virDomainHostdevDefFormatCaps): Likewise. * src/network/bridge_driver.c (networkAllocateActualDevice): Likewise. * src/util/virfile.c (virBuildPathInternal): Likewise. * src/util/virnetdev.c (virNetDevGetVirtualFunctions): Likewise. * src/util/virnetdevmacvlan.c (virNetDevMacVLanVPortProfileCallback): Likewise. * src/util/virtypedparam.c (virTypedParameterAssign): Likewise. * src/util/virutil.c (virGetWin32DirectoryRoot) (virFileWaitForDevices): Likewise. * src/vbox/vbox_common.c (vboxDumpNetwork): Likewise. * tests/seclabeltest.c (main): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.c | 6 ++---- src/interface/interface_backend_netcf.c | 9 +++------ src/network/bridge_driver.c | 3 +-- src/util/virfile.c | 3 +-- src/util/virnetdev.c | 3 +-- src/util/virnetdevmacvlan.c | 5 ++--- src/util/virtypedparam.c | 3 +-- src/util/virutil.c | 6 ++---- src/vbox/vbox_common.c | 4 ++-- tests/seclabeltest.c | 6 ++---- 10 files changed, 17 insertions(+), 31 deletions(-)
ACK, Peter
Now that hanging brace offenders have been fixed, we can automate the check, and document our style. Done as a separate commit from code changes, to make it easier to just backport code changes, if that is ever needed. * cfg.mk (sc_curly_braces_style): Catch hanging braces. * docs/hacking.html.in: Document it. * HACKING: Regenerate. Signed-off-by: Eric Blake <eblake@redhat.com> --- HACKING | 27 +++++++++++++++++++++++++++ cfg.mk | 18 ++++++++++++------ docs/hacking.html.in | 31 +++++++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 6 deletions(-) diff --git a/HACKING b/HACKING index 88a4286..add0841 100644 --- a/HACKING +++ b/HACKING @@ -461,6 +461,33 @@ But if negating a complex condition is too ugly, then at least add braces: x = y; } +Use hanging braces for compound statements: the opening brace of a compound +statement should be on the same line as the condition being tested. Only +top-level function bodies, nested scopes, and compound structure declarations +should ever have { on a line by itself. + + void + foo(int a, int b) + { // correct - function body + int 2d[][] = { + { // correct - complex initialization + 1, 2, + }, + }; + if (a) + { // BAD: compound brace on its own line + do_stuff(); + } + { // correct - nested scope + int tmp; + if (a < b) { // correct - hanging brace + tmp = b; + b = a; + a = tmp; + } + } + } + Preprocessor ============ diff --git a/cfg.mk b/cfg.mk index 122cf58..77f1868 100644 --- a/cfg.mk +++ b/cfg.mk @@ -918,12 +918,18 @@ sc_require_if_else_matching_braces: $(_sc_search_regexp) sc_curly_braces_style: - @files=$$($(VC_LIST_EXCEPT) | grep '\.[ch]$$'); \ - $(GREP) -nHP \ -'^\s*(?!([a-zA-Z_]*for_?each[a-zA-Z_]*) ?\()([_a-zA-Z0-9]+( [_a-zA-Z0-9]+)* ?\()?(\*?[_a-zA-Z0-9]+(,? \*?[_a-zA-Z0-9\[\]]+)+|void)\) ?\{' \ - $$files && { echo '$(ME): Non-K&R style used for curly' \ - 'braces around function body, see' \ - 'HACKING' 1>&2; exit 1; } || : + @files=$$($(VC_LIST_EXCEPT) | grep '\.[ch]$$'); \ + if $(GREP) -nHP \ +'^\s*(?!([a-zA-Z_]*for_?each[a-zA-Z_]*) ?\()([_a-zA-Z0-9]+( [_a-zA-Z0-9]+)* ?\()?(\*?[_a-zA-Z0-9]+(,? \*?[_a-zA-Z0-9\[\]]+)+|void)\) ?\{' \ + $$files; then \ + echo '$(ME): Non-K&R style used for curly braces around' \ + 'function body, see HACKING' 1>&2; exit 1; \ + fi; \ + if $(GREP) -A1 -En ' ((if|for|while|switch) \(|(else|do)\b)[^{]*$$'\ + $$files | grep '^[^ ]*- *{'; then \ + echo '$(ME): Use hanging braces for compound statements,' \ + 'see HACKING' 1>&2; exit 1; \ + fi sc_prohibit_windows_special_chars_in_filename: @files=$$($(VC_LIST_EXCEPT) | grep '[:*?"<>|]'); \ diff --git a/docs/hacking.html.in b/docs/hacking.html.in index bc76542..8f2b9d6 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -593,6 +593,37 @@ } </pre> + <p>Use hanging braces for compound statements: the opening brace + of a compound statement should be on the same line as the + condition being tested. Only top-level function bodies, nested + scopes, and compound structure declarations should ever have { + on a line by itself. + </p> + +<pre> + void + foo(int a, int b) + { // correct - function body + int 2d[][] = { + { // correct - complex initialization + 1, 2, + }, + }; + if (a) + { // BAD: compound brace on its own line + do_stuff(); + } + { // correct - nested scope + int tmp; + if (a < b) { // correct - hanging brace + tmp = b; + b = a; + a = tmp; + } + } + } +</pre> + <h2><a name="preprocessor">Preprocessor</a></h2> <p>Macros defined with an ALL_CAPS name should generally be -- 1.9.3
On 09/04/14 01:17, Eric Blake wrote:
Now that hanging brace offenders have been fixed, we can automate the check, and document our style. Done as a separate commit from code changes, to make it easier to just backport code changes, if that is ever needed.
* cfg.mk (sc_curly_braces_style): Catch hanging braces. * docs/hacking.html.in: Document it. * HACKING: Regenerate.
Signed-off-by: Eric Blake <eblake@redhat.com> --- HACKING | 27 +++++++++++++++++++++++++++ cfg.mk | 18 ++++++++++++------ docs/hacking.html.in | 31 +++++++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 6 deletions(-)
diff --git a/HACKING b/HACKING index 88a4286..add0841 100644 --- a/HACKING +++ b/HACKING
sc_curly_braces_style: - @files=$$($(VC_LIST_EXCEPT) | grep '\.[ch]$$'); \ - $(GREP) -nHP \ -'^\s*(?!([a-zA-Z_]*for_?each[a-zA-Z_]*) ?\()([_a-zA-Z0-9]+( [_a-zA-Z0-9]+)* ?\()?(\*?[_a-zA-Z0-9]+(,? \*?[_a-zA-Z0-9\[\]]+)+|void)\) ?\{' \ - $$files && { echo '$(ME): Non-K&R style used for curly' \ - 'braces around function body, see' \ - 'HACKING' 1>&2; exit 1; } || : + @files=$$($(VC_LIST_EXCEPT) | grep '\.[ch]$$'); \ + if $(GREP) -nHP \ +'^\s*(?!([a-zA-Z_]*for_?each[a-zA-Z_]*) ?\()([_a-zA-Z0-9]+( [_a-zA-Z0-9]+)* ?\()?(\*?[_a-zA-Z0-9]+(,? \*?[_a-zA-Z0-9\[\]]+)+|void)\) ?\{' \ + $$files; then \ + echo '$(ME): Non-K&R style used for curly braces around' \ + 'function body, see HACKING' 1>&2; exit 1; \ + fi; \ + if $(GREP) -A1 -En ' ((if|for|while|switch) \(|(else|do)\b)[^{]*$$'\ + $$files | grep '^[^ ]*- *{'; then \ + echo '$(ME): Use hanging braces for compound statements,' \ + 'see HACKING' 1>&2; exit 1; \ + fi
sc_prohibit_windows_special_chars_in_filename: @files=$$($(VC_LIST_EXCEPT) | grep '[:*?"<>|]'); \
My brain isn't that good in parsing regexes, but they look good to me and the test passed. ACK. Peter
On 09/04/2014 03:39 AM, Peter Krempa wrote:
On 09/04/14 01:17, Eric Blake wrote:
Now that hanging brace offenders have been fixed, we can automate the check, and document our style. Done as a separate commit from code changes, to make it easier to just backport code changes, if that is ever needed.
sc_curly_braces_style: - @files=$$($(VC_LIST_EXCEPT) | grep '\.[ch]$$'); \ - $(GREP) -nHP \ -'^\s*(?!([a-zA-Z_]*for_?each[a-zA-Z_]*) ?\()([_a-zA-Z0-9]+( [_a-zA-Z0-9]+)* ?\()?(\*?[_a-zA-Z0-9]+(,? \*?[_a-zA-Z0-9\[\]]+)+|void)\) ?\{' \ - $$files && { echo '$(ME): Non-K&R style used for curly' \ - 'braces around function body, see' \ - 'HACKING' 1>&2; exit 1; } || : + @files=$$($(VC_LIST_EXCEPT) | grep '\.[ch]$$'); \ + if $(GREP) -nHP \ +'^\s*(?!([a-zA-Z_]*for_?each[a-zA-Z_]*) ?\()([_a-zA-Z0-9]+( [_a-zA-Z0-9]+)* ?\()?(\*?[_a-zA-Z0-9]+(,? \*?[_a-zA-Z0-9\[\]]+)+|void)\) ?\{' \ + $$files; then \ + echo '$(ME): Non-K&R style used for curly braces around' \ + 'function body, see HACKING' 1>&2; exit 1; \ + fi; \
This half was just whitespace changes and reformatting into an explicit shell if instead of implicit &&...
+ if $(GREP) -A1 -En ' ((if|for|while|switch) \(|(else|do)\b)[^{]*$$'\ + $$files | grep '^[^ ]*- *{'; then \
Oops, I need to use $(GREP) here, for consistency.
+ echo '$(ME): Use hanging braces for compound statements,' \ + 'see HACKING' 1>&2; exit 1; \ + fi
...while this added if is the real addition.
sc_prohibit_windows_special_chars_in_filename: @files=$$($(VC_LIST_EXCEPT) | grep '[:*?"<>|]'); \
My brain isn't that good in parsing regexes, but they look good to me and the test passed.
The first grep half is basically saying: find all compound commands, then show that line and the subsequent line, as in: -- src/qemu/qemu_process.c:4997: if (qemuDomainObjBeginJob(driver, dom, src/qemu/qemu_process.c-4998- QEMU_JOB_DESTROY) < 0) The second grep then focuses on the second line (those with filename:-line-) - if any of them is a lone {, then the first line probably needs tweaking to use a hanging brace.
ACK.
Pushed with that minor tweak. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Peter Krempa