On Wed, 11 Jan 2012 15:29:56 +0200
Alexander Nezhinsky <alexandern@xxxxxxxxxxxx> wrote:
> Using concat_buf in mgmt.c.
> The changes influence mainly the SHOW commands (those producing some
> output). Upon concat_buf finalization the dynamically alloctaed buffer
> is stored directly to the mtask response descriptor. No need to worry
> about memory re-allocatin anymore, it is taken care of by concat_buf
> mechanism.
>
> Using concat_buf api highlights a problem pre-existing in the mgmt code:
> a mix of the regular linux error codes (like ENOMEM) and enum tgtadm_errno
> (like TGTADM_NOMEM). Sometimes positive values returned correspond to
> tgtadm_errno, sometimes to the number of bytes written.
> A new function errno2tgtadm() converts regular errno to tgtadm_errno.
> A separate patch will address this problem in a more fundamental manner.
>
> Signed-off-by: Alexander Nezhinsky <alexandern@xxxxxxxxxxxx>
> ---
> usr/mgmt.c | 216 +++++++++++++++++++++++++++++-------------------------------
> 1 files changed, 105 insertions(+), 111 deletions(-)
>
> diff --git a/usr/mgmt.c b/usr/mgmt.c
> index 4397850..819afe2 100644
> --- a/usr/mgmt.c
> +++ b/usr/mgmt.c
> @@ -68,21 +68,33 @@ char mgmt_path[256];
> static struct mgmt_task *mtask_alloc(void);
> static void mtask_free(struct mgmt_task *mtask);
>
> -static void set_show_results(struct tgtadm_rsp *rsp, int *err)
> +static void set_mtask_result(struct mgmt_task *mtask, int err)
> {
> - if (*err < 0)
> - rsp->err = -*err;
> - else {
> - rsp->err = 0;
> - rsp->len = *err + sizeof(*rsp);
> - *err = 0;
> + mtask->rsp.len = sizeof(mtask->rsp);
> + if (!err) {
> + mtask->rsp.err = 0;
> + mtask->rsp.len += mtask->rsp_bsize;
> }
> + else if (err < 0)
> + mtask->rsp.err = -err;
> + else
> + mtask->rsp.err = err;
> +}
> +
> +static enum tgtadm_errno errno2tgtadm(int err)
> +{
> + if (err >= 0)
> + return TGTADM_SUCCESS;
> + else if (err == -ENOMEM)
> + return TGTADM_NOMEM;
> + else
> + return TGTADM_UNKNOWN_ERR;
> }
>
> static int target_mgmt(int lld_no, struct mgmt_task *mtask)
> {
> struct tgtadm_req *req = &mtask->req;
> - struct tgtadm_rsp *rsp = &mtask->rsp;
> + struct concat_buf b;
> int err = TGTADM_INVALID_REQUEST;
>
> switch (req->op) {
> @@ -144,58 +156,52 @@ static int target_mgmt(int lld_no, struct mgmt_task *mtask)
> break;
> }
> case OP_SHOW:
> - if (req->tid < 0) {
> - retry:
> - err = tgt_target_show_all(mtask->rsp_buf, mtask->rsp_bsize);
> - if (err == mtask->rsp_bsize) {
> - char *p;
> - mtask->rsp_bsize <<= 1;
> - p = realloc(mtask->rsp_buf, mtask->rsp_bsize);
> - if (p) {
> - mtask->rsp_buf = p;
> - goto retry;
> - } else {
> - eprintf("out of memory\n");
> - err = TGTADM_NOMEM;
> - }
> - }
> - } else if (tgt_drivers[lld_no]->show)
> + {
> + concat_buf_init(&b, &mtask->rsp_buf, &mtask->rsp_bsize);
Can we kill rsp_buf? That is, we can simply write the response to
*FILE?
> + if (req->tid < 0)
> + err = tgt_target_show_all(&b);
> + else if (tgt_drivers[lld_no]->show)
> err = tgt_drivers[lld_no]->show(req->mode,
> req->tid,
> req->sid,
> req->cid, req->lun,
> - mtask->rsp_buf, mtask->rsp_bsize);
> + &b);
> + if (!err) {
> + err = concat_buf_finish(&b);
> + err = errno2tgtadm(err);
> + }
> + else
> + concat_buf_finish(&b);
> break;
> + }
> default:
> break;
> }
>
> - if (req->op == OP_SHOW)
> - set_show_results(rsp, &err);
> - else {
> - rsp->err = err;
> - rsp->len = sizeof(*rsp);
> - }
> + set_mtask_result(mtask, err);
> return err;
> }
>
> -static int portal_mgmt(int lld_no, struct mgmt_task *mtask,
> - struct tgtadm_req *req,
> - struct tgtadm_rsp *rsp)
> +static int portal_mgmt(int lld_no, struct mgmt_task *mtask)
> {
> + struct tgtadm_req *req = &mtask->req;
> + struct concat_buf b;
> int err = TGTADM_INVALID_REQUEST;
>
> switch (req->op) {
> case OP_SHOW:
> if (tgt_drivers[lld_no]->show) {
> + concat_buf_init(&b, &mtask->rsp_buf, &mtask->rsp_bsize);
> err = tgt_drivers[lld_no]->show(req->mode,
> req->tid, req->sid,
> req->cid, req->lun,
> - mtask->rsp_buf,
> - mtask->rsp_bsize);
> -
> - set_show_results(rsp, &err);
> - return err;
> + &b);
> + if (!err) {
> + err = concat_buf_finish(&b);
> + err = errno2tgtadm(err);
> + }
> + else
> + concat_buf_finish(&b);
> }
> break;
> case OP_NEW:
> @@ -208,19 +214,19 @@ static int portal_mgmt(int lld_no, struct mgmt_task *mtask,
> break;
> }
>
> - rsp->err = err;
> - rsp->len = sizeof(*rsp);
> -
> + set_mtask_result(mtask, err);
> return err;
> }
>
> -static int device_mgmt(int lld_no, struct tgtadm_req *req, char *params,
> - struct tgtadm_rsp *rsp, int *rlen)
> +static int device_mgmt(int lld_no, struct mgmt_task *mtask)
> {
> + struct tgtadm_req *req = &mtask->req;
> + char *params = mtask->req_buf;
> int err = TGTADM_UNSUPPORTED_OPERATION;
>
> switch (req->op) {
> case OP_NEW:
> + eprintf("sz:%d params:%s\n",mtask->req_bsize,params);
> err = tgt_device_create(req->tid, req->device_type, req->lun,
> params, 1);
> break;
> @@ -234,18 +240,16 @@ static int device_mgmt(int lld_no, struct tgtadm_req *req, char *params,
> break;
> }
>
> - rsp->err = err;
> - rsp->len = sizeof(*rsp);
> -
> + set_mtask_result(mtask, err);
> return err;
> }
>
> static int account_mgmt(int lld_no, struct mgmt_task *mtask)
> {
> struct tgtadm_req *req = &mtask->req;
> - struct tgtadm_rsp *rsp = &mtask->rsp;
> - int err = TGTADM_UNSUPPORTED_OPERATION;
> char *user, *password;
> + struct concat_buf b;
> + int err = TGTADM_UNSUPPORTED_OPERATION;
>
> switch (req->op) {
> case OP_NEW:
> @@ -275,36 +279,27 @@ static int account_mgmt(int lld_no, struct mgmt_task *mtask)
> }
> break;
> case OP_SHOW:
> - retry:
> - err = account_show(mtask->rsp_buf, mtask->rsp_bsize);
> - if (err == mtask->rsp_bsize) {
> - char *p;
> - mtask->rsp_bsize <<= 1;
> - p = realloc(mtask->rsp_buf, mtask->rsp_bsize);
> - if (p) {
> - mtask->rsp_buf = p;
> - goto retry;
> - } else
> - err = TGTADM_NOMEM;
> + concat_buf_init(&b, &mtask->rsp_buf, &mtask->rsp_bsize);
> + err = account_show(&b);
> + if (!err) {
> + err = concat_buf_finish(&b);
> + err = errno2tgtadm(err);
> }
> + else
> + concat_buf_finish(&b);
> break;
> default:
> break;
> }
> out:
> - if (req->op == OP_SHOW)
> - set_show_results(rsp, &err);
> - else {
> - rsp->err = err;
> - rsp->len = sizeof(*rsp);
> - }
> + set_mtask_result(mtask, err);
> return err;
> }
>
> static int sys_mgmt(int lld_no, struct mgmt_task *mtask)
> {
> struct tgtadm_req *req = &mtask->req;
> - struct tgtadm_rsp *rsp = &mtask->rsp;
> + struct concat_buf b;
> int err = TGTADM_INVALID_REQUEST;
>
> switch (req->op) {
> @@ -325,50 +320,56 @@ static int sys_mgmt(int lld_no, struct mgmt_task *mtask)
> req->sid, req->lun,
> req->cid, mtask->req_buf);
>
> - rsp->err = err;
> - rsp->len = sizeof(*rsp);
> break;
> case OP_SHOW:
> - err = system_show(req->mode, mtask->rsp_buf, mtask->rsp_bsize);
> - if (err >= 0 && tgt_drivers[lld_no]->show) {
> - err += tgt_drivers[lld_no]->show(req->mode,
> - req->tid, req->sid,
> - req->cid, req->lun,
> - mtask->rsp_buf + err,
> - mtask->rsp_bsize - err);
> + concat_buf_init(&b, &mtask->rsp_buf, &mtask->rsp_bsize);
> + err = system_show(req->mode, &b);
> + if (!err && tgt_drivers[lld_no]->show) {
> + err = tgt_drivers[lld_no]->show(req->mode,
> + req->tid, req->sid,
> + req->cid, req->lun,
> + &b);
> }
> - set_show_results(rsp, &err);
> + if (!err) {
> + err = concat_buf_finish(&b);
> + err = errno2tgtadm(err);
> + }
> + else
> + concat_buf_finish(&b);
> break;
> case OP_DELETE:
> if (is_system_inactive())
> err = 0;
> -
> - rsp->err = err;
> - rsp->len = sizeof(*rsp);
> break;
> default:
> break;
> }
>
> + set_mtask_result(mtask, err);
> return err;
> }
>
> -static int connection_mgmt(int lld_no, struct mgmt_task *mtask,
> - struct tgtadm_req *req,
> - struct tgtadm_rsp *rsp)
> +static int connection_mgmt(int lld_no, struct mgmt_task *mtask)
> {
> + struct tgtadm_req *req = &mtask->req;
> + struct concat_buf b;
> int err = TGTADM_INVALID_REQUEST;
>
> switch (req->op) {
> case OP_SHOW:
> if (tgt_drivers[lld_no]->show) {
> + concat_buf_init(&b, &mtask->rsp_buf, &mtask->rsp_bsize);
> err = tgt_drivers[lld_no]->show(req->mode,
> req->tid, req->sid,
> req->cid, req->lun,
> - mtask->rsp_buf,
> - mtask->rsp_bsize);
> - set_show_results(rsp, &err);
> - return err;
> + &b);
> + if (!err) {
> + err = concat_buf_finish(&b);
> + err = errno2tgtadm(err);
> + }
> + else
> + concat_buf_finish(&b);
> + break;
> }
> break;
> default:
> @@ -377,20 +378,19 @@ static int connection_mgmt(int lld_no, struct mgmt_task *mtask,
> req->tid,
> req->sid, req->lun,
> req->cid, mtask->req_buf);
> - rsp->err = err;
> - rsp->len = sizeof(*rsp);
> break;
> }
>
> + set_mtask_result(mtask, err);
> return err;
> }
>
> static int mtask_execute(struct mgmt_task *mtask)
> {
> struct tgtadm_req *req = &mtask->req;
> - struct tgtadm_rsp *rsp = &mtask->rsp;
> - int lld_no, err = TGTADM_INVALID_REQUEST;
> - int len;
> + struct concat_buf b;
> + int lld_no;
> + int err = TGTADM_INVALID_REQUEST;
>
> if (!strlen(req->lld))
> lld_no = 0;
> @@ -402,8 +402,7 @@ static int mtask_execute(struct mgmt_task *mtask)
> else
> eprintf("driver %s is in state: %s\n",
> req->lld, driver_state_name(tgt_drivers[lld_no]));
> - rsp->err = TGTADM_NO_DRIVER;
> - rsp->len = sizeof(*rsp);
> + set_mtask_result(mtask, TGTADM_NO_DRIVER);
> return 0;
> }
> }
> @@ -420,30 +419,32 @@ static int mtask_execute(struct mgmt_task *mtask)
> err = target_mgmt(lld_no, mtask);
> break;
> case MODE_PORTAL:
> - err = portal_mgmt(lld_no, mtask, req, rsp);
> + err = portal_mgmt(lld_no, mtask);
> break;
> case MODE_DEVICE:
> - err = device_mgmt(lld_no, req, mtask->req_buf, rsp, &len);
> + err = device_mgmt(lld_no, mtask);
> break;
> case MODE_ACCOUNT:
> err = account_mgmt(lld_no, mtask);
> break;
> case MODE_CONNECTION:
> - err = connection_mgmt(lld_no, mtask, req, rsp);
> + err = connection_mgmt(lld_no, mtask);
> break;
> default:
> if (req->op == OP_SHOW && tgt_drivers[lld_no]->show) {
> + concat_buf_init(&b, &mtask->req_buf, &mtask->req_bsize);
> err = tgt_drivers[lld_no]->show(req->mode,
> req->tid, req->sid,
> req->cid, req->lun,
> - mtask->rsp_buf,
> - mtask->rsp_bsize);
> -
> - set_show_results(rsp, &err);
> - } else {
> - rsp->err = err;
> - rsp->len = sizeof(*rsp);
> + &b);
> + if (!err) {
> + err = concat_buf_finish(&b);
> + err = errno2tgtadm(err);
> + }
> + else
> + concat_buf_finish(&b);
> }
> + set_mtask_result(mtask, err);
> break;
> }
>
> @@ -512,13 +513,6 @@ static int mtask_received(struct mgmt_task *mtask, int fd)
> {
> int err;
>
> - mtask->rsp_buf = zalloc(MAX_MGT_BUFSIZE);
> - if (!mtask->rsp_buf) {
> - eprintf("failed to allocate response data buffer\n");
> - return -ENOMEM;
> - }
> - mtask->rsp_bsize = MAX_MGT_BUFSIZE;
> -
> err = mtask_execute(mtask);
> if (err) {
> eprintf("mgmt task processing failed\n");
> --
> 1.7.3.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe stgt" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe stgt" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
