On Tue, Jul 02, 2019 at 10:27:50AM +0200, Ilias Stamatis wrote: > On Tue, Jul 2, 2019 at 10:04 AM Erik Skultety <eskultet@xxxxxxxxxx> wrote: > > > > On Thu, Jun 20, 2019 at 05:11:57PM +0200, Ilias Stamatis wrote: > > > The current implementation has the following bugs: > > > > > > - the vm variable is accessed after calling virDomainObjEndAPI on it > > > > > > - if VIR_STRDUP fails and we jump to the cleanup section, we're calling > > > VIR_FREE on pointers for which we haven't allocated memory > > > > > > - the error type VIR_DOMAIN_DISK_ERROR_NONE is used which contradicts > > > the documentation of the API that says that disks with no errors are not > > > reported > > > > > > This patch fixes all of them and additionally reports errors only for > > > every second disk (instead of reporting errors for all disks) which was > > > the initial intention. > > > > I believe each patch should focus ideally on 1 thing only. So this patch should > > IMO be split into 3 (doesn't matter in what order), each fixing a different bug. > > > > Erik > > Hmm. I think it's impossible to solve the first 2 separately. Because > by solving the 2nd one the 1st is solved as well. The 3rd one is > really adding a "-1" and a "+1" in the VIR_DOMAIN_DISK_ERROR_LAST > line. But this line is touched anyway by the previous fix as well. So > I thought it doesn't worth it to introduce a separate patch for it. > > I think the patch in general is quite small so it shouldn't bet a huge > problem, but if you insist I can separate the 1-line change for fix > no3. from the ones before. Since [1] already addressed almost all of the problems, I don't think there's any need in further review of this series. Erik -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
