Re: [PATCH 1/2] vchan-socket-proxy: add reconnect marker support

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Wed, Dec 07, 2022 at 03:22:46PM +0000, Anthony PERARD wrote:
> On Mon, Sep 05, 2022 at 03:50:18PM +0200, Marek Marczykowski-Górecki wrote:
> > +                reconnect_marker_value = atoi(optarg);
> 
> atoi() isn't great, if there's garbage it just return 0, which is
> validated below.
> 
> Would there be value here in using strtol() which can detect input
> error? To at least help with maybe hard to debug issues?

I'll send v2 with this changed.

> > +                if (reconnect_marker_value < 0 || reconnect_marker_value > 
> > 255) {
> > +                    fprintf(stderr, "invalid argument for 
> > --reconnect-marker, "
> > +                                    "must be a number between 0 and 
> > 255\n");
> > +                    usage(argv);
> > +                }
> > +                break;
> >              case '?':
> >                  usage(argv);
> >          }
> > @@ -509,6 +549,15 @@ int main(int argc, char **argv)
> >                  ret = 1;
> >                  break;
> >              }
> > +            if (reconnect_marker_value != -1) {
> > +                const char marker_buf[] = { reconnect_marker_value };
> > +
> > +                if (libxenvchan_write(state.ctrl, marker_buf, 
> > sizeof(marker_buf))
> > +                        != sizeof(marker_buf)) {
> > +                    fprintf(stderr, "failed to send reconnect marker\n");
> 
> Is this an expected failure? If so, maybe adding "(ignored)" might be
> valuable to someone reading the logs?

No, it isn't, it may mean server is gone or other fatal communication
error. The break below will terminate the process (after performing
cleanup).

> > +                    break;
> > +                }
> > +            }
> >              if (data_loop(&state) != 0)
> >                  break;
> >              /* don't reconnect if output was stdout */
> 
> 
> Otherwise, the patch looks fine. Even if kept as is:
> Acked-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> 
> Thanks,
> 
> -- 
> Anthony PERARD

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature