[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 11/38] Introduce virStreamSkip




On 05/05/2017 07:25 AM, Michal Privoznik wrote:
> On 05/04/2017 11:29 PM, John Ferlan wrote:
>>
>>
>> On 04/20/2017 06:01 AM, Michal Privoznik wrote:
>>> This API can be used to tell the other side of the stream to skip
>>
>> s/can be/is  (unless it can be used for something else ;-))
>>
>>> some bytes in the stream. This can be used to create a sparse
>>> file on the receiving side of a stream.
>>>
>>> It takes just one argument @length, which says how big the hole
>>> is. Since our streams are not rewindable like regular files, we
>>> don't need @whence argument like seek(2) has.
>>
>> lseek is an implementation detail...  However, it could be stated that
>> the skipping would be from the current point in the file forward by some
>> number of bytes.  It's expected to be used in conjunction with code that
>> is copying over the real (or non-zero) data and should be considered an
>> optimization over sending zere data segments.
>>
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn redhat com>
>>> ---
>>>  include/libvirt/libvirt-stream.h |  3 +++
>>>  src/driver-stream.h              |  5 ++++
>>>  src/libvirt-stream.c             | 57 ++++++++++++++++++++++++++++++++++++++++
>>>  src/libvirt_public.syms          |  1 +
>>>  4 files changed, 66 insertions(+)
>>>
>>
>> While it would be unused for now, should @flags be added.  Who knows
>> what use it could have, but avoids a new Flags API, but does cause a few
>> other wording changes here.
> 
> Ah sure. We should have @flags there. Good point.
> 
>>
>> Perhaps it's just me - but "Skip" and "HoleSize" just seem awkward.
>> Would "virStreamSetSkip" and "virStreamGetSkip" be more apropos? (or
>> s/Skip/HoleSize/ - ewww). Names would then follow our more recent
>> function naming guidelines. I think I dislike the HoleSize much more
>> than the Skip.
> 
> SetSkip and GetSkip sound wrong to me instead :D
> 

I understand completely (in more ways than one)! However, I still have
the hacking/coding style guide to fallback upon that requires certain
syntax ;-)... While Skip "qualifies" as a verb and perhaps squeaks by on
that technicality - the "HoleSize" has no verb telling me what HoleSize
is doing. The dichotomy that "Skip" doesn't have a Size object, but Hole
does isn't lost either. I actually think Size is a "given", but as
someone who has trouble creating names that pass muster when my code is
reviewed - who am I to give too much advice here!

Also now that I'm further down the road - I keep having to attempt to
remind myself whether this is a we're setting/performing the set skip
operation or this is a we're getting the set hole size operation.
Function, RPC, structures, etc. names would help unmuddle things.

>>
>>> diff --git a/include/libvirt/libvirt-stream.h b/include/libvirt/libvirt-stream.h
>>> index bee2516..4e0a599 100644
>>> --- a/include/libvirt/libvirt-stream.h
>>> +++ b/include/libvirt/libvirt-stream.h
>>> @@ -50,6 +50,9 @@ int virStreamRecvFlags(virStreamPtr st,
>>>                         size_t nbytes,
>>>                         unsigned int flags);
>>>  
>>> +int virStreamSkip(virStreamPtr st,
>>> +                  unsigned long long length);
>>
>> Was there consideration for using 'off_t' instead of ULL? I know it's an
>> implementation detail of virFDStreamData and lseek() usage, but it does
>> hide things... IDC either way.
> 
> The problem with off_t is that it is signed type, while ULL is unsigned.
> There's not much point in sending a negative offset, is there?
> Moreover, we use ULL for arguments like offset (not sure really why).
> Frankly, I don't really know why. Perhaps some types don't exist everywhere?
> 
> Michal
> 

The thing is the implementation uses a function that expects an off_t.
When I see ULL I'm usually considering memory sizes which can be large.
Not that a file couldn't be, but it usually isn't.

The concern is - could we run into a situation where a coding error
somewhere supplies a negative value that will now appear to be some
inordinately large positive value; whereas, we could avoid that by
stipulating 'virStreamSkip' (or whatever it gets called) expects a
positive and/or greater than 0 value (e.g. virCheckPositiveArgGoto).

Also as I see it, virFileInData calculates *length from two off_t's and
in none of those calculations is it possible to generate a negative
number. If we did, something is wrong. Still since virStreamSkip can be
called and not necessarily use that calculation.

I'll also point out the coding example provided for virStreamSkip:

+ *   while (1) {
+ *     char buf[4096];
+ *     size_t len;               <=====
+ *     if (..in hole...) {
+ *       ..get hole size...
+ *       virStreamSkip(st, len);
                            ^^^^  <=== Not a ULL...

Although the corollary did use the ULL for virStreamHoleSize in its example.

Similar argument for virStreamHoleSize could be made - we shouldn't
receive a negative value, but we have no way of differentiating a coding
mistake and an inordinately large value.

At least by enforcing usage of 'off_t' we're saying - go forward...

John


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]