[libvirt] [PATCH v2 22/38] daemon: Introduce virNetServerProgramSendStreamSkip

[John Ferlan]

On 05/10/2017 07:53 AM, Michal Privoznik wrote:
> On 05/05/2017 05:26 PM, John Ferlan wrote:
>>
>>
>> On 04/20/2017 06:01 AM, Michal Privoznik wrote:
>>> This is just a helper function that takes in a length value,
>>> encodes it into XDR and sends to client.
>>
>> would be adjusted w/ @flags arg....
>>
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>>> ---
>>>  src/libvirt_remote.syms       |  1 +
>>>  src/rpc/virnetserverprogram.c | 33 +++++++++++++++++++++++++++++++++
>>>  src/rpc/virnetserverprogram.h |  7 +++++++
>>>  3 files changed, 41 insertions(+)
>>>
>>> diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
>>> index ca1f3ac..29dceab 100644
>>> --- a/src/libvirt_remote.syms
>>> +++ b/src/libvirt_remote.syms
>>> @@ -178,6 +178,7 @@ virNetServerProgramNew;
>>>  virNetServerProgramSendReplyError;
>>>  virNetServerProgramSendStreamData;
>>>  virNetServerProgramSendStreamError;
>>> +virNetServerProgramSendStreamSkip;
>>>  virNetServerProgramUnknownError;
>>>  
>>>  
>>> diff --git a/src/rpc/virnetserverprogram.c b/src/rpc/virnetserverprogram.c
>>> index d1597f4..6d84056 100644
>>> --- a/src/rpc/virnetserverprogram.c
>>> +++ b/src/rpc/virnetserverprogram.c
>>> @@ -548,6 +548,39 @@ int virNetServerProgramSendStreamData(virNetServerProgramPtr prog,
>>>  }
>>>  
>>>  
>>> +int virNetServerProgramSendStreamSkip(virNetServerProgramPtr prog,
>>> +                                      virNetServerClientPtr client,
>>> +                                      virNetMessagePtr msg,
>>> +                                      int procedure,
>>> +                                      unsigned int serial,
>>> +                                      unsigned long long length)
>>
>> Doesn't follow the newer style
>>
>> int
>> vir...(args...)
>>
>>
>> Of course now it starts dawning on me... if the functions change to
>> SetSkip and GetSkip - then I'd assume that has impact for the RPC
>> nomenclature too. Of course seeing "SetSkip" in a name for RPC would
>> make things even more clear (unless of course you're the one that's been
>> working on this code for a long time and already have the names burned
>> into your memory).
> 
> Exactly. For me Skip and HoleSize have clear meaning. So your suggestion
> is to have: SetSkip and GetSkip? Well, I don't like it that much but if
> that's the only thing that should prevent this from merging ...
> 
> Also, as I've said earlier, I'm gonna send v2 without any name change
> for the time being. As you've correctly noticed, a lot of functions, RPC
> calls, and other stuff have their name derived from current naming.
> Therefore changing that would be a non-trivial amount of work and
> therefore I'd like to do it just once. After we have a clear agreement
> on the naming.
> 
> Also, now that we seem to be using Reviewed-by tags, should I include
> those in my commit messages for v2? I mean, you've reviewed patches
> 00-30. And you'll definitely deserve the tag. But what I'm asking is if
> I should put it there already for v2 or before pushing the patches.
> 
> To extend my thinking further, review is not the same as ACK. So
> Reviewed-by should include all the reviewers even though just one of
> them gave ACK? Or if a patch is respin in version X, should it include
> all the past reviewers? So many questions and not many answers.
> 
> Michal
> 

AFAIC the Reviewed-By morphed from the Tested-By discussion which also
had a variant Signed-Off-By discussion.  I don't think there's a clear
consensus. IDC whether a R-b tag is attributed or not. We don't seem to
have group norms or rules for that yet. Part of me feels though that an
R-B tag would be "functionally equivalent" to an ACK - so if there isn't
an ACK, then should there by an R-B even though clearly it was reviewed.
I don't keep track of "other" norms w/r/t to all these tags...

As for the function names... Well that's the bugger. On one hand this
series started as an RFC long before the 'new' naming rules were put in
place, so perhaps there's an invokable "grandfather clause" that allows
you to bypass the new naming rules.  On the other hand, we should try to
follow the group rules/norms for naming functions for any patches. I'm
fine going with group consensus on this if that's even a possible
achievement,

IOW: is there anyone else reading this series that would like to chime
in with their opinion? How does one bump up the discussion to get such
opinion in the event no one's reading. You're fortunate to at least sit
near a few other such opinionated people ;-). I can "self translate" the
naming scheme here into what I was thinking about too.

John