On Wed, Feb 24, 2021 at 08:03:21PM -0500, Ashton Fagg wrote:
> Ashton Fagg <ash...@fagg.id.au> writes:
>
> > Attached is my first attempt at a "proper" solution. I haven't tested it
> > beyond building as I can't take my machine down for testing right now...
> >
> > But, I'd appreciate a sanity check that I'm on the right track.
>
> Sorry for the noise. I think my mail client mangled the attachment, so
> trying again.
>
> diff --git a/usr.sbin/iscsictl/iscsictl.c b/usr.sbin/iscsictl/iscsictl.c
> index 77f9c74abde..5fc326acca6 100644
> --- a/usr.sbin/iscsictl/iscsictl.c
> +++ b/usr.sbin/iscsictl/iscsictl.c
> @@ -40,6 +40,8 @@ struct pdu *ctl_getpdu(char *, size_t);
> int ctl_sendpdu(int, struct pdu *);
> void show_config(struct ctrlmsghdr *, struct pdu *);
> void show_vscsi_stats(struct ctrlmsghdr *, struct pdu *);
> +void poll_and_wait(void);
> +void register_poll(struct ctrlmsghdr *, struct pdu *);
>
> char cbuf[CONTROL_READ_SIZE];
>
> @@ -48,6 +50,14 @@ struct control {
> int fd;
> } control;
>
> +struct poll_result {
> + u_int8_t result;
> + int attempts;
> +} pr;
> +
> +#define POLL_DELAY 1
> +#define POLL_ATTEMPTS 10
> +
> __dead void
> usage(void)
> {
> @@ -68,7 +78,7 @@ main (int argc, char* argv[])
> char *sockname = ISCSID_CONTROL;
> struct session_ctlcfg *s;
> struct iscsi_config *cf;
> - int ch, val = 0;
> + int ch, poll = 0, val = 0;
>
> /* check flags */
> while ((ch = getopt(argc, argv, "f:s:")) != -1) {
> @@ -135,6 +145,9 @@ main (int argc, char* argv[])
> &cf->initiator, sizeof(cf->initiator)) == -1)
> err(1, "control_compose");
> }
> +
> + /* Reloading, so poll afterwards. */
> + poll = 1;
> SIMPLEQ_FOREACH(s, &cf->sessions, entry) {
> struct ctrldata cdv[3];
> bzero(cdv, sizeof(cdv));
> @@ -174,6 +187,12 @@ main (int argc, char* argv[])
>
> run();
>
> + /* If we've reloaded, we probably should wait in case any new
> connections
> + need to come up (or fail). */
> + if (poll) {
> + poll_and_wait();
> + }
> +
> close(control.fd);
>
> return (0);
> @@ -229,6 +248,10 @@ run_command(struct pdu *pdu)
> case CTRL_INPROGRESS:
> printf("command in progress...\n");
> break;
> + case CTRL_SESS_POLL:
> + done = 1;
> + register_poll(cmh, pdu);
> + break;
> case CTRL_INITIATOR_CONFIG:
> case CTRL_SESSION_CONFIG:
> show_config(cmh, pdu);
> @@ -383,3 +406,43 @@ show_vscsi_stats(struct ctrlmsghdr *cmh, struct pdu *pdu)
> vs->cnt_t2i_status[1],
> vs->cnt_t2i_status[2]);
> }
> +
> +void
> +poll_and_wait(void)
> +{
> + pr.result = (u_int8_t) 0;
> + printf("polling...");
> +
> + for (pr.attempts = 1; pr.attempts <= POLL_ATTEMPTS; ++pr.attempts) {
> + if (control_compose(NULL, CTRL_SESS_POLL, NULL, 0) == -1)
> + err(1, "control_compose");
> +
> + struct pdu *pdu;
> + while ((pdu = TAILQ_FIRST(&control.channel)) != NULL) {
> + run_command(pdu);
> + }
> +
> + /* Poll says we are good to go. */
> + if (pr.result != 0) {
> + printf("ok!\n");
> + return;
> + }
> +
> + /* Poll says we should wait... */
> + printf("%d, ", pr.attempts);
> + sleep(POLL_DELAY);
> + }
> +
> + printf("..,timer exceeded.\n");
> +}
> +
> +void
> +register_poll(struct ctrlmsghdr *cmh, struct pdu *pdu)
> +{
> + u_int8_t *r = &pr.result;
> +
> + if (cmh->len[0] != sizeof(u_int8_t))
> + errx(1, "poll: bad size of response");
> +
> + r = pdu_getbuf(pdu, NULL, 1);
> +}
> diff --git a/usr.sbin/iscsid/Makefile b/usr.sbin/iscsid/Makefile
> index 7a62024e68b..bac59d934f8 100644
> --- a/usr.sbin/iscsid/Makefile
> +++ b/usr.sbin/iscsid/Makefile
> @@ -2,7 +2,7 @@
>
> PROG= iscsid
> SRCS= connection.c control.c initiator.c iscsid.c log.c logmsg.c
> pdu.c \
> - session.c task.c util.c vscsi.c
> + poll.c session.c task.c util.c vscsi.c
>
> MAN= iscsid.8
>
> diff --git a/usr.sbin/iscsid/iscsid.c b/usr.sbin/iscsid/iscsid.c
> index d3526e96363..61f831c3b24 100644
> --- a/usr.sbin/iscsid/iscsid.c
> +++ b/usr.sbin/iscsid/iscsid.c
> @@ -211,6 +211,7 @@ iscsid_ctrl_dispatch(void *ch, struct pdu *pdu)
> struct initiator_config *ic;
> struct session_config *sc;
> struct session *s;
> + struct session_poll *p;
> int *valp;
>
> cmh = pdu_getbuf(pdu, NULL, 0);
> @@ -304,6 +305,20 @@ iscsid_ctrl_dispatch(void *ch, struct pdu *pdu)
>
> control_compose(ch, CTRL_SUCCESS, NULL, 0);
> break;
> + case CTRL_SESS_POLL:
> + p = poll_alloc();
> +
> + TAILQ_FOREACH(s, &initiator->sessions, entry) {
> + poll_session(p, s);
> + }
> +
> + poll_finalize(p);
> +
> + control_compose(ch, CTRL_SESS_POLL,
> + (void *) &(p->status), sizeof(u_int8_t));
No need to type-cast, no need for the () around &(p->status). I prefer to
use sizeof(p->status) since that will adapt properly.
> +
> + poll_dealloc(p);
I think you could just use a stack variable for p here instead of using
alloc and dealloc. The struct is small.
> + break;
> default:
> log_warnx("unknown control message type %d", cmh->type);
> control_compose(ch, CTRL_FAILURE, NULL, 0);
> diff --git a/usr.sbin/iscsid/iscsid.h b/usr.sbin/iscsid/iscsid.h
> index b43fb5dcd99..08d5f251a39 100644
> --- a/usr.sbin/iscsid/iscsid.h
> +++ b/usr.sbin/iscsid/iscsid.h
> @@ -67,7 +67,7 @@ struct ctrldata {
> #define CTRL_LOG_VERBOSE 6
> #define CTRL_VSCSI_STATS 7
> #define CTRL_SHOW_SUM 8
> -
> +#define CTRL_SESS_POLL 9
>
> TAILQ_HEAD(session_head, session);
> TAILQ_HEAD(connection_head, connection);
> @@ -251,6 +251,13 @@ struct session {
> int action;
> };
>
> +struct session_poll {
> + u_int16_t session_count; /* Total number of configured sessions*/
> + u_int16_t init_count; /* Number of sessions in init state */
> + u_int16_t running_count; /* Number of sessions in running state */
> + u_int8_t status; /* Status flag */
> +};
> +
I would just use int for all the variables here. Just keep it simple.
> struct connection {
> struct event ev;
> struct event wev;
> @@ -391,6 +398,12 @@ void vscsi_status(int, int, void *, size_t);
> void vscsi_event(unsigned long, u_int, u_int);
> struct vscsi_stats *vscsi_stats(void);
>
> +/* Session polling */
> +struct session_poll *poll_alloc(void);
> +void poll_dealloc(struct session_poll *);
> +void poll_session(struct session_poll *, struct session *);
> +void poll_finalize(struct session_poll *);
> +
> /* logmsg.c */
> void log_hexdump(void *, size_t);
> void log_pdu(struct pdu *, int);
> diff --git a/usr.sbin/iscsid/poll.c b/usr.sbin/iscsid/poll.c
> new file mode 100644
> index 00000000000..d3c79985d71
> --- /dev/null
> +++ b/usr.sbin/iscsid/poll.c
> @@ -0,0 +1,80 @@
> +/*
> + * Copyright (c) 2021 Dr Ashton Fagg <ash...@fagg.id.au>
> + *
> + * Permission to use, copy, modify, and distribute this software for any
> + * purpose with or without fee is hereby granted, provided that the above
> + * copyright notice and this permission notice appear in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +#include <sys/types.h>
> +#include <sys/socket.h>
> +
> +#include <event.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +#include "iscsid.h"
> +#include "log.h"
> +
> +struct session_poll
> +*poll_alloc(void)
> +{
> + struct session_poll *p;
> +
> + if (!(p = calloc(1, sizeof(*p))))
> + fatal("poll_alloc failed.");
> +
Calloc already sets all memory to 0. So the code below is not needed.
> + p->session_count = (u_int16_t) 0;
> + p->init_count = (u_int16_t) 0;
> + p->running_count = (u_int16_t) 0;
Also there is no need to typecast here. In general try to avoid typecasts
since they can hide hard to find bugs.
> +
> + return p;
> +}
> +
> +
> +void
> +poll_dealloc(struct session_poll *p)
> +{
> + free(p);
> +}
As mentioned above I would just use a stack variable
struct session_poll p = { 0 };
instead of dynamically allocate this.
> +void
> +poll_session(struct session_poll *p, struct session *s)
> +{
> + if (!s)
> + fatal("poll_session failed: invalid session");
> +
> + ++(p->session_count);
No need for the ()
> +
> + /* If SESS_RUNNING, this determines the session has either
> + been brought up successfully, or has failed. Either way,
> + we aren't waiting on it. */
> + if (s->state & SESS_RUNNING)
> + ++(p->running_count);
> + /* Otherwise, it is in SESS_INIT. These need to be waited on. */
> + else if (s->state & SESS_RUNNING)
> + ++(p->init_count);
The two if statements are the same. I guess you want SESS_INIT in the
second.
> + else
> + fatal("poll_session: unknown state.");
> +}
> +
> +void
> +poll_finalize(struct session_poll *p)
> +{
> + /* Perform final book keeping to determine status.
> + *
> + * status will be non-zero if the number of sessions that are
> + * in states other than SESS_INIT is equal to the total number
> + * of configured sessions. This indicates to the poller that
> + * we are not waiting for something to complete. */
Indents are strange here. Guess tab vs spaces.
> + p->status = (u_int8_t) (p->session_count == p->running_count);
> +}
I actually wonder if this code should just return the various counts to
iscsictl so it could display the status (if requested).
I think this is moving in the right direction.
--
:wq Claudio
