Re: [libvirt] [PATCH v2 08/38] util: Introduce virFileInData

On 04/20/2017 06:01 AM, Michal Privoznik wrote: FWIW: This feels like an "inflection point" from which the remainder of the algorithms are built around. I've forgotten anything from the discussion for the original set of patches, but since I had a few cycles to think... Apologies in advance for the length. I guess I was under the impression that the purpose for sparse streams is to find hole's in a file|stream and ensure those don't get copied instead generating the hole on the target. If so, a bunch of different small API's come to mind after reading this patch and trying to internalize what's taking place: vir{File|Stream}SeekHoleSupported -> If SEEK_{DATA|HOLE} returns -1 and EINVAL, then either don't use sparse streams and force usage of the non sparse option or just fail. vir{File|Stream}HasHole -> From beginning of file, find first HOLE < EOF -> If none, then sparse streams won't matter, right? vir{File|Stream}IsInHole -> From @cur if SEEK_DATA > SEEK_HOLE vir{File|Stream}IsInData -> From @cur if SEEK_HOLE > SEEK_DATA vir{File|Stream}GetDataEnd -> From @cur of DATA return @length vir{File|Stream}GetHoleEnd -> From @cur of HOLE return @length We should know where we are and be able to keep track, right? Having to determine @cur each time just seems to be overhead. For the purpose of what's being done - I would think we start at 0 and know we have a findable @end - then it would be "just" (hah!) a matter of ensuring we have holes and then a sequence of finding hole/data from whence we start (since theoretically we could start in a hole, right?). Maybe the following is too simplistic... if (supportsSparse) if ((cur = lseek(fd, 0, SEEK_SET)) != 0) goto error; if (!hasHoles) return fullCopy? (although I think the rest would still work) if ((end = lseek(fd, 0, SEEK_END)) < 0) goto error; inHole = virFile|StreamIsInHole(fd, cur) while (cur < end) if (inHole) get size of hole if ret < 0, then size would be end-cur, right? update remote to write hole update cur to include size of hole else /* assume? in data */ get size of data if ret < 0, then size would be end-cur, right? update remote to include data update cur to include size of data I guess I just became concerned about the various decision/exit points in virFileInData below and started wondering if they were really needed. s/b int virFileIndata(...) Lacking any sort of description of input/returns. Lot's of variables here too - whether fd valid/invalid could cause error... Setting *inData based on where we are... Setting *length based on no returning a -1... and of course returning -1 based on some lseek call failing for various reasons. Usage seems to be only boolean... One would hope this doesn't fail, still could @cur be input? I guess I wonder why we're looking for current position if the purpose is to copy from start to end ensuring that holes aren't copied, but rather just created on the target. I am assuming this function is for the "source" side. if data < 0 and errno = EINVAL, seeking data/holes isn't supported, true? If so then it might be a possible function to check if data/hole seeking is supported. So for the purpose of what you're trying to do should we care? I suppose if you're in a hole and cannot find the end that might be odd. Still if we know upfront what @cur and what @end are, then does some crazy trailing hole/end/eof need to cause an error? Now I'm wondering about the implicit hole @ EOF described above? and the error checks from that that aren't done here. Still does this matter in the "case" you want to use this for? That is if there's no HOLE from @data to @end, then for the purpose of stream copying would you'd just be copying everything from data -> end (e.g. @length = (end - data)? IOW, does it really matter? Would the "best" we can do is just copy everything from this point to the @end regardless of whether that's copying a hole? Again it's just another error that perhaps we could avoid. > + goto cleanup; > + } else { > + /* case 2 */ > + *length = (hole - data); > + } > + } > + > + ret = 0; > + cleanup: > + /* At any rate, reposition back to where we started. */ > + if (cur != (off_t) -1) > + ignore_value(lseek(fd, cur, SEEK_SET)); > return ret; > } > > diff --git a/src/util/virfile.h b/src/util/virfile.h > index 41c25a2..32a1d3c 100644 > --- a/src/util/virfile.h > +++ b/src/util/virfile.h > @@ -340,4 +340,7 @@ int virFileReadValueInt(const char *path, int *value); > int virFileReadValueUint(const char *path, unsigned int *value); > int virFileReadValueBitmap(const char *path, int maxlen, virBitmapPtr *value); > > + unsigned long long *length); You could also go with Pavel's preferred format to follow the .c file format. I'm too lazy to think... Are making sure the following cases: start-data-end start-hole-data-end start-data-hole-end start-hole-data-hole-data-end start-data-hole-data-hole-end start-hole-data-hole-data-hole-end start-data-hole-data-hole-data-end Trying to think if the following would be valid: start-hole-end but my brain keeps telling me to stop thinking. lld on an ull? John I'll be OOO on Mon/Tue - just to "set" expectations of any chance at me reading a response!