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

[Vladimir Sementsov-Ogievskiy]

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



  +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

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

[Eric Blake]

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


  
+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?

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

[Vladimir Sementsov-Ogievskiy]

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 
---
 nbd/server.c | 50 --
 1 file changed, 36 insertions(+), 14 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 112e3f69df..b9860a6dcf 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -218,22 +218,46 @@ nbd_negotiate_send_rep_err(NBDClient *client, uint32_t 
type,
 /* Drop remainder of the current option, and send a reply with the
  * given error type and message. Return -errno on read or write
  * failure; or 0 if connection is still live. */
-static int GCC_FMT_ATTR(4, 5)
-nbd_opt_drop(NBDClient *client, uint32_t type, Error **errp,
- const char *fmt, ...)
+static int GCC_FMT_ATTR(4, 0)
+nbd_opt_vdrop(NBDClient *client, uint32_t type, Error **errp,
+  const char *fmt, va_list va)
 {
 int ret = nbd_drop(client->ioc, client->optlen, errp);
-va_list va;
 
 client->optlen = 0;
 if (!ret) {
-va_start(va, fmt);
 ret = nbd_negotiate_send_rep_verr(client, type, errp, fmt, va);
-va_end(va);
 }
 return ret;
 }
 
+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, ...)
+{
+int ret;
+va_list va;
+
+va_start(va, fmt);
+ret = nbd_opt_vdrop(client, NBD_REP_ERR_INVALID, errp, fmt, va);
+va_end(va);
+
+return ret;
+}
+
 /* Read size bytes from the unparsed payload of the current option.
  * Return -errno on I/O error, 0 if option was completely handled by
  * sending a reply about inconsistent lengths, or 1 on success. */
@@ -241,9 +265,9 @@ static int nbd_opt_read(NBDClient *client, void *buffer, 
size_t size,
 Error **errp)
 {
 if (size > client->optlen) {
-return nbd_opt_drop(client, NBD_REP_ERR_INVALID, errp,
-"Inconsistent lengths in option %s",
-nbd_opt_lookup(client->opt));
+return nbd_opt_invalid(client, errp,
+   "Inconsistent lengths in option %s",
+   nbd_opt_lookup(client->opt));
 }
 client->optlen -= size;
 return qio_channel_read_all(client->ioc, buffer, size, errp) < 0 ? -EIO : 
1;
@@ -398,9 +422,8 @@ static int nbd_reject_length(NBDClient *client, bool fatal, 
Error **errp)
 int ret;
 
 assert(client->optlen);
-ret = nbd_opt_drop(client, NBD_REP_ERR_INVALID, errp,
-   "option '%s' has unexpected length",
-   nbd_opt_lookup(client->opt));
+ret = nbd_opt_invalid(client, errp, "option '%s' has unexpected length",
+  nbd_opt_lookup(client->opt));
 if (fatal && !ret) {
 error_setg(errp, "option '%s' has unexpected length",
nbd_opt_lookup(client->opt));
@@ -438,8 +461,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, 
uint16_t myflags,
 }
 be32_to_cpus();
 if (namelen >= sizeof(name)) {
-return nbd_opt_drop(client, NBD_REP_ERR_INVALID, errp,
-"name too long for qemu");
+return nbd_opt_invalid(client, errp, "name too long for qemu");
 }
 rc = nbd_opt_read(client, name, namelen, errp);
 if (rc <= 0) {
-- 
2.11.1