On 7/27/23 19:36, Eric Blake wrote:
> On Thu, Jul 27, 2023 at 01:23:24PM +0200, Laszlo Ersek wrote:
>> On 7/26/23 19:50, Eric Blake wrote:
>>> This reverts commit 6725fa0e129f9a60d7b89707ef8604e0aeeeaf43, although
>>> with a more modern style.
>>>
>>> Casting a C array to a Go slice just to benefit from potential
>>> optimizations in Go's copy(), is rather complex to understand,
>>> especially when we are still copying things (the main reason to treat
>>> a C array as a Go slice is when avoiding a copy has a benefit).
>>> Without a benchmark showing measurable difference in runtime speed,
>>> and considering that network transit time probably dominates the time
>>> spent on block status and its callback, it is not worth the
>>> complexity. Furthermore, an upcoming patch wants to add a similar
>>> helper function for converting between a list of C and Go structs,
>>> where the copy() trick will not work; and having the two helpers look
>>> alike is beneficial.
>>>
>>> Suggested-by: Laszlo Ersek <ler...@redhat.com>
>>
>> I've needed to dig a bit, but indeed, bullet (8) in:
>>
>> 0e4ff751-88d6-837b-15a5-6f6c370a2f09@redhat.com">http://mid.mail-archive.com/0e4ff751-88d6-837b-15a5-6f6c370a2f09@redhat.com
>> https://listman.redhat.com/archives/libguestfs/2023-June/031736.html
>>
>>> CC: Nir Soffer <nsof...@redhat.com>
>>> Signed-off-by: Eric Blake <ebl...@redhat.com>
>>> ---
>>> generator/GoLang.ml | 14 ++++++++------
>>> 1 file changed, 8 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/generator/GoLang.ml b/generator/GoLang.ml
>>> index 0aa83bdc..77dacadb 100644
>>> --- a/generator/GoLang.ml
>>> +++ b/generator/GoLang.ml
>>> @@ -509,12 +509,14 @@ let
>>>
>>> /* Closures. */
>>>
>>> -func copy_uint32_array (entries *C.uint32_t, count C.size_t) []uint32 {
>>> - ret := make([]uint32, int (count))
>>> - // See
>>> https://github.com/golang/go/wiki/cgo#turning-c-arrays-into-go-slices
>>> - // TODO: Use unsafe.Slice() when we require Go 1.17.
>>> - s := (*[1<<30]uint32)(unsafe.Pointer(entries))[:count:count]
>>> - copy(ret, s)
>>> +func copy_uint32_array(entries *C.uint32_t, count C.size_t) []uint32 {
>>> + ret := make([]uint32, int(count))
>>> + addr := uintptr(unsafe.Pointer(entries))
>>> + for i := 0; i < int(count); i++ {
>>> + ptr := (*C.uint32_t)(unsafe.Pointer(addr))
>>> + ret[i] = uint32(*ptr)
>>> + addr += unsafe.Sizeof(*ptr)
>>> + }
>>> return ret
>>> }
>>> ";
>>
>> This patch mixes four things:
>>
>> - whitespace changes (due to style modernization / gofmt, presumably),
>> - reverting commit 6725fa0e129f,
>> - changes proposed in my email,
>> - functional changes on top of my email.
>>
>> The "func" line matches my proposal (OK), with additional whitespace updates
>> (OK), but has nothing to do with reverting 6725fa0e129f, so calling the
>> patch a "revert" is misleading.
>
> Fair enough. It is undoing the effects of the earlier patch, but not
> in the same way as a straight revert (in part because enough else has
> changed in the meantime that a straight revert is no longer possible).
> Plus the decision on where to stage this in the series (at one point,
> I had it after 6/7 - and you already noted there how much rebase churn
> gets caused as we mix this series around).
>
>>
>> The initialization of "ret" undergoes a whitespace update (OK), but is
>> neither a revert (6725fa0e129f did not change the initialization of "ret"),
>> nor does it match my proposal. In my proposal, I had removed the "int" cast
>> (or conversion) intentionally. Casting a C size_t to a Go int seems wrong.
>> (IIRC I had verified the widths of the Go integer types from the Go
>> documentation.)
>
> I completely missed that point. You do make a valid point that a C
> size_t might be larger than a Go int (does Go even try to run on
> 32-bit platforms?) - but we DO have the additional knowledge that
> because our block status reply results cannot exceed 64M over the
> wire, any count passed to the callback function will fit in 32 bits
> regardless of the width of C's size_t. Maybe an assertion that count
> does not lose precision when cast to Go int is sufficient?
Sure, an assertion (with explanation) would be fine.
>
>>
>> The initialization of "addr" matches my proposal, with some whitespace
>> updates (OK), but is not a revert of 6725fa0e129f.
>>
>> The "for" statement is a revert, but does not match my proposal. My proposal
>> made sure that the loop variable was a Go uint64, so that it could
>> accommodate the C size_t. The only Go syntax I found suitable for that was
>> to replace the ":=" embedded in the "for", with a separate
>> definition/initialization of "i" (where I could spell out the uint64 type),
>> and then to use the "for" variant that was effectively a "while" (using C
>> terminology):
>>
>> var i uint64 = 0
>> for i < uint64(count) {
>> ...
>> i++
>> }
>>
>> Here we cast the C size_t to uint64, which is OK.
>>
>> A different approach to the same end, preserving the ":=" syntax in "for",
>> could be:
>>
>> for i := uint64(0); i < uint64(count); i++ {
>
> I don't know enough Go syntax to quickly state if there is some other
> way to coerce a := expression into having a guaranteed 64-bit native
> type.
>
>> ...
>> }
>>
>> This keeps the "count" cast safe, and it also forces -- I think? -- "i" to
>> have type "uint64", by casting "0" to "uint64" in the ":=" operation.
>>
>> Anyway, I was super careful about those nuances when I wrote my proposal, so
>> I'd like to stick with them. I suggest that:
>>
>> - we not call this patch a "revert", just update the code incrementally;
>> perhaps reference commit 6725fa0e129f in the commit message
>>
>> - we stick with the exact code I proposed (unless there are specific
>> problems with it), applying whitespace updates on top that are now required
>> by gofmt.
>
> Okay, I'll need to rethink how to do this patch (perhaps by
> rearranging it to occur after 7/7, by folding it into the rest of the
> 64-bit series where I add the nbd_extent list conversion code).
>
Thanks!
Laszlo
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
