qemu-block
[Top][All Lists]
Advanced

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

Re: [Qemu-block] [PATCH 1/9] nbd/server: add nbd_opt_invalid helper


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [Qemu-block] [PATCH 1/9] nbd/server: add nbd_opt_invalid helper
Date: Fri, 2 Mar 2018 15:40:09 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0

16.02.2018 01:01, Eric Blake wrote:
On 02/15/2018 07:51 AM, Vladimir Sementsov-Ogievskiy wrote:
NBD_REP_ERR_INVALID is often parameter to nbd_opt_drop and it would
be used more in following patches. So, let's add a helper.

Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
---
  nbd/server.c | 50 ++++++++++++++++++++++++++++++++++++--------------
  1 file changed, 36 insertions(+), 14 deletions(-)

More than twice the lines added compared to what was removed, so it's a tough call whether this refactoring makes a common pattern easier or just adds burden in tracing what gets executed, just to remove a parameter.  I'm not opposed to the patch, but want to see how it helps the rest of the series.

At any rate, the conversion itself is done correctly, so if we keep the patch, it has earned:

Reviewed-by: Eric Blake <address@hidden>


  +static int GCC_FMT_ATTR(4, 5)
+nbd_opt_drop(NBDClient *client, uint32_t type, Error **errp,
+             const char *fmt, ...)
+{
+    int ret;
+    va_list va;
+
+    va_start(va, fmt);
+    ret = nbd_opt_vdrop(client, type, errp, fmt, va);
+    va_end(va);
+
+    return ret;
+}
+
+static int GCC_FMT_ATTR(3, 4)
+nbd_opt_invalid(NBDClient *client, Error **errp, const char *fmt, ...)

No documentation comments?

the function is obvious and is near to nbd_opt_vdrop.. do we really want a comment? And, then, what about seprate comment for nbd_opt_drop?

--
Best regards,
Vladimir




reply via email to

[Prev in Thread] Current Thread [Next in Thread]