On 10/18/19 11:07 AM, Vladimir Sementsov-Ogievskiy wrote:
static int nbd_co_send_extents(NBDClient *client, uint64_t handle, - NBDExtent *extents, unsigned int nb_extents, - uint64_t length, bool last, - uint32_t context_id, Error **errp) + NBDExtentArray *ea, + bool last, uint32_t context_id, Error **errp) { NBDStructuredMeta chunk; - + size_t len = ea->count * sizeof(ea->extents[0]); + g_autofree NBDExtent *extents = g_memdup(ea->extents, len);Why do we need memdup here? What's wrong with modifying ea->extents in place?...To not make ea to be IN-OUT parameter.. I don't like functions with side effects. It will break the code if at some point we call nbd_co_send_extents twice on same ea object. What is the true way? To not memdup, nbd_co_send_extents should consume the whole ea object.. Seems, g_autoptr attribute can't be used for function parameter, gcc complains: nbd/server.c:1983:32: error: ‘cleanup’ attribute ignored [-Werror=attributes] 1983 | g_autoptr(NBDExtentArray) ea, | ^~~~~~~~~ so, is it better to call nbd_co_send_external(... g_steal_pointer(&ea) ...) and than in nbd_co_send_external do g_autoptr(NBDExtentArray) local_ea = ea; NBDExtent *extents = local_ea->extents; ?
No, that makes it worse. It's that much more confusing to track who is allocating what and where it gets cleaned up.
I personally don't see the need to avoid jumping through hoops to avoid an in-out parameter (if we're going to rework code later, we'll notice that we documented how things are supposed to be used), but if in-out parameters bother you, then the approach you used, even with an extra memdup(), is the simplest way to maintain, even if it is not the most efficient.
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
