On Mon, Apr 12, 2021 at 13:46:35 +0200, Michal Privoznik wrote: > Historically, we declared pointer type to our types: > > typedef struct _virXXX virXXX; > typedef virXXX *virXXXPtr; > > But usefulness of such declaration is questionable, at best. > Unfortunately, we can't drop every such declaration - we have to > carry some over, because they are part of public API (e.g. > virDomainPtr). But for internal types - we can do drop them and > use what every other C project uses 'virXXX *'. > > This change was generated by a very ugly shell script that > generated sed script which was then called over each file in the > repository. For the shell script refer to the cover letter: > > https://listman.redhat.com/archives/libvir-list/2021-March/msg00537.html > > Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> > --- > > This is just a resend of an updated patch I've sent earlier: > > https://listman.redhat.com/archives/libvir-list/2021-March/msg00543.html > https://listman.redhat.com/archives/libvir-list/2021-April/msg00011.html > > For full patch refer to my gitlab branch: > > https://gitlab.com/MichalPrivoznik/libvirt/-/commit/2b7ba77c626fdd5545736cea43d59cb7200ea0a4 Preferrably post a 'git fetch' commandline. The web-UI is totally useless for reviewing massive changes. Firstly it breaks freebsd build: In file included from ../dist-unpack/libvirt-7.3.0/src/bhyve/bhyve_firmware.c:29: ../dist-unpack/libvirt-7.3.0/src/bhyve/bhyve_firmware.h:28:25: error: unknown type name 'bhyveConnPtr'; did you mean 'bhyveConn'? bhyveFirmwareFillDomain(bhyveConnPtr driver, ^~~~~~~~~~~~ bhyveConn ../dist-unpack/libvirt-7.3.0/src/bhyve/bhyve_utils.h:70:27: note: 'bhyveConn' declared here typedef struct _bhyveConn bhyveConn; ^ In file included from ../dist-unpack/libvirt-7.3.0/src/bhyve/bhyve_firmware.c:29: ../dist-unpack/libvirt-7.3.0/src/bhyve/bhyve_firmware.h:29:25: error: unknown type name 'virDomainDefPtr' virDomainDefPtr def, ^ ../dist-unpack/libvirt-7.3.0/src/bhyve/bhyve_firmware.c:39:25: error: unknown type name 'bhyveConnPtr'; did you mean 'bhyveConn'? bhyveFirmwareFillDomain(bhyveConnPtr driver, ^~~~~~~~~~~~ bhyveConn ../dist-unpack/libvirt-7.3.0/src/bhyve/bhyve_utils.h:70:27: note: 'bhyveConn' declared here typedef struct _bhyveConn bhyveConn; ^ ../dist-unpack/libvirt-7.3.0/src/bhyve/bhyve_firmware.c:40:25: error: unknown type name 'virDomainDefPtr' virDomainDefPtr def, ^ ../dist-unpack/libvirt-7.3.0/src/bhyve/bhyve_firmware.c:44:67: error: passing 'bhyveConn' (aka 'struct _bhyveConn') to parameter of incompatible type 'struct _bhyveConn *'; take the address with & g_autoptr(virBhyveDriverConfig) cfg = virBhyveDriverGetConfig(driver); ^~~~~~ & ../dist-unpack/libvirt-7.3.0/src/bhyve/bhyve_conf.h:27:74: note: passing argument to parameter 'driver' here struct _virBhyveDriverConfig *virBhyveDriverGetConfig(struct _bhyveConn *driver); ^ 5 errors generated. [302/1136] Compiling C object src/bhyve/libvirt_driver_bhyve_impl.a.p/bhyve_device.c.o See https://gitlab.com/pipo.sk/libvirt/-/jobs/1171847151 I've found few nits: diff --git a/src/bhyve/bhyve_capabilities.h b/src/bhyve/bhyve_capabilities.h index 1b25c000b5..62d07c1c68 100644 --- a/src/bhyve/bhyve_capabilities.h +++ b/src/bhyve/bhyve_capabilities.h @@ -26,11 +26,11 @@ #include "bhyve_utils.h" -virCapsPtr virBhyveCapsBuild(void); -int virBhyveDomainCapsFill(virDomainCapsPtr caps, +virCaps *virBhyveCapsBuild(void); +int virBhyveDomainCapsFill(virDomainCaps *caps, unsigned int bhyvecaps, - virDomainCapsStringValuesPtr firmwares); -virDomainCapsPtr virBhyveDomainCapsBuild(bhyveConnPtr, + virDomainCapsStringValues *firmwares); +virDomainCaps *virBhyveDomainCapsBuild(struct _bhyveConn *, const char *emulatorbin, const char *machine, virArch arch, Function header formatting is broken by this patch if the fucntion header is formatted using the old style with type on same line as function name and the type was a "Ptr". @@ -20949,10 +20949,10 @@ virDomainDefClockParse(virDomainDefPtr def, goto error; if (n) - def->clock.timers = g_new0(virDomainTimerDefPtr, n); + def->clock.timers = g_new0(virDomainTimerDef *, n); for (i = 0; i < n; i++) { - virDomainTimerDefPtr timer = virDomainTimerDefParseXML(nodes[i], + virDomainTimerDef *timer = virDomainTimerDefParseXML(nodes[i], ctxt); if (!timer) goto error; Similarly to the above, declaration of variable and iniit by a function on the same line with broken up argument list. This is by far the most common problem. @@ -2429,12 +2422,12 @@ struct _virDomainRNGDef { union { char *file; /* file name for 'random' source */ - virDomainChrSourceDefPtr chardev; /* a char backend for + virDomainChrSourceDef *chardev; /* a char backend for the EGD source */ } source; Yet another similarity but this time with a trailing comment. @@ -2511,7 +2504,7 @@ struct _virDomainResourceDef { }; struct _virDomainHugePage { - virBitmapPtr nodemask; /* guest's NUMA node mask */ + virBitmap *nodemask; /* guest's NUMA node mask */ unsigned long long size; /* hugepage size in KiB */ }; And also "block aligned" comments. But those should be nuked from orbit. @@ -4213,15 +4213,15 @@ virDomainDeviceGetInfo(virDomainDeviceDefPtr device) /** * virDomainDeviceSetData - * @device: virDomainDeviceDefPtr with ->type filled in - * @devicedata: *DefPtr data for a device. Ex: virDomainDiskDefPtr + * @device: virDomainDeviceDef *with ->type filled in In comments the '*' should be followed with a space. + * @devicedata: *DefPtr data for a device. Ex: virDomainDiskDef * * * Set the data.X variable for the device->type value. Basically * a mapping of virDomainDeviceType to the associated name in * the virDomainDeviceDef union */ void -virDomainDeviceSetData(virDomainDeviceDefPtr device, +virDomainDeviceSetData(virDomainDeviceDef *device, void *devicedata) { switch ((virDomainDeviceType) device->type) { @@ -19568,7 +19568,7 @@ virDomainDefParseBootInitOptions(virDomainDefPtr def, if ((n = virXPathNodeSet("./os/initenv", ctxt, &nodes)) < 0) return -1; - def->os.initenv = g_new0(virDomainOSEnvPtr, n+1); + def->os.initenv = g_new0(virDomainOSEnv *, n + 1); for (i = 0; i < n; i++) { if (!(name = virXMLPropString(nodes[i], "name"))) { virReportError(VIR_ERR_XML_ERROR, "%s", Unrelated change, but I'll squint to overlook it. Most of the nits above have more than one occurence. I've verified that the public types are not impacted and the commit is fairly mechanical. Since I don't really want to look at this any more: If you fix the bulild and function header formatting nits: Reviewed-by: Peter Krempa <pkrempa@xxxxxxxxxx> P.S: Notable eww: (not a problem with the patch though) @@ -17540,9 +17540,9 @@ virDomainChrFind(virDomainDefPtr def, /* Return the address within vmdef to be modified when working with a * chrdefptr of the given type. */ static int G_GNUC_WARN_UNUSED_RESULT -virDomainChrGetDomainPtrsInternal(virDomainDefPtr vmdef, +virDomainChrGetDomainPtrsInternal(virDomainDef *vmdef, virDomainChrDeviceType type, - virDomainChrDefPtr ***arrPtr, + virDomainChrDef ****arrPtr, size_t **cntPtr) { switch (type) {
