Re: [libvirt] [PATCH 06/11] util: use glib base64 encoding/decoding APIs
On Tue, 2019-10-01 at 10:12 -0400, Laine Stump wrote: > On 9/30/19 1:35 PM, Andrea Bolognani wrote: > > Writing a check that compares the situation before a commit and > > after it is not as easy as a point-in-time check. > > Not all that bad though - just examine the lines that start with + Fair enough. That wouldn't work outside of git, but I guess it would be an acceptable compromise as all our CI jobs are executed from inside a git checkout anyway. > > Instead of spending > > a non-trival amount of time implementing something like that, I'd > > rather spend my time dealing with the fallout of a one-time > > conversion. > > Without seeing concrete examples of what actually is "dealing with the > fallout" in both cases, I don't want to speculate too much on which > would cause more difficulty. I would say that even if we do the > conversion all at once, it should be in multiple patches so that if > there is some regression caused by the conversion, a git bisect will > lead to a multi-hundred line commit instead of a multi-thousand line commit. Of course when I talk about "big-bang conversion" I mean done in a single series, not a single patch! -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] util: use glib base64 encoding/decoding APIs
On 9/30/19 1:35 PM, Andrea Bolognani wrote: On Mon, 2019-09-30 at 13:13 -0400, Laine Stump wrote: On 9/30/19 10:05 AM, Andrea Bolognani wrote: I see your point about backports being more painful when you have a bunch of unrelated changes mixed in, but I would still prefer if we converted everything at once and at the same time introduced a suitable syntax-check rule preventing more instances of whatever function we just removed all callers of from creeping back in, or actually just dropping the function altogether. Don't forget that make syntax-check doesn't work properly for many downstream maintenance branches that would be backported to (it has to be disabled due to copyright date checks failing, or something like that). That's a problem for downstream to solve. By the same token, all the existing syntax-check rules are pointless because they can't be guaranteed to hold for downstream branches. Yeah, I'm just bitter that make syntax-check doesn't work downstream, and can't let a mention of the two in the same context go by without making a whining comment. In order to allay Andrea's fears of new usage of VIR_AUTO* that just draws out the conversion, maybe we could (temporarily, until the conversion is complete) put a commit hook in place to disallow new use of VIR_AUTO ? Or just, you know, pay attention in reviews (but of course part of the point of all of this is to eliminate the potential for human error, by depending less on humans paying attention, so... :-P) Writing a check that compares the situation before a commit and after it is not as easy as a point-in-time check. Not all that bad though - just examine the lines that start with + Instead of spending a non-trival amount of time implementing something like that, I'd rather spend my time dealing with the fallout of a one-time conversion. Without seeing concrete examples of what actually is "dealing with the fallout" in both cases, I don't want to speculate too much on which would cause more difficulty. I would say that even if we do the conversion all at once, it should be in multiple patches so that if there is some regression caused by the conversion, a git bisect will lead to a multi-hundred line commit instead of a multi-thousand line commit. (BTW, I'm not firmly in *either* camp, although I may lean a bit more towards a gradual change (but with a *very* steep slope to minimize the period of confusion) That's just a big-bang conversion with extra steps! Well, after all *anything* we each do before meeting a sad and lonely demise is really just a part of Heat Death of the Universe with extra steps. You need to subdivide *somewhere*. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] util: use glib base64 encoding/decoding APIs
On Mon, 2019-09-30 at 13:13 -0400, Laine Stump wrote: > On 9/30/19 10:05 AM, Andrea Bolognani wrote: > > I see your point about backports being more painful when you have > > a bunch of unrelated changes mixed in, but I would still prefer if > > we converted everything at once and at the same time introduced a > > suitable syntax-check rule preventing more instances of whatever > > function we just removed all callers of from creeping back in, or > > actually just dropping the function altogether. > > Don't forget that make syntax-check doesn't work properly for many > downstream maintenance branches that would be backported to (it has to > be disabled due to copyright date checks failing, or something like > that). That's a problem for downstream to solve. By the same token, all the existing syntax-check rules are pointless because they can't be guaranteed to hold for downstream branches. > In order to allay Andrea's fears of new usage of VIR_AUTO* that just > draws out the conversion, maybe we could (temporarily, until the > conversion is complete) put a commit hook in place to disallow new use > of VIR_AUTO ? Or just, you know, pay attention in reviews (but of course > part of the point of all of this is to eliminate the potential for human > error, by depending less on humans paying attention, so... :-P) Writing a check that compares the situation before a commit and after it is not as easy as a point-in-time check. Instead of spending a non-trival amount of time implementing something like that, I'd rather spend my time dealing with the fallout of a one-time conversion. > (BTW, I'm not firmly in *either* camp, although I may lean a bit more > towards a gradual change (but with a *very* steep slope to minimize the > period of confusion) That's just a big-bang conversion with extra steps! -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] util: use glib base64 encoding/decoding APIs
On 9/30/19 10:05 AM, Andrea Bolognani wrote: On Mon, 2019-09-30 at 13:41 +0100, Daniel P. Berrangé wrote: On Mon, Sep 30, 2019 at 02:18:17PM +0200, Andrea Bolognani wrote: On Mon, 2019-09-30 at 13:56 +0200, Pavel Hrdina wrote: Agreed, for now let's keep all the wrappers but eventually we can remove them to make the code cleaner. Note that we should be able to use VIR_AUTOPTR() instead of g_autoptr() even for objects, by simply registering the same GLib-provided free function with our own macros. That way we could keep using VIR_AUTO* everywhere and then, when we're ready, mechanically switch everything to g_auto* in one fell swoop, without having any point in time where the two styles are coexisting. That creates an even bigger conversion later. Such big conversions cause more pain for backports, than doing an incremental conversion at appropriate times. Converting everything to g_autofree right now doesn't give style consistency as we'd still be matching VIR_ALLOC + g_autofree, so I don't see a benefit to a big conversion in 1 step. Incrementally converting VIR_ALLOC + VIR_AUTOFREE at the same time, makes more sense stylewise, as then within the scope of a single method we'd be consistent. I see your point about backports being more painful when you have a bunch of unrelated changes mixed in, but I would still prefer if we converted everything at once and at the same time introduced a suitable syntax-check rule preventing more instances of whatever function we just removed all callers of from creeping back in, or actually just dropping the function altogether. Don't forget that make syntax-check doesn't work properly for many downstream maintenance branches that would be backported to (it has to be disabled due to copyright date checks failing, or something like that). Of course that wouldn't be a problem until some future maintenance branch based on a release that uses glib but still has VIR_AUTO* defined. On the other hand, many *current* maintenance branches that must be targeted by backports don't even have the VIR_AUTO* macros (which were introduced in 4.6.0, but *still* aren't used in a lot of the code), so there are going to be backport headaches anyway, and they are going to be of equal pain regardless of whether we do a big bang conversion or gradual conversion. The differences in pain level will be noticed in downstream maintenance branches >= 4.6.0 and <= 5.9.0 (or whatever version). Doing the conversion incrementally will IMHO result in dragging it for much longer, causing more pain in the long run than ripping the bandaid would. In order to allay Andrea's fears of new usage of VIR_AUTO* that just draws out the conversion, maybe we could (temporarily, until the conversion is complete) put a commit hook in place to disallow new use of VIR_AUTO ? Or just, you know, pay attention in reviews (but of course part of the point of all of this is to eliminate the potential for human error, by depending less on humans paying attention, so... :-P) (BTW, I'm not firmly in *either* camp, although I may lean a bit more towards a gradual change (but with a *very* steep slope to minimize the period of confusion) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] util: use glib base64 encoding/decoding APIs
On Mon, 2019-09-30 at 16:34 +0100, Daniel P. Berrangé wrote: > On Mon, Sep 30, 2019 at 05:24:00PM +0200, Peter Krempa wrote: > > On Mon, Sep 30, 2019 at 16:10:11 +0100, Daniel Berrange wrote: > > > It don't be in a half-done state forever. We can let things be converted > > > incrementally over the next 3-6 months. At the end of say 6 months if > > > anything is left we bulk convert it them. That gets the benefits opf > > > incremental work without downside of stuff remaining unconverted forever. > > > > How is this not a big-bang conversion? > > Most of the conversion patches will be small & targetted. Obviously it > assumes that 95% of the stuff gets converted this way during the first > period. So the final conversion at the end is quite small. But there's no GSoC planned for the next six months! On a more serious note, I don't think this expectation is realistic, and at the end of it all the we'll just end up doing pretty much the same big-bang conversion, only with a few months of neither here nor there in between. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] util: use glib base64 encoding/decoding APIs
On Mon, Sep 30, 2019 at 05:17:32PM +0200, Andrea Bolognani wrote: > On Mon, 2019-09-30 at 15:53 +0100, Daniel P. Berrangé wrote: > > On Mon, Sep 30, 2019 at 04:05:57PM +0200, Andrea Bolognani wrote: > > > I see your point about backports being more painful when you have > > > a bunch of unrelated changes mixed in, but I would still prefer if > > > we converted everything at once and at the same time introduced a > > > suitable syntax-check rule preventing more instances of whatever > > > function we just removed all callers of from creeping back in, or > > > actually just dropping the function altogether. > > > > > > Doing the conversion incrementally will IMHO result in dragging it > > > for much longer, causing more pain in the long run than ripping the > > > bandaid would. > > > > There's really not any significant real world pain from mixing the > > two styles. It is visually distasteful but doesn't cause any functional > > problems at runtime, nor complexity for maintainers. A large conversion > > over the whole codebase does cause very significant pain in conflicts > > for anyone cherry picking patches. That is just not a net win overall. > > I'll take visually mixed styles any day over creating patch conflicts > > in backports. > > If we allow both at the same time, then we'll keep using both going > forward because there's no incentive *not* to mix the two styles; > one of the stated advantages of adopting GLib, making the libvirt > code base more approachable to people familiar with QEMU and other > GLib-using projects, will then not be realized. No matter what we do, Realizing the advantage of familiarity with GLib is not going to be something we win overnight. We have far too much existing code for that to happen, so this familiarity is always something that take time for us to achieve. > Both the conversion from one style to the other and, consequently, > addressing any conflict resulting from it, can be tackled in a > purely mechanical fashion: a bit annoying, sure, but hardly worth > keeping the conversion ongoing forever for in my opinion. I never suggested we want a conversion to run forever. It absolutely must be time limited, it just doesn't all have to be done in one giant big bang. Spreading it over several releases, with a clear final deadline strikes a balance between getting it done and mitigating pain for backporting. 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] util: use glib base64 encoding/decoding APIs
On Mon, Sep 30, 2019 at 05:24:00PM +0200, Peter Krempa wrote: > On Mon, Sep 30, 2019 at 16:10:11 +0100, Daniel Berrange wrote: > > On Mon, Sep 30, 2019 at 05:03:47PM +0200, Peter Krempa wrote: > > > On Mon, Sep 30, 2019 at 15:53:35 +0100, Daniel Berrange wrote: > > > > On Mon, Sep 30, 2019 at 04:05:57PM +0200, Andrea Bolognani wrote: > > > > > On Mon, 2019-09-30 at 13:41 +0100, Daniel P. Berrangé wrote: > > > > > > On Mon, Sep 30, 2019 at 02:18:17PM +0200, Andrea Bolognani wrote: > > > > > > > On Mon, 2019-09-30 at 13:56 +0200, Pavel Hrdina wrote: > > [...] > > > > > There's really not any significant real world pain from mixing the > > > > two styles. It is visually distasteful but doesn't cause any functional > > > > problems at runtime, nor complexity for maintainers. A large conversion > > > > over the whole codebase does cause very significant pain in conflicts > > > > for anyone cherry picking patches. That is just not a net win overall. > > > > I'll take visually mixed styles any day over creating patch conflicts > > > > in backports. > > > > > > I don't see how. If the end-goal is to convert everything to the new > > > form you will get into potential pain/conflicts sooner or later anyways. > > > > If we incrementally convert methods, then when backporting a patch > > related to that method, we have good chance of being able to cherry-pick > > the small conversion patch. If we bulk convert entire file at a time, > > across the whole codebase, attempting to cherry-pick the conversion patches > > will have much higher conflict liklihood. > > I doubt that there will be any motivation to start a incremental > coversion. While when adding VIR_AUTOFREE and coverting to it there was > a clear benefit of simplification in doing so, converting from > VIR_AUTOFREE to g_autofree has exactly 0 benefit. The motivation for conversion is something under our direct control as reviewers. eg we could define a coding style rule that any function which makes a call to any glib API (ie a g_XXX prefix) must be converted as a prior step. > > > Or the other option is to leave it as a half-done lingering refactor and > > > that doesn't help either. > > > > It don't be in a half-done state forever. We can let things be converted > > incrementally over the next 3-6 months. At the end of say 6 months if > > anything is left we bulk convert it them. That gets the benefits opf > > incremental work without downside of stuff remaining unconverted forever. > > How is this not a big-bang conversion? Most of the conversion patches will be small & targetted. Obviously it assumes that 95% of the stuff gets converted this way during the first period. So the final conversion at the end is quite small. 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] util: use glib base64 encoding/decoding APIs
On Mon, Sep 30, 2019 at 16:10:11 +0100, Daniel Berrange wrote: > On Mon, Sep 30, 2019 at 05:03:47PM +0200, Peter Krempa wrote: > > On Mon, Sep 30, 2019 at 15:53:35 +0100, Daniel Berrange wrote: > > > On Mon, Sep 30, 2019 at 04:05:57PM +0200, Andrea Bolognani wrote: > > > > On Mon, 2019-09-30 at 13:41 +0100, Daniel P. Berrangé wrote: > > > > > On Mon, Sep 30, 2019 at 02:18:17PM +0200, Andrea Bolognani wrote: > > > > > > On Mon, 2019-09-30 at 13:56 +0200, Pavel Hrdina wrote: [...] > > > There's really not any significant real world pain from mixing the > > > two styles. It is visually distasteful but doesn't cause any functional > > > problems at runtime, nor complexity for maintainers. A large conversion > > > over the whole codebase does cause very significant pain in conflicts > > > for anyone cherry picking patches. That is just not a net win overall. > > > I'll take visually mixed styles any day over creating patch conflicts > > > in backports. > > > > I don't see how. If the end-goal is to convert everything to the new > > form you will get into potential pain/conflicts sooner or later anyways. > > If we incrementally convert methods, then when backporting a patch > related to that method, we have good chance of being able to cherry-pick > the small conversion patch. If we bulk convert entire file at a time, > across the whole codebase, attempting to cherry-pick the conversion patches > will have much higher conflict liklihood. I doubt that there will be any motivation to start a incremental coversion. While when adding VIR_AUTOFREE and coverting to it there was a clear benefit of simplification in doing so, converting from VIR_AUTOFREE to g_autofree has exactly 0 benefit. > > > Or the other option is to leave it as a half-done lingering refactor and > > that doesn't help either. > > It don't be in a half-done state forever. We can let things be converted > incrementally over the next 3-6 months. At the end of say 6 months if > anything is left we bulk convert it them. That gets the benefits opf > incremental work without downside of stuff remaining unconverted forever. How is this not a big-bang conversion? -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] util: use glib base64 encoding/decoding APIs
On Mon, 2019-09-30 at 15:53 +0100, Daniel P. Berrangé wrote: > On Mon, Sep 30, 2019 at 04:05:57PM +0200, Andrea Bolognani wrote: > > I see your point about backports being more painful when you have > > a bunch of unrelated changes mixed in, but I would still prefer if > > we converted everything at once and at the same time introduced a > > suitable syntax-check rule preventing more instances of whatever > > function we just removed all callers of from creeping back in, or > > actually just dropping the function altogether. > > > > Doing the conversion incrementally will IMHO result in dragging it > > for much longer, causing more pain in the long run than ripping the > > bandaid would. > > There's really not any significant real world pain from mixing the > two styles. It is visually distasteful but doesn't cause any functional > problems at runtime, nor complexity for maintainers. A large conversion > over the whole codebase does cause very significant pain in conflicts > for anyone cherry picking patches. That is just not a net win overall. > I'll take visually mixed styles any day over creating patch conflicts > in backports. If we allow both at the same time, then we'll keep using both going forward because there's no incentive *not* to mix the two styles; one of the stated advantages of adopting GLib, making the libvirt code base more approachable to people familiar with QEMU and other GLib-using projects, will then not be realized. Both the conversion from one style to the other and, consequently, addressing any conflict resulting from it, can be tackled in a purely mechanical fashion: a bit annoying, sure, but hardly worth keeping the conversion ongoing forever for in my opinion. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] util: use glib base64 encoding/decoding APIs
On Mon, Sep 30, 2019 at 05:03:47PM +0200, Peter Krempa wrote: > On Mon, Sep 30, 2019 at 15:53:35 +0100, Daniel Berrange wrote: > > On Mon, Sep 30, 2019 at 04:05:57PM +0200, Andrea Bolognani wrote: > > > On Mon, 2019-09-30 at 13:41 +0100, Daniel P. Berrangé wrote: > > > > On Mon, Sep 30, 2019 at 02:18:17PM +0200, Andrea Bolognani wrote: > > > > > On Mon, 2019-09-30 at 13:56 +0200, Pavel Hrdina wrote: > > [...] > > > > > Incrementally converting VIR_ALLOC + VIR_AUTOFREE at the same > > > > time, makes more sense stylewise, as then within the scope of a > > > > single method we'd be consistent. > > > > > > I see your point about backports being more painful when you have > > > a bunch of unrelated changes mixed in, but I would still prefer if > > > we converted everything at once and at the same time introduced a > > > suitable syntax-check rule preventing more instances of whatever > > > function we just removed all callers of from creeping back in, or > > > actually just dropping the function altogether. > > > > > > Doing the conversion incrementally will IMHO result in dragging it > > > for much longer, causing more pain in the long run than ripping the > > > bandaid would. > > > > There's really not any significant real world pain from mixing the > > two styles. It is visually distasteful but doesn't cause any functional > > problems at runtime, nor complexity for maintainers. A large conversion > > over the whole codebase does cause very significant pain in conflicts > > for anyone cherry picking patches. That is just not a net win overall. > > I'll take visually mixed styles any day over creating patch conflicts > > in backports. > > I don't see how. If the end-goal is to convert everything to the new > form you will get into potential pain/conflicts sooner or later anyways. If we incrementally convert methods, then when backporting a patch related to that method, we have good chance of being able to cherry-pick the small conversion patch. If we bulk convert entire file at a time, across the whole codebase, attempting to cherry-pick the conversion patches will have much higher conflict liklihood. > Or the other option is to leave it as a half-done lingering refactor and > that doesn't help either. It don't be in a half-done state forever. We can let things be converted incrementally over the next 3-6 months. At the end of say 6 months if anything is left we bulk convert it them. That gets the benefits opf incremental work without downside of stuff remaining unconverted forever. 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] util: use glib base64 encoding/decoding APIs
On Mon, Sep 30, 2019 at 15:53:35 +0100, Daniel Berrange wrote: > On Mon, Sep 30, 2019 at 04:05:57PM +0200, Andrea Bolognani wrote: > > On Mon, 2019-09-30 at 13:41 +0100, Daniel P. Berrangé wrote: > > > On Mon, Sep 30, 2019 at 02:18:17PM +0200, Andrea Bolognani wrote: > > > > On Mon, 2019-09-30 at 13:56 +0200, Pavel Hrdina wrote: [...] > > > Incrementally converting VIR_ALLOC + VIR_AUTOFREE at the same > > > time, makes more sense stylewise, as then within the scope of a > > > single method we'd be consistent. > > > > I see your point about backports being more painful when you have > > a bunch of unrelated changes mixed in, but I would still prefer if > > we converted everything at once and at the same time introduced a > > suitable syntax-check rule preventing more instances of whatever > > function we just removed all callers of from creeping back in, or > > actually just dropping the function altogether. > > > > Doing the conversion incrementally will IMHO result in dragging it > > for much longer, causing more pain in the long run than ripping the > > bandaid would. > > There's really not any significant real world pain from mixing the > two styles. It is visually distasteful but doesn't cause any functional > problems at runtime, nor complexity for maintainers. A large conversion > over the whole codebase does cause very significant pain in conflicts > for anyone cherry picking patches. That is just not a net win overall. > I'll take visually mixed styles any day over creating patch conflicts > in backports. I don't see how. If the end-goal is to convert everything to the new form you will get into potential pain/conflicts sooner or later anyways. Or the other option is to leave it as a half-done lingering refactor and that doesn't help either. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] util: use glib base64 encoding/decoding APIs
On Mon, Sep 30, 2019 at 04:05:57PM +0200, Andrea Bolognani wrote: > On Mon, 2019-09-30 at 13:41 +0100, Daniel P. Berrangé wrote: > > On Mon, Sep 30, 2019 at 02:18:17PM +0200, Andrea Bolognani wrote: > > > On Mon, 2019-09-30 at 13:56 +0200, Pavel Hrdina wrote: > > > > Agreed, for now let's keep all the wrappers but eventually we can remove > > > > them to make the code cleaner. > > > > > > Note that we should be able to use VIR_AUTOPTR() instead of > > > g_autoptr() even for objects, by simply registering the same > > > GLib-provided free function with our own macros. > > > > > > That way we could keep using VIR_AUTO* everywhere and then, when > > > we're ready, mechanically switch everything to g_auto* in one fell > > > swoop, without having any point in time where the two styles are > > > coexisting. > > > > That creates an even bigger conversion later. Such big conversions > > cause more pain for backports, than doing an incremental conversion > > at appropriate times. > > > > Converting everything to g_autofree right now doesn't give style > > consistency as we'd still be matching VIR_ALLOC + g_autofree, > > so I don't see a benefit to a big conversion in 1 step. > > > > Incrementally converting VIR_ALLOC + VIR_AUTOFREE at the same > > time, makes more sense stylewise, as then within the scope of a > > single method we'd be consistent. > > I see your point about backports being more painful when you have > a bunch of unrelated changes mixed in, but I would still prefer if > we converted everything at once and at the same time introduced a > suitable syntax-check rule preventing more instances of whatever > function we just removed all callers of from creeping back in, or > actually just dropping the function altogether. > > Doing the conversion incrementally will IMHO result in dragging it > for much longer, causing more pain in the long run than ripping the > bandaid would. There's really not any significant real world pain from mixing the two styles. It is visually distasteful but doesn't cause any functional problems at runtime, nor complexity for maintainers. A large conversion over the whole codebase does cause very significant pain in conflicts for anyone cherry picking patches. That is just not a net win overall. I'll take visually mixed styles any day over creating patch conflicts in backports. 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] util: use glib base64 encoding/decoding APIs
On Mon, 2019-09-30 at 13:41 +0100, Daniel P. Berrangé wrote: > On Mon, Sep 30, 2019 at 02:18:17PM +0200, Andrea Bolognani wrote: > > On Mon, 2019-09-30 at 13:56 +0200, Pavel Hrdina wrote: > > > Agreed, for now let's keep all the wrappers but eventually we can remove > > > them to make the code cleaner. > > > > Note that we should be able to use VIR_AUTOPTR() instead of > > g_autoptr() even for objects, by simply registering the same > > GLib-provided free function with our own macros. > > > > That way we could keep using VIR_AUTO* everywhere and then, when > > we're ready, mechanically switch everything to g_auto* in one fell > > swoop, without having any point in time where the two styles are > > coexisting. > > That creates an even bigger conversion later. Such big conversions > cause more pain for backports, than doing an incremental conversion > at appropriate times. > > Converting everything to g_autofree right now doesn't give style > consistency as we'd still be matching VIR_ALLOC + g_autofree, > so I don't see a benefit to a big conversion in 1 step. > > Incrementally converting VIR_ALLOC + VIR_AUTOFREE at the same > time, makes more sense stylewise, as then within the scope of a > single method we'd be consistent. I see your point about backports being more painful when you have a bunch of unrelated changes mixed in, but I would still prefer if we converted everything at once and at the same time introduced a suitable syntax-check rule preventing more instances of whatever function we just removed all callers of from creeping back in, or actually just dropping the function altogether. Doing the conversion incrementally will IMHO result in dragging it for much longer, causing more pain in the long run than ripping the bandaid would. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] util: use glib base64 encoding/decoding APIs
On Mon, Sep 30, 2019 at 09:02:36AM +0200, Peter Krempa wrote:
> On Fri, Sep 27, 2019 at 18:17:28 +0100, Daniel Berrange wrote:
> > Replace use of the gnulib base64 module with glib's own base64 API family.
> >
> > Signed-off-by: Daniel P. Berrangé
> > ---
> > bootstrap.conf| 1 -
> > src/conf/virsecretobj.c | 38 +++
> > src/libvirt_private.syms | 1 -
> > src/libxl/libxl_conf.c| 3 +--
> > src/qemu/qemu_agent.c | 9 +++-
> > src/qemu/qemu_command.c | 5 ++--
> > src/qemu/qemu_domain.c| 8 +++
> > src/secret/secret_driver.c| 1 -
> > src/storage/storage_backend_rbd.c | 4 +---
> > src/util/virstring.c | 21 -
> > src/util/virstring.h | 2 --
> > tools/virsh-secret.c | 17 --
> > 12 files changed, 22 insertions(+), 88 deletions(-)
>
> [...]
>
> > @@ -698,23 +697,17 @@ virSecretObjSaveConfig(virSecretObjPtr obj)
> > int
> > virSecretObjSaveData(virSecretObjPtr obj)
> > {
> > -char *base64 = NULL;
> > -int ret = -1;
> > +g_autofree char *base64 = NULL;
>
> I'm not a fan of adding another style here. Either use VIR_FREE(), or
> convert all VIR_FREE to g_autofree first.
>
> I'm aware it will be unavoidable to use the glib auto pointer macro for
> complex types but we should at least here where it's interchangable have
> some kind of consistency.
I was attempting to be consistent in matching g_new/g_free/g_autofree,
but as you say, it probably makes more sense to be consistent within
the scope of a single method.
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] util: use glib base64 encoding/decoding APIs
On Mon, Sep 30, 2019 at 02:18:17PM +0200, Andrea Bolognani wrote:
> On Mon, 2019-09-30 at 13:56 +0200, Pavel Hrdina wrote:
> > On Mon, Sep 30, 2019 at 12:45:56PM +0100, Daniel P. Berrangé wrote:
> > > On Mon, Sep 30, 2019 at 01:41:52PM +0200, Pavel Hrdina wrote:
> > > > On Mon, Sep 30, 2019 at 09:02:36AM +0200, Peter Krempa wrote:
> > > > > On Fri, Sep 27, 2019 at 18:17:28 +0100, Daniel Berrange wrote:
> > > > > > @@ -698,23 +697,17 @@ virSecretObjSaveConfig(virSecretObjPtr obj)
> > > > > > int
> > > > > > virSecretObjSaveData(virSecretObjPtr obj)
> > > > > > {
> > > > > > -char *base64 = NULL;
> > > > > > -int ret = -1;
> > > > > > +g_autofree char *base64 = NULL;
> > > > >
> > > > > I'm not a fan of adding another style here. Either use VIR_FREE(), or
> > > > > convert all VIR_FREE to g_autofree first.
> > > > >
> > > > > I'm aware it will be unavoidable to use the glib auto pointer macro
> > > > > for
> > > > > complex types but we should at least here where it's interchangable
> > > > > have
> > > > > some kind of consistency.
> > > >
> > > > Here I agree with Peter, for this series I would use VIR_FREE() where
> > > > it's possible and only for glib objects we can use g_autoptr.
> > > >
> > > > But eventually I would like to switch to g_autofree and friends in order
> > > > to eliminate our specific helpers in favor of glib helpers.
> > > >
> > > > This also brings a question if we should keep our wrappers for glib or
> > > > use it directly. For example the string functions that we have.
> > >
> > > Where any libvirt code just duplicates something that alrady exists, then
> > > I think there's no compelling reason to keep it, the best code is code
> > > that doesn't exist.
> > >
> > > I don't want todo too many big bang replacements though, so I think best
> > > to deprecate existing libvirt code and phase it out incrementally in many
> > > cases.
> >
> > Agreed, for now let's keep all the wrappers but eventually we can remove
> > them to make the code cleaner.
>
> Note that we should be able to use VIR_AUTOPTR() instead of
> g_autoptr() even for objects, by simply registering the same
> GLib-provided free function with our own macros.
>
> That way we could keep using VIR_AUTO* everywhere and then, when
> we're ready, mechanically switch everything to g_auto* in one fell
> swoop, without having any point in time where the two styles are
> coexisting.
That creates an even bigger conversion later. Such big conversions
cause more pain for backports, than doing an incremental conversion
at appropriate times.
Converting everything to g_autofree right now doesn't give style
consistency as we'd still be matching VIR_ALLOC + g_autofree,
so I don't see a benefit to a big conversion in 1 step.
Incrementally converting VIR_ALLOC + VIR_AUTOFREE at the same
time, makes more sense stylewise, as then within the scope of a
single method we'd be consistent.
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] util: use glib base64 encoding/decoding APIs
On Mon, Sep 30, 2019 at 02:21:46PM +0200, Peter Krempa wrote: > On Mon, Sep 30, 2019 at 13:19:41 +0100, Daniel Berrange wrote: > > On Mon, Sep 30, 2019 at 02:03:53PM +0200, Peter Krempa wrote: > > > On Mon, Sep 30, 2019 at 13:56:06 +0200, Pavel Hrdina wrote: > > > > On Mon, Sep 30, 2019 at 12:45:56PM +0100, Daniel P. Berrangé wrote: > > > > > On Mon, Sep 30, 2019 at 01:41:52PM +0200, Pavel Hrdina wrote: > > > > > > On Mon, Sep 30, 2019 at 09:02:36AM +0200, Peter Krempa wrote: > > > > > > > On Fri, Sep 27, 2019 at 18:17:28 +0100, Daniel Berrange wrote: > > > > > > > > Replace use of the gnulib base64 module with glib's own base64 > > > > > > > > API family. > > > > > > > > > > > > > > > > Signed-off-by: Daniel P. Berrangé > > > > > > > > --- > > > > > > [..] > > > > > > > > > > > > > > > Here I agree with Peter, for this series I would use VIR_FREE() > > > > > > where > > > > > > it's possible and only for glib objects we can use g_autoptr. > > > > > > > > > > > > But eventually I would like to switch to g_autofree and friends in > > > > > > order > > > > > > to eliminate our specific helpers in favor of glib helpers. > > > > > > > > > > > > This also brings a question if we should keep our wrappers for glib > > > > > > or > > > > > > use it directly. For example the string functions that we have. > > > > > > > > > > Where any libvirt code just duplicates something that alrady exists, > > > > > then > > > > > I think there's no compelling reason to keep it, the best code is code > > > > > that doesn't exist. > > > > > > > > > > I don't want todo too many big bang replacements though, so I think > > > > > best > > > > > to deprecate existing libvirt code and phase it out incrementally in > > > > > many > > > > > cases. > > > > > > I agree in case of the other infrastructure for automatic pointers as > > > that will require more changes. > > > > > > In this case I don't see why we shouldn't just replace all use of > > > VIR_AUTOFREE with g_autofree if the idea is to use g_autofree from now > > > on. > > > > Well that's 1500 uses, across 150 files, so quite a big bang conversion. > > It would need to be split up quite alot otherwise it will be a backport > > conflict magnet. Certainly we want to clean this at some point, its just > > a question of timing. > > > > My preference is to focus on things with functional benefit as the higher > > priority. > > Then please stick with VIR_AUTOFREE for any code you plan to introduce. That forces an even bigger switch over at a later date. An incremental conversion is much less painful overall, even if there is a period when there are two styles in use. 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] util: use glib base64 encoding/decoding APIs
On Mon, Sep 30, 2019 at 13:19:41 +0100, Daniel Berrange wrote: > On Mon, Sep 30, 2019 at 02:03:53PM +0200, Peter Krempa wrote: > > On Mon, Sep 30, 2019 at 13:56:06 +0200, Pavel Hrdina wrote: > > > On Mon, Sep 30, 2019 at 12:45:56PM +0100, Daniel P. Berrangé wrote: > > > > On Mon, Sep 30, 2019 at 01:41:52PM +0200, Pavel Hrdina wrote: > > > > > On Mon, Sep 30, 2019 at 09:02:36AM +0200, Peter Krempa wrote: > > > > > > On Fri, Sep 27, 2019 at 18:17:28 +0100, Daniel Berrange wrote: > > > > > > > Replace use of the gnulib base64 module with glib's own base64 > > > > > > > API family. > > > > > > > > > > > > > > Signed-off-by: Daniel P. Berrangé > > > > > > > --- > > > > [..] > > > > > > > > > > > > Here I agree with Peter, for this series I would use VIR_FREE() where > > > > > it's possible and only for glib objects we can use g_autoptr. > > > > > > > > > > But eventually I would like to switch to g_autofree and friends in > > > > > order > > > > > to eliminate our specific helpers in favor of glib helpers. > > > > > > > > > > This also brings a question if we should keep our wrappers for glib or > > > > > use it directly. For example the string functions that we have. > > > > > > > > Where any libvirt code just duplicates something that alrady exists, > > > > then > > > > I think there's no compelling reason to keep it, the best code is code > > > > that doesn't exist. > > > > > > > > I don't want todo too many big bang replacements though, so I think best > > > > to deprecate existing libvirt code and phase it out incrementally in > > > > many > > > > cases. > > > > I agree in case of the other infrastructure for automatic pointers as > > that will require more changes. > > > > In this case I don't see why we shouldn't just replace all use of > > VIR_AUTOFREE with g_autofree if the idea is to use g_autofree from now > > on. > > Well that's 1500 uses, across 150 files, so quite a big bang conversion. > It would need to be split up quite alot otherwise it will be a backport > conflict magnet. Certainly we want to clean this at some point, its just > a question of timing. > > My preference is to focus on things with functional benefit as the higher > priority. Then please stick with VIR_AUTOFREE for any code you plan to introduce. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] util: use glib base64 encoding/decoding APIs
On Mon, Sep 30, 2019 at 02:03:53PM +0200, Peter Krempa wrote: > On Mon, Sep 30, 2019 at 13:56:06 +0200, Pavel Hrdina wrote: > > On Mon, Sep 30, 2019 at 12:45:56PM +0100, Daniel P. Berrangé wrote: > > > On Mon, Sep 30, 2019 at 01:41:52PM +0200, Pavel Hrdina wrote: > > > > On Mon, Sep 30, 2019 at 09:02:36AM +0200, Peter Krempa wrote: > > > > > On Fri, Sep 27, 2019 at 18:17:28 +0100, Daniel Berrange wrote: > > > > > > Replace use of the gnulib base64 module with glib's own base64 API > > > > > > family. > > > > > > > > > > > > Signed-off-by: Daniel P. Berrangé > > > > > > --- > > [..] > > > > > > > > > Here I agree with Peter, for this series I would use VIR_FREE() where > > > > it's possible and only for glib objects we can use g_autoptr. > > > > > > > > But eventually I would like to switch to g_autofree and friends in order > > > > to eliminate our specific helpers in favor of glib helpers. > > > > > > > > This also brings a question if we should keep our wrappers for glib or > > > > use it directly. For example the string functions that we have. > > > > > > Where any libvirt code just duplicates something that alrady exists, then > > > I think there's no compelling reason to keep it, the best code is code > > > that doesn't exist. > > > > > > I don't want todo too many big bang replacements though, so I think best > > > to deprecate existing libvirt code and phase it out incrementally in many > > > cases. > > I agree in case of the other infrastructure for automatic pointers as > that will require more changes. > > In this case I don't see why we shouldn't just replace all use of > VIR_AUTOFREE with g_autofree if the idea is to use g_autofree from now > on. Well that's 1500 uses, across 150 files, so quite a big bang conversion. It would need to be split up quite alot otherwise it will be a backport conflict magnet. Certainly we want to clean this at some point, its just a question of timing. My preference is to focus on things with functional benefit as the higher priority. 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] util: use glib base64 encoding/decoding APIs
On Mon, 2019-09-30 at 13:56 +0200, Pavel Hrdina wrote:
> On Mon, Sep 30, 2019 at 12:45:56PM +0100, Daniel P. Berrangé wrote:
> > On Mon, Sep 30, 2019 at 01:41:52PM +0200, Pavel Hrdina wrote:
> > > On Mon, Sep 30, 2019 at 09:02:36AM +0200, Peter Krempa wrote:
> > > > On Fri, Sep 27, 2019 at 18:17:28 +0100, Daniel Berrange wrote:
> > > > > @@ -698,23 +697,17 @@ virSecretObjSaveConfig(virSecretObjPtr obj)
> > > > > int
> > > > > virSecretObjSaveData(virSecretObjPtr obj)
> > > > > {
> > > > > -char *base64 = NULL;
> > > > > -int ret = -1;
> > > > > +g_autofree char *base64 = NULL;
> > > >
> > > > I'm not a fan of adding another style here. Either use VIR_FREE(), or
> > > > convert all VIR_FREE to g_autofree first.
> > > >
> > > > I'm aware it will be unavoidable to use the glib auto pointer macro for
> > > > complex types but we should at least here where it's interchangable have
> > > > some kind of consistency.
> > >
> > > Here I agree with Peter, for this series I would use VIR_FREE() where
> > > it's possible and only for glib objects we can use g_autoptr.
> > >
> > > But eventually I would like to switch to g_autofree and friends in order
> > > to eliminate our specific helpers in favor of glib helpers.
> > >
> > > This also brings a question if we should keep our wrappers for glib or
> > > use it directly. For example the string functions that we have.
> >
> > Where any libvirt code just duplicates something that alrady exists, then
> > I think there's no compelling reason to keep it, the best code is code
> > that doesn't exist.
> >
> > I don't want todo too many big bang replacements though, so I think best
> > to deprecate existing libvirt code and phase it out incrementally in many
> > cases.
>
> Agreed, for now let's keep all the wrappers but eventually we can remove
> them to make the code cleaner.
Note that we should be able to use VIR_AUTOPTR() instead of
g_autoptr() even for objects, by simply registering the same
GLib-provided free function with our own macros.
That way we could keep using VIR_AUTO* everywhere and then, when
we're ready, mechanically switch everything to g_auto* in one fell
swoop, without having any point in time where the two styles are
coexisting.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] util: use glib base64 encoding/decoding APIs
On Mon, Sep 30, 2019 at 13:56:06 +0200, Pavel Hrdina wrote: > On Mon, Sep 30, 2019 at 12:45:56PM +0100, Daniel P. Berrangé wrote: > > On Mon, Sep 30, 2019 at 01:41:52PM +0200, Pavel Hrdina wrote: > > > On Mon, Sep 30, 2019 at 09:02:36AM +0200, Peter Krempa wrote: > > > > On Fri, Sep 27, 2019 at 18:17:28 +0100, Daniel Berrange wrote: > > > > > Replace use of the gnulib base64 module with glib's own base64 API > > > > > family. > > > > > > > > > > Signed-off-by: Daniel P. Berrangé > > > > > --- [..] > > > > > > Here I agree with Peter, for this series I would use VIR_FREE() where > > > it's possible and only for glib objects we can use g_autoptr. > > > > > > But eventually I would like to switch to g_autofree and friends in order > > > to eliminate our specific helpers in favor of glib helpers. > > > > > > This also brings a question if we should keep our wrappers for glib or > > > use it directly. For example the string functions that we have. > > > > Where any libvirt code just duplicates something that alrady exists, then > > I think there's no compelling reason to keep it, the best code is code > > that doesn't exist. > > > > I don't want todo too many big bang replacements though, so I think best > > to deprecate existing libvirt code and phase it out incrementally in many > > cases. I agree in case of the other infrastructure for automatic pointers as that will require more changes. In this case I don't see why we shouldn't just replace all use of VIR_AUTOFREE with g_autofree if the idea is to use g_autofree from now on. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] util: use glib base64 encoding/decoding APIs
On Mon, Sep 30, 2019 at 12:45:56PM +0100, Daniel P. Berrangé wrote:
> On Mon, Sep 30, 2019 at 01:41:52PM +0200, Pavel Hrdina wrote:
> > On Mon, Sep 30, 2019 at 09:02:36AM +0200, Peter Krempa wrote:
> > > On Fri, Sep 27, 2019 at 18:17:28 +0100, Daniel Berrange wrote:
> > > > Replace use of the gnulib base64 module with glib's own base64 API
> > > > family.
> > > >
> > > > Signed-off-by: Daniel P. Berrangé
> > > > ---
> > > > bootstrap.conf| 1 -
> > > > src/conf/virsecretobj.c | 38 +++
> > > > src/libvirt_private.syms | 1 -
> > > > src/libxl/libxl_conf.c| 3 +--
> > > > src/qemu/qemu_agent.c | 9 +++-
> > > > src/qemu/qemu_command.c | 5 ++--
> > > > src/qemu/qemu_domain.c| 8 +++
> > > > src/secret/secret_driver.c| 1 -
> > > > src/storage/storage_backend_rbd.c | 4 +---
> > > > src/util/virstring.c | 21 -
> > > > src/util/virstring.h | 2 --
> > > > tools/virsh-secret.c | 17 --
> > > > 12 files changed, 22 insertions(+), 88 deletions(-)
> > >
> > > [...]
> > >
> > > > @@ -698,23 +697,17 @@ virSecretObjSaveConfig(virSecretObjPtr obj)
> > > > int
> > > > virSecretObjSaveData(virSecretObjPtr obj)
> > > > {
> > > > -char *base64 = NULL;
> > > > -int ret = -1;
> > > > +g_autofree char *base64 = NULL;
> > >
> > > I'm not a fan of adding another style here. Either use VIR_FREE(), or
> > > convert all VIR_FREE to g_autofree first.
> > >
> > > I'm aware it will be unavoidable to use the glib auto pointer macro for
> > > complex types but we should at least here where it's interchangable have
> > > some kind of consistency.
> >
> > Here I agree with Peter, for this series I would use VIR_FREE() where
> > it's possible and only for glib objects we can use g_autoptr.
> >
> > But eventually I would like to switch to g_autofree and friends in order
> > to eliminate our specific helpers in favor of glib helpers.
> >
> > This also brings a question if we should keep our wrappers for glib or
> > use it directly. For example the string functions that we have.
>
> Where any libvirt code just duplicates something that alrady exists, then
> I think there's no compelling reason to keep it, the best code is code
> that doesn't exist.
>
> I don't want todo too many big bang replacements though, so I think best
> to deprecate existing libvirt code and phase it out incrementally in many
> cases.
Agreed, for now let's keep all the wrappers but eventually we can remove
them to make the code cleaner.
Pavel
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] util: use glib base64 encoding/decoding APIs
On Mon, Sep 30, 2019 at 01:41:52PM +0200, Pavel Hrdina wrote:
> On Mon, Sep 30, 2019 at 09:02:36AM +0200, Peter Krempa wrote:
> > On Fri, Sep 27, 2019 at 18:17:28 +0100, Daniel Berrange wrote:
> > > Replace use of the gnulib base64 module with glib's own base64 API family.
> > >
> > > Signed-off-by: Daniel P. Berrangé
> > > ---
> > > bootstrap.conf| 1 -
> > > src/conf/virsecretobj.c | 38 +++
> > > src/libvirt_private.syms | 1 -
> > > src/libxl/libxl_conf.c| 3 +--
> > > src/qemu/qemu_agent.c | 9 +++-
> > > src/qemu/qemu_command.c | 5 ++--
> > > src/qemu/qemu_domain.c| 8 +++
> > > src/secret/secret_driver.c| 1 -
> > > src/storage/storage_backend_rbd.c | 4 +---
> > > src/util/virstring.c | 21 -
> > > src/util/virstring.h | 2 --
> > > tools/virsh-secret.c | 17 --
> > > 12 files changed, 22 insertions(+), 88 deletions(-)
> >
> > [...]
> >
> > > @@ -698,23 +697,17 @@ virSecretObjSaveConfig(virSecretObjPtr obj)
> > > int
> > > virSecretObjSaveData(virSecretObjPtr obj)
> > > {
> > > -char *base64 = NULL;
> > > -int ret = -1;
> > > +g_autofree char *base64 = NULL;
> >
> > I'm not a fan of adding another style here. Either use VIR_FREE(), or
> > convert all VIR_FREE to g_autofree first.
> >
> > I'm aware it will be unavoidable to use the glib auto pointer macro for
> > complex types but we should at least here where it's interchangable have
> > some kind of consistency.
>
> Here I agree with Peter, for this series I would use VIR_FREE() where
> it's possible and only for glib objects we can use g_autoptr.
>
> But eventually I would like to switch to g_autofree and friends in order
> to eliminate our specific helpers in favor of glib helpers.
>
> This also brings a question if we should keep our wrappers for glib or
> use it directly. For example the string functions that we have.
Where any libvirt code just duplicates something that alrady exists, then
I think there's no compelling reason to keep it, the best code is code
that doesn't exist.
I don't want todo too many big bang replacements though, so I think best
to deprecate existing libvirt code and phase it out incrementally in many
cases.
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] util: use glib base64 encoding/decoding APIs
On Mon, Sep 30, 2019 at 09:02:36AM +0200, Peter Krempa wrote:
> On Fri, Sep 27, 2019 at 18:17:28 +0100, Daniel Berrange wrote:
> > Replace use of the gnulib base64 module with glib's own base64 API family.
> >
> > Signed-off-by: Daniel P. Berrangé
> > ---
> > bootstrap.conf| 1 -
> > src/conf/virsecretobj.c | 38 +++
> > src/libvirt_private.syms | 1 -
> > src/libxl/libxl_conf.c| 3 +--
> > src/qemu/qemu_agent.c | 9 +++-
> > src/qemu/qemu_command.c | 5 ++--
> > src/qemu/qemu_domain.c| 8 +++
> > src/secret/secret_driver.c| 1 -
> > src/storage/storage_backend_rbd.c | 4 +---
> > src/util/virstring.c | 21 -
> > src/util/virstring.h | 2 --
> > tools/virsh-secret.c | 17 --
> > 12 files changed, 22 insertions(+), 88 deletions(-)
>
> [...]
>
> > @@ -698,23 +697,17 @@ virSecretObjSaveConfig(virSecretObjPtr obj)
> > int
> > virSecretObjSaveData(virSecretObjPtr obj)
> > {
> > -char *base64 = NULL;
> > -int ret = -1;
> > +g_autofree char *base64 = NULL;
>
> I'm not a fan of adding another style here. Either use VIR_FREE(), or
> convert all VIR_FREE to g_autofree first.
>
> I'm aware it will be unavoidable to use the glib auto pointer macro for
> complex types but we should at least here where it's interchangable have
> some kind of consistency.
Here I agree with Peter, for this series I would use VIR_FREE() where
it's possible and only for glib objects we can use g_autoptr.
But eventually I would like to switch to g_autofree and friends in order
to eliminate our specific helpers in favor of glib helpers.
This also brings a question if we should keep our wrappers for glib or
use it directly. For example the string functions that we have.
Pavel
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] util: use glib base64 encoding/decoding APIs
On Fri, Sep 27, 2019 at 18:17:28 +0100, Daniel Berrange wrote:
> Replace use of the gnulib base64 module with glib's own base64 API family.
>
> Signed-off-by: Daniel P. Berrangé
> ---
> bootstrap.conf| 1 -
> src/conf/virsecretobj.c | 38 +++
> src/libvirt_private.syms | 1 -
> src/libxl/libxl_conf.c| 3 +--
> src/qemu/qemu_agent.c | 9 +++-
> src/qemu/qemu_command.c | 5 ++--
> src/qemu/qemu_domain.c| 8 +++
> src/secret/secret_driver.c| 1 -
> src/storage/storage_backend_rbd.c | 4 +---
> src/util/virstring.c | 21 -
> src/util/virstring.h | 2 --
> tools/virsh-secret.c | 17 --
> 12 files changed, 22 insertions(+), 88 deletions(-)
[...]
> @@ -698,23 +697,17 @@ virSecretObjSaveConfig(virSecretObjPtr obj)
> int
> virSecretObjSaveData(virSecretObjPtr obj)
> {
> -char *base64 = NULL;
> -int ret = -1;
> +g_autofree char *base64 = NULL;
I'm not a fan of adding another style here. Either use VIR_FREE(), or
convert all VIR_FREE to g_autofree first.
I'm aware it will be unavoidable to use the glib auto pointer macro for
complex types but we should at least here where it's interchangable have
some kind of consistency.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
