This is just a helper function that takes in a length value,
encodes it into XDR and sends to client.

Signed-off-by: Michal Privoznik <mprivozn@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)
+{
+    virNetStreamSkip data;
+
+    VIR_DEBUG("client=%p msg=%p length=%llu", client, msg, length);
+
+    memset(&data, 0, sizeof(data));
+    data.length = length;
+
+    msg->header.prog = prog->program;
+    msg->header.vers = prog->version;
+    msg->header.proc = procedure;
+    msg->header.type = VIR_NET_STREAM_SKIP;
+    msg->header.serial = serial;
+    msg->header.status = VIR_NET_CONTINUE;
+
+    if (virNetMessageEncodeHeader(msg) < 0)
+        return -1;
+
+    if (virNetMessageEncodePayload(msg,
+                                   (xdrproc_t) xdr_virNetStreamSkip,
+                                   &data) < 0)
+        return -1;
+
+    return virNetServerClientSendMessage(client, msg);
+}
+
+
 void virNetServerProgramDispose(void *obj ATTRIBUTE_UNUSED)
 {
 }
diff --git a/src/rpc/virnetserverprogram.h b/src/rpc/virnetserverprogram.h
index 531fca0..eba2168 100644
--- a/src/rpc/virnetserverprogram.h
+++ b/src/rpc/virnetserverprogram.h
@@ -104,4 +104,11 @@ int virNetServerProgramSendStreamData(virNetServerProgramPtr prog,
                                       const char *data,
                                       size_t len);
 
+int virNetServerProgramSendStreamSkip(virNetServerProgramPtr prog,
+                                      virNetServerClientPtr client,
+                                      virNetMessagePtr msg,
+                                      int procedure,
+                                      unsigned int serial,
+                                      unsigned long long length);
+
 #endif /* __VIR_NET_SERVER_PROGRAM_H__ */
-- 
2.10.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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).

Will need a flags argument too for @data...


John

> +{
> +    virNetStreamSkip data;
> +
> +    VIR_DEBUG("client=%p msg=%p length=%llu", client, msg, length);
> +
> +    memset(&data, 0, sizeof(data));
> +    data.length = length;
> +
> +    msg->header.prog = prog->program;
> +    msg->header.vers = prog->version;
> +    msg->header.proc = procedure;
> +    msg->header.type = VIR_NET_STREAM_SKIP;
> +    msg->header.serial = serial;
> +    msg->header.status = VIR_NET_CONTINUE;
> +
> +    if (virNetMessageEncodeHeader(msg) < 0)
> +        return -1;
> +
> +    if (virNetMessageEncodePayload(msg,
> +                                   (xdrproc_t) xdr_virNetStreamSkip,
> +                                   &data) < 0)
> +        return -1;
> +
> +    return virNetServerClientSendMessage(client, msg);
> +}
> +
> +
>  void virNetServerProgramDispose(void *obj ATTRIBUTE_UNUSED)
>  {
>  }
> diff --git a/src/rpc/virnetserverprogram.h b/src/rpc/virnetserverprogram.h
> index 531fca0..eba2168 100644
> --- a/src/rpc/virnetserverprogram.h
> +++ b/src/rpc/virnetserverprogram.h
> @@ -104,4 +104,11 @@ int virNetServerProgramSendStreamData(virNetServerProgramPtr prog,
>                                        const char *data,
>                                        size_t len);
>  
> +int virNetServerProgramSendStreamSkip(virNetServerProgramPtr prog,
> +                                      virNetServerClientPtr client,
> +                                      virNetMessagePtr msg,
> +                                      int procedure,
> +                                      unsigned int serial,
> +                                      unsigned long long length);
> +
>  #endif /* __VIR_NET_SERVER_PROGRAM_H__ */
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 05/10/2017 01:53 PM, 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@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.

For some unknown reason I failed to see the obvious.
virStreamSendHole()   <-- for public APIs
virStreamRecvHole()

VIR_NET_STREAM_HOLE   <-- for RPC packet

What do you think?

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 05/11/2017 02:36 AM, Michal Privoznik wrote:
> On 05/10/2017 01:53 PM, 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@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.
> 
> For some unknown reason I failed to see the obvious.
> virStreamSendHole()   <-- for public APIs
> virStreamRecvHole()
> 
> VIR_NET_STREAM_HOLE   <-- for RPC packet
> 
> What do you think?
> 
> Michal
> 

Seems reasonable - it'll be (hopefully) obvious that the two are related
and should cause no "self inflicted pain" to figure out which is a send
and which is a receive that I kept tripping over with Skip and HoleSize.

John

FWIW: At one point during reading the code I think I also had the hey
why not just use the @flags argument for Send/Recv APIs, but I recall
ruling it out since @data is checked for NULL and of course that would
"overload" those API's.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list