On Wed, Dec 16, 2015 at 09:19:07AM -0500, John Ferlan wrote: > > > On 12/16/2015 08:48 AM, Ján Tomko wrote: > > On Tue, Dec 15, 2015 at 03:03:33PM -0500, John Ferlan wrote: > >> https://bugzilla.redhat.com/show_bug.cgi?id=1270709 > >> > >> When a volume wipe is successful, perform a volume refresh afterwards to > >> update any volume data that may be used in future volume commands, such as > >> volume resize. For a raw file volume, a wipe could truncate the file and > >> a followup volume resize the capacity may fail because the volume target > >> allocation isn't updated to reflect the wipe activity. > >> > >> Since the documentation doesn't mention this side effect of the zero > >> algorithm for a raw file volume, update the various documentation to > >> describe the results. > >> > > > > The documentation does not belong in this patch. Also, we could > > intentionally keep it vague so that we don't have to commit to that > > behavior. > > > > Adding the documentation was a reaction to your review of v1: > > http://www.redhat.com/archives/libvir-list/2015-December/msg00464.html > > where you queried whether we should update the documentation "to reflect > that there might not be any pass over the old data". > I was thinking out loud and I did not mean that the documentation changes should be a part of this patch. Sorry if I was not clear enough. > Whether the doc changes are kept or not I suppose then becomes > "consensus based". I see value in describing what's done and I see value > in being vague enough so that we could change to do something else in > the future. > > The question thus remains, should the current truncate be considered a > bug? Or a happy consequence/feature? > I have a slight preference for treating it as a feature and not adjusting the docs. > >> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx> > >> --- > >> v1: > >> http://www.redhat.com/archives/libvir-list/2015-December/msg00085.html > >> > >> Changes since v1: > >> * Use the preferred call format from review > > > >> * Cause error if refreshVol fails > > > > If my patch adjusting the return value gets pushed before this one: > > https://www.redhat.com/archives/libvir-list/2015-December/msg00467.html > > > > that change is just cosmetic. > > yep. > > > Otherwise, I don't think a patch adding refreshVol should be changing > > the return value. > > > > So again, your initial review says: > > More readable as: > if (func() < 0) > goto cleanup; > > Which is what I followed. Other than the value of ret being -errno on > fdatasync - is the objection based solely on you'd like to see a > separate patch to handle the ret differently for the wipeVol call, then > a patch for refreshVol? > The patch I linked ajdusts the return value for the only code path that did not follow the 0/-1 convention. So: ACK to the non-documenation hunk of this patch, in its entirety. I would prefer if you could wait for my patch adjusting the return values before pushing this one, but that's not a hard prerequisite. Jan
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list