Libguestfs - Feb 2021 - [PATCH libnbd] copy: Refactor ‘struct rw’.

On Mon, Feb 22, 2021 at 07:06:39PM +0200, Nir Soffer
wrote:
> On Mon, Feb 22, 2021 at 5:42 PM Richard W.M. Jones <rjones at
redhat.com> wrote:
> >
> > Make this a (fairly) abstract structure.  At least hide the subtype
> > fields from the main program.  This change is pure refactoring and
> > doesn?t change the semantics.
> 
> Nicer this way, although a little less type safe.
> > +  return (struct rw *) rwf;
> 
> We can avoid the cast here by returning &(rwf->rw)
I'll make that change (and elsewhere too).
> > -static int
> > -open_local (const char *prog,
> > -            const char *filename, bool writing, struct rw *rw)
> > +static struct rw *
> > +open_local (const char *filename, bool writing)
> >  {
> >    int flags, fd;
> > +  struct stat stat;
> >
> >    if (strcmp (filename, "-") == 0) {
> >      synchronous = true;
> >      fd = writing ? STDOUT_FILENO : STDIN_FILENO;
> >      if (writing && isatty (fd)) {
> > -      fprintf (stderr, "%s: refusing to write to tty\n",
prog);
> > +      fprintf (stderr, "%s: refusing to write to tty\n",
"nbdcopy");
> 
> Is it intended to replace prog with "nbdcopy"?
The change is a bit messed up - I think I may have moved this code
around twice.  But even better would be to use a getprogname wrapper
(a la gnulib).
> I looked at it briefly, this is a large change, but generally it
> looks good.
OK to push this (with fixes)?  I sent you privately your patch from
the weekend rebased on top of this one.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

On Mon, Feb 22, 2021 at 7:25 PM Richard W.M. Jones <rjones at redhat.com>
wrote:
>
> On Mon, Feb 22, 2021 at 07:06:39PM +0200, Nir Soffer wrote:
> > On Mon, Feb 22, 2021 at 5:42 PM Richard W.M. Jones <rjones at
redhat.com> wrote:
> > >
> > > Make this a (fairly) abstract structure.  At least hide the
subtype
> > > fields from the main program.  This change is pure refactoring
and
> > > doesn?t change the semantics.
> >
> > Nicer this way, although a little less type safe.
>
> > > +  return (struct rw *) rwf;
> >
> > We can avoid the cast here by returning &(rwf->rw)
>
> I'll make that change (and elsewhere too).
>
> > > -static int
> > > -open_local (const char *prog,
> > > -            const char *filename, bool writing, struct rw *rw)
> > > +static struct rw *
> > > +open_local (const char *filename, bool writing)
> > >  {
> > >    int flags, fd;
> > > +  struct stat stat;
> > >
> > >    if (strcmp (filename, "-") == 0) {
> > >      synchronous = true;
> > >      fd = writing ? STDOUT_FILENO : STDIN_FILENO;
> > >      if (writing && isatty (fd)) {
> > > -      fprintf (stderr, "%s: refusing to write to
tty\n", prog);
> > > +      fprintf (stderr, "%s: refusing to write to
tty\n", "nbdcopy");
> >
> > Is it intended to replace prog with "nbdcopy"?
>
> The change is a bit messed up - I think I may have moved this code
> around twice.  But even better would be to use a getprogname wrapper
> (a la gnulib).
>
> > I looked at it briefly, this is a large change, but generally it
> > looks good.
>
> OK to push this (with fixes)?
Looks safe enough to me.
> I sent you privately your patch from
> the weekend rebased on top of this one.
Thanks, will test again.