Re: [PATCH 6/6] lib: Drop internal virXXXPtr typedefs
On 3/12/21 8:59 AM, Michal Privoznik wrote: On 3/11/21 7:47 PM, Daniel P. Berrangé wrote: On Thu, Mar 11, 2021 at 06:54:20PM +0100, Michal Privoznik wrote: Historically, we declared pointer type to our types: typedef struct _virXXX virXXX; typedef virXXX *virXXXPtr; Also dropping usage of the "Ptr" should be separate from dropping the typedefs themselves as I think that'll make conflicts easier to deal with. Yeah, this makes sense. I'll split it. On a second thought, does it make sense? I don't think that somebody would want to backport just the rewrite without dropping the typedefs, or backport just the typedef drop without the rewrite. It's all or nothing. Michal
Re: [PATCH 6/6] lib: Drop internal virXXXPtr typedefs
On Thu, Mar 11, 2021 at 18:47:37 +, Daniel P. Berrangé wrote: > On Thu, Mar 11, 2021 at 06:54:20PM +0100, 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: > > > > $URL > > > > Signed-off-by: Michal Privoznik > > --- > > docs/advanced-tests.rst |2 +- > > docs/api_extension.html.in|2 +- > > docs/coding-style.rst |2 +- > > snip > > > tools/virt-login-shell-helper.c |4 +- > > tools/vsh-table.c | 36 +- > > tools/vsh-table.h | 12 +- > > tools/vsh.c |4 +- > > 732 files changed, 29237 insertions(+), 30131 deletions(-) > > Converting every single file at the same time in one commit is > guaranting backporting conflict hell. > > eg if I'm backporting a patch from src/qemu, it is much > saner if I can cherry-pick the "Ptr" conversion from src/qemu > and only deal with conflicts in that, not the entire soure > tree. I don't think it's a good idea to try to backport anything from this patch. There's no need to do so, upstream will strictly use types without Ptr and all of them already exist upstream. Backporting any future changes to the code which still has *Ptr should not cause any issues. Except for moving big parts of code (but such change would likely conflict for other reasons too) and trivial context when the patch touches lines close to virSomethingPtr. Jirka
Re: [PATCH 6/6] lib: Drop internal virXXXPtr typedefs
On 3/11/21 7:47 PM, Daniel P. Berrangé wrote: On Thu, Mar 11, 2021 at 06:54:20PM +0100, 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: $URL Signed-off-by: Michal Privoznik --- docs/advanced-tests.rst |2 +- docs/api_extension.html.in|2 +- docs/coding-style.rst |2 +- snip tools/virt-login-shell-helper.c |4 +- tools/vsh-table.c | 36 +- tools/vsh-table.h | 12 +- tools/vsh.c |4 +- 732 files changed, 29237 insertions(+), 30131 deletions(-) Converting every single file at the same time in one commit is guaranting backporting conflict hell. eg if I'm backporting a patch from src/qemu, it is much saner if I can cherry-pick the "Ptr" conversion from src/qemu and only deal with conflicts in that, not the entire soure tree. So if we're going to do this conversion, IMHO the actual commits need to be way more granular. At the very least I think this needs to be split per driver, with tests/ associated with their driver. The util/ directory needs to be split up and likewise the conf/ directory per object type I think. I though about it this way too, but then I realized, that the only type of merge conflicts we will get while backporting is context. I mean, after this commit, every subsequent commit will use 'virXXX *' which is perfectly okay even now. And thus what we might get the only type of conflict - cherry picked commit already expects 'virXXX *' (e.g. it renames a variable), but the downstream has 'virXXXPtr'. But this kind of conflict is trivial to resolve. Another reason to put it into one huge patch was to not bother with dependencies. It's not uncommon that our commits touch different locations at the same time, they are not strictly aiming on one submodule (e.g. src/conf and src/qemu at the same time, or src/util and tools/virsh). The point is, making the granularity satisfactory fine grained is hard. Also dropping usage of the "Ptr" should be separate from dropping the typedefs themselves as I think that'll make conflicts easier to deal with. Yeah, this makes sense. I'll split it. Michal
Re: [PATCH 6/6] lib: Drop internal virXXXPtr typedefs
On Thu, Mar 11, 2021 at 06:54:20PM +0100, 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: > > $URL > > Signed-off-by: Michal Privoznik > --- > docs/advanced-tests.rst |2 +- > docs/api_extension.html.in|2 +- > docs/coding-style.rst |2 +- snip > tools/virt-login-shell-helper.c |4 +- > tools/vsh-table.c | 36 +- > tools/vsh-table.h | 12 +- > tools/vsh.c |4 +- > 732 files changed, 29237 insertions(+), 30131 deletions(-) Converting every single file at the same time in one commit is guaranting backporting conflict hell. eg if I'm backporting a patch from src/qemu, it is much saner if I can cherry-pick the "Ptr" conversion from src/qemu and only deal with conflicts in that, not the entire soure tree. So if we're going to do this conversion, IMHO the actual commits need to be way more granular. At the very least I think this needs to be split per driver, with tests/ associated with their driver. The util/ directory needs to be split up and likewise the conf/ directory per object type I think. Also dropping usage of the "Ptr" should be separate from dropping the typedefs themselves as I think that'll make conflicts easier to deal with. 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 :|
[PATCH 6/6] lib: Drop internal virXXXPtr typedefs
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: $URL Signed-off-by: Michal Privoznik --- docs/advanced-tests.rst |2 +- docs/api_extension.html.in|2 +- docs/coding-style.rst |2 +- docs/internals/command.html.in| 12 +- docs/internals/rpc.html.in| 32 +- scripts/esx_vi_generator.py |6 +- scripts/hyperv_wmi_generator.py |6 +- src/access/viraccessdriver.h | 52 +- src/access/viraccessdrivernop.c | 46 +- src/access/viraccessdriverpolkit.c| 52 +- src/access/viraccessdriverstack.c | 82 +- src/access/viraccessdriverstack.h |4 +- src/access/viraccessmanager.c | 78 +- src/access/viraccessmanager.h | 57 +- src/admin/admin_remote.c | 56 +- src/admin/admin_server.c | 34 +- src/admin/admin_server.h | 26 +- src/admin/admin_server_dispatch.c | 129 +- src/admin/admin_server_dispatch.h |8 +- src/admin/libvirt-admin.c |8 +- src/bhyve/bhyve_capabilities.c| 26 +- src/bhyve/bhyve_capabilities.h|8 +- src/bhyve/bhyve_command.c | 124 +- src/bhyve/bhyve_command.h | 14 +- src/bhyve/bhyve_conf.c| 18 +- src/bhyve/bhyve_conf.h|9 +- src/bhyve/bhyve_device.c | 30 +- src/bhyve/bhyve_device.h |6 +- src/bhyve/bhyve_domain.c | 36 +- src/bhyve/bhyve_domain.h |7 +- src/bhyve/bhyve_driver.c | 162 +- src/bhyve/bhyve_driver.h |6 +- src/bhyve/bhyve_monitor.c | 34 +- src/bhyve/bhyve_monitor.h |7 +- src/bhyve/bhyve_parse_command.c | 40 +- src/bhyve/bhyve_parse_command.h |4 +- src/bhyve/bhyve_process.c | 52 +- src/bhyve/bhyve_process.h | 16 +- src/bhyve/bhyve_utils.h | 25 +- src/conf/backup_conf.c| 54 +- src/conf/backup_conf.h| 24 +- src/conf/capabilities.c | 240 +- src/conf/capabilities.h | 116 +- src/conf/checkpoint_conf.c| 80 +- src/conf/checkpoint_conf.h| 23 +- src/conf/cpu_conf.c | 78 +- src/conf/cpu_conf.h | 71 +- src/conf/device_conf.c| 36 +- src/conf/device_conf.h| 42 +- src/conf/domain_addr.c| 316 +-- src/conf/domain_addr.h| 128 +- src/conf/domain_audit.c | 96 +- src/conf/domain_audit.h | 76 +- src/conf/domain_capabilities.c| 70 +- src/conf/domain_capabilities.h| 39 +- src/conf/domain_conf.c| 2509 - src/conf/domain_conf.h| 967 --- src/conf/domain_event.c | 552 ++-- src/conf/domain_event.h | 178 +- src/conf/domain_nwfilter.c| 18 +- src/conf/domain_nwfilter.h|6 +- src/conf/domain_validate.c| 64 +- src/conf/domain_validate.h| 10 +- src/conf/interface_conf.c | 76 +- src/conf/interface_conf.h | 18 +- src/conf/moment_conf.c|8 +- src/conf/moment_conf.h|8 +- src/conf/netdev_bandwidth_conf.c | 12 +- src/conf/netdev_bandwidth_conf.h |6 +- src/conf/netdev_vlan_conf.c |4 +- src/conf/netdev_vlan_conf.h |4 +- src/conf/netdev_vport_profile_conf.c |6 +- src/conf/netdev_vport_profile_conf.h |4 +- src/conf/network_conf.c | 198 +- src/conf/network_conf.h | 105 +- src/conf/network_event.c | 26 +-
