[stgt] [PATCH 3/6] mgmt and concat_buf: using concat_buf in mgmt.c

[FUJITA Tomonori]

On Fri, 13 Jan 2012 14:59:04 +0000
Alexander Nezhinsky <alexandern at mellanox.com> wrote:

> >> > Can we kill rsp_buf? That is, we can simply write the response to
> >> > *FILE?
> 
> >> What do you mean? To redirect the FILE* directly to the socket, somehow?
> 
> >> My implementation separated concat_buf and its use, so that it
> >> is employed for response in tgtd and for request in tgtadm.
> >> It leaves the resulting string in the user-supplied buffer pointer,
> >> and the buffer can be handled and used as required by a specific
> >> user context. Do you see anything wrong with this approach?
> 
> > I thought about more cleanup like adding something like FILE *rsp_fp.
> > Then "show" functions like tgt_target_show_all just call
> > fprintf. tgtd reads fp and write the data to ipc socket. I don't think
> > "show" functions need to handle buffers directly.
> 
> mtask->resp_buf and mtask->resp_bsize are just holders. 
> We will need some holders anyway, as the file will be opened under 
> OP_SHOW case and the like (it is unnecessary in other cases).
> open_memstream() takes the buffer and size pointer arguments, 
> so they should be provided there, when the file opens. 
> Moreover, they should be stored in a global context,
> of which mtask is the only relevant one, as we need the values 
> to write to the socket. 
> So we end up with essentially the same scheme.   

We need to hold the size? The buffer dynamically grows. Why does the
size matter? The buffer is necessary since we call free in the end.

# if we need to know the size of the buffer for some reasons, ftell
# can be used.


> For example, we declare FILE *fp in "struct mtask". It is initialized to NULL.
> Then in functions like mgmt.c::*_mgmt(), under OP_SHOW case
> we can call open_memstream() directly, mtask->fp gets a value and 
> then in mtask_handler() , under MTASK_STATE_RSP_SEND case
> we can uniformly check if mtask->fp is not NULL, to close it and send 
> the resulting buffers.
> The problem is that we have to  pass open_memstream() those pointers,
> which will get the values only under  MTASK_STATE_RSP_SEND.

I think that the new buffer structure can be embedded in mtask.

> Perhaps we can make concat_buf a member of mtask and store the pointers
> in concat_buf, so that the calls for OP_SHOW will be: 
> concat_buf_init(&mtask->concat_buf) 

Sounds good.

>     (internally passing &mtask->concat_buf.ptr, &mtask->concat_buf.size 
>       to open_memstream) and

I'm not sure this part. Why passing the buf and size?
MTASK_STATE_RSP_SEND doesn't need either.


> concat_buf_finish(&mtask->concat_buf)
> 
> and under  MTASK_STATE_RSP_SEND case:
> if (mtask->concat_buf.size)
>    write(socket_fd, mtask->concat_buf.ptr, mtask->concat_buf.size)

Would it better to hide the size (the detail of the buffer) from
mgmt.c? I mean that MTASK_STATE_RSP_SEND can use fp?

while (fgets(buf, sizeof(buf), fp))
      write(socket_fd, buf, sizeof(buf));

# can we use splice system call here instead?

--
To unsubscribe from this list: send the line "unsubscribe stgt" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html